MartinJesusDev opened a new pull request, #16429:
URL: https://github.com/apache/grails-core/pull/16429
## Description
Fixes #16425. Targets `8.0.x`, as agreed in the issue.
Under `@CompileStatic`, a criteria closure nested inside another criteria
closure is compiled as
`((Closure) getOwner()).getDelegate().<dsl>(...)`, so the subquery
restrictions are added to the
**enclosing** criteria and the subquery is left empty. Inline
`exists`/`notExists` subqueries then
either fail with `QueryException: could not resolve property X of: <outer
entity>` or silently
return wrong results. This is the documented behaviour of GROOVY-9283: with
no resolve strategy,
the static compiler assumes `OWNER_FIRST` and a closure whose owner is
another `@DelegatesTo`
closure resolves through the owner's delegate.
This PR declares `strategy = Closure.DELEGATE_FIRST` on the `@DelegatesTo`
annotations of the
criteria closure API, matching the strategy GORM already applies at runtime
in `build()`.
Annotation-only change: no runtime behavior change; the two compile-time
effects are documented
in the upgrade notes.
- `AbstractDetachedCriteria` / `DetachedCriteria`: `where`, `whereLazy`,
`build`, `buildLazy` and
the closure subquery helpers `in`, `inList`, `notIn`, `eqAll`, `gtAll`,
`ltAll`, `geAll`,
`leAll`, `gtSome`, `geSome`, `ltSome`, `leSome`.
- `grails-datamapping-rx`: the same overrides, kept consistent with the base
class. This module
is currently disabled in `settings.gradle` (commented out), so it is not
compiled or tested by
the build; the change only keeps the overrides from breaking when the
module is re-enabled.
- Internal forwarders keep their public signatures and cast the argument
instead, so the
forwarding break does not spread: `buildQueryableCriteria`,
`withPopulatedQuery`,
`GormStaticApi.where`, `HibernateGormStaticApi.where/whereLazy`,
`RxGormStaticApi.where/whereLazy/findAll/find`.
- `GormEntity.where` is intentionally not part of this change: it has no
`@DelegatesTo` today and
is better addressed separately.
- Left unchanged deliberately: `and`/`or`/`not`, `projections`,
`find`/`get`/`list`/`count`/
`asBoolean`. The forms requested in the issue all resolve correctly with
the annotated methods
(covered by the regression spec).
Two changes can affect statically compiled application code, both documented
in the new
`upgrading80x.adoc` section:
1. Forwarding a `Closure` parameter to one of these methods no longer
compiles
(`Closure parameter with resolve strategy OWNER_FIRST passed to method
with resolve strategy
DELEGATE_FIRST`). Fix: declare the same strategy on your parameter, or
cast the argument.
2. Name resolution inside a criteria closure now prefers the criteria
delegate, which is what
dynamic code already did (e.g. `getOrders()` on the class vs the
criteria's `orders`).
Note on `resolveStrategy`: calling
`Closure.setResolveStrategy(DELEGATE_FIRST)` at runtime is not
enough here — GORM already does it in `build()` and the bug persisted. Under
`@CompileStatic` the
receiver is fixed in the generated bytecode, so the strategy has to be
declared in the annotation.
### Test evidence
`NestedCriteriaCompileStaticSpec` (new unit spec, no DB) drives a
`@CompileStatic` fixture and
asserts where each restriction lands via `getCriteria()`:
| Form | Before | After |
|---|---|---|
| `exists` / `notExists` with inline `where` (`eq`) | FAIL | PASS |
| `eqProperty` inside the nested `where` | FAIL | PASS |
| where-query syntax with typed variables | FAIL | PASS |
| nested `where` inside an `or { }` junction | FAIL | PASS |
| nested `build { }` | FAIL | PASS |
| `inList('id') { … projections { id() } }` | FAIL | PASS |
| `gtAll('id') { … }` | FAIL | PASS |
| hoisted subquery (control) | PASS | PASS |
Before the change the outer criteria is e.g. `[Query$Equals, Query$Exists]`;
after it is
`[Query$Exists]` and the inner criteria holds the `Query$Equals`. `javap` of
the compiled nested
closure shows `getDelegate()` + `DetachedCriteria.eq` (no `getOwner()`).
Local commands (JDK 21+):
```
./gradlew :grails-datamapping-core:test --rerun-tasks
./gradlew :grails-datamapping-core:codeStyle
./gradlew :grails-data-hibernate7-core:compileGroovy
```
Note: aggregated HTML test report generation can fail in this environment
for a test whose name
contains a non-ASCII character (`DefaultSchemaHandlerSpec`, `... → ...`):
two report directories
for the same test coexist with different encodings (`→` UTF-8 and `?` ASCII)
and Gradle 9.8 fails
hashing that output. This is pre-existing and unrelated to the change; for
local runs the report
directory was cleaned and the HTML report plus the aggregate report tasks
(`:grails-test-report:testAggregateTestReport`,
`:grails-test-report:markdownAggregateTestReport`)
were excluded.
`./gradlew :grails-datamapping-core:test --rerun-tasks`: **BUILD SUCCESSFUL,
1342 tests, 0
failures** (including the 9 regression cases).
`:grails-datamapping-core:codeStyle`,
`:grails-data-hibernate7-core:compileGroovy` and the hibernate7 test suite
(3293 tests, 0 failures)
are green locally. Full `./gradlew build --rerun-tasks` (TCKs and test
examples included) did not
complete locally in a reasonable time and was stopped after ~6,000 tasks; CI
will run the full
suite.
The violation aggregate (`./gradlew cleanViolationReports
aggregateViolations --continue`) is
clean: CodeNarc, Checkstyle, PMD and the repository conventions report no
violations; SpotBugs is
disabled.
## Contributor Checklist
Please review the following checklist before submitting your pull request.
Pull requests that do not meet these requirements may be closed without review.
### Issue and Scope
- [x] This PR is linked to an existing issue that has been **acknowledged or
approved** by the project team. If no approved issue exists, please give
background on why this change is necessary. Tickets are preferred for release
change log history.
- [x] This PR addresses the **complete scope** of the linked issue. Partial
implementations or unfinished work should not be submitted for review.
- [x] This PR contains a **single, focused change**. Unrelated changes
should be submitted as separate pull requests.
- [x] This PR targets the **correct branch** for the type of change:
- **Patch release branches** (e.g., `7.0.x`): Bug fixes only. No new
features or API changes.
- **Minor release branches** (e.g., `7.1.x`): New features are welcome,
but breaking existing APIs must be avoided.
- **Major release branches** (e.g., `8.0.x`): Reserved for major
changes. Breaking API changes are permitted.
### Code Quality
- [x] I have **added or updated tests** that cover the changes introduced in
this PR. All code contributions are expected to include appropriate test
coverage.
- [ ] I have verified that all existing tests pass by running `./gradlew
build --rerun-tasks`. (Not run to completion locally — see the note above.
`:grails-datamapping-core:test` 1342/0 and the hibernate7 suite are green; full
monorepo compile and code style are green; CI covers the full suite.)
- [x] My code follows the project's **code style** guidelines. I have run
`./gradlew codeStyle` and resolved any violations. See [Code
Style](../CONTRIBUTING.md#code-style) for details.
- [x] This PR does **not** include mass reformatting, style-only changes, or
large-scale refactoring unless it was **explicitly approved** in the linked
issue. Unsolicited reformatting will not be accepted.
- [x] If generative AI tooling was used in preparing this contribution, a
quality model was used to ensure contributions are **consistent with the
project's quality standards**.
### Licensing and Attribution
- [x] All contributed code is provided under the [Apache License
2.0](https://www.apache.org/licenses/LICENSE-2.0), and new source files include
the appropriate **Apache license header**.
- [x] I have the necessary rights to submit this contribution and confirm it
is my own original work (see [Legal
Notice](../CONTRIBUTING.md#i-want-to-contribute)).
- [x] If generative AI tooling was used in preparing this contribution, I
have followed the [Apache Software Foundation's policy on generative
tooling](https://www.apache.org/legal/generative-tooling.html) and have
properly attributed its use.
### Documentation
- [x] If this PR introduces user-facing changes, I have included or updated
the relevant documentation. (Upgrade note added to `upgrading80x.adoc`; no API
change.)
- [ ] If this PR adds a new feature, I have updated the **What's New**
section of the Grails Guide. (N/A: bug fix.)
- [x] If this PR introduces breaking changes or changes that require user
action during an upgrade, I have updated the **Upgrade Notes** for the
corresponding version in the Grails Guide.
- [x] The PR description clearly explains **what** was changed and **why**.
---
> **Generative AI attribution**: the analysis (including the `javap`
evidence), the change and the
> regression spec were prepared with AI assistance, and reviewed, compiled
and tested locally by
> the contributor, who is accountable for every line.
> **First-time contributors:** Please read our [Contributing
Guide](../CONTRIBUTING.md) before submitting.
> Pull requests that appear to be auto-generated, incomplete, or unrelated
to an approved issue may be
> closed to help maintainers focus on reviewed and planned work. We
appreciate your understanding.
--
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]
