From 4508212d405411a31b2e928a7610aa650e28ca89 Mon Sep 17 00:00:00 2001 From: Gunnar Hillert Date: Tue, 11 Oct 2011 14:33:25 -0400 Subject: [PATCH] Code Review Fixes * AbstractReplyProducingMessageHandler - throw MessageHandlingException instead of MessagingException * In Parser classes reduce validation logic, where the the Xml Schema already enforces it * Better null handling in class ProcedureParameter + more unit tests --- .../jdbc/StoredProcOutboundGateway.java | 6 +- .../jdbc/StoredProcPollingChannelAdapter.java | 5 +- .../jdbc/config/StoredProcParserUtils.java | 20 ++-- .../jdbc/storedproc/ProcedureParameter.java | 12 ++- .../storedproc/ProcedureParameterTest.java | 93 +++++++++++++++++++ 5 files changed, 119 insertions(+), 17 deletions(-) create mode 100644 spring-integration-jdbc/src/test/java/org/springframework/integration/jdbc/storedproc/ProcedureParameterTest.java diff --git a/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/StoredProcOutboundGateway.java b/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/StoredProcOutboundGateway.java index 87e434d8d6..193171df95 100644 --- a/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/StoredProcOutboundGateway.java +++ b/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/StoredProcOutboundGateway.java @@ -21,8 +21,8 @@ import java.util.Map; import javax.sql.DataSource; -import org.springframework.beans.factory.InitializingBean; import org.springframework.integration.Message; +import org.springframework.integration.MessageHandlingException; import org.springframework.integration.MessagingException; import org.springframework.integration.handler.AbstractReplyProducingMessageHandler; import org.springframework.integration.jdbc.storedproc.ProcedureParameter; @@ -38,7 +38,7 @@ import org.springframework.util.Assert; * * @since 2.1 */ -public class StoredProcOutboundGateway extends AbstractReplyProducingMessageHandler implements InitializingBean { +public class StoredProcOutboundGateway extends AbstractReplyProducingMessageHandler { private final StoredProcExecutor executor; @@ -87,7 +87,7 @@ public class StoredProcOutboundGateway extends AbstractReplyProducingMessageHand payload = resultMap.values().iterator().next(); } else if (this.expectSingleResult && resultMap.size() > 1) { - throw new MessagingException( + throw new MessageHandlingException(requestMessage, "Stored Procedure/Function call returned more than " + "1 result object and expectSingleResult was 'true'. "); diff --git a/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/StoredProcPollingChannelAdapter.java b/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/StoredProcPollingChannelAdapter.java index a004efedfb..665594301c 100644 --- a/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/StoredProcPollingChannelAdapter.java +++ b/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/StoredProcPollingChannelAdapter.java @@ -43,7 +43,7 @@ import org.springframework.util.Assert; */ public class StoredProcPollingChannelAdapter extends IntegrationObjectSupport implements MessageSource { - final StoredProcExecutor executor; + private final StoredProcExecutor executor; private volatile boolean expectSingleResult = false; @@ -117,8 +117,7 @@ public class StoredProcPollingChannelAdapter extends IntegrationObjectSupport im } protected Map doPoll() { - Map payload = this.executor.executeStoredProcedure(); - return payload; + return this.executor.executeStoredProcedure(); } public String getComponentType(){ diff --git a/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/config/StoredProcParserUtils.java b/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/config/StoredProcParserUtils.java index 592f624eb8..1de0e2ac8f 100644 --- a/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/config/StoredProcParserUtils.java +++ b/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/config/StoredProcParserUtils.java @@ -43,7 +43,7 @@ import org.w3c.dom.Element; */ public final class StoredProcParserUtils { - private static final Log logger = LogFactory + private static final Log LOGGER = LogFactory .getLog(StoredProcParserUtils.class); /** Prevent instantiation. */ @@ -134,12 +134,7 @@ public final class StoredProcParserUtils { if (StringUtils.hasText(name)) { parameterBuilder.addPropertyValue("name", name); - } else { - parserContext - .getReaderContext() - .error("The 'name' attribute must be set for the Stored Procedure parameter element.", - storedProcComponent); - } + } if (StringUtils.hasText(expression)) { parameterBuilder.addPropertyValue("expression", expression); @@ -148,9 +143,14 @@ public final class StoredProcParserUtils { if (StringUtils.hasText(value)) { if (!StringUtils.hasText(type)) { - logger.info(String - .format("Type attribute not set for Store Procedure parameter '%s'. Defaulting to 'java.lang.String'.", - value)); + + if (LOGGER.isInfoEnabled()) { + LOGGER.info(String + .format("Type attribute not set for Store " + + "Procedure parameter '%s'. Defaulting to " + + "'java.lang.String'.", value)); + } + parameterBuilder.addPropertyValue("value", new TypedStringValue(value, String.class)); diff --git a/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/storedproc/ProcedureParameter.java b/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/storedproc/ProcedureParameter.java index a634de113d..f5a1692169 100644 --- a/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/storedproc/ProcedureParameter.java +++ b/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/storedproc/ProcedureParameter.java @@ -64,7 +64,7 @@ public class ProcedureParameter { public ProcedureParameter(String name, Object value, String expression) { super(); - Assert.hasText(name, "Please provide a name."); + Assert.hasText(name, "'name' must not be empty."); this.name = name; this.value = value; @@ -99,6 +99,10 @@ public class ProcedureParameter { Assert.notNull(procedureParameters, "The Collection of procedureParameters must not be null."); + for (ProcedureParameter parameter : procedureParameters) { + Assert.notNull(parameter, "'procedureParameters' must not contain null values."); + } + Map staticParameters = new HashMap(); for (ProcedureParameter parameter : procedureParameters) { @@ -119,6 +123,12 @@ public class ProcedureParameter { */ public static Map convertStaticParameters(Collection procedureParameters) { + Assert.notNull(procedureParameters, "The Collection of procedureParameters must not be null."); + + for (ProcedureParameter parameter : procedureParameters) { + Assert.notNull(parameter, "'procedureParameters' must not contain null values."); + } + Map staticParameters = new HashMap(); for (ProcedureParameter parameter : procedureParameters) { diff --git a/spring-integration-jdbc/src/test/java/org/springframework/integration/jdbc/storedproc/ProcedureParameterTest.java b/spring-integration-jdbc/src/test/java/org/springframework/integration/jdbc/storedproc/ProcedureParameterTest.java new file mode 100644 index 0000000000..e0a54291d2 --- /dev/null +++ b/spring-integration-jdbc/src/test/java/org/springframework/integration/jdbc/storedproc/ProcedureParameterTest.java @@ -0,0 +1,93 @@ +package org.springframework.integration.jdbc.storedproc; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +import org.junit.Test; + +public class ProcedureParameterTest { + + @Test + public void testProcedureParameterStringObjectString() { + + try { + new ProcedureParameter(null, "value", "expression"); + } catch(IllegalArgumentException e) { + assertEquals("'name' must not be empty.", e.getMessage()); + return; + } + + fail("Expected Exception"); + } + + @Test + public void testConvertExpressions() { + List procedureParameters = getProcedureParameterList(); + Map expressionParameters = + ProcedureParameter.convertExpressions(procedureParameters); + + assertTrue("Expected 2 expression parameters.", expressionParameters.size() == 2); + } + + @Test + public void testConvertStaticParameters() { + + List procedureParameters = getProcedureParameterList(); + Map staticParameters = + ProcedureParameter.convertStaticParameters(procedureParameters); + + assertTrue("Expected 3 static parameters.", staticParameters.size() == 3); + } + + @Test + public void testConvertStaticParametersWithNullValueInList() { + + List procedureParameters = getProcedureParameterList(); + procedureParameters.add(1, null); + + try { + ProcedureParameter.convertStaticParameters(procedureParameters); + } catch(IllegalArgumentException e) { + assertEquals("'procedureParameters' must not contain null values.", e.getMessage()); + return; + } + + fail("Expected Exception"); + + } + + @Test + public void testConvertExpressionParametersWithNullValueInList() { + + List procedureParameters = getProcedureParameterList(); + procedureParameters.add(1, null); + + try { + ProcedureParameter.convertExpressions(procedureParameters); + } catch(IllegalArgumentException e) { + assertEquals("'procedureParameters' must not contain null values.", e.getMessage()); + return; + } + + fail("Expected Exception"); + + } + + private List getProcedureParameterList() { + + List procedureParameterList = new ArrayList(); + procedureParameterList.add(new ProcedureParameter("param1", "value1", null)); + procedureParameterList.add(new ProcedureParameter("param2", "value1", null)); + procedureParameterList.add(new ProcedureParameter("param3", "value1", null)); + procedureParameterList.add(new ProcedureParameter("param4", null, "expression1")); + procedureParameterList.add(new ProcedureParameter("param5", null, "expression2")); + + return procedureParameterList; + } + +}