MartinJesusDev opened a new issue, #16674:
URL: https://github.com/apache/grails-core/issues/16674
### Expected Behavior
The GORM criteria API (`where { }`, `createCriteria`, `DetachedCriteria`,
the bulk `updateAll`/`deleteAll`)
behaves as the reference manual documents it, consistently across the
datastores that support each feature,
and never drops a restriction silently:
```groovy
Person.where { age > avg(age) }.list() // documented
aggregate subquery
Person.where { upper(lastName) == 'SIMPSON' }.list() // documented
function call
Team.where { def t = Team; def c = club // documented
alias variables
exists(Club.where { id == t.club.id }.property('name')) }.list()
Person.where { }.readOnly(true).list() // documented
read only mode
Person.where { }.max(2).deleteAll() // must not
silently delete every row
```
### Actual Behaviour
An audit of the Grails 8 criteria/query surface against the manual, the
Hibernate 5 implementation and the
code found **28 findings**: 23 in the Hibernate 7 implementation (F3 is open
for a maintainer decision and F7
needs no change), 1 in the Rx module (F5, out of this branch), 2 in the
non-SQL implementations (F24, F25) and
2 found by a code review of the branch (F26, F27). Most are capabilities the
Hibernate 7 rewrite dropped, or
conditions it ignored **silently** (the query matches every row instead of
failing). The complete inventory,
with the fix commit and the test that covers each finding:
**How to read the table.** Each row is one finding: the area, what a user
saw before the branch (the finding), the commit that fixes it in the branch
above (linked to the fork) and the spec that covers it. *decision* means the
branch does not change it because it needs a maintainer decision; *no change
needed* means the behaviour is already usable as documented; *out of branch*
means the module is disabled in 8.0.x; *separate issue* means it is tracked in
another issue of this audit. The detailed notes at the end explain the two
non-obvious ones (F20 and F7).
`F18`/`F19` were false alarms during the audit (test authoring); the
numbering keeps the gaps on purpose.
| # | Area | Finding | Fix commit (branch `audit/gorm-criteria-8.0.x-clean`)
| Test |
|---|------|---------|--------------------------------------------------------|------|
| F1 | reads | a deferred (lazy) subquery was not materialised, so
`exists`/`inList` with a lazily built `where` matched nothing |
[`0fc027f04a`](https://github.com/MartinJesusDev/grails-core/commit/0fc027f04a)
| `DetachCriteriaSubquerySpec` |
| F2 | reads | translating a subquery mutated the criteria projection list,
so a second execution differed |
[`a412c55e7a`](https://github.com/MartinJesusDev/grails-core/commit/a412c55e7a),
[`fdd04528be`](https://github.com/MartinJesusDev/grails-core/commit/fdd04528be)
| `DetachCriteriaSubquerySpec`, `CriteriaSubqueryMatrixSpec` |
| F3 | reads | `singleResult` with more than one row behaves inconsistently
across the API (some paths return the first row, others fail) and the
documentation does not state the contract, so it needs a maintainer decision.
The branch pins the current shape with a test. | — (decision) |
`DetachedCriteriaProjectionSpec` |
| F4 | API | `GormEntity.where`/`whereLazy`/`whereAny`/`findAll` lacked
`@DelegatesTo` (unusable from `@CompileStatic`) |
[`09af071871`](https://github.com/MartinJesusDev/grails-core/commit/09af071871)
| `GormEntityWhereCompileStaticSpec` |
| F5 | API | the Rx `DetachedCriteria` resolves closure calls with
`OWNER_FIRST` instead of `DELEGATE_FIRST`, so a where closure can pick up the
wrong receiver. The Rx module is disabled in 8.0.x, so the fix is out of this
branch. | out of branch | — |
| F6 | tests | the Hibernate 7 criteria translation had no guard for the
criterion registry and little compile-static coverage, so a missing handler
only showed up at runtime. Tests added: translator completeness, compile-static
API, builder path, inheritance and a PostgreSQL oracle. |
[`4ac0a93c8b`](https://github.com/MartinJesusDev/grails-core/commit/4ac0a93c8b),
[`149af266b1`](https://github.com/MartinJesusDev/grails-core/commit/149af266b1)
| `CriteriaTranslatorCompletenessSpec` |
| F7 | API | `createCriteria()` omits builder-only methods: the
Hibernate/JPA ones (`createAlias`, `fetchMode`, `lock`, `sql*`) cannot be
shared, most are internal, and the generic projection/pagination ones
(`projections`, `maxResults`, …) work in `@CompileStatic` inside `list {
}`/`withCriteria { }`. No change needed. | — (no change needed) |
`StaticCompiledCriteriaSpec` |
| F8 | writes | several subqueries in one bulk write shared parameter
positions |
[`5675818fbb`](https://github.com/MartinJesusDev/grails-core/commit/5675818fbb)
| `CriteriaWriteOperationsSpec` |
| F9 | writes | `exists`/`notExists` were not supported in the bulk write
path |
[`50d85ac379`](https://github.com/MartinJesusDev/grails-core/commit/50d85ac379)
| `CriteriaWriteOperationsSpec` |
| F10 | writes | bulk writes did not filter through to-one association paths
|
[`0c5ded46d1`](https://github.com/MartinJesusDev/grails-core/commit/0c5ded46d1)
| `CriteriaWriteOperationsSpec` |
| F11 | writes | association aliases and the `{alias}` placeholder in bulk
writes |
[`58e1ed76e9`](https://github.com/MartinJesusDev/grails-core/commit/58e1ed76e9)
| `CriteriaWriteOperationsSpec` |
| F12 | writes | a deferred subquery in the HQL write path |
[`822653628b`](https://github.com/MartinJesusDev/grails-core/commit/822653628b)
| `CriteriaWriteOperationsSpec` |
| F13 | reads | where-query functions (`upper`, `lower`, `trim`, `length`,
`year`, …) threw `UnsupportedOperationException` (Hibernate 5 supported them;
the manual documents them) |
[`337484dc14`](https://github.com/MartinJesusDev/grails-core/commit/337484dc14)
| `WhereQueryDocumentedFormsSpec`, `DataServiceSpec` |
| F14 | reads | comparing a property with a subquery for `gt`/`ge`/`lt`/`le`
bound the criteria as a parameter (the manual documents `gt "age", new
DetachedCriteria(Person).build { projections { avg "age" } }`) |
[`337484dc14`](https://github.com/MartinJesusDev/grails-core/commit/337484dc14)
| `WhereQueryDocumentedFormsSpec`, `CriteriaWriteOperationsSpec` |
| F15a | reads | an unresolved property path in a property comparison was
dropped silently (every row matched) |
[`337484dc14`](https://github.com/MartinJesusDev/grails-core/commit/337484dc14)
| `WhereQueryDocumentedFormsSpec` |
| F15b | reads | a criteria root alias was not resolvable from a subquery
(#14623) |
[`337484dc14`](https://github.com/MartinJesusDev/grails-core/commit/337484dc14)
| `SubqueryAliasSpec` |
| F16 | reads | `in`/`inList` with a null member failed the whole query |
[`ee593c46c8`](https://github.com/MartinJesusDev/grails-core/commit/ee593c46c8)
| `WhereQueryDocumentedFormsSpec`, `CriteriaWriteOperationsSpec` |
| F17 | mapping | the binder ignored the Hibernate annotations `@Formula`,
`@SQLRestriction` and `@FilterDef`/`@Filter`: the property loaded as `null`,
the restriction was dropped (every query returned the rows it was meant to
hide) and the session could not enable the filter. The binder maps them now; a
restriction declared on a subclass fails with a clear message (declare it on
the root entity). |
[`0b595f31a4`](https://github.com/MartinJesusDev/grails-core/commit/0b595f31a4),
[`7c2fd42f99`](https://github.com/MartinJesusDev/grails-core/commit/7c2fd42f99),
[`b811cce4e5`](https://github.com/MartinJesusDev/grails-core/commit/b811cce4e5)
| `CriteriaMappingFeaturesSpec` |
| F17d | mapping | a `@SQLRestriction` on a subclass was dropped silently;
it fails with a clear message now (declare it on the root entity) |
[`a77279235c`](https://github.com/MartinJesusDev/grails-core/commit/a77279235c),
[`2dbd58e7fc`](https://github.com/MartinJesusDev/grails-core/commit/2dbd58e7fc)
| `SubclassSqlRestrictionSpec`, `SubClassBinderSpec` |
| F20 | writes | a batch operation silently ignored the pagination of the
criteria: `Person.where { }.max(2).deleteAll()` deleted **every** matching row
(a bulk statement has no limit clause and the builder never read `defaultMax`).
A plain `deleteAll()` still deletes everything; only the paginated call is
rejected, with a message naming the values (see the detailed notes). |
[`e070f5e678`](https://github.com/MartinJesusDev/grails-core/commit/e070f5e678)
| `CriteriaWriteOperationsSpec` |
| F21 | services | `@Where({ amount > avg(amount) })` generated invalid SQL
(the aggregate projection landed on the outer query) |
[`e654e88e71`](https://github.com/MartinJesusDev/grails-core/commit/e654e88e71)
| `DataServiceSpec` |
| F22 | reads | a comparison through a nested alias path (`t.club.id`) was
evaluated at runtime |
[`d891e5c317`](https://github.com/MartinJesusDev/grails-core/commit/d891e5c317)
| `SubqueryAliasSpec` |
| F23 | reads | `readOnly(true)`/`cache(true)` on a detached criteria were
no-ops |
[`f2941fe1a0`](https://github.com/MartinJesusDev/grails-core/commit/f2941fe1a0)
| `WhereQueryDocumentedFormsSpec`, `HibernateCriteriaBuilderSpec` |
| F23b | reads | the Hibernate settings of the query arguments map
(`readOnly`, `cache`, `fetchSize`, `timeout`, `flushMode`) were dropped for
criteria |
[`c5b634eb12`](https://github.com/MartinJesusDev/grails-core/commit/c5b634eb12),
[`8ead79eaca`](https://github.com/MartinJesusDev/grails-core/commit/8ead79eaca)
| `WhereQueryDocumentedFormsSpec` |
| F24 | non-SQL | on MongoDB and Neo4j the four aggregate subquery forms of
the manual fail with `MissingMethodException: avg(<DetachedCriteria>)`: the
transformer emits a call their criteria API cannot satisfy (a different call
than on Hibernate, where it works). Both can express the forms with their own
machinery (Mongo: `$expr`/aggregation pipeline; Neo4j: a Cypher subquery), so
it is an implementation gap; until then it should be a clear 'not supported'
message and documented. A separate issue of this audit tracks it; the branch
pins the current state with pending specs. | — (separate issue) | pending specs
|
| F25 | tests | the TCK sources are compiled without the where-query AST
transformation, so a TCK test written with operators or property references
runs untransformed and passes without testing anything (for example
`WqFoo.where { bar == null }` compares a `PropertyReference` and always reports
0). The where-query coverage of the kit is partly fake; a separate issue of
this audit tracks the cause (the TCK rework lost the wiring). | — (separate
issue) | — |
| F26 | reads | a deferred criteria (`whereLazy`/`buildLazy` and the
subqueries the transformer builds) applied its closure **in place** on first
use: two threads sharing one criteria could both see it and apply it twice,
duplicating criteria or projections. The read and bulk paths materialise a copy
now; only the builder path applies in place, which is single threaded by
nature. |
[`cb4fc2f439`](https://github.com/MartinJesusDev/grails-core/commit/cb4fc2f439)
| `DetachCriteriaSubquerySpec` |
| F27 | reads | `AbstractDetachedCriteria.clone()` copied every field but
the root alias, so a derived criteria (`sort`, `max`, `build`, `whereLazy`) and
the copy the deferred materialisation returns lost it: a subquery referring to
the alias failed with `Cannot resolve property`. The clone keeps the alias now,
and the read path takes the alias, the max and the offset from the materialised
criteria. |
[`e3deb72e10`](https://github.com/MartinJesusDev/grails-core/commit/e3deb72e10)
| `SubqueryAliasSpec` |
### Steps To Reproduce
**Where the change can be reviewed** (fork of `apache/grails-core`):
- Branch:
<https://github.com/MartinJesusDev/grails-core/tree/audit/gorm-criteria-8.0.x-clean>
- Compare with `8.0.x`:
<https://github.com/MartinJesusDev/grails-core/compare/8.0.x...audit/gorm-criteria-8.0.x-clean>
- Full commit list (one commit per finding, the finding in every message):
`git log --oneline 8.0.x..audit/gorm-criteria-8.0.x-clean`
- Production files touched (21):
`git diff --name-only 8.0.x...audit/gorm-criteria-8.0.x-clean | grep
src/main`
**Run the reproduction** (H2, no containers):
```bash
# the documented forms end to end
./gradlew :grails-data-hibernate7-core:test --tests
"*WhereQueryDocumentedFormsSpec" --tests "*SubqueryAliasSpec"
# the bulk write path (subqueries, aliases, association paths, the
pagination rejection)
./gradlew :grails-data-hibernate7-core:test --tests
"*CriteriaWriteOperationsSpec"
# the binder annotations (formula, restriction, filters)
./gradlew :grails-data-hibernate7-core:test --tests
"*CriteriaMappingFeaturesSpec" --tests "*SubclassSqlRestrictionSpec"
# the data service @Where path
./gradlew :grails-data-hibernate7-core:test --tests "*DataServiceSpec"
```
Before the branch: `upper(name) == 'A'` throws
`UnsupportedOperationException`; `gt('age', <subquery>)` throws
`QueryArgumentException`; `max(2).deleteAll()` deletes every row;
`readOnly(true)` does nothing;
`@SQLRestriction`/`@Formula`/`@FilterDef` are ignored; the alias form loses
its restriction silently. With the
branch, each of those has a test that proves the fixed behaviour.
**CI runs of the branch** — all three are green: the gate at 19:26 UTC
(style, analysis, rat, validate and
core), all-suites at 18:47 UTC (functional, hibernate5, hibernate7, mongodb,
redis, neo4j, springsecurity) and
docs at 18:00 UTC. They run on public runners:
- gate:
<https://github.com/MartinJesusDev/grails-core-ci/actions/runs/39297989319>
- all-suites (functional, hibernate5, hibernate7, mongodb, redis, neo4j,
springsecurity):
<https://github.com/MartinJesusDev/grails-core-ci/actions/runs/39297994535>
- docs:
<https://github.com/MartinJesusDev/grails-core-ci/actions/runs/39297999214>
### Environment Information
- Grails 8.0.x, `grails-data-hibernate7` (Hibernate ORM 7.4.x) with H2 in
memory; PostgreSQL 16 through
Testcontainers for the SQL oracle.
- `grails-data-hibernate5-core` (Hibernate 5.6) and the MongoDB/Neo4j/Redis
suites run in the matrix.
- JDK 21 (Liberica), Gradle 9.8.x.
### Example Application
Not a separate application: the branch above is the reproduction. Every
finding has a focused test in the
tree, and the table points at the spec that covers it. A reviewer can check
out the fork branch and run the
commands of *Steps To Reproduce*; the manual sections that document the
forms are the
`whereQueries`/`detachedCriteria`/`criteria` pages of the Hibernate manual,
and the migration note is in
`upgrading80x.adoc`, section 26.11 (*Criteria and Query Behaviour*), which
the branch adds.
### Version
8.0.x
### API and behaviour impact
The branch adds public API (binary compatible, with no removals or renames)
and documents the behaviour changes in the upgrade guide:
- New `default` methods: `Query.readOnly(boolean)` (no-op for datastores
without support) and
`QueryableCriteria.materializeLazyCriteria()` (returns `this` by default,
overridden by
`grails.gorm.DetachedCriteria`).
- New accessors:
`AbstractDetachedCriteria.getDefaultMax()`/`getDefaultOffset()` (needed by the
batch
pagination guard) and `HibernateQuery.getQueryFlushMode()` (the test of
the arguments map).
- `HibernateQuery` implements `QueryArgumentsAware`, so a criteria receives
the same settings map as the HQL
methods.
Behaviour changes that can make an application fail at startup, both
documented with the action to take in
`upgrading80x.adoc` section 26.11:
- a batch operation with `max`/`offset` now throws (it used to touch every
matching row);
- a `@SQLRestriction` declared on a subclass now fails at mapping (declare
it on the root entity).
### Open items (not changed by the branch)
- **F3** — `singleResult` with more than one row: fail loudly or return the
first row? A maintainer decision;
the branch pins the current shape with a test.
- **F5** — the Rx `DetachedCriteria` resolves with `OWNER_FIRST`; the module
is disabled in 8.0.x.
- **F7** — `createCriteria()` typing: no change needed (the generic methods
work inside the closure form and
the Hibernate/JPA-only ones cannot be shared); explained in the detailed
notes.
- **F24 / F25** — the aggregate forms on MongoDB/Neo4j and the TCK transform
are filed as separate issues of
this audit (the TCK issue explains why its where-query coverage is partly
vacuous today).
### Proposed plan
**Four pull requests**, one per subsystem, split from the branch so each
finding can be reviewed against the
failing test it fixes:
| PR | Theme (one subsystem) | Findings | Size |
|----|-----------------------|----------|------|
| 1 | the Hibernate 7 read translation | F1, F2, F4, F6, F13, F14, F15a,
F15b, F16, F22, F23, F23b, F26, F27 | ~25 files |
| 2 | the bulk write path | F8–F12, F20 | ~6 files |
| 3 | the binder annotations | F17, F17d | ~6 files |
| 4 | the data service `@Where` translation | F21 | 2 files |
Open them smallest first (4 → 3 → 2 → 1) so the maintainers can steer the
style before the core change. The
`What's New` entry for the features is in the branch (commit `0fbf51a26d`);
the two breaking behaviours are in
the Upgrade Notes (26.11). F24 and F25 are separate issues of this audit;
F3, F5 and F7 are not changed by the
branch.
### AI usage
The audit, the implementation and the tests were done with an AI coding
agent (OpenCode, model DeepSeek
V4.1 Flash), reviewed iteratively against the repository rules and the CI
gates. The pull requests will state
this explicitly, as CONTRIBUTING requires.
### Detailed notes
**F20 — the batch pagination.** `max(2)` on a criteria is read pagination
(`defaultMax`), and `deleteAll()`
builds an HQL bulk statement (`delete from Person where ...`), which has no
`LIMIT`. Before the branch the
builder never read `defaultMax`, so the call deleted every matching row and
returned the full count with no
warning (reproduced: 5 rows deleted with `max(2)`). The manual already
documents that batch operations reject
join queries and `sqlRestriction` with an exception; pagination is the same
kind of limitation, and the only
destructive one. The branch rejects it
(`InvalidDataAccessResourceUsageException`, naming `maxResults`/`offset`)
and documents it in `detachedCriteria.adoc`. A plain
`deleteAll()`/`updateAll()` without pagination is
unaffected. "Delete at most N" is not implemented because HQL has no
portable `LIMIT` on `delete`, and which N
rows would need an explicit order: today a caller selects the ids with an
order and deletes by
`inList('id', ids)`.
**F7 — `createCriteria()` typing (no change needed).** The 46 methods
`HibernateCriteriaBuilder` declares
beyond `BuildableCriteria`/`Criteria` are internal plumbing, six
Hibernate/JPA-only methods that cannot be shared
with Mongo/Neo4j, and the generic projection and pagination methods, which
work in `@CompileStatic` inside
`list { }`/`withCriteria { }` — the TCK's `StaticCompiledCriteriaQueries`
uses `maxResults`, `projections` and
`count` that way. Only the fluent style outside a closure needs a cast.
Putting the Hibernate-only methods on the
shared interface would force every datastore to implement them, so no code
change.
--
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]
