JCLOUDS-1008: Add @Encoded to @QueryParam.

Adds support for the @Encoded option for the @QueryParam annotation.
The @Encoded params are not encoded, while all parameters that don't
have it are encoded. The change applies to the @QueryParam annotation
on a single parameter. There is no way to express @Encoded on the list
of parameters and their values in @QueryParams.

The big change is that query parameter encoding is now handled within
the annotation processor, as opposed to relying on the UriBuilder to
perform the encoding. This is required since the UriBuilder does not
have any information about additional annotations associated with each
of the query parameters.

Also, adds unit tests for making sure keys and values are properly
encoded when using the @QueryParams option.

core/src/main/java/org/jclouds/http/Uris.java

@@ -282,9 +282,9 @@ public final class Uris {
          return build(ImmutableMap.<String, Object> of());
       }
 
-      public URI buildNoEncoding(Map<String, ?> variables) {
+      public URI build(Map<String, ?> variables, boolean encodePath, boolean encodeQuery) {
          try {
-            return new URI(expand(variables, false));
+            return new URI(expand(variables, encodePath, encodeQuery));
          } catch (URISyntaxException e) {
             throw new IllegalArgumentException(e);
          }
@@ -296,13 +296,13 @@ public final class Uris {
        */
       public URI build(Map<String, ?> variables) {
          try {
-            return new URI(expand(variables, true));
+            return new URI(expand(variables, true, true));
          } catch (URISyntaxException e) {
             throw new IllegalArgumentException(e);
          }
       }
 
-      private String expand(Map<String, ?> variables, boolean shouldEncode) {
+      private String expand(Map<String, ?> variables, boolean encodePath, boolean encodeQuery) {
          StringBuilder b = new StringBuilder();
          if (scheme != null)
             b.append(scheme).append("://");
@@ -311,14 +311,19 @@ public final class Uris {
          if (port != null)
             b.append(':').append(port);
          if (path != null) {
-            if (shouldEncode) {
+            if (encodePath) {
                b.append(urlEncode(UriTemplates.expand(path, variables), skipPathEncoding));
             } else {
                b.append(UriTemplates.expand(path, variables));
             }
          }
-         if (!query.isEmpty())
+         if (!query.isEmpty()) {
-            b.append('?').append(encodeQueryLine(query));
+            if (encodeQuery) {
+               b.append('?').append(encodeQueryLine(query));
+            } else {
+               b.append('?').append(buildQueryLine(query));
+            }
+         }
          return b.toString();
       }
 

core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java

@@ -40,6 +40,7 @@ import static org.jclouds.http.Uris.uriBuilder;
 import static org.jclouds.io.Payloads.newPayload;
 import static org.jclouds.reflect.Reflection2.getInvokableParameters;
 import static org.jclouds.util.Strings2.replaceTokens;
+import static org.jclouds.util.Strings2.urlEncode;
 
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Array;
@@ -102,7 +103,6 @@ import org.jclouds.rest.annotations.VirtualHost;
 import org.jclouds.rest.annotations.WrapWith;
 import org.jclouds.rest.binders.BindMapToStringPayload;
 import org.jclouds.rest.binders.BindToJsonPayloadWrappedWith;
-import org.jclouds.util.Strings2;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Function;
@@ -231,10 +231,10 @@ public class RestAnnotationProcessor implements Function<Invocation, HttpRequest
 
       overridePathEncoding(uriBuilder, invocation);
 
-      boolean encodedParams = isEncodedUsed(invocation);
+      boolean encodeFullPath = !isEncodedUsed(invocation);
       if (caller != null)
-         tokenValues.putAll(addPathAndGetTokens(caller, uriBuilder, encodedParams));
+         tokenValues.putAll(addPathAndGetTokens(caller, uriBuilder, encodeFullPath));
-      tokenValues.putAll(addPathAndGetTokens(invocation, uriBuilder, encodedParams));
+      tokenValues.putAll(addPathAndGetTokens(invocation, uriBuilder, encodeFullPath));
       Multimap<String, Object> formParams;
       if (caller != null) {
          formParams = addFormParams(tokenValues, caller);
@@ -270,7 +270,8 @@ public class RestAnnotationProcessor implements Function<Invocation, HttpRequest
             headers.put(header.getKey(), replaceTokens(header.getValue(), tokenValues));
          }
          for (Entry<String, String> query : options.buildQueryParameters().entries()) {
-            queryParams.put(query.getKey(), replaceTokens(query.getValue(), tokenValues));
+            queryParams.put(urlEncode(query.getKey(), '/', ','),
+                  urlEncode(replaceTokens(query.getValue(), tokenValues), '/', ','));
          }
          for (Entry<String, String> form : options.buildFormParameters().entries()) {
             formParams.put(form.getKey(), replaceTokens(form.getValue(), tokenValues));
@@ -291,11 +292,9 @@ public class RestAnnotationProcessor implements Function<Invocation, HttpRequest
 
       requestBuilder.headers(filterOutContentHeaders(headers));
 
-      if (encodedParams) {
+      // Query parameter encoding is handled in the annotation processor
-         requestBuilder.endpoint(uriBuilder.buildNoEncoding(convertUnsafe(tokenValues)));
+      requestBuilder.endpoint(uriBuilder.build(convertUnsafe(tokenValues), /*encodePath=*/encodeFullPath,
-      } else {
+            /*encodeQuery=*/ false));
-         requestBuilder.endpoint(uriBuilder.build(convertUnsafe(tokenValues)));
-      }
 
       if (payload == null) {
          PayloadEnclosing payloadEnclosing = findOrNull(invocation.getArgs(), PayloadEnclosing.class);
@@ -395,12 +394,13 @@ public class RestAnnotationProcessor implements Function<Invocation, HttpRequest
       return endpoint;
    }
 
-   private Multimap<String, Object> addPathAndGetTokens(Invocation invocation, UriBuilder uriBuilder, boolean encoded) {
+   private Multimap<String, Object> addPathAndGetTokens(Invocation invocation, UriBuilder uriBuilder,
+                                                        boolean encodeFullPath) {
       if (invocation.getInvokable().getOwnerType().getRawType().isAnnotationPresent(Path.class))
          uriBuilder.appendPath(invocation.getInvokable().getOwnerType().getRawType().getAnnotation(Path.class).value());
       if (invocation.getInvokable().isAnnotationPresent(Path.class))
          uriBuilder.appendPath(invocation.getInvokable().getAnnotation(Path.class).value());
-      return getPathParamKeyValues(invocation, encoded);
+      return getPathParamKeyValues(invocation, encodeFullPath);
    }
 
    private Multimap<String, Object> addFormParams(Multimap<String, ?> tokenValues, Invocation invocation) {
@@ -452,11 +452,12 @@ public class RestAnnotationProcessor implements Function<Invocation, HttpRequest
 
    private void addQuery(Multimap<String, Object> queryParams, QueryParams query, Multimap<String, ?> tokenValues) {
       for (int i = 0; i < query.keys().length; i++) {
+         String key = urlEncode(query.keys()[i], '/', ',');
          if (query.values()[i].equals(QueryParams.NULL)) {
-            queryParams.removeAll(query.keys()[i]);
+            queryParams.removeAll(key);
-            queryParams.put(query.keys()[i], null);
+            queryParams.put(key, null);
          } else {
-            queryParams.put(query.keys()[i], replaceTokens(query.values()[i], tokenValues));
+            queryParams.put(key, urlEncode(replaceTokens(query.values()[i], tokenValues), '/', ','));
          }
       }
    }
@@ -768,7 +769,7 @@ public class RestAnnotationProcessor implements Function<Invocation, HttpRequest
       return !parametersWithAnnotation(invocation.getInvokable(), Encoded.class).isEmpty();
    }
 
-   private Multimap<String, Object> getPathParamKeyValues(Invocation invocation, boolean encodedParams) {
+   private Multimap<String, Object> getPathParamKeyValues(Invocation invocation, boolean encodeFullPath) {
       Multimap<String, Object> pathParamValues = LinkedHashMultimap.create();
       for (Parameter param : parametersWithAnnotation(invocation.getInvokable(), PathParam.class)) {
          PathParam pathParam = param.getAnnotation(PathParam.class);
@@ -776,8 +777,8 @@ public class RestAnnotationProcessor implements Function<Invocation, HttpRequest
          Optional<?> paramValue = getParamValue(invocation, param.getAnnotation(ParamParser.class), param.hashCode(),
                paramKey);
          if (paramValue.isPresent()) {
-            if (encodedParams && !param.isAnnotationPresent(Encoded.class)) {
+            if (!encodeFullPath && !param.isAnnotationPresent(Encoded.class)) {
-               pathParamValues.put(paramKey, Strings2.urlEncode(paramValue.get().toString()));
+               pathParamValues.put(paramKey, urlEncode(paramValue.get().toString()));
             } else {
                pathParamValues.put(paramKey, paramValue.get().toString());
             }
@@ -821,16 +822,28 @@ public class RestAnnotationProcessor implements Function<Invocation, HttpRequest
       Multimap<String, Object> queryParamValues = LinkedHashMultimap.create();
       for (Parameter param : parametersWithAnnotation(invocation.getInvokable(), QueryParam.class)) {
          QueryParam queryParam = param.getAnnotation(QueryParam.class);
-         String paramKey = queryParam.value();
+         String paramKey = urlEncode(queryParam.value(), '/', ',');
          Optional<?> paramValue = getParamValue(invocation, param.getAnnotation(ParamParser.class), param.hashCode(),
                paramKey);
          if (paramValue.isPresent())
             if (paramValue.get() instanceof Iterable) {
                @SuppressWarnings("unchecked")
                Iterable<String> iterableStrings = transform(Iterable.class.cast(paramValue.get()), toStringFunction());
+               if (!param.isAnnotationPresent(Encoded.class)) {
+                  iterableStrings = transform(iterableStrings, new Function<String, String>() {
+                     @Override
+                     public String apply(@Nullable String s) {
+                        return urlEncode(s, '/', ',');
+                     }
+                  });
+               }
                queryParamValues.putAll(paramKey, iterableStrings);
             } else {
-               queryParamValues.put(paramKey, paramValue.get().toString());
+               String value = paramValue.get().toString();
+               if (!param.isAnnotationPresent(Encoded.class)) {
+                  value = urlEncode(value, '/', ',');
+               }
+               queryParamValues.put(paramKey, value);
             }
       }
       return queryParamValues;

core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java

@@ -24,6 +24,7 @@ import static org.jclouds.io.Payloads.newInputStreamPayload;
 import static org.jclouds.io.Payloads.newStringPayload;
 import static org.jclouds.providers.AnonymousProviderMetadata.forApiOnEndpoint;
 import static org.jclouds.reflect.Reflection2.method;
+import static org.jclouds.util.Strings2.urlEncode;
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertNull;
 import static org.testng.Assert.assertTrue;
@@ -483,6 +484,12 @@ public class RestAnnotationProcessorTest extends BaseRestApiTest {
       @Path("/")
       public void queryParamIterable(@Nullable @QueryParam("foo") Iterable<String> bars) {
       }
+
+      @FOO
+      @Path("/")
+      @QueryParams(keys = { "test param"}, values = { "foo bar" })
+      public void queryKeyEncoded() {
+      }
    }
 
    public void testQuery() throws SecurityException, NoSuchMethodException {
@@ -575,6 +582,30 @@ public class RestAnnotationProcessorTest extends BaseRestApiTest {
       assertEquals(request.getMethod(), "FOO");
    }
 
+   public void testQueryEncodedKey() throws SecurityException, NoSuchMethodException {
+      GeneratedHttpRequest request = processor.apply(Invocation.create(method(TestQuery.class, "queryKeyEncoded")));
+      assertEquals(request.getEndpoint().getHost(), "localhost");
+      assertEquals(request.getEndpoint().getPath(), "/");
+      assertEquals(request.getEndpoint().getRawQuery(), "x-ms-version=2009-07-17&test%20param=foo%20bar");
+      assertEquals(request.getMethod(), "FOO");
+   }
+
+   @QueryParams(keys = "test%param", values = "percent%")
+   public class TestInterfaceQueryParam {
+      @FOO
+      @Path("/")
+      public void query() {
+      }
+   }
+
+   public void testInterfaceEncodedKey() throws SecurityException, NoSuchMethodException {
+      GeneratedHttpRequest request = processor.apply(Invocation.create(method(TestInterfaceQueryParam.class, "query")));
+      assertEquals(request.getEndpoint().getHost(), "localhost");
+      assertEquals(request.getEndpoint().getPath(), "/");
+      assertEquals(request.getEndpoint().getRawQuery(), "test%25param=percent%25");
+      assertEquals(request.getMethod(), "FOO");
+   }
+
    interface TestPayloadParamVarargs {
       @POST
       void varargs(HttpRequestOptions... options);
@@ -663,6 +694,16 @@ public class RestAnnotationProcessorTest extends BaseRestApiTest {
       assertPayloadEquals(request, "fooya", "application/unknown", false);
    }
 
+   public void testQueryVarargsEncoding() throws Exception {
+      Invokable<?, ?> method = method(TestPayloadParamVarargs.class, "varargsWithReq", String.class,
+            HttpRequestOptions[].class);
+      GeneratedHttpRequest request = processor.apply(
+            Invocation.create(method,
+                  ImmutableList.<Object> of("required param",
+                        new TestHttpRequestOptions().queryParams(ImmutableMultimap.of("key", "foo bar")))));
+      assertRequestLineEquals(request, "POST http://localhost:9999?key=foo%20bar HTTP/1.1");
+   }
+
    public void testDuplicateHeaderAndQueryVarargs() throws Exception {
       Invokable<?, ?> method = method(TestPayloadParamVarargs.class, "varargs",
             HttpRequestOptions[].class);
@@ -1556,6 +1597,21 @@ public class RestAnnotationProcessorTest extends BaseRestApiTest {
       public void oneQueryParamExtractor(@QueryParam("one") @ParamParser(FirstCharacter.class) String one) {
       }
 
+      @GET
+      @Path("/")
+      public void oneQueryParam(@QueryParam("one") String one) {
+      }
+
+      @GET
+      @Path("/")
+      public void encodedQueryParam(@QueryParam("encoded") @Encoded String encoded) {
+      }
+
+      @GET
+      @Path("/")
+      public void encodedQueryListParam(@QueryParam("encoded") @Encoded List<String> encodedStrings) {
+      }
+
       @POST
       @Path("/")
       public void oneFormParamExtractor(@FormParam("one") @ParamParser(FirstCharacter.class) String one) {
@@ -1608,6 +1664,41 @@ public class RestAnnotationProcessorTest extends BaseRestApiTest {
       assertPayloadEquals(request, null, null, false);
    }
 
+   @Test
+   public void testEncodedQueryParam() throws Exception {
+      Invokable<?, ?> method = method(TestPath.class, "encodedQueryParam", String.class);
+      GeneratedHttpRequest request = processor.apply(Invocation.create(method,
+            ImmutableList.<Object> of("foo%20bar")));
+      assertRequestLineEquals(request, "GET http://localhost:9999/?encoded=foo%20bar HTTP/1.1");
+      assertNonPayloadHeadersEqual(request, "");
+      assertPayloadEquals(request, null, null, false);
+
+      method = method(TestPath.class, "encodedQueryListParam", List.class);
+      String [] args = {"foo%20bar", "foo/bar"};
+      request = processor.apply(Invocation.create(method,
+            ImmutableList.<Object> of(ImmutableList.of("foo%20bar", "foo/bar"))));
+      assertRequestLineEquals(request, "GET http://localhost:9999/?encoded=foo%20bar&encoded=foo/bar HTTP/1.1");
+      assertNonPayloadHeadersEqual(request, "");
+      assertPayloadEquals(request, null, null, false);
+   }
+
+   @DataProvider(name = "queryStrings")
+   public Object[][] createQueryData() {
+      return new Object[][] { { "normal" }, { "sp ace" }, { "qu?stion" }, { "unic₪de" }, { "path/foo" }, { "colon:" },
+            { "asteri*k" }, { "quote\"" }, { "great<r" }, { "lesst>en" }, { "p|pe" } };
+   }
+
+   @Test(dataProvider = "queryStrings")
+   public void testQueryParam(String val) {
+      Invokable<?, ?> method = method(TestPath.class, "oneQueryParam", String.class);
+      GeneratedHttpRequest request = processor.apply(Invocation.create(method,
+            ImmutableList.<Object> of(val)));
+      assertRequestLineEquals(request, String.format("GET http://localhost:9999/?one=%s HTTP/1.1",
+            urlEncode(val, '/', ',')));
+      assertNonPayloadHeadersEqual(request, "");
+      assertPayloadEquals(request, null, null, false);
+   }
+
    @Test
    public void testFormParamExtractor() throws Exception {
       Invokable<?, ?> method = method(TestPath.class, "oneFormParamExtractor", String.class);