jdaugherty commented on code in PR #16696:
URL: https://github.com/apache/grails-core/pull/16696#discussion_r4330729582
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/PredicateGenerator.groovy:
##########
@@ -84,6 +85,72 @@ class PredicateGenerator {
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
JpaQueryContext#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 10.0
+ */
+ default Predicate handle(
+ AbstractQuery<?> criteriaQuery,
+ From<?, ?> root,
+ HibernateCriteriaBuilder criteriaBuilder,
+ Query.Criterion criterion,
+ JpaQueryContext context
Review Comment:
Handing handlers the `JpaQueryContext` itself makes the whole class a plugin
API. A handler can call `addFrom`, `registerAlias`, `setRoot`, `setParent`,
`bindParameter` and `clone`, and the docs now point plugin authors at it, so
the class can no longer change without breaking them. Its own javadoc describes
it as the translation state of the query.
The issue asked for the context or a resolver for property paths. Would a
small public interface that `JpaQueryContext` implements work here, with
`getFullyQualifiedPath(String)` and whatever else a handler really needs?
`CriterionHandler`, `ContextualCriterionHandler`, `OrderHandler` and the guide
would then depend on that interface only.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/JpaCriteriaQueryCreator.groovy:
##########
@@ -287,6 +287,16 @@ class JpaCriteriaQueryCreator<T> {
if (!orders.isEmpty()) {
List<jakarta.persistence.criteria.Order> jpaOrders = [];
for (Query.Order order : orders) {
+ PredicateGenerator.OrderHandler customHandler =
PredicateGenerator.orderHandler(order.class);
+ if (customHandler != null) {
+ // the handler of a Query.Order subclass builds its JPA
order, or leaves the order out
+ jakarta.persistence.criteria.Order customOrder =
+ customHandler.handle(cq, context.root,
criteriaBuilder, order, context);
Review Comment:
`count()` keeps the criteria's orders, so this loop also runs for count
queries. `DetachedCriteria.count()` and `createCriteria().count {}` go through
`HibernateQuery.countResults()`, which builds the query with `createQuery()`
and so with `assignOrderBy`. On H2 that fails for any order. With an order
handler:
```
Column "CTE1_0.AMOUNT" must be in the GROUP BY list
select count(cte1_0.id) from criteria_test_entity cte1_0 order by
(cte1_0.amount%cast(? as integer)) desc
```
A plain `order('amount')` fails the same way (`... order by cte1_0.amount`),
so this predates the PR and is on 8.0.x too. Only the paged-list total escapes
it, because `HibernatePagedResultList` calls `clearOrders()` first.
The new docs and the `OrderHandler` javadoc ask each handler to return
`null` on a count. Neither handler in the docs nor
`PriceDistanceOrderHandlerProvider` does, so `new
DetachedCriteria(Product).build { orderByPriceDistance 10.00 }.count()` throws.
I think the framework should leave the orders out of a count query instead,
which fixes property orders as well. Since 8.0.x has the bug, could that fix
land there first? This PR could then drop the count advice from the docs and
the javadoc, and add a `count()` case with an order handler next to the list
cases.
##########
grails-data-hibernate7/core/src/test/groovy/grails/orm/HibernateCriteriaBuilderDirectSpec.groovy:
##########
@@ -389,6 +390,81 @@ class HibernateCriteriaBuilderDirectSpec extends
HibernateGormDatastoreSpec {
cleanup:
PredicateGenerator.clearCustomCriterionHandlers()
}
+
+ 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 {
Review Comment:
Both queries put `children { }` before the custom criterion, so the handler
always finds the join the association block already made, through
`joinTracker.getJoin` in `ExpressionResolver.resolveFromAlias`. When the custom
criterion comes first, the handler's `getFullyQualifiedPath` call creates the
join instead, from the alias that alias discovery registered, and the
association block then reuses it. That branch works: I probed it locally and
got `['A']` and an empty list, as here. No test covers it, though. Could you
add the reversed order to this feature?
--
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]
