diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index a63d81080c..96e2ec1b21 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -31,6 +31,10 @@ Use subheadings with the "=====" level for adding notes for unreleased changes: === Unreleased +[float] +===== Bug fixes +* Fixed problems with public API annotation inheritance - {pull}3551[#3551] + [[release-notes-1.x]] === Java Agent version 1.x diff --git a/apm-agent-plugin-sdk/src/main/java/co/elastic/apm/agent/sdk/bytebuddy/AnnotationValueOffsetMappingFactory.java b/apm-agent-plugin-sdk/src/main/java/co/elastic/apm/agent/sdk/bytebuddy/AnnotationValueOffsetMappingFactory.java index 98e1d433c8..de58b9602b 100644 --- a/apm-agent-plugin-sdk/src/main/java/co/elastic/apm/agent/sdk/bytebuddy/AnnotationValueOffsetMappingFactory.java +++ b/apm-agent-plugin-sdk/src/main/java/co/elastic/apm/agent/sdk/bytebuddy/AnnotationValueOffsetMappingFactory.java @@ -22,6 +22,7 @@ import co.elastic.apm.agent.sdk.logging.LoggerFactory; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.annotation.AnnotationDescription; +import net.bytebuddy.description.annotation.AnnotationValue; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.method.ParameterDescription; import net.bytebuddy.description.type.TypeDescription; @@ -32,6 +33,11 @@ import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; +import java.util.ArrayDeque; +import java.util.Collections; +import java.util.IdentityHashMap; +import java.util.Queue; +import java.util.Set; import static net.bytebuddy.matcher.ElementMatchers.named; @@ -58,20 +64,31 @@ public Target resolve(TypeDescription instrumentedType, MethodDescription instru @Nullable private Object getAnnotationValue(MethodDescription instrumentedMethod, AnnotationValueExtractor annotationValueExtractor) { - MethodDescription methodDescription = instrumentedMethod; - do { - for (TypeDescription typeDescription : methodDescription.getDeclaredAnnotations().asTypeList()) { - if (named(annotationValueExtractor.annotationClassName()).matches(typeDescription)) { - for (MethodDescription.InDefinedShape annotationMethod : typeDescription.getDeclaredMethods()) { - if (annotationMethod.getName().equals(annotationValueExtractor.method())) { - return methodDescription.getDeclaredAnnotations().ofType(typeDescription).getValue(annotationMethod).resolve(); - } + Queue typesToCheck = new ArrayDeque<>(); + typesToCheck.add(instrumentedMethod.getDeclaringType().asErasure()); + Set alreadyCheckedTypes = Collections.newSetFromMap(new IdentityHashMap()); + + while (!typesToCheck.isEmpty()) { + TypeDescription type = typesToCheck.poll(); + if (alreadyCheckedTypes.add(type)) { + MethodDescription method = findMethodWithSameSignature(type, instrumentedMethod); + if (method != null) { + AnnotationValue value = findValueOnMethod(method, annotationValueExtractor); + if (value != null) { + return value.resolve(); } } + + TypeDescription.Generic superClass = type.getSuperClass(); + if (superClass != null) { + typesToCheck.add(superClass.asErasure()); + } + for (TypeDescription.Generic interfaceType : type.getInterfaces()) { + typesToCheck.add(interfaceType.asErasure()); + } } + } - methodDescription = findInstrumentedMethodInSuperClass(methodDescription.getDeclaringType().getSuperClass(), instrumentedMethod); - } while (methodDescription != null); Class defaultValueProvider = annotationValueExtractor.defaultValueProvider(); try { return defaultValueProvider.getDeclaredConstructor().newInstance().getDefaultValue(); @@ -89,12 +106,8 @@ private Object getAnnotationValue(MethodDescription instrumentedMethod, Annotati } @Nullable - private MethodDescription findInstrumentedMethodInSuperClass(@Nullable TypeDescription.Generic superClass, MethodDescription instrumentedMethod) { - if (superClass == null) { - return null; - - } - for (MethodDescription declaredMethod : superClass.getDeclaredMethods()) { + private MethodDescription findMethodWithSameSignature(TypeDescription declaringType, MethodDescription instrumentedMethod) { + for (MethodDescription declaredMethod : declaringType.getDeclaredMethods()) { if (instrumentedMethod.getInternalName().equals(declaredMethod.getInternalName()) && instrumentedMethod.getParameters().asTypeList().asErasures().equals(declaredMethod.getParameters().asTypeList().asErasures())) { return declaredMethod; @@ -103,6 +116,21 @@ private MethodDescription findInstrumentedMethodInSuperClass(@Nullable TypeDescr return null; } + @Nullable + private static AnnotationValue findValueOnMethod(MethodDescription method, AnnotationValueExtractor valueExtractor) { + for (TypeDescription typeDescription : method.getDeclaredAnnotations().asTypeList()) { + if (named(valueExtractor.annotationClassName()).matches(typeDescription)) { + for (MethodDescription.InDefinedShape annotationMethod : typeDescription.getDeclaredMethods()) { + if (annotationMethod.getName().equals(valueExtractor.method())) { + return method.getDeclaredAnnotations().ofType(typeDescription).getValue(annotationMethod); + } + } + } + } + return null; + } + + @Retention(RetentionPolicy.RUNTIME) @Target(ElementType.PARAMETER) public @interface AnnotationValueExtractor { @@ -114,7 +142,8 @@ private MethodDescription findInstrumentedMethodInSuperClass(@Nullable TypeDescr } public interface DefaultValueProvider { - @Nullable Object getDefaultValue(); + @Nullable + Object getDefaultValue(); } public static class NullDefaultValueProvider implements DefaultValueProvider { diff --git a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationInheritanceTest.java b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationInheritanceTest.java index 902fea1c74..3ad2468ab7 100644 --- a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationInheritanceTest.java +++ b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationInheritanceTest.java @@ -31,6 +31,11 @@ import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInstance; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationTargetException; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.doReturn; @@ -94,6 +99,7 @@ void testClassWithoutAnnotations() { @TestInstance(TestInstance.Lifecycle.PER_CLASS) class EnabledPublicApiAnnotationInheritance { + @BeforeAll void beforeAll() { init(true); @@ -104,6 +110,13 @@ void afterAll() { reset(); } + private TestClassBase createTestClassInstance(Class testClass) + throws InstantiationException, IllegalAccessException, InvocationTargetException, NoSuchMethodException { + Constructor declaredConstructor = testClass.getDeclaredConstructor(); + declaredConstructor.setAccessible(true); + return declaredConstructor.newInstance(); + } + @Test void testClassWithAnnotations() { invokeApiMethods(new ClassWithAnnotations()); @@ -111,36 +124,43 @@ void testClassWithAnnotations() { assertThat(reporter.getSpans()).hasSize(1); } - @Test - void testInheritedCaptureTransaction() { - new ClassWithoutAnnotations().captureTransaction(); - checkTransaction("ClassWithoutAnnotations#captureTransaction"); + @ParameterizedTest + @ValueSource(classes = {ClassWithoutAnnotations.class, TransitiveClassWithoutAnnotations.class, InterfaceImplementor.class}) + void testInheritedCaptureTransaction(Class testClass) throws Exception { + TestClassBase instance = createTestClassInstance(testClass); + instance.captureTransaction(); + checkTransaction(testClass.getSimpleName() + "#captureTransaction"); } - @Test - void testInheritedCaptureSpan() { + + @ParameterizedTest + @ValueSource(classes = {ClassWithoutAnnotations.class, TransitiveClassWithoutAnnotations.class, InterfaceImplementor.class}) + void testInheritedCaptureSpan(Class testClass) throws Exception { + TestClassBase instance = createTestClassInstance(testClass); Transaction transaction = ElasticApm.startTransaction(); try (Scope scope = transaction.activate()) { - new ClassWithoutAnnotations().captureSpan(); + instance.captureSpan(); } transaction.end(); - checkSpan("ClassWithoutAnnotations#captureSpan"); + checkSpan(testClass.getSimpleName() + "#captureSpan"); } - @Test - void testInheritedTracedWithoutActiveTransaction() { - new ClassWithoutAnnotations().traced(); - checkTransaction("ClassWithoutAnnotations#traced"); + @ParameterizedTest + @ValueSource(classes = {ClassWithoutAnnotations.class, TransitiveClassWithoutAnnotations.class, InterfaceImplementor.class}) + void testInheritedTracedWithoutActiveTransaction(Class testClass) throws Exception { + createTestClassInstance(testClass).traced(); + checkTransaction(testClass.getSimpleName() + "#traced"); } - @Test - void testInheritedTracedWithActiveTransaction() { + @ParameterizedTest + @ValueSource(classes = {ClassWithoutAnnotations.class, TransitiveClassWithoutAnnotations.class, InterfaceImplementor.class}) + void testInheritedTracedWithActiveTransaction(Class testClass) throws Exception { Transaction transaction = ElasticApm.startTransaction(); try (Scope scope = transaction.activate()) { - new ClassWithoutAnnotations().traced(); + createTestClassInstance(testClass).traced(); } transaction.end(); - checkSpan("ClassWithoutAnnotations#traced"); + checkSpan(testClass.getSimpleName() + "#traced"); } private void checkTransaction(String name) { @@ -166,7 +186,16 @@ private void invokeApiMethods(ClassWithAnnotations classWithAnnotations) { classWithAnnotations.traced(); } - static class ClassWithAnnotations { + + abstract static class TestClassBase { + abstract void captureTransaction(); + + abstract void captureSpan(); + + abstract void traced(); + } + + static class ClassWithAnnotations extends TestClassBase { @CaptureTransaction void captureTransaction() { } @@ -193,4 +222,46 @@ void captureSpan() { void traced() { } } + + static class EmptyClass extends ClassWithAnnotations { + } + + static class TransitiveClassWithoutAnnotations extends EmptyClass { + @Override + void captureTransaction() { + } + + @Override + void captureSpan() { + } + + @Override + void traced() { + } + } + + interface InterfaceWithAnnotations { + @CaptureTransaction + void captureTransaction(); + + @CaptureSpan + void captureSpan(); + + @Traced + void traced(); + } + + static class InterfaceImplementor extends TestClassBase implements InterfaceWithAnnotations { + + public void captureTransaction() { + } + + public void captureSpan() { + } + + @Override + public void traced() { + } + } + }