You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by David Currie <no...@github.com> on 2018/09/11 11:04:41 UTC

[jclouds/jclouds] JCLOUDS-1447: URL encode source header on copyObject (#1238)

As described in https://jira.apache.org/jira/browse/JCLOUDS-1447, S3 CopyObject requires the `x-amz-copy-source` header to be URL encoded.

This is not universally true of all headers though (for example the equals in `x-amz-copy-source-range`) therefore introducing a new parameter on `@Headers` to indicate whether URL encoding should take place.

Also updated `testUseBucketWithUpperCaseName` to remove the testing for legacy naming conventions as these are no longer supported by AWS.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Correct failing testUseBucketWithUpperCaseName
  * URL encode x-amz-copy-source for S3 CopyObject

-- File Changes --

    M apis/s3/src/main/java/org/jclouds/s3/S3Client.java (4)
    M apis/s3/src/test/java/org/jclouds/s3/S3ClientLiveTest.java (17)
    M apis/s3/src/test/java/org/jclouds/s3/S3ClientMockTest.java (15)
    M core/src/main/java/org/jclouds/rest/annotations/Headers.java (6)
    M core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java (2)
    M providers/aws-s3/src/test/java/org/jclouds/aws/s3/AWSS3ClientLiveTest.java (46)

-- Patch Links --

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1238

Re: [jclouds/jclouds] JCLOUDS-1447: URL encode source header on copyObject (#1238)

Posted by Ignasi Barrera <no...@github.com>.
nacx approved this pull request.





-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1238#pullrequestreview-154608939

Re: [jclouds/jclouds] JCLOUDS-1447: URL encode source header on copyObject (#1238)

Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.



> @@ -763,6 +763,8 @@ private static void addHeader(Multimap<String, String> headers, Headers header,
       for (int i = 0; i < header.keys().length; i++) {
          String value = header.values()[i];
          value = replaceTokens(value, tokenValues);
+         if (header.urlEncode().length > i && header.urlEncode()[i])

[minor] Change to `i < header.urlEncode().length && header.urlEncode()[i]`? The bounds check seems to be written that way around more commonly, which might make it easier to understand that's going on here

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1238#pullrequestreview-154462976

Re: [jclouds/jclouds] JCLOUDS-1447: URL encode source header on copyObject (#1238)

Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.



> @@ -408,6 +408,21 @@ public void testCopyObject() throws Exception {
       }
    }
 
+   String sourceKeyRequiringEncoding = "apples#?:$&'\"<>čॐ";

+1 to move into the test, if possible

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1238#discussion_r216878071

Re: [jclouds/jclouds] JCLOUDS-1447: URL encode source header on copyObject (#1238)

Posted by David Currie <no...@github.com>.
davidcurrie commented on this pull request.



> @@ -408,6 +408,21 @@ public void testCopyObject() throws Exception {
       }
    }
 
+   String sourceKeyRequiringEncoding = "apples#?:$&'\"<>čॐ";

That was my natural inclination but I was following the pattern used for `sourceKey` elsewhere in the class. Happy to inline though given that this is currently only used in one place.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1238#discussion_r216957372

Re: [jclouds/jclouds] JCLOUDS-1447: URL encode source header on copyObject (#1238)

Posted by Ignasi Barrera <no...@github.com>.
>can you think of any other providers that need to URL encode headers?

I'm mostly familiar with compute providers, and those don't use HTTP headers as extensively as Blob storage providers, so I'd say we're fine.

@davidcurrie could you please add some unit tests to the `RestAnnotationProcessorTest` class? There are several tests that verify the behavior of the `@Headers` annotation and we'd rather have some tests there that verify the encoding feature.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1238#issuecomment-420575237

Re: [jclouds/jclouds] JCLOUDS-1447: URL encode source header on copyObject (#1238)

Posted by Andrew Gaul <no...@github.com>.
Closed #1238.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1238#event-1841442277

Re: [jclouds/jclouds] JCLOUDS-1447: URL encode source header on copyObject (#1238)

Posted by David Currie <no...@github.com>.
@davidcurrie pushed 1 commit.

66d76bc  Address review comments


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1238/files/360c0fbbd0eccae7138ed0d92c3af8a14184760b..66d76bce77225bb2b97d6ca9acfc6b5e965df544

Re: [jclouds/jclouds] JCLOUDS-1447: URL encode source header on copyObject (#1238)

Posted by Andrew Gaul <no...@github.com>.
gaul approved this pull request.

I am surprised we have not encountered this previously.  @nacx can you think of any other providers that need to URL encode headers?

> @@ -408,6 +408,21 @@ public void testCopyObject() throws Exception {
       }
    }
 
+   String sourceKeyRequiringEncoding = "apples#?:$&'\"<>čॐ";

Should this be a local variable?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1238#pullrequestreview-154376899

Re: [jclouds/jclouds] JCLOUDS-1447: URL encode source header on copyObject (#1238)

Posted by Jesse Glick <no...@github.com>.
jglick approved this pull request.





-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1238#pullrequestreview-154169386

Re: [jclouds/jclouds] JCLOUDS-1447: URL encode source header on copyObject (#1238)

Posted by Andrew Gaul <no...@github.com>.
Reworded commit message, shuffled some code between commits, and pushed to master as 5803de0f8ea0fef324b15c4a5a3a801ea7e505f6 and 7ebf12bf3867c7cdead772b8c80675c8a41bf7fa and 2.1.x as 8693687766380901057f1e69070723e7f708bede and f74d1c0976538f343cd192c18b2fccf91eb22b57.  Thank you for your contribution @davidcurrie!  Please use separate pull requests for unrelated commits in the future.


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1238#issuecomment-420687655