Skip to content

Commit 7171e59

Browse files
kgilpinclaude
andcommitted
fix: record methods with @Labels even if they look trivial
Methods annotated with @Labels (or named explicitly under `methods:` in appmap.yml, or labeled there) were being dropped by the getter/setter trivial-method filter before the agent ever consulted the opt-in. They now bypass that filter so explicit user intent always wins. Extract the @Labels-by-name reading from CodeObject into a shared LabelUtil so ConfigCondition can ask the same question during match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent a2d21e0 commit 7171e59

8 files changed

Lines changed: 270 additions & 27 deletions

File tree

agent/src/main/java/com/appland/appmap/config/AppMapPackage.java

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,15 @@ public String[] getLabels() {
9999
return this.labels;
100100
}
101101

102+
/**
103+
* @return {@code true} if this config came from an explicit {@code methods:} entry in
104+
* {@code appmap.yml} (i.e. the user named the method directly), rather than from a
105+
* generic include in exclude mode.
106+
*/
107+
public boolean isExplicit() {
108+
return this.name != null;
109+
}
110+
102111
/**
103112
* Checks if the given fully qualified name matches this configuration.
104113
* Supports matching against both simple and fully qualified class names for

agent/src/main/java/com/appland/appmap/output/v1/CodeObject.java

Lines changed: 4 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
package com.appland.appmap.output.v1;
22

3-
import java.lang.reflect.InvocationTargetException;
4-
import java.lang.reflect.Method;
53
import java.lang.reflect.Modifier;
64
import java.util.ArrayDeque;
75
import java.util.ArrayList;
@@ -12,9 +10,9 @@
1210

1311
import com.alibaba.fastjson.annotation.JSONField;
1412
import com.appland.appmap.util.GitUtil;
13+
import com.appland.appmap.util.LabelUtil;
1514
import com.appland.appmap.util.Logger;
1615

17-
import javassist.CtAppMapClassType;
1816
import javassist.CtBehavior;
1917
import javassist.CtClass;
2018

@@ -187,25 +185,9 @@ public CodeObject(CtBehavior behavior, String[] labels) {
187185
final String file = CodeObject.getSourceFilePath(ctclass);
188186
final int lineno = behavior.getMethodInfo().getLineNumber(0);
189187

190-
try {
191-
// Look for the Labels annotation by class name. If we introduce a
192-
// compile-time dependency on Labels.class, it will get relocated by the
193-
// shadowing process, and so won't match the annotation the user put on
194-
// their method.
195-
final String labelsClass = "com.appland.appmap.annotation.Labels";
196-
if (behavior.hasAnnotation(labelsClass)) {
197-
Object annotation = CtAppMapClassType.getAnnotation(behavior, labelsClass);
198-
Method value = annotation.getClass().getMethod("value");
199-
labels = (String[])(value.invoke(annotation));
200-
}
201-
} catch (ClassNotFoundException e) {
202-
Logger.println(e);
203-
} catch (IllegalAccessException e) {
204-
Logger.println(e);
205-
} catch (InvocationTargetException e) {
206-
Logger.println(e);
207-
} catch (NoSuchMethodException e) {
208-
Logger.println(e);
188+
String[] annotationLabels = LabelUtil.readAnnotationLabels(behavior);
189+
if (annotationLabels != null) {
190+
labels = annotationLabels;
209191
}
210192

211193
this.setType("function")

agent/src/main/java/com/appland/appmap/process/conditions/ConfigCondition.java

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import com.appland.appmap.transform.annotations.AppMapAppMethod;
1212
import com.appland.appmap.util.AppMapBehavior;
1313
import com.appland.appmap.util.FullyQualifiedName;
14+
import com.appland.appmap.util.LabelUtil;
1415
import com.appland.appmap.util.Logger;
1516

1617
import javassist.CtBehavior;
@@ -57,7 +58,7 @@ private boolean doMatch(CtBehavior behavior, Map<String, Object> matchResult) {
5758
}
5859
}
5960

60-
if (!AppMapBehavior.isRecordable(behavior) || ignoreMethod(behavior)) {
61+
if (!AppMapBehavior.isRecordable(behavior)) {
6162
return false;
6263
}
6364

@@ -67,12 +68,31 @@ private boolean doMatch(CtBehavior behavior, Map<String, Object> matchResult) {
6768
}
6869

6970
final AppMapPackage.LabelConfig ls = AppMapConfig.get().includes(new FullyQualifiedName(behavior));
70-
if (ls != null) {
71-
matchResult.put("labels", ls.getLabels());
72-
return true;
71+
if (ls == null) {
72+
return false;
7373
}
7474

75-
return false;
75+
// Explicit opt-ins override the trivial-method filter:
76+
// - @Labels annotation on the method
77+
// - method named directly under "methods:" in appmap.yml
78+
// - labels attached to the method via appmap.yml
79+
if (!isExplicitlyLabeled(behavior, ls) && ignoreMethod(behavior)) {
80+
return false;
81+
}
82+
83+
matchResult.put("labels", ls.getLabels());
84+
return true;
85+
}
86+
87+
private static boolean isExplicitlyLabeled(CtBehavior behavior, AppMapPackage.LabelConfig ls) {
88+
if (LabelUtil.hasLabelAnnotation(behavior)) {
89+
return true;
90+
}
91+
if (ls.isExplicit()) {
92+
return true;
93+
}
94+
String[] configLabels = ls.getLabels();
95+
return configLabels != null && configLabels.length > 0;
7696
}
7797

7898
private static final Pattern SETTER_PATTERN = Pattern.compile("^set[A-Z].*");
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
package com.appland.appmap.util;
2+
3+
import java.lang.reflect.Method;
4+
5+
import javassist.CtAppMapClassType;
6+
import javassist.CtBehavior;
7+
8+
/**
9+
* Reads the {@code @Labels} annotation from a {@link CtBehavior} by class name, avoiding a
10+
* compile-time dependency on {@code com.appland.appmap.annotation.Labels}. The annotation class
11+
* gets relocated by the agent's shadowing process, so a direct reference would not match the
12+
* annotation the user actually placed on their method.
13+
*/
14+
public final class LabelUtil {
15+
public static final String LABELS_CLASS = "com.appland.appmap.annotation.Labels";
16+
17+
private LabelUtil() {}
18+
19+
public static boolean hasLabelAnnotation(CtBehavior behavior) {
20+
try {
21+
return behavior.hasAnnotation(LABELS_CLASS);
22+
} catch (Exception e) {
23+
Logger.println(e);
24+
return false;
25+
}
26+
}
27+
28+
/**
29+
* @return the {@code value()} of the {@code @Labels} annotation on the given behavior, or
30+
* {@code null} if the annotation is not present or cannot be read.
31+
*/
32+
public static String[] readAnnotationLabels(CtBehavior behavior) {
33+
try {
34+
if (!behavior.hasAnnotation(LABELS_CLASS)) {
35+
return null;
36+
}
37+
Object annotation = CtAppMapClassType.getAnnotation(behavior, LABELS_CLASS);
38+
Method value = annotation.getClass().getMethod("value");
39+
return (String[])(value.invoke(annotation));
40+
} catch (Exception e) {
41+
Logger.println(e);
42+
return null;
43+
}
44+
}
45+
}

agent/src/test/java/com/appland/appmap/process/conditions/ConfigConditionTest.java

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,16 @@
55
import static org.junit.jupiter.api.DynamicContainer.dynamicContainer;
66
import static org.junit.jupiter.api.DynamicTest.dynamicTest;
77

8+
import java.util.HashMap;
9+
import java.util.Map;
810
import java.util.stream.Stream;
911

1012
import org.junit.jupiter.api.AfterAll;
13+
import org.junit.jupiter.api.AfterEach;
1114
import org.junit.jupiter.api.BeforeAll;
15+
import org.junit.jupiter.api.BeforeEach;
1216
import org.junit.jupiter.api.DynamicNode;
17+
import org.junit.jupiter.api.Nested;
1318
import org.junit.jupiter.api.Test;
1419
import org.junit.jupiter.api.TestFactory;
1520
import org.junit.jupiter.api.extension.ExtensionContext;
@@ -21,9 +26,12 @@
2126
import org.junit.jupiter.params.provider.Arguments;
2227
import org.junit.jupiter.params.provider.MethodSource;
2328

29+
import com.appland.appmap.config.AppMapConfig;
30+
import com.appland.appmap.config.AppMapPackage;
2431
import com.appland.appmap.test.util.ClassBuilder;
2532
import com.appland.appmap.test.util.MethodBuilder;
2633
import com.appland.appmap.util.AppMapClassPool;
34+
import com.appland.appmap.util.LabelUtil;
2735

2836
import javassist.CtClass;
2937

@@ -142,6 +150,101 @@ static Stream<Arguments> notSetters() {
142150
.setReturnType("java.lang.Integer").endMethod()));
143151
}
144152

153+
@Nested
154+
class TrivialFilterBypass {
155+
private static final String PKG = "com.appland.testfixture";
156+
157+
private final ConfigCondition condition = new ConfigCondition();
158+
private AppMapPackage[] originalPackages;
159+
private int classCounter;
160+
161+
@BeforeEach
162+
public void saveConfig() {
163+
originalPackages = AppMapConfig.get().packages;
164+
}
165+
166+
@AfterEach
167+
public void restoreConfig() {
168+
AppMapConfig.get().packages = originalPackages;
169+
}
170+
171+
private MethodBuilder freshGetter(String methodName) {
172+
ClassBuilder cb = new ClassBuilder(PKG + ".Class" + (classCounter++));
173+
MethodBuilder mb = cb.beginMethod();
174+
mb.setName(methodName)
175+
.setBody("return \"x\";");
176+
try {
177+
mb.setReturnType("java.lang.String");
178+
} catch (Exception e) {
179+
throw new RuntimeException(e);
180+
}
181+
return mb;
182+
}
183+
184+
private boolean matches(MethodBuilder mb) {
185+
Map<String, Object> result = new HashMap<>();
186+
return condition.match(mb.getBehavior(), result);
187+
}
188+
189+
@Test
190+
public void plainGetterIsSkippedWithoutLabel() throws Exception {
191+
AppMapConfig.get().packages = new AppMapPackage[] {
192+
new AppMapPackage(PKG, null, false, null)
193+
};
194+
MethodBuilder mb = freshGetter("getValue");
195+
mb.endMethod();
196+
assertFalse(matches(mb));
197+
}
198+
199+
@Test
200+
public void getterWithLabelsAnnotationIsRecorded() throws Exception {
201+
AppMapConfig.get().packages = new AppMapPackage[] {
202+
new AppMapPackage(PKG, null, false, null)
203+
};
204+
MethodBuilder mb = freshGetter("getSecret");
205+
mb.addAnnotation(LabelUtil.LABELS_CLASS).endMethod();
206+
assertTrue(matches(mb));
207+
}
208+
209+
@Test
210+
public void setterWithLabelsAnnotationIsRecorded() throws Exception {
211+
AppMapConfig.get().packages = new AppMapPackage[] {
212+
new AppMapPackage(PKG, null, false, null)
213+
};
214+
ClassBuilder cb = new ClassBuilder(PKG + ".Class" + (classCounter++));
215+
MethodBuilder mb = cb.beginMethod();
216+
mb.setName("setSecret")
217+
.addParameter("java.lang.String", "value")
218+
.addAnnotation(LabelUtil.LABELS_CLASS)
219+
.endMethod();
220+
assertTrue(matches(mb));
221+
}
222+
223+
@Test
224+
public void getterExplicitlyNamedInConfigIsRecorded() throws Exception {
225+
AppMapPackage.LabelConfig methodConfig = new AppMapPackage.LabelConfig(
226+
"Class.*", "getValue", new String[] {});
227+
AppMapConfig.get().packages = new AppMapPackage[] {
228+
new AppMapPackage(PKG, null, false, new AppMapPackage.LabelConfig[] { methodConfig })
229+
};
230+
MethodBuilder mb = freshGetter("getValue");
231+
mb.endMethod();
232+
assertTrue(matches(mb));
233+
}
234+
235+
@Test
236+
public void getterWithLabelsAttachedInConfigIsRecorded() throws Exception {
237+
AppMapPackage.LabelConfig methodConfig = new AppMapPackage.LabelConfig(
238+
"Class.*", "getValue", new String[] { "secret" });
239+
AppMapConfig.get().packages = new AppMapPackage[] {
240+
new AppMapPackage(PKG, null, false, new AppMapPackage.LabelConfig[] { methodConfig })
241+
};
242+
MethodBuilder mb = freshGetter("getValue");
243+
mb.endMethod();
244+
assertTrue(matches(mb));
245+
}
246+
}
247+
145248
static class ClassBuilderResolver implements ParameterResolver {
146249
@Override
147250
public boolean supportsParameter(ParameterContext parameterContext,

agent/src/test/java/com/appland/appmap/test/util/MethodBuilder.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import javassist.bytecode.CodeAttribute;
1616
import javassist.bytecode.ConstPool;
1717
import javassist.bytecode.Descriptor;
18+
import javassist.bytecode.LineNumberAttributeTestHelper;
1819
import javassist.bytecode.LocalVariableAttribute;
1920
import javassist.bytecode.annotation.Annotation;
2021

@@ -30,6 +31,7 @@ public class MethodBuilder {
3031
private Integer modifiers = Modifier.PUBLIC;
3132
private List<ParameterBuilder> parameters = new ArrayList<ParameterBuilder>();
3233
private List<AnnotationBuilder> annotations = new ArrayList<AnnotationBuilder>();
34+
private boolean withLineNumber = true;
3335
private CtMethod behavior;
3436

3537
public CtMethod getBehavior() {
@@ -249,6 +251,9 @@ private CtMethod build() throws CannotCompileException {
249251

250252
codeAttribute.getAttributes().add(locals);
251253

254+
if (withLineNumber) {
255+
codeAttribute.getAttributes().add(LineNumberAttributeTestHelper.singleEntry(constPool, 1));
256+
}
252257

253258
final AnnotationsAttribute annotationAttribute =
254259
new AnnotationsAttribute(constPool, AnnotationsAttribute.visibleTag);
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
package com.appland.appmap.util;
2+
3+
import static org.junit.jupiter.api.Assertions.assertFalse;
4+
import static org.junit.jupiter.api.Assertions.assertTrue;
5+
6+
import org.junit.jupiter.api.AfterAll;
7+
import org.junit.jupiter.api.BeforeAll;
8+
import org.junit.jupiter.api.Test;
9+
10+
import com.appland.appmap.test.util.ClassBuilder;
11+
import com.appland.appmap.test.util.MethodBuilder;
12+
13+
public class LabelUtilTest {
14+
@BeforeAll
15+
public static void beforeAll() {
16+
AppMapClassPool.acquire(Thread.currentThread().getContextClassLoader());
17+
}
18+
19+
@AfterAll
20+
public static void afterAll() throws Exception {
21+
AppMapClassPool.release();
22+
}
23+
24+
@Test
25+
public void detectsLabelsAnnotationByName() throws Exception {
26+
MethodBuilder mb = new ClassBuilder("LabelUtilTest$Labeled").beginMethod();
27+
mb.setName("getSecret")
28+
.setBody("return \"x\";")
29+
.setReturnType("java.lang.String")
30+
.addAnnotation(LabelUtil.LABELS_CLASS)
31+
.endMethod();
32+
assertTrue(LabelUtil.hasLabelAnnotation(mb.getBehavior()));
33+
}
34+
35+
@Test
36+
public void unlabeledMethodReturnsFalse() throws Exception {
37+
MethodBuilder mb = new ClassBuilder("LabelUtilTest$Unlabeled").beginMethod();
38+
mb.setName("getSecret")
39+
.setBody("return \"x\";")
40+
.setReturnType("java.lang.String")
41+
.endMethod();
42+
assertFalse(LabelUtil.hasLabelAnnotation(mb.getBehavior()));
43+
}
44+
}

0 commit comments

Comments
 (0)