fix: address review of the dialect resolution changes - #362
Merged
Conversation
Resolving the dialect while constructing a SchemaValidator opened a JDBC connection, so merely creating one could fail on pool initialisation, authentication or the network before validate() was ever called. The dialect is now resolved inside validate(), from the connection it already holds, which removes the extra connection rather than moving it. The helper that duplicated Providers.getSqlDialect(DataSource, StormConfig) goes with it. Constraint discovery was recorded for the schema as a whole, so one table that a driver could not answer for disabled primary and unique key validation for every other table, hiding mismatches unrelated to it. Discovery is now recorded per table. Primary keys and unique keys were recorded as one kind although the JDBC metadata strategy reads them with separate calls, so a driver that could not report unique indexes also cost the primary keys. They are separate kinds now, and each failure comment names the kind it belongs to.
There was a problem hiding this comment.
Pull request overview
This PR is a follow-up to #360 to refine schema validation behavior by (1) deferring SQL dialect resolution until validation time (so building a SchemaValidator doesn’t need to open a connection) and (2) making constraint discovery tracking more precise (per-table, and with primary/unique keys tracked separately) to avoid over-skipping validation when JDBC metadata calls fail partially.
Changes:
- Move dialect resolution from
SchemaValidator.of(DataSource, …)construction time intovalidate()using the already-open validation connection. - Track constraint discovery per table and split “keys” into
PRIMARY_KEYvsUNIQUE_KEYto prevent unrelated failures from disabling validation broadly. - Add regression tests using a
DatabaseMetaDataproxy to simulate per-table/per-method JDBC metadata failures.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| storm-core/src/test/java/st/orm/core/template/impl/DatabaseSchemaTest.java | Adds JDBC-metadata failure simulation tests to verify per-table discovery and PK/UK separation. |
| storm-core/src/main/java/st/orm/core/template/impl/SchemaValidator.java | Defers dialect resolution to validation time and threads the resolved dialect through schema reads/validation. |
| storm-core/src/main/java/st/orm/core/template/impl/DatabaseSchema.java | Changes constraint discovery tracking to be per-table and splits key kinds into PRIMARY_KEY / UNIQUE_KEY. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
119
to
133
| /** | ||
| * A kind of constraint a schema read discovers. | ||
| * | ||
| * <p>Each kind is read by one query (or one set of metadata calls) per strategy, so a failure applies to the | ||
| * kind as a whole. See {@link #isDiscovered(ConstraintKind)} for why the outcome is recorded.</p> | ||
| * <p>The kinds are separate because a strategy can read one and fail on another: the JDBC metadata strategy | ||
| * asks for each with its own call. See {@link #isDiscovered(String, ConstraintKind)} for why the outcome is | ||
| * recorded.</p> | ||
| */ | ||
| public enum ConstraintKind { | ||
| /** Primary keys and unique keys, which every strategy reads together. */ | ||
| KEY, | ||
| /** Primary keys. */ | ||
| PRIMARY_KEY, | ||
| /** Unique keys. */ | ||
| UNIQUE_KEY, | ||
| /** Foreign keys. */ | ||
| FOREIGN_KEY | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #360, which merged before its review comments were addressed. Three of the five were raised inline, two were suppressed; all five held up.
Resolving the dialect no longer opens a connection
SchemaValidator.of(DataSource)resolved the dialect while constructing the validator, which reached the database for its product name. Building a validator could therefore fail on connection pool initialisation, authentication or the network beforevalidate()was ever called.validate()already holds an open connection, andProviders.getSqlDialect(Connection, StormConfig)already exists, so the dialect is resolved there instead. The extra connection is gone rather than moved. A dialect passed explicitly to the three-argument factory still wins, andORMTemplateImplkeeps resolving from its own data source with its own config.This also removes the helper that duplicated
Providers.getSqlDialect(DataSource, StormConfig), including its fallback branch, which the default provider made unreachable.Discovery is recorded per table
Constraint discovery was recorded for the schema as a whole. Under the JDBC metadata strategy, which asks per table, one table a driver could not answer for disabled primary and unique key validation for every table. A view that does not support
getPrimaryKeyswould have hidden real mismatches across the schema.Discovery is now recorded per table, so a failure costs only the table it happened on. The bulk strategies read every table in one query, so their success or failure still covers the whole schema, which the same per-table record expresses.
Primary keys and unique keys are separate kinds
They were one
KEYkind, but the JDBC metadata strategy reads them with separate calls:getPrimaryKeysandgetIndexInfo. A driver that could not report unique indexes therefore also cost the primary keys. They are nowPRIMARY_KEYandUNIQUE_KEY, and each failure comment names the kind it belongs to rather than saying "keys" in a foreign key block.Tests
Two tests in
DatabaseSchemaTestcover the new precision, using aDatabaseMetaDataproxy that fails one call for one table:Unlike the tests in #360, these two cannot be run against the previous code to show them failing: the enum constants and the
isDiscoveredsignature both changed, so there is no earlier version to point them at. They hold by construction.The regression tests from #360 still bite: pointing the deferred resolution back at the product-blind lookup fails two of them.
Full reactor on this base: BUILD SUCCESS, 7240 tests.