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. Project: http://git-wip-us.apache.org/repos/asf/jclouds/repo Commit: http://git-wip-us.apache.org/repos/asf/jclouds/commit/5bbcb834 Tree: http://git-wip-us.apache.org/repos/asf/jclouds/tree/5bbcb834 Diff: http://git-wip-us.apache.org/repos/asf/jclouds/diff/5bbcb834 Branch: refs/heads/master Commit: 5bbcb8342f9eb5aadcb99885931891e5a8362255 Parents: 94b3cba Author: Timur Alperovich <[email protected]> Authored: Fri Oct 16 01:48:58 2015 -0700 Committer: Ignasi Barrera <[email protected]> Committed: Wed Oct 21 00:06:23 2015 +0200 ---------------------------------------------------------------------- core/src/main/java/org/jclouds/http/Uris.java | 19 ++-- .../rest/internal/RestAnnotationProcessor.java | 53 +++++++----- .../internal/RestAnnotationProcessorTest.java | 91 ++++++++++++++++++++ 3 files changed, 136 insertions(+), 27 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/jclouds/blob/5bbcb834/core/src/main/java/org/jclouds/http/Uris.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/jclouds/http/Uris.java b/core/src/main/java/org/jclouds/http/Uris.java index bf8449b..441db62 100644 --- a/core/src/main/java/org/jclouds/http/Uris.java +++ b/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()) - b.append('?').append(encodeQueryLine(query)); + if (!query.isEmpty()) { + if (encodeQuery) { + b.append('?').append(encodeQueryLine(query)); + } else { + b.append('?').append(buildQueryLine(query)); + } + } return b.toString(); } http://git-wip-us.apache.org/repos/asf/jclouds/blob/5bbcb834/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java b/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java index e1c7468..113e9dc 100644 --- a/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java +++ b/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(invocation, uriBuilder, encodedParams)); + tokenValues.putAll(addPathAndGetTokens(caller, uriBuilder, encodeFullPath)); + 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) { - requestBuilder.endpoint(uriBuilder.buildNoEncoding(convertUnsafe(tokenValues))); - } else { - requestBuilder.endpoint(uriBuilder.build(convertUnsafe(tokenValues))); - } + // Query parameter encoding is handled in the annotation processor + requestBuilder.endpoint(uriBuilder.build(convertUnsafe(tokenValues), /*encodePath=*/encodeFullPath, + /*encodeQuery=*/ false)); 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.put(query.keys()[i], null); + queryParams.removeAll(key); + 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)) { - pathParamValues.put(paramKey, Strings2.urlEncode(paramValue.get().toString())); + if (!encodeFullPath && !param.isAnnotationPresent(Encoded.class)) { + 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; http://git-wip-us.apache.org/repos/asf/jclouds/blob/5bbcb834/core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java b/core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java index 95bed02..6a9dd73 100644 --- a/core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java +++ b/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) { @@ -1609,6 +1665,41 @@ public class RestAnnotationProcessorTest extends BaseRestApiTest { } @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); GeneratedHttpRequest request = processor.apply(Invocation.create(method,
