Skip to content

Fix: Handle MySQL NULL schema behavior#38595

Open
AtharvaMa wants to merge 2 commits into
apache:masterfrom
AtharvaMa:fix/mysql-null-schema-handling-28469
Open

Fix: Handle MySQL NULL schema behavior#38595
AtharvaMa wants to merge 2 commits into
apache:masterfrom
AtharvaMa:fix/mysql-null-schema-handling-28469

Conversation

@AtharvaMa

Copy link
Copy Markdown

Fixes #28469

Changes proposed in this pull request:
-Changed MySQLMetaDataLoader.java so that it works right when Connection.getCatalog() gives back null.
-Added a specific unit test assertLoadWithNullCatalog in MySQLMetaDataLoaderTest.java to simulate the null catalog behavior and verify the metadata loads successfully.
-Update the logic that incorrectly fell back to an empty string "" when the catalog was null(null != ""), preventing the subsequent TableNotExistsException and NPEs.


Before committing this PR, I'm sure that I have checked the following options:

  • My code follows the code of conduct of this project.
  • I have self-reviewed the commit code.
  • I have (or in comment I request) added corresponding labels for the pull request.
  • I have passed maven check locally : ./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.
  • I have made corresponding changes to the documentation.
  • I have added corresponding unit tests for my changes.
  • I have updated the Release Notes of the current development version. For more details, see Update Release Note

@AtharvaMa

Copy link
Copy Markdown
Author

Hi maintainers, could you please review my PR and trigger the CI/CD checks?

@AtharvaMa

Copy link
Copy Markdown
Author

Hi maintainers, There is a styling error which I resolve could you please trigger the CI/CD checks..?
Also, as I'm new to open source, I’d appreciate any feedback or suggestions you might have on the PR

@AtharvaMa

Copy link
Copy Markdown
Author

Hi everyone, I’m just checking in on the status of this PR. I’m happy to make any requested changes or address any feedback to help move this forward. Please let me know if there is anything I can do to assist with the review process. Thanks

@terrymanu terrymanu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decision

  • Merge Verdict: Not Mergeable
  • Reviewed Scope: Latest PR head c0d23757d626c74e760a39f8ac9f3750ce8de0d1; local merge-base 04cecfc6f980319ad3aa5c8fabce30f9b06ea61f; GitHub /pulls/38595/files matched local git diff --name-status with 2 changed files: database/connector/dialect/mysql/src/main/java/org/apache/shardingsphere/database/connector/mysql/metadata/data/loader/MySQLMetaDataLoader.java and database/connector/dialect/mysql/src/test/java/org/apache/shardingsphere/database/connector/mysql/metadata/data/loader/MySQLMetaDataLoaderTest.java. Supporting paths checked: infra/common/src/main/java/org/apache/shardingsphere/infra/metadata/database/schema/util/SchemaMetaDataUtils.java, infra/common/src/main/java/org/apache/shardingsphere/infra/datanode/DataNodes.java, and features/mask/core/src/main/java/org/apache/shardingsphere/mask/rule/MaskRule.java.
  • Not Reviewed Scope: No GitHub Actions / CI status, no live MySQL JDBC or Proxy smoke, and no Maven test run. SQL parser family scan is not applicable because no parser files changed.
  • Need Expert Review: Yes, a MySQL metadata/schema-loading maintainer should re-check the revised production-path behavior. No separate security or dependency review is needed.

Positive Feedback

  • The change is small and targets the right MySQL metadata loader method.
  • The new test covers one simple null catalog case where the fallback table-to-schema cache is already populated.

Major Issues

  • [Blocker] Null-catalog root cause is not fixed for the reported masking-only topology (database/connector/dialect/mysql/src/main/java/org/apache/shardingsphere/database/connector/mysql/metadata/data/loader/MySQLMetaDataLoader.java:208)
    • Symptom: The new branch falls back to GlobalDataSourceRegistry.getInstance().getCachedDatabaseTables().get(tableNames.iterator().next()) when Connection#getCatalog() is null or empty (MySQLMetaDataLoader.java:209-211). However, the added test seeds that cache directly (MySQLMetaDataLoaderTest.java:97-100) instead of proving that the real schema-building path populates it. In the masking-only scenario from issue #28469, MaskRule contributes a MaskTableMapperRuleAttribute only (features/mask/core/src/main/java/org/apache/shardingsphere/mask/rule/MaskRule.java:57), so DataNodes#getDataNodes(table) has no data node schema to cache. SchemaMetaDataUtils only writes the fallback cache while iterating data nodes whose data source name contains a schema (infra/common/src/main/java/org/apache/shardingsphere/infra/metadata/database/schema/util/SchemaMetaDataUtils.java:96-105).
    • Risk: For jdbc:mysql://host:3306/ with no selected database, which MySQL documents as a valid state where DATABASE() returns NULL, this can still bind a null schema into the metadata SQL and return empty metadata. That leaves the original TableNotExistsException / wrong metadata path unresolved for the reported ShardingSphere-JDBC + Masking use case.
    • Action: Please fix the production fallback source, not only the cached-subset symptom. Either carry the authoritative schema into the loader material, or explicitly load the relevant schemas when MySQL has no current catalog. Please add a regression test through SchemaMetaDataUtils or GenericSchemaBuilder for the masking-only no-current-schema path, without manually inserting into GlobalDataSourceRegistry.

Newly Introduced Issues

  • [Blocker] null catalog with an empty table set can now fail before metadata loading (database/connector/dialect/mysql/src/main/java/org/apache/shardingsphere/database/connector/mysql/metadata/data/loader/MySQLMetaDataLoader.java:210)
    • Symptom: The new null == catalog || catalog.isEmpty() branch calls tableNames.iterator().next() (MySQLMetaDataLoader.java:211). The existing empty-table test only covers a non-empty catalog (MySQLMetaDataLoaderTest.java:268), so the adjacent case actualTableNames = empty plus catalog = null is unvalidated.
    • Risk: The MySQL loader's existing "load without requested table names" path can regress to NoSuchElementException before it decides whether to load all metadata, return empty metadata, or use an explicit schema.
    • Action: Please define the expected behavior for empty requested table names when MySQL has no current catalog, guard the fallback accordingly, and add a dedicated regression test.

Next Steps

  • Rework schema fallback so it comes from an authoritative production source instead of a manually seeded tableName -> database global cache.
  • Add production-path regression coverage for ShardingSphere-JDBC + Masking with a MySQL URL that has no schema selected.
  • Add at least one counterexample for fallback binding, such as missing cache, empty requested table set, or same table name in two schemas.
  • Keep the current non-empty catalog and empty-string catalog cases covered.
  • Run the scoped verification after the fix, for example: ./mvnw -pl database/connector/dialect/mysql -am -DskipITs -Dspotless.skip=true -Dtest=MySQLMetaDataLoaderTest -Dsurefire.failIfNoSpecifiedTests=false test, then run the project-required Spotless and Checkstyle gates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MySQL's behavior without current schema

2 participants