The GitHub Actions job "Code Style" on grails-core.git/8.1.x has succeeded. Run started by GitHub user borinquenkid (triggered by borinquenkid).
Head commit for run: 75cd61800df1087e131d2fe18156d13af79417f5 / Walter B Duque de Estrada <[email protected]> Add test coverage and fix compiler crashes in GORM AST transforms (#16148) * Add test coverage for DetachedCriteriaASTTransformation and document 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 <[email protected]> * Add broad test coverage for DetachedCriteriaTransformer's where-query 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 <[email protected]> * Close remaining coverage gaps in addCriteriaCall and association property 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 <[email protected]> * Add test coverage for JpaGormEntityTransformation's local transform entry 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 <[email protected]> * Add direct test coverage for AbstractGormASTTransformation's template 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 <[email protected]> * Add test coverage for AbstractTraitApplyingGormASTTransformation's default 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 <[email protected]> * Add priority() coverage for DirtyCheckTransformation 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 <[email protected]> * Fix @Tenant compile-time crash and add coverage for TenantTransform's 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 <[email protected]> * Fix compiler crash in OrderedGormTransformation's error path and add 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 <[email protected]> * Add test coverage for ServiceTransformation's implementer-adapter and 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 <[email protected]> * Fix IntelliJ warnings on DirtyCheckingTransformer 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 <[email protected]> * Fix IntelliJ warnings on DirtyCheckTransformation, dedup shared visit() 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 <[email protected]> * Remove the structurally-unreachable malformed-astNodes guard from LocalTransformationSupport 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 <[email protected]> * Fix IntelliJ warnings on GormEntityTransformation - 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 <[email protected]> * Address PR #16148 review comments from jdaugherty - 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 <[email protected]> --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Report URL: https://github.com/apache/grails-core/actions/runs/31964706061 With regards, GitHub Actions via GitBox
