jamesfredley opened a new pull request, #15447:
URL: https://github.com/apache/grails-core/pull/15447
## Summary
- Fix `DetachedCriteriaTransformer` AST transform to track variables that
are re-assigned (not just declared with initializer), so `where` query
composition works correctly inside `if/else` blocks
- Add `junctions` to `AbstractDetachedCriteria.clone()` for completeness
## Problem
When building dynamic GORM `where` queries with conditional logic,
re-assigned variables are silently ignored:
```groovy
def query
if (someCondition) {
query = DomainClass.where { visible == true } // This was silently
ignored!
} else {
query = DomainClass.where { visible == false }
}
query = query.where { name == 'test' } // This chained where also broken
```
The `DetachedCriteriaTransformer` only tracked variables declared with an
initializer (`def query = DomainClass.where{...}`) via
`visitDeclarationExpression()`, but not variables that were re-assigned (`query
= DomainClass.where{...}`) which are `BinaryExpression` nodes. When the
transformer encountered `query.where{...}`, it couldn't find `query` in its
`detachedCriteriaVariables` map, so the closure body was left untransformed. At
runtime, the untransformed closure's property access resolved via
`propertyMissing` to a non-mutating clone, and the criteria were silently
dropped.
## Root Cause
The `DetachedCriteriaTransformer` AST transform has a gap in its variable
tracking:
1. `visitDeclarationExpression()` handles `def query =
DomainClass.where{...}` - registers variable in `detachedCriteriaVariables`
2. **No handler** existed for re-assignment `query = DomainClass.where{...}`
- this is a `BinaryExpression`, not a `DeclarationExpression`
3. When the transformer encounters `query.where{visible == true}`:
- `detachedCriteriaVariables.get("query")` returns `null` (never tracked)
- Fallback: `var.getType()` returns `Object` (declared as `def query`
with no initializer)
- Neither path matches, so the closure body is NOT transformed
4. At runtime, the untransformed closure evaluates as a no-op
## Fix
1. **`DetachedCriteriaTransformer.java`**: Added `visitBinaryExpression()`
override that detects assignments where the RHS is a `DomainClass.where{...}`
call or `new DetachedCriteria(DomainClass)` constructor, and registers the LHS
variable in `detachedCriteriaVariables` with the correct type. Also updates the
variable's declared type to `DetachedCriteria<DomainClass>` so subsequent
`query.where{...}` calls are properly transformed.
2. **`AbstractDetachedCriteria.groovy`**: Added `junctions` to the `clone()`
method to ensure complex AND/OR query compositions are preserved during cloning.
## Tests
Added 3 new test cases to `WhereMethodSpec.groovy`:
- Re-assigned variable in `if/else` blocks (both branches verified)
- Re-assigned variable without initializer (`def query; query =
Person.where{...}`)
- Triple-chained `where` on re-assigned variable
All 80 WhereMethodSpec tests pass. Code style check passes.
--
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]
