MrBean: Fix detection of inherited default method in Java 8+ interface #109
Conversation
…efault methods in Java 8+ interfaces
| try { | ||
| // getMethod returns the most-specific method implementation, for public methods only (which is any method in an interface) | ||
| Method effectiveMethod = implementedType.getRawClass().getMethod(name, argTypes); | ||
| if (BeanUtil.isConcrete(effectiveMethod)) { |
There was a problem hiding this comment.
Quick question on this one: is the other case (not concrete) not conclusive?
That is, why not just return BeanUtil.isConcrete(effectiveMethod) as is, instead of continuing?
I am assuming here that if method is found, it is the visible declaration.
I suspect most of the time continuing would only be wasteful and is unlikely to give wrong answer....
but I have occasionally used pattern of re-abstract'ing methods (esp. toString() / equals() from Object) to force re-implementing by sub-classes.
There was a problem hiding this comment.
Oh, that's an interesting thought. Yes... if the method is returned, then I think you have the definitive answer as to whether the method is concrete for the implemented type.
I have occasionally used pattern of re-abstract'ing methods (esp. toString() / equals() from Object) to force re-implementing by sub-classes.
I think if the implemented type returned one of those "re-abstracted" methods, then it would be abstract, and the MrBean-generated class would be required to provide an implementation. I'm not sure how MrBean would handle this, probably it would throw an error, since it's not a property?
So, to clarify what we're talking about, I think it would be correct to simply return BeanUtil.isConcrete(effectiveMethod) here.
| for (JavaType curr = implementedType; (curr != null) && !curr.isJavaLangObject(); | ||
| curr = curr.getSuperClass()) { | ||
| // 29-Nov-2015, tatu: Avoiding exceptions would be good, so would linear scan | ||
| // be better here? | ||
| try { | ||
| Method effectiveMethod = curr.getRawClass().getDeclaredMethod(name, argTypes); | ||
| if (effectiveMethod != null && BeanUtil.isConcrete(effectiveMethod)) { | ||
| if (BeanUtil.isConcrete(effectiveMethod)) { |
There was a problem hiding this comment.
Currently any "re-abstracted" methods are skipped over by this check. If the concrete method exists on a class other than java.lang.Object, then I think MrBean will find it, and return true, which may be incorrect.
I'll write a test case to confirm this behavior.
There was a problem hiding this comment.
Much appreciated! It's good that we can keep modules up to date with "new" versions of JDK (Java 8 isn't that new of course, but it wasn't around when Mr Bean was written and still isn't the baseline).
This PR is equivalent to #102 but targets
2.11instead. The fix (if you wish to cherry-pick separately) is in e0bfe3f.Problem
When the type to materialize extends/implements some interface that has a
defaultmethod implementation (with a non-bean method name), MrBean will treat the method as if it lacks an implementation.Solution
Use
Class.getMethodto locate the exact method that the implemented type will utilize, which properly handles this case.Verification
I've added several unit tests to cover the fixed behavior, and to verify existing, related behavior:
TestSimpleMaterializedInterfaces.testInheritedDefaultMethodInInterfaceis the test case that exercise the current issue, which is now resolved.TestSimpleMaterializedInterfaces.testDefaultMethodInInterfaceis a simpler test case that is already passing with the existing implementation.TestAbstractClassesandTestAbstractClassesWithOverrideshave been extended to exercise some cases that justify the original code inhasConcreteOverride. These test cases were already passing with the existing implementation.