From 43d39d4e8ae67b10c12c44805a310d68164b5139 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Mon, 3 Oct 2022 11:23:14 +0200 Subject: [PATCH] Enforce need for type reflection in RuntimeHintsAgent Prior to this commit, the AOT infrastructure would rely on the fact that native runtime reflection on a type would only consider methods/fields/constructors that had specific hints contributed. When listing them through the reflection API on the type, the native image would only return those for which we had hints contributed. This behavior will soon change in GraalVM and will better align with the JVM behavior: when asking for all declared methods on a type in a native image, we should get all existing methods, not just the ones registered previously in the native image. This commit aligns the behavior of the `RuntimeHintsAgent` and removes the now misleading predicates as a consequence. Closes gh-29205 --- .../aot/RuntimeHintsAgentTests.java | 16 ------- .../aot/agent/InstrumentedBridgeMethods.java | 27 ----------- .../aot/agent/InstrumentedMethod.java | 27 +++-------- .../aot/agent/InstrumentedMethodTests.java | 47 +++++-------------- .../predicate/ReflectionHintsPredicates.java | 23 --------- .../ReflectionHintsPredicatesTests.java | 36 -------------- 6 files changed, 18 insertions(+), 158 deletions(-) diff --git a/integration-tests/src/test/java/org/springframework/aot/RuntimeHintsAgentTests.java b/integration-tests/src/test/java/org/springframework/aot/RuntimeHintsAgentTests.java index e29518bae307..2309cbc01d6e 100644 --- a/integration-tests/src/test/java/org/springframework/aot/RuntimeHintsAgentTests.java +++ b/integration-tests/src/test/java/org/springframework/aot/RuntimeHintsAgentTests.java @@ -164,22 +164,6 @@ private static Stream instrumentedReflectionMethods() { throw new RuntimeException(e); } }, MethodReference.of(Method.class, "invoke")), - Arguments.of((Runnable) () -> { - try { - toStringMethod.getAnnotations(); - } - catch (Exception e) { - throw new RuntimeException(e); - } - }, MethodReference.of(Method.class, "getAnnotations")), - Arguments.of((Runnable) () -> { - try { - toStringMethod.getParameterTypes(); - } - catch (Exception e) { - throw new RuntimeException(e); - } - }, MethodReference.of(Method.class, "getParameterTypes")), Arguments.of((Runnable) () -> { try { privateGreetMethod.invoke(new PrivateClass()); diff --git a/spring-core-test/src/main/java/org/springframework/aot/agent/InstrumentedBridgeMethods.java b/spring-core-test/src/main/java/org/springframework/aot/agent/InstrumentedBridgeMethods.java index 30c9aef2c5df..df2d880c6ee2 100644 --- a/spring-core-test/src/main/java/org/springframework/aot/agent/InstrumentedBridgeMethods.java +++ b/spring-core-test/src/main/java/org/springframework/aot/agent/InstrumentedBridgeMethods.java @@ -18,7 +18,6 @@ import java.io.IOException; import java.io.InputStream; -import java.lang.annotation.Annotation; import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.lang.reflect.InvocationHandler; @@ -336,32 +335,6 @@ public static Object constructornewInstance(Constructor constructor, Object.. * Bridge methods for java.lang.reflect.Method */ - public static Annotation[] methodgetAnnotations(Method method) { - Annotation[] result = null; - try { - result = method.getAnnotations(); - } - finally { - RecordedInvocation invocation = RecordedInvocation.of(InstrumentedMethod.METHOD_GETANNOTATIONS) - .onInstance(method).returnValue(result).build(); - RecordedInvocationsPublisher.publish(invocation); - } - return result; - } - - public static Class[] methodgetParameterTypes(Method method) { - Class[] result = null; - try { - result = method.getParameterTypes(); - } - finally { - RecordedInvocation invocation = RecordedInvocation.of(InstrumentedMethod.METHOD_GETPARAMETERTYPES) - .onInstance(method).returnValue(result).build(); - RecordedInvocationsPublisher.publish(invocation); - } - return result; - } - public static Object methodinvoke(Method method, Object object, Object... arguments) throws InvocationTargetException, IllegalAccessException { Object result = null; boolean accessibilityChanged = false; diff --git a/spring-core-test/src/main/java/org/springframework/aot/agent/InstrumentedMethod.java b/spring-core-test/src/main/java/org/springframework/aot/agent/InstrumentedMethod.java index 091cfbef583c..d798c10fd72d 100644 --- a/spring-core-test/src/main/java/org/springframework/aot/agent/InstrumentedMethod.java +++ b/spring-core-test/src/main/java/org/springframework/aot/agent/InstrumentedMethod.java @@ -93,8 +93,7 @@ enum InstrumentedMethod { Class thisClass = invocation.getInstance(); return reflection().onType(TypeReference.of(thisClass)).withAnyMemberCategory( MemberCategory.INTROSPECT_PUBLIC_CONSTRUCTORS, MemberCategory.INTROSPECT_DECLARED_CONSTRUCTORS, - MemberCategory.INVOKE_PUBLIC_CONSTRUCTORS, MemberCategory.INVOKE_DECLARED_CONSTRUCTORS) - .or(reflection().onType(TypeReference.of(thisClass)).withAnyConstructor()); + MemberCategory.INVOKE_PUBLIC_CONSTRUCTORS, MemberCategory.INVOKE_DECLARED_CONSTRUCTORS); } ), @@ -130,8 +129,7 @@ enum InstrumentedMethod { invocation -> { Class thisClass = invocation.getInstance(); return reflection().onType(TypeReference.of(thisClass)) - .withAnyMemberCategory(MemberCategory.INTROSPECT_DECLARED_CONSTRUCTORS, MemberCategory.INVOKE_DECLARED_CONSTRUCTORS) - .or(reflection().onType(TypeReference.of(thisClass)).withAnyConstructor()); + .withAnyMemberCategory(MemberCategory.INTROSPECT_DECLARED_CONSTRUCTORS, MemberCategory.INVOKE_DECLARED_CONSTRUCTORS); }), /** @@ -155,8 +153,7 @@ enum InstrumentedMethod { CLASS_GETDECLAREDFIELDS(Class.class, "getDeclaredFields", HintType.REFLECTION, invocation -> { Class thisClass = invocation.getInstance(); - return reflection().onType(TypeReference.of(thisClass)).withMemberCategory(MemberCategory.DECLARED_FIELDS) - .or(reflection().onType(TypeReference.of(thisClass)).withAnyField()); + return reflection().onType(TypeReference.of(thisClass)).withMemberCategory(MemberCategory.DECLARED_FIELDS); } ), @@ -183,8 +180,7 @@ enum InstrumentedMethod { invocation -> { Class thisClass = invocation.getInstance(); return reflection().onType(TypeReference.of(thisClass)) - .withAnyMemberCategory(MemberCategory.INTROSPECT_DECLARED_METHODS, MemberCategory.INVOKE_DECLARED_METHODS) - .or(reflection().onType(TypeReference.of(thisClass)).withAnyMethod()); + .withAnyMemberCategory(MemberCategory.INTROSPECT_DECLARED_METHODS, MemberCategory.INVOKE_DECLARED_METHODS); } ), @@ -211,8 +207,7 @@ enum InstrumentedMethod { invocation -> { Class thisClass = invocation.getInstance(); return reflection().onType(TypeReference.of(thisClass)) - .withAnyMemberCategory(MemberCategory.PUBLIC_FIELDS, MemberCategory.DECLARED_FIELDS) - .or(reflection().onType(TypeReference.of(thisClass)).withAnyField()); + .withAnyMemberCategory(MemberCategory.PUBLIC_FIELDS, MemberCategory.DECLARED_FIELDS); } ), @@ -242,8 +237,7 @@ enum InstrumentedMethod { Class thisClass = invocation.getInstance(); return reflection().onType(TypeReference.of(thisClass)).withAnyMemberCategory( MemberCategory.INTROSPECT_PUBLIC_METHODS, MemberCategory.INTROSPECT_DECLARED_METHODS, - MemberCategory.INVOKE_PUBLIC_METHODS, MemberCategory.INVOKE_DECLARED_METHODS) - .or(reflection().onType(TypeReference.of(thisClass)).withAnyMethod()); + MemberCategory.INVOKE_PUBLIC_METHODS, MemberCategory.INVOKE_DECLARED_METHODS); } ), @@ -265,15 +259,6 @@ enum InstrumentedMethod { CONSTRUCTOR_NEWINSTANCE(Constructor.class, "newInstance", HintType.REFLECTION, invocation -> reflection().onConstructor(invocation.getInstance()).invoke()), - /** - * {@link Method#getParameterTypes()}. - */ - METHOD_GETANNOTATIONS(Method.class, "getAnnotations", HintType.REFLECTION, - invocation -> reflection().onMethod(invocation.getInstance())), - - METHOD_GETPARAMETERTYPES(Method.class, "getParameterTypes", HintType.REFLECTION, - invocation -> reflection().onMethod(invocation.getInstance())), - /** * {@link Method#invoke(Object, Object...)}. */ diff --git a/spring-core-test/src/test/java/org/springframework/aot/agent/InstrumentedMethodTests.java b/spring-core-test/src/test/java/org/springframework/aot/agent/InstrumentedMethodTests.java index 6b989174d3f9..a9e4ab8520c0 100644 --- a/spring-core-test/src/test/java/org/springframework/aot/agent/InstrumentedMethodTests.java +++ b/spring-core-test/src/test/java/org/springframework/aot/agent/InstrumentedMethodTests.java @@ -16,7 +16,6 @@ package org.springframework.aot.agent; -import java.lang.reflect.Method; import java.lang.reflect.Proxy; import java.util.Collections; import java.util.Comparator; @@ -193,9 +192,9 @@ void classGetConstructorsShouldNotMatchTypeReflectionHint() { } @Test - void classGetConstructorsShouldMatchConstructorReflectionHint() throws Exception { + void classGetConstructorsShouldNotMatchConstructorReflectionHint() throws Exception { hints.reflection().registerConstructor(String.class.getConstructor(), ExecutableMode.INVOKE); - assertThatInvocationMatches(InstrumentedMethod.CLASS_GETCONSTRUCTORS, this.stringGetConstructors); + assertThatInvocationDoesNotMatch(InstrumentedMethod.CLASS_GETCONSTRUCTORS, this.stringGetConstructors); } @Test @@ -249,9 +248,9 @@ void classGetDeclaredConstructorsShouldNotMatchTypeReflectionHint() { } @Test - void classGetDeclaredConstructorsShouldMatchConstructorReflectionHint() throws Exception { + void classGetDeclaredConstructorsShouldNotMatchConstructorReflectionHint() throws Exception { hints.reflection().registerConstructor(String.class.getConstructor(), ExecutableMode.INVOKE); - assertThatInvocationMatches(InstrumentedMethod.CLASS_GETDECLAREDCONSTRUCTORS, this.stringGetDeclaredConstructors); + assertThatInvocationDoesNotMatch(InstrumentedMethod.CLASS_GETDECLAREDCONSTRUCTORS, this.stringGetDeclaredConstructors); } @Test @@ -349,9 +348,9 @@ void classGetDeclaredMethodsShouldNotMatchTypeReflectionHint() { } @Test - void classGetDeclaredMethodsShouldMatchMethodReflectionHint() throws Exception { + void classGetDeclaredMethodsShouldNotMatchMethodReflectionHint() throws Exception { hints.reflection().registerMethod(String.class.getMethod("toString"), ExecutableMode.INVOKE); - assertThatInvocationMatches(InstrumentedMethod.CLASS_GETDECLAREDMETHODS, this.stringGetScaleMethod); + assertThatInvocationDoesNotMatch(InstrumentedMethod.CLASS_GETDECLAREDMETHODS, this.stringGetScaleMethod); } @Test @@ -391,9 +390,9 @@ void classGetMethodsShouldNotMatchTypeReflectionHint() { } @Test - void classGetMethodsShouldMatchMethodReflectionHint() throws Exception { + void classGetMethodsShouldNotMatchMethodReflectionHint() throws Exception { hints.reflection().registerMethod(String.class.getMethod("toString"), ExecutableMode.INVOKE); - assertThatInvocationMatches(InstrumentedMethod.CLASS_GETMETHODS, this.stringGetMethods); + assertThatInvocationDoesNotMatch(InstrumentedMethod.CLASS_GETMETHODS, this.stringGetMethods); } @Test @@ -452,28 +451,6 @@ void classGetMethodShouldNotMatchForWrongType() { assertThatInvocationDoesNotMatch(InstrumentedMethod.CLASS_GETMETHOD, this.stringGetToStringMethod); } - @Test - void methodGetAnnotationsShouldMatchIntrospectHintOnMethod() throws NoSuchMethodException { - Method toString = String.class.getMethod("toString"); - RecordedInvocation invocation = RecordedInvocation.of(InstrumentedMethod.METHOD_GETANNOTATIONS) - .onInstance(toString).withArguments("testString") - .returnValue(toString.getAnnotations()).build(); - hints.reflection().registerType(String.class, typeHint -> typeHint.withMethod("toString", - Collections.emptyList(), ExecutableMode.INTROSPECT)); - assertThatInvocationMatches(InstrumentedMethod.METHOD_GETANNOTATIONS, invocation); - } - - @Test - void methodGetParameterTypesShouldMatchIntrospectHintOnMethod() throws NoSuchMethodException { - Method toString = String.class.getMethod("toString"); - RecordedInvocation invocation = RecordedInvocation.of(InstrumentedMethod.METHOD_GETPARAMETERTYPES) - .onInstance(toString).withArguments("testString") - .returnValue(toString.getParameterTypes()).build(); - hints.reflection().registerType(String.class, typeHint -> typeHint.withMethod("toString", - Collections.emptyList(), ExecutableMode.INTROSPECT)); - assertThatInvocationMatches(InstrumentedMethod.METHOD_GETPARAMETERTYPES, invocation); - } - @Test void methodInvokeShouldMatchInvokeHintOnMethod() throws NoSuchMethodException { RecordedInvocation invocation = RecordedInvocation.of(InstrumentedMethod.METHOD_INVOKE) @@ -555,9 +532,9 @@ void classGetDeclaredFieldsShouldNotMatchTypeHint() { } @Test - void classGetDeclaredFieldsShouldMatchFieldHint() throws Exception { + void classGetDeclaredFieldsShouldNotMatchFieldHint() throws Exception { hints.reflection().registerField(String.class.getDeclaredField("value")); - assertThatInvocationMatches(InstrumentedMethod.CLASS_GETDECLAREDFIELDS, this.stringGetDeclaredFields); + assertThatInvocationDoesNotMatch(InstrumentedMethod.CLASS_GETDECLAREDFIELDS, this.stringGetDeclaredFields); } @Test @@ -626,9 +603,9 @@ void classGetFieldsShouldNotMatchTypeHint() { } @Test - void classGetFieldsShouldMatchFieldHint() throws Exception { + void classGetFieldsShouldNotMatchFieldHint() throws Exception { hints.reflection().registerField(String.class.getDeclaredField("value")); - assertThatInvocationMatches(InstrumentedMethod.CLASS_GETFIELDS, this.stringGetFields); + assertThatInvocationDoesNotMatch(InstrumentedMethod.CLASS_GETFIELDS, this.stringGetFields); } } diff --git a/spring-core/src/main/java/org/springframework/aot/hint/predicate/ReflectionHintsPredicates.java b/spring-core/src/main/java/org/springframework/aot/hint/predicate/ReflectionHintsPredicates.java index ca71c640396e..2de9a4118289 100644 --- a/spring-core/src/main/java/org/springframework/aot/hint/predicate/ReflectionHintsPredicates.java +++ b/spring-core/src/main/java/org/springframework/aot/hint/predicate/ReflectionHintsPredicates.java @@ -243,29 +243,6 @@ public Predicate withAnyMemberCategory(MemberCategory... memberCat .anyMatch(memberCategory -> getTypeHint(hints).getMemberCategories().contains(memberCategory))); } - /** - * Refine the current predicate to only match if a hint is present for any of its constructors. - * @return the refined {@link RuntimeHints} predicate - */ - public Predicate withAnyConstructor() { - return this.and(hints -> getTypeHint(hints).constructors().findAny().isPresent()); - } - - /** - * Refine the current predicate to only match if a hint is present for any of its methods. - * @return the refined {@link RuntimeHints} predicate - */ - public Predicate withAnyMethod() { - return this.and(hints -> getTypeHint(hints).methods().findAny().isPresent()); - } - - /** - * Refine the current predicate to only match if a hint is present for any of its fields. - * @return the refined {@link RuntimeHints} predicate - */ - public Predicate withAnyField() { - return this.and(hints -> getTypeHint(hints).fields().findAny().isPresent()); - } } public abstract static class ExecutableHintPredicate implements Predicate { diff --git a/spring-core/src/test/java/org/springframework/aot/hint/predicate/ReflectionHintsPredicatesTests.java b/spring-core/src/test/java/org/springframework/aot/hint/predicate/ReflectionHintsPredicatesTests.java index e4ae6127de7c..cf6999160dd9 100644 --- a/spring-core/src/test/java/org/springframework/aot/hint/predicate/ReflectionHintsPredicatesTests.java +++ b/spring-core/src/test/java/org/springframework/aot/hint/predicate/ReflectionHintsPredicatesTests.java @@ -296,18 +296,6 @@ void privateConstructorInvocationMatchesInvokeDeclaredConstructors() { assertPredicateMatches(reflection.onConstructor(privateConstructor).invoke()); } - @Test - void reflectionOnAnyConstructorDoesNotMatchTypeReflection() { - runtimeHints.reflection().registerType(SampleClass.class); - assertPredicateDoesNotMatch(reflection.onType(SampleClass.class).withAnyConstructor()); - } - - @Test - void reflectionOnAnyConstructorMatchesConstructorReflection() { - runtimeHints.reflection().registerConstructor(publicConstructor, ExecutableMode.INVOKE); - assertPredicateMatches(reflection.onType(SampleClass.class).withAnyConstructor()); - } - } @Nested @@ -457,18 +445,6 @@ void privateMethodInvocationMatchesInvokeDeclaredMethods() { assertPredicateMatches(reflection.onMethod(SampleClass.class, "privateMethod").invoke()); } - @Test - void reflectionOnAnyMethodDoesNotMatchTypeReflection() { - runtimeHints.reflection().registerType(SampleClass.class); - assertPredicateDoesNotMatch(reflection.onType(SampleClass.class).withAnyMethod()); - } - - @Test - void reflectionOnAnyMethodMatchesMethodReflection() { - runtimeHints.reflection().registerMethod(publicMethod, ExecutableMode.INVOKE); - assertPredicateMatches(reflection.onType(SampleClass.class).withAnyMethod()); - } - } @Nested @@ -527,18 +503,6 @@ void privateFieldReflectionMatchesDeclaredFieldsHint() { assertPredicateMatches(reflection.onField(SampleClass.class, "privateField")); } - @Test - void reflectionOnAnyFieldDoesNotMatchTypeReflection() { - runtimeHints.reflection().registerType(SampleClass.class); - assertPredicateDoesNotMatch(reflection.onType(SampleClass.class).withAnyField()); - } - - @Test - void reflectionOnAnyFieldMatchesFieldReflection() { - runtimeHints.reflection().registerField(publicField); - assertPredicateMatches(reflection.onType(SampleClass.class).withAnyField()); - } - } private void assertPredicateMatches(Predicate predicate) {