borinquenkid commented on code in PR #16820:
URL: https://github.com/apache/grails-core/pull/16820#discussion_r4482402025
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/criteria/AbstractDetachedCriteria.groovy:
##########
@@ -1108,16 +1168,47 @@ abstract class AbstractDetachedCriteria<T> implements
Criteria, Cloneable {
protected abstract QueryableCriteria buildQueryableCriteria(Closure
queryClosure)
+ /**
+ * Applies a pending {@code buildLazy} closure to this criteria, if any,
and clears it. Used when this
+ * criteria is itself extended; a query reads a deferred criteria through
{@link #materializeLazyCriteria()}.
+ */
protected void applyLazyCriteria() {
if (lazyQuery == null) {
return
}
def criteria = lazyQuery
lazyQuery = null
+ materializedTemplate = null
this.with(criteria)
}
+ /**
+ * Returns this criteria with its deferred query applied, without mutating
it. The deferred closure runs
+ * once, on a private template that is never handed out; every call
returns a fresh copy of that template, so
+ * a criteria shared between threads is never changed by a query that
reads or extends the result.
+ * A criteria without a deferred query returns itself.
+ */
+ AbstractDetachedCriteria<T> materializeLazyCriteria() {
+ if (lazyQuery == null) {
+ return this
+ }
+ AbstractDetachedCriteria<T> template = materializedTemplate
Review Comment:
Fixed in df1a73a04b, and you were right that it was one cause with several
faces. The template was a second copy of the state: queries read it, while most
mutators changed the criteria itself, so anything added after the first query
was skipped. There is no template now. `applyLazyCriteria()` runs the closure
once, under a lock with a re-entrancy guard, on the criteria itself, and every
method that reads or writes state, plus `clone()`, calls it first: `setAlias`,
`order`, `sort`, `join`, `select`, `createAlias`, all the projection methods
and the getters. A query reads a copy. `DetachedCriteriaDeferredQuerySpec` has
one row for each way in (32 of its 33 rows fail on the previous
implementation), and `DetachedCriteriaCloneSpec` pins an existing association
extended after the first query. While writing it I found `clone()` built the
copy's projection list as an inner class of the source criteria, so its methods
belonged to the wrong criteria; a clone builds its own now.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/criteria/AbstractDetachedCriteria.groovy:
##########
@@ -1108,16 +1168,47 @@ abstract class AbstractDetachedCriteria<T> implements
Criteria, Cloneable {
protected abstract QueryableCriteria buildQueryableCriteria(Closure
queryClosure)
+ /**
+ * Applies a pending {@code buildLazy} closure to this criteria, if any,
and clears it. Used when this
+ * criteria is itself extended; a query reads a deferred criteria through
{@link #materializeLazyCriteria()}.
+ */
protected void applyLazyCriteria() {
if (lazyQuery == null) {
return
}
def criteria = lazyQuery
lazyQuery = null
+ materializedTemplate = null
Review Comment:
Fixed in df1a73a04b. A derived criteria no longer runs the closure again:
`clone()` applies the pending closure first and the copy carries the result, so
`where`, `max`, `offset`, `withConnection` and `buildLazy` all derive from the
first run. Your scenario is pinned against Hibernate 5 and 7 (`criteria.where {
ne('name', 'Z') }` after the value changed from A to B returns the A row; the
original still returns A), and it fails on the previous implementation.
`buildLazy` on a deferred criteria also keeps the earlier closure now instead
of replacing it. The assertions that expected a read to show an empty criteria
are rewritten in 6d2431c227, because a read runs the closure like every other
way in.
--
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]
