matrei commented on issue #16425:
URL: https://github.com/apache/grails-core/issues/16425#issuecomment-5887230278
Thanks for the detailed report. I can confirm it on `7.2.x` (Groovy 4) and
on `8.0.x` (Groovy 5.1.3). The `@DelegatesTo` declarations are identical on
`7.0.x`. Without `@CompileStatic` the same code builds the correct criteria.
With it, a restriction inside the nested closure is added to the enclosing
criteria and the subquery is left empty:
```groovy
@CompileStatic
static DetachedCriteria<Author> query() {
new DetachedCriteria<Author>(Author).where {
exists new DetachedCriteria<Book>(Book).where { eq 'title', 'X'
}.id()
}
}
// Author criteria: [Equals(title, X), Exists(Book criteria: [])]
```
One note on the snippet in the description: `where { title == 'X' }` called
on a `new DetachedCriteria<>(Book)` receiver is not rewritten by the
where-query transformation, so as written it compiles to a boolean comparison
instead of a restriction. The bug does reproduce with the where-query syntax
when the receivers are typed variables, as well as with explicit criteria
methods:
```groovy
DetachedCriteria<Author> authors = new DetachedCriteria<Author>(Author)
DetachedCriteria<Book> books = new DetachedCriteria<Book>(Book)
authors.where { exists books.where { title == 'X' }.id() }
// Author criteria: [Equals(title, X), Exists(Book criteria: [])]
```
Other forms that end up on the outer criteria:
- `eqProperty` inside the nested `where`
- a nested `where` inside an `or { }` junction
- nested `build { }`
- closure subqueries such as `inList('id') { eq 'name', 'X'; projections {
id() } }` and `gtAll('id') { ... }`
In each case the inner restriction is added to the outer criteria. The
`inList`/`gtAll` cases need the subquery helpers in the fix as well, not only
`where`/`build`.
Your analysis matches GROOVY-9283, and `strategy = Closure.DELEGATE_FIRST`
is the right direction. I prototyped it on `DetachedCriteria` and
`AbstractDetachedCriteria` (`where`, `whereLazy`, `build`, `buildLazy`) and all
of the nested forms above then produce the correct criteria. The change does
have two effects on statically compiled application code, and we need to
account for them before deciding where it lands:
1. **Forwarding a closure parameter no longer compiles.** The same error you
hit on the internal forwarders also hits user code. This compiles today and
fails after the change:
```groovy
@CompileStatic
class BookQueries {
static DetachedCriteria<Book>
withExtra(@DelegatesTo(DetachedCriteria) Closure extra) {
new DetachedCriteria<Book>(Book).where(extra)
// [Static type checking] - Closure parameter with resolve
strategy OWNER_FIRST
// passed to method with resolve strategy DELEGATE_FIRST
}
}
```
The same happens with a plain `Closure extra` parameter. Callers have to
add `strategy = Closure.DELEGATE_FIRST` to their own parameter, or cast.
2. **Name resolution changes silently.** A property or method that exists on
both the enclosing class and the criteria resolves to the criteria after the
change. It already does that in dynamic code, so this brings static code in
line with it. Still, it is a behaviour change. For example, in a class with a
`getOrders()` method, `where { inList 'name', orders }` currently uses the
class's `orders`. After the change it uses the criteria's (empty) order list.
Because of these, I think this is better suited to `8.0.x`, with an entry in
the upgrade notes, than to a `7.x` patch release. Other maintainers may see
that differently.
Some suggestions for the PR:
- **Keep the public signature changes small.** For internal forwarders like
`GormStaticApi.where/find/findAll`, `buildQueryableCriteria` and
`withPopulatedQuery`, casting the argument (`build((Closure) callable)`) is
enough to satisfy the type checker. That way the forwarding break in (1)
doesn't also spread to `GormEntity`, `GormStaticOperations` and
`TenantDelegatingGormOperations`.
- **Treat `GormEntity.where` as a separate change.**
`GormEntity.where(Closure)` and related methods currently have no
`@DelegatesTo` at all, so `Author.where { exists Book.where { ... }.id() }`
doesn't type-check under `@CompileStatic` today. Adding it would be a separate
improvement. It needs its own verification against the where-query
transformation and the datastore TCKs.
- **Cover each affected form in the regression test.** Under
`@CompileStatic`, cover explicit criteria methods, the where-query syntax with
typed variables, junctions, and the closure subquery helpers. Also cover a
hoisted subquery as a control.
- **Document the change.** Add a note to the upgrade guide in `grails-doc`
about the stricter closure typing and the forwarding error, and how to fix it.
The workarounds in the description (`@CompileDynamic` on the method, or
hoisting the subquery into a local variable) work as described.
--
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]
