Add test coverage and fix compiler crashes in GORM AST transforms - #16148
Open
borinquenkid wants to merge 15 commits into
Open
Add test coverage and fix compiler crashes in GORM AST transforms#16148borinquenkid wants to merge 15 commits into
borinquenkid wants to merge 15 commits into
Conversation
…protectSqlInjectionAttacks DetachedCriteriaASTTransformation had 0% test coverage since the global transform normally makes the local, annotation-driven one redundant in a real build. Isolate it by disabling the global transform, proving the local transform is independently necessary and sufficient. Coverage on the class moves 0% -> 90%. Also document the protectSqlInjectionAttacks system property kill switch for the compile-time SQL injection check, which was previously mentioned only in the 8.0.x upgrade notes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… DSL
DetachedCriteriaTransformer sat at 43% instruction / 33% branch coverage
despite being the core of the where{}/find{}/findAll{} query DSL
rewriting. Add six specs covering the DSL surface that was previously
untested: comparison/collection operators (==, !=, >, <, in, between,
size(), property-to-property, and/or), negation, SQL function calls
(year, lower, etc.), association property paths, static-field where
declarations across every supported statement kind (if/else, for,
while, switch, try/catch/finally, return), and closure-cast-to-
DetachedCriteria assignments.
Where a static field is initialized directly from Domain.where{}, the
transform builds a real DetachedCriteria with no live datastore
required, so most specs assert on the actual Query.Criterion objects
produced via the public getCriteria()/getProjections() API rather than
only on generated-code structure.
Coverage moves 43% -> 78% instruction, 33% -> 59% branch.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…erty queries Follow-up to the prior where-query DSL coverage pass: the two largest remaining gaps in DetachedCriteriaTransformer were addCriteriaCall (71%/55%) and handleAssociationQueryViaPropertyExpression (54%/39%). Add four specs targeting the specific uncovered branches: - Aggregate functions called directly (not via .of()) with a non-property argument or an unknown property, and the property() pseudo-function combined with a subquery-mappable operator (rewritten into an *All subquery criterion). - Property-name and self-class aliases (`def t = someProperty`, `def a = Domain`) compared against association or plain properties, rewritten into *Property criterion calls. - A function call wrapped around a two-level association path, a distinct branch from both the single-level case and the plain (non-function) multi-level comparison. - A direct dotted comparison against an embedded (non-domain) property, distinct from the existing block-call embedded syntax coverage. Where execution needs a live, GORM-enhanced PersistentEntity this module doesn't have, these compile to the transform's own CANONICALIZATION phase and inspect the resulting AST for the exact rewrite produced, rather than only asserting the source compiles. Coverage moves 78% -> 86% instruction, 59% -> 64% branch overall; addCriteriaCall to 95%/68%, handleAssociationQueryViaPropertyExpression to 89%/63%. The one remaining gap in the class (getPropertyNamesForAssociation's null-fallback check) is confirmed dead code - that method can never return null. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ntry point JpaGormEntityTransformation sat at 14% coverage. The existing spec only exercised it indirectly via GlobalJpaEntityTransform, which applies it to classes that already carry @jakarta.persistence.Entity - so the class's own local, @grails.gorm.annotation.JpaEntity-driven entry point (visit(ASTNode[], SourceUnit)) and the branch that actually adds the missing @entity annotation never ran. Add three specs: a class annotated @JpaEntity without @entity (proving the local path adds the annotation and applies GORM entity enhancement), a class already carrying both annotations (proving the annotation isn't added twice), and priority() ordering. Coverage moves 14% -> 60% instruction, ~0% -> 58% branch. The two remaining uncovered lines are defensive guards (malformed astNodes array, non-matching annotation type) unreachable through any class Groovy's own local-transform dispatch would actually produce. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… method AbstractGormASTTransformation sat at 53% coverage despite being the shared base for every GORM annotation-driven AST transform - it was only ever exercised indirectly through subclasses (TenantTransform, RollbackTransform, etc.), all of which only drive its "normal" path: a matching annotation on a not-yet-visited node. Add a spec with a minimal test-only subclass and call the class's own public visit(ASTNode[], SourceUnit) template method directly, covering the two branches no subclass's tests happened to exercise: an annotation that doesn't match the subclass's declared annotation type, and a node that was already visited once (the applied-marker idempotency guard). Also covers getOrder()'s delegation to priority(). Coverage moves 53% -> 61% instruction, 40% -> 68% branch. The one remaining uncovered line (a malformed-astNodes defensive guard) is, like the equivalent guards found elsewhere in this branch's other transform specs, unreachable through any class Groovy's own local-transform dispatch would actually produce. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…fault trait-weaving path AbstractTraitApplyingGormASTTransformation sat at 57% coverage. Its only concrete subclass in this module, ServiceTransformation, overrides shouldWeave with its own logic and calls the static weaveTraitWithGenerics directly rather than through the instance weaveTrait method - so the base class's own default behavior (shouldWeave returning true, weaveTrait delegating to Groovy's TraitComposer, and several weaveTraitWithGenerics edge branches: no-generics traits, interface class nodes, and partial/full generic-arity mismatches) was never exercised. Add a spec covering these directly: the generics edge cases against bare ClassNodes (same technique as the sibling AbstractGormASTTransformationSpec), and the instance weaveTrait method - including the real TraitComposer.doExtendTraits call - by compiling a class through a test-only local transform (TestTraitWeavingTransformation, applied via ApplyTestTraitWeaving) and asserting the compiled class actually gained the woven trait's method, following the same local-transform-testing pattern used for DetachedCriteriaASTTransformation earlier in this branch. Coverage moves 57% -> 98% instruction, 40% -> 72% branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
DirtyCheckTransformation's own visit() logic was already well covered by the extensive existing spec (its @DirtyCheck-driven normal path is exercised throughout), but priority() itself was never called directly. Add a one-line test for it. Coverage moves 58% -> 61% instruction. The two remaining uncovered branches (a malformed-astNodes defensive guard, and an annotation-type mismatch early-return) are unreachable through any real @DirtyCheck usage - the annotation's @target(TYPE) restriction and Groovy's own local-transform dispatch guarantee those conditions can't occur, matching the same pattern already found in this branch's other local AST transform specs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… closure-resolver path Writing a test for @tenant's closure-based tenant resolution (the one branch of TenantTransform#buildDelegatingMethodCall the existing CurrentTenant/WithoutTenant specs never exercised) surfaced a real bug: supplying a non-closure @tenant value crashed the compiler with an internal NullPointerException wrapped as a GroovyBugError instead of a clean compile error. Root cause: the error path called the inherited AbstractASTTransformation#addError(String, ASTNode), which reads from that class's own sourceUnit field - never populated anywhere in this transform's call chain, since sourceUnit is threaded through as a method parameter instead. Fixed by reporting the error directly through that parameter's error collector, matching the pattern already used elsewhere in this codebase for AST transforms that don't rely on the inherited field. Also add tests for @tenant applied at both method and class level, the now-fixed non-closure error path, getAnnotationType(), and the two hasTenantAnnotation branches (a method with @WithoutTenant, and being called directly with a bare ClassNode) that weren't reached by any existing spec. Coverage moves 65% -> 95% instruction, 67% -> 82% branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…coverage OrderedGormTransformation is the shared dispatcher every GORM annotation-driven transform routes through (@tenant, @CurrentTenant, @WithoutTenant, @transactional, @Rollback, @readonly), but it was only ever exercised via real transforms that are all CompilationUnitAware - so the branch of collectAndOrderGormTransformations taken for a discovered transform that ISN'T CompilationUnitAware, and the catch block that runs when a transform's GormASTTransformationClass name can't be loaded, were both untested. Writing a test for the unloadable-transform-name path surfaced the same bug fixed earlier in TenantTransform: the catch block calls the inherited AbstractASTTransformation#addError(String, ASTNode), which reads that class's own sourceUnit field - never populated here, since visit() never called init() to set it. Any misconfigured or broken custom GORM transform reference would crash the compiler with an internal NullPointerException instead of a clean error message. Fixed with a one-line call to the inherited init(astNodes, source), the idiomatic way AbstractASTTransformation subclasses are meant to populate that field. Add a spec covering both previously-unexercised branches plus priority(), using test-only marker annotations/transforms following the same local-transform-testing pattern used elsewhere in this branch. Coverage moves 70% -> 82% instruction, 71% -> 73% branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… descriptor-writing paths ServiceTransformation sat at 77%/58% coverage. Add tests for several previously-unexercised paths: - The constructor-validation error on abstract data services (confirmed already safe - it uses addErrorAndContinue, not the crash-prone pattern found and fixed elsewhere in this branch). - ServiceImplementerAdapter loading/deduplication and the AdaptedImplementer handling, via test-only ServiceLoader-registered fixtures (support/) that only ever match a deliberately obscure method name so they can't interfere with any other @service in the module's test suite. - generateServiceDescriptor's real file-writing path (creating and appending to a META-INF/services descriptor), using a real target directory pointed at a temp dir so nothing leaks into the real build output. - Domain mapping-closure resolution edge cases (non-closure mapping value, unrelated leading statements before the datasource call, a mapping closure that never calls datasource) and the generated-method-replaces-user-override cleanup path, added to the existing ConnectionRoutingServiceTransformSpec alongside its other mapping/connection-routing coverage. - priority(). Two branches (the implementers=/adapters= annotation members) were left uncovered: ServiceTransformation.LOADED_IMPLEMENTORS is a static field populated once per test JVM by whichever @service compiles first, which makes those branches unreachable without either depending on test execution order or reflectively resetting internal state - both of which this branch's testing conventions rule out. Coverage moves 77% -> 88% instruction, 58% -> 71% branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Cleanup pass addressing IntelliJ's inspection warnings, all semantic-preserving: - Unused catch parameter renamed to 'ignored'. - 11 unnecessary qualified references removed (Modifier.PUBLIC, GeneralUtils.*, and AstUtils.isDomainClass were all already available via existing static imports; the fully-qualified org.codehaus.groovy.transform.trait.TraitComposer reference was replaced with a proper import). The now-unused plain imports for Modifier, GeneralUtils, and AstUtils were removed. - 8 helper methods that don't touch instance state made static (isDefinedInTransientsNode, resolvePropertyReturnType, isAnnotatedWithJavaValidationApi, getGetterAndSetterForPropertyName, isSetter, isGetter, weaveIntoExistingSetter, createMarkDirtyMethodCall). The nested GetterAndSetter class was also made static since it never referenced the enclosing instance - required once its factory method became static. - isDefinedInTransientsNode given an explicit `return false` for the branch that previously fell through with no return value on a boolean-returning method. - 6 .equals() calls on Groovy value types replaced with ==, which is equivalent here (Groovy's == is equals()-based with added null-safety, not Java reference equality). Verified via a full, unfiltered module test suite run - no test changes were needed since none of this altered behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…() guard - Removed the unused MY_TYPE_NAME field (dead since it was added; never referenced anywhere in the codebase). - Replaced .equals() with == for the ClassNode comparison. - Extracted the 12-line visit(ASTNode[], SourceUnit) validation guard duplicated verbatim between DirtyCheckTransformation and JpaGormEntityTransformation into a new shared LocalTransformationSupport.resolveAnnotatedClassOrNull, used by both. Behavior is unchanged - same malformed-type guard, same annotation-type/ClassNode checks, same early-return contract. Extracting this logic out of the two AST transforms into a plain static method made it directly unit-testable for the first time (previously only reachable, if at all, through real compilation). Added LocalTransformationSupportSpec covering the reachable branches. One branch remains uncovered and is called out explicitly in the spec's docs rather than silently skipped: the malformed-astNodes-shape guard casts both array slots before checking their type, so any input that would fail the check throws a plain ClassCastException from the cast itself first - the intended RuntimeException can never actually be constructed. This is a pre-existing latent issue inherited unchanged from both original call sites, not introduced here. Verified via a full, unfiltered module test suite run - no behavior change for real compilation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…alTransformationSupport The guard cast both array slots to their expected type before checking whether they actually were that type, so any input that would fail the check threw a plain ClassCastException from the cast itself first - the intended RuntimeException could never actually be constructed. Removed the dead branch and left a comment explaining why the two casts are trusted rather than defensively checked. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Removed the unused public SERIALIZABLE_CLASS_NODE field (confirmed dead repo-wide). - Deduplicated the 17-line visit(ASTNode[], SourceUnit) guard - the same pattern already extracted from DirtyCheckTransformation and JpaGormEntityTransformation - by routing this class through LocalTransformationSupport.resolveAnnotatedClassOrNull too. - Replaced the fully-qualified org.codehaus.groovy.transform.trait.TraitComposer reference with a proper import. - Removed a dead `= null` initializer on gormEntityTrait that every branch immediately overwrote before any read. - Renamed two unused catch parameters. The nested try/catch in visit(ClassNode, SourceUnit) needed distinct names to avoid a scope collision, and to preserve CodeNarc's EmptyCatchBlock exemption (keyed to the literal name `ignored`) on the genuinely-empty inner catch. - Removed the unused getAssociationMethodNode parameter from injectAssociationsForJpaEntity and its call site. - Made 13 private/protected helper methods static; none touch instance state (the sole instance field is compilationUnit). - Replaced 7 .equals() calls on Groovy value types with == (Groovy's == is equals()-based with added null-safety here, not Java reference equality). Verified via a full, unfiltered module test suite run - no behavior change. Caught and fixed two issues along the way: a TraitComposer import that was accidentally dropped mid-edit, and a CodeNarc EmptyCatchBlock violation introduced by renaming the inner catch parameter away from the exemption-matching name. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens several compile-time GORM AST transformations in grails-datamapping-core (preventing internal compiler crashes on malformed inputs) and substantially expands Spock test coverage across the AST transform surface, including GORM query transforms and service/data-source routing behavior. It also documents the protectSqlInjectionAttacks system-property kill switch in the security guide.
Changes:
- Fixes AST transform error/reporting paths to emit proper compilation errors instead of crashing (notably
OrderedGormTransformationand@Tenanthandling). - Adds extensive, focused Spock specs to exercise previously-uncovered branches in core GORM transforms (trait weaving, ordering/dispatch, where-query rewriting, service transform/adapters).
- Introduces a shared
LocalTransformationSupportguard used by multiple local annotation-driven transforms and updates docs for the SQL-injection safety transform kill switch.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| grails-doc/src/en/guide/security/securingAgainstAttacks.adoc | Documents protectSqlInjectionAttacks=false as a last-resort build-wide kill switch. |
| grails-datamapping-core/src/test/resources/META-INF/services/org.grails.datastore.gorm.services.ServiceImplementerAdapter | Registers test-only ServiceImplementerAdapter implementations for ServiceLoader-based tests. |
| grails-datamapping-core/src/test/resources/META-INF/services/org.grails.datastore.gorm.services.ServiceImplementer | Registers a test-only ServiceImplementer for ServiceLoader-based tests. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/TestWeavableTrait.groovy | Test-only non-generic trait used to exercise trait weaving. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/TestTraitWeavingTransformation.groovy | Test-only local AST transform to validate real compilation-unit trait weaving. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/SingleGenericTestTrait.groovy | Test-only trait with one generic parameter for generics-arity edge cases. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/DoubleGenericTestTrait.groovy | Test-only trait with two generic parameters for partial-argument padding behavior. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/OrderedGormTransformationSpec.groovy | Covers OrderedGormTransformation dispatch branches + unloadable transform error path. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/NonCompilationUnitAwareTestTransformation.groovy | Test transform that intentionally does not implement CompilationUnitAware. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/ApplyUnloadableGormTransform.java | Marker annotation that forces a missing transform class to test error reporting. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/ApplyTestTraitWeaving.java | Marker annotation used to trigger the test trait-weaving transform. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/ApplyNonCompilationUnitAwareTransform.java | Marker annotation used to route via OrderedGormTransformation to a non-CUA transform. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AbstractTraitApplyingGormASTTransformationSpec.groovy | Directly tests default shouldWeave, weaveTraitWithGenerics, and real unit weaving. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AbstractGormASTTransformationSpec.groovy | Covers AbstractGormASTTransformation template visit early returns and applied-marker logic. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/services/transform/support/ProbeServiceImplementerAdapter.groovy | Test adapter that wraps a test implementer to exercise adapted-implementer handling. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/services/transform/support/ProbeServiceImplementer.groovy | Test implementer loaded via ServiceLoader to drive ServiceTransformation adapter logic. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/services/transform/support/NoOpServiceImplementerAdapter.groovy | Second inert adapter to exercise adapter de-duplication behavior. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/services/transform/support/AdaptedProbeServiceImplementer.groovy | Adapted implementer with deterministic ordering to ensure branch coverage. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/services/transform/ServiceTransformationSpec.groovy | Adds compile-time specs for constructor validation, adapters, descriptor writing, priority. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryStaticFieldSpec.groovy | Exercises statement-kind flattening and field-initializer rewrite paths. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryPropertyAliasSpec.groovy | AST-inspects alias-driven *Property rewrites at canonicalization phase. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryOperatorSpec.groovy | Validates operator-to-criterion rewriting by asserting exact Query.Criterion objects. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryNegationSpec.groovy | Covers negation rewrite + compile error for invalid operand shape. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryMultiLevelAssociationFunctionSpec.groovy | Covers function-call dispatch through multi-level association property paths (AST inspection). |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryFunctionCallSpec.groovy | Covers direct-property function calls + structural association-case coverage + RHS error case. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryEmbeddedPropertyPathSpec.groovy | AST-inspects embedded-property dotted-path rewrite + unknown-property compile error. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryDetachedCriteriaCastSpec.groovy | Covers closure-cast rewrite on fields and local vars + unknown-property error. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryAssociationPathSpec.groovy | Structural coverage for single/multi-level association closure synthesis + invalid cases. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryAggregateSubqueryErrorSpec.groovy | Covers aggregate direct-arg validation and property() subquery operator rewrite behavior. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/DetachedCriteriaASTTransformationSpec.groovy | Isolates local transform behavior by disabling the global transform and asserting closure capture. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/dirty/checking/DirtyCheckTransformationSpec.groovy | Adds explicit priority assertion for DirtyCheck transform ordering. |
| grails-datamapping-core/src/test/groovy/org/grails/compiler/gorm/LocalTransformationSupportSpec.groovy | Unit tests for the new shared local-transform guard helper. |
| grails-datamapping-core/src/test/groovy/org/grails/compiler/gorm/JpaEntityTransformSpec.groovy | Adds coverage for local @JpaEntity entry-point behavior + priority ordering. |
| grails-datamapping-core/src/test/groovy/grails/gorm/services/ConnectionRoutingServiceTransformSpec.groovy | Adds edge-case coverage for mapping values, mixed statements, and tx-manager override behavior. |
| grails-datamapping-core/src/test/groovy/grails/gorm/annotation/multitenancy/TenantTransformSpec.groovy | Adds coverage for @Tenant closure-valued branch + non-closure error path + hasTenantAnnotation. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/OrderedGormTransformation.groovy | Initializes transform state via init(...) to prevent NPEs during error reporting. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/multitenancy/transform/TenantTransform.groovy | Replaces crashing error path with a proper SyntaxErrorMessage bound to source positions. |
| grails-datamapping-core/src/main/groovy/org/grails/compiler/gorm/LocalTransformationSupport.groovy | New shared helper for local transform guards (visit(ASTNode[], SourceUnit)). |
| grails-datamapping-core/src/main/groovy/org/grails/compiler/gorm/JpaGormEntityTransformation.groovy | Uses LocalTransformationSupport to simplify local entry-point guarding. |
| grails-datamapping-core/src/main/groovy/org/grails/compiler/gorm/GormEntityTransformation.groovy | Uses LocalTransformationSupport, refines comparisons/qualifiers, and standardizes TraitComposer usage. |
| grails-datamapping-core/src/main/groovy/org/grails/compiler/gorm/DirtyCheckTransformation.groovy | Uses LocalTransformationSupport to simplify local entry-point guarding + retains priority ordering. |
| grails-datamapping-core/src/main/groovy/org/grails/compiler/gorm/DirtyCheckingTransformer.groovy | Cleanup/refactor (static helpers, comparisons, TraitComposer usage) while preserving behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 8.1.x #16148 +/- ##
==================================================
+ Coverage 51.3608% 53.1152% +1.7545%
- Complexity 18145 18966 +821
==================================================
Files 2079 2080 +1
Lines 97189 97200 +11
Branches 16871 16871
==================================================
+ Hits 49917 51628 +1711
+ Misses 39915 38173 -1742
- Partials 7357 7399 +42
🚀 New features to boost your workflow:
|
jdaugherty
reviewed
Aug 14, 2026
jdaugherty
left a comment
jdaugherty
left a comment
Contributor
There was a problem hiding this comment.
Coverage work here is welcome and the two compiler-crash fixes are real. A few things to tighten before this goes in.
One carry-over from the review of #16135 (which this stack sits on): the same question about overlap with existing tests applies here. grails-datamapping-core-test/src/test/groovy/grails/gorm/tests/WhereMethodSpec.groovy is a 2000-line functional spec over the same where-query surface, and this PR adds ten new WhereQuery*Spec files against the transform. Please call out in the description what these cover that WhereMethodSpec (and the existing WhereQueryClosureCaptureSpec / WhereQueryEmbeddedBlockTransformSpec) does not, so we are not maintaining two parallel suites for the same behaviour.
The other theme is the same one raised on #16140: several of the new specs assert against internals (the extracted helper, protected/static transform methods, generated closure class names) rather than the behaviour a user's compilation actually produces. Details inline.
| } | ||
| else { | ||
| addError('@Tenant value should be a closure', annotationNode) | ||
| sourceUnit.getErrorCollector().addErrorAndContinue( |
Contributor
There was a problem hiding this comment.
This is exactly AstUtils.error(SourceUnit, ASTNode, String), which already exists (grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/reflect/AstUtils.groovy:818) and builds the same SyntaxErrorMessage/SyntaxException pair from the node's positions. Please call it instead of hand-rolling it here:
AstUtils.error(sourceUnit, annotationNode, '@Tenant value should be a closure')That also drops the two new imports.
| throw new RuntimeException("Internal error: wrong types: ${astNodes[0].getClass()} / ${astNodes[1].getClass()}") | ||
| } | ||
|
|
||
| init(astNodes, source) |
Contributor
There was a problem hiding this comment.
init(...) re-checks the exact condition the hand-rolled guard three lines above already rejected — AbstractASTTransformation.init throws GroovyBugError when nodes[0] is not an AnnotationNode or nodes[1] is not an AnnotatedNode. Now that init is being called, the manual instanceof check and its RuntimeException are dead weight; move init(astNodes, source) to the top of the method and delete the guard.
| private static MethodNode REMOVE_FROM_METHOD_NODE = GORM_ENTITY_CLASS_NODE.getMethods('removeFrom').get(0) | ||
| private static MethodNode GET_ASSOCIATION_ID_METHOD_NODE = GORM_ENTITY_CLASS_NODE.getMethods('getAssociationId').get(0) | ||
| public static final Parameter[] ADD_TO_PARAMETERS = [new Parameter(AstUtils.OBJECT_CLASS_NODE, 'obj')] as Parameter[] | ||
| public static final ClassNode SERIALIZABLE_CLASS_NODE = ClassHelper.make(Serializable).getPlainNodeReference() |
Contributor
There was a problem hiding this comment.
SERIALIZABLE_CLASS_NODE is public static final on a shipped class, so removing it is a source- and binary-compatibility break for anything outside this repo (there are no in-repo callers). Same category of change in DirtyCheckingTransformer: weaveIntoExistingSetter, createMarkDirtyMethodCall, getGetterAndSetterForPropertyName and isAnnotatedWithJavaValidationApi go from protected to protected static (subclasses that override them stop compiling), and GetterAndSetter goes from an inner class to a static nested class (its constructor signature changes).
If that is intentional for 8.1.x, fine — but it is a behaviour-neutral cleanup commit carrying an API break, so please say so in the description rather than leaving it in a "fix IntelliJ warnings" commit.
| bookService.listBooks() | ||
|
|
||
| then: 'an exception was thrown because GORM is not setup, proving the delegating call was generated and reached' | ||
| thrown(IllegalStateException) |
Contributor
There was a problem hiding this comment.
This assertion does not prove what the then: label claims. buildDelegatingMethodCall emits the ServiceRegistry.targetDatastore(...).getService(TenantService) declaration as the first statement of the rewritten body on every branch, so the IllegalStateException is raised before the closure is ever cloned or called. The spec would pass identically if the @Tenant closure branch were never taken — including on the non-closure error path this PR just changed.
Assert something that only the closure branch produces: inspect the rewritten MethodNode (you already have compileAndFindMethod in this spec) for the $tenantResolver / $tenantId declarations, or check the generated method's parameter list. Same for the class-level test at line 81.
| noExceptionThrown() | ||
|
|
||
| and: 'one closure for the outer where-block and one nested closure for the association segment walked' | ||
| List<Class<?>> queryClosures = findQueryClosures(gcl, 'findByAuthorName').sort { it.name.count('$_closure') } |
Contributor
There was a problem hiding this comment.
These assertions are pinned to Groovy's generated closure class-naming scheme — the _<methodName>_ substring and the number of $_closure occurrences in the class name. That is compiler-internal naming, not output of DetachedCriteriaTransformer: a change to how Groovy names nested closures silently breaks these tests, and a change to the transform that stops synthesizing association closures could still leave the counts intact.
The other specs in this PR (e.g. WhereQueryEmbeddedPropertyPathSpec) inspect the transformed AST at canonicalization instead — please use that approach here so the assertion is about the nested delegate.<association> { ... } calls actually generated.
| LocalTransformationSupport.resolveAnnotatedClassOrNull([annotationNode, targetClass] as ASTNode[], ANNOTATION_TYPE) == null | ||
| } | ||
|
|
||
| void "returns null when the annotated node is not a class"() { |
Contributor
There was a problem hiding this comment.
Two things here.
This branch cannot occur in production: @DirtyCheck, @Entity and @JpaEntity are all declared @Target([ElementType.TYPE]), so Groovy never dispatches these local transforms on a FieldNode. That sits oddly beside commit 53e1767, which removed the malformed-astNodes guard from this same class precisely because it was structurally unreachable — the same argument applies to the !(parent instanceof ClassNode) half of the check this spec exists to cover.
More generally, the whole spec drives an extracted internal helper directly, which is what CLAUDE.md rule 9 asks us to avoid; the guard is already exercised end-to-end whenever a source annotated with @DirtyCheck / @Entity / @JpaEntity is compiled. Also worth fixing the class javadoc: it names DirtyCheckTransformation and JpaGormEntityTransformation, but GormEntityTransformation uses the helper too.
- TenantTransform: use AstUtils.error() instead of hand-rolling a
SyntaxErrorMessage/SyntaxException.
- OrderedGormTransformation: drop the now-dead instanceof guard now
that init(astNodes, source) runs first and covers the same check.
- Revert protected/public instance methods and fields that had
silently become static (GormEntityTransformation.injectVersionProperty/
injectIdProperty/getOrCreateListProperty/SERIALIZABLE_CLASS_NODE,
DirtyCheckingTransformer.weaveIntoExistingSetter/
createMarkDirtyMethodCall/getGetterAndSetterForPropertyName/
isAnnotatedWithJavaValidationApi/GetterAndSetter) to avoid a
source/binary compatibility break hidden in a warnings-cleanup
commit; private-only static conversions are unaffected.
- TenantTransformSpec: assert on the rewritten method's AST (presence
of a $tenantResolver local) instead of catching IllegalStateException,
which fired identically on every branch and proved nothing about the
closure branch specifically.
- WhereQueryAssociationPathSpec: assert on the actual nested
delegate.<association> { ... } calls generated, instead of pinning
to Groovy's internal closure class-naming scheme.
- Delete LocalTransformationSupportSpec: it drove the extracted
internal helper directly, which CLAUDE.md rule 9 asks us to avoid;
the guard is already exercised end-to-end via @DirtyCheck/@Entity/
@JpaEntity compilation in existing specs. Documented all three
callers on LocalTransformationSupport's class javadoc instead.
Verified via a full grails-datamapping-core test run (2175 tests,
0 failures/errors) plus clean codeStyle/codenarcMain.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
✅ All tests passed ✅🏷️ Commit: 5503c15 Learn more about TestLens at testlens.app. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated no new comments.
Suppressed comments (1)
grails-datamapping-core/src/main/groovy/org/grails/compiler/gorm/LocalTransformationSupport.groovy:61
- resolveAnnotatedClassOrNull currently casts astNodes[0]/[1] before any shape/type checks. This can still crash the compiler with ArrayIndexOutOfBoundsException/ClassCastException if the transform is invoked with an unexpected node array (even if that “shouldn’t happen”). Since this helper exists to prevent internal crashes, it should defensively validate astNodes length and element types before casting.
static ClassNode resolveAnnotatedClassOrNull(ASTNode[] astNodes, ClassNode expectedAnnotationType) {
// Groovy's local-transform dispatch always supplies [AnnotationNode, AnnotatedNode] here,
// so these casts are trusted rather than defensively checked first: a shape mismatch would
// throw ClassCastException from the cast itself, before any check could run anyway.
AnnotationNode node = (AnnotationNode) astNodes[0]
AnnotatedNode parent = (AnnotatedNode) astNodes[1]
if (expectedAnnotationType != node.getClassNode() || !(parent instanceof ClassNode)) {
return null
jdaugherty
approved these changes
Aug 16, 2026
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.
Summary
transform classes (the
query/transformpackage plusDirtyCheckTransformation,JpaGormEntityTransformation,AbstractGormASTTransformation,AbstractTraitApplyingGormASTTransformation,OrderedGormTransformation,TenantTransform,ServiceTransformation) — several classes move from 0-70%to 80-100% instruction coverage.
@TenantandOrderedGormTransformation's error-reporting paths threw an internalNullPointerException/GroovyBugError instead of a clean compile error on malformed
input, both traced to the same root cause (an inherited
sourceUnitfield that wasnever populated).
DirtyCheckingTransformer,DirtyCheckTransformation, andGormEntityTransformation(unused code, redundantqualifiers,
equals()vs==, methods that can be static), and extracts aduplicated
visit()guard shared across three transform classes into a newLocalTransformationSupporthelper.protectSqlInjectionAttackskill-switch system property in thesecurity guide.
Test plan
grails-datamapping-coretest suite green throughout(
./gradlew :grails-datamapping-core:test)codeStyle+codenarcMain/codenarcTestclean🤖 Generated with Claude Code