On Tue, Nov 11, 2008 at 3:42 AM, Willem Jiang <[EMAIL PROTECTED]> wrote: > Hi Claus, > > Thanks for reviewing the code, I will change to throw the > IllegalArgumentException. > > For the "if (username == null || password == null)" , I need to exclude > the saturation of username and password are all empty.
Ah that was it. Would you mind adding a code comment on this in the java file? Then we know its on purpose. > > Willem > > Claus Ibsen wrote: >> Hi >> >> I think you should throw an IllegalArgumentException instead of plain >> CamelException. >> >> Also I am wondering if we should push for ObjectHelper.notNull or >> other similar feature to test for mandatory options. So we do it >> consistent. >> >> Also I am a bit puzzled with the else statement and the ifs. >> I would write the 2nd if as: >> if (username == null || password == null) >> >> However why not throw a specific error message so end user know if its >> the username or password that is missing. >> I would write it as >> if (username == null) >> throw new IAE(Username is not set) >> if (password == null) >> throw new IAE(password is not set) >> >> But do you break anything? As now it looks like username and password >> is mandatory. Well I can only see a small part of the code. I am sure >> are on top of it ;) >> >> On Mon, Nov 10, 2008 at 3:10 PM, <[EMAIL PROTECTED]> wrote: >>> Author: ningjiang >>> Date: Mon Nov 10 06:10:16 2008 >>> New Revision: 712662 >>> >>> URL: http://svn.apache.org/viewvc?rev=712662&view=rev >>> Log: >>> CAMEL-246 allow the username and password to be specified on the >>> JMSComponent / JMSConfiguration >>> >>> Modified: >>> >>> activemq/camel/trunk/components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsComponent.java >>> >>> activemq/camel/trunk/components/camel-jms/src/test/java/org/apache/camel/component/jms/JmsEndpointConfigurationTest.java >>> >>> Modified: >>> activemq/camel/trunk/components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsComponent.java >>> URL: >>> http://svn.apache.org/viewvc/activemq/camel/trunk/components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsComponent.java?rev=712662&r1=712661&r2=712662&view=diff >>> ============================================================================== >>> --- >>> activemq/camel/trunk/components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsComponent.java >>> (original) >>> +++ >>> activemq/camel/trunk/components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsComponent.java >>> Mon Nov 10 06:10:16 2008 >>> @@ -23,6 +23,7 @@ >>> import javax.jms.Session; >>> >>> import org.apache.camel.CamelContext; >>> +import org.apache.camel.CamelException; >>> import org.apache.camel.Endpoint; >>> import org.apache.camel.HeaderFilterStrategyAware; >>> import org.apache.camel.component.jms.requestor.Requestor; >>> @@ -36,6 +37,7 @@ >>> import org.springframework.context.ApplicationContextAware; >>> import org.springframework.core.task.TaskExecutor; >>> import org.springframework.jms.connection.JmsTransactionManager; >>> +import >>> org.springframework.jms.connection.UserCredentialsConnectionFactoryAdapter; >>> import org.springframework.jms.core.JmsOperations; >>> import org.springframework.jms.support.converter.MessageConverter; >>> import org.springframework.jms.support.destination.DestinationResolver; >>> @@ -402,6 +404,20 @@ >>> if (selector != null) { >>> endpoint.setSelector(selector); >>> } >>> + String username = getAndRemoveParameter(parameters, "username", >>> String.class); >>> + String password = getAndRemoveParameter(parameters, "password", >>> String.class); >>> + if (username != null && password != null) { >>> + ConnectionFactory cf = >>> endpoint.getConfiguration().getConnectionFactory(); >>> + UserCredentialsConnectionFactoryAdapter ucfa = new >>> UserCredentialsConnectionFactoryAdapter(); >>> + ucfa.setTargetConnectionFactory(cf); >>> + ucfa.setPassword(password); >>> + ucfa.setUsername(username); >>> + endpoint.getConfiguration().setConnectionFactory(ucfa); >>> + } else { >>> + if (username != null || password != null) { >>> + throw new CamelException("The JmsComponent's username or >>> password is null"); >>> + } >>> + } >>> setProperties(endpoint.getConfiguration(), parameters); >>> return endpoint; >>> } >>> >>> Modified: >>> activemq/camel/trunk/components/camel-jms/src/test/java/org/apache/camel/component/jms/JmsEndpointConfigurationTest.java >>> URL: >>> http://svn.apache.org/viewvc/activemq/camel/trunk/components/camel-jms/src/test/java/org/apache/camel/component/jms/JmsEndpointConfigurationTest.java?rev=712662&r1=712661&r2=712662&view=diff >>> ============================================================================== >>> --- >>> activemq/camel/trunk/components/camel-jms/src/test/java/org/apache/camel/component/jms/JmsEndpointConfigurationTest.java >>> (original) >>> +++ >>> activemq/camel/trunk/components/camel-jms/src/test/java/org/apache/camel/component/jms/JmsEndpointConfigurationTest.java >>> Mon Nov 10 06:10:16 2008 >>> @@ -22,9 +22,12 @@ >>> >>> import org.apache.activemq.ActiveMQConnectionFactory; >>> import org.apache.camel.CamelContext; >>> +import org.apache.camel.CamelException; >>> import org.apache.camel.ContextTestSupport; >>> import org.apache.camel.Exchange; >>> import org.apache.camel.Processor; >>> +import org.apache.camel.ResolveEndpointFailedException; >>> +import >>> org.springframework.jms.connection.UserCredentialsConnectionFactoryAdapter; >>> import org.springframework.jms.core.JmsOperations; >>> import org.springframework.jms.core.JmsTemplate; >>> import org.springframework.jms.listener.AbstractMessageListenerContainer; >>> @@ -52,6 +55,31 @@ >>> JmsEndpoint endpoint = (JmsEndpoint) >>> resolveMandatoryEndpoint("jms:topic:Foo.Bar?durableSubscriptionName=James&clientId=ABC"); >>> assertDurableSubscriberEndpointIsValid(endpoint); >>> } >>> + >>> + public void testSetUsernameAndPassword() throws Exception { >>> + JmsEndpoint endpoint = (JmsEndpoint) >>> resolveMandatoryEndpoint("jms:topic:Foo.Bar?username=James&password=ABC"); >>> + ConnectionFactory cf = >>> endpoint.getConfiguration().getConnectionFactory(); >>> + assertNotNull("The connectionFactory should not be null", cf); >>> + assertTrue("The connectionFactory should be the instance of >>> UserCredentialsConnectionFactoryAdapter", >>> + cf instanceof UserCredentialsConnectionFactoryAdapter); >>> + } >>> + >>> + public void testNotSetUsernameOrPassword() { >>> + try { >>> + JmsEndpoint endpoint = (JmsEndpoint) >>> resolveMandatoryEndpoint("jms:topic:Foo.Bar?username=James"); >>> + fail("Expect the exception here"); >>> + } catch (ResolveEndpointFailedException exception) { >>> + // Expect the exception here >>> + } >>> + >>> + try { >>> + JmsEndpoint endpoint = (JmsEndpoint) >>> resolveMandatoryEndpoint("jms:topic:Foo.Bar?password=ABC"); >>> + fail("Expect the exception here"); >>> + } catch (ResolveEndpointFailedException exception) { >>> + // Expect the exception here >>> + } >>> + >>> + } >>> >>> public void testSelector() throws Exception { >>> JmsEndpoint endpoint = (JmsEndpoint) >>> resolveMandatoryEndpoint("jms:Foo.Bar?selector=foo%3D'ABC'"); >>> >>> >>> >> >> >> > > -- /Claus Ibsen Apache Camel Committer Blog: http://davsclaus.blogspot.com/
