diff --git a/agent/src/main/java/com/appland/appmap/config/AppMapPackage.java b/agent/src/main/java/com/appland/appmap/config/AppMapPackage.java index 1a6ff8cf..a87e4b1e 100644 --- a/agent/src/main/java/com/appland/appmap/config/AppMapPackage.java +++ b/agent/src/main/java/com/appland/appmap/config/AppMapPackage.java @@ -99,6 +99,15 @@ public String[] getLabels() { return this.labels; } + /** + * @return {@code true} if this config came from an explicit {@code methods:} entry in + * {@code appmap.yml} (i.e. the user named the method directly), rather than from a + * generic include in exclude mode. + */ + public boolean isExplicit() { + return this.name != null; + } + /** * Checks if the given fully qualified name matches this configuration. * Supports matching against both simple and fully qualified class names for diff --git a/agent/src/main/java/com/appland/appmap/output/v1/CodeObject.java b/agent/src/main/java/com/appland/appmap/output/v1/CodeObject.java index 25b2b3d0..46b9d9d2 100644 --- a/agent/src/main/java/com/appland/appmap/output/v1/CodeObject.java +++ b/agent/src/main/java/com/appland/appmap/output/v1/CodeObject.java @@ -1,7 +1,5 @@ package com.appland.appmap.output.v1; -import java.lang.reflect.InvocationTargetException; -import java.lang.reflect.Method; import java.lang.reflect.Modifier; import java.util.ArrayDeque; import java.util.ArrayList; @@ -12,9 +10,9 @@ import com.alibaba.fastjson.annotation.JSONField; import com.appland.appmap.util.GitUtil; +import com.appland.appmap.util.LabelUtil; import com.appland.appmap.util.Logger; -import javassist.CtAppMapClassType; import javassist.CtBehavior; import javassist.CtClass; @@ -187,25 +185,9 @@ public CodeObject(CtBehavior behavior, String[] labels) { final String file = CodeObject.getSourceFilePath(ctclass); final int lineno = behavior.getMethodInfo().getLineNumber(0); - try { - // Look for the Labels annotation by class name. If we introduce a - // compile-time dependency on Labels.class, it will get relocated by the - // shadowing process, and so won't match the annotation the user put on - // their method. - final String labelsClass = "com.appland.appmap.annotation.Labels"; - if (behavior.hasAnnotation(labelsClass)) { - Object annotation = CtAppMapClassType.getAnnotation(behavior, labelsClass); - Method value = annotation.getClass().getMethod("value"); - labels = (String[])(value.invoke(annotation)); - } - } catch (ClassNotFoundException e) { - Logger.println(e); - } catch (IllegalAccessException e) { - Logger.println(e); - } catch (InvocationTargetException e) { - Logger.println(e); - } catch (NoSuchMethodException e) { - Logger.println(e); + String[] annotationLabels = LabelUtil.readAnnotationLabels(behavior); + if (annotationLabels != null) { + labels = annotationLabels; } this.setType("function") diff --git a/agent/src/main/java/com/appland/appmap/process/conditions/ConfigCondition.java b/agent/src/main/java/com/appland/appmap/process/conditions/ConfigCondition.java index 5bfef89d..64a6d20c 100644 --- a/agent/src/main/java/com/appland/appmap/process/conditions/ConfigCondition.java +++ b/agent/src/main/java/com/appland/appmap/process/conditions/ConfigCondition.java @@ -11,6 +11,7 @@ import com.appland.appmap.transform.annotations.AppMapAppMethod; import com.appland.appmap.util.AppMapBehavior; import com.appland.appmap.util.FullyQualifiedName; +import com.appland.appmap.util.LabelUtil; import com.appland.appmap.util.Logger; import javassist.CtBehavior; @@ -57,7 +58,7 @@ private boolean doMatch(CtBehavior behavior, Map matchResult) { } } - if (!AppMapBehavior.isRecordable(behavior) || ignoreMethod(behavior)) { + if (!AppMapBehavior.isRecordable(behavior)) { return false; } @@ -67,12 +68,31 @@ private boolean doMatch(CtBehavior behavior, Map matchResult) { } final AppMapPackage.LabelConfig ls = AppMapConfig.get().includes(new FullyQualifiedName(behavior)); - if (ls != null) { - matchResult.put("labels", ls.getLabels()); - return true; + if (ls == null) { + return false; } - return false; + // Explicit opt-ins override the trivial-method filter: + // - @Labels annotation on the method + // - method named directly under "methods:" in appmap.yml + // - labels attached to the method via appmap.yml + if (!isExplicitlyLabeled(behavior, ls) && ignoreMethod(behavior)) { + return false; + } + + matchResult.put("labels", ls.getLabels()); + return true; + } + + private static boolean isExplicitlyLabeled(CtBehavior behavior, AppMapPackage.LabelConfig ls) { + if (LabelUtil.hasLabelAnnotation(behavior)) { + return true; + } + if (ls.isExplicit()) { + return true; + } + String[] configLabels = ls.getLabels(); + return configLabels != null && configLabels.length > 0; } private static final Pattern SETTER_PATTERN = Pattern.compile("^set[A-Z].*"); diff --git a/agent/src/main/java/com/appland/appmap/util/LabelUtil.java b/agent/src/main/java/com/appland/appmap/util/LabelUtil.java new file mode 100644 index 00000000..bdc56213 --- /dev/null +++ b/agent/src/main/java/com/appland/appmap/util/LabelUtil.java @@ -0,0 +1,45 @@ +package com.appland.appmap.util; + +import java.lang.reflect.Method; + +import javassist.CtAppMapClassType; +import javassist.CtBehavior; + +/** + * Reads the {@code @Labels} annotation from a {@link CtBehavior} by class name, avoiding a + * compile-time dependency on {@code com.appland.appmap.annotation.Labels}. The annotation class + * gets relocated by the agent's shadowing process, so a direct reference would not match the + * annotation the user actually placed on their method. + */ +public final class LabelUtil { + public static final String LABELS_CLASS = "com.appland.appmap.annotation.Labels"; + + private LabelUtil() {} + + public static boolean hasLabelAnnotation(CtBehavior behavior) { + try { + return behavior.hasAnnotation(LABELS_CLASS); + } catch (Exception e) { + Logger.println(e); + return false; + } + } + + /** + * @return the {@code value()} of the {@code @Labels} annotation on the given behavior, or + * {@code null} if the annotation is not present or cannot be read. + */ + public static String[] readAnnotationLabels(CtBehavior behavior) { + try { + if (!behavior.hasAnnotation(LABELS_CLASS)) { + return null; + } + Object annotation = CtAppMapClassType.getAnnotation(behavior, LABELS_CLASS); + Method value = annotation.getClass().getMethod("value"); + return (String[])(value.invoke(annotation)); + } catch (Exception e) { + Logger.println(e); + return null; + } + } +} diff --git a/agent/src/test/java/com/appland/appmap/process/conditions/ConfigConditionTest.java b/agent/src/test/java/com/appland/appmap/process/conditions/ConfigConditionTest.java index 4f492846..22e7a55f 100644 --- a/agent/src/test/java/com/appland/appmap/process/conditions/ConfigConditionTest.java +++ b/agent/src/test/java/com/appland/appmap/process/conditions/ConfigConditionTest.java @@ -5,11 +5,16 @@ import static org.junit.jupiter.api.DynamicContainer.dynamicContainer; import static org.junit.jupiter.api.DynamicTest.dynamicTest; +import java.util.HashMap; +import java.util.Map; import java.util.stream.Stream; import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DynamicNode; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestFactory; import org.junit.jupiter.api.extension.ExtensionContext; @@ -21,9 +26,12 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import com.appland.appmap.config.AppMapConfig; +import com.appland.appmap.config.AppMapPackage; import com.appland.appmap.test.util.ClassBuilder; import com.appland.appmap.test.util.MethodBuilder; import com.appland.appmap.util.AppMapClassPool; +import com.appland.appmap.util.LabelUtil; import javassist.CtClass; @@ -142,6 +150,101 @@ static Stream notSetters() { .setReturnType("java.lang.Integer").endMethod())); } + @Nested + class TrivialFilterBypass { + private static final String PKG = "com.appland.testfixture"; + + private final ConfigCondition condition = new ConfigCondition(); + private AppMapPackage[] originalPackages; + private int classCounter; + + @BeforeEach + public void saveConfig() { + originalPackages = AppMapConfig.get().packages; + } + + @AfterEach + public void restoreConfig() { + AppMapConfig.get().packages = originalPackages; + } + + private MethodBuilder freshGetter(String methodName) { + ClassBuilder cb = new ClassBuilder(PKG + ".Class" + (classCounter++)); + MethodBuilder mb = cb.beginMethod(); + mb.setName(methodName) + .setBody("return \"x\";"); + try { + mb.setReturnType("java.lang.String"); + } catch (Exception e) { + throw new RuntimeException(e); + } + return mb; + } + + private boolean matches(MethodBuilder mb) { + Map result = new HashMap<>(); + return condition.match(mb.getBehavior(), result); + } + + @Test + public void plainGetterIsSkippedWithoutLabel() throws Exception { + AppMapConfig.get().packages = new AppMapPackage[] { + new AppMapPackage(PKG, null, false, null) + }; + MethodBuilder mb = freshGetter("getValue"); + mb.endMethod(); + assertFalse(matches(mb)); + } + + @Test + public void getterWithLabelsAnnotationIsRecorded() throws Exception { + AppMapConfig.get().packages = new AppMapPackage[] { + new AppMapPackage(PKG, null, false, null) + }; + MethodBuilder mb = freshGetter("getSecret"); + mb.addAnnotation(LabelUtil.LABELS_CLASS).endMethod(); + assertTrue(matches(mb)); + } + + @Test + public void setterWithLabelsAnnotationIsRecorded() throws Exception { + AppMapConfig.get().packages = new AppMapPackage[] { + new AppMapPackage(PKG, null, false, null) + }; + ClassBuilder cb = new ClassBuilder(PKG + ".Class" + (classCounter++)); + MethodBuilder mb = cb.beginMethod(); + mb.setName("setSecret") + .addParameter("java.lang.String", "value") + .addAnnotation(LabelUtil.LABELS_CLASS) + .endMethod(); + assertTrue(matches(mb)); + } + + @Test + public void getterExplicitlyNamedInConfigIsRecorded() throws Exception { + AppMapPackage.LabelConfig methodConfig = new AppMapPackage.LabelConfig( + "Class.*", "getValue", new String[] {}); + AppMapConfig.get().packages = new AppMapPackage[] { + new AppMapPackage(PKG, null, false, new AppMapPackage.LabelConfig[] { methodConfig }) + }; + MethodBuilder mb = freshGetter("getValue"); + mb.endMethod(); + assertTrue(matches(mb)); + } + + @Test + public void getterWithLabelsAttachedInConfigIsRecorded() throws Exception { + AppMapPackage.LabelConfig methodConfig = new AppMapPackage.LabelConfig( + "Class.*", "getValue", new String[] { "secret" }); + AppMapConfig.get().packages = new AppMapPackage[] { + new AppMapPackage(PKG, null, false, new AppMapPackage.LabelConfig[] { methodConfig }) + }; + MethodBuilder mb = freshGetter("getValue"); + mb.endMethod(); + assertTrue(matches(mb)); + } + } + static class ClassBuilderResolver implements ParameterResolver { @Override public boolean supportsParameter(ParameterContext parameterContext, diff --git a/agent/src/test/java/com/appland/appmap/test/util/MethodBuilder.java b/agent/src/test/java/com/appland/appmap/test/util/MethodBuilder.java index fa2559ac..31e839f5 100644 --- a/agent/src/test/java/com/appland/appmap/test/util/MethodBuilder.java +++ b/agent/src/test/java/com/appland/appmap/test/util/MethodBuilder.java @@ -15,6 +15,7 @@ import javassist.bytecode.CodeAttribute; import javassist.bytecode.ConstPool; import javassist.bytecode.Descriptor; +import javassist.bytecode.LineNumberAttributeTestHelper; import javassist.bytecode.LocalVariableAttribute; import javassist.bytecode.annotation.Annotation; @@ -30,6 +31,7 @@ public class MethodBuilder { private Integer modifiers = Modifier.PUBLIC; private List parameters = new ArrayList(); private List annotations = new ArrayList(); + private boolean withLineNumber = true; private CtMethod behavior; public CtMethod getBehavior() { @@ -249,6 +251,9 @@ private CtMethod build() throws CannotCompileException { codeAttribute.getAttributes().add(locals); + if (withLineNumber) { + codeAttribute.getAttributes().add(LineNumberAttributeTestHelper.singleEntry(constPool, 1)); + } final AnnotationsAttribute annotationAttribute = new AnnotationsAttribute(constPool, AnnotationsAttribute.visibleTag); diff --git a/agent/src/test/java/com/appland/appmap/util/LabelUtilTest.java b/agent/src/test/java/com/appland/appmap/util/LabelUtilTest.java new file mode 100644 index 00000000..0a37142b --- /dev/null +++ b/agent/src/test/java/com/appland/appmap/util/LabelUtilTest.java @@ -0,0 +1,44 @@ +package com.appland.appmap.util; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import com.appland.appmap.test.util.ClassBuilder; +import com.appland.appmap.test.util.MethodBuilder; + +public class LabelUtilTest { + @BeforeAll + public static void beforeAll() { + AppMapClassPool.acquire(Thread.currentThread().getContextClassLoader()); + } + + @AfterAll + public static void afterAll() throws Exception { + AppMapClassPool.release(); + } + + @Test + public void detectsLabelsAnnotationByName() throws Exception { + MethodBuilder mb = new ClassBuilder("LabelUtilTest$Labeled").beginMethod(); + mb.setName("getSecret") + .setBody("return \"x\";") + .setReturnType("java.lang.String") + .addAnnotation(LabelUtil.LABELS_CLASS) + .endMethod(); + assertTrue(LabelUtil.hasLabelAnnotation(mb.getBehavior())); + } + + @Test + public void unlabeledMethodReturnsFalse() throws Exception { + MethodBuilder mb = new ClassBuilder("LabelUtilTest$Unlabeled").beginMethod(); + mb.setName("getSecret") + .setBody("return \"x\";") + .setReturnType("java.lang.String") + .endMethod(); + assertFalse(LabelUtil.hasLabelAnnotation(mb.getBehavior())); + } +} diff --git a/agent/src/test/java/javassist/bytecode/LineNumberAttributeTestHelper.java b/agent/src/test/java/javassist/bytecode/LineNumberAttributeTestHelper.java new file mode 100644 index 00000000..840f7a2a --- /dev/null +++ b/agent/src/test/java/javassist/bytecode/LineNumberAttributeTestHelper.java @@ -0,0 +1,35 @@ +package javassist.bytecode; + +/** + * Test helper that creates a {@link LineNumberAttribute} by invoking its package-private + * constructor. Used by tests that synthesize methods via Javassist and need a non-negative + * line number on the resulting bytecode (e.g. so the agent does not treat the method as + * runtime-generated). + */ +public final class LineNumberAttributeTestHelper { + private LineNumberAttributeTestHelper() {} + + /** + * Build a {@code LineNumberTable} with a single entry mapping pc=0 to the given line. + */ + public static LineNumberAttribute singleEntry(ConstPool cp, int line) { + byte[] info = new byte[6]; + // table_length (u2) + info[0] = 0; + info[1] = 1; + // start_pc (u2) = 0 + info[2] = 0; + info[3] = 0; + // line_number (u2) + info[4] = (byte)((line >> 8) & 0xff); + info[5] = (byte)(line & 0xff); + try { + java.lang.reflect.Constructor ctor = + LineNumberAttribute.class.getDeclaredConstructor(ConstPool.class, byte[].class); + ctor.setAccessible(true); + return ctor.newInstance(cp, info); + } catch (ReflectiveOperationException e) { + throw new IllegalStateException(e); + } + } +}