[Springframework-developer] patch: prototype ProxyFactoryBean

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

The following patch addresses several issues in the ProxyFactoryBean 
which prevent it from correctly creating beans with prototype advisors:

-Cannot create proxy when using prototype advisor when singleton=false
-Cannot create proxy when using prototype interceptor when singletone=false
-Cannot create proxy when using singleton target and singleton=false 
(ie, to use a prototype advisor on a singleton target)

Essentially, at the moment the only part of a prototype ProxyFactoryBean 
which may be a prototype is the target bean.

The patch contains 2 new test cases in PorxyFactoryBeanTests which 
illustrate 2 examples:  a prototype target wrapped by a prototype 
advisor, and a singleton target wrapped by a prototype interceptor.

They demonstrate that the LockMixinAdvisor/LockMixin is not currently 
usable as a prototype.

Each test, prior to applying the patch, fails with this message:

BeanCreationException: Error creating bean with name 
'prototypeTestBeanProxySingletonTarget' defined in class path resource 
[proxyFactoryTests.xml]: Initialization of bean failed; nested exception 
is org.springframework.aop.framework.AopConfigException: TargetSource 
specified more than once: Specify in targetSource property or at the END 
of the interceptorNames list
...

Note that in neither case is a targetSource specified more than once.

The main defect is that when the factory is initialized, it avoids 
loading prototype beans which appear in the interceptorNames property. 
Instead it substitutes a null value.  When this null value is later 
examined, it cannot be determined whether the bean was supposed to be a 
target or an advisor.

The solution I used is to use a marker object which holds the place of 
prototype advisors in the case that a bean class implements Advisor or 
Interceptor.  If the bean class does not implement one of these 
interfaces, it is assumed to be the target.

Additionally, I changed some of the messages in exceptions and debug 
output to be more clear/accurate.

Comments, suggestions, insults welcome.

Chris Eldredge

Patch follows inline.


Index: src/org/springframework/aop/framework/ProxyFactoryBean.java
===================================================================
RCS file: 
/cvsroot/springframework/spring/src/org/springframework/aop/framework/ProxyFactoryBean.java,v
retrieving revision 1.37
diff -u -r1.37 ProxyFactoryBean.java

--- src/org/springframework/aop/framework/ProxyFactoryBean.java	9 Sep 
2004 14:37:18 -0000	1.37
+++ src/org/springframework/aop/framework/ProxyFactoryBean.java	11 Sep 
2004 22:10:32 -0000
@@ -23,6 +23,7 @@
  import java.util.List;
  import java.util.Map;

+import org.aopalliance.aop.Advice;
  import org.aopalliance.intercept.Interceptor;

  import org.springframework.aop.Advisor;
@@ -38,6 +39,7 @@
  import org.springframework.beans.factory.BeanFactoryUtils;
  import org.springframework.beans.factory.FactoryBean;
  import org.springframework.beans.factory.ListableBeanFactory;
+import org.springframework.beans.factory.support.BeanDefinitionRegistry;
  import org.springframework.core.OrderComparator;

  /**
@@ -233,6 +235,21 @@
  	}

  	/**
+	 * Marker class which holds a place in the sourceMap when
+	 * ProxyFactoryBean is configured for prototype usage.  This
+	 * instance will be replaced by the actual prototype advisor
+	 * when a new instance is requested.
+	 */
+	private class MarkerAdvisor implements Advisor {
+		public Advice getAdvice() {
+			return null;
+		}
+		public boolean isPerInstance() {
+			return true;
+		}
+	}
+	
+	/**
  	 * Create the advisor (interceptor) chain. The advisors that are sourced
  	 * from a BeanFactory will be refreshed each time a new prototype 
instance
  	 * is added. Interceptors added programmatically through the factory API
@@ -270,6 +287,16 @@
  				// avoid unnecessary creation of prototype bean just for advisor 
chain initialization
  				if (isSingleton() || 
this.beanFactory.isSingleton(this.interceptorNames[i])) {
  					advice = this.beanFactory.getBean(this.interceptorNames[i]);
+				} else if (this.beanFactory instanceof BeanDefinitionRegistry){
+					// discover the bean type without creating an instance
+					BeanDefinitionRegistry bdr = (BeanDefinitionRegistry) 
this.beanFactory;
+					Class cls = 
bdr.getBeanDefinition(this.interceptorNames[i]).getBeanClass();
+
+					if (Advisor.class.isAssignableFrom(cls) || 
Interceptor.class.isAssignableFrom(cls)) {
+						// Don't load the real prototype, but put a marker in its place so
+						// it isn't lost
+						advice = new MarkerAdvisor();
+					}
  				}
  				addAdvisorOnChainCreation(advice, this.interceptorNames[i]);
  			}
@@ -356,8 +383,7 @@
  		// Can only use interceptorName -> TargetSource conversion once,
  		// for the last entry in the interceptorNames list.
  		if (this.targetName != null) {
-			throw new AopConfigException("TargetSource specified more than once 
in interceptorNames list:" +
-					"Specify in targetSource property or ONCE at the END of the 
interceptorNames list");
+			throw new AopConfigException("Cannot specify more than one 
non-advisor bean name in interceptorNames");
  		}
  		
  		// We need to convert to an Advisor if necessary so that our source 
reference matches
@@ -383,7 +409,7 @@
  			// The default set by AdvisedSupport superclass is OK.
  			if (this.targetSource != EMPTY_TARGET_SOURCE) {
  				throw new AopConfigException("TargetSource specified more than 
once: " +
-						"Specify in targetSource property or at the END of the 
interceptorNames list");
+						"Specify in target property or targetSource property or at the 
END of the interceptorNames list");
  			}
  			if (logger.isDebugEnabled()) {
  				logger.debug("Adding TargetSource [" + advisor + "] with name [" + 
name + "]");
@@ -395,14 +421,17 @@
  	}
  	
  	private void refreshTarget() {
-		if (logger.isDebugEnabled()) {
-			logger.debug("Refreshing target with name '" + this.targetName + "'");
-		}
  		if (this.targetName == null) {
-			throw new AopConfigException("Target name cannot be null when 
refreshing!");
+			if (logger.isDebugEnabled()) {
+				logger.debug("Not refreshing target: bean name not specified in 
interceptorNames");
+			}
+		} else {
+			if (logger.isDebugEnabled()) {
+				logger.debug("Refreshing target with name '" + this.targetName + "'");
+			}
+			Object target = this.beanFactory.getBean(this.targetName);
+			setTarget(target);
  		}
-		Object target = this.beanFactory.getBean(this.targetName);
-		setTarget(target);
  	}

  	/**
Index: test/org/springframework/aop/framework/ProxyFactoryBeanTests.java
===================================================================
RCS file: 
/cvsroot/springframework/spring/test/org/springframework/aop/framework/ProxyFactoryBeanTests.java,v
retrieving revision 1.34
diff -u -r1.34 ProxyFactoryBeanTests.java
--- test/org/springframework/aop/framework/ProxyFactoryBeanTests.java	30 
Aug 2004 15:53:58 -0000	1.34
+++ test/org/springframework/aop/framework/ProxyFactoryBeanTests.java	11 
Sep 2004 22:10:36 -0000
@@ -103,7 +103,7 @@
  		catch (BeanCreationException ex) {
  			// Root cause of the problem must be an AOP exception
  			AopConfigException aex = (AopConfigException) ex.getCause();
-			assertTrue(aex.getMessage().indexOf("TargetSource") != -1);
+			assertTrue(aex.getMessage().indexOf("Cannot specify more than one") 
!= -1);
  		}
  	}
  	
@@ -592,6 +592,60 @@
  		assertFalse("Not serializable because an interceptor isn't 
serializable", SerializationTestUtils.isSerializable(p));
  	}
  	
+	// This test fails with 
src/org/springframework/aop/framework/ProxyFactoryBean.java in build 96	
+	public void testPrototypeAdvisor() {
+		BeanFactory bf = new XmlBeanFactory(new 
ClassPathResource("proxyFactoryTests.xml", getClass()));
+		
+		ITestBean bean1 = (ITestBean) bf.getBean("prototypeTestBeanProxy");
+		ITestBean bean2 = (ITestBean) bf.getBean("prototypeTestBeanProxy");
+		
+		bean1.setAge(3);
+		bean2.setAge(4);
+		
+		assertEquals(3, bean1.getAge());
+		assertEquals(4, bean2.getAge());
+		
+		((Lockable) bean1).lock();
+		
+		try {
+			bean1.setAge(5);
+			fail("expected LockedException");
+		} catch (LockedException e) {
+		}
+		
+		try {
+			bean2.setAge(6);
+		} catch (LockedException e) {
+			fail("did not expect LockedException");
+		}
+	}
+
+	// This test fails with 
src/org/springframework/aop/framework/ProxyFactoryBean.java in build 96
+	public void testPrototypeInterceptorSingletonTarget() {
+		BeanFactory bf = new XmlBeanFactory(new 
ClassPathResource("proxyFactoryTests.xml", getClass()));
+		
+		ITestBean bean1 = (ITestBean) 
bf.getBean("prototypeTestBeanProxySingletonTarget");
+		ITestBean bean2 = (ITestBean) 
bf.getBean("prototypeTestBeanProxySingletonTarget");
+		
+		bean1.setAge(1);
+		bean2.setAge(2);
+		
+		assertEquals(2, bean1.getAge());
+		
+		((Lockable) bean1).lock();
+		
+		try {
+			bean1.setAge(5);
+			fail("expected LockedException");
+		} catch (LockedException e) {
+		}
+		
+		try {
+			bean2.setAge(6);
+		} catch (LockedException e) {
+			fail("did not expect LockedException");
+		}
+	}	

  	/**
  	 * Fires only on void methods. Saves list of methods intercepted.
Index: test/org/springframework/aop/framework/proxyFactoryTests.xml
===================================================================
RCS file: 
/cvsroot/springframework/spring/test/org/springframework/aop/framework/proxyFactoryTests.xml,v
retrieving revision 1.1
diff -u -r1.1 proxyFactoryTests.xml
--- test/org/springframework/aop/framework/proxyFactoryTests.xml	30 Nov 
2003 17:17:33 -0000	1.1
+++ test/org/springframework/aop/framework/proxyFactoryTests.xml	11 Sep 
2004 22:10:36 -0000
@@ -148,7 +148,49 @@
  		class="org.springframework.context.event.ConsoleListener">
  	</bean>
  	
+	<bean id="prototypeLockMixinAdvisor"
+		class="org.springframework.aop.framework.LockMixinAdvisor"
+		singleton="false"
+	/>
+	
+	<bean id="prototypeTestBean"
+			class="org.springframework.beans.TestBean"
+			singleton="false"
+	/>
+	
+	<bean id="prototypeTestBeanProxy"
+			class="org.springframework.aop.framework.ProxyFactoryBean"
+	>	
+		<property 
name="proxyInterfaces"><value>org.springframework.beans.ITestBean</value></property> 
	
+		<property name="singleton"><value>false</value></property>
+		<property name="interceptorNames">
+			<list>
+				<value>prototypeLockMixinAdvisor</value>
+				<value>prototypeTestBean</value>
+			</list>
+		</property>		
+	</bean>

-</beans>		
+	<bean id="prototypeLockMixinInterceptor"
+		class="org.springframework.aop.framework.LockMixin"
+		singleton="false"
+	/>
  	
-	
\ No newline at end of file
+	<bean id="prototypeTestBeanProxySingletonTarget"
+			class="org.springframework.aop.framework.ProxyFactoryBean"
+	>	
+		<property name="proxyInterfaces">
+			<list>
+				<value>org.springframework.beans.ITestBean</value>
+				<value>org.springframework.aop.framework.Lockable</value>
+			</list>
+		</property>	
+		<property name="singleton"><value>false</value></property>
+		<property name="target"><ref local="prototypeTestBean"/></property>
+		<property name="interceptorNames">
+			<list>
+				<value>prototypeLockMixinInterceptor</value>
+			</list>
+		</property>		
+	</bean>
+</beans>
\ No newline at end of file



