Search every interface when checking whether a class node implements one by codeconsole · Pull Request #16401 · apache/grails-core

codeconsole · 2026-09-25T06:33:48Z

GrailsASTUtils.isSubclassOfOrImplementsInterface and the datastore's AstUtils.findInterface walk a class node's interfaces. For the first interface that has super-interfaces of its own, they returned whatever the search of those super-interfaces found, and never looked at the interfaces after it:

ClassNode[] childInterfaces = anInterface.getInterfaces();
if (childInterfaces != null && childInterfaces.length > 0) {
    return implementsInterfaceInternal(childInterfaces, interfaceName);   // false ends the search
}

So a class declaring implements Unrelated, Target, where Unrelated extends anything, was reported as not implementing Target. The same happened to one reaching Target through an interface that extends two, when the first of those has super-interfaces of its own. Both now search every interface.

This came up in #16399. The unit-test detection there needed a check like this and uses Groovy's own ClassNode#implementsInterface instead, so this fix is separate.

What the callers decide

FactoriesFileWriter: which classes are listed in grails.factories as ArtefactHandler or TraitInjector implementations.
BootInitializerClassInjector: whether a class is a GrailsPluginApplication, which gets no generated boot initializer.
GormEntityTransformation (through AstUtils.findInterface): whether a domain class is an RxEntity, which gets no injected id and version.

Each would have missed a class in that position. Nothing in this repository is in it. I compiled every module that implements ArtefactHandler or TraitInjector before and after the change, and all 16 generated grails.factories files are byte-identical.

Validation

./gradlew :grails-core:codeStyle :grails-datastore-core:codeStyle \
          :grails-core:test :grails-datastore-core:test :grails-datamapping-core:test \
          :grails-web-boot:test :grails-controllers:test :grails-test-suite-uber:test --continue

New cases in GrailsASTUtilsSpec (the target after an interface with its own super-interfaces, the same one level deeper, the same reached through a superclass, plus direct and negative cases) and in AstUtilsSpec. Without the fix, the three bug cases and the findInterface case fail, and the direct and negative cases pass.
3,080 tests, 0 failures, all executed fresh: grails-core 688, grails-datastore-core 277, grails-datamapping-core 1,333, grails-web-boot 3, grails-controllers 203, test-suite-uber 576.
grails.factories comparison: :classes for grails-codecs, -controllers, -core, -datamapping-support, -domain-class, -taglib, -web-taglib, -interceptors, -quartz, -rest-transforms, -web-core, -web-databinding and -web-boot, before and after. 24 compile tasks re-ran against the fixed transform, and all 16 main-source grails.factories files are byte-identical.
Checkstyle and CodeNarc: no violations in either module.

GrailsASTUtils.isSubclassOfOrImplementsInterface and the datastore's AstUtils.findInterface walk a class's interfaces, and for the first one with super-interfaces of its own they returned whatever the search of those super-interfaces found, without looking at the interfaces after it. A class declaring "implements Unrelated, Target", where Unrelated extends anything, was reported as not implementing Target; so was one reaching Target through an interface that extends two, the first with its own super-interfaces. The callers decide real things: FactoriesFileWriter which classes are listed in grails.factories as ArtefactHandler or TraitInjector implementations, BootInitializerClassInjector whether a class is a GrailsPluginApplication, and GormEntityTransformation whether a domain class is an RxEntity and so gets no injected id and version. Each would have missed a class in that position. Nothing in this repository is: every generated grails.factories is byte-identical before and after.

jdaugherty

Verified locally on 7a81bae: the three bug cases in GrailsASTUtilsSpec and the findInterface case in AstUtilsSpec fail against the unfixed sources and pass with the fix. The grails-core, grails-datastore-core and grails-web-boot tests and codeStyle are green.

@matrei the same early return is on 7.0.x, in both GrailsASTUtils.implementsInterfaceInternal and the datastore AstUtils.implementsInterfaceInternal. Should we backport this?

jdaugherty · 2026-09-25T06:43:37Z

            ClassNode[] childInterfaces = anInterface.getInterfaces();
-            if (childInterfaces != null && childInterfaces.length > 0) {
-                return implementsInterfaceInternal(childInterfaces, interfaceName);
+            if (childInterfaces != null && childInterfaces.length > 0 &&
+                    implementsInterfaceInternal(childInterfaces, interfaceName)) {
+                return true;
            }


Optional: Groovy's ClassNode#implementsInterface(ClassNode) already does this traversal. It walks the superclass chain, declaresInterface searches every super-interface, and ClassNode equality is by name. So implementsInterface above could be

private static boolean implementsInterface(ClassNode classNode, String interfaceName) { return classNode.implementsInterface(ClassHelper.make(interfaceName)); }

and implementsInterfaceInternal removed. #16399 uses the same API for its check.

Done in 398c123: implementsInterface now returns classNode.implementsInterface(ClassHelper.make(interfaceName)), and implementsInterfaceInternal is gone. I checked it against Groovy 5.1.0's ClassNode (superclass walk, declaresInterface over every super-interface, equality by name). The GrailsASTUtilsSpec cases pass, and with the dependent modules' compileGroovy forced to re-run, every generated grails.factories is still byte-identical to 8.0.x.

jdaugherty · 2026-09-25T06:43:37Z

+            // Every interface is searched. Returning the first one's super-interfaces' answer missed
+            // an interface listed after one that has super-interfaces of its own.
            ClassNode[] childInterfaces = anInterface.getInterfaces()
            if (childInterfaces != null && childInterfaces.length > 0) {
-                return implementsInterfaceInternal(childInterfaces, interfaceName)
+                ClassNode found = implementsInterfaceInternal(childInterfaces, interfaceName)
+                if (found != null) {
+                    return found
+                }
            }


Nit: this comment describes the old bug rather than the code, so it fits better in the commit message (same for the one in GrailsASTUtils if that helper stays). The length > 0 check is also redundant now, since recursing on an empty array returns null:

Suggested change

// Every interface is searched. Returning the first one's super-interfaces' answer missed

// an interface listed after one that has super-interfaces of its own.

ClassNode[] childInterfaces = anInterface.getInterfaces()

if (childInterfaces != null && childInterfaces.length > 0) {

return implementsInterfaceInternal(childInterfaces, interfaceName)

ClassNode found = implementsInterfaceInternal(childInterfaces, interfaceName)

if (found != null) {

return found

}

}

ClassNode[] childInterfaces = anInterface.getInterfaces()

if (childInterfaces != null) {

ClassNode found = implementsInterfaceInternal(childInterfaces, interfaceName)

if (found != null) {

return found

}

}

Applied in 398c123: the comment moved to the commit message and the length check is dropped. findInterface keeps its own walk, since it returns the interface node it finds, which ClassNode#implementsInterface doesn't give back.

GrailsASTUtils.implementsInterface now asks Groovy's ClassNode implementsInterface, which walks the superclass chain and searches every super-interface, so the hand-written walk is gone. In the datastore's AstUtils, which returns the interface node it finds, the length check is redundant (recursing on an empty array finds nothing) and the comment described the old bug rather than the code.

codecov · 2026-09-25T07:46:38Z

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 57.6117%. Comparing base (4f1a5c8) to head (398c123).

Files with missing lines	Patch %	Lines
...g/grails/datastore/mapping/reflect/AstUtils.groovy	75.0000%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##                8.0.x     #16401        +/-   ##
==================================================
+ Coverage     57.5998%   57.6117%   +0.0118%     
+ Complexity      22697      22693         -4     
==================================================
  Files            2133       2133                
  Lines          104226     104215        -11     
  Branches        18692      18689         -3     
==================================================
+ Hits            60034      60040         +6     
+ Misses          35906      35890        -16     
+ Partials         8286       8285         -1

Files with missing lines	Coverage Δ
.../org/grails/compiler/injection/GrailsASTUtils.java	`57.1202% <100.0000%> (-0.5542%)`	⬇️
...g/grails/datastore/mapping/reflect/AstUtils.groovy	`69.6370% <75.0000%> (+1.5307%)`	⬆️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

testlens-app · 2026-09-25T08:29:21Z

✅ All tests passed ✅

🏷️ Commit: 398c123
▶️ Tests: 82233 executed
⚪️ Checks: 88/88 completed

Learn more about TestLens at testlens.app/docs.

matrei

Review of #16401: Search every interface when checking whether a class node implements one

Head: 398c1230d3 (2 commits) · Base: 8.0.x · merges cleanly, CI green.

What I ran (on the head): :grails-core:test --tests GrailsASTUtilsSpec (14 tests, 0 failures) and :grails-datastore-core:test --tests AstUtilsSpec (6 tests, 0 failures).

The fix looks right to me. Delegating GrailsASTUtils.implementsInterface to ClassNode#implementsInterface is the better end state. In Groovy 5.1.3 it walks the superclass chain down to null, and declaresInterface checks every direct interface before recursing into each one. ClassNode#equals compares by name, so the node from ClassHelper.make(interfaceName) matches exactly what the old name comparison matched. Keeping the hand-written walk in AstUtils.findInterface makes sense, because it has to return the node it found, and the new loop now falls through correctly when a branch finds nothing.

I also looked for other hand-rolled interface walkers with the same early return. AstUtils.findAbstractMethodsInternal, GrailsBeansASTTransformation.collectMethodNames/findStaticFinalField and the CLI's hasAtLeastOneInterface all either recurse into every interface or intentionally check only direct ones. So these two helpers are the only ones affected.

A few small things, none blocking:

1. 7.0.x has the same bug

Both implementsInterfaceInternal helpers on 7.0.x still return the result of the first interface with super-interfaces (GrailsASTUtils.java:753 and AstUtils.groovy:798). Since this is a bug fix, should it target 7.0.x and be merged forward? If not, it could be backported there. The change applies as-is, because ClassNode#implementsInterface and declaresInterface are the same in Groovy 4.0.x.

2. PR description

"What the callers decide" leaves out GlobalGrailsClassInjectorTransformation:163, which calls isSubclassOfOrImplementsInterface(classNode, 'grails.boot.config.GrailsAutoConfiguration'). That is a class, so the isSubclassOf half decides it and the interface walk doesn't change anything. It's still worth listing for completeness.
The snippet and "Both now search every interface" describe the first commit. After 398c1230d3, the GrailsASTUtils half defers to Groovy instead. It would help to reword this before the squash, so the merged commit message matches the code.

3. Test nits

In GrailsASTUtilsSpec, the TARGET/BASE/WITH_SUPER/MIXED constants sit between two feature methods. The spec reads more easily with them at the top of the class, next to any other fields.
The 'the same, reached through a superclass' row pushes the || column out of line with the other rows.

Approving; the 7.0.x question can be settled separately.

github-project-automation Bot added this to Apache Grails Sep 25, 2026

codeconsole requested review from borinquenkid, jdaugherty, matrei and sbglasius September 25, 2026 06:34

codeconsole added this to the grails:8.0.0-RC2 milestone Sep 25, 2026

jdaugherty reviewed Sep 25, 2026

View reviewed changes

codeconsole requested a review from jdaugherty September 25, 2026 06:57

matrei approved these changes Sep 25, 2026

View reviewed changes

jdaugherty merged commit 5fd59fc into apache:8.0.x Sep 25, 2026
91 checks passed

github-project-automation Bot moved this to Done in Apache Grails Sep 25, 2026

Navigation Menu

Uh oh!

Search every interface when checking whether a class node implements one - #16401

Search every interface when checking whether a class node implements one#16401
jdaugherty merged 2 commits into
apache:8.0.xfrom
codeconsole:fix/ast-implements-interface-8.0.x

codeconsole commented Sep 25, 2026

Uh oh!

jdaugherty left a comment

Uh oh!

jdaugherty Sep 25, 2026

Uh oh!

codeconsole Sep 25, 2026

Uh oh!

jdaugherty Sep 25, 2026

Uh oh!

codeconsole Sep 25, 2026

Uh oh!

codecov Bot commented Sep 25, 2026

Uh oh!

testlens-app Bot commented Sep 25, 2026

Uh oh!

matrei left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

codeconsole commented Sep 25, 2026

What the callers decide

Validation

Uh oh!

jdaugherty left a comment

Choose a reason for hiding this comment

Uh oh!

jdaugherty Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

codeconsole Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

jdaugherty Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

codeconsole Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Sep 25, 2026

Codecov Report

Uh oh!

testlens-app Bot commented Sep 25, 2026

✅ All tests passed ✅

Uh oh!

matrei left a comment

Choose a reason for hiding this comment

Review of #16401: Search every interface when checking whether a class node implements one

1. 7.0.x has the same bug

2. PR description

3. Test nits

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants