[borinquenkid]

Summary

Resolves #16563 on 8.x. A plugin or an application can extend the Hibernate 7 criteria builder with its own criteria and orders:

• A criterion handler can take the query's QueryPathContext (a new small interface that JpaQueryContext implements: getRoot(), getFullyQualifiedPath(String), getFullyQualifiedExpression(String)). Through it a handler resolves alias.property or association.property to the join GORM created for the association, so a custom criterion and a built-in one on the same association restrict the same row instead of each joining the association again. A four-argument handler keeps working; ContextualCriterionHandler is the five-argument form, and a closure with five parameters is coerced to it.
• An OrderHandler turns a Query.Order subclass into a JPA order by any expression, in its place among the query's orders, registered in code or through a META-INF/services OrderHandlerProvider.
• Handler discovery waits for the providers: loadServiceProviders loads under a lock and publishes its flag only after both ServiceLoader loops, so a query that arrives during the discovery no longer decides, with the handlers still unregistered, that an order has no handler. A handler registered in code stays ahead of a discovered one.
• A count query leaves its orders out, handled ones too (the fix of Hibernate 7: leave the criteria's orders out of a count query #16731, included here as the same commit so this branch tests against it; the merge-up reconciles them).

This is the 8.1.x version of #16696, which was reviewed on the Java-to-Groovy Hibernate 7 branch and answers that review: the interface instead of the context class, the discovery race, the count query, the reversed-order and DetachedCriteria.list() coverage, and the restored workflow triggers. #16696 becomes the merge-up of this pull request onto that branch.

Documented in the GORM for Hibernate 7 guide (Extending the Criteria Builder) and the what's new of Grails 8; the criteria-extension example application exercises both handler kinds through providers.

Verification

• HibernateCriteriaBuilderDirectSpec: a contextual handler on an association path, before and after the association block; an order handler through the builder and through DetachedCriteria.list(); a count() with a handled order; a handler returning null.
• OrderHandlerDiscoverySpec: one thread blocked inside a provider found through META-INF/services, a second lookup waits and receives the handler; fails with the previous flag.
• PredicateGeneratorSpec, JpaCriteriaQueryCreatorSpec: the translator-level cases.
• :grails-data-hibernate7-core:test, checkstyleMain, codenarcTest: see the CI run.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.3534%. Comparing base (8910a5c) to head (a46a5c9).
⚠️ Report is 93 commits behind head on 8.1.x.

Additional details and impacted files

@@                Coverage Diff                 @@
##                8.1.x     #16733        +/-   ##
==================================================
+ Coverage     63.3231%   63.3534%   +0.0303%     
- Complexity      28459      28510        +51     
==================================================
  Files            2311       2314         +3     
  Lines          113600     113683        +83     
  Branches        20675      20686        +11     
==================================================
+ Hits            71935      72022        +87     
+ Misses          32242      32238         -4     
  Partials         9423       9423                

see 10 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jdaugherty]

Re-checked at 66f1f26 against 8.1.x. The four changed specs pass locally on JDK 21 (101 tests), and the asks from #16696 are all in: QueryPathContext, discovery that waits for the providers, the reversed-order case, and count queries leaving their orders out. I also ran the guide's ByRandom order, whose property name is not a real property, through the builder, DetachedCriteria.list(), a paged list and count(). All four work.

On landing order: this branch carries #16731 as a cherry-pick (83b6c5d). If this PR merges first, the 8.0.x to 8.1.x merge-up of #16731 conflicts in JpaCriteriaQueryCreator (the comment line this PR extends with "handled ones too") and in HibernateCriteriaBuilderDirectSpec (the count feature next to the new ones). I checked with git merge-tree. If #16731 lands first and 8.0.x is merged up, a rebase of this branch drops the cherry-pick as already applied, and the second commit applies cleanly on top.

[jdaugherty]

A five-parameter closure only becomes a ContextualCriterionHandler with the explicit as. Without it, Groovy coerces the closure to CriterionHandler, and the query then fails at run time:

PredicateGenerator.registerCriterionHandler(CriteriaTestPathLike) { query, root, cb, criterion, context ->
    cb.like(context.getFullyQualifiedPath(criterion.property) as Expression<String>, criterion.value as String)
}
new DetachedCriteria(CriteriaTestEntity).build { add(new CriteriaTestPathLike('name', 'A%')) }.list()

groovy.lang.MissingMethodException: No signature of method: doCall ... is applicable for argument types:
(SqmSelectStatement, SqmRoot, SqmCriteriaNodeBuilder...)

This is easy to hit because registerOrderHandler takes the same closure shape without the as, and the guide shows it that way. The two registrations look alike, but only the order one works.

Could registerCriterionHandler get a Closure overload that wraps the closure as a ContextualCriterionHandler when getMaximumNumberOfParameters() is 5, and as a CriterionHandler otherwise? I checked on Groovy 5.1.2: Groovy chooses a Closure parameter over SAM coercion, both dynamically and under @CompileStatic, and an explicit as ContextualCriterionHandler still picks the existing overload. With that overload both shapes work without the as. If you'd rather keep the API as it is, please say in this javadoc and in the guide that the as is required and what happens without it.

[borinquenkid]

Done in 54a4454: registerCriterionHandler(Class, Closure) registers a closure with five parameters as a ContextualCriterionHandler and any other closure as a CriterionHandler, so both shapes work without as, the way the order handler does. An explicit as ContextualCriterionHandler still picks the existing overload. The javadoc and the guide drop the as; the Direct spec registers a four and a five parameter closure without it.

[jdaugherty]

Does this map need to be public? registerOrderHandler, orderHandler and clearCustomOrderHandlers already cover registration, lookup and cleanup. Code that reads the map directly skips loadServiceProviders(), so it can find no handler before discovery has run, which is the situation the new lock prevents. This is new API in 8.1, so making the map private keeps it out of the public contract. CUSTOM_HANDLERS is already public, so I'd leave that one alone.

[borinquenkid]

Done in 54a4454: CUSTOM_ORDER_HANDLERS is private. Nothing outside PredicateGenerator read it; registerOrderHandler, orderHandler and clearCustomOrderHandlers cover registration, lookup and cleanup.

[jdaugherty]

This is a new file, so it should have the ASF source header, as OrderHandlerDiscoverySpec and the new example classes in this PR do, not the "original author or authors" header. RAT accepts both (the existing files in this package that use the old header pass it), so CI won't flag it.

[borinquenkid]

Done in 54a4454: QueryPathContext has the ASF header.

[jdaugherty]

The first case in #16563 is an alias from createAlias, and the guide and the QueryPathContext javadoc mention it first, but this feature only tests association paths. A createAlias alias reaches the registry differently: a HibernateAlias is registered up front, while the association block's alias comes from alias discovery. I probed it, and the alias shares the join both in a detached criteria and in the builder (through hibernateQuery.add(...)):

createAlias('children', 'ch')
eq('ch.name', 'Kid')
add(new CriteriaTestPathLike('ch.name', 'O%'))   // [] ; with 'K%': ['A']

Could you add that case here?

[borinquenkid]

Added in 54a4454, as "a contextual criterion handler resolves the alias of createAlias to the join GORM created for it": the createAlias('children', 'ch') alias shares the join with eq('ch.name', 'Kid'), in a detached criteria and in the builder (through hibernateQuery.add(...)); K% finds the parent and O% finds none.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

3 open issues

• The sentence “the maps keep the first registration” is misleading now that… · New
• CountDownLatch#await() can throw InterruptedException. In this test provider, an interrupt… · New
• This newly added ServiceLoader provider file lacks the Apache license header comment that is… · New

_{Review effort: Lite}

[borinquenkid]

Reworded in 54a4454: discovery uses putIfAbsent, so a discovered handler does not replace one that is already registered, and registering in code uses put, so it replaces a discovered one.

[borinquenkid]

Done in 54a4454: the test provider catches the InterruptedException, restores the interrupt flag and fails fast with a clear message.

[borinquenkid]

Done in 54a4454: the example's order provider file has the ASF header, as the test resource's has.

[jamesfredley]

Not approving. The discovery race and the CHANGEME-FOR-ACCEPTANCE filter are not in this port. A custom order still cannot see a sqlProjection alias.

[borinquenkid]

Pushed 1e47f7e, from jamesfredley's review on #16696: QueryPathContext now exposes getSelectionAlias(String), so an order handler can order by the expression a sqlProjection records under its column alias; predicate property resolution is unchanged. HibernateCriteriaBuilderSqlProjectionSpec orders by a sqlProjection alias through a handler and asserts the rows come in the projection's order and the SQL orders by the select item (order by 1 desc), not by a column. The guide names the accessor.

[jamesfredley]

Not approving. getSelectionAlias is new, but the lookup the earlier comment named still misses the projection.

[jamesfredley]

getFullyQualifiedExpression('total') still asks only the property resolver. The sqlProjection alias is recorded with registerSelectionAlias, so this call returns null, or an entity property named total. The context javadoc says this method accepts the alias of a projection.

Return the recorded selection when the path is that alias. Please order through getFullyQualifiedExpression('total'), not only getSelectionAlias.

[jdaugherty]

I probed this at 1e47f7e to see what each lookup returns for a sqlProjection alias, with width * 10 as scaled where no scaled property exists and width * 10 as height where height is a property:

alias	getFullyQualifiedExpression	getSelectionAlias	built-in order(alias)	built-in eq(alias, …)
scaled	null; cb.desc(null) then fails with "Ambiguous method overloading for SqmCriteriaNodeBuilder#desc"	the projection	order by 1 desc	"Cannot use comparison criteria on non-existent property [scaled]"
height	the property, order by spb1_0.height desc	the projection	order by 1 desc	where spb1_0.height=?

For sqlGroupProjection 'width, sum(height) as total', a handler using getSelectionAlias('total') gives order by 2 desc, the same SQL as order('total', 'desc').

The two built-in columns of that table are not an accident of the translator. They are the rule SQL, Hibernate 5 and this module already follow:

• SQL scopes a select alias to ORDER BY, where it wins over a column of the same name, and never to WHERE. On H2 2.5.250, the test database: select width * 10 as height from t order by height desc returns 40, 20; ... where height = 9 restricts the column; ... where scaled = 20 fails with Column "SCALED" not found.
• Hibernate 5's Criteria API, which the plugin in Hibernate 7: let a CriterionHandler resolve property paths and order by expressions #16563 is porting from, does the same. Order.toSqlString resolves through CriteriaQueryTranslator.getColumnsUsingProjection, which first looks for a projection alias and then for a property; SimpleExpression.toSqlString, behind eq and the other restrictions, resolves through findColumns, which looks for a property only (hibernate-core 5.6.11).
• This module encoded it when sqlProjection landed: assignOrderBy checks the sqlProjection alias before the property, and registerSelectionAlias keeps the alias out of property resolution, with "Column aliases never shadow properties in restrictions" in cb1f659 (Hibernate 7: support sqlProjection and sqlGroupProjection in criteria queries #16548).

Folding the selection into getFullyQualifiedExpression changes that behavior, because the built-in predicates resolve through the same method and assignCriteria runs after the projections are registered. Selection first makes eq('height', 9) restrict the projection instead of the column on every query whose projection alias matches a property, which is the shadowing the commit above rules out and SQL does not do. Selection as a fallback keeps eq as it is, but then a custom order resolves height to the property where order('height') resolves it to the projection, so the built-in order and the handler order the same alias differently.

Two lookups avoid both. An order handler that checks getSelectionAlias first and getFullyQualifiedExpression second orders exactly as order(alias) and as Hibernate 5's Order do, and a criterion handler that uses getFullyQualifiedExpression alone restricts exactly as eq does. So I would keep the accessor this commit adds rather than change the resolver.

What is wrong at this commit is the contract. The QueryPathContext.getFullyQualifiedExpression javadoc says it takes "the alias of a projection" and returns "the expression an alias or projection stands for". That holds for an alias the translator registers with registerAlias and not for the column alias of a sqlProjection, which is where this thread started. Could the javadoc say which aliases it resolves and point to getSelectionAlias for the column alias of a sqlProjection or sqlGroupProjection, and could the guide's order-handler paragraph say to check getSelectionAlias first, the way order(alias) does?

[borinquenkid]

Same change as the thread above (54a4454): the path resolves to the recorded selection when no property has the name, and the order goes through getFullyQualifiedExpression('total') in a spec of its own.

[borinquenkid]

Reverted in 3bdd024, on jdaugherty's analysis in his reply to your second comment: getFullyQualifiedExpression resolves as the built-in predicates do, so a projection alias is not folded into it. With width * 10 as height, resolving the alias there would make eq('height', 9) restrict the projection instead of the column, which SQL does not do (a select alias is visible to order by, not to where) and neither does Hibernate 5. Your first request, to expose a selection lookup and leave predicate resolution alone, is what getSelectionAlias is. An order handler asks it first and getFullyQualifiedExpression second, and then orders exactly as order(alias) does, for scaled (no such property) and for height (a property). The javadoc and the guide say so, and HibernateCriteriaBuilderSqlProjectionSpec pins it, including that eq('height', 9) restricts the property.

[borinquenkid]

Done in 3bdd024: the resolver is back to what it was, and QueryPathContext.getFullyQualifiedExpression documents which aliases it resolves and points to getSelectionAlias for the column alias of a sqlProjection or sqlGroupProjection. The guide's order-handler paragraph says to ask getSelectionAlias first and getFullyQualifiedExpression second, and that a criterion handler asks the second alone. The new feature in HibernateCriteriaBuilderSqlProjectionSpec runs your two cases (scaled, which no property has, and height, which one has) and the eq('height', 9) that must restrict the column.

[jdaugherty]

Re-checked at 1e47f7e. The new feature in HibernateCriteriaBuilderSqlProjectionSpec passes locally with the other four changed specs (126 tests), and getSelectionAlias covers the total case of a sqlGroupProjection as well: a handler ordering by it produces order by 2 desc, the SQL that order('total', 'desc') produces. My view on the open thread about getFullyQualifiedExpression is in that thread; what I would still change there is the javadoc and the guide sentence, not the resolver.

The four threads from my first round are still open at this commit: the Closure overload for registerCriterionHandler, CUSTOM_ORDER_HANDLERS being public, the header of QueryPathContext.java, and a createAlias case in HibernateCriteriaBuilderDirectSpec.

On landing: #16731 merged on 8.0.x today, so the cherry-pick here is now upstream. Once 8.0.x is merged up into 8.1.x, a rebase of this branch drops it ("patch contents already upstream"; I tried it on a simulated merge-up) and the two feature commits apply cleanly. A merge of 8.1.x into the branch conflicts instead, in JpaCriteriaQueryCreator and HibernateCriteriaBuilderDirectSpec, so rebase rather than merge.

CI at this commit: the main build is green. The red shards are the Groovy snapshot canary, failing in the MongoDB modules (#16030), unrelated to this change.

[jdaugherty]

I probed this at 1e47f7e to see what each lookup returns for a sqlProjection alias, with width * 10 as scaled where no scaled property exists and width * 10 as height where height is a property:

alias	getFullyQualifiedExpression	getSelectionAlias	built-in order(alias)	built-in eq(alias, …)
scaled	null; cb.desc(null) then fails with "Ambiguous method overloading for SqmCriteriaNodeBuilder#desc"	the projection	order by 1 desc	"Cannot use comparison criteria on non-existent property [scaled]"
height	the property, order by spb1_0.height desc	the projection	order by 1 desc	where spb1_0.height=?

For sqlGroupProjection 'width, sum(height) as total', a handler using getSelectionAlias('total') gives order by 2 desc, the same SQL as order('total', 'desc').

The two built-in columns of that table are not an accident of the translator. They are the rule SQL, Hibernate 5 and this module already follow:

• SQL scopes a select alias to ORDER BY, where it wins over a column of the same name, and never to WHERE. On H2 2.5.250, the test database: select width * 10 as height from t order by height desc returns 40, 20; ... where height = 9 restricts the column; ... where scaled = 20 fails with Column "SCALED" not found.
• Hibernate 5's Criteria API, which the plugin in Hibernate 7: let a CriterionHandler resolve property paths and order by expressions #16563 is porting from, does the same. Order.toSqlString resolves through CriteriaQueryTranslator.getColumnsUsingProjection, which first looks for a projection alias and then for a property; SimpleExpression.toSqlString, behind eq and the other restrictions, resolves through findColumns, which looks for a property only (hibernate-core 5.6.11).
• This module encoded it when sqlProjection landed: assignOrderBy checks the sqlProjection alias before the property, and registerSelectionAlias keeps the alias out of property resolution, with "Column aliases never shadow properties in restrictions" in cb1f659 (Hibernate 7: support sqlProjection and sqlGroupProjection in criteria queries #16548).

Folding the selection into getFullyQualifiedExpression changes that behavior, because the built-in predicates resolve through the same method and assignCriteria runs after the projections are registered. Selection first makes eq('height', 9) restrict the projection instead of the column on every query whose projection alias matches a property, which is the shadowing the commit above rules out and SQL does not do. Selection as a fallback keeps eq as it is, but then a custom order resolves height to the property where order('height') resolves it to the projection, so the built-in order and the handler order the same alias differently.

Two lookups avoid both. An order handler that checks getSelectionAlias first and getFullyQualifiedExpression second orders exactly as order(alias) and as Hibernate 5's Order do, and a criterion handler that uses getFullyQualifiedExpression alone restricts exactly as eq does. So I would keep the accessor this commit adds rather than change the resolver.

What is wrong at this commit is the contract. The QueryPathContext.getFullyQualifiedExpression javadoc says it takes "the alias of a projection" and returns "the expression an alias or projection stands for". That holds for an alias the translator registers with registerAlias and not for the column alias of a sqlProjection, which is where this thread started. Could the javadoc say which aliases it resolves and point to getSelectionAlias for the column alias of a sqlProjection or sqlGroupProjection, and could the guide's order-handler paragraph say to check getSelectionAlias first, the way order(alias) does?

[jamesfredley]

Approving. getFullyQualifiedExpression('total') now falls back to the registered sqlProjection expression when total is not a mapped property. The earlier comment is resolved.

[testlens-app]

🔎 No tests executed 🔎

🏷️ Commit: 591bde1
▶️ Tests: 0 executed
⚪️ Checks: 49/49 completed

---

Learn more about TestLens at testlens.app/docs.