terrymanu commented on PR #38737: URL: https://github.com/apache/shardingsphere/pull/38737#issuecomment-4556580977
### Decision - **Merge Verdict: Mergeable** - **Reviewed Scope:** PR #38737 latest head `368d1fcaf9e1`; final diff in `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/context/MCPRequestScope.java`; related MCP handler context interfaces, tool/resource/completion dispatch paths, and GitHub-visible comments/reviews. - **Not Reviewed Scope:** GitHub Actions / CI / check-runs, full `./mvnw clean install -B -T1C -Pcheck`, and runtime behavior outside MCP modules. - **Need Expert Review:** No additional security, concurrency, performance, or protocol expert review is required. ### Basis - The previous blocker is resolved: `MCPRequestScope` still uses class-level `@Getter` to reduce boilerplate, but excludes `databaseCapabilityProvider` and `metadataContext` with `@Getter(AccessLevel.NONE)`, so internal request-scope lifecycle objects are no longer exposed. - The generated bytecode method list confirms the public surface is back to the expected scope: it includes only `getActiveTransport()`, `getWorkflowSessionContext()`, `getMetadataQueryFacade()`, `getExecutionFacade()`, `getQueryFacade()`, `getDatabaseContext()`, `getCapabilityFacade()`, and `close()`, with no `getDatabaseCapabilityProvider()` / `getMetadataContext()`. - Handler boundaries remain unchanged: tool/resource/completion dispatch still casts only to the supported `MCPServiceHandlerContext`, `MCPDatabaseHandlerContext`, and `MCPWorkflowHandlerContext` interfaces. - Blast-radius review found no new risk: core metadata/search/execute/resource paths and MCP feature handlers such as encrypt/mask still access capabilities through the original interfaces. - Performance and compatibility risk is acceptable: this only changes accessor generation and does not add loops, I/O, remote calls, dependencies, SQL parser changes, routing changes, default schema behavior, config flags, or high-frequency DML/DQL `computeIfAbsent` changes. - The final PR diff only contains `MCPRequestScope.java`; no unrelated final code changes were found. ### Verification - `./mvnw -pl mcp/core -am -DskipITs -Dspotless.skip=true -Dtest=MCPToolControllerTest,MCPCompletionServiceTest,MCPResourceControllerTest,ToolDefinitionRegistryTest,ResourceDefinitionRegistryTest -Dsurefire.failIfNoSpecifiedTests=false test` passed: 42 tests, 0 failures. - `./mvnw checkstyle:check -Pcheck -T1C` passed. - `./mvnw spotless:check -Pcheck -T1C` passed. - `javap` was used to inspect generated `MCPRequestScope` methods and confirmed the internal getters are absent. - GitHub Actions / CI status was intentionally not used for this merge decision. -- 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]
