Presort beans in ControllerAdviceBean.findAnnotatedBeans()

Prior to this commit, all clients of
ControllerAdviceBean.findAnnotatedBeans() sorted the returned list
manually. In addition, clients within the core Spring Framework
unnecessarily used AnnotationAwareOrderComparator instead of
OrderComparator to sort the list.

This commit presorts the ControllerAdviceBean list using OrderComparator
directly within ControllerAdviceBean.findAnnotatedBeans().

Closes gh-23188

Author: sbrannen

spring-web/src/main/java/org/springframework/web/bind/annotation/ControllerAdvice.java

@@ -34,17 +34,18 @@
  *
  * <p>Classes annotated with {@code @ControllerAdvice} can be declared explicitly
  * as Spring beans or auto-detected via classpath scanning. All such beans are
- * sorted based on {@link org.springframework.core.Ordered Ordered} /
- * {@link org.springframework.core.PriorityOrdered PriorityOrdered} semantics or
+ * sorted based on {@link org.springframework.core.Ordered Ordered} semantics or
  * {@link org.springframework.core.annotation.Order @Order} /
- * {@link javax.annotation.Priority @Priority} declarations, with {@code Ordered} /
- * {@code PriorityOrdered} semantics taking precedence over {@code @Order} /
- * {@code @Priority} declarations. {@code @ControllerAdvice} beans are then
- * applied in that order at runtime. For handling exceptions, an
- * {@code @ExceptionHandler} will be picked on the first advice with a matching
- * exception handler method. For model attributes and {@code InitBinder}
- * initialization, {@code @ModelAttribute} and {@code @InitBinder} methods will
- * also follow {@code @ControllerAdvice} order.
+ * {@link javax.annotation.Priority @Priority} declarations, with {@code Ordered}
+ * semantics taking precedence over {@code @Order} / {@code @Priority} declarations.
+ * {@code @ControllerAdvice} beans are then applied in that order at runtime.
+ * Note, however, that {@code @ControllerAdvice} beans that implement
+ * {@link org.springframework.core.PriorityOrdered PriorityOrdered} are <em>not</em>
+ * given priority over {@code @ControllerAdvice} beans that implement {@code Ordered}.
+ * For handling exceptions, an {@code @ExceptionHandler} will be picked on the
+ * first advice with a matching exception handler method. For model attributes
+ * and {@code InitBinder} initialization, {@code @ModelAttribute} and
+ * {@code @InitBinder} methods will also follow {@code @ControllerAdvice} order.
  *
  * <p>Note: For {@code @ExceptionHandler} methods, a root exception match will be
  * preferred to just matching a cause of the current exception, among the handler

spring-web/src/main/java/org/springframework/web/method/ControllerAdviceBean.java

@@ -22,6 +22,7 @@
 import org.springframework.beans.factory.BeanFactory;
 import org.springframework.beans.factory.BeanFactoryUtils;
 import org.springframework.context.ApplicationContext;
+import org.springframework.core.OrderComparator;
 import org.springframework.core.Ordered;
 import org.springframework.core.annotation.AnnotatedElementUtils;
 import org.springframework.core.annotation.OrderUtils;
@@ -215,6 +216,11 @@ public String toString() {
 	 * Find beans annotated with {@link ControllerAdvice @ControllerAdvice} in the
 	 * given {@link ApplicationContext} and wrap them as {@code ControllerAdviceBean}
 	 * instances.
+	 * <p>As of Spring Framework 5.2, the {@code ControllerAdviceBean} instances
+	 * in the returned list are sorted using {@link OrderComparator#sort(List)}.
+	 * @see #getOrder()
+	 * @see OrderComparator
+	 * @see Ordered
 	 */
 	public static List<ControllerAdviceBean> findAnnotatedBeans(ApplicationContext context) {
 		List<ControllerAdviceBean> adviceBeans = new ArrayList<>();
@@ -226,6 +232,7 @@ public static List<ControllerAdviceBean> findAnnotatedBeans(ApplicationContext c
 				adviceBeans.add(new ControllerAdviceBean(name, context, controllerAdvice));
 			}
 		}
+		OrderComparator.sort(adviceBeans);
 		return adviceBeans;
 	}
 

spring-web/src/test/java/org/springframework/web/method/ControllerAdviceBeanTests.java

@@ -18,13 +18,19 @@
 
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
+import java.util.List;
+
 import javax.annotation.Priority;
 
 import org.junit.Test;
 
 import org.springframework.beans.BeanUtils;
 import org.springframework.beans.factory.BeanFactory;
+import org.springframework.context.annotation.AnnotationConfigApplicationContext;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
 import org.springframework.core.Ordered;
+import org.springframework.core.PriorityOrdered;
 import org.springframework.core.annotation.Order;
 import org.springframework.web.bind.annotation.ControllerAdvice;
 import org.springframework.web.bind.annotation.RestController;
@@ -36,7 +42,7 @@
 import static org.mockito.Mockito.verify;
 
 /**
- * Unit tests for {@link ControllerAdviceBean}.
+ * Unit and integration tests for {@link ControllerAdviceBean}.
  *
  * @author Brian Clozel
  * @author Sam Brannen
@@ -113,14 +119,14 @@ public void orderedViaOrderedInterfaceForBeanInstance() {
 
 	@Test
 	public void orderedViaAnnotationForBeanName() {
-		assertOrder(OrderAnnotationControllerAdvice.class, 42);
-		assertOrder(PriorityAnnotationControllerAdvice.class, 42);
+		assertOrder(OrderAnnotationControllerAdvice.class, 100);
+		assertOrder(PriorityAnnotationControllerAdvice.class, 200);
 	}
 
 	@Test
 	public void orderedViaAnnotationForBeanInstance() {
-		assertOrder(new OrderAnnotationControllerAdvice(), 42);
-		assertOrder(new PriorityAnnotationControllerAdvice(), 42);
+		assertOrder(new OrderAnnotationControllerAdvice(), 100);
+		assertOrder(new PriorityAnnotationControllerAdvice(), 200);
 	}
 
 	@Test
@@ -192,6 +198,25 @@ public void multipleMatch() {
 		assertNotApplicable("should not match", bean, InheritanceController.class);
 	}
 
+	@Test
+	@SuppressWarnings({ "rawtypes", "unchecked" })
+	public void findAnnotatedBeansSortsBeans() {
+		Class[] expectedTypes = {
+			// Since ControllerAdviceBean currently treats PriorityOrdered the same as Ordered,
+			// OrderedControllerAdvice is sorted before PriorityOrderedControllerAdvice.
+			OrderedControllerAdvice.class,
+			PriorityOrderedControllerAdvice.class,
+			OrderAnnotationControllerAdvice.class,
+			PriorityAnnotationControllerAdvice.class,
+			SimpleControllerAdvice.class,
+		};
+
+		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(Config.class);
+		List<ControllerAdviceBean> adviceBeans = ControllerAdviceBean.findAnnotatedBeans(context);
+
+		assertThat(adviceBeans).extracting(ControllerAdviceBean::getBeanType).containsExactly(expectedTypes);
+	}
+
 	private void assertEqualsHashCodeAndToString(ControllerAdviceBean bean1, ControllerAdviceBean bean2, String toString) {
 		assertThat(bean1).isEqualTo(bean2);
 		assertThat(bean2).isEqualTo(bean1);
@@ -237,14 +262,17 @@ private void assertNotApplicable(String message, ControllerAdviceBean controller
 	static class SimpleControllerAdvice {}
 
 	@ControllerAdvice
-	@Order(42)
+	@Order(100)
 	static class OrderAnnotationControllerAdvice {}
 
 	@ControllerAdvice
-	@Priority(42)
+	@Priority(200)
 	static class PriorityAnnotationControllerAdvice {}
 
 	@ControllerAdvice
+	// @Order and @Priority should be ignored due to implementation of Ordered.
+	@Order(100)
+	@Priority(200)
 	static class OrderedControllerAdvice implements Ordered {
 
 		@Override
@@ -253,6 +281,18 @@ public int getOrder() {
 		}
 	}
 
+	@ControllerAdvice
+	// @Order and @Priority should be ignored due to implementation of PriorityOrdered.
+	@Order(100)
+	@Priority(200)
+	static class PriorityOrderedControllerAdvice implements PriorityOrdered {
+
+		@Override
+		public int getOrder() {
+			return 55;
+		}
+	}
+
 	@ControllerAdvice(annotations = ControllerAnnotation.class)
 	static class AnnotationSupport {}
 
@@ -294,4 +334,33 @@ static abstract class AbstractController {}
 
 	static class InheritanceController extends AbstractController {}
 
+	@Configuration(proxyBeanMethods = false)
+	static class Config {
+
+		@Bean
+		SimpleControllerAdvice simpleControllerAdvice() {
+			return new SimpleControllerAdvice();
+		}
+
+		@Bean
+		OrderAnnotationControllerAdvice orderAnnotationControllerAdvice() {
+			return new OrderAnnotationControllerAdvice();
+		}
+
+		@Bean
+		PriorityAnnotationControllerAdvice priorityAnnotationControllerAdvice() {
+			return new PriorityAnnotationControllerAdvice();
+		}
+
+		@Bean
+		OrderedControllerAdvice orderedControllerAdvice() {
+			return new OrderedControllerAdvice();
+		}
+
+		@Bean
+		PriorityOrderedControllerAdvice priorityOrderedControllerAdvice() {
+			return new PriorityOrderedControllerAdvice();
+		}
+	}
+
 }

spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ControllerMethodResolver.java

@@ -36,7 +36,6 @@
 import org.springframework.core.MethodIntrospector;
 import org.springframework.core.ReactiveAdapterRegistry;
 import org.springframework.core.annotation.AnnotatedElementUtils;
-import org.springframework.core.annotation.AnnotationAwareOrderComparator;
 import org.springframework.http.codec.HttpMessageReader;
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
@@ -223,8 +222,6 @@ private static List<HandlerMethodArgumentResolver> initResolvers(ArgumentResolve
 
 	private void initControllerAdviceCaches(ApplicationContext applicationContext) {
 		List<ControllerAdviceBean> beans = ControllerAdviceBean.findAnnotatedBeans(applicationContext);
-		AnnotationAwareOrderComparator.sort(beans);
-
 		for (ControllerAdviceBean bean : beans) {
 			Class<?> beanType = bean.getBeanType();
 			if (beanType != null) {

spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolver.java

@@ -31,7 +31,6 @@
 import org.springframework.beans.factory.InitializingBean;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.ApplicationContextAware;
-import org.springframework.core.annotation.AnnotationAwareOrderComparator;
 import org.springframework.http.HttpStatus;
 import org.springframework.http.converter.ByteArrayHttpMessageConverter;
 import org.springframework.http.converter.HttpMessageConverter;
@@ -273,8 +272,6 @@ private void initExceptionHandlerAdviceCache() {
 		}
 
 		List<ControllerAdviceBean> adviceBeans = ControllerAdviceBean.findAnnotatedBeans(getApplicationContext());
-		AnnotationAwareOrderComparator.sort(adviceBeans);
-
 		for (ControllerAdviceBean adviceBean : adviceBeans) {
 			Class<?> beanType = adviceBean.getBeanType();
 			if (beanType == null) {

spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerAdapter.java

@@ -37,7 +37,6 @@
 import org.springframework.core.ParameterNameDiscoverer;
 import org.springframework.core.ReactiveAdapterRegistry;
 import org.springframework.core.annotation.AnnotatedElementUtils;
-import org.springframework.core.annotation.AnnotationAwareOrderComparator;
 import org.springframework.core.log.LogFormatUtils;
 import org.springframework.core.task.AsyncTaskExecutor;
 import org.springframework.core.task.SimpleAsyncTaskExecutor;
@@ -578,7 +577,6 @@ private void initControllerAdviceCache() {
 		}
 
 		List<ControllerAdviceBean> adviceBeans = ControllerAdviceBean.findAnnotatedBeans(getApplicationContext());
-		AnnotationAwareOrderComparator.sort(adviceBeans);
 
 		List<Object> requestResponseBodyAdviceBeans = new ArrayList<>();
 

spring-websocket/src/main/java/org/springframework/web/socket/messaging/WebSocketAnnotationMethodMessageHandler.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2018 the original author or authors.
+ * Copyright 2002-2019 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.
@@ -20,7 +20,6 @@
 import java.util.List;
 
 import org.springframework.context.ApplicationContext;
-import org.springframework.core.annotation.AnnotationAwareOrderComparator;
 import org.springframework.lang.Nullable;
 import org.springframework.messaging.MessageChannel;
 import org.springframework.messaging.SubscribableChannel;
@@ -62,14 +61,10 @@ private void initControllerAdviceCache() {
 			logger.trace("Looking for @MessageExceptionHandler mappings: " + context);
 		}
 		List<ControllerAdviceBean> beans = ControllerAdviceBean.findAnnotatedBeans(context);
-		AnnotationAwareOrderComparator.sort(beans);
 		initMessagingAdviceCache(MessagingControllerAdviceBean.createFromList(beans));
 	}
 
-	private void initMessagingAdviceCache(@Nullable List<MessagingAdviceBean> beans) {
-		if (beans == null) {
-			return;
-		}
+	private void initMessagingAdviceCache(List<MessagingAdviceBean> beans) {
 		for (MessagingAdviceBean bean : beans) {
 			Class<?> type = bean.getBeanType();
 			if (type != null) {