Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix inheritance for API annotations #3551

Merged
merged 7 commits into from
Mar 11, 2024
Merged

Fix inheritance for API annotations #3551

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d4aec6d
Added test-cases
JonasKunz Mar 5, 2024
b628e6d
Fix annotation value discovery algorithm
JonasKunz Mar 5, 2024
56d9cef
Added changelog
JonasKunz Mar 5, 2024
66421d1
Fix compilation for java 7
JonasKunz Mar 5, 2024
25f5855
Fix test formatting
JonasKunz Mar 11, 2024
07c77fe
Merge remote-tracking branch 'elastic/main' into capture-span-fix
JonasKunz Mar 11, 2024
662483f
Fix changelog
JonasKunz Mar 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
63 changes: 46 additions & 17 deletions ...src/main/java/co/elastic/apm/agent/sdk/bytebuddy/AnnotationValueOffsetMappingFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -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<TypeDescription> typesToCheck = new ArrayDeque<>();
typesToCheck.add(instrumentedMethod.getDeclaringType().asErasure());
Set<TypeDescription> alreadyCheckedTypes = Collections.newSetFromMap(new IdentityHashMap<TypeDescription, Boolean>());

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<? extends DefaultValueProvider> defaultValueProvider = annotationValueExtractor.defaultValueProvider();
try {
return defaultValueProvider.getDeclaredConstructor().newInstance().getDefaultValue();
Expand All @@ -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;
Expand All @@ -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 {
Expand All @@ -114,7 +142,8 @@ private MethodDescription findInstrumentedMethodInSuperClass(@Nullable TypeDescr
}

public interface DefaultValueProvider {
@Nullable Object getDefaultValue();
@Nullable
Object getDefaultValue();
}

public static class NullDefaultValueProvider implements DefaultValueProvider {
Expand Down
105 changes: 88 additions & 17 deletions ...nt-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationInheritanceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -94,6 +99,7 @@ void testClassWithoutAnnotations() {
@TestInstance(TestInstance.Lifecycle.PER_CLASS)
class EnabledPublicApiAnnotationInheritance {


@BeforeAll
void beforeAll() {
init(true);
Expand All @@ -104,43 +110,57 @@ void afterAll() {
reset();
}

private TestClassBase createTestClassInstance(Class<? extends TestClassBase> testClass)
throws InstantiationException, IllegalAccessException, InvocationTargetException, NoSuchMethodException {
Constructor<? extends TestClassBase> declaredConstructor = testClass.getDeclaredConstructor();
declaredConstructor.setAccessible(true);
return declaredConstructor.newInstance();
}

@Test
void testClassWithAnnotations() {
invokeApiMethods(new ClassWithAnnotations());
assertThat(reporter.getTransactions()).hasSize(3);
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<? extends TestClassBase> 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<? extends TestClassBase> 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<? extends TestClassBase> 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<? extends TestClassBase> 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) {
Expand All @@ -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() {
}
Expand All @@ -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() {
}
}

}
Loading