Re: svn commit: r1164334 - in /camel/trunk/components: camel-cache/src/test/java/org/apache/camel/component/cache/ camel-exec/src/main/java/org/apache/camel/component/exec/ camel-exec/src/test/java/org/apache/camel/component/exec/

[Hadrian Zbarcea]

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());