Repository navigation
Grails 8 upgrade and Hibernate 7 support - #136
Conversation
Bump version to 9.0.0-SNAPSHOT and add sbglasius as developer.
Under Grails 8's Groovy, casting a String to Enum calls Enum.valueOf(Enum, ...) and throws IllegalArgumentException instead of ClassCastException, so invalid enum values no longer produced the expected HibernateException.
Move the Hibernate-independent code (hstore parsing, BidiEnumMap, PgArrayUtils) and its unit tests to a new grails-postgresql-extensions-core module, and the Hibernate 5 plugin from plugin/ to plugin-hibernate5/, keeping its grails-postgresql-extensions artifact. PgArrayUtils now throws IllegalArgumentException, which the Hibernate 5 criterion rethrows as HibernateException as before. The developer list moves to the root build so all published modules share it.
Add grails-postgresql-extensions-hibernate7 with the same user types, dialect, criteria and orders as the Hibernate 5 plugin, in the same packages: - User types implement the Hibernate 7 UserType API, and the dialect registers the column types of their custom SQL type codes with the DdlTypeRegistry. - Hibernate 7 has no Criteria API, so the criteria are GORM criteria (PgCriterion) turned into JPA predicates by a PredicateGenerator.CriterionHandlerProvider. They render PostgreSQL SQL through a self-rendering function and bind their values as parameters. - GORM only orders by properties, so sqlFormula() and byRandom() add a criterion that appends the formula to the orders of entity queries. Document the plugin, and the GORM 8.0.0 workaround for the 'native' id generator not creating its sequences on PostgreSQL.
A copy of test-app1 that uses grails-data-hibernate7 and the Hibernate 7 plugin, with the same domain classes, services and integration specs. It maps ids with the 'sequence' generator, as GORM 8.0.0 does not create the sequences of the default 'native' generator on PostgreSQL.
Each test app now starts its own PostgreSQL container with the hstore extension through the Testcontainers JDBC URL, instead of sharing one database. The apps run in parallel and interfered with each other's data in the shared database. The CI test job runs directly on the runner, so Testcontainers can use its Docker, and no longer needs the postgres service or the psql setup steps.
matrei
left a comment
There was a problem hiding this comment.
Thanks, this is a big step forward. Binding hstore values as parameters instead of inlining them, escaping the json attribute, Testcontainers isolation and the module split all look good. A few comments inline. Also, the Dockerfile looks unused now, and the hibernate7 plugin descriptor's developer list is missing you.
| for (int i = 0; i < segments.length - 1; i++) { | ||
| String segment = segments[i] | ||
| Join<?, ?> join = from.joins.find { Join<?, ?> j -> j.attribute.name == segment } | ||
| from = join ?: from.join(segment) |
There was a problem hiding this comment.
This creates the join directly on the JPA From, outside GORM's JoinTracker. GORM's ExpressionResolver only looks there, so when a pg criterion on an alias runs before a GORM criterion on the same alias, e.g.
createAlias('like', 'l')
pgArrayContains('l.favoriteNumbers', 3)
eq('l.name', 'x')we get two joins to like. On a to-many association that changes the semantics (the conditions can match different rows) and produces duplicate results. A createAlias(..., LEFT_OUTER_JOIN) also becomes an inner join for the pg criterion. The reverse order works because the existing join is found in from.joins.
Could we at least honour the alias's join type, add a test mixing pg and GORM criteria on the same alias, and maybe ask upstream to pass the JpaQueryContext to CriterionHandler?
There was a problem hiding this comment.
Good catch, and it turned out to be wider than the order: GORM 8 itself joins the association twice, once for createAlias (unused) and once for the alias, and the pg criterion picked the unused one, so the mixed test failed in both orders. GORM also always makes the alias join inner, even with LEFT_JOIN (pure GORM fails that on Hibernate 7).
Fixed in 9b34ee8:
- For an inner-join alias, the pg criterion is added together with an always-true GORM restriction on the alias (
l.id is null or l.id is not null), so GORM creates the alias join first, and the criterion uses the latest join of the association, which is that one. pg and GORM criteria now share one join in either order. - For an outer-join alias, the criterion uses the join with the alias's join type, so
LEFT_JOINis honoured for pg criteria. GORM criteria on that alias still get GORM's inner join.
PgArrayCriteriaWithAliasIntegrationSpec covers both orders on a to-many association and the left join, on Hibernate 5 and 7. Reported upstream as apache/grails-core#16562 (double join, join type), and asked for the JpaQueryContext in apache/grails-core#16563.
| def criteriaQuery = (CriteriaQuery<?>) query | ||
| List<Order> orders = new ArrayList<>(criteriaQuery.orderList) | ||
| orders << cb.asc(cb.function(PgSqlFunction.EXPRESSION, Object, cb.literal(formula))) | ||
| criteriaQuery.orderBy(orders) |
There was a problem hiding this comment.
This relies on JpaCriteriaQueryCreator.createQuery() calling assignOrderBy before assignCriteria; if GORM swaps them, the formula is silently overwritten. It also means the formula always ends up after property orders, so order(byRandom()); order('name') orders differently than on Hibernate 5.
And since it's a predicate returning cb.conjunction(), using it inside or {} makes the disjunction always true, and inside not {} matches nothing.
Could we document this and add tests combining it with a property order and with pagination (list(max:))?
There was a problem hiding this comment.
Fixed in 9b34ee8:
- The formula is inserted at the position
order(...)was called in, counting property orders and earlier formula orders, soorder(byRandom()); order('name')orders like on Hibernate 5. - The criterion returns no predicate (
null, whichPredicateGeneratorfilters out), so it no longer changes the result of anor {}ornot {}it's called in. - The reliance on
assignOrderByrunning beforeassignCriteriais documented inOrderBySqlFormulaand the README.
New tests in PgOrderIntegrationSpec (run on both apps): a formula before and after a property order, pagination with list(max:, offset:) and totalCount, and a formula inside or {}. The first test would catch GORM swapping the two steps. Asked upstream for a way to contribute orders in apache/grails-core#16563.
|
|
||
| private static PgCriterion valueFunction(String propertyName, String sqlFunction, Object value) { | ||
| new PgCriterion(propertyName, { AbstractQuery<?> query, From<?, ?> root, JpaCriteriaBuilder cb, Path<?> property -> | ||
| sql(cb, sqlFunction, property, cb.value(String.valueOf(value))) |
There was a problem hiding this comment.
String.valueOf(null) binds the string 'null', whereas Hibernate 5 bound SQL NULL. Same in JsonCriterias.
There was a problem hiding this comment.
Fixed in 9b34ee8: values are bound through PgCriterion.text(), which binds SQL NULL for null, as Hibernate 5 did. Covered by a new test in PgHstoreContainsKeyIntegrationSpec.
| private static PgCriterion jsonExpression(String propertyName, String jsonOp, String jsonAttribute, String sqlOp, Object value) { | ||
| String attribute = jsonAttribute.replace("'", "''") | ||
| new PgCriterion(propertyName, { AbstractQuery<?> query, From<?, ?> root, JpaCriteriaBuilder cb, Path<?> property -> | ||
| sql(cb, "?$jsonOp'$attribute' $sqlOp ?", property, cb.value(String.valueOf(value))) |
There was a problem hiding this comment.
pgJsonHasFieldValue('data', 'name', null) will now match rows where the value is the text "null", instead of nothing as on Hibernate 5.
There was a problem hiding this comment.
Fixed in 9b34ee8, same as the hstore one. PgJsonValuesIntegrationSpec now checks that a null value matches nothing, not even a stored "null".
| // Must be provided by the Grails application | ||
| } | ||
|
|
||
| api 'com.google.code.gson:gson' |
There was a problem hiding this comment.
Gson isn't part of the public API here (only used inside JsonMapType/JsonCriterias), and it's implementation in the hibernate5 module. Should this be implementation too?
There was a problem hiding this comment.
Yes. It was api because Groovy loads the extension classes when it compiles the application, and JsonCriterias had a static Gson field. The criteria now use Gson through a small JsonUtils helper, so it's implementation again (9b34ee8).
| import org.hibernate.boot.model.TypeContributions | ||
| import org.hibernate.dialect.PostgreSQLDialect | ||
| import org.hibernate.service.ServiceRegistry | ||
| import org.hibernate.type.descriptor.sql.internal.DdlTypeImpl |
There was a problem hiding this comment.
Nit: DdlTypeImpl is in Hibernate's internal package, so it may break on a Hibernate minor update. Probably fine for now, but worth keeping in mind.
There was a problem hiding this comment.
Replaced it with a small DdlType implementation in the dialect, so it only uses Hibernate's public SPI (e095270).
| return mapFunction.map(o) | ||
| } | ||
| throw new HibernateException("criteria doesn't support values of type: ${o?.class?.name}. Try: $expectedType or List<$expectedType> instead") | ||
| throw new IllegalArgumentException("criteria doesn't support values of type: ${o?.class?.name}. Try: $expectedType or List<$expectedType> instead") |
There was a problem hiding this comment.
This changes the exception type for anyone calling PgArrayUtils directly (HibernateException -> IllegalArgumentException). Fine by me, but worth a line in the release notes.
There was a problem hiding this comment.
Added an "Upgrading to 9.0.0" section to the README that mentions it (e095270), and I'll put it in the release notes when 9.0.0 is published.
| implementation 'org.apache.grails:grails-dependencies-starter-web' | ||
|
|
||
| integrationTestImplementation 'org.apache.grails:grails-dependencies-test' | ||
| integrationTestRuntimeOnly 'org.testcontainers:testcontainers-postgresql' |
There was a problem hiding this comment.
test-app2's domain classes, services and integration specs are identical copies of test-app1 (~3k lines). Could we share at least the integration specs, e.g. via sourceSets.integrationTest.groovy.srcDir pointing at app1, so the two don't drift apart?
There was a problem hiding this comment.
Done in 11d3f4f: test-app2 now compiles test-app1's domain classes, services, init class and integration specs through extra source dirs, and keeps only its own config. About 3,900 lines removed, and both apps run the same 633 tests.
| and queries of an application do not change when it moves from Hibernate 5 to Hibernate 7. | ||
|
|
||
| > [!NOTE] | ||
| > With GORM 8.0.0 for Hibernate 7, the default `native` id generator does not create its sequences on PostgreSQL. |
There was a problem hiding this comment.
Could you file the GrailsNativeGenerator issue with Apache Grails (its constructor swallows the initialize exception) and link it here and in test-app2's application.groovy?
There was a problem hiding this comment.
Filed as apache/grails-core#16561, and linked in the README and in test-app2's application.groovy (e095270).
test-app2 compiles the domain classes, services, init class and integration specs of test-app1 instead of keeping copies, so the Hibernate 5 and Hibernate 7 test apps run the same specs and cannot drift apart.
- A criterion on an alias with an inner join makes GORM create the join of the alias first and uses it, so it applies to the same join as GORM criteria on the alias, in any order. For an alias with an outer join it uses a join with that join type, as GORM 8.0.0 always joins an alias with an inner join (apache/grails-core#16562). - An order by an SQL formula keeps its position among the property orders, and adds no predicate, so it does not change the result of an or {} or not {} it is called in. - Null values are bound as SQL NULL instead of the text 'null', like Hibernate 5. - Gson is an implementation dependency again: the criteria use it through JsonUtils, so the extension classes Groovy loads when compiling an application do not reference it. Add tests for criteria on an alias of a collection and of an outer join, for formula orders combined with property orders, pagination and or {}, and for null values.
- Register the column types of the Hibernate 7 dialect with an own DdlType instead of Hibernate's internal DdlTypeImpl - Add Søren Berg Glasius to the developers of both plugin descriptors - Remove the Dockerfile, as the test apps run PostgreSQL with Testcontainers - Document upgrading to 9.0.0 and the Hibernate 7 limitations, and link the GORM issues
|
Thanks for the review @matrei! All comments are addressed in 11d3f4f, 9b34ee8 and e095270, with replies inline. The two general points:
I also filed the GORM issues found along the way: apache/grails-core#16561 ( |
matrei
left a comment
There was a problem hiding this comment.
Thanks for the thorough follow-up, and for filing the upstream issues! All my comments are addressed, LGTM. Two small nits inline, not blocking, fine as follow-ups.
| if (criterion.joinType != JoinType.INNER) { | ||
| return addToCriteria(self, criterion) | ||
| } | ||
| String identity = "${association.alias}.${association.association.associatedEntity.identity.name}" |
There was a problem hiding this comment.
Nit: for an associated entity with a composite id, identity is null, so this would NPE. Using the criterion's own property for the always-true restriction (e.g. l.favoriteNumbers is null or l.favoriteNumbers is not null) would avoid the identity lookup altogether.
There was a problem hiding this comment.
Good point, fixed in edcab87: the always-true restriction now uses the criterion's own property (l.favoriteNumbers is null or l.favoriteNumbers is not null), so there's no identity lookup. GORM still resolves the alias join from it, and the alias specs pass on both apps.
| criteriaQuery.orderBy(orders) | ||
| } | ||
| // No predicate, so the order does not change the result of an or {} or not {} it is called in | ||
| null |
There was a problem hiding this comment.
Nit: with no predicate, an or {} that contains only an order becomes an empty disjunction, which JPA treats as false, so or { order(byRandom()) } returns no rows. Unlikely in practice, so maybe just a line in the docs.
There was a problem hiding this comment.
Agreed, documented in edcab87: there's now a line about it in the README's Hibernate 7 limitations, plus a comment in OrderBySqlFormula. The handler can't tell the order is alone in the junction, so I left the behaviour as is.
- Resolve an inner join alias with a restriction on the property of the criterion
instead of on the identity of the associated entity, which is null for a composite id
- Document that an or {} with only an order by an SQL formula matches no rows
Fix #135
Upgrades the plugin to Grails 8.0.0 (Java 21, Gradle 9.8.0) and adds a Hibernate 7 version of the plugin next to the Hibernate 5 one.
Changes
Grails 8 upgrade: Grails 8.0.0, Java 21, Gradle 9.8.0, version 9.0.0-SNAPSHOT. CI and release workflows build with Java 21.
Fix:
PgArrayExpressionchecksinstanceof Enumexplicitly. In Grails 8, Groovy turns aString→Enumcast intoEnum.valueOf(Enum, ...), which throwsIllegalArgumentExceptioninstead ofClassCastException, so invalid enum values no longer produced the expectedHibernateException.Module split:
plugin-coregrails-postgresql-extensions-core: hstore parsing,BidiEnumMap,PgArrayUtils; no Hibernate dependencyplugin-hibernate5grails-postgresql-extensions: the existing plugin, with unchanged coordinatesplugin-hibernate7grails-postgresql-extensions-hibernate7: newBoth plugins use the same packages and class names, so applications keep their domain classes and queries when they switch.
Hibernate 7 port:
UserTypeAPI. The dialect registers the column types of the custom SQL type codes with theDdlTypeRegistry.pg*criteria are now GORM criteria that GORM'sPredicateGenerator.CriterionHandlerProviderSPI turns into JPA predicates. They render through a self-rendering SQL function and bind values as parameters (hstore values used to be inlined in the SQL).sqlFormula()andbyRandom()add a criterion that appends the formula to theORDER BYof entity queries. This is a workaround until GORM has a hook for ordering by SQL.test-app2: a copy of test-app1 on Hibernate 7, running the same integration specs.
Testcontainers: each test app starts its own PostgreSQL container through the
jdbc:tc:URL. Before, the apps ran in parallel against one shared database and interfered with each other's data. The CI test job now runs directly on the runner, without the postgres service or the psql setup steps.GORM 8.0.0 issue
With Hibernate 7 on PostgreSQL, the default
nativeid generator never creates its sequences:GrailsNativeGeneratorswallows the initialisation of itsSequenceStyleGeneratordelegate. test-app2 and the README work around it withid generator: 'sequence'. This should be reported to Apache Grails.Testing
./gradlew checkpasses locally: