You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "terrymanu (via GitHub)" <gi...@apache.org> on 2026/06/29 18:22:32 UTC

[PR] Simplify MCP helper code [shardingsphere]

terrymanu opened a new pull request, #38960:
URL: https://github.com/apache/shardingsphere/pull/38960

   Refactor small duplicated MCP and MCP E2E helper flows without changing public behavior. Reuse shared payload field builders, remove redundant wrapper methods, simplify repeated provider collections, and keep existing descriptor, SQL classification, workflow, and model-facing formatter tests passing.
   
   For #35294.


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Simplify MCP helper code [shardingsphere]

Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
terrymanu commented on PR #38960:
URL: https://github.com/apache/shardingsphere/pull/38960#issuecomment-4835728693

   ### Summary
   
   **Review Result: Mergeable**
   
   Reason: This is code-scope approval only. The latest PR head keeps the MCP helper changes behavior-preserving, the changed paths have targeted test coverage, and CI was not reviewed by request.
   
   ### Evidence
   
   - The provider changes in `mcp/features/encrypt/src/main/java/org/apache/shardingsphere/mcp/feature/encrypt/EncryptMCPHandlerProvider.java` and `mcp/features/mask/src/main/java/org/apache/shardingsphere/mcp/feature/mask/MaskMCPHandlerProvider.java` only replace `Arrays.asList` / `Collections.singleton` with `List.of`; reviewed callers use the returned collections for iteration/stream registration, not mutation.
   - `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/descriptor/MCPDescriptorCatalogPayloadBuilder.java` preserves the resource payload field order and keeps the only semantic difference as `uri` versus `uriTemplate`.
   - `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/tool/handler/execute/SQLStatementTargetResolver.java` combines two identical `ON` source-collection branches. `isStatementObjectType(...)` returns on the first SQL object type keyword, so the combined `INDEX`, `TRIGGER`, and `POLICY` check is equivalent.
   - `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/programmatic/AbstractHttpProgrammaticRuntimeE2ETest.java` now delegates directly to helpers that already normalize JSON-RPC error payloads.
   - Existing tests cover the touched behavior surfaces, including descriptor payload metadata/annotations, workflow missing-input handling, SQL classification target extraction, provider registration, sharding workflow planning, JDBC metadata test helpers, and LLM model-facing formatter output.
   
   ### Review Details
   
   - Review Focus: Code Correctness Review. CI not reviewed by request.
   - Reviewed Scope: PR #38960 latest head `4494bbde9bfa087f18b95d2750a739984387b6a6`; base/merge-base `967924ab8d0130a3c1ccd53857ab2aa8e9b8df9b`. Reviewed all 11 changed files under `mcp/**` and `test/e2e/mcp/**`; local triple-dot file list matched GitHub `/pulls/38960/files`.
   - Not Reviewed Scope: GitHub Actions/check-runs, full Docker/native MCP E2E, and repository areas outside the changed MCP/MCP E2E files.
   - Verification:
     - `./mvnw -pl mcp/bootstrap,mcp/support,mcp/core,mcp/features/encrypt,mcp/features/mask,mcp/features/sharding -am -DskipITs -Dspotless.skip=true -Dtest=YamlHttpTransportConfigurationSwapperTest,MCPDescriptorCatalogPayloadBuilderTest,WorkflowPlanningSupportTest,MCPJdbcMetadataLoaderTest,StatementClassifierTest,EncryptMCPHandlerProviderTest,MaskMCPHandlerProviderTest,ShardingWorkflowPlanningServiceTest -Dsurefire.failIfNoSpecifiedTests=false test -B -ntp` passed with exit code 0.
     - `./mvnw -pl test/e2e/mcp -am -DskipITs -Dspotless.skip=true -Dtest=LLMMCPModelFacingToolResponseFormatterTest -Dsurefire.failIfNoSpecifiedTests=false test -B -ntp` passed with exit code 0.
     - `./mvnw checkstyle:check -Pcheck -T1C` passed with exit code 0.
   - Release Note / User Docs: Not required. The PR is an internal behavior-preserving MCP helper simplification for the broader open MCP umbrella issue #35294 and does not change user-facing configuration, protocol behavior, migration steps, or diagnostics.


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Simplify MCP helper code [shardingsphere]

Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
terrymanu merged PR #38960:
URL: https://github.com/apache/shardingsphere/pull/38960


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org