diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index d6053dc06fd2f6444db12980238cd1ff20759784..8fa52292250800f72548b4208a0e78b79def7e6d 100644 --- a/core/src/main/java/hudson/Util.java +++ b/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; } /** diff --git a/core/src/test/java/hudson/util/IsOverriddenTest.java b/core/src/test/java/hudson/util/IsOverriddenTest.java index 3c24d945dd8995ecd5b93ea45b2abc74aa011ce7..6e374b6094216b82400ac7fcc17bbe553378b844 100644 --- a/core/src/test/java/hudson/util/IsOverriddenTest.java +++ b/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"; } + } + + }