Repository navigation
refactor(datastore): one internal declaration of the supported SQL databases - #16877
borinquenkid wants to merge 7 commits into
Conversation
…tabases Three readers of the database each held their own list of Hibernate dialect classes and their own copy of the lookup: SqlSyntax, RegexDialectPattern and UniqueIndexNulls on Hibernate 7, and RlikeExpression's instanceof chain on Hibernate 5. SupportedDatabase is now the one declaration per module, keyed on the dialect, and each of them asks it. DialectKeyed becomes ClassKeyed in grails-datastore-core, which names no database, so a store that is not SQL can declare its own definitions the same way. Both are marked as internal API in their Javadoc. Two readings change, to agree with each other and with Hibernate 5: rlike on PostgreSQL with Hibernate 7 uses ~*, and with Hibernate 5 a database with no regular expression operator falls back to like instead of failing. The criteria reference and the upgrade guide say so. Claude-Session: https://claude.ai/code/session_01Dmki4Tgxp7dLBhzpe2xT93
jamesfredley
left a comment
There was a problem hiding this comment.
Not approved. PostgreSQL rlike becomes case-insensitive. MariaDB is classified as MySQL, and Sybase is classified as SQL Server.
| // MariaDB's dialect extends MySQL's, and both read RLIKE | ||
| MYSQL(SqlSyntax.MYSQL, "?1 RLIKE ?2", UniqueIndexNulls.DISTINCT, MySQLDialect.class), | ||
|
|
||
| POSTGRESQL(SqlSyntax.POSTGRESQL, "?1 ~* ?2", UniqueIndexNulls.DISTINCT, PostgreSQLDialect.class), |
There was a problem hiding this comment.
findAllByNameRlike('^al.*') on PostgreSQL used to render ~ and return only alice. This renders ~* and also returns Alice. Keep the case-sensitive operator.
There was a problem hiding this comment.
Hibernate 7 keeps the case-sensitive ~, as before this PR. Hibernate 5 has always rendered ~* (RlikeExpression on 8.0.x already does), so the two differ today; #16882 tracks that. This PR changes neither.
| public enum SupportedDatabase implements ClassKeyed<SupportedDatabase> { | ||
|
|
||
| // MariaDB's dialect extends MySQL's, and both read RLIKE | ||
| MYSQL(SqlSyntax.MYSQL, "?1 RLIKE ?2", UniqueIndexNulls.DISTINCT, MySQLDialect.class), |
There was a problem hiding this comment.
MariaDBDialect extends a MySQL dialect and this list has no MariaDB entry, so forDialect returns MYSQL. SybaseDialect is listed under SQL_SERVER, so a Sybase database is reported as SQL Server. Keep each product as itself.
There was a problem hiding this comment.
Same as the reply below: MARIADB is declared before MYSQL on both versions, and SYBASE has its own entry.
… SupportedDatabase
…tor, the Hibernate 5 difference is #16882
|
Pushed da3870e, by point:
|
jamesfredley
left a comment
There was a problem hiding this comment.
Not approved. Hibernate 5 still classifies MariaDB as MySQL. Hibernate 7 now keeps PostgreSQL rlike case-sensitive, and keeps MariaDB and Sybase as their own databases.
The Hibernate 5 SupportedDatabase had no MariaDB entry, so forDialect returned MYSQL for MariaDB. MARIADB is declared before MYSQL, as the Hibernate 7 declaration is, and matches a regular expression the same way MySQL does. Claude-Session: https://claude.ai/code/session_01Dmki4RzWwreEC2GkkyFnvL
|
@jamesfredley Fixed in 78ca94c, on top of da3870e. The Hibernate 5 It matches a regular expression with |
jamesfredley
left a comment
There was a problem hiding this comment.
MariaDB is now classified as MariaDB. Thread PRRT_kwDOAAfRJ860nBMd resolved. Not approving. rlike still rewrites a mapped column that already contains ?2.
|
|
||
| private boolean isOracleDialect(Dialect dialect) { | ||
| return (dialect instanceof Oracle8iDialect); | ||
| return SupportedDatabase.forDialect(dialect).regexPattern().replace("?1", columns[0]).replace("?2", "?"); |
There was a problem hiding this comment.
Input: rlike on a property whose mapped column already contains ?2. The column SQL is rewritten, so the query names a column that does not exist. A formula that contains the literal ?2 is changed too. Replace ?2 before inserting the column SQL.
There was a problem hiding this comment.
Fixed in 71a4286: the value marker is replaced in the pattern before the column SQL goes in, so a column or formula containing ?2 stays as written. Spec added.
…16877) The regular expression pattern of a database is filled with the column first and with the value marker second, so a mapped column or formula that contains the text ?2 was changed as well and the query named a column that does not exist. The value marker is now replaced in the pattern before the column is put in. Claude-Session: https://claude.ai/code/session_01Dmki4RzWwreEC2GkkyFnvL
|
@jamesfredley Fixed in 71a4286. The value marker |
jamesfredley
left a comment
There was a problem hiding this comment.
rlike no longer rewrites a mapped column that already contains ?2.
The upgrade guide entry about the Hibernate rlike rendering follows the entry about exception mappings. Claude-Session: https://claude.ai/code/session_01Dmki4RzWwreEC2GkkyFnvL
jamesfredley
left a comment
There was a problem hiding this comment.
rlike no longer rewrites a mapped column that already contains ?2.
jamesfredley
left a comment
There was a problem hiding this comment.
rlike no longer rewrites a mapped column that already contains ?2.
Three readers of the database each kept their own list of Hibernate dialect classes and their own copy of the lookup:
SqlSyntax,RegexDialectPatternandUniqueIndexNullson Hibernate 7, and theinstanceofchain ofRlikeExpressionon Hibernate 5. A new database or a new dialect meant finding all of them.Change
SupportedDatabaseis the one declaration of the supported SQL databases in each Hibernate module, keyed on the Hibernate dialect. Hibernate 7 declaressqlSyntax(),regexPattern()anduniqueIndexNulls(); Hibernate 5 declaresregexPattern().SqlSyntax,GrailsRLikeFunctionContributor,UniqueIndexNullsandRlikeExpressionask it. The two modules declare their own because the two Hibernate versions render different SQL for some databases.ClassKeyedingrails-datastore-corereplacesDialectKeyed. It names no database, so a store that is not SQL declares its own definitions the same way, and core holds only the universal lookup.Behavior that changes
rlikewith Hibernate 5 on a database with no regular expression operator that Grails uses (SQL Server, Sybase, CockroachDB, Db2 for z/OS) falls back tolike, as with Hibernate 7, where it used to fail withrlike is not supported with the configured dialect.The criteria reference and section 91 of the Grails 8 upgrade guide describe it.
Not changed here:
rlikeon PostgreSQL is~*(case-insensitive) with Hibernate 5 and~(case-sensitive) with Hibernate 7. The Hibernate 7 rendering is kept so this refactor changes no rows; the difference is tracked in #16882. Running every supported database in Testcontainers is #16881.Tests
ClassKeyedSpec, and aSupportedDatabaseSpecin each module with every dialect, the default, the regular expression pattern, the syntax and the unique index reading.RlikeExpressionSpecfor the fallback, andRLikeHibernate7Specrunsrlikeagainst H2 and the Testcontainers databases and expects the rows Hibernate 5 returns on each. The Db2 row is skipped on arm machines and its case expectation has not been run.Related
#16583 (the
uniqueconstraint with a null value, 9.0.x) adds its unique-null properties to these declarations and drops its own copies of the dialect lists. This is the part of it that does not depend on that feature, so that it can land on 8.0.x first.https://claude.ai/code/session_01Dmki4Tgxp7dLBhzpe2xT93