From b4375aae21189baba6a5fba1fc5bc335c5eb7ac4 Mon Sep 17 00:00:00 2001
From: dhgarrette <dhgarrette@2e8e966d-6415-0410-94b9-dc4e65435d03>
Date: Sat, 7 Mar 2009 07:16:06 +0000
Subject: [PATCH] BATCH-1118: Modified StepListenerFactoryBean so that listener
 methods specified via annotations or the metaDataMap are checked for correct
 signatures.  This allows incorrect methods to fail eagerly.  For added
 flexibility, methods are also allowed if they do not specify any parameters.

---
 .../listener/StepListenerFactoryBean.java     |  56 ++++++++-
 .../StepListenerFactoryBeanTests.java         | 106 ++++++++++++++++++
 .../batch/support/MethodInvokerUtils.java     |  34 ++++--
 3 files changed, 181 insertions(+), 15 deletions(-)

diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/listener/StepListenerFactoryBean.java b/spring-batch-core/src/main/java/org/springframework/batch/core/listener/StepListenerFactoryBean.java
index 243030c45..e7c51f498 100644
--- a/spring-batch-core/src/main/java/org/springframework/batch/core/listener/StepListenerFactoryBean.java
+++ b/spring-batch-core/src/main/java/org/springframework/batch/core/listener/StepListenerFactoryBean.java
@@ -15,9 +15,11 @@
  */
 package org.springframework.batch.core.listener;
 
-import static org.springframework.batch.support.MethodInvokerUtils.getMethodInvokerByAnnotation;
 import static org.springframework.batch.support.MethodInvokerUtils.getMethodInvokerForInterface;
+import static org.springframework.batch.support.MethodInvokerUtils.getParamTypesString;
 
+import java.lang.annotation.Annotation;
+import java.lang.reflect.Method;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
@@ -31,13 +33,15 @@ import org.springframework.batch.support.MethodInvoker;
 import org.springframework.batch.support.MethodInvokerUtils;
 import org.springframework.beans.factory.FactoryBean;
 import org.springframework.beans.factory.InitializingBean;
+import org.springframework.core.annotation.AnnotationUtils;
 import org.springframework.util.Assert;
+import org.springframework.util.ReflectionUtils;
 
 /**
  * {@link FactoryBean} implementation that builds a {@link StepListener} based
  * on the various lifecycle methods or annotations that are provided. There are
  * three possible ways of having a method called as part of a
- * {@link StepListener} lifecyle:
+ * {@link StepListener} lifecycle:
  * 
  * <ul>
  * <li>Interface implementation: By implementing any of the subclasses of
@@ -86,12 +90,12 @@ public class StepListenerFactoryBean implements FactoryBean, InitializingBean {
 		// For every entry in the map, try and find a method by interface, name,
 		// or annotation. If the same
 		for (Entry<String, String> entry : metaDataMap.entrySet()) {
-			StepListenerMetaData metaData = StepListenerMetaData.fromPropertyName(entry.getKey());
+			final StepListenerMetaData metaData = StepListenerMetaData.fromPropertyName(entry.getKey());
 			Set<MethodInvoker> invokers = new NullIgnoringSet<MethodInvoker>();
 			invokers.add(getMethodInvokerByName(entry.getValue(), delegate, metaData.getParamTypes()));
 			invokers.add(getMethodInvokerForInterface(metaData.getListenerInterface(), metaData.getMethodName(),
 					delegate, metaData.getParamTypes()));
-			invokers.add(getMethodInvokerByAnnotation(metaData.getAnnotation(), delegate));
+			invokers.add(getMethodInvokerByAnnotation(metaData, metaData.getAnnotation()));
 			if (!invokers.isEmpty()) {
 				invokerMap.put(metaData.getMethodName(), invokers);
 				listenerInterfaces.add(metaData.getListenerInterface());
@@ -110,6 +114,43 @@ public class StepListenerFactoryBean implements FactoryBean, InitializingBean {
 		return proxyFactory.getProxy();
 	}
 
+	/**
+	 * Create a MethodInvoker from the delegate based on the annotationType.
+	 * Ensure that the annotated method has a valid set of parameters.
+	 * 
+	 * @param metaData
+	 * @param annotationType
+	 * @return
+	 */
+	private MethodInvoker getMethodInvokerByAnnotation(final StepListenerMetaData metaData,
+			final Class<? extends Annotation> annotationType) {
+		MethodInvoker mi = MethodInvokerUtils.getMethodInvokerByAnnotation(annotationType, delegate);
+		if (mi != null) {
+			ReflectionUtils.doWithMethods(delegate.getClass(), new ReflectionUtils.MethodCallback() {
+				public void doWith(Method method) throws IllegalArgumentException, IllegalAccessException {
+					Annotation annotation = AnnotationUtils.findAnnotation(method, annotationType);
+					if (annotation != null) {
+						Class<?>[] paramTypes = method.getParameterTypes();
+						if (paramTypes.length > 0) {
+							Class<?>[] expectedParamTypes = metaData.getParamTypes();
+
+							String errorMsg = "The method [" + method.getName() + "] on target class ["
+									+ delegate.getClass().getSimpleName() + "] is incompatable with the signature ["
+									+ getParamTypesString(expectedParamTypes) + "] expected for the annotation ["
+									+ metaData.getAnnotation().getSimpleName() + "].";
+
+							Assert.isTrue(paramTypes.length == expectedParamTypes.length, errorMsg);
+							for (int i = 0; i < paramTypes.length; i++) {
+								Assert.isTrue(paramTypes[i].equals(expectedParamTypes[i]), errorMsg);
+							}
+						}
+					}
+				}
+			});
+		}
+		return mi;
+	}
+
 	private MethodInvoker getMethodInvokerByName(String methodName, Object candidate, Class<?>... params) {
 		if (methodName != null) {
 			return MethodInvokerUtils.createMethodInvokerByName(candidate, methodName, false, params);
@@ -139,6 +180,7 @@ public class StepListenerFactoryBean implements FactoryBean, InitializingBean {
 	/**
 	 * Convenience method to wrap any object and expose the appropriate
 	 * {@link StepListener} interfaces.
+	 * 
 	 * @param delegate a delegate object
 	 * @return a StepListener instance constructed from the delegate
 	 */
@@ -151,16 +193,18 @@ public class StepListenerFactoryBean implements FactoryBean, InitializingBean {
 	/**
 	 * Convenience method to check whether the given object is or can be made
 	 * into a {@link StepListener}.
+	 * 
 	 * @param delegate the object to check
 	 * @return true if the delegate is an instance of any of the
-	 * {@link StepListener} interfaces, or contains the marker annotations
+	 *         {@link StepListener} interfaces, or contains the marker
+	 *         annotations
 	 */
 	public static boolean isListener(Object delegate) {
 		if (delegate instanceof StepListener) {
 			return true;
 		}
 		for (StepListenerMetaData metaData : StepListenerMetaData.values()) {
-			if (getMethodInvokerByAnnotation(metaData.getAnnotation(), delegate) != null) {
+			if (MethodInvokerUtils.getMethodInvokerByAnnotation(metaData.getAnnotation(), delegate) != null) {
 				return true;
 			}
 		}
diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/listener/StepListenerFactoryBeanTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/listener/StepListenerFactoryBeanTests.java
index ad90ac0e8..34f215966 100644
--- a/spring-batch-core/src/test/java/org/springframework/batch/core/listener/StepListenerFactoryBeanTests.java
+++ b/spring-batch-core/src/test/java/org/springframework/batch/core/listener/StepListenerFactoryBeanTests.java
@@ -20,8 +20,10 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.springframework.batch.core.listener.StepListenerMetaData.AFTER_CHUNK;
 import static org.springframework.batch.core.listener.StepListenerMetaData.AFTER_STEP;
+import static org.springframework.batch.core.listener.StepListenerMetaData.AFTER_WRITE;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -50,6 +52,7 @@ import org.springframework.batch.core.annotation.BeforeWrite;
 import org.springframework.batch.core.annotation.OnProcessError;
 import org.springframework.batch.core.annotation.OnReadError;
 import org.springframework.batch.core.annotation.OnWriteError;
+import org.springframework.batch.core.configuration.xml.AbstractTestComponent;
 import org.springframework.util.Assert;
 
 /**
@@ -189,6 +192,109 @@ public class StepListenerFactoryBeanTests {
 		assertTrue(listener instanceof StepListener);
 	}
 	
+	@Test
+	public void testEmptySignatureAnnotation() {
+		AbstractTestComponent delegate = new AbstractTestComponent() {
+			@SuppressWarnings("unused")
+			@AfterWrite
+			public void aMethod() {
+				executed = true;
+			}
+		};
+		factoryBean.setDelegate(delegate);
+		@SuppressWarnings("unchecked")
+		ItemWriteListener<String> listener = (ItemWriteListener<String>) factoryBean.getObject();
+		listener.afterWrite(Arrays.asList("foo", "bar"));
+		assertTrue(delegate.isExecuted());
+	}
+
+	@Test
+	public void testRightSignatureAnnotation() {
+		AbstractTestComponent delegate = new AbstractTestComponent() {
+			@SuppressWarnings("unused")
+			@AfterWrite
+			public void aMethod(List<String> items) {
+				executed = true;
+				assertEquals("foo", items.get(0));
+				assertEquals("bar", items.get(1));
+			}
+		};
+		factoryBean.setDelegate(delegate);
+		@SuppressWarnings("unchecked")
+		ItemWriteListener<String> listener = (ItemWriteListener<String>) factoryBean.getObject();
+		listener.afterWrite(Arrays.asList("foo", "bar"));
+		assertTrue(delegate.isExecuted());
+	}
+
+	@Test(expected = IllegalArgumentException.class)
+	public void testWrongSignatureAnnotation() {
+		AbstractTestComponent delegate = new AbstractTestComponent() {
+			@SuppressWarnings("unused")
+			@AfterWrite
+			public void aMethod(Integer item) {
+				executed = true;
+			}
+		};
+		factoryBean.setDelegate(delegate);
+		factoryBean.getObject();
+	}
+
+	@Test
+	public void testEmptySignatureNamedMethod() {
+		AbstractTestComponent delegate = new AbstractTestComponent() {
+			@SuppressWarnings("unused")
+			@AfterWrite
+			public void aMethod() {
+				executed = true;
+			}
+		};
+		factoryBean.setDelegate(delegate);
+		Map<String, String> metaDataMap = new HashMap<String, String>();
+		metaDataMap.put(AFTER_WRITE.getPropertyName(), "aMethod");
+		factoryBean.setMetaDataMap(metaDataMap);
+		@SuppressWarnings("unchecked")
+		ItemWriteListener<String> listener = (ItemWriteListener<String>) factoryBean.getObject();
+		listener.afterWrite(Arrays.asList("foo", "bar"));
+		assertTrue(delegate.isExecuted());
+	}
+
+	@Test
+	public void testRightSignatureNamedMethod() {
+		AbstractTestComponent delegate = new AbstractTestComponent() {
+			@SuppressWarnings("unused")
+			@AfterWrite
+			public void aMethod(List<String> items) {
+				executed = true;
+				assertEquals("foo", items.get(0));
+				assertEquals("bar", items.get(1));
+			}
+		};
+		factoryBean.setDelegate(delegate);
+		Map<String, String> metaDataMap = new HashMap<String, String>();
+		metaDataMap.put(AFTER_WRITE.getPropertyName(), "aMethod");
+		factoryBean.setMetaDataMap(metaDataMap);
+		@SuppressWarnings("unchecked")
+		ItemWriteListener<String> listener = (ItemWriteListener<String>) factoryBean.getObject();
+		listener.afterWrite(Arrays.asList("foo", "bar"));
+		assertTrue(delegate.isExecuted());
+	}
+
+	@Test(expected = IllegalArgumentException.class)
+	public void testWrongSignatureNamedMethod() {
+		AbstractTestComponent delegate = new AbstractTestComponent() {
+			@SuppressWarnings("unused")
+			@AfterWrite
+			public void aMethod(Integer item) {
+				executed = true;
+			}
+		};
+		factoryBean.setDelegate(delegate);
+		Map<String, String> metaDataMap = new HashMap<String, String>();
+		metaDataMap.put(AFTER_WRITE.getPropertyName(), "aMethod");
+		factoryBean.setMetaDataMap(metaDataMap);
+		factoryBean.getObject();
+	}
+
 	private class MultipleAfterStep implements StepExecutionListener {
 
 		int callcount = 0;
diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/support/MethodInvokerUtils.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/support/MethodInvokerUtils.java
index 65d373888..b96caac67 100644
--- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/support/MethodInvokerUtils.java
+++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/support/MethodInvokerUtils.java
@@ -50,18 +50,34 @@ public class MethodInvokerUtils {
 			Class<?>... paramTypes) {
 		Assert.notNull(object, "Object to invoke must not be null");
 		Method method = ClassUtils.getMethodIfAvailable(object.getClass(), methodName, paramTypes);
-		// if no method was found for the given parameters, and the parameters
-		// aren't required
-		if (method == null && !paramsRequired) {
-			// try with no params
-			method = ClassUtils.getMethodIfAvailable(object.getClass(), methodName, new Class[] {});
-		}
 		if (method == null) {
-			return null;
+			String errorMsg = "no method found with name [" + methodName + "] on class ["
+					+ object.getClass().getSimpleName() + "] compatable with the signature ["
+					+ getParamTypesString(paramTypes) + "].";
+			Assert.isTrue(!paramsRequired, errorMsg);
+			// if no method was found for the given parameters, and the
+			// parameters aren't required, then try with no params
+			method = ClassUtils.getMethodIfAvailable(object.getClass(), methodName, new Class[] {});
+			Assert.notNull(method, errorMsg);
 		}
-		else {
-			return new SimpleMethodInvoker(object, method);
+		return new SimpleMethodInvoker(object, method);
+	}
+
+	/**
+	 * Create a String representation of the array of parameter types.
+	 * 
+	 * @param paramTypes
+	 * @return String
+	 */
+	public static String getParamTypesString(Class<?>... paramTypes) {
+		StringBuffer paramTypesList = new StringBuffer("(");
+		for (int i = 0; i < paramTypes.length; i++) {
+			paramTypesList.append(paramTypes[i].getSimpleName());
+			if (i + 1 < paramTypes.length) {
+				paramTypesList.append(", ");
+			}
 		}
+		return paramTypesList.append(")").toString();
 	}
 
 	/**