terrymanu commented on PR #39073:
URL: https://github.com/apache/shardingsphere/pull/39073#issuecomment-4948972089
### Summary
**Review Result: Mergeable**
- Reason: The latest head is ready to merge within the reviewed code scope.
This is a code-scope-only conclusion; CI not reviewed by request.
- The PR removes silent unknown-database fallbacks from capability lookup,
dialect SPI resolution, and the Encrypt/Mask planning context, addressing the
actual root cause rather than only handling the resulting exception.
### Evidence
-
`mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/workflow/WorkflowProxyQueryService.java:75-77`
no longer returns an empty database type. It throws the existing
`DatabaseCapabilityNotFoundException` when the capability is absent.
-
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseDialect.java:57-64`
requires valid `DatabaseType` and `DialectDatabaseMetaData` SPI services
instead of fabricating quote, schema, or identifier-case defaults.
-
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/spi/MCPFeatureQueryFacade.java:45-52`
makes database-type lookup an explicit implementation contract. The sole
production implementation in the complete source scope has been updated.
-
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowPlanningContextValidator.java:90-100`
verifies both the runtime profile and MCP capability.
`WorkflowPlanningSupport` and the Encrypt and Mask call chains propagate the
query facade correctly.
- Regression tests cover:
- A runtime profile that exists without a corresponding capability.
- Capability lookup failure.
- Empty or unknown database types.
- Missing dialect metadata.
- Invalid-identifier precedence and prevention of further metadata
discovery after capability failure.
- Database-type-aware fixtures for adjacent Broadcast, read/write-splitting,
Shadow, and Sharding features were updated consistently. No behavioral
regression, state leakage, or production side effect was found.
### Review Details
- Review Focus: Code Correctness Review — CI not reviewed by request
- Reviewed Scope: All 29 files reported by GitHub `/pulls/39073/files`;
latest head `154332962a16a59fb7d5aa02c1d94392b9f91985`, base and merge-base
`25d65170b191a1ee56dd8268dfca227985d59878`, base ref `master`. The local
triple-dot file list matched the GitHub file list exactly.
- Not Reviewed Scope: GitHub Actions, check runs, and remote CI status.
Unchanged code was reviewed only where required to trace callers and adjacent
execution paths.
- Verification:
- The tracked source was confirmed to match the latest PR head exactly.
- `./mvnw spotless:apply -Pcheck -T1C` — exit 0.
- `./mvnw checkstyle:check -Pcheck -T1C` — exit 0, with 0 violations.
- A local `install` covering the 11 MCP modules and dependencies selected
by `ci-mcp.yml` — exit 0, `BUILD SUCCESS`; all 22 changed test classes were
executed.
- PR metadata, paginated file lists, commits, comments, and reviews were
accessible. No public-evidence gap affected this result.
- Release Note / User Docs: Not required. This change tightens fail-fast
behavior in an unreleased MCP path and introduces no configuration, migration,
dependency, packaging, or user-workflow changes.
--
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]
