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. 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'"); >> >> >> > > >
