diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java index 4be2cce8cc20..f953fdf125a2 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java @@ -436,12 +436,14 @@ public static class Field { private final Set<Flag> flags; private final String name; private final Type type; + private final boolean declared; - public Field(Source[] sources, Flag[] flags, String name, Type fieldType) { + public Field(Source[] sources, Flag[] flags, String name, Type fieldType, boolean declared) { this.sources = new LinkedHashSet<>(Arrays.asList(sources)); this.flags = new LinkedHashSet<>(Arrays.asList(flags)); this.name = name; - type = fieldType; + this.type = fieldType; + this.declared = declared; } public String getName() { @@ -460,6 +462,14 @@ public Type getType() { return type; } + /** + * Denotes whether this field is declared in the class {@link Reference} it is a part of. If + * {@code false} then this field is just used and most likely is declared in the super class. + */ + public boolean isDeclared() { + return declared; + } + public Field merge(Field anotherField) { if (!equals(anotherField) || !type.equals(anotherField.type)) { throw new IllegalStateException("illegal merge " + this + " != " + anotherField); @@ -468,7 +478,8 @@ public Field merge(Field anotherField) { Reference.merge(sources, anotherField.sources).toArray(new Source[0]), mergeFlags(flags, anotherField.flags).toArray(new Flag[0]), name, - type); + type, + declared || anotherField.declared); } @Override @@ -537,8 +548,8 @@ public Builder withFlag(Flag flag) { } public Builder withField( - Source[] sources, Flag[] fieldFlags, String fieldName, Type fieldType) { - Field field = new Field(sources, fieldFlags, fieldName, fieldType); + Source[] sources, Flag[] fieldFlags, String fieldName, Type fieldType, boolean declared) { + Field field = new Field(sources, fieldFlags, fieldName, fieldType, declared); int existingIndex = fields.indexOf(field); if (existingIndex == -1) { fields.add(field); diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/MuzzleCodeGenerator.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/MuzzleCodeGenerator.java index f42925d90375..acbd21e95be2 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/MuzzleCodeGenerator.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/MuzzleCodeGenerator.java @@ -399,6 +399,9 @@ private void generateMuzzleReferenceMatcherMethod(ReferenceCollector collector) false); } + // declared flag + mv.visitLdcInsn(field.isDeclared()); + mv.visitMethodInsn( Opcodes.INVOKEVIRTUAL, "io/opentelemetry/javaagent/tooling/muzzle/Reference$Builder", @@ -409,7 +412,8 @@ private void generateMuzzleReferenceMatcherMethod(ReferenceCollector collector) Reference.Source[].class, Reference.Flag[].class, String.class, - Type.class)), + Type.class, + boolean.class)), false); } for (Reference.Method method : references[i].getMethods()) { diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollectingClassVisitor.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollectingClassVisitor.java index d52e86e75ff7..1e98184a8331 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollectingClassVisitor.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollectingClassVisitor.java @@ -209,8 +209,15 @@ public FieldVisitor visitField( // Additional references we could check // - annotations on field - // intentionally not creating refs to fields here. - // Will create refs in method instructions to include line numbers. + Type fieldType = Type.getType(descriptor); + + // remember that this field was declared in the currently visited helper class + addReference( + new Reference.Builder(refSourceClassName) + .withSource(refSourceClassName) + .withField(new Source[0], new Flag[0], name, fieldType, true) + .build()); + return super.visitField(access, name, descriptor, signature, value); } @@ -328,7 +335,8 @@ public void visitFieldInsn(int opcode, String owner, String name, String descrip }, fieldFlags.toArray(new Reference.Flag[0]), name, - fieldType) + fieldType, + false) .build()); Type underlyingFieldType = underlyingType(fieldType); diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/HelperReferenceWrapper.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/HelperReferenceWrapper.java index 7967381d9aa0..dca66a6adf1e 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/HelperReferenceWrapper.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/HelperReferenceWrapper.java @@ -14,6 +14,7 @@ import java.util.Map; import java.util.Objects; import java.util.stream.Stream; +import net.bytebuddy.description.field.FieldDescription; import net.bytebuddy.description.method.MethodDescription.InDefinedShape; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.pool.TypePool; @@ -38,6 +39,9 @@ public interface HelperReferenceWrapper { /** Returns an iterable with all non-private, non-static methods declared in the wrapped type. */ Stream<Method> getMethods(); + /** Returns an iterable with all non-private fields declared in the wrapped type. */ + Stream<Field> getFields(); + final class Method { private final boolean isAbstract; private final String declaringClass; @@ -85,6 +89,41 @@ public int hashCode() { } } + final class Field { + private final String name; + private final String descriptor; + + public Field(String name, String descriptor) { + this.name = name; + this.descriptor = descriptor; + } + + public String getName() { + return name; + } + + public String getDescriptor() { + return descriptor; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + Field field = (Field) o; + return Objects.equals(name, field.name) && Objects.equals(descriptor, field.descriptor); + } + + @Override + public int hashCode() { + return Objects.hash(name, descriptor); + } + } + class Factory { private final TypePool classpathPool; private final Map<String, Reference> helperReferences; @@ -161,6 +200,21 @@ private Method toMethod(Reference.Method method) { method.getName(), method.getDescriptor()); } + + @Override + public Stream<Field> getFields() { + return reference.getFields().stream() + .filter(this::isDeclaredAndNotPrivate) + .map(this::toField); + } + + private boolean isDeclaredAndNotPrivate(Reference.Field field) { + return field.isDeclared() && !field.getFlags().contains(VisibilityFlag.PRIVATE); + } + + private Field toField(Reference.Field field) { + return new Field(field.getName(), field.getType().getDescriptor()); + } } private static final class ClasspathType implements HelperReferenceWrapper { @@ -210,6 +264,19 @@ private Method toMethod(InDefinedShape method) { return new Method( method.isAbstract(), type.getName(), method.getInternalName(), method.getDescriptor()); } + + @Override + public Stream<Field> getFields() { + return type.getDeclaredFields().stream().filter(this::isNotPrivate).map(this::toField); + } + + private boolean isNotPrivate(FieldDescription.InDefinedShape field) { + return !field.isPrivate(); + } + + private Field toField(FieldDescription.InDefinedShape field) { + return new Field(field.getName(), field.getDescriptor()); + } } } } diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/ReferenceMatcher.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/ReferenceMatcher.java index 7ca74a30445c..98a19b8c052d 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/ReferenceMatcher.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/ReferenceMatcher.java @@ -25,6 +25,7 @@ import java.util.Map; import java.util.Set; import java.util.function.Predicate; +import java.util.stream.Collectors; import net.bytebuddy.description.field.FieldDescription; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; @@ -146,12 +147,38 @@ private List<Mismatch> checkMatch(Reference reference, ClassLoader loader) { } // for helper classes we make sure that all abstract methods from super classes and interfaces are - // implemented + // implemented and that all accessed fields are defined somewhere in the type hierarchy private List<Mismatch> checkHelperClassMatch(Reference helperClass, TypePool typePool) { List<Mismatch> mismatches = emptyList(); HelperReferenceWrapper helperWrapper = new Factory(typePool, references).create(helperClass); + Set<HelperReferenceWrapper.Field> undeclaredFields = + helperClass.getFields().stream() + .filter(f -> !f.isDeclared()) + .map(f -> new HelperReferenceWrapper.Field(f.getName(), f.getType().getDescriptor())) + .collect(Collectors.toSet()); + + // if there are any fields in this helper class that's not declared here, check the type + // hierarchy + if (!undeclaredFields.isEmpty()) { + Set<HelperReferenceWrapper.Field> superClassFields = new HashSet<>(); + collectFieldsFromTypeHierarchy(helperWrapper, superClassFields); + + undeclaredFields.removeAll(superClassFields); + for (HelperReferenceWrapper.Field missingField : undeclaredFields) { + mismatches = + lazyAdd( + mismatches, + new Mismatch.MissingField( + helperClass.getSources().toArray(new Source[0]), + helperClass.getClassName(), + missingField.getName(), + missingField.getDescriptor())); + } + } + + // skip abstract method check if this type does not have super type or is abstract if (!helperWrapper.hasSuperTypes() || helperWrapper.isAbstract()) { return mismatches; } @@ -177,6 +204,13 @@ private List<Mismatch> checkHelperClassMatch(Reference helperClass, TypePool typ return mismatches; } + private static void collectFieldsFromTypeHierarchy( + HelperReferenceWrapper type, Set<HelperReferenceWrapper.Field> fields) { + + type.getFields().forEach(fields::add); + type.getSuperTypes().forEach(superType -> collectFieldsFromTypeHierarchy(superType, fields)); + } + private static void collectMethodsFromTypeHierarchy( HelperReferenceWrapper type, Set<Method> abstractMethods, Set<Method> plainMethods) { diff --git a/testing-common/src/test/groovy/muzzle/HelperReferenceWrapperTest.groovy b/testing-common/src/test/groovy/muzzle/HelperReferenceWrapperTest.groovy index 97fc116775ef..e97b2e8548dc 100644 --- a/testing-common/src/test/groovy/muzzle/HelperReferenceWrapperTest.groovy +++ b/testing-common/src/test/groovy/muzzle/HelperReferenceWrapperTest.groovy @@ -30,6 +30,9 @@ class HelperReferenceWrapperTest extends Specification { .withSuperName(baseHelperClass.className) .withInterface(HelperReferenceWrapperTestClasses.Interface2.name) .withMethod(new Reference.Source[0], new Reference.Flag[0], "bar", Type.VOID_TYPE) + .withField(new Reference.Source[0], new Reference.Flag[0], "field", Type.getType("Ljava/lang/Object;"), false) + .withField(new Reference.Source[0], new Reference.Flag[0], "declaredField", Type.getType("Ljava/lang/Object;"), true) + .withField(new Reference.Source[0], [Reference.Flag.VisibilityFlag.PRIVATE] as Reference.Flag[], "privateFieldsAreSkipped", Type.getType("Ljava/lang/Object;"), true) .build() def "should wrap helper types"() { @@ -56,6 +59,14 @@ class HelperReferenceWrapperTest extends Specification { } } + with(helper.fields.collect(toList())) { + it.size() == 1 + with(it[0]) { + it.name == "declaredField" + it.descriptor == "Ljava/lang/Object;" + } + } + helper.hasSuperTypes() with(helper.superTypes.collect(toList())) { it.size() == 2 @@ -84,6 +95,14 @@ class HelperReferenceWrapperTest extends Specification { abstractClasspathType.getMethods().collect(toList()).isEmpty() + with(abstractClasspathType.fields.collect(toList())) { + it.size() == 1 + with(it[0]) { + it.name == "field" + it.descriptor == "Ljava/lang/Object;" + } + } + abstractClasspathType.hasSuperTypes() with(abstractClasspathType.superTypes.collect(toList())) { it.size() == 2 diff --git a/testing-common/src/test/groovy/muzzle/ReferenceCollectorTest.groovy b/testing-common/src/test/groovy/muzzle/ReferenceCollectorTest.groovy index 19d9dd314ac6..0a687f905cd5 100644 --- a/testing-common/src/test/groovy/muzzle/ReferenceCollectorTest.groovy +++ b/testing-common/src/test/groovy/muzzle/ReferenceCollectorTest.groovy @@ -148,6 +148,28 @@ class ReferenceCollectorTest extends Specification { } } + def "should collect field declaration references"() { + when: + def collector = new ReferenceCollector({ it == DeclaredFieldTestClass.Helper.name }) + collector.collectReferencesFromAdvice(DeclaredFieldTestClass.Advice.name) + def references = collector.references + + then: + println references + + with(references[DeclaredFieldTestClass.Helper.name]) {helperClass -> + def superField = findField(helperClass, 'superField') + !superField.declared + + def field = findField(helperClass, 'helperField') + field.declared + } + + with(references[DeclaredFieldTestClass.LibraryBaseClass.name]) {libraryBaseClass -> + libraryBaseClass.fields.empty + } + } + def "should find all helper classes"() { when: def collector = new ReferenceCollector({ false }) diff --git a/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy b/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy index 6e5f503f1823..a23e3af8f749 100644 --- a/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy +++ b/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy @@ -18,6 +18,7 @@ import static io.opentelemetry.javaagent.tooling.muzzle.matcher.Mismatch.Missing import static io.opentelemetry.javaagent.tooling.muzzle.matcher.Mismatch.MissingMethod import static muzzle.TestClasses.MethodBodyAdvice +import external.LibraryBaseClass import io.opentelemetry.instrumentation.TestHelperClasses import io.opentelemetry.instrumentation.test.utils.ClasspathUtils import io.opentelemetry.javaagent.tooling.muzzle.Reference @@ -146,7 +147,7 @@ class ReferenceMatcherTest extends Specification { def "field match #fieldTestDesc"() { setup: def reference = new Reference.Builder(classToCheck.name) - .withField(new Source[0], fieldFlags as Reference.Flag[], fieldName, Type.getType(fieldType)) + .withField(new Source[0], fieldFlags as Reference.Flag[], fieldName, Type.getType(fieldType), false) .build() when: @@ -182,7 +183,7 @@ class ReferenceMatcherTest extends Specification { def "should not check abstract #desc helper classes"() { given: - def reference = new Reference.Builder("io.opentelemetry.instrumentation.Helper") + def reference = new Reference.Builder(className) .withSuperName(TestHelperClasses.HelperSuperClass.name) .withFlag(ABSTRACT) .withMethod(new Source[0], [ABSTRACT] as Reference.Flag[], "unimplemented", Type.VOID_TYPE) @@ -197,7 +198,7 @@ class ReferenceMatcherTest extends Specification { where: desc | className - "internal" | "com.external.otel.instrumentation.Helper" + "internal" | "io.opentelemetry.instrumentation.Helper" "external" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper" } @@ -217,7 +218,7 @@ class ReferenceMatcherTest extends Specification { where: desc | className - "internal" | "com.external.otel.instrumentation.Helper" + "internal" | "io.opentelemetry.instrumentation.Helper" "external" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper" } @@ -237,7 +238,7 @@ class ReferenceMatcherTest extends Specification { where: desc | className - "internal" | "com.external.otel.instrumentation.Helper" + "internal" | "io.opentelemetry.instrumentation.Helper" "external" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper" } @@ -259,7 +260,7 @@ class ReferenceMatcherTest extends Specification { where: desc | className - "internal" | "com.external.otel.instrumentation.Helper" + "internal" | "io.opentelemetry.instrumentation.Helper" "external" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper" } @@ -286,10 +287,52 @@ class ReferenceMatcherTest extends Specification { where: desc | className - "internal" | "com.external.otel.instrumentation.Helper" + "internal" | "io.opentelemetry.instrumentation.Helper" + "external" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper" + } + + def "should check #desc helper class whether used fields are declared in the super class"() { + given: + def helper = new Reference.Builder(className) + .withSuperName(LibraryBaseClass.name) + .withField(new Source[0], new Reference.Flag[0], "field", Type.getType("Ljava/lang/Integer;"), false) + .build() + + when: + def mismatches = createMatcher([helper], [helper.className]) + .getMismatchedReferenceSources(this.class.classLoader) + + then: + mismatches.empty + + where: + desc | className + "internal" | "io.opentelemetry.instrumentation.Helper" "external" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper" } + def "should fail helper class when it uses fields undeclared in the super class: #desc"() { + given: + def helper = new Reference.Builder(className) + .withSuperName(LibraryBaseClass.name) + .withField(new Source[0], new Reference.Flag[0], fieldName, Type.getType(fieldType), false) + .build() + + when: + def mismatches = createMatcher([helper], [helper.className]) + .getMismatchedReferenceSources(this.class.classLoader) + + then: + getMismatchClassSet(mismatches) == [MissingField] as Set + + where: + desc | className | fieldName | fieldType + "internal helper, different field name" | "io.opentelemetry.instrumentation.Helper" | "differentField" | "Ljava/lang/Integer;" + "internal helper, different field type" | "io.opentelemetry.instrumentation.Helper" | "field" | "Lcom/external/DifferentType;" + "external helper, different field name" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper" | "differentField" | "Ljava/lang/Integer;" + "external helper, different field type" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper" | "field" | "Lcom/external/DifferentType;" + } + private static ReferenceMatcher createMatcher(Collection<Reference> references = [], List<String> helperClasses = []) { new ReferenceMatcher(helperClasses, references as Reference[], { it.startsWith(TEST_EXTERNAL_INSTRUMENTATION_PACKAGE) }) diff --git a/testing-common/src/test/java/external/LibraryBaseClass.java b/testing-common/src/test/java/external/LibraryBaseClass.java new file mode 100644 index 000000000000..a3b858a84648 --- /dev/null +++ b/testing-common/src/test/java/external/LibraryBaseClass.java @@ -0,0 +1,10 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package external; + +public abstract class LibraryBaseClass { + protected Integer field; +} diff --git a/testing-common/src/test/java/muzzle/DeclaredFieldTestClass.java b/testing-common/src/test/java/muzzle/DeclaredFieldTestClass.java new file mode 100644 index 000000000000..3caa7edc3ac0 --- /dev/null +++ b/testing-common/src/test/java/muzzle/DeclaredFieldTestClass.java @@ -0,0 +1,26 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package muzzle; + +public class DeclaredFieldTestClass { + public static class Advice { + public void instrument() { + new Helper().foo(); + } + } + + public static class Helper extends LibraryBaseClass { + private String helperField; + + public void foo() { + superField.toString(); + } + } + + public static class LibraryBaseClass { + protected Object superField; + } +} diff --git a/testing-common/src/test/java/muzzle/HelperReferenceWrapperTestClasses.java b/testing-common/src/test/java/muzzle/HelperReferenceWrapperTestClasses.java index 652dc4c62e73..e9c1d14c0ab7 100644 --- a/testing-common/src/test/java/muzzle/HelperReferenceWrapperTestClasses.java +++ b/testing-common/src/test/java/muzzle/HelperReferenceWrapperTestClasses.java @@ -15,6 +15,9 @@ interface Interface2 { } abstract static class AbstractClasspathType implements Interface1 { + private Object privateFieldsAreIgnored; + protected Object field; + static void staticMethodsAreIgnored() {} private void privateMethodsToo() {}