You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@camel.apache.org by Claus Ibsen <cl...@gmail.com> on 2008/11/10 15:49:09 UTC
Re: svn commit: r712662 - in /activemq/camel/trunk/components/camel-jms/src: main/java/org/apache/camel/component/jms/JmsComponent.java test/java/org/apache/camel/component/jms/JmsEndpointConfigurationTest.java
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, <ni...@apache.org> 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/
Re: svn commit: r712662 - in /activemq/camel/trunk/components/camel-jms/src: main/java/org/apache/camel/component/jms/JmsComponent.java test/java/org/apache/camel/component/jms/JmsEndpointConfigurationTest.java
Posted by Claus Ibsen <cl...@gmail.com>.
On Tue, Nov 11, 2008 at 3:42 AM, Willem Jiang <wi...@gmail.com> 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, <ni...@apache.org> 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/
Re: svn commit: r712662 - in /activemq/camel/trunk/components/camel-jms/src:
main/java/org/apache/camel/component/jms/JmsComponent.java test/java/org/apache/camel/component/jms/JmsEndpointConfigurationTest.java
Posted by Willem Jiang <wi...@gmail.com>.
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, <ni...@apache.org> 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'");
>>
>>
>>
>
>
>