Nullability fine-tuning around declaration inconsistencies

Issue: SPR-15720
Issue: SPR-15792

Author: jhoeller

spring-aop/src/main/java/org/springframework/aop/Advisor.java

@@ -31,9 +31,18 @@
  * implemented using interception.
  *
  * @author Rod Johnson
+ * @author Juergen Hoeller
  */
 public interface Advisor {
 
+	/**
+	 * Common placeholder for an empty {@code Advice} to be returned from
+	 * {@link #getAdvice()} if no proper advice has been configured (yet).
+	 * @since 5.0
+	 */
+	Advice EMPTY_ADVICE = new Advice() {};
+
+
 	/**
 	 * Return the advice part of this aspect. An advice may be an
 	 * interceptor, a before advice, a throws advice, etc.

spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJPointcutAdvisor.java

@@ -23,7 +23,6 @@
 import org.springframework.core.Ordered;
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
-import org.springframework.util.ObjectUtils;
 
 /**
  * AspectJPointcutAdvisor that adapts an {@link AbstractAspectJAdvice}
@@ -53,11 +52,11 @@ public AspectJPointcutAdvisor(AbstractAspectJAdvice advice) {
 		this.pointcut = advice.buildSafePointcut();
 	}
 
+
 	public void setOrder(int order) {
 		this.order = order;
 	}
 
-
 	@Override
 	public boolean isPerInstance() {
 		return true;
@@ -93,12 +92,12 @@ public boolean equals(Object other) {
 			return false;
 		}
 		AspectJPointcutAdvisor otherAdvisor = (AspectJPointcutAdvisor) other;
-		return (ObjectUtils.nullSafeEquals(this.advice, otherAdvisor.advice));
+		return this.advice.equals(otherAdvisor.advice);
 	}
 
 	@Override
 	public int hashCode() {
-		return AspectJPointcutAdvisor.class.hashCode();
+		return AspectJPointcutAdvisor.class.hashCode() * 29 + this.advice.hashCode();
 	}
 
 }

spring-aop/src/main/java/org/springframework/aop/config/AdvisorComponentDefinition.java

@@ -39,11 +39,11 @@ public class AdvisorComponentDefinition extends AbstractComponentDefinition {
 
 	private final BeanDefinition advisorDefinition;
 
-	private String description;
+	private final String description;
 
-	private BeanReference[] beanReferences;
+	private final BeanReference[] beanReferences;
 
-	private BeanDefinition[] beanDefinitions;
+	private final BeanDefinition[] beanDefinitions;
 
 
 	public AdvisorComponentDefinition(String advisorBeanName, BeanDefinition advisorDefinition) {
@@ -57,11 +57,7 @@ public AdvisorComponentDefinition(
 		Assert.notNull(advisorDefinition, "'advisorDefinition' must not be null");
 		this.advisorBeanName = advisorBeanName;
 		this.advisorDefinition = advisorDefinition;
-		unwrapDefinitions(advisorDefinition, pointcutDefinition);
-	}
-
 
-	private void unwrapDefinitions(BeanDefinition advisorDefinition, @Nullable BeanDefinition pointcutDefinition) {
 		MutablePropertyValues pvs = advisorDefinition.getPropertyValues();
 		BeanReference adviceReference = (BeanReference) pvs.get("adviceBeanName");
 		Assert.state(adviceReference != null, "Missing 'adviceBeanName' property");

spring-aop/src/main/java/org/springframework/aop/config/SimpleBeanFactoryAwareAspectInstanceFactory.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2017 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.
@@ -21,6 +21,8 @@
 import org.springframework.beans.factory.BeanFactoryAware;
 import org.springframework.beans.factory.config.ConfigurableBeanFactory;
 import org.springframework.core.Ordered;
+import org.springframework.lang.Nullable;
+import org.springframework.util.Assert;
 import org.springframework.util.ClassUtils;
 import org.springframework.util.StringUtils;
 
@@ -34,8 +36,10 @@
  */
 public class SimpleBeanFactoryAwareAspectInstanceFactory implements AspectInstanceFactory, BeanFactoryAware {
 
+	@Nullable
 	private String aspectBeanName;
 
+	@Nullable
 	private BeanFactory beanFactory;
 
 
@@ -50,9 +54,7 @@ public void setAspectBeanName(String aspectBeanName) {
 	@Override
 	public void setBeanFactory(BeanFactory beanFactory) {
 		this.beanFactory = beanFactory;
-		if (!StringUtils.hasText(this.aspectBeanName)) {
-			throw new IllegalArgumentException("'aspectBeanName' is required");
-		}
+		Assert.notNull(this.aspectBeanName, "'aspectBeanName' is required");
 	}
 
 
@@ -62,6 +64,8 @@ public void setBeanFactory(BeanFactory beanFactory) {
 	 */
 	@Override
 	public Object getAspectInstance() {
+		Assert.state(this.beanFactory != null, "No BeanFactory set");
+		Assert.state(this.aspectBeanName != null, "No 'aspectBeanName' set");
 		return this.beanFactory.getBean(this.aspectBeanName);
 	}
 
@@ -77,7 +81,8 @@ public ClassLoader getAspectClassLoader() {
 
 	@Override
 	public int getOrder() {
-		if (this.beanFactory.isSingleton(this.aspectBeanName) &&
+		if (this.beanFactory != null && this.aspectBeanName != null &&
+				this.beanFactory.isSingleton(this.aspectBeanName) &&
 				this.beanFactory.isTypeMatch(this.aspectBeanName, Ordered.class)) {
 			return ((Ordered) this.beanFactory.getBean(this.aspectBeanName)).getOrder();
 		}

spring-aop/src/main/java/org/springframework/aop/framework/AbstractAdvisingBeanPostProcessor.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 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.
@@ -22,6 +22,7 @@
 import org.springframework.aop.Advisor;
 import org.springframework.aop.support.AopUtils;
 import org.springframework.beans.factory.config.BeanPostProcessor;
+import org.springframework.lang.Nullable;
 
 /**
  * Base class for {@link BeanPostProcessor} implementations that apply a
@@ -33,6 +34,7 @@
 @SuppressWarnings("serial")
 public abstract class AbstractAdvisingBeanPostProcessor extends ProxyProcessorSupport implements BeanPostProcessor {
 
+	@Nullable
 	protected Advisor advisor;
 
 	protected boolean beforeExistingAdvisors = false;
@@ -61,7 +63,7 @@ public Object postProcessBeforeInitialization(Object bean, String beanName) {
 
 	@Override
 	public Object postProcessAfterInitialization(Object bean, String beanName) {
-		if (bean instanceof AopInfrastructureBean) {
+		if (bean instanceof AopInfrastructureBean || this.advisor == null) {
 			// Ignore AOP infrastructure such as scoped proxies.
 			return bean;
 		}
@@ -125,6 +127,9 @@ protected boolean isEligible(Class<?> targetClass) {
 		if (eligible != null) {
 			return eligible;
 		}
+		if (this.advisor == null) {
+			return false;
+		}
 		eligible = AopUtils.canApply(this.advisor, targetClass);
 		this.eligibleBeans.put(targetClass, eligible);
 		return eligible;

spring-aop/src/main/java/org/springframework/aop/framework/AdvisedSupport.java

@@ -107,7 +107,7 @@ public class AdvisedSupport extends ProxyConfig implements Advised {
 	 * No-arg constructor for use as a JavaBean.
 	 */
 	public AdvisedSupport() {
-		initMethodCache();
+		this.methodCache = new ConcurrentHashMap<>(32);
 	}
 
 	/**
@@ -119,13 +119,6 @@ public AdvisedSupport(Class<?>... interfaces) {
 		setInterfaces(interfaces);
 	}
 
-	/**
-	 * Initialize the method cache.
-	 */
-	private void initMethodCache() {
-		this.methodCache = new ConcurrentHashMap<>(32);
-	}
-
 
 	/**
 	 * Set the given object as target.
@@ -558,7 +551,7 @@ private void readObject(ObjectInputStream ois) throws IOException, ClassNotFound
 		ois.defaultReadObject();
 
 		// Initialize transient fields.
-		initMethodCache();
+		this.methodCache = new ConcurrentHashMap<>(32);
 	}
 
 

spring-aop/src/main/java/org/springframework/aop/framework/autoproxy/AbstractAdvisorAutoProxyCreator.java

@@ -25,6 +25,7 @@
 import org.springframework.beans.factory.config.ConfigurableListableBeanFactory;
 import org.springframework.core.annotation.AnnotationAwareOrderComparator;
 import org.springframework.lang.Nullable;
+import org.springframework.util.Assert;
 
 /**
  * Generic auto proxy creator that builds AOP proxies for specific beans
@@ -48,6 +49,7 @@
 @SuppressWarnings("serial")
 public abstract class AbstractAdvisorAutoProxyCreator extends AbstractAutoProxyCreator {
 
+	@Nullable
 	private BeanFactoryAdvisorRetrievalHelper advisorRetrievalHelper;
 
 
@@ -101,6 +103,7 @@ protected List<Advisor> findEligibleAdvisors(Class<?> beanClass, String beanName
 	 * @return the List of candidate Advisors
 	 */
 	protected List<Advisor> findCandidateAdvisors() {
+		Assert.state(this.advisorRetrievalHelper != null, "No BeanFactoryAdvisorRetrievalHelper available");
 		return this.advisorRetrievalHelper.findAdvisorBeans();
 	}
 

spring-aop/src/main/java/org/springframework/aop/framework/autoproxy/AbstractAutoProxyCreator.java

@@ -203,7 +203,7 @@ public void setApplyCommonInterceptorsFirst(boolean applyCommonInterceptorsFirst
 	}
 
 	@Override
-	public void setBeanFactory(@Nullable BeanFactory beanFactory) {
+	public void setBeanFactory(BeanFactory beanFactory) {
 		this.beanFactory = beanFactory;
 	}
 
@@ -241,10 +241,10 @@ public Object getEarlyBeanReference(Object bean, String beanName) throws BeansEx
 	}
 
 	@Override
-	public Object postProcessBeforeInstantiation(Class<?> beanClass, @Nullable String beanName) throws BeansException {
+	public Object postProcessBeforeInstantiation(Class<?> beanClass, String beanName) throws BeansException {
 		Object cacheKey = getCacheKey(beanClass, beanName);
 
-		if (beanName == null || !this.targetSourcedBeans.contains(beanName)) {
+		if (!StringUtils.hasLength(beanName) || !this.targetSourcedBeans.contains(beanName)) {
 			if (this.advisedBeans.containsKey(cacheKey)) {
 				return null;
 			}
@@ -257,15 +257,15 @@ public Object postProcessBeforeInstantiation(Class<?> beanClass, @Nullable Strin
 		// Create proxy here if we have a custom TargetSource.
 		// Suppresses unnecessary default instantiation of the target bean:
 		// The TargetSource will handle target instances in a custom fashion.
-		if (beanName != null) {
-			TargetSource targetSource = getCustomTargetSource(beanClass, beanName);
-			if (targetSource != null) {
+		TargetSource targetSource = getCustomTargetSource(beanClass, beanName);
+		if (targetSource != null) {
+			if (StringUtils.hasLength(beanName)) {
 				this.targetSourcedBeans.add(beanName);
-				Object[] specificInterceptors = getAdvicesAndAdvisorsForBean(beanClass, beanName, targetSource);
-				Object proxy = createProxy(beanClass, beanName, specificInterceptors, targetSource);
-				this.proxyTypes.put(cacheKey, proxy.getClass());
-				return proxy;
 			}
+			Object[] specificInterceptors = getAdvicesAndAdvisorsForBean(beanClass, beanName, targetSource);
+			Object proxy = createProxy(beanClass, beanName, specificInterceptors, targetSource);
+			this.proxyTypes.put(cacheKey, proxy.getClass());
+			return proxy;
 		}
 
 		return null;
@@ -333,8 +333,8 @@ protected Object getCacheKey(Class<?> beanClass, @Nullable String beanName) {
 	 * @param cacheKey the cache key for metadata access
 	 * @return a proxy wrapping the bean, or the raw bean instance as-is
 	 */
-	protected Object wrapIfNecessary(Object bean, @Nullable String beanName, Object cacheKey) {
-		if (beanName != null && this.targetSourcedBeans.contains(beanName)) {
+	protected Object wrapIfNecessary(Object bean, String beanName, Object cacheKey) {
+		if (StringUtils.hasLength(beanName) && this.targetSourcedBeans.contains(beanName)) {
 			return bean;
 		}
 		if (Boolean.FALSE.equals(this.advisedBeans.get(cacheKey))) {
@@ -391,7 +391,7 @@ protected boolean isInfrastructureClass(Class<?> beanClass) {
 	 * @param beanName the name of the bean
 	 * @return whether to skip the given bean
 	 */
-	protected boolean shouldSkip(Class<?> beanClass, @Nullable String beanName) {
+	protected boolean shouldSkip(Class<?> beanClass, String beanName) {
 		return false;
 	}
 
@@ -582,8 +582,7 @@ protected void customizeProxyFactory(ProxyFactory proxyFactory) {
 	 * @see #PROXY_WITHOUT_ADDITIONAL_INTERCEPTORS
 	 */
 	@Nullable
-	protected abstract Object[] getAdvicesAndAdvisorsForBean(Class<?> beanClass,
-			@Nullable String beanName, @Nullable TargetSource customTargetSource)
-			throws BeansException;
+	protected abstract Object[] getAdvicesAndAdvisorsForBean(Class<?> beanClass, String beanName,
+			@Nullable TargetSource customTargetSource) throws BeansException;
 
 }

spring-aop/src/main/java/org/springframework/aop/framework/autoproxy/BeanNameAutoProxyCreator.java

@@ -76,7 +76,9 @@ public void setBeanNames(String... beanNames) {
 	 */
 	@Override
 	@Nullable
-	protected Object[] getAdvicesAndAdvisorsForBean(Class<?> beanClass, String beanName, @Nullable TargetSource targetSource) {
+	protected Object[] getAdvicesAndAdvisorsForBean(
+			Class<?> beanClass, String beanName, @Nullable TargetSource targetSource) {
+
 		if (this.beanNames != null) {
 			for (String mappedName : this.beanNames) {
 				if (FactoryBean.class.isAssignableFrom(beanClass)) {

spring-aop/src/main/java/org/springframework/aop/framework/autoproxy/InfrastructureAdvisorAutoProxyCreator.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2017 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.
@@ -18,6 +18,7 @@
 
 import org.springframework.beans.factory.config.BeanDefinition;
 import org.springframework.beans.factory.config.ConfigurableListableBeanFactory;
+import org.springframework.lang.Nullable;
 
 /**
  * Auto-proxy creator that considers infrastructure Advisor beans only,
@@ -29,6 +30,7 @@
 @SuppressWarnings("serial")
 public class InfrastructureAdvisorAutoProxyCreator extends AbstractAdvisorAutoProxyCreator {
 
+	@Nullable
 	private ConfigurableListableBeanFactory beanFactory;
 
 
@@ -40,7 +42,7 @@ protected void initBeanFactory(ConfigurableListableBeanFactory beanFactory) {
 
 	@Override
 	protected boolean isEligibleAdvisorBean(String beanName) {
-		return (this.beanFactory.containsBeanDefinition(beanName) &&
+		return (this.beanFactory != null && this.beanFactory.containsBeanDefinition(beanName) &&
 				this.beanFactory.getBeanDefinition(beanName).getRole() == BeanDefinition.ROLE_INFRASTRUCTURE);
 	}