From d0231cb29a4c51ed2cab07238c72ebcebe07523d Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Mon, 24 Jun 2019 14:48:18 +0300 Subject: [PATCH] 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 --- .../web/bind/annotation/ControllerAdvice.java | 21 ++--- .../web/method/ControllerAdviceBean.java | 7 ++ .../web/method/ControllerAdviceBeanTests.java | 83 +++++++++++++++++-- .../annotation/ControllerMethodResolver.java | 3 - .../ExceptionHandlerExceptionResolver.java | 3 - .../RequestMappingHandlerAdapter.java | 2 - ...bSocketAnnotationMethodMessageHandler.java | 9 +- 7 files changed, 96 insertions(+), 32 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/bind/annotation/ControllerAdvice.java b/spring-web/src/main/java/org/springframework/web/bind/annotation/ControllerAdvice.java index 5de9100019..47ebcd6314 100644 --- a/spring-web/src/main/java/org/springframework/web/bind/annotation/ControllerAdvice.java +++ b/spring-web/src/main/java/org/springframework/web/bind/annotation/ControllerAdvice.java @@ -34,17 +34,18 @@ import org.springframework.stereotype.Component; * *

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 not + * 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. * *

Note: For {@code @ExceptionHandler} methods, a root exception match will be * preferred to just matching a cause of the current exception, among the handler diff --git a/spring-web/src/main/java/org/springframework/web/method/ControllerAdviceBean.java b/spring-web/src/main/java/org/springframework/web/method/ControllerAdviceBean.java index 0c54d039fa..6f7349c9b3 100644 --- a/spring-web/src/main/java/org/springframework/web/method/ControllerAdviceBean.java +++ b/spring-web/src/main/java/org/springframework/web/method/ControllerAdviceBean.java @@ -22,6 +22,7 @@ import java.util.List; 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 class ControllerAdviceBean implements Ordered { * Find beans annotated with {@link ControllerAdvice @ControllerAdvice} in the * given {@link ApplicationContext} and wrap them as {@code ControllerAdviceBean} * instances. + *

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 findAnnotatedBeans(ApplicationContext context) { List adviceBeans = new ArrayList<>(); @@ -226,6 +232,7 @@ public class ControllerAdviceBean implements Ordered { adviceBeans.add(new ControllerAdviceBean(name, context, controllerAdvice)); } } + OrderComparator.sort(adviceBeans); return adviceBeans; } diff --git a/spring-web/src/test/java/org/springframework/web/method/ControllerAdviceBeanTests.java b/spring-web/src/test/java/org/springframework/web/method/ControllerAdviceBeanTests.java index 4236c8256d..d1bd28a9a0 100644 --- a/spring-web/src/test/java/org/springframework/web/method/ControllerAdviceBeanTests.java +++ b/spring-web/src/test/java/org/springframework/web/method/ControllerAdviceBeanTests.java @@ -18,13 +18,19 @@ package org.springframework.web.method; 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.mock; 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 class ControllerAdviceBeanTests { @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 class ControllerAdviceBeanTests { 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 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 @@ public class ControllerAdviceBeanTests { 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 class ControllerAdviceBeanTests { } } + @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 @@ public class ControllerAdviceBeanTests { 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(); + } + } + } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ControllerMethodResolver.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ControllerMethodResolver.java index 0c5c30a915..961b922074 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ControllerMethodResolver.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ControllerMethodResolver.java @@ -36,7 +36,6 @@ import org.springframework.core.KotlinDetector; 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 @@ class ControllerMethodResolver { private void initControllerAdviceCaches(ApplicationContext applicationContext) { List beans = ControllerAdviceBean.findAnnotatedBeans(applicationContext); - AnnotationAwareOrderComparator.sort(beans); - for (ControllerAdviceBean bean : beans) { Class beanType = bean.getBeanType(); if (beanType != null) { diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolver.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolver.java index 74d29db10a..f3c403e7bb 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolver.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolver.java @@ -31,7 +31,6 @@ import org.springframework.aop.support.AopUtils; 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 @@ public class ExceptionHandlerExceptionResolver extends AbstractHandlerMethodExce } List adviceBeans = ControllerAdviceBean.findAnnotatedBeans(getApplicationContext()); - AnnotationAwareOrderComparator.sort(adviceBeans); - for (ControllerAdviceBean adviceBean : adviceBeans) { Class beanType = adviceBean.getBeanType(); if (beanType == null) { diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerAdapter.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerAdapter.java index 4bb3490261..0b1efe7d28 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerAdapter.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerAdapter.java @@ -37,7 +37,6 @@ import org.springframework.core.MethodIntrospector; 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 @@ public class RequestMappingHandlerAdapter extends AbstractHandlerMethodAdapter } List adviceBeans = ControllerAdviceBean.findAnnotatedBeans(getApplicationContext()); - AnnotationAwareOrderComparator.sort(adviceBeans); List requestResponseBodyAdviceBeans = new ArrayList<>(); diff --git a/spring-websocket/src/main/java/org/springframework/web/socket/messaging/WebSocketAnnotationMethodMessageHandler.java b/spring-websocket/src/main/java/org/springframework/web/socket/messaging/WebSocketAnnotationMethodMessageHandler.java index bb27b60fce..847741b2c1 100644 --- a/spring-websocket/src/main/java/org/springframework/web/socket/messaging/WebSocketAnnotationMethodMessageHandler.java +++ b/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.ArrayList; 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 @@ public class WebSocketAnnotationMethodMessageHandler extends SimpAnnotationMetho logger.trace("Looking for @MessageExceptionHandler mappings: " + context); } List beans = ControllerAdviceBean.findAnnotatedBeans(context); - AnnotationAwareOrderComparator.sort(beans); initMessagingAdviceCache(MessagingControllerAdviceBean.createFromList(beans)); } - private void initMessagingAdviceCache(@Nullable List beans) { - if (beans == null) { - return; - } + private void initMessagingAdviceCache(List beans) { for (MessagingAdviceBean bean : beans) { Class type = bean.getBeanType(); if (type != null) {