Copilot commented on code in PR #16733: URL: https://github.com/apache/grails-core/pull/16733#discussion_r4352310075
########## 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: `CountDownLatch#await()` can throw `InterruptedException`. In this test provider, an interrupt (e.g., from `shutdownNow()` or a failing test) would bubble out of ServiceLoader discovery and can create hard-to-debug, intermittent failures. Consider catching `InterruptedException`, restoring the interrupt flag (`Thread.currentThread().interrupt()`), and failing fast with a clear runtime exception (or unblocking in a deterministic way) so test behavior remains predictable. ########## grails-test-examples/hibernate7/criteria-extension/src/main/resources/META-INF/services/org.grails.orm.hibernate.query.PredicateGenerator$OrderHandlerProvider: ########## @@ -0,0 +1 @@ +example.query.PriceDistanceOrderHandlerProvider Review Comment: This newly added ServiceLoader provider file lacks the Apache license header comment that is present in the similar test resource provider file. For consistency (and to avoid potential license-compliance tooling issues), consider adding the standard header using `#` comment lines above the provider class name. ########## 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: The sentence “the maps keep the first registration” is misleading now that `registerOrderHandler(...)` (and the existing criterion registration) uses `put(...)`, which intentionally allows code-registered handlers to override ServiceLoader-discovered handlers. Suggest rewording to accurately reflect the rule: discovery uses `putIfAbsent` so it does not override existing registrations, while explicit registration in code takes precedence. -- 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]
