borinquenkid commented on code in PR #16820:
URL: https://github.com/apache/grails-core/pull/16820#discussion_r4480299308
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/criteria/AbstractDetachedCriteria.groovy:
##########
@@ -872,7 +872,14 @@ abstract class AbstractDetachedCriteria<T> implements
Criteria, Cloneable {
@CompileStatic
AbstractDetachedCriteria<T> clone() {
AbstractDetachedCriteria criteria = newInstance()
- criteria.@criteria = new ArrayList(this.criteria)
+ Map<DetachedAssociationCriteria, DetachedAssociationCriteria>
associationCopies = new IdentityHashMap<>()
+ List<Query.Criterion> clonedCriteria = new
ArrayList<>(this.criteria.size())
+ for (Query.Criterion criterion : this.criteria) {
+ clonedCriteria.add(criterion instanceof
DetachedAssociationCriteria ?
+ copyAssociationCriteria((DetachedAssociationCriteria)
criterion, associationCopies) : criterion)
Review Comment:
Fixed in 97949fc0b4: `clone()` now copies junctions recursively
(`copyCriterion`), creating a new Conjunction/Disjunction/Negation and copying
children through the same association map, so an alias added inside `or
{}`/`and {}`/`not {}` reaches the copy and not the shared node. Covered per
junction type and nested in `DetachedCriteriaCloneSpec`.
##########
grails-test-examples/hibernate5/grails-hibernate/src/integration-test/groovy/functional/tests/WhereLazyScenarioSpec.groovy:
##########
@@ -0,0 +1,140 @@
+/*
+ * 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 functional.tests
+
+import java.util.concurrent.Callable
+import java.util.concurrent.Executors
+
+import grails.gorm.DetachedCriteria
+import grails.testing.mixin.integration.Integration
+import spock.lang.Specification
+
+/**
+ * Runs criteria built with whereLazy against a real database: the same
criteria used again,
+ * as a subquery, for bulk writes and from several threads. The rows are
committed so that
+ * other threads can see them, and removed again after each scenario.
+ */
+@Integration(applicationClass = Application)
+class WhereLazyScenarioSpec extends Specification {
+
+ void setup() {
+ ScenarioItem.withNewTransaction {
+ def ann = new ScenarioOwner(name: 'ann').save(flush: true,
failOnError: true)
+ def bob = new ScenarioOwner(name: 'bob').save(flush: true,
failOnError: true)
+ new ScenarioItem(name: 'a', qty: 1, status: ScenarioStatus.NEW,
owner: ann).save(flush: true, failOnError: true)
+ new ScenarioItem(name: 'b', qty: 2, status: ScenarioStatus.NEW,
owner: ann).save(flush: true, failOnError: true)
+ new ScenarioItem(name: 'c', qty: 3, status: ScenarioStatus.DONE,
owner: bob).save(flush: true, failOnError: true)
+ }
+ }
+
+ void cleanup() {
+ ScenarioItem.withNewTransaction {
+ ScenarioItem.list()*.delete(flush: true)
+ ScenarioOwner.list()*.delete(flush: true)
+ }
+ }
+
+ private static <T> T inSession(Closure<T> work) {
+ ScenarioItem.withNewSession(work)
+ }
+
+ private static List<String> names(Collection<ScenarioItem> items) {
+ items*.name.sort()
+ }
+
+ void "a lazy criteria returns the same rows each time it is used"() {
+ given:
+ DetachedCriteria<ScenarioItem> crit = ScenarioItem.whereLazy { qty >=
2 }
+
+ when:
+ def first = inSession { names(crit.list()) }
+ def second = inSession { names(crit.list()) }
+ def total = inSession { crit.count() }
+
+ then:
+ first == ['b', 'c']
+ second == ['b', 'c']
+ total == 2
+ }
+
+ void "a lazy criteria can be narrowed by a second where"() {
+ given:
+ DetachedCriteria<ScenarioItem> crit = ScenarioItem.whereLazy { qty >=
2 }
+
+ when:
+ def narrowed = inSession { names(crit.where { status ==
ScenarioStatus.DONE }.list()) }
+ def original = inSession { names(crit.list()) }
+
+ then:
+ narrowed == ['c']
+ original == ['b', 'c']
+ }
+
+ void "a lazy criteria keeps its restriction as a subquery"() {
+ given:
+ def annIds = ScenarioOwner.whereLazy { name == 'ann' }.property('id')
Review Comment:
Agreed, and fixed in 97949fc0b4: the H5 `WhereLazyScenarioSpec` is removed,
since H5 `whereLazy` is plain `where`. H5 now has real `buildLazy` coverage in
`DetachCriteriaSubquerySpec` (core), and H5 materializes at
`getHibernateDetachedCriteria`.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/query/api/QueryableCriteria.java:
##########
@@ -57,4 +57,22 @@ public interface QueryableCriteria<T> extends Criteria {
*/
String getAlias();
+ /**
+ * Applies a deferred criteria definition, if any, so that {@link
#getCriteria()} and
+ * {@link #getProjections()} can be read. Implementations without deferred
criteria do nothing.
+ */
+ default void applyLazyCriteria() {
Review Comment:
Fixed in 97949fc0b4: `applyLazyCriteria()` is gone from `QueryableCriteria`;
only `materializeLazyCriteria()` stays as the default. `applyLazyCriteria()` is
`protected` again in `AbstractDetachedCriteria`.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/criteria/AbstractDetachedCriteria.groovy:
##########
@@ -1118,6 +1150,22 @@ abstract class AbstractDetachedCriteria<T> implements
Criteria, Cloneable {
this.with(criteria)
}
+ /**
+ * Returns this criteria with its deferred query applied, without mutating
it: the original keeps the
+ * deferred query, so a criteria shared between threads materializes a
copy each time and no deferred
+ * definition is applied twice.
+ */
+ AbstractDetachedCriteria<T> materializeLazyCriteria() {
+ Closure pending = lazyQuery
+ if (pending == null) {
+ return this
+ }
+ AbstractDetachedCriteria<T> materialized = clone()
+ materialized.lazyQuery = null
+ materialized.with(pending)
Review Comment:
Stated and decided in 97949fc0b4: the closure runs once, on first use. The
result is kept in a private template and each materialization returns a copy of
it, so use never changes the criteria and the closure is not re-run. Documented
in the `buildLazy` Javadoc; `DetachedCriteriaCloneSpec` pins run-once and
independent copies.
##########
grails-datastore-core/src/test/groovy/org/grails/datastore/mapping/query/jpa/JpaQueryBuilderFunctionSpec.groovy:
##########
@@ -119,6 +120,38 @@ class JpaQueryBuilderFunctionSpec extends Specification {
then:
thrown(InvalidDataAccessResourceUsageException)
}
+
+ void "JpaQueryBuilder(QueryableCriteria) materializes deferred criteria on
construction and in subqueries"() {
Review Comment:
Addressed in 97949fc0b4: the spec now has a deferred In-subquery case with
an exact expected query string and parameters, so the
`materializeLazyCriteria()` call in `buildSubQuery` is exercised for real.
RxGORM's `prepareQuery` race is not in this diff and `grails-datamapping-rx` is
not built, so I left it alone.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/criteria/AbstractDetachedCriteria.groovy:
##########
@@ -872,7 +872,14 @@ abstract class AbstractDetachedCriteria<T> implements
Criteria, Cloneable {
@CompileStatic
AbstractDetachedCriteria<T> clone() {
AbstractDetachedCriteria criteria = newInstance()
- criteria.@criteria = new ArrayList(this.criteria)
+ Map<DetachedAssociationCriteria, DetachedAssociationCriteria>
associationCopies = new IdentityHashMap<>()
+ List<Query.Criterion> clonedCriteria = new
ArrayList<>(this.criteria.size())
+ for (Query.Criterion criterion : this.criteria) {
+ clonedCriteria.add(criterion instanceof
DetachedAssociationCriteria ?
+ copyAssociationCriteria((DetachedAssociationCriteria)
criterion, associationCopies) : criterion)
Review Comment:
Fixed in 97949fc0b4 by the recursive junction copy: the `or {
createAlias(...) }` association node is now a separate copy, so the deferred
`createAlias('books','b')` no longer reuses it. `DetachedCriteriaCloneSpec`
covers a restriction reaching the junction copy.
--
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]
