[PR] fix: where query composition fails when variable is re-assigned in if/else blocks [grails-core]

["jamesfredley (via GitHub)" <gi...@apache.org>]

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: notifications-unsubscribe@grails.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix: where query composition fails when variable is re-assigned in if/else blocks [grails-core]

["davydotcom (via GitHub)" <gi...@apache.org>]

davydotcom commented on PR #15447:
URL: https://github.com/apache/grails-core/pull/15447#issuecomment-3953420080

   this is huge. I have lost count how many times this bites us. Thanks for finding and fixing, glad I finally remembered to bring it up :/
   


-- 
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: notifications-unsubscribe@grails.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix: where query composition fails when variable is re-assigned in if/else blocks [grails-core]

["jamesfredley (via GitHub)" <gi...@apache.org>]

jamesfredley commented on PR #15447:
URL: https://github.com/apache/grails-core/pull/15447#issuecomment-3953880458

   It was added to an existing functional test on https://github.com/apache/grails-core/pull/15447/changes/4db4ed81261143b51f6bbdfab951fb6f9832ec10 


-- 
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: notifications-unsubscribe@grails.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix: where query composition fails when variable is re-assigned in if/else blocks [grails-core]

["jamesfredley (via GitHub)" <gi...@apache.org>]

jamesfredley merged PR #15447:
URL: https://github.com/apache/grails-core/pull/15447


-- 
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: notifications-unsubscribe@grails.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org