terrymanu commented on PR #39003:
URL: https://github.com/apache/shardingsphere/pull/39003#issuecomment-4885153034
### Summary
**Review Result: Mergeable**
Reason: Code-scope review only. The PR consistently refines the MCP resource
URI template API contract, propagates the public API rename across handlers,
descriptor indexes, tests, and docs, and adds focused coverage for resource
runtime-failure conversion. CI not reviewed by request.
### Evidence
- The API contract is clearer and internally consistent:
`MCPResourceHandler#getResourceUriTemplate()` documents fixed URIs as URI
templates without variables and keeps handler failures as runtime-converted
exceptions
(`mcp/api/src/main/java/org/apache/shardingsphere/mcp/api/resource/MCPResourceHandler.java:39`,
`mcp/api/src/main/java/org/apache/shardingsphere/mcp/api/resource/MCPResourceHandler.java:53`).
`MCPResponse` also documents successful-response ownership and runtime error
conversion
(`mcp/api/src/main/java/org/apache/shardingsphere/mcp/api/protocol/response/MCPResponse.java:27`).
- The descriptor/index path now keys resource descriptors consistently by
`getUriTemplate()` for both fixed resources and resource templates
(`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/descriptor/MCPDescriptorCatalogIndex.java:60`).
- Resource read failures are now converted at the transport boundary through
`MCPTransportErrorFactory`, matching the intended runtime-owned error surface
(`mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/resource/MCPResourceSpecificationFactory.java:114`).
- The new regression test covers a runtime failure from resource dispatch
and verifies it becomes an MCP transport error with `INTERNAL_ERROR`
(`mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/resource/MCPResourceSpecificationFactoryTest.java:103`).
- Developer documentation now describes the handler success/failure contract
and clarifies fixed URI versus URI template semantics
(`docs/document/content/dev-manual/mcp.en.md:70`,
`docs/document/content/dev-manual/mcp.en.md:77`).
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: PR #39003 latest head
`7e00d1f4844e21ff48e0057d3aa1447ea8441c92`, base
`bf9ea8826b62c45412b09c9f46e541a64f5448bd`, merge-base
`bf9ea8826b62c45412b09c9f46e541a64f5448bd`. Reviewed all 71 changed files under
MCP API/support/core/bootstrap/features, MCP tests, e2e MCP fixture, and
developer docs. Local triple-dot file list matched GitHub `/pulls/39003/files`.
- Not Reviewed Scope: GitHub Actions/check-runs, full MCP distribution
packaging, Docker/native image publishing, and runtime E2E startup were not
reviewed.
- Verification:
- `./mvnw -pl
mcp/api,mcp/support,mcp/core,mcp/bootstrap,mcp/features/broadcast,mcp/features/encrypt,mcp/features/mask,mcp/features/readwrite-splitting,mcp/features/shadow,mcp/features/sharding,test/e2e/mcp
-am -DskipITs -Dspotless.skip=true -Dcheckstyle.skip=true -DskipTests
test-compile`: exit 0.
- `./mvnw -pl mcp/api,mcp/support,mcp/core,mcp/bootstrap,test/e2e/mcp -am
-DskipITs -Dspotless.skip=true -Dcheckstyle.skip=true -DfailIfNoTests=false
-Dsurefire.failIfNoSpecifiedTests=false
-Dtest=MCPResourceDescriptorTest,MCPDescriptorCatalogIndexTest,MCPDescriptorCatalogLoaderTest,MCPProtocolDescriptorCatalogTest,ResourceDefinitionRegistryTest,CoreResourceHandlerSurfaceTest,MCPResourceSpecificationFactoryTest,PluginFixtureStatusResourceHandlerTest
test`: exit 0.
- `./mvnw spotless:check -Pcheck -T1C`: exit 0.
- `./mvnw checkstyle:check -Pcheck -T1C`: exit 0.
- Release Note / User Docs: Developer docs are updated. A release note is
not required for this code-scope review because the MCP module is not present
in the checked 5.5.3 release tag, and the PR updates the dev-facing MCP
documentation for the new contract.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
