Refine exception handling for type not present versus access exception

jhoeller · jhoeller · commit dd0d26b4ba39 · 2024-01-06T23:10:30.000+01:00
Includes TypeVariable bypass for reflection-free annotation retrieval. Includes info log message for annotation attribute retrieval failure. Closes gh-27182 (cherry picked from commit 70247c4)
diff --git a/spring-core/src/main/java/org/springframework/core/SerializableTypeWrapper.java b/spring-core/src/main/java/org/springframework/core/SerializableTypeWrapper.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2023 the original author or authors.
+ * Copyright 2002-2024 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -214,7 +214,12 @@ else if (Type[].class == method.getReturnType() && ObjectUtils.isEmpty(args)) {
 				return result;
 			}
 
-			return ReflectionUtils.invokeMethod(method, this.provider.getType(), args);
+			Type type = this.provider.getType();
+			if (type instanceof TypeVariable<?> tv && method.getName().equals("getName")) {
+				// Avoid reflection for common comparison of type variables
+				return tv.getName();
+			}
+			return ReflectionUtils.invokeMethod(method, type, args);
 		}
 	}
 
diff --git a/spring-core/src/main/java/org/springframework/core/annotation/AnnotationUtils.java b/spring-core/src/main/java/org/springframework/core/annotation/AnnotationUtils.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2023 the original author or authors.
+ * Copyright 2002-2024 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -757,7 +757,7 @@ public static boolean isInJavaLangAnnotationPackage(@Nullable String annotationT
 	 * Google App Engine's late arrival of {@code TypeNotPresentExceptionProxy} for
 	 * {@code Class} values (instead of early {@code Class.getAnnotations() failure}).
 	 * <p>This method not failing indicates that {@link #getAnnotationAttributes(Annotation)}
-	 * won't failure either (when attempted later on).
+	 * won't fail either (when attempted later on).
 	 * @param annotation the annotation to validate
 	 * @throws IllegalStateException if a declared {@code Class} attribute could not be read
 	 * @since 4.3.15
@@ -1056,8 +1056,7 @@ public static Object getValue(@Nullable Annotation annotation, @Nullable String
 			return null;
 		}
 		catch (Throwable ex) {
-			rethrowAnnotationConfigurationException(ex);
-			handleIntrospectionFailure(annotation.getClass(), ex);
+			handleValueRetrievalFailure(annotation, ex);
 			return null;
 		}
 	}
@@ -1073,14 +1072,18 @@ public static Object getValue(@Nullable Annotation annotation, @Nullable String
 	 * @return the value returned from the method invocation
 	 * @since 5.3.24
 	 */
-	static Object invokeAnnotationMethod(Method method, Object annotation) {
+	@Nullable
+	static Object invokeAnnotationMethod(Method method, @Nullable Object annotation) {
+		if (annotation == null) {
+			return null;
+		}
 		if (Proxy.isProxyClass(annotation.getClass())) {
 			try {
 				InvocationHandler handler = Proxy.getInvocationHandler(annotation);
 				return handler.invoke(annotation, method, null);
 			}
 			catch (Throwable ex) {
-				// ignore and fall back to reflection below
+				// Ignore and fall back to reflection below
 			}
 		}
 		return ReflectionUtils.invokeMethod(method, annotation);
@@ -1114,20 +1117,32 @@ static void rethrowAnnotationConfigurationException(Throwable ex) {
 	 * @see #rethrowAnnotationConfigurationException
 	 * @see IntrospectionFailureLogger
 	 */
-	static void handleIntrospectionFailure(@Nullable AnnotatedElement element, Throwable ex) {
+	static void handleIntrospectionFailure(AnnotatedElement element, Throwable ex) {
 		rethrowAnnotationConfigurationException(ex);
 		IntrospectionFailureLogger logger = IntrospectionFailureLogger.INFO;
 		boolean meta = false;
 		if (element instanceof Class<?> clazz && Annotation.class.isAssignableFrom(clazz)) {
-			// Meta-annotation or (default) value lookup on an annotation type
+			// Meta-annotation introspection failure
 			logger = IntrospectionFailureLogger.DEBUG;
 			meta = true;
 		}
 		if (logger.isEnabled()) {
-			String message = meta ?
-					"Failed to meta-introspect annotation " :
-					"Failed to introspect annotations on ";
-			logger.log(message + element + ": " + ex);
+			logger.log("Failed to " + (meta ? "meta-introspect annotation " : "introspect annotations on ") +
+					element + ": " + ex);
+		}
+	}
+
+	/**
+	 * Handle the supplied value retrieval exception.
+	 * @param annotation the annotation instance from which to retrieve the value
+	 * @param ex the exception that we encountered
+	 * @see #handleIntrospectionFailure
+	 */
+	private static void handleValueRetrievalFailure(Annotation annotation, Throwable ex) {
+		rethrowAnnotationConfigurationException(ex);
+		IntrospectionFailureLogger logger = IntrospectionFailureLogger.INFO;
+		if (logger.isEnabled()) {
+			logger.log("Failed to retrieve value from " + annotation + ": " + ex);
 		}
 	}
 
diff --git a/spring-core/src/main/java/org/springframework/core/annotation/AnnotationsScanner.java b/spring-core/src/main/java/org/springframework/core/annotation/AnnotationsScanner.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2023 the original author or authors.
+ * Copyright 2002-2024 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -453,7 +453,7 @@ static Annotation[] getDeclaredAnnotations(AnnotatedElement source, boolean defe
 				for (int i = 0; i < annotations.length; i++) {
 					Annotation annotation = annotations[i];
 					if (isIgnorable(annotation.annotationType()) ||
-							!AttributeMethods.forAnnotationType(annotation.annotationType()).isValid(annotation)) {
+							!AttributeMethods.forAnnotationType(annotation.annotationType()).canLoad(annotation)) {
 						annotations[i] = null;
 					}
 					else {
diff --git a/spring-core/src/main/java/org/springframework/core/annotation/AttributeMethods.java b/spring-core/src/main/java/org/springframework/core/annotation/AttributeMethods.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2023 the original author or authors.
+ * Copyright 2002-2024 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -45,7 +45,7 @@ final class AttributeMethods {
 		if (m1 != null && m2 != null) {
 			return m1.getName().compareTo(m2.getName());
 		}
-		return m1 != null ? -1 : 1;
+		return (m1 != null ? -1 : 1);
 	};
 
 
@@ -87,18 +87,26 @@ private AttributeMethods(@Nullable Class<? extends Annotation> annotationType, M
 	/**
 	 * Determine if values from the given annotation can be safely accessed without
 	 * causing any {@link TypeNotPresentException TypeNotPresentExceptions}.
+	 * <p>This method is designed to cover Google App Engine's late arrival of such
+	 * exceptions for {@code Class} values (instead of the more typical early
+	 * {@code Class.getAnnotations() failure} on a regular JVM).
 	 * @param annotation the annotation to check
 	 * @return {@code true} if all values are present
 	 * @see #validate(Annotation)
 	 */
-	boolean isValid(Annotation annotation) {
+	boolean canLoad(Annotation annotation) {
 		assertAnnotation(annotation);
 		for (int i = 0; i < size(); i++) {
 			if (canThrowTypeNotPresentException(i)) {
 				try {
 					AnnotationUtils.invokeAnnotationMethod(get(i), annotation);
 				}
+				catch (IllegalStateException ex) {
+					// Plain invocation failure to expose -> leave up to attribute retrieval
+					// (if any) where such invocation failure will be logged eventually.
+				}
 				catch (Throwable ex) {
+					// TypeNotPresentException etc. -> annotation type not actually loadable.
 					return false;
 				}
 			}
@@ -108,13 +116,13 @@ boolean isValid(Annotation annotation) {
 
 	/**
 	 * Check if values from the given annotation can be safely accessed without causing
-	 * any {@link TypeNotPresentException TypeNotPresentExceptions}. In particular,
-	 * this method is designed to cover Google App Engine's late arrival of such
+	 * any {@link TypeNotPresentException TypeNotPresentExceptions}.
+	 * <p>This method is designed to cover Google App Engine's late arrival of such
 	 * exceptions for {@code Class} values (instead of the more typical early
-	 * {@code Class.getAnnotations() failure}).
+	 * {@code Class.getAnnotations() failure} on a regular JVM).
 	 * @param annotation the annotation to validate
 	 * @throws IllegalStateException if a declared {@code Class} attribute could not be read
-	 * @see #isValid(Annotation)
+	 * @see #canLoad(Annotation)
 	 */
 	void validate(Annotation annotation) {
 		assertAnnotation(annotation);
@@ -123,6 +131,9 @@ void validate(Annotation annotation) {
 				try {
 					AnnotationUtils.invokeAnnotationMethod(get(i), annotation);
 				}
+				catch (IllegalStateException ex) {
+					throw ex;
+				}
 				catch (Throwable ex) {
 					throw new IllegalStateException("Could not obtain annotation attribute value for " +
 							get(i).getName() + " declared on " + annotation.annotationType(), ex);
@@ -147,7 +158,7 @@ private void assertAnnotation(Annotation annotation) {
 	@Nullable
 	Method get(String name) {
 		int index = indexOf(name);
-		return index != -1 ? this.attributeMethods[index] : null;
+		return (index != -1 ? this.attributeMethods[index] : null);
 	}
 
 	/**
diff --git a/spring-core/src/test/java/org/springframework/core/annotation/AnnotationIntrospectionFailureTests.java b/spring-core/src/test/java/org/springframework/core/annotation/AnnotationIntrospectionFailureTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2024 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -43,55 +43,45 @@ class AnnotationIntrospectionFailureTests {
 
 	@Test
 	void filteredTypeThrowsTypeNotPresentException() throws Exception {
-		FilteringClassLoader classLoader = new FilteringClassLoader(
-				getClass().getClassLoader());
-		Class<?> withExampleAnnotation = ClassUtils.forName(
-				WithExampleAnnotation.class.getName(), classLoader);
-		Annotation annotation = withExampleAnnotation.getAnnotations()[0];
+		FilteringClassLoader classLoader = new FilteringClassLoader(getClass().getClassLoader());
+		Class<?> withAnnotation = ClassUtils.forName(WithExampleAnnotation.class.getName(), classLoader);
+		Annotation annotation = withAnnotation.getAnnotations()[0];
 		Method method = annotation.annotationType().getMethod("value");
 		method.setAccessible(true);
-		assertThatExceptionOfType(TypeNotPresentException.class).isThrownBy(() ->
-				ReflectionUtils.invokeMethod(method, annotation))
-			.withCauseInstanceOf(ClassNotFoundException.class);
+		assertThatExceptionOfType(TypeNotPresentException.class)
+				.isThrownBy(() -> ReflectionUtils.invokeMethod(method, annotation))
+				.withCauseInstanceOf(ClassNotFoundException.class);
 	}
 
 	@Test
 	@SuppressWarnings("unchecked")
 	void filteredTypeInMetaAnnotationWhenUsingAnnotatedElementUtilsHandlesException() throws Exception {
-		FilteringClassLoader classLoader = new FilteringClassLoader(
-				getClass().getClassLoader());
-		Class<?> withExampleMetaAnnotation = ClassUtils.forName(
-				WithExampleMetaAnnotation.class.getName(), classLoader);
-		Class<Annotation> exampleAnnotationClass = (Class<Annotation>) ClassUtils.forName(
-				ExampleAnnotation.class.getName(), classLoader);
-		Class<Annotation> exampleMetaAnnotationClass = (Class<Annotation>) ClassUtils.forName(
-				ExampleMetaAnnotation.class.getName(), classLoader);
-		assertThat(AnnotatedElementUtils.getMergedAnnotationAttributes(
-				withExampleMetaAnnotation, exampleAnnotationClass)).isNull();
-		assertThat(AnnotatedElementUtils.getMergedAnnotationAttributes(
-				withExampleMetaAnnotation, exampleMetaAnnotationClass)).isNull();
-		assertThat(AnnotatedElementUtils.hasAnnotation(withExampleMetaAnnotation,
-				exampleAnnotationClass)).isFalse();
-		assertThat(AnnotatedElementUtils.hasAnnotation(withExampleMetaAnnotation,
-				exampleMetaAnnotationClass)).isFalse();
+		FilteringClassLoader classLoader = new FilteringClassLoader(getClass().getClassLoader());
+		Class<?> withAnnotation = ClassUtils.forName(WithExampleMetaAnnotation.class.getName(), classLoader);
+		Class<Annotation> annotationClass = (Class<Annotation>)
+				ClassUtils.forName(ExampleAnnotation.class.getName(), classLoader);
+		Class<Annotation> metaAnnotationClass = (Class<Annotation>)
+				ClassUtils.forName(ExampleMetaAnnotation.class.getName(), classLoader);
+		assertThat(AnnotatedElementUtils.getMergedAnnotationAttributes(withAnnotation, annotationClass)).isNull();
+		assertThat(AnnotatedElementUtils.getMergedAnnotationAttributes(withAnnotation, metaAnnotationClass)).isNull();
+		assertThat(AnnotatedElementUtils.hasAnnotation(withAnnotation, annotationClass)).isFalse();
+		assertThat(AnnotatedElementUtils.hasAnnotation(withAnnotation, metaAnnotationClass)).isFalse();
 	}
 
 	@Test
 	@SuppressWarnings("unchecked")
 	void filteredTypeInMetaAnnotationWhenUsingMergedAnnotationsHandlesException() throws Exception {
-		FilteringClassLoader classLoader = new FilteringClassLoader(
-				getClass().getClassLoader());
-		Class<?> withExampleMetaAnnotation = ClassUtils.forName(
-				WithExampleMetaAnnotation.class.getName(), classLoader);
-		Class<Annotation> exampleAnnotationClass = (Class<Annotation>) ClassUtils.forName(
-				ExampleAnnotation.class.getName(), classLoader);
-		Class<Annotation> exampleMetaAnnotationClass = (Class<Annotation>) ClassUtils.forName(
-				ExampleMetaAnnotation.class.getName(), classLoader);
-		MergedAnnotations annotations = MergedAnnotations.from(withExampleMetaAnnotation);
-		assertThat(annotations.get(exampleAnnotationClass).isPresent()).isFalse();
-		assertThat(annotations.get(exampleMetaAnnotationClass).isPresent()).isFalse();
-		assertThat(annotations.isPresent(exampleMetaAnnotationClass)).isFalse();
-		assertThat(annotations.isPresent(exampleAnnotationClass)).isFalse();
+		FilteringClassLoader classLoader = new FilteringClassLoader(getClass().getClassLoader());
+		Class<?> withAnnotation = ClassUtils.forName(WithExampleMetaAnnotation.class.getName(), classLoader);
+		Class<Annotation> annotationClass = (Class<Annotation>)
+				ClassUtils.forName(ExampleAnnotation.class.getName(), classLoader);
+		Class<Annotation> metaAnnotationClass = (Class<Annotation>)
+				ClassUtils.forName(ExampleMetaAnnotation.class.getName(), classLoader);
+		MergedAnnotations annotations = MergedAnnotations.from(withAnnotation);
+		assertThat(annotations.get(annotationClass).isPresent()).isFalse();
+		assertThat(annotations.get(metaAnnotationClass).isPresent()).isFalse();
+		assertThat(annotations.isPresent(metaAnnotationClass)).isFalse();
+		assertThat(annotations.isPresent(annotationClass)).isFalse();
 	}
 
 
@@ -103,17 +93,16 @@ static class FilteringClassLoader extends OverridingClassLoader {
 
 		@Override
 		protected boolean isEligibleForOverriding(String className) {
-			return className.startsWith(
-					AnnotationIntrospectionFailureTests.class.getName());
+			return className.startsWith(AnnotationIntrospectionFailureTests.class.getName()) ||
+					className.startsWith("jdk.internal");
 		}
 
 		@Override
-		protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundException {
-			if (name.startsWith(AnnotationIntrospectionFailureTests.class.getName()) &&
-					name.contains("Filtered")) {
+		protected Class<?> loadClassForOverriding(String name) throws ClassNotFoundException {
+			if (name.contains("Filtered") || name.startsWith("jdk.internal")) {
 				throw new ClassNotFoundException(name);
 			}
-			return super.loadClass(name, resolve);
+			return super.loadClassForOverriding(name);
 		}
 	}
 
diff --git a/spring-core/src/test/java/org/springframework/core/annotation/AttributeMethodsTests.java b/spring-core/src/test/java/org/springframework/core/annotation/AttributeMethodsTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2023 the original author or authors.
+ * Copyright 2002-2024 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -112,7 +112,7 @@ void isValidWhenHasTypeNotPresentExceptionReturnsFalse() {
 		ClassValue annotation = mockAnnotation(ClassValue.class);
 		given(annotation.value()).willThrow(TypeNotPresentException.class);
 		AttributeMethods attributes = AttributeMethods.forAnnotationType(annotation.annotationType());
-		assertThat(attributes.isValid(annotation)).isFalse();
+		assertThat(attributes.canLoad(annotation)).isFalse();
 	}
 
 	@Test
@@ -121,7 +121,7 @@ void isValidWhenDoesNotHaveTypeNotPresentExceptionReturnsTrue() {
 		ClassValue annotation = mock();
 		given(annotation.value()).willReturn((Class) InputStream.class);
 		AttributeMethods attributes = AttributeMethods.forAnnotationType(annotation.annotationType());
-		assertThat(attributes.isValid(annotation)).isTrue();
+		assertThat(attributes.canLoad(annotation)).isTrue();
 	}
 
 	@Test
diff --git a/spring-core/src/test/java/org/springframework/core/annotation/MergedAnnotationsTests.java b/spring-core/src/test/java/org/springframework/core/annotation/MergedAnnotationsTests.java

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,5 @@`
`1`	`1`	`/*`
`2`		`- * Copyright 2002-2023 the original author or authors.`
	`2`	`+ * Copyright 2002-2024 the original author or authors.`
`3`	`3`	`*`
`4`	`4`	`* Licensed under the Apache License, Version 2.0 (the "License");`
`5`	`5`	`* you may not use this file except in compliance with the License.`
`@@ -214,7 +214,12 @@ else if (Type[].class == method.getReturnType() && ObjectUtils.isEmpty(args)) {`
`214`	`214`	`return result;`
`215`	`215`	`}`
`216`	`216`
`217`		`- return ReflectionUtils.invokeMethod(method, this.provider.getType(), args);`
	`217`	`+ Type type = this.provider.getType();`
	`218`	`+ if (type instanceof TypeVariable<?> tv && method.getName().equals("getName")) {`
	`219`	`+ // Avoid reflection for common comparison of type variables`
	`220`	`+ return tv.getName();`
	`221`	`+ }`
	`222`	`+ return ReflectionUtils.invokeMethod(method, type, args);`
`218`	`223`	`}`
`219`	`224`	`}`
`220`	`225`