terrymanu commented on PR #38752:
URL: https://github.com/apache/shardingsphere/pull/38752#issuecomment-4573610021
### Decision
- **Merge Verdict: Mergeable**
- **Reviewed Scope:** PR #38752 latest head
`3a2aac11083eb207febfae4844c82b22fea88525`; local merge-base
`543a55be3eb257b254fb094dcac9e1dd520515d4`; 34 GitHub-listed files under
`distribution/mcp`, `docs/document/content/user-manual/shardingsphere-mcp`,
`mcp/bootstrap`, `mcp/support`, and `mcp/core`. Local triple-dot file list
matched GitHub `/pulls/38752/files`.
- **Not Reviewed Scope:** GitHub Actions / CI status, manual smoke against a
real ShardingSphere-Proxy instance, and repo areas outside the PR file list.
- **Need Expert Review:** No mandatory additional expert review identified.
### Basis
- The root-cause path is fixed at the configuration boundary: `username` and
`driverClassName` are now required and non-empty, while `password` can be
omitted and is normalized to an empty string before creating
`RuntimeDatabaseConfiguration`.
- The previous empty-driver behavior is removed from the runtime connection
path, so blank or missing driver configuration now fails as
`missing_jdbc_driver` instead of relying on implicit JDBC driver
auto-registration.
- Runtime JDBC/profile/metadata failures are converted into safe runtime
categories, including the new `authorization_failed` category, without exposing
JDBC URLs or credentials to MCP clients.
- HTTP origin rejection now returns a generic 403 message and logs only safe
server-side reason categories; the submitted `Origin` value is not reflected.
- STDIO risk is reduced by keeping the packaged console appender on
`System.err`; static search found no MCP `System.out` or `printStackTrace`
pollution in the reviewed scope.
- Metadata empty-state responses now distinguish `no_runtime_database`,
`unknown_database`, `not_found`, and `empty_scope`, with tests covering the
main negative and adjacent cases.
- Shared-path scan: the changed MCP runtime configuration path affects all
MCP-supported database capability dialects, including MySQL, MariaDB,
PostgreSQL, openGauss, Oracle, SQLServer, ClickHouse, Doris, Hive, Presto,
Firebird, and feature plugins such as Encrypt/Mask. The change is
dialect-neutral because it only requires an explicit driver class and preserves
database-type resolution; non-MySQL metadata coverage remains present through
the dialect sequence metadata tests.
- Adversarial pass found no unresolved cross-dialect regression,
feature-disabled path regression, or partial original-symptom fix. No unrelated
file changes were found in the PR scope.
### Verification
- Boundary check: GitHub `/pulls/38752/files` matched local `git diff $(git
merge-base apache/master apache/pr/38752)..apache/pr/38752 --name-only`.
- Reviewer-run targeted tests:
- `./mvnw -pl mcp/bootstrap -am -DskipITs -Dspotless.skip=true
-Dtest=YamlRuntimeDatabaseConfigurationSwapperTest,YamlRuntimeDatabaseConfigurationsSwapperTest,MCPLaunchConfigurationTest,YamlMCPLaunchConfigurationSwapperTest,YamlMCPTransportConfigurationSwapperTest,MCPConfigurationLoaderTest,OriginHeaderConstraintTest
-Dsurefire.failIfNoSpecifiedTests=false test` passed, 66 tests.
- `./mvnw -pl mcp/support -am -DskipITs -Dspotless.skip=true
-Dtest=RuntimeDatabaseConfigurationTest,RuntimeDatabaseConnectionExceptionTest,MCPJdbcMetadataLoaderTest
-Dsurefire.failIfNoSpecifiedTests=false test` passed, 45 tests.
- `./mvnw -pl mcp/core -am -DskipITs -Dspotless.skip=true
-Dtest=MetadataResourceHandlerTest,RuntimeStatusHandlerTest,MCPErrorConverterTest
-Dsurefire.failIfNoSpecifiedTests=false test` passed, 50 tests.
- Reviewer-run packaging/style checks:
- `./mvnw spotless:apply -Pcheck -T1C` passed.
- `./mvnw checkstyle:check -Pcheck -T1C` passed.
- `./mvnw -pl distribution/mcp -am -DskipTests -Dspotless.skip=true
package` passed.
- Static searches found no reviewed-scope residue for H2/demo runtime
wording, empty-driver/JDBC 4 guidance, old `logic_db` demo URL, `STDOUT`
appender references, or MCP stdout pollution.
--
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]
