[jclouds] JCLOUDS-1008: Add @Encoded annotation. (#861)

[Timur Alperovich <no...@github.com>]

Certain providers (e.g. Google Cloud Storage) place tokens that should
be encoded in the request path (e.g. GET
http://<host>/b/<bucket>/o/<object>) and expect them to be
percent-encoded. In the above example a GET request for "foo/bar"
should be translated to http://<host>/b/<bucket>/o/foo%2Fbar.
Currently, there is no way to express this in jclouds, as the entire
request path is encoded exactly once and there is no control over
whether a request parameter should be handled specially. In the
example above, "/" are not encoded in the path and the URL is
submitted as "http://<host>/b/<bucket>/o/foo/bar", which may be wrong.

This patch extends the annotation processor to support @Encoded for
the individual parameters of the request. However, this means that the
entire path is _NOT_ URL encoded. The caller *must* make sure that the
appropriate parameters are encoded.
You can view, comment on, or merge this pull request online at:

  https://github.com/jclouds/jclouds/pull/861

-- Commit Summary --

  * JCLOUDS-1008: Add @Encoded annotation.

-- File Changes --

    M core/src/main/java/org/jclouds/http/Uris.java (21)
    M core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java (20)
    M core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java (14)

-- Patch Links --

https://github.com/jclouds/jclouds/pull/861.patch
https://github.com/jclouds/jclouds/pull/861.diff

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/861

Re: [jclouds] JCLOUDS-1008: Add @Encoded annotation. (#861)

[Ignasi Barrera <no...@github.com>]

> Do you think this is something that should be added now or deferred until there is a use case for it?

I'd add it now, for consistency and completeness of the feature.


---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/861#issuecomment-147008484

Re: [jclouds] JCLOUDS-1008: Add @Encoded annotation. (#861)

[Ignasi Barrera <no...@github.com>]

Ok. I really think it is necessary to isolate the encoding calls. We'll still have to process the annotations in several places in the RestAnnotationProcessor, but the details on *how to encode* (or how to call the encoding logic) shouldn't be spread along the class.
I agree to fix this in another PR (but let's make sure that happens :)) and will merge this later today/early tomorrow.

Thanks for the great effort in these changes @timuralp!

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/861#issuecomment-149697264

Re: [jclouds] JCLOUDS-1008: Add @Encoded annotation. (#861)

[Ignasi Barrera <no...@github.com>]

Thanks @timuralp! This lgtm and is good to merge.
One last question: should this also apply to query parameters?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/861#issuecomment-147004430

Re: [jclouds] JCLOUDS-1008: Add @Encoded annotation. (#861)

[Timur Alperovich <no...@github.com>]

If I'm reading ```http/utils/Queries.java``` correctly, the parameters are individually encoded, since the ```UriBuilder``` class will call ```encodeQueryLine()```, which in turn encodes the string before appending it to the URI. In that sense, I think you're right that maybe adding ```@Encoded``` to the query parameters may make sense to indicate that jclouds should skip encoding the parameter value.

Do you think this is something that should be added now or deferred until there is a use case for it?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/861#issuecomment-147006237

Re: [jclouds] JCLOUDS-1008: Add @Encoded annotation. (#861)

[Ignasi Barrera <no...@github.com>]

Closed #861.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/861#event-440905343

Re: [jclouds] JCLOUDS-1008: Add @Encoded annotation. (#861)

[Timur Alperovich <no...@github.com>]

No problem -- the original approach was still not quite right, as I used @encoded to indicate that a parameter should be encoded, rather than it was previously encoded. I reworked it to be inline with the ```javax.ws.rs.Encoded``` documentation above. Let me know if it still doesn't make sense! I'll push an update to google labs to use this annotation.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/861#issuecomment-146660139

Re: [jclouds] JCLOUDS-1008: Add @Encoded annotation. (#861)

[Ignasi Barrera <no...@github.com>]

> @@ -1543,6 +1549,14 @@ public void testPathParamExtractor() throws Exception {
>     }
>  
>     @Test
> +   public void testPathParamEncoding() throws Exception {
> +      Invokable<?, ?> method = method(TestPath.class, "onePathEncodedParam", String.class);
> +      GeneratedHttpRequest request = processor.apply(Invocation.create(method,
> +            ImmutableList.<Object> of("foo/bar")));
> +      assertRequestLineEquals(request, "GET http://localhost:9999/foo%2Fbar HTTP/1.1");

Use a more complex path in the test to also verify the fact that the entire path is not encoded. We need to verify that in the tests too.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/861/files#r40968986

Re: [jclouds] JCLOUDS-1008: Add @Encoded annotation. (#861)

[Ignasi Barrera <no...@github.com>]

Thanks @timuralp. I see what you mean in the commit message. One thing that worries me about this change is the number of places where the encoding happens in the rest annotation processor. There are many places where this logic has to be taken into account, and that will make the corre more error-prone and difficult to maintain.

What about a different approach: taking into account that [the values query parameter map in the UriBuilder](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/Uris.java#L102) are Objects and not simple Strings, and that those values are in the end [serialized using their toString() method](https://github.com/timuralp/jclouds/blob/00242207f9c5d8b87e931bdc80a4c37c2752a686/core/src/main/java/org/jclouds/http/utils/Queries.java#L109-L144), we could have an object, say `QueryValue`, that has the original value, a boolean saying if it needs to be encoded or not, and a `toString()` method that properly serializes it taking into account that flag.

This way, the RestAnnotationProcessor should only take care of generating QueryValue objects for each query param, and all encoding logic remains in the UriBuilder and the Queries classes. WDYT about exploring this approach?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/861#issuecomment-149343731

Re: [jclouds] JCLOUDS-1008: Add @Encoded annotation. (#861)

[Ignasi Barrera <no...@github.com>]

Although it might require a bit more investigation and work, I think that what you describe, and using `@SkipEncoding` where needed (as those are really the "corner cases"), makes more sense and is more consistent with what we're trying to achieve with all these changes.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/861#issuecomment-144858035

Re: [jclouds] JCLOUDS-1008: Add @Encoded annotation. (#861)

[Ignasi Barrera <no...@github.com>]

Merged to master as [94b3cba6](http://git-wip-us.apache.org/repos/asf/jclouds/commit/94b3cba6) and [5bbcb834](http://git-wip-us.apache.org/repos/asf/jclouds/commit/5bbcb834). Thanks @timuralp!

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/861#issuecomment-149720743

Re: [jclouds] JCLOUDS-1008: Add @Encoded annotation. (#861)

[Ignasi Barrera <no...@github.com>]

Thanks @timuralp!

> However, this means that the entire path is NOT URL encoded

Having this in mind it would make more sense to consider the `@Encoded` annotation at method level, instead of being at individual parameters. This enforces the fact that it applies to all parameters in the method and that user has to take care of all parameters, not only the encoded ones. WDYT?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/861#issuecomment-144855746

Re: [jclouds] JCLOUDS-1008: Add @Encoded annotation. (#861)

[Timur Alperovich <no...@github.com>]

Added ```@Encoded``` to the ```QueryParam```. In the process, all of the encoding handling for ```QueryParam``` and ```QueryParams``` was moved into the ```RestAnnotationProcessor```. I tried to explain the reasoning and give some context in the commit comment.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/861#issuecomment-148863454

Re: [jclouds] JCLOUDS-1008: Add @Encoded annotation. (#861)

[Timur Alperovich <no...@github.com>]

@nacx I think you're right -- that does make more sense. The contract then is that the user can either use the old style path encoding where jclouds will encode the entire path string once (and not encode "/" in parameters) or encode each parameter individually.

Honestly, I considered changing jclouds to the second model altogether from its current behavior, but was concerned about breaking any consumers that may rely on the behavior. For example, in S3 the blob name should include "/" in the request and not encode it (although that could be resolved with ```@SkipEncoding``` on the parameter level).

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/861#issuecomment-144857229

Re: [jclouds] JCLOUDS-1008: Add @Encoded annotation. (#861)

[Ignasi Barrera <no...@github.com>]

> @@ -147,6 +149,7 @@ public Part apply(Entry<String, Object> from) {
>     private final GetAcceptHeaders getAcceptHeaders;
>     private final Invocation caller;
>     private final boolean stripExpectHeader;
> +   private boolean encodedParams;

We should assume the annotation processor could be a singleton and avoid adding state to it. Consider removing this member variable and an alternate way to get its value in each invocation to the processor to avoid possible errors in subsequent calls due to an old state.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/861/files#r40968494

Re: [jclouds] JCLOUDS-1008: Add @Encoded annotation. (#861)

[Timur Alperovich <no...@github.com>]

I'd like to keep the ```UriBuilder``` as dumb as possible. These encoding issues arose from the fact that the encoding logic got baked into the ```UriBuilder```, making it hard to maintain a clean abstraction. Moving encoding out of it makes the most sense. The only question is what makes more sense: annotation processor or an auxiliary class, such as ```QueryValue```. Using ```QueryValue``` will allow us to sink the conditional around encoding and out of the annotation processor itself, so that seems like a win. On the other hand, the code around extracting the ```@Encoded``` value remains. Let me give it a shot and see what this might look like.

Also, should we split off this work into another PR? It's becoming a bit more involved and blocks fixing the actual bug in GCS (addressed by the first commit in this PR).

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/861#issuecomment-149387151

Re: [jclouds] JCLOUDS-1008: Add @Encoded annotation. (#861)

[Timur Alperovich <no...@github.com>]

> @@ -147,6 +149,7 @@ public Part apply(Entry<String, Object> from) {
>     private final GetAcceptHeaders getAcceptHeaders;
>     private final Invocation caller;
>     private final boolean stripExpectHeader;
> +   private boolean encodedParams;

Ok -- let me rework this and remove the member variable.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/861/files#r40968631

Re: [jclouds] JCLOUDS-1008: Add @Encoded annotation. (#861)

[Ignasi Barrera <no...@github.com>]

I was a bit confused by the use of the `javax.ws.rs.Encoded` annotation. As I understand its meaning, it just says "do not decode this parameter; just pass the value as-is".

This made me think that the implementation would delegate to the user the responsibility of encoding the parameters, which would seem more consistent? If the method is annotated `@Encoded`, then jclouds will assume everything is properly encoded and won't attempt to encode anything. With this approach, and now that I properly understand the impl, it makes sense the original commit where the `@Encoded` annotation was at parameter level: parameters annotated `@Encoded` should *not* be encoded by jclouds, and the others will (and the URL as a whole won't be encoded if any parameter is annotated).

The current implementation, and apologies for not having understood properly this in the first review round, looks a bit difficult to understand: if the method is annotated `@Encoded`, then the URl as a whole won't be encoded but every path parameter will be encoded.

I think the first one (annotation per param) is consistent with the meaning of the `javax.ws.rs.Encoded` annotation, and the current one make sense too, but then I'd create a different annotation `@OnlyEncodeParameters` or similar, to properly illustrate the behavior.

Honestly I think the first approach is cleaner. Apologies for the misunderstanding!

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/861#issuecomment-145785758