SGF-423 - Handle improper ClassCastException thrown from SDG's Function Execution interface and annotation-based support when a GemFire Function throws an Exception.

Additional refactoring and added serveral unit tests.

src/main/java/org/springframework/data/gemfire/function/config/AbstractFunctionExecutionConfigurationSource.java

@@ -71,7 +71,9 @@ abstract class AbstractFunctionExecutionConfigurationSource implements FunctionE
 	}
 
 	public Collection<ScannedGenericBeanDefinition> getCandidates(ResourceLoader loader) {
-		ClassPathScanningCandidateComponentProvider scanner = new FunctionExecutionComponentProvider(getIncludeFilters(),functionExecutionAnnotationTypes);
+		ClassPathScanningCandidateComponentProvider scanner = new FunctionExecutionComponentProvider(
+			getIncludeFilters(), getFunctionExecutionAnnotationTypes());
+
 		scanner.setResourceLoader(loader);
 
 		for (TypeFilter filter : getExcludeFilters()) {
@@ -84,9 +86,11 @@ abstract class AbstractFunctionExecutionConfigurationSource implements FunctionE
 			if (logger.isDebugEnabled()) {
 				logger.debug("scanning package " + basePackage);
 			}
-			Collection<BeanDefinition> components = scanner.findCandidateComponents(basePackage);
-			for (BeanDefinition definition : components) {
-				result.add((ScannedGenericBeanDefinition)definition);
+
+			Collection<BeanDefinition> candidateComponents = scanner.findCandidateComponents(basePackage);
+
+			for (BeanDefinition beanDefinition : candidateComponents) {
+				result.add((ScannedGenericBeanDefinition) beanDefinition);
 			}
 		}
 

src/main/java/org/springframework/data/gemfire/function/config/FunctionExecutionBeanDefinitionRegistrar.java

@@ -12,8 +12,6 @@
  */
 package org.springframework.data.gemfire.function.config;
 
-import java.lang.annotation.Annotation;
-import java.util.HashSet;
 import java.util.Set;
 
 import org.springframework.beans.factory.support.BeanDefinitionReaderUtils;
@@ -21,7 +19,6 @@ import org.springframework.beans.factory.support.BeanDefinitionRegistry;
 import org.springframework.context.annotation.ImportBeanDefinitionRegistrar;
 import org.springframework.context.annotation.ScannedGenericBeanDefinition;
 import org.springframework.core.io.DefaultResourceLoader;
-import org.springframework.core.io.ResourceLoader;
 import org.springframework.core.type.AnnotationMetadata;
 import org.springframework.util.Assert;
 import org.springframework.util.StringUtils;
@@ -52,7 +49,9 @@ class FunctionExecutionBeanDefinitionRegistrar implements ImportBeanDefinitionRe
 	void registerBeanDefinitions(AbstractFunctionExecutionConfigurationSource functionExecutionConfigurationSource,
 			BeanDefinitionRegistry registry) {
 
-		for (ScannedGenericBeanDefinition beanDefinition : functionExecutionConfigurationSource.getCandidates(new DefaultResourceLoader())) {
+		for (ScannedGenericBeanDefinition beanDefinition : functionExecutionConfigurationSource.getCandidates(
+				new DefaultResourceLoader())) {
+
 			String functionExecutionAnnotation = getFunctionExecutionAnnotation(beanDefinition,
 				AnnotationFunctionExecutionConfigurationSource.getFunctionExecutionAnnotationTypeNames());
 

src/main/java/org/springframework/data/gemfire/function/config/FunctionExecutionConfiguration.java

@@ -31,6 +31,12 @@ class FunctionExecutionConfiguration  {
 
 	private final String annotationType;
 
+	/* constructor for testing purposes only! */
+	FunctionExecutionConfiguration() {
+		this.annotationType = null;
+		this.attributes = null;
+	}
+
 	FunctionExecutionConfiguration(ScannedGenericBeanDefinition beanDefinition, String annotationType) {
 		try {
 			this.annotationType = annotationType;

src/main/java/org/springframework/data/gemfire/function/config/ServerBasedExecutionBeanDefinitionBuilder.java

@@ -44,7 +44,7 @@ abstract class ServerBasedExecutionBeanDefinitionBuilder extends AbstractFunctio
 		String cache = (String) configuration.getAttribute("cache");
 		String pool = (String) configuration.getAttribute("pool");
 
-		Assert.state(StringUtils.hasText(cache) && !(StringUtils.hasText(pool)), String.format(
+		Assert.state(!(StringUtils.hasText(cache) && StringUtils.hasText(pool)), String.format(
 			"invalid configuration for interface %s; cannot specify both 'pool' and 'cache'",
 				configuration.getFunctionExecutionInterface().getName()));
 

src/main/java/org/springframework/data/gemfire/function/execution/AbstractFunctionExecution.java

@@ -29,22 +29,27 @@ import com.gemstone.gemfire.cache.execute.FunctionService;
 import com.gemstone.gemfire.cache.execute.ResultCollector;
 
 /**
- * Base class for * Creating a GemFire {@link Execution} using {@link FunctionService}
- * Protected setters support method chaining
- * @author  David Turanski
+ * Base class for * Creating a GemFire {@link Execution} using {@link FunctionService}.  Protected setters support
+ * method chaining.
+ *
+ * @author David Turanski
+ * @author John Blum
  */
-
 abstract class AbstractFunctionExecution {
 
 	private final static String NO_RESULT_MESSAGE = "Cannot return any result as the Function#hasResult() is false";
 
+	private long timeout;
+
+	private Function function;
+
 	protected final Log logger = LogFactory.getLog(this.getClass());
 
-	private volatile ResultCollector<?, ?> resultCollector;
 	private Object[] args;
-	private Function function;
+
+	private volatile ResultCollector<?, ?> resultCollector;
+
 	private String functionId;
-	private long timeout;
 
 	public AbstractFunctionExecution(Function function, Object... args) {
 		Assert.notNull(function, "function cannot be null");
@@ -62,22 +67,22 @@ abstract class AbstractFunctionExecution {
 	AbstractFunctionExecution() {
 	}
 
-	ResultCollector<?, ?> getCollector() {
-		return resultCollector;
-	}
-
 	Object[] getArgs() {
 		return args;
 	}
 
-	String getFunctionId() {
-		return functionId;
+	ResultCollector<?, ?> getCollector() {
+		return resultCollector;
 	}
 
 	Function getFunction() {
 		return function;
 	}
 
+	String getFunctionId() {
+		return functionId;
+	}
+
 	long getTimeout() {
 		return timeout;
 	}
@@ -88,33 +93,31 @@ abstract class AbstractFunctionExecution {
 
 	@SuppressWarnings("unchecked")
 	<T> Iterable<T> execute(Boolean returnResult) {
-		Execution execution = this.getExecution();
-		if (getKeys() != null) {
-			execution = execution.withFilter(getKeys());
-		}
-		if (getCollector() != null) {
-			execution = execution.withCollector(getCollector());
-		}
-
-		ResultCollector<?, ?> resultCollector = null;
+		Execution execution = getExecution();
 
 		execution = execution.withArgs(getArgs());
+		execution = (getCollector() == null ? execution : execution.withCollector(getCollector()));
+		execution = (getKeys() == null ? execution : execution.withFilter(getKeys()));
+
+		ResultCollector<?, ?> resultCollector;
 
 		if (isRegisteredFunction()) {
-			resultCollector = (ResultCollector<?, ?>) execution.execute(functionId);
-		} else {
-			resultCollector = (ResultCollector<?, ?>) execution.execute(function);
+			resultCollector = execution.execute(functionId);
+		}
+		else {
+			resultCollector = execution.execute(function);
+
 			if (!function.hasResult()) {
-				return (Iterable<T>) null;
+				return null;
 			}
 		}
-		
+
 		if (!returnResult) {
-			return (Iterable<T>) null;
+			return null;
 		}
 
 		if (logger.isDebugEnabled()) {
-			logger.debug("using ResultsCollector:" + resultCollector.getClass().getName());
+			logger.debug("using ResultsCollector " + resultCollector.getClass().getName());
 		}
 
 		Iterable<T> results = null;
@@ -123,41 +126,53 @@ abstract class AbstractFunctionExecution {
 			if (this.timeout > 0) {
 				try {
 					results = (Iterable<T>) resultCollector.getResult(this.timeout, TimeUnit.MILLISECONDS);
-				} catch (FunctionException e) {
-					throw new RuntimeException(e);
-				} catch (InterruptedException e) {
+				}
+				catch (FunctionException e) {
 					throw new RuntimeException(e);
 				}
-			} else {
-
+				catch (InterruptedException e) {
+					throw new RuntimeException(e);
+				}
+			}
+			else {
 				results = (Iterable<T>) resultCollector.getResult();
 			}
 
 			return replaceSingletonNullCollectionWithEmptyList(results);
-			
-		} catch (FunctionException e) {
-			//TODO: Come up with a better way to determine that the function should not return a result;
+		}
+		catch (FunctionException e) {
+			//TODO Come up with a better way to determine that the function should not return a result;
 			if (!e.getMessage().equals(NO_RESULT_MESSAGE)) {
 				throw e;
 			}
 		}
-		
+
 		return results;
 	}
 
+	@SuppressWarnings("unchecked")
 	<T> T executeAndExtract() {
-		Iterable<T> results = this.execute();
+		Iterable<T> results = execute();
+
 		if (results == null || !results.iterator().hasNext()) {
 			return null;
 		}
 
-		return results.iterator().next();
+		Object result = results.iterator().next();
+
+		if (result instanceof Throwable) {
+			throw new FunctionException(String.format("Execution of Function %1$s failed",
+				(function != null ? function.getClass().getName() : String.format("with ID '%1$s'", functionId))),
+					(Throwable) result);
+		}
+
+		return (T) result;
 	}
 
 	protected abstract Execution getExecution();
 
-	protected AbstractFunctionExecution setFunctionId(String functionId) {
-		this.functionId = functionId;
+	protected AbstractFunctionExecution setArgs(Object... args) {
+		this.args = args;
 		return this;
 	}
 
@@ -166,17 +181,8 @@ abstract class AbstractFunctionExecution {
 		return this;
 	}
 
-	protected AbstractFunctionExecution setArgs(Object... args) {
-		this.args = args;
-		return this;
-	}
-
-	protected Set<?> getKeys() {
-		return null;
-	}
-
-	protected AbstractFunctionExecution setTimeout(long timeout) {
-		this.timeout = timeout;
+	protected AbstractFunctionExecution setFunctionId(String functionId) {
+		this.functionId = functionId;
 		return this;
 	}
 
@@ -185,29 +191,33 @@ abstract class AbstractFunctionExecution {
 		return this;
 	}
 
-	/**
-	 * @return
-	 */
+	protected AbstractFunctionExecution setTimeout(long timeout) {
+		this.timeout = timeout;
+		return this;
+	}
+
+	protected Set<?> getKeys() {
+		return null;
+	}
+
 	private boolean isRegisteredFunction() {
 		return function == null;
 	}
 
 	private <T> Iterable<T> replaceSingletonNullCollectionWithEmptyList(Iterable<T> results) {
-		if (results == null) {
-			return results;
-		}
-		Iterator<T> it = results.iterator();
+		if (results != null) {
+			Iterator<T> it = results.iterator();
 
-		if (!it.hasNext()) {
-			return results;
-		}
+			if (!it.hasNext()) {
+				return results;
+			}
 
-		if (it.next() == null && !it.hasNext()) {
-			return new ArrayList<T>();
+			if (it.next() == null && !it.hasNext()) {
+				return new ArrayList<T>();
+			}
 		}
 
 		return results;
-
 	}
 
-}
+}

src/main/java/org/springframework/data/gemfire/function/execution/AbstractFunctionTemplate.java

@@ -15,100 +15,79 @@ package org.springframework.data.gemfire.function.execution;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
-import com.gemstone.gemfire.cache.execute.Execution;
 import com.gemstone.gemfire.cache.execute.Function;
 import com.gemstone.gemfire.cache.execute.ResultCollector;
 
 /**
- * 
- * The base class for Gemfire function templates used to invoke Gemfire functions
- * @author David Turanski
+ * The base class for GemFire FunctionTemplates used to invoke GemFire Functions.
  *
+ * @author David Turanski
+ * @author John Blum
+ * @see com.gemstone.gemfire.cache.execute.Function
+ * @see com.gemstone.gemfire.cache.execute.ResultCollector
  */
-abstract class AbstractFunctionTemplate  implements GemfireFunctionOperations {
+abstract class AbstractFunctionTemplate implements GemfireFunctionOperations {
 
 	protected Log log = LogFactory.getLog(this.getClass());
-	 
+
 	protected long timeout;
+
 	protected volatile ResultCollector<?, ?> resultCollector;
-	
+
 	@Override
 	public <T> Iterable<T> execute(Function function, Object... args) {
-		 AbstractFunctionExecution functionExecution = getFunctionExecution()
-				 .setArgs(args)
-				 .setFunction(function);
-		 return execute(functionExecution);
+		 return execute(getFunctionExecution().setArgs(args).setFunction(function));
 	}
 
 	@Override
 	public <T> T executeAndExtract(Function function, Object... args) {
-		 AbstractFunctionExecution functionExecution = getFunctionExecution()
-				 .setArgs(args)
-				 .setFunction(function);
-		 		
-		return this.<T> executeAndExtract(functionExecution);		
+		return executeAndExtract(getFunctionExecution().setArgs(args).setFunction(function));
 	}
 
 	@Override
 	public <T> Iterable<T> execute(String functionId, Object... args) {
-		AbstractFunctionExecution functionExecution = getFunctionExecution()
-				 .setArgs(args)
-				 .setFunctionId(functionId);
-		 return execute(functionExecution);		 
+		return execute(getFunctionExecution().setArgs(args).setFunctionId(functionId));
 	}
-	
-	@Override 
-	public void executeWithNoResult(String functionId, Object... args) {
-		AbstractFunctionExecution functionExecution = getFunctionExecution()
-				 .setArgs(args)
-				 .setFunctionId(functionId);
-		 execute(functionExecution,false);		 
-	}
-	 
+
 	@Override
 	public <T> T executeAndExtract(String functionId, Object... args) {
-		 AbstractFunctionExecution functionExecution = getFunctionExecution()
-				 .setArgs(args)
-				 .setFunctionId(functionId);
-		return this.<T>executeAndExtract(functionExecution);		
+		return executeAndExtract(getFunctionExecution().setArgs(args).setFunctionId(functionId));
+	}
+
+	@Override
+	public void executeWithNoResult(String functionId, Object... args) {
+		execute(getFunctionExecution().setArgs(args).setFunctionId(functionId), false);
 	}
 
 	@Override
 	public <T> T execute(GemfireFunctionCallback<T> callback) {
-		Execution execution = getFunctionExecution().getExecution();
-		return callback.doInGemfire(execution);
+		return callback.doInGemfire(getFunctionExecution().getExecution());
 	}
-	 
-	
+
 	protected <T> Iterable<T> execute(AbstractFunctionExecution execution) {
-		 execution.setTimeout(timeout)
-		 .setResultCollector(resultCollector);
-		 return execution.execute();
+		 return execution.setTimeout(timeout).setResultCollector(resultCollector).execute();
 	}
 	
 	protected <T> Iterable<T> execute(AbstractFunctionExecution execution, boolean returnResult) {
-		 execution.setTimeout(timeout)
-		 .setResultCollector(resultCollector);
-		 return execution.execute(returnResult);
+		 return execution.setTimeout(timeout).setResultCollector(resultCollector).execute(returnResult);
 	}
-	
+
 	protected <T> T executeAndExtract(AbstractFunctionExecution execution) {
-		 execution.setTimeout(timeout)
-		 .setResultCollector(resultCollector);
-		 return execution.<T>executeAndExtract();
+		 return execution.setTimeout(timeout).setResultCollector(resultCollector).executeAndExtract();
 	}
-	
-	public void setTimeout(long timeout) {
-		this.timeout = timeout;
-	}
-	
+
 	public void setResultCollector(ResultCollector<?,?> resultCollector) {
 		this.resultCollector = resultCollector;
 	}
-	
+
 	public ResultCollector<?,?> getResultCollector() {
 		return this.resultCollector;
 	}
-	
+
+	public void setTimeout(long timeout) {
+		this.timeout = timeout;
+	}
+
 	protected abstract AbstractFunctionExecution getFunctionExecution();
+
 }

src/main/java/org/springframework/data/gemfire/function/execution/GemfireFunctionProxyFactoryBean.java

@@ -75,9 +75,8 @@ public class GemfireFunctionProxyFactoryBean implements FactoryBean<Object>, Met
 
 	@Override
 	public Object invoke(MethodInvocation invocation) throws Throwable {
-
 		if (AopUtils.isToStringMethod(invocation.getMethod())) {
-			return "Gemfire function proxy for service interface [" + this.functionExecutionInterface + "]";
+			return "GemFire Function Proxy for service interface [" + this.functionExecutionInterface + "]";
 		}
 
 		if (logger.isDebugEnabled()) {

src/main/java/org/springframework/data/gemfire/function/execution/GemfireOnServerFunctionTemplate.java

@@ -18,30 +18,26 @@ import com.gemstone.gemfire.cache.client.Pool;
 
 /**
  * @author David Turanski
- *
+ * @author John Blum
  */
 public class GemfireOnServerFunctionTemplate  extends AbstractFunctionTemplate {
-	
-	private final RegionService cache;
-	private final Pool pool;
 
-  
-	public GemfireOnServerFunctionTemplate (RegionService cache) {
+	private final Pool pool;
+	private final RegionService cache;
+
+	public GemfireOnServerFunctionTemplate(RegionService cache) {
 		this.cache = cache;
 		this.pool = null;
 	}
 	
-	public GemfireOnServerFunctionTemplate (Pool pool) {
-		this.pool = pool;
+	public GemfireOnServerFunctionTemplate(Pool pool) {
 		this.cache = null;
+		this.pool = pool;
 	}
- 
+
 	@Override
 	protected AbstractFunctionExecution getFunctionExecution() {
-		 if (this.pool == null) {
-			 return new ServerFunctionExecution(this.cache);
-		 }
-		 return new PoolServerFunctionExecution(this.pool);
+		return (pool != null ? new PoolServerFunctionExecution(this.pool) : new ServerFunctionExecution(this.cache));
 	}
 	 
 }

src/main/java/org/springframework/data/gemfire/function/execution/ServerFunctionExecution.java

@@ -24,20 +24,17 @@ import com.gemstone.gemfire.cache.execute.FunctionService;
  *
  */
 class ServerFunctionExecution extends AbstractFunctionExecution {
-	
 
-	private RegionService regionService;
+	private final RegionService regionService;
 
-  
 	public ServerFunctionExecution(RegionService regionService) {
-		super();
-		Assert.notNull(regionService,"regionService cannot be null");
+		Assert.notNull(regionService, "RegionService must not be null");
 		this.regionService = regionService; 
 	}
-	
- 
+
 	@Override
 	protected Execution getExecution() {
 		return FunctionService.onServer(this.regionService);
 	}
+
 }