terrymanu commented on PR #39035:
URL: https://github.com/apache/shardingsphere/pull/39035#issuecomment-4903112905
### Summary
**Review Result: Mergeable**
Reason: The reviewed PR scope is code-scope ready. The patch is small,
matches the MCP cleanup intent, preserves existing public behavior, and adds
focused coverage for the sharding request merge path. CI not reviewed by
request.
### Evidence
- The architecture boundary test now covers all current MCP feature package
roots, not only `encrypt` and `mask`:
`mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/MCPArchitectureBoundaryTest.java:39`.
The generic module scan still targets only `mcp/api`, `mcp/support`,
`mcp/core`, and `mcp/bootstrap` main sources, so it keeps the intended
shared-vs-feature dependency boundary.
- `ShardingWorkflowRequest.merge(...)` now copies generic `WorkflowRequest`
fields before overlaying sharding-specific current request fields:
`mcp/features/sharding/src/main/java/org/apache/shardingsphere/mcp/feature/sharding/tool/model/ShardingWorkflowRequest.java:75`.
This aligns sharding with the existing merge pattern used by other MCP feature
requests.
- The new regression test covers generic previous request fields (`planId`,
`database`, `table`, `operationType`) plus current sharding fields in the same
merge result:
`mcp/features/sharding/src/test/java/org/apache/shardingsphere/mcp/feature/sharding/tool/model/ShardingWorkflowRequestTest.java:63`.
- The descriptor swapper refactor keeps fixed-resource and resource-template
URI ownership explicit by passing either `uri` or `uriTemplate` into a private
helper, without changing descriptor payload shape:
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/descriptor/MCPDescriptorCatalogYamlSwapper.java:76`.
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: PR #39035, head
`5fee713dca22cb657a5e11ff29dd6f3bbec1b0e3`, base
`2c98ccedf8a9e5d022467bdc5c21c1ab214fdb3a`, merge-base
`2c98ccedf8a9e5d022467bdc5c21c1ab214fdb3a`. Reviewed files from GitHub
`/pulls/39035/files`: `MCPArchitectureBoundaryTest.java`,
`ShardingWorkflowRequest.java`, `ShardingWorkflowRequestTest.java`, and
`MCPDescriptorCatalogYamlSwapper.java`. Local diff file list matched GitHub
file list.
- Not Reviewed Scope: GitHub Actions/check-runs, release workflow, Docker
distribution, and MCP E2E runtime behavior were not reviewed.
- Verification:
- `./mvnw -pl mcp/bootstrap,mcp/support,mcp/features/sharding -DskipITs
-Dspotless.skip=true
-Dtest=MCPArchitectureBoundaryTest,ShardingWorkflowRequestTest,MCPDescriptorCatalogYamlSwapperTest
test -Dsurefire.failIfNoSpecifiedTests=false` exited 0.
- `./mvnw spotless:check -Pcheck -T1C` exited 0.
- `./mvnw checkstyle:check -Pcheck -T1C` exited 0.
- Release Note / User Docs: Not required. The PR changes internal MCP test
coverage, request merge behavior, and descriptor construction refactoring
without adding user-facing configuration, protocol fields, or documented MCP
tool/resource semantics.
--
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]
