Improve muzzle check for constructors (#4591)

open-telemetry · Nov 5, 2021 · 59c1ce1 · 59c1ce1
1 parent 123de52
commit 59c1ce1
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 0 deletions.
diff --git a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceMatcher.java b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceMatcher.java
@@ -278,13 +278,21 @@ private static FieldDescription.InDefinedShape findField(
 
   private static MethodDescription.InDefinedShape findMethod(
       MethodRef methodRef, TypeDescription typeOnClasspath) {
+
     for (MethodDescription.InDefinedShape methodDescription :
         typeOnClasspath.getDeclaredMethods()) {
       if (methodDescription.getInternalName().equals(methodRef.getName())
           && methodDescription.getDescriptor().equals(methodRef.getDescriptor())) {
         return methodDescription;
       }
     }
+
+    // if the method we're looking for is a constructor, we're only checking the direct super type;
+    // and skipping all the interfaces and indirect superclasses
+    if (methodRef.isConstructor()) {
+      return null;
+    }
+
     if (typeOnClasspath.getSuperClass() != null) {
       MethodDescription.InDefinedShape methodOnSupertype =
           findMethod(methodRef, typeOnClasspath.getSuperClass().asErasure());

diff --git a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/references/MethodRef.java b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/references/MethodRef.java
@@ -54,6 +54,10 @@ public String getDescriptor() {
     return descriptor;
   }
 
+  public boolean isConstructor() {
+    return "<init>".equals(name);
+  }
+
   MethodRef merge(MethodRef anotherMethod) {
     if (!equals(anotherMethod)) {
       throw new IllegalStateException("illegal merge " + this + " != " + anotherMethod);

diff --git a/muzzle/src/test/groovy/io/opentelemetry/javaagent/tooling/muzzle/ReferenceMatcherTest.groovy b/muzzle/src/test/groovy/io/opentelemetry/javaagent/tooling/muzzle/ReferenceMatcherTest.groovy
@@ -11,6 +11,7 @@ import io.opentelemetry.instrumentation.test.utils.ClasspathUtils
 import io.opentelemetry.javaagent.tooling.muzzle.references.ClassRef
 import io.opentelemetry.javaagent.tooling.muzzle.references.Flag
 import io.opentelemetry.javaagent.tooling.muzzle.references.Source
+import muzzle.TestClasses
 import muzzle.TestClasses.MethodBodyAdvice
 import org.objectweb.asm.Type
 import spock.lang.Shared
@@ -320,6 +321,30 @@ class ReferenceMatcherTest extends Specification {
     "external helper, different field type" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper" | "field"          | "Lcom/external/DifferentType;"
   }
 
+  def "should fail #desc helper class when the library parent class has different constructor"() {
+    given:
+    def helper = ClassRef.builder(className)
+      .setSuperClassName(TestClasses.BaseClassWithConstructor.name)
+      .build()
+    // muzzle codegen plugin has captured a no-arg constructor reference;
+    // the actual constructor of the base class on the classpath requires a long
+    def baseClassRef = ClassRef.builder(TestClasses.BaseClassWithConstructor.name)
+      .addMethod(new Source[0], new Flag[0], "<init>", Type.VOID_TYPE)
+      .build()
+
+    when:
+    def mismatches = createMatcher([(helper.className): helper, (baseClassRef.className): baseClassRef], [helper.className])
+      .getMismatchedReferenceSources(this.class.classLoader)
+
+    then:
+    getMismatchClassSet(mismatches) == [Mismatch.MissingMethod] as Set
+
+    where:
+    desc       | className
+    "internal" | "io.opentelemetry.instrumentation.Helper"
+    "external" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper"
+  }
+
   private static ReferenceMatcher createMatcher(Map<String, ClassRef> references = [:],
                                                 List<String> helperClasses = []) {
     new ReferenceMatcher(helperClasses, references, { it.startsWith(TEST_EXTERNAL_INSTRUMENTATION_PACKAGE) })

diff --git a/muzzle/src/test/java/muzzle/TestClasses.java b/muzzle/src/test/java/muzzle/TestClasses.java
@@ -84,6 +84,10 @@ public static class SomeClassWithFields {
     public interface AnotherInterface extends SomeInterface {}
   }
 
+  public abstract static class BaseClassWithConstructor {
+    protected BaseClassWithConstructor(long l) {}
+  }
+
   public static class LdcAdvice {
     public static void ldcMethod() {
       MethodBodyAdvice.A.class.getName();