jdaugherty commented on code in PR #16401:
URL: https://github.com/apache/grails-core/pull/16401#discussion_r4101892298
##########
grails-core/src/main/groovy/org/grails/compiler/injection/GrailsASTUtils.java:
##########
@@ -774,11 +774,13 @@ private static boolean
implementsInterfaceInternal(ClassNode[] interfaces, Strin
if (anInterface.getName().equals(interfaceName)) {
return true;
}
+ // 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);
+ if (childInterfaces != null && childInterfaces.length > 0 &&
+ implementsInterfaceInternal(childInterfaces,
interfaceName)) {
+ return true;
}
Review Comment:
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
```java
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.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/reflect/AstUtils.groovy:
##########
@@ -820,11 +820,15 @@ class AstUtils {
if (anInterface.getName().equals(interfaceName)) {
return anInterface
}
+ // 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
+ }
}
Review Comment:
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`:
```suggestion
ClassNode[] childInterfaces = anInterface.getInterfaces()
if (childInterfaces != null) {
ClassNode found =
implementsInterfaceInternal(childInterfaces, interfaceName)
if (found != null) {
return found
}
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
