Frankly people have been using Camel for 4.5 years with url's just like that without any problems.
I think its bad judgment to create JIRA tickets with "critical" bugs and mark them as fix for 2.8.1 etc, as if the sky is falling down on people. Instead as you write somewhere, it could be put as a goal for Camel 3.0 to have URL validation more strict, and to start throwing exception if the uris is not 100% according to that spec. But for 2.x that is 2+ years old and in light of this is how Camel has been doing it since its birth, eg 4.5 years then it would IMHO be better to keep it as is in 1.x/2.x and put up for 3.0 roadmap instead. On Fri, Sep 2, 2011 at 3:50 PM, Hadrian Zbarcea <hzbar...@gmail.com> wrote: > Still not sure how to answer, but let me try. The issue(s) I created and > partially fixed are related to passing invalid URIs to Camel to create > endpoints. I could understand the questions in the following ways: > > 1. I don't see any problem. > 2. I see a problem, but: > a. No need to fix it because it worked like that for a long time > b. It needs to be fixed, but it's not critical > > 1. From the beginning of the project we said that Camel uses URIs for both > identifying and configuring endpoint. All the presentations, books, > webinars, you name it said the same thing. What represents a URI is clearly > defined [1] and unambiguous. We never said that we take something that looks > like a URI but it's not a URI. As such we would expect the users to pass in > valid URIs, nothing new there. > The fact that in DefaultComponent.createEndpoint(String) we use > UnsafeUriCharactersEncoder.encode when creating/parsing the URI was a > mistake that hid the issue. Even worse, we designed components (see 2b) that > *only* accept invalid URIs for some scenarios. How retarded it that? > > 2a. This logic applied to any bug with see. Camel worked for so many years > with this bug, why fix it now? > > 2b. Designing components in ways that only work with invalid data (URIs) is > a critical design issue, imho. Another reason I made it critical is to draw > attention (which it did, that's good). Please note that I mentioned that we > need to find a fix (I have one) *and* workaround that does not force users > to fix their URIs immediately, and the usage of bad URIs should be > deprecated and removed in 3.0 > > I hope that clarifies it, > Hadrian > > [1] http://www.ietf.org/rfc/rfc2396.txt > > > On 09/02/2011 08:32 AM, Claus Ibsen wrote: >> >> On Fri, Sep 2, 2011 at 2:20 PM, Hadrian Zbarcea<hzbar...@gmail.com> >> wrote: >>> >>> Not sure I understand your request, please clarify. >>> >> >> Can you post on @dev what you see as problems with the endpoint uris. >> You create a number of tickets and mark then as critical bugs, without >> much details. People >> have been using this without of problems for years. >> >> You then change many unit tests to url encode, also tests which are >> used for wiki documentation. >> Which makes it a bit confusing for end users, as there is no >> documentation why they have to do this now. >> >> For example when they use camel-exec >> >> >> >>> Hadrian >>> >>> >>> On 09/02/2011 01:59 AM, Claus Ibsen wrote: >>>> >>>> This seems over reacting. >>>> >>>> Can you post on @dev why you suddenly change all the working unit >>>> tests like this? >>>> Camel end users have been using these components without problems, and >>>> they are easier to use without having to do URL encoding! >>>> >>>> >>>> On Fri, Sep 2, 2011 at 4:36 AM,<hadr...@apache.org> wrote: >>>>> >>>>> Author: hadrian >>>>> Date: Fri Sep 2 02:36:12 2011 >>>>> New Revision: 1164334 >>>>> >>>>> URL: http://svn.apache.org/viewvc?rev=1164334&view=rev >>>>> Log: >>>>> More URI encoding fixes >>>>> >>>>> Modified: >>>>> >>>>> >>>>> camel/trunk/components/camel-cache/src/test/java/org/apache/camel/component/cache/CacheProducerTest.java >>>>> >>>>> >>>>> camel/trunk/components/camel-exec/src/main/java/org/apache/camel/component/exec/ExecComponent.java >>>>> >>>>> >>>>> camel/trunk/components/camel-exec/src/test/java/org/apache/camel/component/exec/ExecEndpointTest.java >>>>> >>>>> Modified: >>>>> >>>>> camel/trunk/components/camel-cache/src/test/java/org/apache/camel/component/cache/CacheProducerTest.java >>>>> URL: >>>>> >>>>> http://svn.apache.org/viewvc/camel/trunk/components/camel-cache/src/test/java/org/apache/camel/component/cache/CacheProducerTest.java?rev=1164334&r1=1164333&r2=1164334&view=diff >>>>> >>>>> >>>>> ============================================================================== >>>>> --- >>>>> >>>>> camel/trunk/components/camel-cache/src/test/java/org/apache/camel/component/cache/CacheProducerTest.java >>>>> (original) >>>>> +++ >>>>> >>>>> camel/trunk/components/camel-cache/src/test/java/org/apache/camel/component/cache/CacheProducerTest.java >>>>> Fri Sep 2 02:36:12 2011 >>>>> @@ -133,7 +133,7 @@ public class CacheProducerTest extends C >>>>> public void configure() { >>>>> onException(CacheException.class). >>>>> handled(true). >>>>> - to("log:*** LOGGER"). >>>>> + to("log:LOGGER"). >>>>> to("mock:CacheProducerTest.cacheException"); >>>>> >>>>> from("direct:a"). >>>>> @@ -185,7 +185,7 @@ public class CacheProducerTest extends C >>>>> public void configure() { >>>>> onException(CacheException.class). >>>>> handled(true). >>>>> - to("log:*** LOGGER"). >>>>> + to("log:LOGGER"). >>>>> to("mock:CacheProducerTest.cacheException"); >>>>> >>>>> from("direct:a"). >>>>> @@ -222,7 +222,7 @@ public class CacheProducerTest extends C >>>>> public void configure() { >>>>> onException(CacheException.class). >>>>> handled(true). >>>>> - to("log:*** LOGGER"). >>>>> + to("log:LOGGER"). >>>>> to("mock:CacheProducerTest.cacheException"); >>>>> >>>>> from("direct:a"). >>>>> @@ -258,7 +258,7 @@ public class CacheProducerTest extends C >>>>> public void configure() { >>>>> onException(CacheException.class). >>>>> handled(true). >>>>> - to("log:*** LOGGER"). >>>>> + to("log:LOGGER"). >>>>> to("mock:CacheProducerTest.cacheException"); >>>>> >>>>> from("direct:a"). >>>>> @@ -279,7 +279,7 @@ public class CacheProducerTest extends C >>>>> public void configure() { >>>>> onException(CacheException.class). >>>>> handled(true). >>>>> - to("log:*** LOGGER"). >>>>> + to("log:LOGGER"). >>>>> to("mock:CacheProducerTest.cacheException"); >>>>> >>>>> from("direct:a"). >>>>> @@ -305,7 +305,7 @@ public class CacheProducerTest extends C >>>>> onException(CacheException.class). >>>>> handled(true). >>>>> >>>>> >>>>> choice().when(exceptionMessage().isEqualTo(CacheConstants.CACHE_OPERATION >>>>> + >>>>> " UNKNOWN is not supported.")). >>>>> - to("log:*** LOGGER"). >>>>> + to("log:LOGGER"). >>>>> >>>>> to("mock:CacheProducerTest.cacheException").end(); >>>>> >>>>> from("direct:a"). >>>>> @@ -332,7 +332,7 @@ public class CacheProducerTest extends C >>>>> public void configure() { >>>>> onException(CacheException.class). >>>>> handled(true). >>>>> - to("log:*** LOGGER"). >>>>> + to("log:LOGGER"). >>>>> to("mock:CacheProducerTest.cacheException"); >>>>> >>>>> from("direct:a"). >>>>> @@ -360,7 +360,7 @@ public class CacheProducerTest extends C >>>>> public void configure() { >>>>> onException(CacheException.class). >>>>> handled(true). >>>>> - to("log:*** LOGGER"). >>>>> + to("log:LOGGER"). >>>>> to("mock:CacheProducerTest.cacheException"); >>>>> >>>>> from("direct:a"). >>>>> @@ -388,7 +388,7 @@ public class CacheProducerTest extends C >>>>> public void configure() { >>>>> onException(CacheException.class). >>>>> handled(true). >>>>> - to("log:*** LOGGER"). >>>>> + to("log:LOGGER"). >>>>> to("mock:CacheProducerTest.cacheException"); >>>>> >>>>> from("direct:a"). >>>>> >>>>> Modified: >>>>> >>>>> camel/trunk/components/camel-exec/src/main/java/org/apache/camel/component/exec/ExecComponent.java >>>>> URL: >>>>> >>>>> http://svn.apache.org/viewvc/camel/trunk/components/camel-exec/src/main/java/org/apache/camel/component/exec/ExecComponent.java?rev=1164334&r1=1164333&r2=1164334&view=diff >>>>> >>>>> >>>>> ============================================================================== >>>>> --- >>>>> >>>>> camel/trunk/components/camel-exec/src/main/java/org/apache/camel/component/exec/ExecComponent.java >>>>> (original) >>>>> +++ >>>>> >>>>> camel/trunk/components/camel-exec/src/main/java/org/apache/camel/component/exec/ExecComponent.java >>>>> Fri Sep 2 02:36:12 2011 >>>>> @@ -16,6 +16,7 @@ >>>>> */ >>>>> package org.apache.camel.component.exec; >>>>> >>>>> +import java.net.URLDecoder; >>>>> import java.util.Map; >>>>> >>>>> import org.apache.camel.Endpoint; >>>>> @@ -31,7 +32,7 @@ public class ExecComponent extends Defau >>>>> protected Endpoint createEndpoint(String uri, String remaining, >>>>> Map<String, Object> parameters) throws Exception { >>>>> ExecEndpoint endpoint = new ExecEndpoint(uri, this); >>>>> setProperties(endpoint, parameters); >>>>> - endpoint.setExecutable(remaining); >>>>> + endpoint.setExecutable(URLDecoder.decode(remaining, "UTF-8")); >>>>> return endpoint; >>>>> } >>>>> } >>>>> >>>>> Modified: >>>>> >>>>> camel/trunk/components/camel-exec/src/test/java/org/apache/camel/component/exec/ExecEndpointTest.java >>>>> URL: >>>>> >>>>> http://svn.apache.org/viewvc/camel/trunk/components/camel-exec/src/test/java/org/apache/camel/component/exec/ExecEndpointTest.java?rev=1164334&r1=1164333&r2=1164334&view=diff >>>>> >>>>> >>>>> ============================================================================== >>>>> --- >>>>> >>>>> camel/trunk/components/camel-exec/src/test/java/org/apache/camel/component/exec/ExecEndpointTest.java >>>>> (original) >>>>> +++ >>>>> >>>>> camel/trunk/components/camel-exec/src/test/java/org/apache/camel/component/exec/ExecEndpointTest.java >>>>> Fri Sep 2 02:36:12 2011 >>>>> @@ -18,6 +18,7 @@ package org.apache.camel.component.exec; >>>>> >>>>> import org.apache.camel.CamelContext; >>>>> import org.apache.camel.Component; >>>>> +import org.apache.camel.util.UnsafeUriCharactersEncoder; >>>>> >>>>> import org.junit.Before; >>>>> import org.junit.Test; >>>>> @@ -102,7 +103,8 @@ public class ExecEndpointTest extends Ab >>>>> @DirtiesContext >>>>> public void testCreateEndpointWithArgs() throws Exception { >>>>> String args = "arg1 arg2 arg3"; >>>>> - ExecEndpoint e = createExecEndpoint("exec:test?args=" + args); >>>>> + // Need to properly encode the URI >>>>> + ExecEndpoint e = createExecEndpoint("exec:test?args=" + >>>>> args.replaceAll(" ", "+")); >>>>> assertEquals(args, e.getArgs()); >>>>> } >>>>> >>>>> @@ -110,7 +112,7 @@ public class ExecEndpointTest extends Ab >>>>> @DirtiesContext >>>>> public void testCreateEndpointWithArgs2() throws Exception { >>>>> String args = "arg1 \"arg2 \" arg3"; >>>>> - ExecEndpoint e = createExecEndpoint("exec:test?args=" + args); >>>>> + ExecEndpoint e = createExecEndpoint("exec:test?args=" + >>>>> UnsafeUriCharactersEncoder.encode(args)); >>>>> assertEquals(args, e.getArgs()); >>>>> } >>>>> >>>>> @@ -145,7 +147,7 @@ public class ExecEndpointTest extends Ab >>>>> String dir = "\"c:/program files/wokr/temp\""; >>>>> String uri = "exec:" + cmd + "?workingDir=" + dir; >>>>> >>>>> - ExecEndpoint endpoint = createExecEndpoint(uri); >>>>> + ExecEndpoint endpoint = >>>>> createExecEndpoint(UnsafeUriCharactersEncoder.encode(uri)); >>>>> assertEquals(cmd, endpoint.getExecutable()); >>>>> assertNull(endpoint.getArgs()); >>>>> assertNotNull(endpoint.getCommandExecutor()); >>>>> @@ -159,7 +161,7 @@ public class ExecEndpointTest extends Ab >>>>> String executable = "C:/Program Files/test/text.exe"; >>>>> String uri = "exec:" + executable; >>>>> >>>>> - ExecEndpoint endpoint = createExecEndpoint(uri); >>>>> + ExecEndpoint endpoint = >>>>> createExecEndpoint(UnsafeUriCharactersEncoder.encode(uri)); >>>>> >>>>> assertNull(endpoint.getArgs()); >>>>> assertNull(endpoint.getWorkingDir()); >>>>> @@ -175,7 +177,8 @@ public class ExecEndpointTest extends Ab >>>>> String argsEscaped = "arg1 arg2 \"arg 3\""; >>>>> long timeout = 10000L; >>>>> >>>>> - ExecEndpoint e = >>>>> createExecEndpoint("exec:executable.exe?workingDir=" + workingDir + >>>>> "&timeout=" + timeout + "&args=" + argsEscaped); >>>>> + String uri = "exec:executable.exe?workingDir=" + workingDir + >>>>> "&timeout=" + timeout + "&args=" + argsEscaped; >>>>> + ExecEndpoint e = >>>>> createExecEndpoint(UnsafeUriCharactersEncoder.encode(uri)); >>>>> assertEquals(workingDir, e.getWorkingDir()); >>>>> assertEquals(argsEscaped, e.getArgs()); >>>>> assertEquals(timeout, e.getTimeout()); >>>>> @@ -192,7 +195,7 @@ public class ExecEndpointTest extends Ab >>>>> builder.append("&outFile=" + outFile); >>>>> >>>>> >>>>> builder.append("&commandExecutor=#customExecutor&binding=#customBinding"); >>>>> >>>>> - ExecEndpoint e = createExecEndpoint(builder.toString()); >>>>> + ExecEndpoint e = >>>>> >>>>> createExecEndpoint(UnsafeUriCharactersEncoder.encode(builder.toString())); >>>>> assertEquals(workingDir, e.getWorkingDir()); >>>>> assertEquals(timeout, e.getTimeout()); >>>>> assertEquals(outFile, e.getOutFile()); >>>>> >>>>> >>>>> >>>> >>>> >>>> >>> >> >> >> > -- Claus Ibsen ----------------- FuseSource Email: cib...@fusesource.com Web: http://fusesource.com Twitter: davsclaus, fusenews Blog: http://davsclaus.blogspot.com/ Author of Camel in Action: http://www.manning.com/ibsen/
