terrymanu commented on PR #39049:
URL: https://github.com/apache/shardingsphere/pull/39049#issuecomment-4912203075
### Summary
**Review Result: Mergeable**
Reason: Code-scope review only; CI not reviewed by request. The latest PR
head keeps the MCP system-schema filtering contract intact while simplifying
the fallback boundary: explicit MCP option schemas are used when present, and
dialect `SystemDatabase` schemas are used only when the MCP option has no
schema list.
### Evidence
-
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialect.java:184`
now makes the precedence explicit: non-empty
`MCPDatabaseCapabilityOption#getSystemSchemas()` wins; otherwise
`SystemDatabase#getSystemSchemas()` is used when available.
- The public behavior still flows through `isSystemSchema(...)`, which trims
empty schema names and performs case-insensitive membership checks at
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialect.java:164`
and
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialect.java:193`.
- Existing focused tests cover the important branches:
- dialect-only system schemas at
`mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialectTest.java:153`
- MCP option schemas overriding different dialect schemas at
`mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialectTest.java:167`
- different dialect schemas being ignored when an option schema list
exists at
`mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialectTest.java:183`
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: PR #39049, head SHA
`59027db44c730d96cee6d02bc11c87e105d5a000`, base SHA
`7f7a9f4f15b60b2ef89bb740b3c3f66f99d4b00c`, merge-base SHA
`7f7a9f4f15b60b2ef89bb740b3c3f66f99d4b00c`. Reviewed file:
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialect.java`.
Local triple-dot file list matched GitHub `/pulls/39049/files`.
- Not Reviewed Scope: GitHub Actions/check-runs, broader MCP E2E behavior,
and files outside the PR diff.
- Verification: `./mvnw -pl mcp/support -Dtest=MCPDatabaseDialectTest
-DskipITs -Dspotless.skip=true -Dsurefire.failIfNoSpecifiedTests=false test
-Pcheck` exited `0`; `MCPDatabaseDialectTest` ran 32 tests with 0 failures and
0 errors.
- Release Note / User Docs: Not required; this is an internal MCP support
refactor with no user-facing configuration, API, or behavior change.
--
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]
