Merged
Conversation
Add four agent-facing rules covering documentation, API hygiene, and test discipline: - New features require documentation updates in grails-doc - No internal or package-private APIs in user-facing docs - Tests must use only the public API surface as an end user would - Always review existing tests and add/enhance coverage for changes Assisted-by: Claude Code <Claude@Claude.ai>
docs: add quality rules to AGENTS.md critical rules
…/else blocks The DetachedCriteriaTransformer AST transform only tracked variables declared with an initializer (DeclarationExpression) but not variables that were re-assigned (BinaryExpression). When a where query variable was declared without an initializer and then assigned inside if/else blocks, the transformer failed to recognize it as a DetachedCriteria, leaving the closure body untransformed. At runtime, the untransformed criteria were silently ignored. Add visitBinaryExpression() to DetachedCriteriaTransformer to track re-assigned variables in the detachedCriteriaVariables map and update the variable type to DetachedCriteria. Also add junctions to the AbstractDetachedCriteria clone() method for completeness. Assisted-by: OpenCode <opencode@opencode.ai>
…ed variables Add integration tests to GormWhereQueryAdvancedSpec covering: - Where query composition with re-assigned variable in if/else block - Where query with variable declared without initializer then assigned - Triple-chained where clauses on a re-assigned variable Assisted-by: OpenCode <opencode@opencode.ai>
Also handle new DetachedCriteria(DomainClass) re-assignments by updating the variable's declared type to DetachedCriteria<DomainClass>, matching the pattern already used for where() method re-assignments. This ensures the type-based fallback path in visitMethodCallExpression correctly identifies the variable as a DetachedCriteria for closure transformation. Addresses Copilot review feedback on PR #15447. Assisted-by: OpenCode <opencode@opencode.ai>
…mposition fix: where query composition fails when variable is re-assigned in if/else blocks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR merges changes from the 7.0.x branch into 7.1.x, primarily addressing an issue with GORM where query composition when variables are re-assigned (particularly in conditional blocks like if/else statements). The fix ensures that AST transformations properly track variables assigned to Domain.where{} calls, enabling proper chained where query transformations.
Changes:
- Enhanced DetachedCriteriaTransformer to track variable re-assignments to where queries
- Fixed AbstractDetachedCriteria.clone() to properly copy the junctions field
- Added comprehensive test coverage for re-assigned variable scenarios
- Updated AGENTS.md with additional guidelines for documentation and testing
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/transform/DetachedCriteriaTransformer.java | Added visitBinaryExpression() method to track variable assignments to Domain.where{} and new DetachedCriteria() calls, enabling proper AST transformation for chained where queries on re-assigned variables |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/criteria/AbstractDetachedCriteria.groovy | Fixed clone() method to properly copy the junctions field, preventing shared state between cloned instances |
| grails-datamapping-core-test/src/test/groovy/grails/gorm/tests/WhereMethodSpec.groovy | Added unit tests covering where query composition with re-assigned variables in if/else blocks, without initializers, and with multiple chained where clauses |
| grails-test-examples/gorm/src/integration-test/groovy/gorm/GormWhereQueryAdvancedSpec.groovy | Added integration tests demonstrating where query composition patterns with re-assigned variables in realistic scenarios |
| AGENTS.md | Added four new guidelines (7-10) covering documentation requirements, API visibility, testing practices, and test review expectations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.