Revised ControllerAdvice basePackages handling (based on package names instead of java.lang.Package)

Issue: SPR-12506

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

@@ -19,10 +19,9 @@ package org.springframework.web.method;
 import java.lang.annotation.Annotation;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.LinkedHashSet;
 import java.util.List;
-
-import org.apache.commons.logging.Log;
-import org.apache.commons.logging.LogFactory;
+import java.util.Set;
 
 import org.springframework.beans.factory.BeanFactory;
 import org.springframework.beans.factory.BeanFactoryUtils;
@@ -39,76 +38,77 @@ import org.springframework.web.bind.annotation.ControllerAdvice;
  * Encapsulates information about an {@linkplain ControllerAdvice @ControllerAdvice}
  * Spring-managed bean without necessarily requiring it to be instantiated.
  *
- * <p>The {@link #findAnnotatedBeans(ApplicationContext)} method can be used to discover
- * such beans. However, an {@code ControllerAdviceBean} may be created from
- * any object, including ones without an {@code @ControllerAdvice}.
+ * <p>The {@link #findAnnotatedBeans(ApplicationContext)} method can be used to
+ * discover such beans. However, a {@code ControllerAdviceBean} may be created
+ * from any object, including ones without an {@code @ControllerAdvice}.
  *
  * @author Rossen Stoyanchev
  * @author Brian Clozel
+ * @author Juergen Hoeller
  * @since 3.2
  */
 public class ControllerAdviceBean implements Ordered {
 
-	private static final Log logger = LogFactory.getLog(ControllerAdviceBean.class);
-
 	private final Object bean;
 
+	private final BeanFactory beanFactory;
+
 	private final int order;
 
-	private final BeanFactory beanFactory;
+	private final Set<String> basePackages;
 
-	private final List<Package> basePackages = new ArrayList<Package>();
+	private final List<Class<?>> assignableTypes;
 
-	private final List<Class<? extends Annotation>> annotations = new ArrayList<Class<? extends Annotation>>();
+	private final List<Class<? extends Annotation>> annotations;
 
-	private final List<Class<?>> assignableTypes = new ArrayList<Class<?>>();
 
+	/**
+	 * Create a {@code ControllerAdviceBean} using the given bean instance.
+	 * @param bean the bean instance
+	 */
+	public ControllerAdviceBean(Object bean) {
+		this(bean, null);
+	}
 
 	/**
-	 * Create an instance using the given bean name.
+	 * Create a {@code ControllerAdviceBean} using the given bean name.
 	 * @param beanName the name of the bean
 	 * @param beanFactory a BeanFactory that can be used later to resolve the bean
 	 */
 	public ControllerAdviceBean(String beanName, BeanFactory beanFactory) {
-		Assert.hasText(beanName, "Bean name must not be null");
-		Assert.notNull(beanFactory, "BeanFactory must not be null");
-
-		if (!beanFactory.containsBean(beanName)) {
-			throw new IllegalArgumentException(
-					"BeanFactory [" + beanFactory + "] does not contain bean with name '" + beanName + "'");
+		this((Object) beanName, beanFactory);
 	}
 
-		this.bean = beanName;
+	private ControllerAdviceBean(Object bean, BeanFactory beanFactory) {
+		this.bean = bean;
 		this.beanFactory = beanFactory;
+		Class<?> beanType;
 
-		Class<?> beanType = this.beanFactory.getType(beanName);
+		if (bean instanceof String) {
+			String beanName = (String) bean;
+			Assert.hasText(beanName, "Bean name must not be null");
+			Assert.notNull(beanFactory, "BeanFactory must not be null");
+			if (!beanFactory.containsBean(beanName)) {
+				throw new IllegalArgumentException("BeanFactory [" + beanFactory +
+						"] does not contain specified controller advice bean '" + beanName + "'");
+			}
+			beanType = this.beanFactory.getType(beanName);
 			this.order = initOrderFromBeanType(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()));
 		}
-
-	/**
-	 * Create an instance using the given bean instance.
-	 * @param bean the bean
-	 */
-	public ControllerAdviceBean(Object bean) {
+		else {
 			Assert.notNull(bean, "Bean must not be null");
-		this.bean = bean;
+			beanType = bean.getClass();
 			this.order = initOrderFromBean(bean);
+		}
 
-		Class<?> beanType = bean.getClass();
-		ControllerAdvice annotation = AnnotationUtils.findAnnotation(beanType,ControllerAdvice.class);
-		Assert.notNull(annotation, "Bean type [" + 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;
+		ControllerAdvice annotation = AnnotationUtils.findAnnotation(beanType, ControllerAdvice.class);
+		if (annotation == null) {
+			throw new IllegalArgumentException(
+					"Bean type [" + beanType.getName() + "] is not annotated as @ControllerAdvice");
+		}
+		this.basePackages = initBasePackages(annotation);
+		this.assignableTypes = Arrays.asList(annotation.assignableTypes());
+		this.annotations = Arrays.asList(annotation.annotations());
 	}
 
 
@@ -150,6 +150,11 @@ public class ControllerAdviceBean implements Ordered {
 			return true;
 		}
 		else if (beanType != null) {
+			for (String basePackage : this.basePackages) {
+				if (ClassUtils.getPackageName(beanType).startsWith(basePackage)) {
+					return true;
+				}
+			}
 			for (Class<?> clazz : this.assignableTypes) {
 				if (ClassUtils.isAssignable(clazz, beanType)) {
 					return true;
@@ -160,25 +165,25 @@ public class ControllerAdviceBean implements Ordered {
 					return true;
 				}
 			}
-			String packageName = beanType.getPackage().getName();
-			for (Package basePackage : this.basePackages) {
-				if (packageName.startsWith(basePackage.getName())) {
-					return true;
-				}
-			}
 		}
 		return false;
 	}
 
 	private boolean hasSelectors() {
-		return (!this.basePackages.isEmpty() || !this.annotations.isEmpty() || !this.assignableTypes.isEmpty());
+		return (!this.basePackages.isEmpty() || !this.assignableTypes.isEmpty() || !this.annotations.isEmpty());
 	}
 
 
 	@Override
 	public boolean equals(Object other) {
-		return (this == other ||
-				(other instanceof ControllerAdviceBean && this.bean.equals(((ControllerAdviceBean) other).bean)));
+		if (this == other) {
+			return true;
+		}
+		if (!(other instanceof ControllerAdviceBean)) {
+			return false;
+		}
+		ControllerAdviceBean otherAdvice = (ControllerAdviceBean) other;
+		return (this.bean.equals(otherAdvice.bean) && this.beanFactory == otherAdvice.beanFactory);
 	}
 
 	@Override
@@ -215,31 +220,20 @@ public class ControllerAdviceBean implements Ordered {
 		return OrderUtils.getOrder(beanType, Ordered.LOWEST_PRECEDENCE);
 	}
 
-	private static List<Package> initBasePackagesFromBeanType(Class<?> beanType, ControllerAdvice annotation) {
-		List<Package> basePackages = new ArrayList<Package>();
-		List<String> basePackageNames = new ArrayList<String>();
-		basePackageNames.addAll(Arrays.asList(annotation.value()));
-		basePackageNames.addAll(Arrays.asList(annotation.basePackages()));
-		for (String pkgName : basePackageNames) {
-			if (StringUtils.hasText(pkgName)) {
-				Package pkg = Package.getPackage(pkgName);
-				if (pkg != null) {
-					basePackages.add(pkg);
+	private static Set<String> initBasePackages(ControllerAdvice annotation) {
+		Set<String> basePackages = new LinkedHashSet<String>();
+		for (String basePackage : annotation.value()) {
+			if (StringUtils.hasText(basePackage)) {
+				basePackages.add(basePackage);
 			}
-				else {
-					logger.warn("Package [" + pkgName + "] was not found, see [" + beanType.getName() + "]");
 		}
+		for (String basePackage : annotation.basePackages()) {
+			if (StringUtils.hasText(basePackage)) {
+				basePackages.add(basePackage);
 			}
 		}
 		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() + "]");
-			}
+			basePackages.add(ClassUtils.getPackageName(markerClass));
 		}
 		return basePackages;
 	}