terrymanu commented on PR #39054:
URL: https://github.com/apache/shardingsphere/pull/39054#issuecomment-4912776009
### Summary
**Review Result: Mergeable**
Reason: Code-scope review only. The latest PR head keeps the
workflow-planning refactor within the MCP support/sharding boundary, preserves
the existing `WorkflowPlanningSupport` behavior through focused delegate
classes, adds direct tests for the new public helpers, and passes focused local
verification. CI not reviewed by request.
### Evidence
- The PR splits the previous `WorkflowPlanningSupport` responsibilities
without changing the public support entry points: `WorkflowPlanningSupport` now
delegates planning-context checks and algorithm requirement collection through
`WorkflowPlanningContextValidator` and `WorkflowAlgorithmRequirementCollector`,
while keeping the same externally used methods in place
(`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowPlanningSupport.java`).
- The extracted context validator still covers the same planning path:
missing database, identifier validation, schema inference, table lookup, and
column lookup are preserved in
`WorkflowPlanningContextValidator.ensurePlanningContext`
(`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowPlanningContextValidator.java`).
- The extracted algorithm requirement collector preserves the prior behavior
for blocking algorithm issues, default property application, missing required
properties, and readiness gating
(`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowAlgorithmRequirementCollector.java`).
- The nested MCP input-schema validation is covered by a focused negative
test for unsupported nested fields, and the descriptor catalog tests still run
successfully
(`mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/descriptor/MCPDescriptorCatalogValidatorTest.java`).
- The new public helper classes have direct focused tests, and existing
wrapper-level `WorkflowPlanningSupportTest` still exercises the
production-facing support API. The added sharding kernel test covers the
missing-database clarification path and cleanup drop-only validation
(`mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowPlanningContextValidatorTest.java`,
`mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowAlgorithmRequirementCollectorTest.java`,
`mcp/features/sharding/src/test/java/org/apache/shardingsphere/mcp/feature/sharding/tool/service/ShardingWorkflowPlanningKernelTest.java`).
- No substantive unrelated changes were found in the GitHub changed-file
scope. No release note or user documentation is required because this is an
internal MCP workflow-planning boundary/refactor plus validation/test coverage
change, with no user-facing configuration, protocol, SQL syntax, migration, or
compatibility behavior change identified.
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: PR `apache/shardingsphere#39054`, latest head SHA
`3d180810e60fbf0b1004c2bf1deaec25eaeda402`, base/merge-base SHA
`2e4bc504ed792cee62ae56fbb511c11c5bbbf470`. GitHub `/pulls/39054/files` listed
8 files, and the local triple-dot file list matched GitHub.
- Reviewed Files:
-
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/descriptor/MCPToolDescriptorCatalogValidator.java`
-
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowAlgorithmRequirementCollector.java`
-
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowPlanningContextValidator.java`
-
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowPlanningSupport.java`
-
`mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/descriptor/MCPDescriptorCatalogValidatorTest.java`
-
`mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowAlgorithmRequirementCollectorTest.java`
-
`mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowPlanningContextValidatorTest.java`
-
`mcp/features/sharding/src/test/java/org/apache/shardingsphere/mcp/feature/sharding/tool/service/ShardingWorkflowPlanningKernelTest.java`
- Not Reviewed Scope: GitHub Actions/check-runs/logs were not reviewed by
request. Full MCP CI, production Proxy/container E2E, and unrelated local
untracked files were not reviewed.
- Verification:
- `./mvnw -pl mcp/support,mcp/features/sharding -am -DskipITs
-Dspotless.skip=true -Dsurefire.failIfNoSpecifiedTests=false -B -ntp test`
exited `0`; `mcp-support` reported 573 tests with 0 failures/errors/skips, and
`mcp-feature-sharding` reported 124 tests with 0 failures/errors/skips.
- `./mvnw -pl mcp/support,mcp/features/sharding -am -Pcheck -DskipTests
-DskipITs -Dspotless.skip=true -B -ntp checkstyle:check` exited `0`.
- `./mvnw -pl mcp/support,mcp/features/sharding -Pcheck -DskipTests
-DskipITs -B -ntp spotless:check` exited `0`.
- GitHub PR metadata, changed files, comments, and reviews were accessible
through the public GitHub REST API. No review comments or reviews were present
at the time of review. No inaccessible GitHub endpoint affects this
code-correctness result.
- Release Note / User Docs: Not required; the PR is internal MCP
workflow-planning refactor/validation/test work and does not introduce a
user-facing feature, configuration change, compatibility requirement, migration
path, or troubleshooting behavior requiring release documentation.
--
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]
