borinquenkid commented on code in PR #16733:
URL: https://github.com/apache/grails-core/pull/16733#discussion_r4361018266
##########
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:
Done in 54a44543b2: `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.
##########
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:
Done in 54a44543b2: `CUSTOM_ORDER_HANDLERS` is private. Nothing outside
`PredicateGenerator` read it; `registerOrderHandler`, `orderHandler` and
`clearCustomOrderHandlers` cover registration, lookup and cleanup.
##########
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:
Done in 54a44543b2: `QueryPathContext` has the ASF header.
##########
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:
Added in 54a44543b2, 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.
##########
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 =
+ new ConcurrentHashMap<>();
+
+ private static final Object SERVICE_LOADER_LOCK = new Object();
+ private static volatile boolean serviceLoadersInitialized = false;
+
+ /**
+ * Loads the handler providers once. A caller that arrives while another
thread is loading them waits until the
+ * loading is complete, so no query decides on a handler before the
discovered ones are registered. A discovered
+ * handler never replaces one registered in code: the maps keep the first
registration.
Review Comment:
Reworded in 54a44543b2: 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.
##########
grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/hibernatequery/OrderHandlerDiscoverySpec.groovy:
##########
@@ -0,0 +1,115 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package grails.gorm.tests.hibernatequery
+
+import java.util.concurrent.Callable
+import java.util.concurrent.CountDownLatch
+import java.util.concurrent.ExecutorService
+import java.util.concurrent.Executors
+import java.util.concurrent.Future
+import java.util.concurrent.TimeUnit
+import java.util.concurrent.TimeoutException
+
+import spock.lang.Specification
+
+import org.grails.datastore.mapping.query.Query
+import org.grails.orm.hibernate.query.PredicateGenerator
+
+/**
+ * The handler providers are discovered on first use. A query that arrives
while another thread discovers them must
+ * wait for the discovery instead of deciding, with the handlers still
unregistered, that an order has no handler.
+ * {@link OrderHandlerDiscoveryProvider} is on the test classpath through
{@code META-INF/services} and blocks inside
+ * its discovery while the spec holds its gate.
+ */
+class OrderHandlerDiscoverySpec extends Specification {
+
+ void cleanup() {
+ OrderHandlerDiscoveryProvider.gate.countDown()
+ OrderHandlerDiscoveryProvider.gate = new CountDownLatch(0)
+ PredicateGenerator.clearCustomOrderHandlers()
+ }
+
+ void "a query that arrives while the providers load waits for them and
gets the discovered order handler"() {
+ given: "discovery has not run, and the provider blocks until the spec
releases it"
+ PredicateGenerator.clearCustomOrderHandlers()
+ OrderHandlerDiscoveryProvider.gate = new CountDownLatch(1)
+ OrderHandlerDiscoveryProvider.entered = new CountDownLatch(1)
+ ExecutorService executor = Executors.newFixedThreadPool(2)
+ Callable<PredicateGenerator.OrderHandler> lookup = {
PredicateGenerator.orderHandler(OrderHandlerDiscoveryOrder) }
+
+ when: "one thread starts the discovery and blocks inside the provider,
and a second asks for the handler meanwhile"
+ Future<PredicateGenerator.OrderHandler> first = executor.submit(lookup)
+ boolean providerEntered =
OrderHandlerDiscoveryProvider.entered.await(10, TimeUnit.SECONDS)
+ Future<PredicateGenerator.OrderHandler> second =
executor.submit(lookup)
+ boolean secondAnsweredDuringDiscovery = true
+ try {
+ second.get(500, TimeUnit.MILLISECONDS)
+ } catch (TimeoutException ignored) {
+ secondAnsweredDuringDiscovery = false
+ }
+ OrderHandlerDiscoveryProvider.gate.countDown()
+
+ then: "the second caller waited for the discovery, and both callers
get the discovered handler"
+ providerEntered
+ !secondAnsweredDuringDiscovery
+ first.get(10, TimeUnit.SECONDS) != null
+ second.get(10, TimeUnit.SECONDS) != null
+
+ cleanup:
+ executor.shutdownNow()
+ }
+
+ void "a handler registered in code is kept over the discovered one"() {
+ given:
+ PredicateGenerator.clearCustomOrderHandlers()
+ PredicateGenerator.OrderHandler registered = { query, root, cb, order,
context -> null }
+ PredicateGenerator.registerOrderHandler(OrderHandlerDiscoveryOrder,
registered)
+
+ expect:
+
PredicateGenerator.orderHandler(OrderHandlerDiscoveryOrder).is(registered)
+ }
+
+}
+
+class OrderHandlerDiscoveryOrder extends Query.Order {
+
+ OrderHandlerDiscoveryOrder() {
+ super('discovered')
+ }
+
+}
+
+class OrderHandlerDiscoveryProvider implements
PredicateGenerator.OrderHandlerProvider {
+
+ static volatile CountDownLatch gate = new CountDownLatch(0)
+ static volatile CountDownLatch entered = new CountDownLatch(0)
+
+ @Override
+ Class<? extends Query.Order> orderType() {
+ OrderHandlerDiscoveryOrder
+ }
+
+ @Override
+ PredicateGenerator.OrderHandler orderHandler() {
+ entered.countDown()
+ gate.await()
Review Comment:
Done in 54a44543b2: the test provider catches the `InterruptedException`,
restores the interrupt flag and fails fast with a clear message.
--
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]
