fix: resolve the validation dialect from the database product - #360
Conversation
Schema validation resolved its dialect with the no-argument provider lookup, which returns the first registered dialect regardless of the database it is about to read. The template factories already select by database product; validation now does the same, and falls back to the first registered dialect only when no provider claims the product. With several dialect modules on the classpath the previous lookup could hand an H2 database the MySQL dialect, whose constraint discovery reads a KEY_COLUMN_USAGE column H2 does not have. That query failed, and a failed discovery left the constraint maps empty, which validation read as "the database has no such constraints" and reported every foreign key as missing. A schema read now records which constraint kinds it managed to read, and validation skips the checks for a kind that stayed unknown, so a database that cannot answer the query is no longer reported as wrong. The cause of the failed read is logged at debug rather than discarded. Fixes #359
There was a problem hiding this comment.
Pull request overview
This PR fixes schema validation flakiness and false negatives when multiple SQL dialect modules are present by (1) resolving the validator dialect based on the actual database product, and (2) distinguishing “constraints could not be discovered” from “constraints are missing” so validation skips unknown constraint kinds rather than reporting incorrect errors.
Changes:
- Update
SchemaValidator.of(DataSource…)to resolveSqlDialectusing the database product (with a fallback when no provider matches). - Track constraint discovery success in
DatabaseSchemaviaConstraintKind+isDiscovered(...), and skip PK/UK/FK checks when discovery failed; log discovery failures at debug. - Add focused tests covering dialect mismatch behavior and constraint discovery outcomes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| storm-core/src/main/java/st/orm/core/template/impl/SchemaValidator.java | Resolve dialect from DB product and skip PK/UK/FK validations when the corresponding constraint kind couldn’t be discovered. |
| storm-core/src/main/java/st/orm/core/template/impl/DatabaseSchema.java | Track which constraint kinds were successfully discovered and log discovery failures instead of silently swallowing exceptions. |
| storm-core/src/test/java/st/orm/core/template/impl/SchemaValidatorConstraintDiscoveryTest.java | New regression test ensuring FK validation is skipped (not reported missing) when FK discovery fails due to dialect mismatch. |
| storm-core/src/test/java/st/orm/core/template/impl/DatabaseSchemaTest.java | New assertions validating isDiscovered(ConstraintKind) behavior for matching vs mismatched discovery strategies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private static SqlDialect resolveSqlDialect(@Nonnull DataSource dataSource) { | ||
| StormConfig config = StormConfig.defaults(); | ||
| SqlDialectProvider provider = getSqlDialectProvider(getDatabaseProductName(dataSource)); | ||
| return provider != null ? provider.getSqlDialect(config) : getSqlDialect(config); | ||
| } |
| @@ -268,8 +311,10 @@ private static void readConstraintsFromJdbcMetadata( | |||
| primaryKeysByTable.computeIfAbsent(pkTableName, k -> new ArrayList<>()) | |||
| .add(new DbPrimaryKey(pkTableName, columnName, keySeq)); | |||
| } | |||
| } catch (SQLException ignored) { | |||
| // Some databases/views may not support getPrimaryKeys; skip gracefully. | |||
| } catch (SQLException e) { | |||
| // Some databases/views may not support getPrimaryKeys; the keys stay unknown. | |||
| LOGGER.debug("Failed to read primary keys for table '{}'.", tableName, e); | |||
| keysRead = false; | |||
| } | |||
| // Some databases/views may not support getImportedKeys; the keys stay unknown. | ||
| LOGGER.debug("Failed to read foreign keys for table '{}'.", tableName, e); |
ORMTemplate.validateSchema() and validateSchemaOrThrow() build their validator with a dialect resolved from the first registered provider whenever the template carries no explicit provider filter, even though the template knows the data source it is bound to. The filter is absent exactly when no dialect module claims the database product, so the fallback could hand the schema read an unrelated vendor's dialect. Resolving from the data source keeps validation on the dialect that matches the database, and an explicit provider filter still wins.
Audit of the other dialect resolution pointsChecked every place that resolves a
Also fixed: validation through the template
Left alone: the query dialect fallback
So with a vendor module on the classpath and a database no module claims, SQL is generated with that vendor's dialect. Measured with Every provider claims only its own product name, so this also covers a case that may be load-bearing for someone today: MariaDB with only Test coverage noteThe |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
storm-core/src/main/java/st/orm/core/template/impl/DatabaseSchema.java:339
- In the JDBC_METADATA strategy, a failure in getIndexInfo() sets keysRead=false, which marks ConstraintKind.KEY as undiscovered and will also skip primary key validation (SchemaValidator gates PK checks on KEY). Since primary keys (getPrimaryKeys) and unique keys (getIndexInfo) are separate calls, it’s possible for PKs to be readable while UKs are not; in that case PK validation would be unnecessarily disabled.
} catch (SQLException e) {
// Some databases/views may not support getIndexInfo; the keys stay unknown.
LOGGER.debug("Failed to read unique indexes for table '{}'.", tableName, e);
keysRead = false;
}
storm-core/src/main/java/st/orm/core/template/impl/SchemaValidator.java:140
- resolveSqlDialect() duplicates Providers.getSqlDialect(DataSource, StormConfig) and adds a fallback branch that is effectively unreachable when the default dialect provider is present (it supports all products). Using the existing DataSource-based resolution keeps behavior consistent with other templates and avoids drifting from Providers logic.
private static SqlDialect resolveSqlDialect(@Nonnull DataSource dataSource) {
StormConfig config = StormConfig.defaults();
SqlDialectProvider provider = getSqlDialectProvider(getDatabaseProductName(dataSource));
return provider != null ? provider.getSqlDialect(config) : getSqlDialect(config);
}
Every module carries at most one dialect, so nothing in the reactor could observe a resolution that ignores the database, and the same defect was fixed twice before a test could see it. This module's dialect claims only MySQL. Adding H2 to its test scope creates the one configuration the reactor can check: a database no provider here claims, where resolving without asking the database lands on this module's dialect instead of the default. Reading an H2 schema with MySQL's constraint queries silently reads nothing, so the tests assert on findings that require the constraints to have been read, a foreign key that is absent and one that points elsewhere, through both the SchemaValidator factory and ORMTemplate.validateSchema. One test asserts that the mismatch is still present, so adding another dialect to this module's test scope fails loudly rather than quietly leaving the rest asserting nothing.
Regression tests for the resolution itselfThe two fixes above landed without a test that could see them, because catching a resolution that ignores the database needs two dialects on one classpath and every module carries at most one. That gap is why the same defect had to be fixed twice.
The assertions are on findings that are only possible once the constraints have actually been read, since MySQL's constraint discovery against H2 fails silently and leaves nothing to report:
both through
Each test was checked against the unfixed code:
Full |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
storm-core/src/main/java/st/orm/core/template/impl/DatabaseSchema.java:352
- In the JDBC metadata constraint discovery, this catch block is specifically about foreign key discovery, but the comment says "the keys stay unknown", which is ambiguous (could read as primary/unique keys). Clarifying this avoids confusion when reading logs and discovery semantics.
// Some databases/views may not support getImportedKeys; the keys stay unknown.
storm-core/src/main/java/st/orm/core/template/impl/DatabaseSchema.java:122
ConstraintKind.KEYis documented as "Primary keys and unique keys, which every strategy reads together", but the JDBC_METADATA strategy reads primary keys and unique indexes via separate metadata calls (and can fail independently). Rewording the enum comment avoids stating a guarantee that the implementation doesn’t uphold.
/** Primary keys and unique keys, which every strategy reads together. */
A template resolves the dialect of the database it is bound to and hands it to its processor, which uses it to bind parameters and to size fetches. The SQL template it generates statements with was built on the shared PS template instead, whose dialect is resolved once with no database in view, and was only corrected when a provider filter happened to be set. A filter is set only when a dialect module claims the database product, so with a vendor module on the classpath and a database no module claims, the two halves of a query disagreed: statements written in that vendor's syntax, executed with the dialect the template had resolved for itself. The resolved dialect is now carried through the constructor chain and applied to the SQL template as well, so both halves speak for the same database. An explicit provider filter still takes precedence. The accompanying test compares exact dialect classes: the vendor dialects extend the default one, so an instanceof check cannot tell them apart.
The query dialect is fixed too, and a correctionI said earlier that the query dialect fallback was a judgement call about MariaDB. That framing rested on an inference, and the first test I wrote for it appeared to show the query path was fine. Both were wrong, for the same reason: What was wrongA template resolves the dialect of the database it is bound to and hands it to its processor, which binds parameters and sizes fetches with it. The SQL template it generates statements with was built on the shared A filter is set only when a dialect module claims the database product. So with a vendor module on the classpath and a database no module claims, the two halves of a query disagreed about the database: statements written in that vendor's syntax, executed with the dialect the template had resolved for itself. The file already carried a comment noting the two could diverge. The resolved dialect is now carried through the constructor chain and applied to the SQL template as well. An explicit provider filter still takes precedence. That also changes how the MariaDB case should be read. MariaDB with only Still not changed: JPA
Verification
|
…base A JPA template built its SQL on the shared JPA template, whose dialect is resolved once from the classpath with no database in view, and corrected it only when a provider filter was set. Nothing set one, so a persistence unit was written for whichever dialect module happened to sort first. Jakarta Persistence publishes the data source a persistence unit was configured with, which is the portable way to reach its database: an entity manager cannot be unwrapped to a Connection on every provider. The template now resolves its dialect from that data source. A persistence unit configured with a connection URL rather than a data source, or one whose database cannot be reached while the template is being built, leaves the database unknown; the dialect then comes from the classpath as before and the cause is logged at debug. An explicit provider filter still takes precedence.
JPA resolves from its persistence unit tooThe remaining path is fixed. What made it look undecidable was the assumption that a JPA template has no way to reach its database. It does, just not the obvious one. Probing what an entity manager actually exposes under Hibernate: Jakarta Persistence publishes the data source a persistence unit was configured with, so the template resolves its dialect from that, the same way the JDBC path does. Both the current property names and their No regression where the database stays unknownA persistence unit configured with a connection URL rather than a data source, or one whose database cannot be reached while the template is being built, leaves the database unknown. The dialect then comes from the classpath exactly as before, and the cause is logged at debug rather than swallowed. So the concern I raised earlier, that a JPA user with a single vendor module would lose their dialect, does not apply: they keep it, and they now get the right one whenever the database can be identified. Verification
It sets Full reactor with all four fixes: BUILD SUCCESS, 7238 tests, no failures or errors. Every dialect resolution point that has a database in reach now asks it. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
storm-core/src/main/java/st/orm/core/template/impl/SchemaValidator.java:139
SchemaValidator.of(DataSource)now resolves the dialect by callingProviders.getDatabaseProductName(dataSource), which opens a connection and throws aPersistenceExceptionif the DataSource cannot provide one. That makes validator construction fail eagerly in scenarios where the database is temporarily unreachable or the DataSource is lazily initialized; previously it could still be constructed (dialect resolved from classpath) and fail later during actual validation.
Consider catching the runtime exception and falling back to classpath-based dialect resolution when product-name detection fails, so validation can still be invoked (and fail in the usual place) instead of failing at factory time.
private static SqlDialect resolveSqlDialect(@Nonnull DataSource dataSource) {
StormConfig config = StormConfig.defaults();
SqlDialectProvider provider = getSqlDialectProvider(getDatabaseProductName(dataSource));
return provider != null ? provider.getSqlDialect(config) : getSqlDialect(config);
Fixes #359
The defect
SchemaValidator.of(DataSource)resolved its dialect with the no-argumentProviders.getSqlDialect(), which returns the first registeredSqlDialectProviderwithout consulting the database it is about to read. It had theDataSourcein hand, andProviders.getSqlDialect(dataSource, config)already selects by product name, which is whatPreparedStatementTemplateImpldoes. Queries therefore stayed on the right dialect while validation did not.With several dialect modules registered, the winner of that
findFirst()depends on service loading order. An H2 database could be read with the MySQL dialect, whoseINFORMATION_SCHEMA_REFERENCINGstrategy queriesKEY_COLUMN_USAGE.REFERENCED_TABLE_NAME, a column H2 does not have.The failure then became invisible.
DatabaseSchemaswallowed theSQLException, and an empty foreign key map reads exactly like a schema with no foreign keys, so validation reported every@FKas missing. The existing comments state the intent ("foreign key validation will be skipped"); the implementation reported instead of skipping.The change
Dialect resolution.
SchemaValidator.of(DataSource)andof(DataSource, ModelBuilder)resolve the dialect from the database product, falling back to the first registered dialect only when no provider claims the product.Unknown is not missing. A schema read records which constraint kinds it managed to read (
DatabaseSchema.isDiscovered). Validation skips the primary key, unique key and foreign key checks for a kind that stayed unknown, so a database that cannot answer a discovery query is reported as unknown rather than as wrong. Kinds are tracked separately: a failed foreign key query no longer suppresses primary key validation. The cause of a failed read is logged at debug instead of being discarded.Verification
SchemaValidatorConstraintDiscoveryTestcovers the reporting half: with a dialect whose constraint discovery does not fit the database, the foreign key that exists is not reported missing, while the primary keys that were readable are still validated. Both new assertions fail without the change.DatabaseSchemaTestcovers the read half: a fitting strategy discovers the foreign key, a mismatched one leavesFOREIGN_KEYundiscovered whileKEYstays discovered.storm-h2,storm-mysqlandstorm-postgresqltogether, which failed its suite consistently before, now passes five consecutive runs.