jdaugherty commented on code in PR #16733:
URL: https://github.com/apache/grails-core/pull/16733#discussion_r4352272438
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/PredicateGenerator.java:
##########
@@ -119,13 +235,68 @@ public interface CriterionHandlerProvider {
public static final Map<Class<? extends Query.Criterion>,
CriterionHandler> CUSTOM_HANDLERS =
new ConcurrentHashMap<>();
- private static final AtomicBoolean SERVICE_LOADERS_INITIALIZED = new
AtomicBoolean(false);
+ public static final Map<Class<? extends Query.Order>, OrderHandler>
CUSTOM_ORDER_HANDLERS =
Review Comment:
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.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/PredicateGenerator.java:
##########
@@ -90,6 +90,122 @@ Predicate handle(
HibernateCriteriaBuilder criteriaBuilder,
Query.Criterion criterion
);
+
+ /**
+ * Converts a criterion with the context of the query it belongs to.
The context resolves a property path
+ * the way the built-in criteria do: {@link
QueryPathContext#getFullyQualifiedPath(String)} turns
+ * {@code alias.property}, for an alias created with {@code
createAlias}, or {@code association.property}
+ * into a path on the join GORM created for that association, so a
custom criterion and a GORM criterion on
+ * the same association restrict the same row instead of each joining
the association again. This default
+ * ignores the context and calls {@link #handle(AbstractQuery, From,
HibernateCriteriaBuilder, Query.Criterion)};
+ * a handler that needs the context implements {@link
ContextualCriterionHandler}.
+ *
+ * @param criteriaQuery the query or subquery the predicate is for
+ * @param root the root of that query
+ * @param criteriaBuilder the criteria builder
+ * @param criterion the criterion to convert
+ * @param context the context of the query: its root, its joins and
its aliases
+ * @return the predicate, or {@code null} to add none
+ * @since 8.1
+ */
+ default Predicate handle(
+ AbstractQuery<?> criteriaQuery,
+ From<?, ?> root,
+ HibernateCriteriaBuilder criteriaBuilder,
+ Query.Criterion criterion,
+ QueryPathContext context
+ ) {
+ return handle(criteriaQuery, root, criteriaBuilder, criterion);
+ }
+ }
+
+ /**
+ * A {@link CriterionHandler} that is given the context of the query,
through which it resolves property paths
+ * with {@link QueryPathContext#getFullyQualifiedPath(String)}. A closure
with five parameters is coerced to it:
Review Comment:
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:
```groovy
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.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/QueryPathContext.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * Copyright 2024-2025 the original author or authors.
Review Comment:
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.
##########
grails-data-hibernate7/core/src/test/groovy/grails/orm/HibernateCriteriaBuilderDirectSpec.groovy:
##########
@@ -389,6 +390,129 @@ class HibernateCriteriaBuilderDirectSpec extends
HibernateGormDatastoreSpec {
cleanup:
PredicateGenerator.clearCustomCriterionHandlers()
}
+
+ void "a count query leaves the criteria's orders out"() {
+ expect: "a count with an order counts every row instead of failing on
the order by"
+ new DetachedCriteria(CriteriaTestEntity).build { order('amount',
'desc') }.count() == 4L
+ new DetachedCriteria(CriteriaTestEntity).build {
+ gt('amount', 15)
+ order('name')
+ }.count() == 3L
+
+ and: "a row count projection with an order returns the count"
+ c.list {
+ projections { rowCount() }
+ order('amount')
+ } == [4L]
+
+ and: "the builder's count with an order counts every row"
+ new HibernateCriteriaBuilder(CriteriaTestEntity,
manager.hibernateDatastore.sessionFactory, manager.hibernateDatastore)
+ .count { order('amount', 'desc') } == 4L
+
+ and: "the order still applies to the list"
+ new DetachedCriteria(CriteriaTestEntity).build { order('amount',
'desc') }.list()*.name == ['D', 'C', 'B', 'A']
+ }
+
+ void "a contextual criterion handler resolves an association path to the
join GORM created for it"() {
+ given: "a parent with two children, and a handler that resolves its
criterion's path through the context"
+ var parent = CriteriaTestEntity.findByName('A')
+ parent.addToChildren(new CriteriaTestChild(name: 'Kid'))
+ parent.addToChildren(new CriteriaTestChild(name: 'Other')).save(flush:
true)
+ PredicateGenerator.registerCriterionHandler(CriteriaTestPathLike, {
query, root, cb, criterion, context ->
+ def c = criterion as CriteriaTestPathLike
+ cb.like(context.getFullyQualifiedPath(c.property) as
Expression<String>, c.value as String)
+ } as PredicateGenerator.ContextualCriterionHandler)
+
+ when: "a GORM criterion and the custom criterion restrict the same
association"
+ List<CriteriaTestEntity> both = new
DetachedCriteria(CriteriaTestEntity).build {
+ children { eq('name', 'Kid') }
+ add(new CriteriaTestPathLike('children.name', 'K%'))
+ }.list()
+ List<CriteriaTestEntity> different = new
DetachedCriteria(CriteriaTestEntity).build {
+ children { eq('name', 'Kid') }
+ add(new CriteriaTestPathLike('children.name', 'O%'))
+ }.list()
+
+ then: "a child matching both criteria is found"
+ both*.name == ['A']
+
+ and: "two children matching one criterion each are not: the handler
used the join GORM created, not a second one"
+ different.empty
+
+ when: "the custom criterion comes before the association block"
Review Comment:
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(...)`):
```groovy
createAlias('children', 'ch')
eq('ch.name', 'Kid')
add(new CriteriaTestPathLike('ch.name', 'O%')) // [] ; with 'K%': ['A']
```
Could you add that case here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
