jdaugherty commented on code in PR #16820:
URL: https://github.com/apache/grails-core/pull/16820#discussion_r4473914656
##########
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:
The title says "and in subqueries", but this feature only covers the root
criteria. In this spec the `materializeLazyCriteria()` call in `buildSubQuery`
is only reached with the stub above that returns itself (line 77), so the spec
does not notice if that call is missing. A case where the `In` subquery stub
materializes to a different criteria would cover it.
##########
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:
This copies the association nodes that sit directly in `criteria`, but a
node added inside `or {}` / `and {}` / `not {}` lives in a `Query.Junction`,
and the junction is still shared by reference. After `clone()` the junction
holds the original node while `associationCriteriaMap` holds a copy, so the
tree and the map no longer refer to the same node (the invariant the
`copyAssociationCriteria` Javadoc states), and `createAlias()` on a derived
criteria sets the alias on a node that is not in the tree.
This passes on the base branch and fails on this head with
`PathElementException: Could not resolve attribute 'o' of '...User'` (H7 core,
with the `User` / `Organisation` classes from `DetachCriteriaSubquerySpec`):
```groovy
DetachedCriteria<User> base = new DetachedCriteria<>(User).build {
or {
organisation { eq('name', 'A') }
eq('email', 'user1@b')
}
}
List<User> result = base.build {
createAlias('organisation', 'o')
like('o.name', 'A%')
}.list()
```
The same query with `organisation { ... }` at the top level instead of
inside the `or` works. Copying junctions recursively through the same
`associationCopies` map would keep the tree and the map in step. Please add a
clone test with the association node inside a junction.
##########
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:
On Hibernate 5 this spec never has a deferred criteria.
`GormStaticApi.whereLazy` is `where(callable)`; only H7's
`HibernateGormStaticApi` overrides it with `buildLazy`. The `criteria` list of
`ScenarioOwner.whereLazy { ... }` is already populated on H5 before anything
runs, so the five features here pass on H5 with or without this PR.
With a criteria that really is deferred, H5 queries still drop the
restriction, because `HibernateCriteriaBuilder.getHibernateDetachedCriteria`
reads `queryableCriteria.getCriteria()` without materializing. On this head, in
`grails-data-hibernate5-core` with `Organisation` rows `A` and `B`:
```groovy
def sub = new DetachedCriteria<>(Organisation).buildLazy { eq('name', 'A')
}.property('name')
Organisation.where { inList('name', sub) }.list()*.name // [A, B],
expected [A]
Organisation.where { notIn('name', sub) }.list()*.name // [],
expected [B]
Organisation.where { inList('name', sub) }.deleteAll() // 1, because
JpaQueryBuilder now materializes
```
`exists` with a deferred subquery that matches nothing returns every row as
well. So on H5 the same criteria selects different rows for `list()` and for
`deleteAll()`.
Every H5 subquery path goes through `getHibernateDetachedCriteria`, so
materializing at the top of it covers them all. If H5 queries are meant to be
out of scope here, please say so in the description and on #16814, and drop the
H5 copy of this spec, since it reads as coverage it does not give. Either way,
a `buildLazy` case in the H5 `DetachCriteriaSubquerySpec` is what would
exercise this.
##########
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:
A behavior change that should be stated: the deferred closure now runs on
every execution. On the base branch it ran once, at first use, and the result
stuck.
```groovy
Map wanted = [name: 'A']
Closure deferred = { eq('name', wanted.name) }
def criteria = new DetachedCriteria<>(Organisation).buildLazy(deferred)
criteria.list()*.name // [A]
wanted.name = 'B'
criteria.list()*.name // base branch: [A] this PR: [B]
```
Either meaning is defensible, but the difference is observable and
`whereLazy` is documented as returning a criteria "that is lazily initialized".
Please decide which one is intended, say so in the PR description and in the
`whereLazy` / `buildLazy` Javadoc, and add a test that pins it. If
evaluate-once is the intent, the materialized copy can be cached instead of
rebuilt.
##########
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:
Nothing calls this through the interface. The H7 handlers and
`JpaQueryBuilder` use `materializeLazyCriteria()`, and the only callers of
`applyLazyCriteria()` left are `AbstractDetachedCriteria.add()` and the RxGORM
criteria, each on itself.
Declaring it here, and widening
`AbstractDetachedCriteria.applyLazyCriteria()` from `protected` to public,
publishes the in-place mutation this PR removes as new API in a patch release.
The Javadoc on the `AbstractDetachedCriteria` method ("Query implementations
call this before they translate a criteria that only appears as a subquery
value") also describes a design the PR no longer uses.
Please drop it from the interface and leave the method `protected`. The
tests that pin it (`QueryableCriteria default methods return self and do
nothing` in part, and `test applyLazyCriteria mutates in place idempotently
when called directly`) go with it.
--
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]
