ARTEMIS-361 Fixing URI Encoding of Connection Factory properties

https://issues.apache.org/jira/browse/ARTEMIS-361

artemis-commons/src/main/java/org/apache/activemq/artemis/utils/uri/URISchema.java

@@ -21,6 +21,7 @@ import java.io.UnsupportedEncodingException;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.net.URLDecoder;
+import java.net.URLEncoder;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -96,7 +97,16 @@ public abstract class URISchema<T, P> {
 
    protected abstract T internalNewObject(URI uri, Map<String, String> query, P param) throws Exception;
 
-   protected abstract URI internalNewURI(T bean) throws Exception;
+   /** This is the default implementation.
+    *  Sub classes are should provide a proper implementation for their schemas. */
+   protected URI internalNewURI(T bean) throws Exception {
+      String query = URISchema.getData(null, bean);
+
+      return new URI(getSchemaName(),
+          null,
+          "//", query, null);
+
+   }
 
    private static final BeanUtilsBean beanUtils = new BeanUtilsBean();
 
@@ -106,6 +116,14 @@ public abstract class URISchema<T, P> {
       }
    }
 
+   public static String decodeURI(String value) throws UnsupportedEncodingException {
+      return URLDecoder.decode(value, "UTF-8");
+   }
+
+   public static String encodeURI(String value) throws UnsupportedEncodingException {
+      return URLEncoder.encode(value, "UTF-8");
+   }
+
    static {
       // This is to customize the BeanUtils to use Fluent Proeprties as well
       beanUtils.getPropertyUtils().addBeanIntrospector(new FluentPropertyBeanIntrospectorWithIgnores());
@@ -120,8 +138,8 @@ public abstract class URISchema<T, P> {
             for (int i = 0; i < parameters.length; i++) {
                int p = parameters[i].indexOf("=");
                if (p >= 0) {
-                  String name = URLDecoder.decode(parameters[i].substring(0, p), "UTF-8");
+                  String name = decodeURI(parameters[i].substring(0, p));
-                  String value = URLDecoder.decode(parameters[i].substring(p + 1), "UTF-8");
+                  String value = decodeURI(parameters[i].substring(p + 1));
                   rc.put(name, value);
                }
                else {
@@ -193,6 +211,7 @@ public abstract class URISchema<T, P> {
 
    public static String getData(List<String> ignored, Object... beans) throws Exception {
       StringBuilder sb = new StringBuilder();
+      boolean empty = true;
       synchronized (beanUtils) {
          for (Object bean : beans) {
             if (bean != null) {
@@ -201,7 +220,11 @@ public abstract class URISchema<T, P> {
                   if (descriptor.getReadMethod() != null && isWriteable(descriptor, ignored)) {
                      String value = beanUtils.getProperty(bean, descriptor.getName());
                      if (value != null) {
-                        sb.append("&").append(descriptor.getName()).append("=").append(value);
+                        if (!empty) {
+                           sb.append("&");
+                        }
+                        empty = false;
+                        sb.append(descriptor.getName()).append("=").append(encodeURI(value));
                      }
                   }
                }

artemis-commons/src/test/java/org/apache/activemq/artemis/utils/URIParserTest.java

@@ -44,6 +44,29 @@ public class URIParserTest {
       Assert.assertEquals("green", fruit.getColor());
       Assert.assertEquals("something", fruit.getFluentName());
 
+   }
+
+   /**
+    * this is just a simple test to validate the model
+    *
+    * @throws Throwable
+    */
+   @Test
+   public void testGenerateWithEncoding() throws Throwable {
+      FruitParser parser = new FruitParser();
+      Fruit myFruit = new Fruit("tomato&fruit");
+      myFruit.setHost("somehost&uui");
+      // I'm trying to break things as you can see here with some weird encoding
+      myFruit.setFluentName("apples&bananas with &host=3344");
+      URI uri = parser.createSchema("fruit", myFruit);
+
+      Fruit newFruit = (Fruit)parser.newObject(uri, "something");
+
+      Assert.assertEquals(myFruit.getHost(), newFruit.getHost());
+      Assert.assertEquals(myFruit.getFluentName(), newFruit.getFluentName());
+
+
+
    }
 
    /**
@@ -96,10 +119,6 @@ public class URIParserTest {
          return setData(uri, new Fruit(getSchemaName()), query);
       }
 
-      @Override
-      protected URI internalNewURI(FruitBase bean) {
-         return null;
-      }
    }
 
    class FruitBaseSchema extends URISchema<FruitBase, String> {
@@ -113,11 +132,6 @@ public class URIParserTest {
       public FruitBase internalNewObject(URI uri, Map<String, String> query, String fruitName) throws Exception {
          return setData(uri, new FruitBase(getSchemaName()), query);
       }
-
-      @Override
-      protected URI internalNewURI(FruitBase bean) {
-         return null;
-      }
    }
 
    public static class FruitBase {

artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ServerLocatorImpl.java

@@ -1630,6 +1630,14 @@ public final class ServerLocatorImpl implements ServerLocatorInternal, Discovery
       topology.removeClusterTopologyListener(listener);
    }
 
+   /**
+    * for tests only and not part of the public interface. Do not use it.
+    * @return
+    */
+   public TransportConfiguration[] getInitialConnectors() {
+      return initialConnectors;
+   }
+
    private void addFactory(ClientSessionFactoryInternal factory) {
       if (factory == null) {
          return;

artemis-core-client/src/main/java/org/apache/activemq/artemis/uri/schema/serverLocator/TCPServerLocatorSchema.java

@@ -102,19 +102,30 @@ public class TCPServerLocatorSchema extends AbstractServerLocatorSchema {
       return params.get("host") != null ? (String) params.get("host") : "localhost";
    }
 
-   private static String createQuery(Map<String, Object> params, String query) {
+   private static String createQuery(Map<String, Object> params, String query) throws Exception {
       StringBuilder cb;
+      boolean empty;
       if (query == null) {
          cb = new StringBuilder();
+         empty = true;
       }
       else {
          cb = new StringBuilder(query);
+         empty = false;
       }
-      for (String param : params.keySet()) {
+
-         if (cb.length() > 0) {
+      for (Map.Entry<String, Object> entry : params.entrySet()) {
+         if (entry.getValue() != null) {
+            if (!empty) {
                cb.append("&");
             }
-         cb.append(param).append("=").append(params.get(param));
+            else {
+               empty = false;
+            }
+            cb.append(encodeURI(entry.getKey()));
+            cb.append("=");
+            cb.append(encodeURI(entry.getValue().toString()));
+         }
       }
       return cb.toString();
    }

artemis-jms-client/src/test/java/org/apache/activemq/artemis/uri/ConnectionFactoryURITest.java

@@ -38,6 +38,8 @@ import org.apache.activemq.artemis.api.core.TransportConfiguration;
 import org.apache.activemq.artemis.api.core.UDPBroadcastEndpointFactory;
 import org.apache.activemq.artemis.api.jms.ActiveMQJMSClient;
 import org.apache.activemq.artemis.api.jms.JMSFactoryType;
+import org.apache.activemq.artemis.core.client.impl.ServerLocatorImpl;
+import org.apache.activemq.artemis.core.remoting.impl.netty.NettyConnector;
 import org.apache.activemq.artemis.core.remoting.impl.netty.NettyConnectorFactory;
 import org.apache.activemq.artemis.core.remoting.impl.netty.TransportConstants;
 import org.apache.activemq.artemis.jms.client.ActiveMQConnectionFactory;
@@ -78,6 +80,32 @@ public class ConnectionFactoryURITest {
       }
    }
 
+   @Test
+   public void testWeirdEncodingsOnIP() throws Exception {
+      // This is to make things worse. Having & and = on the property shouldn't break it
+      final String BROKEN_PROPERTY = "e80::56ee:75ff:fe53:e6a7%25enp0s25&host=[fe80::56ee:75ff:fe53:e6a7]#";
+
+      Map<String, Object> params = new HashMap<>();
+      params.put(TransportConstants.LOCAL_ADDRESS_PROP_NAME, BROKEN_PROPERTY);
+
+      TransportConfiguration configuration = new TransportConfiguration(NettyConnector.class.getName(), params);
+
+
+      ActiveMQConnectionFactory factory = ActiveMQJMSClient.createConnectionFactoryWithHA(JMSFactoryType.CF, configuration);
+
+      URI uri = factory.toURI();
+
+      ActiveMQConnectionFactory newFactory = ActiveMQJMSClient.createConnectionFactory(uri.toString(), "somefactory");
+
+
+      TransportConfiguration[] initialConnectors = ((ServerLocatorImpl)newFactory.getServerLocator()).getInitialConnectors();
+
+      Assert.assertEquals(1, initialConnectors.length);
+
+
+      Assert.assertEquals(BROKEN_PROPERTY, initialConnectors[0].getParams().get(TransportConstants.LOCAL_ADDRESS_PROP_NAME).toString());
+   }
+
    @Test
    public void testIPv6NewURI() throws Exception {
       for (String IPV6 : V6IPs) {