From c4a8bf9c4dd745183d918c05f63752a5a7d317a0 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Fri, 11 Oct 2013 19:28:28 +0200 Subject: [PATCH 1/2] Add new features on @ControllerAdvice Prior to this commit, @ControllerAdvice annotated beans would assist all known Controllers, by applying @ExceptionHandler, @InitBinder, and @ModelAttribute. This commit updates the @ControllerAdvice annotation, which accepts now base package names, assignableTypes, annotations and basePackageClasses. If attributes are set, only Controllers that match those selectors will be assisted by the annotated class. This commit does not change the default behavior when no value is set, i.e. @ControllerAdvice(). Issue: SPR-10222 --- .../web/bind/annotation/ControllerAdvice.java | 68 ++++++++- .../web/method/ControllerAdviceBean.java | 113 +++++++++++++- .../web/method/ControllerAdviceBeanTests.java | 144 ++++++++++++++++++ .../ExceptionHandlerExceptionResolver.java | 12 +- .../RequestMappingHandlerAdapter.java | 16 +- ...xceptionHandlerExceptionResolverTests.java | 78 +++++++++- .../RequestMappingHandlerAdapterTests.java | 34 +++++ 7 files changed, 450 insertions(+), 15 deletions(-) create mode 100644 spring-web/src/test/java/org/springframework/web/method/ControllerAdviceBeanTests.java 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 6201383ffb0..b65bcef1aa3 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2013 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. @@ -34,7 +34,21 @@ import org.springframework.stereotype.Component; * {@link InitBinder @InitBinder}, and {@link ModelAttribute @ModelAttribute} * methods that apply to all {@link RequestMapping @RequestMapping} methods. * + *

One of {@link #annotations()}, {@link #basePackageClasses()}, + * {@link #basePackages()} or its alias {@link #value()} + * may be specified to define specific subsets of Controllers + * to assist. When multiple selectors are applied, OR logic is applied - + * meaning selected Controllers should match at least one selector. + * + *

The default behavior (i.e. if used without any selector), + * the {@code @ControllerAdvice} annotated class will + * assist all known Controllers. + * + *

Note that those checks are done at runtime, so adding many attributes and using + * multiple strategies may have negative impacts (complexity, performance). + * * @author Rossen Stoyanchev + * @author Brian Clozel * @since 3.2 */ @Target(ElementType.TYPE) @@ -43,4 +57,56 @@ import org.springframework.stereotype.Component; @Component public @interface ControllerAdvice { + /** + * Alias for the {@link #basePackages()} attribute. + * Allows for more concise annotation declarations e.g.: + * {@code @ControllerAdvice("org.my.pkg")} instead of + * {@code @ControllerAdvice(basePackages="org.my.pkg")}. + * @since 4.0 + */ + String[] value() default {}; + + /** + * Array of base packages. + * Controllers that belong to those base packages will be selected + * to be assisted by the annotated class, e.g.: + * {@code @ControllerAdvice(basePackages="org.my.pkg")} + * {@code @ControllerAdvice(basePackages={"org.my.pkg","org.my.other.pkg"})} + * + *

{@link #value()} is an alias for this attribute. + *

Use {@link #basePackageClasses()} for a type-safe alternative to String-based package names. + * @since 4.0 + */ + String[] basePackages() default {}; + + /** + * Array of classes. + * Controllers that are assignable to at least one of the given types + * will be assisted by the {@code @ControllerAdvice} annotated class. + * @since 4.0 + */ + Class[] assignableTypes() default {}; + + + /** + * Array of annotations. + * Controllers that are annotated with this/one of those annotation(s) + * will be assisted by the {@code @ControllerAdvice} annotated class. + * + *

Consider creating a special annotation or use a predefined one, + * like {@link RestController @RestController}. + * @since 4.0 + */ + Class[] annotations() default {}; + + /** + * Type-safe alternative to {@link #value()} for specifying the packages + * to select Controllers to be assisted by the {@code @ControllerAdvice} + * annotated class. + * + *

Consider creating a special no-op marker class or interface in each package + * that serves no purpose other than being referenced by this attribute. + * @since 4.0 + */ + Class[] basePackageClasses() default {}; } 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 eeefa75044f..7724796bd2d 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2013 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. @@ -15,9 +15,13 @@ */ package org.springframework.web.method; +import java.lang.annotation.Annotation; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.springframework.beans.factory.BeanFactory; import org.springframework.context.ApplicationContext; import org.springframework.core.Ordered; @@ -25,6 +29,7 @@ import org.springframework.core.annotation.AnnotationUtils; import org.springframework.core.annotation.Order; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; +import org.springframework.util.StringUtils; import org.springframework.web.bind.annotation.ControllerAdvice; /** @@ -36,6 +41,7 @@ import org.springframework.web.bind.annotation.ControllerAdvice; * any object, including ones without an {@code @ControllerAdvice}. * * @author Rossen Stoyanchev + * @author Brian Clozel * @since 3.2 */ public class ControllerAdviceBean implements Ordered { @@ -46,6 +52,13 @@ public class ControllerAdviceBean implements Ordered { private final BeanFactory beanFactory; + private final List basePackages = new ArrayList(); + + private final List> annotations = new ArrayList>(); + + private final List> assignableTypes = new ArrayList>(); + + private static final Log logger = LogFactory.getLog(ControllerAdviceBean.class); /** * Create an instance using the given bean name. @@ -59,7 +72,11 @@ public class ControllerAdviceBean implements Ordered { "Bean factory [" + beanFactory + "] does not contain bean " + "with name [" + beanName + "]"); this.bean = beanName; this.beanFactory = beanFactory; - this.order = initOrderFromBeanType(this.beanFactory.getType(beanName)); + Class beanType = this.beanFactory.getType(beanName); + this.order = initOrderFromBeanType(beanType); + this.basePackages.addAll(initBasePackagesFromBeanType(beanType)); + this.annotations.addAll(initAnnotationsFromBeanType(beanType)); + this.assignableTypes.addAll(initAssignableTypesFromBeanType(beanType)); } private static int initOrderFromBeanType(Class beanType) { @@ -67,6 +84,56 @@ public class ControllerAdviceBean implements Ordered { return (annot != null) ? annot.value() : Ordered.LOWEST_PRECEDENCE; } + private static List initBasePackagesFromBeanType(Class beanType) { + List basePackages = new ArrayList(); + ControllerAdvice annotation = AnnotationUtils.findAnnotation(beanType,ControllerAdvice.class); + Assert.notNull(annotation,"BeanType ["+beanType.getName()+"] is not annotated @ControllerAdvice"); + for (String pkgName : (String[])AnnotationUtils.getValue(annotation)) { + if (StringUtils.hasText(pkgName)) { + Package pack = Package.getPackage(pkgName); + if(pack != null) { + basePackages.add(pack); + } else { + logger.warn("Package [" + pkgName + "] was not found, see [" + + beanType.getName() + "]"); + } + } + } + for (String pkgName : (String[])AnnotationUtils.getValue(annotation,"basePackages")) { + if (StringUtils.hasText(pkgName)) { + Package pack = Package.getPackage(pkgName); + if(pack != null) { + basePackages.add(pack); + } else { + logger.warn("Package [" + pkgName + "] was not found, see [" + + beanType.getName() + "]"); + } + } + } + for (Class markerClass : (Class[])AnnotationUtils.getValue(annotation,"basePackageClasses")) { + Package pack = markerClass.getPackage(); + if(pack != null) { + basePackages.add(pack); + } else { + logger.warn("Package was not found for class [" + markerClass.getName() + + "], see [" + beanType.getName() + "]"); + } + } + return basePackages; + } + + private static List> initAnnotationsFromBeanType(Class beanType) { + ControllerAdvice annotation = AnnotationUtils.findAnnotation(beanType,ControllerAdvice.class); + Class[] annotations = (Class[])AnnotationUtils.getValue(annotation,"annotations"); + return Arrays.asList(annotations); + } + + private static List> initAssignableTypesFromBeanType(Class beanType) { + ControllerAdvice annotation = AnnotationUtils.findAnnotation(beanType,ControllerAdvice.class); + Class[] assignableTypes = (Class[])AnnotationUtils.getValue(annotation,"assignableTypes"); + return Arrays.asList(assignableTypes); + } + /** * Create an instance using the given bean instance. * @param bean the bean @@ -75,6 +142,9 @@ public class ControllerAdviceBean implements Ordered { Assert.notNull(bean, "'bean' must not be null"); this.bean = bean; this.order = initOrderFromBean(bean); + this.basePackages.addAll(initBasePackagesFromBeanType(bean.getClass())); + this.annotations.addAll(initAnnotationsFromBeanType(bean.getClass())); + this.assignableTypes.addAll(initAssignableTypesFromBeanType(bean.getClass())); this.beanFactory = null; } @@ -124,6 +194,45 @@ public class ControllerAdviceBean implements Ordered { return (this.bean instanceof String) ? this.beanFactory.getBean((String) this.bean) : this.bean; } + /** + * Checks whether the bean type given as a parameter should be assisted by + * the current {@code @ControllerAdvice} annotated bean. + * + * @param beanType the type of the bean + * @see org.springframework.web.bind.annotation.ControllerAdvice + * @since 4.0 + */ + public boolean isApplicableToBeanType(Class beanType) { + if(hasNoSelector()) { + return true; + } + else if(beanType != null) { + String packageName = beanType.getPackage().getName(); + for(Package basePackage : this.basePackages) { + if(packageName.startsWith(basePackage.getName())) { + return true; + } + } + for(Class annotationClass : this.annotations) { + if(AnnotationUtils.findAnnotation(beanType, annotationClass) != null) { + return true; + } + } + for(Class clazz : this.assignableTypes) { + if(ClassUtils.isAssignable(clazz, beanType)) { + return true; + } + } + } + return false; + } + + private boolean hasNoSelector() { + return this.basePackages.isEmpty() && this.annotations.isEmpty() + && this.assignableTypes.isEmpty(); + } + + @Override public boolean equals(Object o) { if (this == o) { 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 new file mode 100644 index 00000000000..918b146da66 --- /dev/null +++ b/spring-web/src/test/java/org/springframework/web/method/ControllerAdviceBeanTests.java @@ -0,0 +1,144 @@ +package org.springframework.web.method; + +import org.junit.Test; +import org.springframework.web.bind.annotation.ControllerAdvice; +import org.springframework.web.bind.annotation.RestController; + +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; + +import static org.junit.Assert.*; + +/** + * @author Brian Clozel + */ +public class ControllerAdviceBeanTests { + + @Test + public void shouldMatchAll() { + ControllerAdviceBean bean = new ControllerAdviceBean(new SimpleControllerAdvice()); + assertApplicable("should match all", bean, AnnotatedController.class); + assertApplicable("should match all", bean, ImplementationController.class); + assertApplicable("should match all", bean, InheritanceController.class); + assertApplicable("should match all", bean, String.class); + } + + @Test + public void basePackageSupport() { + ControllerAdviceBean bean = new ControllerAdviceBean(new BasePackageSupport()); + assertApplicable("base package support", bean, AnnotatedController.class); + assertApplicable("base package support", bean, ImplementationController.class); + assertApplicable("base package support", bean, InheritanceController.class); + assertNotApplicable("bean not in package", bean, String.class); + } + + @Test + public void basePackageValueSupport() { + ControllerAdviceBean bean = new ControllerAdviceBean(new BasePackageValueSupport()); + assertApplicable("base package support", bean, AnnotatedController.class); + assertApplicable("base package support", bean, ImplementationController.class); + assertApplicable("base package support", bean, InheritanceController.class); + assertNotApplicable("bean not in package", bean, String.class); + } + + @Test + public void annotationSupport() { + ControllerAdviceBean bean = new ControllerAdviceBean(new AnnotationSupport()); + assertApplicable("annotation support", bean, AnnotatedController.class); + assertNotApplicable("this bean is not annotated", bean, InheritanceController.class); + } + + @Test + public void markerClassSupport() { + ControllerAdviceBean bean = new ControllerAdviceBean(new MarkerClassSupport()); + assertApplicable("base package class support", bean, AnnotatedController.class); + assertApplicable("base package class support", bean, ImplementationController.class); + assertApplicable("base package class support", bean, InheritanceController.class); + assertNotApplicable("bean not in package", bean, String.class); + } + + @Test + public void shouldNotMatch() { + ControllerAdviceBean bean = new ControllerAdviceBean(new ShouldNotMatch()); + assertNotApplicable("should not match", bean, AnnotatedController.class); + assertNotApplicable("should not match", bean, ImplementationController.class); + assertNotApplicable("should not match", bean, InheritanceController.class); + assertNotApplicable("should not match", bean, String.class); + } + + @Test + public void assignableTypesSupport() { + ControllerAdviceBean bean = new ControllerAdviceBean(new AssignableTypesSupport()); + assertApplicable("controller implements assignable", bean, ImplementationController.class); + assertApplicable("controller inherits assignable", bean, InheritanceController.class); + assertNotApplicable("not assignable", bean, AnnotatedController.class); + assertNotApplicable("not assignable", bean, String.class); + } + + @Test + public void multipleMatch() { + ControllerAdviceBean bean = new ControllerAdviceBean(new MultipleSelectorsSupport()); + assertApplicable("controller implements assignable", bean, ImplementationController.class); + assertApplicable("controller is annotated", bean, AnnotatedController.class); + assertNotApplicable("should not match", bean, InheritanceController.class); + } + + private void assertApplicable(String message, ControllerAdviceBean controllerAdvice, + Class controllerBeanType) { + assertNotNull(controllerAdvice); + assertTrue(message,controllerAdvice.isApplicableToBeanType(controllerBeanType)); + } + + private void assertNotApplicable(String message, ControllerAdviceBean controllerAdvice, + Class controllerBeanType) { + assertNotNull(controllerAdvice); + assertFalse(message,controllerAdvice.isApplicableToBeanType(controllerBeanType)); + } + + // ControllerAdvice classes + + @ControllerAdvice + static class SimpleControllerAdvice {} + + @ControllerAdvice(annotations = ControllerAnnotation.class) + static class AnnotationSupport {} + + @ControllerAdvice(basePackageClasses = MarkerClass.class) + static class MarkerClassSupport {} + + @ControllerAdvice(assignableTypes = {ControllerInterface.class, + AbstractController.class}) + static class AssignableTypesSupport {} + + @ControllerAdvice(basePackages = "org.springframework.web.method") + static class BasePackageSupport {} + + @ControllerAdvice("org.springframework.web.method") + static class BasePackageValueSupport {} + + @ControllerAdvice(annotations = ControllerAnnotation.class, + assignableTypes = ControllerInterface.class) + static class MultipleSelectorsSupport {} + + @ControllerAdvice(basePackages = "java.util", + annotations = RestController.class) + static class ShouldNotMatch {} + + // Support classes + + static class MarkerClass {} + + @Retention(RetentionPolicy.RUNTIME) + static @interface ControllerAnnotation {} + + @ControllerAnnotation + public static class AnnotatedController {} + + static interface ControllerInterface {} + + static class ImplementationController implements ControllerInterface {} + + static abstract class AbstractController {} + + static class InheritanceController extends AbstractController {} +} 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 8a62923a429..2dc94d55c93 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2013 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. @@ -351,8 +351,8 @@ public class ExceptionHandlerExceptionResolver extends AbstractHandlerMethodExce * @return a method to handle the exception, or {@code null} */ protected ServletInvocableHandlerMethod getExceptionHandlerMethod(HandlerMethod handlerMethod, Exception exception) { + Class handlerType = (handlerMethod != null) ? handlerMethod.getBeanType() : null; if (handlerMethod != null) { - Class handlerType = handlerMethod.getBeanType(); ExceptionHandlerMethodResolver resolver = this.exceptionHandlerCache.get(handlerType); if (resolver == null) { resolver = new ExceptionHandlerMethodResolver(handlerType); @@ -364,9 +364,11 @@ public class ExceptionHandlerExceptionResolver extends AbstractHandlerMethodExce } } for (Entry entry : this.exceptionHandlerAdviceCache.entrySet()) { - Method method = entry.getValue().resolveMethod(exception); - if (method != null) { - return new ServletInvocableHandlerMethod(entry.getKey().resolveBean(), method); + if(entry.getKey().isApplicableToBeanType(handlerType)) { + Method method = entry.getValue().resolveMethod(exception); + if (method != null) { + return new ServletInvocableHandlerMethod(entry.getKey().resolveBean(), method); + } } } return 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 4d2de70c265..bf5a17505e9 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 @@ -776,9 +776,11 @@ public class RequestMappingHandlerAdapter extends AbstractHandlerMethodAdapter i List attrMethods = new ArrayList(); // Global methods first for (Entry> entry : this.modelAttributeAdviceCache.entrySet()) { - Object bean = entry.getKey().resolveBean(); - for (Method method : entry.getValue()) { - attrMethods.add(createModelAttributeMethod(binderFactory, bean, method)); + if(entry.getKey().isApplicableToBeanType(handlerType)) { + Object bean = entry.getKey().resolveBean(); + for (Method method : entry.getValue()) { + attrMethods.add(createModelAttributeMethod(binderFactory, bean, method)); + } } } for (Method method : methods) { @@ -806,9 +808,11 @@ public class RequestMappingHandlerAdapter extends AbstractHandlerMethodAdapter i List initBinderMethods = new ArrayList(); // Global methods first for (Entry> entry : this.initBinderAdviceCache .entrySet()) { - Object bean = entry.getKey().resolveBean(); - for (Method method : entry.getValue()) { - initBinderMethods.add(createInitBinderMethod(bean, method)); + if(entry.getKey().isApplicableToBeanType(handlerType)) { + Object bean = entry.getKey().resolveBean(); + for (Method method : entry.getValue()) { + initBinderMethods.add(createInitBinderMethod(bean, method)); + } } } for (Method method : methods) { diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolverTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolverTests.java index 4c93cac51fc..4fab4071029 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolverTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolverTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2013 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. @@ -204,6 +204,35 @@ public class ExceptionHandlerExceptionResolverTests { assertEquals("TestExceptionResolver: IllegalStateException", this.response.getContentAsString()); } + @Test + public void resolveExceptionControllerAdviceHandler() throws Exception { + AnnotationConfigApplicationContext cxt = new AnnotationConfigApplicationContext(MyControllerAdviceConfig.class); + this.resolver.setApplicationContext(cxt); + this.resolver.afterPropertiesSet(); + + IllegalStateException ex = new IllegalStateException(); + HandlerMethod handlerMethod = new HandlerMethod(new ResponseBodyController(), "handle"); + ModelAndView mav = this.resolver.resolveException(this.request, this.response, handlerMethod, ex); + + assertNotNull("Exception was not handled", mav); + assertTrue(mav.isEmpty()); + assertEquals("BasePackageTestExceptionResolver: IllegalStateException", this.response.getContentAsString()); + } + + @Test + public void resolveExceptionControllerAdviceNoHandler() throws Exception { + AnnotationConfigApplicationContext cxt = new AnnotationConfigApplicationContext(MyControllerAdviceConfig.class); + this.resolver.setApplicationContext(cxt); + this.resolver.afterPropertiesSet(); + + IllegalStateException ex = new IllegalStateException(); + ModelAndView mav = this.resolver.resolveException(this.request, this.response, null, ex); + + assertNotNull("Exception was not handled", mav); + assertTrue(mav.isEmpty()); + assertEquals("DefaultTestExceptionResolver: IllegalStateException", this.response.getContentAsString()); + } + private void assertMethodProcessorCount(int resolverCount, int handlerCount) { assertEquals(resolverCount, this.resolver.getArgumentResolvers().getResolvers().size()); @@ -288,4 +317,51 @@ public class ExceptionHandlerExceptionResolverTests { } } + @ControllerAdvice("java.lang") + @Order(1) + static class NotCalledTestExceptionResolver { + + @ExceptionHandler + @ResponseBody + public String handleException(IllegalStateException ex) { + return "NotCalledTestExceptionResolver: " + ClassUtils.getShortName(ex.getClass()); + } + } + + @ControllerAdvice("org.springframework.web.servlet.mvc.method.annotation") + @Order(2) + static class BasePackageTestExceptionResolver { + + @ExceptionHandler + @ResponseBody + public String handleException(IllegalStateException ex) { + return "BasePackageTestExceptionResolver: " + ClassUtils.getShortName(ex.getClass()); + } + } + + @ControllerAdvice + @Order(3) + static class DefaultTestExceptionResolver { + + @ExceptionHandler + @ResponseBody + public String handleException(IllegalStateException ex) { + return "DefaultTestExceptionResolver: " + ClassUtils.getShortName(ex.getClass()); + } + } + + @Configuration + static class MyControllerAdviceConfig { + @Bean public NotCalledTestExceptionResolver notCalledTestExceptionResolver() { + return new NotCalledTestExceptionResolver(); + } + + @Bean public BasePackageTestExceptionResolver basePackageTestExceptionResolver() { + return new BasePackageTestExceptionResolver(); + } + + @Bean public DefaultTestExceptionResolver defaultTestExceptionResolver() { + return new DefaultTestExceptionResolver(); + } + } } \ No newline at end of file diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerAdapterTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerAdapterTests.java index d08c7956021..a5acd05a691 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerAdapterTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerAdapterTests.java @@ -25,7 +25,9 @@ import org.junit.Test; import org.springframework.mock.web.test.MockHttpServletRequest; import org.springframework.mock.web.test.MockHttpServletResponse; import org.springframework.ui.Model; +import org.springframework.web.bind.WebDataBinder; import org.springframework.web.bind.annotation.ControllerAdvice; +import org.springframework.web.bind.annotation.InitBinder; import org.springframework.web.bind.annotation.ModelAttribute; import org.springframework.web.bind.annotation.SessionAttributes; import org.springframework.web.context.support.StaticWebApplicationContext; @@ -184,6 +186,21 @@ public class RequestMappingHandlerAdapterTests { assertEquals("gAttr2", mav.getModel().get("attr2")); } + @Test + public void modelAttributePackageNameAdvice() throws Exception { + this.webAppContext.registerSingleton("mapa", ModelAttributePackageAdvice.class); + this.webAppContext.registerSingleton("manupa", ModelAttributeNotUsedPackageAdvice.class); + this.webAppContext.refresh(); + + HandlerMethod handlerMethod = handlerMethod(new SimpleController(), "handle"); + this.handlerAdapter.afterPropertiesSet(); + ModelAndView mav = this.handlerAdapter.handle(this.request, this.response, handlerMethod); + + assertEquals("lAttr1", mav.getModel().get("attr1")); + assertEquals("gAttr2", mav.getModel().get("attr2")); + assertEquals(null,mav.getModel().get("attr3")); + } + private HandlerMethod handlerMethod(Object handler, String methodName, Class... paramTypes) throws Exception { Method method = handler.getClass().getDeclaredMethod(methodName, paramTypes); @@ -237,4 +254,21 @@ public class RequestMappingHandlerAdapterTests { } } + @ControllerAdvice({"org.springframework.web.servlet.mvc.method.annotation","java.lang"}) + private static class ModelAttributePackageAdvice { + + @ModelAttribute + public void addAttributes(Model model) { + model.addAttribute("attr2", "gAttr2"); + } + } + + @ControllerAdvice("java.lang") + private static class ModelAttributeNotUsedPackageAdvice { + + @ModelAttribute + public void addAttributes(Model model) { + model.addAttribute("attr3", "gAttr3"); + } + } } \ No newline at end of file From 4f28c77db7875cec6a4bc6a3beae98308ba6dd01 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Fri, 18 Oct 2013 12:24:55 -0400 Subject: [PATCH 2/2] Polish ControllerAdvice selectors Issue: SPR-10222 --- .../web/bind/annotation/ControllerAdvice.java | 39 +++--- .../web/method/ControllerAdviceBean.java | 114 ++++++++---------- .../web/method/ControllerAdviceBeanTests.java | 12 +- 3 files changed, 80 insertions(+), 85 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 b65bcef1aa3..e5bac276c21 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 @@ -16,6 +16,7 @@ package org.springframework.web.bind.annotation; +import java.lang.annotation.Annotation; import java.lang.annotation.Documented; import java.lang.annotation.ElementType; import java.lang.annotation.Retention; @@ -60,29 +61,44 @@ public @interface ControllerAdvice { /** * Alias for the {@link #basePackages()} attribute. * Allows for more concise annotation declarations e.g.: - * {@code @ControllerAdvice("org.my.pkg")} instead of + * {@code @ControllerAdvice("org.my.pkg")} is equivalent to * {@code @ControllerAdvice(basePackages="org.my.pkg")}. + * * @since 4.0 */ String[] value() default {}; /** * Array of base packages. - * Controllers that belong to those base packages will be selected - * to be assisted by the annotated class, e.g.: - * {@code @ControllerAdvice(basePackages="org.my.pkg")} + * Controllers that belong to those base packages will be included, e.g.: + * {@code @ControllerAdvice(basePackages="org.my.pkg")} or * {@code @ControllerAdvice(basePackages={"org.my.pkg","org.my.other.pkg"})} * *

{@link #value()} is an alias for this attribute. - *

Use {@link #basePackageClasses()} for a type-safe alternative to String-based package names. + *

Also consider using {@link #basePackageClasses()} as a type-safe + * alternative to String-based package names. + * * @since 4.0 */ String[] basePackages() default {}; + /** + * Type-safe alternative to {@link #value()} for specifying the packages + * to select Controllers to be assisted by the {@code @ControllerAdvice} + * annotated class. + * + *

Consider creating a special no-op marker class or interface in each package + * that serves no purpose other than being referenced by this attribute. + * + * @since 4.0 + */ + Class[] basePackageClasses() default {}; + /** * Array of classes. * Controllers that are assignable to at least one of the given types * will be assisted by the {@code @ControllerAdvice} annotated class. + * * @since 4.0 */ Class[] assignableTypes() default {}; @@ -95,18 +111,9 @@ public @interface ControllerAdvice { * *

Consider creating a special annotation or use a predefined one, * like {@link RestController @RestController}. - * @since 4.0 - */ - Class[] annotations() default {}; - - /** - * Type-safe alternative to {@link #value()} for specifying the packages - * to select Controllers to be assisted by the {@code @ControllerAdvice} - * annotated class. * - *

Consider creating a special no-op marker class or interface in each package - * that serves no purpose other than being referenced by this attribute. * @since 4.0 */ - Class[] basePackageClasses() default {}; + Class[] annotations() default {}; + } 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 7724796bd2d..b82f31bd36f 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 @@ -46,6 +46,8 @@ import org.springframework.web.bind.annotation.ControllerAdvice; */ public class ControllerAdviceBean implements Ordered { + private static final Log logger = LogFactory.getLog(ControllerAdviceBean.class); + private final Object bean; private final int order; @@ -54,11 +56,10 @@ public class ControllerAdviceBean implements Ordered { private final List basePackages = new ArrayList(); - private final List> annotations = new ArrayList>(); + private final List> annotations = new ArrayList>(); private final List> assignableTypes = new ArrayList>(); - private static final Log logger = LogFactory.getLog(ControllerAdviceBean.class); /** * Create an instance using the given bean name. @@ -70,13 +71,19 @@ public class ControllerAdviceBean implements Ordered { Assert.notNull(beanFactory, "'beanFactory' must not be null"); Assert.isTrue(beanFactory.containsBean(beanName), "Bean factory [" + beanFactory + "] does not contain bean " + "with name [" + beanName + "]"); + this.bean = beanName; this.beanFactory = beanFactory; + Class beanType = this.beanFactory.getType(beanName); this.order = initOrderFromBeanType(beanType); - this.basePackages.addAll(initBasePackagesFromBeanType(beanType)); - this.annotations.addAll(initAnnotationsFromBeanType(beanType)); - this.assignableTypes.addAll(initAssignableTypesFromBeanType(beanType)); + + ControllerAdvice annotation = AnnotationUtils.findAnnotation(beanType,ControllerAdvice.class); + Assert.notNull(annotation, "BeanType [" + beanType.getName() + "] is not annotated @ControllerAdvice"); + + this.basePackages.addAll(initBasePackagesFromBeanType(beanType, annotation)); + this.annotations.addAll(Arrays.asList(annotation.annotations())); + this.assignableTypes.addAll(Arrays.asList(annotation.assignableTypes())); } private static int initOrderFromBeanType(Class beanType) { @@ -84,56 +91,35 @@ public class ControllerAdviceBean implements Ordered { return (annot != null) ? annot.value() : Ordered.LOWEST_PRECEDENCE; } - private static List initBasePackagesFromBeanType(Class beanType) { + private static List initBasePackagesFromBeanType(Class beanType, ControllerAdvice annotation) { List basePackages = new ArrayList(); - ControllerAdvice annotation = AnnotationUtils.findAnnotation(beanType,ControllerAdvice.class); - Assert.notNull(annotation,"BeanType ["+beanType.getName()+"] is not annotated @ControllerAdvice"); - for (String pkgName : (String[])AnnotationUtils.getValue(annotation)) { + List basePackageNames = new ArrayList(); + basePackageNames.addAll(Arrays.asList(annotation.value())); + basePackageNames.addAll(Arrays.asList(annotation.basePackages())); + for (String pkgName : basePackageNames) { if (StringUtils.hasText(pkgName)) { - Package pack = Package.getPackage(pkgName); - if(pack != null) { - basePackages.add(pack); - } else { - logger.warn("Package [" + pkgName + "] was not found, see [" - + beanType.getName() + "]"); + Package pkg = Package.getPackage(pkgName); + if(pkg != null) { + basePackages.add(pkg); } - } - } - for (String pkgName : (String[])AnnotationUtils.getValue(annotation,"basePackages")) { - if (StringUtils.hasText(pkgName)) { - Package pack = Package.getPackage(pkgName); - if(pack != null) { - basePackages.add(pack); - } else { - logger.warn("Package [" + pkgName + "] was not found, see [" - + beanType.getName() + "]"); + else { + logger.warn("Package [" + pkgName + "] was not found, see [" + beanType.getName() + "]"); } } } - for (Class markerClass : (Class[])AnnotationUtils.getValue(annotation,"basePackageClasses")) { - Package pack = markerClass.getPackage(); - if(pack != null) { - basePackages.add(pack); - } else { - logger.warn("Package was not found for class [" + markerClass.getName() - + "], see [" + beanType.getName() + "]"); - } + for (Class markerClass : annotation.basePackageClasses()) { + Package pack = markerClass.getPackage(); + if (pack != null) { + basePackages.add(pack); + } + else { + logger.warn("Package was not found for class [" + markerClass.getName() + + "], see [" + beanType.getName() + "]"); + } } return basePackages; } - private static List> initAnnotationsFromBeanType(Class beanType) { - ControllerAdvice annotation = AnnotationUtils.findAnnotation(beanType,ControllerAdvice.class); - Class[] annotations = (Class[])AnnotationUtils.getValue(annotation,"annotations"); - return Arrays.asList(annotations); - } - - private static List> initAssignableTypesFromBeanType(Class beanType) { - ControllerAdvice annotation = AnnotationUtils.findAnnotation(beanType,ControllerAdvice.class); - Class[] assignableTypes = (Class[])AnnotationUtils.getValue(annotation,"assignableTypes"); - return Arrays.asList(assignableTypes); - } - /** * Create an instance using the given bean instance. * @param bean the bean @@ -142,9 +128,14 @@ public class ControllerAdviceBean implements Ordered { Assert.notNull(bean, "'bean' must not be null"); this.bean = bean; this.order = initOrderFromBean(bean); - this.basePackages.addAll(initBasePackagesFromBeanType(bean.getClass())); - this.annotations.addAll(initAnnotationsFromBeanType(bean.getClass())); - this.assignableTypes.addAll(initAssignableTypesFromBeanType(bean.getClass())); + + Class beanType = bean.getClass(); + ControllerAdvice annotation = AnnotationUtils.findAnnotation(beanType,ControllerAdvice.class); + Assert.notNull(annotation, "BeanType [" + beanType.getName() + "] is not annotated @ControllerAdvice"); + + this.basePackages.addAll(initBasePackagesFromBeanType(beanType, annotation)); + this.annotations.addAll(Arrays.asList(annotation.annotations())); + this.assignableTypes.addAll(Arrays.asList(annotation.assignableTypes())); this.beanFactory = null; } @@ -195,31 +186,31 @@ public class ControllerAdviceBean implements Ordered { } /** - * Checks whether the bean type given as a parameter should be assisted by - * the current {@code @ControllerAdvice} annotated bean. + * Checks whether the given bean type should be assisted by this + * {@code @ControllerAdvice} instance. * - * @param beanType the type of the bean + * @param beanType the type of the bean to check * @see org.springframework.web.bind.annotation.ControllerAdvice * @since 4.0 */ public boolean isApplicableToBeanType(Class beanType) { - if(hasNoSelector()) { + if(!hasSelectors()) { return true; } - else if(beanType != null) { - String packageName = beanType.getPackage().getName(); - for(Package basePackage : this.basePackages) { - if(packageName.startsWith(basePackage.getName())) { + else if (beanType != null) { + for (Class clazz : this.assignableTypes) { + if(ClassUtils.isAssignable(clazz, beanType)) { return true; } } - for(Class annotationClass : this.annotations) { + for (Class annotationClass : this.annotations) { if(AnnotationUtils.findAnnotation(beanType, annotationClass) != null) { return true; } } - for(Class clazz : this.assignableTypes) { - if(ClassUtils.isAssignable(clazz, beanType)) { + String packageName = beanType.getPackage().getName(); + for (Package basePackage : this.basePackages) { + if(packageName.startsWith(basePackage.getName())) { return true; } } @@ -227,9 +218,8 @@ public class ControllerAdviceBean implements Ordered { return false; } - private boolean hasNoSelector() { - return this.basePackages.isEmpty() && this.annotations.isEmpty() - && this.assignableTypes.isEmpty(); + private boolean hasSelectors() { + return (!this.basePackages.isEmpty() || !this.annotations.isEmpty() || !this.assignableTypes.isEmpty()); } 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 918b146da66..25f0ccefde2 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 @@ -1,12 +1,12 @@ package org.springframework.web.method; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; + import org.junit.Test; import org.springframework.web.bind.annotation.ControllerAdvice; import org.springframework.web.bind.annotation.RestController; -import java.lang.annotation.Retention; -import java.lang.annotation.RetentionPolicy; - import static org.junit.Assert.*; /** @@ -116,12 +116,10 @@ public class ControllerAdviceBeanTests { @ControllerAdvice("org.springframework.web.method") static class BasePackageValueSupport {} - @ControllerAdvice(annotations = ControllerAnnotation.class, - assignableTypes = ControllerInterface.class) + @ControllerAdvice(annotations = ControllerAnnotation.class, assignableTypes = ControllerInterface.class) static class MultipleSelectorsSupport {} - @ControllerAdvice(basePackages = "java.util", - annotations = RestController.class) + @ControllerAdvice(basePackages = "java.util", annotations = {RestController.class}) static class ShouldNotMatch {} // Support classes