[JENKINS-62723] Improve Util.isOverridden

It will now deal with final/abstract methods appropriately.
It will also clearly report bad arguments (base not base of derived, or base does not declare the method).
Added a unit test for interface cases.

Small gap to be looked into: overrides provided via default implementation of a (derived) interface.

core/src/main/java/hudson/Util.java

@@ -1432,36 +1432,63 @@ public class Util {
     }
 
     /**
-     * Checks if the method defined on the base type with the given arguments
-     * is overridden in the given derived type.
-     */
-    public static boolean isOverridden(@NonNull Class base, @NonNull Class derived, @NonNull String methodName, @NonNull Class... types) {
-        return !getMethod(base, methodName, types).equals(getMethod(derived, methodName, types));
+     * Checks whether the method defined on the base type with the given arguments is overridden in the given derived
+     * type.
+     *
+     * @param base       The base type.
+     * @param derived    The derived type.
+     * @param methodName The name of the method.
+     * @param types      The types of the arguments for the method.
+     * @return {@code true} when {@code derived} provides the specified method other than as inherited from {@code base}.
+     */
+    public static boolean isOverridden(@NonNull Class<?> base, @NonNull Class<?> derived, @NonNull String methodName, @NonNull Class<?>... types) {
+        // If derived is not a subclass or implementor of base, it can't override any method
+        if (!base.isAssignableFrom(derived)) {
+            throw new IllegalArgumentException("The specified derived class (" + derived.getCanonicalName() + ") does not derive from the specified base class (" + base.getCanonicalName() + ").");
+        }
+        final Method baseMethod = Util.getMethod(base, null, methodName, types);
+        if (baseMethod == null) {
+            throw new IllegalArgumentException("The specified method is not declared by the specified base class (" + base.getCanonicalName() + "), or it is private, static or final.");
+        }
+        final Method derivedMethod = Util.getMethod(derived, base, methodName, types);
+        // the lookup will either return null or the base method when no override has been found (depending on whether
+        // the base is an interface)
+        return derivedMethod != null && derivedMethod != baseMethod;
     }
 
-    private static Method getMethod(@NonNull Class clazz, @NonNull String methodName, @NonNull Class... types) {
-        Method res = null;
+    private static Method getMethod(@NonNull Class<?> clazz, @Nullable Class<?> base, @NonNull String methodName, @NonNull Class<?>... types) {
         try {
-            res = clazz.getDeclaredMethod(methodName, types);
-            // private, static or final methods can not be overridden
-            if (res != null && (Modifier.isPrivate(res.getModifiers()) || Modifier.isFinal(res.getModifiers()) 
-                    || Modifier.isStatic(res.getModifiers()))) {
-                res = null;
+            final Method res = clazz.getDeclaredMethod(methodName, types);
+            final int mod = res.getModifiers();
+            // private and static methods are never ok, and end the search
+            if (Modifier.isPrivate(mod) || Modifier.isStatic(mod)) {
+                return null;
+            }
+            // when looking for the base/declaring method, final is not ok
+            if (base == null && Modifier.isFinal(mod)) {
+                return null;
             }
+            // when looking for the overriding method, abstract is not ok
+            if (base != null && Modifier.isAbstract(mod)) {
+                return null;
+            }
+            return res;
         } catch (NoSuchMethodException e) {
             // Method not found in clazz, let's search in superclasses
-            Class superclass = clazz.getSuperclass();
+            Class<?> superclass = clazz.getSuperclass();
+            // TODO: When base is an interface, it's _possible_ that the class may implement a subinterface of base
+            //       which provides a default implementation. Such a case is not currently detected as an override.
             if (superclass != null) {
-                res = getMethod(superclass, methodName, types);
+                // if the superclass doesn't derive from base anymore, stop looking
+                if (base != null && !base.isAssignableFrom(superclass)) {
+                    return null;
+                }
+                return getMethod(superclass, base, methodName, types);
             }
+            return null;
         } catch (SecurityException e) {
             throw new AssertionError(e);
         }
-        if (res == null) {
-            throw new IllegalArgumentException(
-                    String.format("Method %s not found in %s (or it is private, final or static)", methodName, clazz.getName()));
-        }
-        return res;
     }
 
     /**

core/src/test/java/hudson/util/IsOverriddenTest.java

@@ -30,7 +30,7 @@ import static org.junit.Assert.assertTrue;
 import hudson.Util;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
-import org.junit.Ignore;
+
 import org.junit.Rule;
 import org.junit.rules.ErrorCollector;
 import org.jvnet.hudson.test.Issue;
@@ -57,13 +57,19 @@ public class IsOverriddenTest {
 
     /**
      * Negative test.
-     * Trying to check for a method which does not exist in the hierarchy,
+     * Trying to check for a method which does not exist in the hierarchy.
      */
     @Test(expected = IllegalArgumentException.class)
     public void isOverriddenNegativeTest() {
         Util.isOverridden(Base.class, Derived.class, "method2");
     }
 
+    /** Specifying a base class that is not a base class should result in an error. */
+    @Test(expected = IllegalArgumentException.class)
+    public void badHierarchyIsReported() {
+        Util.isOverridden(Derived.class, Base.class, "method");
+    }
+
     /**
      * Do not inspect private methods.
      */
@@ -86,7 +92,6 @@ public class IsOverriddenTest {
     }
     public class Derived extends Intermediate {}
 
-    @Ignore("TODO fails as predicted")
     @Issue("JENKINS-62723")
     @Test
     public void finalOverrides() {
@@ -120,5 +125,44 @@ public class IsOverriddenTest {
         public final void m2() {}
     }
 
+    @Issue("JENKINS-62723")
+    @Test
+    public void baseInterface() {
+        // Normal case: classes implementing interface methods
+        errors.checkSucceeds(() -> {assertThat("I1 does not override I1.foo", Util.isOverridden(I1.class, I1.class, "foo"), is(false)); return null;});
+        errors.checkSucceeds(() -> {assertThat("I2 does not override I1.foo", Util.isOverridden(I1.class, I2.class, "foo"), is(false)); return null;});
+        errors.checkSucceeds(() -> {assertThat("C1 does not override I1.foo", Util.isOverridden(I1.class, C1.class, "foo"), is(false)); return null;});
+        errors.checkSucceeds(() -> {assertThat("C2 does not override I1.foo", Util.isOverridden(I1.class, C2.class, "foo"), is(false)); return null;});
+        errors.checkSucceeds(() -> {assertThat("C3 overrides I1.foo", Util.isOverridden(I1.class, C3.class, "foo"), is(true)); return null;});
+        errors.checkSucceeds(() -> {assertThat("C4 overrides I1.foo", Util.isOverridden(I1.class, C4.class, "foo"), is(true)); return null;});
+        // Special case: interfaces providing default implementation of base interface
+        errors.checkSucceeds(() -> {assertThat("I1 does not override I1.bar", Util.isOverridden(I1.class, I1.class, "bar"), is(false)); return null;});
+        errors.checkSucceeds(() -> {assertThat("I2 overrides I1.bar", Util.isOverridden(I1.class, I2.class, "bar"), is(true)); return null;});
+        errors.checkSucceeds(() -> {assertThat("C1 does not override I1.bar", Util.isOverridden(I1.class, C1.class, "bar"), is(false)); return null;});
+        // TODO: Not currently handled by isOverridden()
+        //errors.checkSucceeds(() -> {assertThat("C2 overrides I1.bar (via I2)", Util.isOverridden(I1.class, C2.class, "bar"), is(true)); return null;});
+        //errors.checkSucceeds(() -> {assertThat("C3 overrides I1.bar (via I2)", Util.isOverridden(I1.class, C3.class, "bar"), is(true)); return null;});
+        errors.checkSucceeds(() -> {assertThat("C4 overrides I1.bar", Util.isOverridden(I1.class, C4.class, "bar"), is(true)); return null;});
+    }
+    private interface I1 {
+        String foo();
+        String bar();
+    }
+    private interface I2 extends I1 {
+        default String bar() { return "default"; }
+    }
+    private static abstract class C1 implements I1 {
+    }
+    private static abstract class C2 extends C1 implements I2 {
+        @Override public abstract String foo();
+    }
+    private static abstract class C3 extends C2 {
+        @Override public String foo() { return "foo"; }
+    }
+    private static class C4 extends C3 implements I1 {
+        @Override public String bar() { return "bar"; }
+    }
+
+
 }