Skip to content

Commit 4012208

Browse files
prdoyleprdoyle
authored
Mandatory class name (#118626)
* Use $$ for static check methods * Always use class name from checker method name * Spotless * Varargs assertParseCheckerMethodSignatureThrows * Remove hasReceiver again * Spotless * Cosmetic changes * Change to new check method naming convention
1 parent 009b597 commit 4012208

File tree

9 files changed

+108
-121
lines changed

9 files changed

+108
-121
lines changed

‎libs/entitlement/asm-provider/src/main/java/org/elasticsearch/entitlement/instrumentation/impl/InstrumentationServiceImpl.java‎

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -67,9 +67,19 @@ public MethodVisitor visitMethod(
6767
private static final Type CLASS_TYPE = Type.getType(Class.class);
6868

6969
static MethodKey parseCheckerMethodSignature(String checkerMethodName, Type[] checkerMethodArgumentTypes) {
70-
var classNameStartIndex = checkerMethodName.indexOf('$');
71-
var classNameEndIndex = checkerMethodName.lastIndexOf('$');
70+
boolean targetMethodIsStatic;
71+
int classNameEndIndex = checkerMethodName.lastIndexOf("$$");
72+
int methodNameStartIndex;
73+
if (classNameEndIndex == -1) {
74+
targetMethodIsStatic = false;
75+
classNameEndIndex = checkerMethodName.lastIndexOf('$');
76+
methodNameStartIndex = classNameEndIndex + 1;
77+
} else {
78+
targetMethodIsStatic = true;
79+
methodNameStartIndex = classNameEndIndex + 2;
80+
}
7281

82+
var classNameStartIndex = checkerMethodName.indexOf('$');
7383
if (classNameStartIndex == -1 || classNameStartIndex >= classNameEndIndex) {
7484
throw new IllegalArgumentException(
7585
String.format(
@@ -82,15 +92,17 @@ static MethodKey parseCheckerMethodSignature(String checkerMethodName, Type[] ch
8292
);
8393
}
8494

85-
// No "className" (check$$methodName) -> method is instance, and we'll get the class from the actual typed argument
86-
final boolean targetMethodIsStatic = classNameStartIndex + 1 != classNameEndIndex;
8795
// No "methodName" (check$package_ClassName$) -> method is ctor
8896
final boolean targetMethodIsCtor = classNameEndIndex + 1 == checkerMethodName.length();
89-
final String targetMethodName = targetMethodIsCtor ? "<init>" : checkerMethodName.substring(classNameEndIndex + 1);
97+
final String targetMethodName = targetMethodIsCtor ? "<init>" : checkerMethodName.substring(methodNameStartIndex);
98+
99+
final String targetClassName = checkerMethodName.substring(classNameStartIndex + 1, classNameEndIndex).replace('_', '/');
100+
if (targetClassName.isBlank()) {
101+
throw new IllegalArgumentException(String.format(Locale.ROOT, "Checker method %s has no class name", checkerMethodName));
102+
}
90103

91-
final String targetClassName;
92104
final List<String> targetParameterTypes;
93-
if (targetMethodIsStatic) {
105+
if (targetMethodIsStatic || targetMethodIsCtor) {
94106
if (checkerMethodArgumentTypes.length < 1 || CLASS_TYPE.equals(checkerMethodArgumentTypes[0]) == false) {
95107
throw new IllegalArgumentException(
96108
String.format(
@@ -101,7 +113,6 @@ static MethodKey parseCheckerMethodSignature(String checkerMethodName, Type[] ch
101113
);
102114
}
103115

104-
targetClassName = checkerMethodName.substring(classNameStartIndex + 1, classNameEndIndex).replace('_', '/');
105116
targetParameterTypes = Arrays.stream(checkerMethodArgumentTypes).skip(1).map(Type::getInternalName).toList();
106117
} else {
107118
if (checkerMethodArgumentTypes.length < 2
@@ -117,10 +128,9 @@ static MethodKey parseCheckerMethodSignature(String checkerMethodName, Type[] ch
117128
)
118129
);
119130
}
120-
var targetClassType = checkerMethodArgumentTypes[1];
121-
targetClassName = targetClassType.getInternalName();
122131
targetParameterTypes = Arrays.stream(checkerMethodArgumentTypes).skip(2).map(Type::getInternalName).toList();
123132
}
133+
boolean hasReceiver = (targetMethodIsStatic || targetMethodIsCtor) == false;
124134
return new MethodKey(targetClassName, targetMethodName, targetParameterTypes);
125135
}
126136
}

‎libs/entitlement/asm-provider/src/main/java/org/elasticsearch/entitlement/instrumentation/impl/InstrumenterImpl.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,7 @@ public MethodVisitor visitMethod(int access, String name, String descriptor, Str
152152
if (isAnnotationPresent == false) {
153153
boolean isStatic = (access & ACC_STATIC) != 0;
154154
boolean isCtor = "<init>".equals(name);
155+
boolean hasReceiver = (isStatic || isCtor) == false;
155156
var key = new MethodKey(className, name, Stream.of(Type.getArgumentTypes(descriptor)).map(Type::getInternalName).toList());
156157
var instrumentationMethod = checkMethods.get(key);
157158
if (instrumentationMethod != null) {

‎libs/entitlement/asm-provider/src/test/java/org/elasticsearch/entitlement/instrumentation/impl/InstrumentationServiceImplTests.java‎

Lines changed: 54 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -32,17 +32,17 @@ public class InstrumentationServiceImplTests extends ESTestCase {
3232
static class TestTargetClass {}
3333

3434
interface TestChecker {
35-
void check$org_example_TestTargetClass$staticMethod(Class<?> clazz, int arg0, String arg1, Object arg2);
35+
void check$org_example_TestTargetClass$$staticMethod(Class<?> clazz, int arg0, String arg1, Object arg2);
3636

37-
void check$$instanceMethodNoArgs(Class<?> clazz, TestTargetClass that);
37+
void check$org_example_TestTargetClass$instanceMethodNoArgs(Class<?> clazz, TestTargetClass that);
3838

39-
void check$$instanceMethodWithArgs(Class<?> clazz, TestTargetClass that, int x, int y);
39+
void check$org_example_TestTargetClass$instanceMethodWithArgs(Class<?> clazz, TestTargetClass that, int x, int y);
4040
}
4141

4242
interface TestCheckerOverloads {
43-
void check$org_example_TestTargetClass$staticMethodWithOverload(Class<?> clazz, int x, int y);
43+
void check$org_example_TestTargetClass$$staticMethodWithOverload(Class<?> clazz, int x, int y);
4444

45-
void check$org_example_TestTargetClass$staticMethodWithOverload(Class<?> clazz, int x, String y);
45+
void check$org_example_TestTargetClass$$staticMethodWithOverload(Class<?> clazz, int x, String y);
4646
}
4747

4848
interface TestCheckerCtors {
@@ -62,7 +62,7 @@ public void testInstrumentationTargetLookup() throws IOException {
6262
equalTo(
6363
new CheckMethod(
6464
"org/elasticsearch/entitlement/instrumentation/impl/InstrumentationServiceImplTests$TestChecker",
65-
"check$org_example_TestTargetClass$staticMethod",
65+
"check$org_example_TestTargetClass$$staticMethod",
6666
List.of("Ljava/lang/Class;", "I", "Ljava/lang/String;", "Ljava/lang/Object;")
6767
)
6868
)
@@ -71,17 +71,11 @@ public void testInstrumentationTargetLookup() throws IOException {
7171
assertThat(
7272
checkMethods,
7373
hasEntry(
74-
equalTo(
75-
new MethodKey(
76-
"org/elasticsearch/entitlement/instrumentation/impl/InstrumentationServiceImplTests$TestTargetClass",
77-
"instanceMethodNoArgs",
78-
List.of()
79-
)
80-
),
74+
equalTo(new MethodKey("org/example/TestTargetClass", "instanceMethodNoArgs", List.of())),
8175
equalTo(
8276
new CheckMethod(
8377
"org/elasticsearch/entitlement/instrumentation/impl/InstrumentationServiceImplTests$TestChecker",
84-
"check$$instanceMethodNoArgs",
78+
"check$org_example_TestTargetClass$instanceMethodNoArgs",
8579
List.of(
8680
"Ljava/lang/Class;",
8781
"Lorg/elasticsearch/entitlement/instrumentation/impl/InstrumentationServiceImplTests$TestTargetClass;"
@@ -93,17 +87,11 @@ public void testInstrumentationTargetLookup() throws IOException {
9387
assertThat(
9488
checkMethods,
9589
hasEntry(
96-
equalTo(
97-
new MethodKey(
98-
"org/elasticsearch/entitlement/instrumentation/impl/InstrumentationServiceImplTests$TestTargetClass",
99-
"instanceMethodWithArgs",
100-
List.of("I", "I")
101-
)
102-
),
90+
equalTo(new MethodKey("org/example/TestTargetClass", "instanceMethodWithArgs", List.of("I", "I"))),
10391
equalTo(
10492
new CheckMethod(
10593
"org/elasticsearch/entitlement/instrumentation/impl/InstrumentationServiceImplTests$TestChecker",
106-
"check$$instanceMethodWithArgs",
94+
"check$org_example_TestTargetClass$instanceMethodWithArgs",
10795
List.of(
10896
"Ljava/lang/Class;",
10997
"Lorg/elasticsearch/entitlement/instrumentation/impl/InstrumentationServiceImplTests$TestTargetClass;",
@@ -127,7 +115,7 @@ public void testInstrumentationTargetLookupWithOverloads() throws IOException {
127115
equalTo(
128116
new CheckMethod(
129117
"org/elasticsearch/entitlement/instrumentation/impl/InstrumentationServiceImplTests$TestCheckerOverloads",
130-
"check$org_example_TestTargetClass$staticMethodWithOverload",
118+
"check$org_example_TestTargetClass$$staticMethodWithOverload",
131119
List.of("Ljava/lang/Class;", "I", "Ljava/lang/String;")
132120
)
133121
)
@@ -140,7 +128,7 @@ public void testInstrumentationTargetLookupWithOverloads() throws IOException {
140128
equalTo(
141129
new CheckMethod(
142130
"org/elasticsearch/entitlement/instrumentation/impl/InstrumentationServiceImplTests$TestCheckerOverloads",
143-
"check$org_example_TestTargetClass$staticMethodWithOverload",
131+
"check$org_example_TestTargetClass$$staticMethodWithOverload",
144132
List.of("Ljava/lang/Class;", "I", "I")
145133
)
146134
)
@@ -182,7 +170,7 @@ public void testInstrumentationTargetLookupWithCtors() throws IOException {
182170

183171
public void testParseCheckerMethodSignatureStaticMethod() {
184172
var methodKey = InstrumentationServiceImpl.parseCheckerMethodSignature(
185-
"check$org_example_TestClass$staticMethod",
173+
"check$org_example_TestClass$$staticMethod",
186174
new Type[] { Type.getType(Class.class) }
187175
);
188176

@@ -191,7 +179,7 @@ public void testParseCheckerMethodSignatureStaticMethod() {
191179

192180
public void testParseCheckerMethodSignatureStaticMethodWithArgs() {
193181
var methodKey = InstrumentationServiceImpl.parseCheckerMethodSignature(
194-
"check$org_example_TestClass$staticMethod",
182+
"check$org_example_TestClass$$staticMethod",
195183
new Type[] { Type.getType(Class.class), Type.getType("I"), Type.getType(String.class) }
196184
);
197185

@@ -200,7 +188,7 @@ public void testParseCheckerMethodSignatureStaticMethodWithArgs() {
200188

201189
public void testParseCheckerMethodSignatureStaticMethodInnerClass() {
202190
var methodKey = InstrumentationServiceImpl.parseCheckerMethodSignature(
203-
"check$org_example_TestClass$InnerClass$staticMethod",
191+
"check$org_example_TestClass$InnerClass$$staticMethod",
204192
new Type[] { Type.getType(Class.class) }
205193
);
206194

@@ -225,94 +213,80 @@ public void testParseCheckerMethodSignatureCtorWithArgs() {
225213
assertThat(methodKey, equalTo(new MethodKey("org/example/TestClass", "<init>", List.of("I", "java/lang/String"))));
226214
}
227215

228-
public void testParseCheckerMethodSignatureIncorrectName() {
229-
var exception = assertThrows(
230-
IllegalArgumentException.class,
231-
() -> InstrumentationServiceImpl.parseCheckerMethodSignature("check$staticMethod", new Type[] { Type.getType(Class.class) })
232-
);
216+
public void testParseCheckerMethodSignatureOneDollarSign() {
217+
assertParseCheckerMethodSignatureThrows("has incorrect name format", "check$method", Type.getType(Class.class));
218+
}
233219

234-
assertThat(exception.getMessage(), containsString("has incorrect name format"));
220+
public void testParseCheckerMethodSignatureMissingClass() {
221+
assertParseCheckerMethodSignatureThrows("has incorrect name format", "check$$staticMethod", Type.getType(Class.class));
222+
}
223+
224+
public void testParseCheckerMethodSignatureBlankClass() {
225+
assertParseCheckerMethodSignatureThrows("no class name", "check$$$staticMethod", Type.getType(Class.class));
235226
}
236227

237228
public void testParseCheckerMethodSignatureStaticMethodIncorrectArgumentCount() {
238-
var exception = assertThrows(
239-
IllegalArgumentException.class,
240-
() -> InstrumentationServiceImpl.parseCheckerMethodSignature("check$ClassName$staticMethod", new Type[] {})
241-
);
242-
assertThat(exception.getMessage(), containsString("It must have a first argument of Class<?> type"));
229+
assertParseCheckerMethodSignatureThrows("It must have a first argument of Class<?> type", "check$ClassName$staticMethod");
243230
}
244231

245232
public void testParseCheckerMethodSignatureStaticMethodIncorrectArgumentType() {
246-
var exception = assertThrows(
247-
IllegalArgumentException.class,
248-
() -> InstrumentationServiceImpl.parseCheckerMethodSignature(
249-
"check$ClassName$staticMethod",
250-
new Type[] { Type.getType(String.class) }
251-
)
233+
assertParseCheckerMethodSignatureThrows(
234+
"It must have a first argument of Class<?> type",
235+
"check$ClassName$$staticMethod",
236+
Type.getType(String.class)
252237
);
253-
assertThat(exception.getMessage(), containsString("It must have a first argument of Class<?> type"));
254238
}
255239

256240
public void testParseCheckerMethodSignatureInstanceMethod() {
257241
var methodKey = InstrumentationServiceImpl.parseCheckerMethodSignature(
258-
"check$$instanceMethod",
242+
"check$org_example_TestClass$instanceMethod",
259243
new Type[] { Type.getType(Class.class), Type.getType(TestTargetClass.class) }
260244
);
261245

262-
assertThat(
263-
methodKey,
264-
equalTo(
265-
new MethodKey(
266-
"org/elasticsearch/entitlement/instrumentation/impl/InstrumentationServiceImplTests$TestTargetClass",
267-
"instanceMethod",
268-
List.of()
269-
)
270-
)
271-
);
246+
assertThat(methodKey, equalTo(new MethodKey("org/example/TestClass", "instanceMethod", List.of())));
272247
}
273248

274249
public void testParseCheckerMethodSignatureInstanceMethodWithArgs() {
275250
var methodKey = InstrumentationServiceImpl.parseCheckerMethodSignature(
276-
"check$$instanceMethod",
251+
"check$org_example_TestClass$instanceMethod",
277252
new Type[] { Type.getType(Class.class), Type.getType(TestTargetClass.class), Type.getType("I"), Type.getType(String.class) }
278253
);
279254

280-
assertThat(
281-
methodKey,
282-
equalTo(
283-
new MethodKey(
284-
"org/elasticsearch/entitlement/instrumentation/impl/InstrumentationServiceImplTests$TestTargetClass",
285-
"instanceMethod",
286-
List.of("I", "java/lang/String")
287-
)
288-
)
289-
);
255+
assertThat(methodKey, equalTo(new MethodKey("org/example/TestClass", "instanceMethod", List.of("I", "java/lang/String"))));
290256
}
291257

292258
public void testParseCheckerMethodSignatureInstanceMethodIncorrectArgumentTypes() {
293-
var exception = assertThrows(
294-
IllegalArgumentException.class,
295-
() -> InstrumentationServiceImpl.parseCheckerMethodSignature("check$$instanceMethod", new Type[] { Type.getType(String.class) })
259+
assertParseCheckerMethodSignatureThrows(
260+
"It must have a first argument of Class<?> type",
261+
"check$org_example_TestClass$instanceMethod",
262+
Type.getType(String.class)
296263
);
297-
assertThat(exception.getMessage(), containsString("It must have a first argument of Class<?> type"));
298264
}
299265

300266
public void testParseCheckerMethodSignatureInstanceMethodIncorrectArgumentCount() {
301-
var exception = assertThrows(
302-
IllegalArgumentException.class,
303-
() -> InstrumentationServiceImpl.parseCheckerMethodSignature("check$$instanceMethod", new Type[] { Type.getType(Class.class) })
267+
assertParseCheckerMethodSignatureThrows(
268+
"a second argument of the class containing the method to instrument",
269+
"check$org_example_TestClass$instanceMethod",
270+
Type.getType(Class.class)
304271
);
305-
assertThat(exception.getMessage(), containsString("a second argument of the class containing the method to instrument"));
306272
}
307273

308274
public void testParseCheckerMethodSignatureInstanceMethodIncorrectArgumentTypes2() {
275+
assertParseCheckerMethodSignatureThrows(
276+
"a second argument of the class containing the method to instrument",
277+
"check$org_example_TestClass$instanceMethod",
278+
Type.getType(Class.class),
279+
Type.getType("I")
280+
);
281+
}
282+
283+
private static void assertParseCheckerMethodSignatureThrows(String messageText, String methodName, Type... methodArgs) {
309284
var exception = assertThrows(
310285
IllegalArgumentException.class,
311-
() -> InstrumentationServiceImpl.parseCheckerMethodSignature(
312-
"check$$instanceMethod",
313-
new Type[] { Type.getType(Class.class), Type.getType("I") }
314-
)
286+
() -> InstrumentationServiceImpl.parseCheckerMethodSignature(methodName, methodArgs)
315287
);
316-
assertThat(exception.getMessage(), containsString("a second argument of the class containing the method to instrument"));
288+
289+
assertThat(exception.getMessage(), containsString(messageText));
317290
}
291+
318292
}

0 commit comments

Comments
 (0)