You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@jclouds.apache.org by Timur Alperovich <no...@github.com> on 2015/09/11 23:36:24 UTC

[jclouds] JCLOUDS-217: do not double decode strings. (#856)

This PR removes the decoding multi map from URI and makes changes primarily to tests to not work around its behavior. This does not fix an existing issue, but I believe it simplifies the internals of handling parameters in jclouds.

Background
-----------------

Currently, calling ```addQueryParam()``` when constructing an HttpRequest results in inserting the key and the value into a decoding MultiMap. What it means is that the parameter value is decoded prior to being inserted and then encoded again on retrieval.

Further, there is some additional logic that checks whether a string is encoded or not in the ```Strings2.urlDecode()``` method. This behavior is also problematic such that providers need to make sure they encode strings before adding them as a query parameter, as they may not get the expected result back (e.g. a string that contains "+" is decoded into " " and encoded into "%20").

The jclouds code currently works around these issues, however, I believe the parameter handling is not clear to developers and leads to confusion. The PR suggests a more straightforward approach:
1. all query parameters will be encoded by ```addQueryParam()```
2. the query parameters are _never_ decoded by ```addQueryParam()```

I tested the PR with Azure, AWS S3, Swift, and the Filesystem backend. I removed one workaround from AWS S3, but otherwise all providers were functional. The majority of the changes are in fixing up tests to not use encoded strings (as these now become doubly encoded, so a parameter "my%20account" is turned into "my%2520account".

You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * JCLOUDS-217: Do not decode query strings.
  * Skip broken Azure test.
  * JCLOUDS-217: Cloudstack: avoid encoded strings.
  * JCLOUDS-217: Old Swift should encode blob names.
  * JCLOUDS-217: AWS-S3: Remove special encoding.

-- File Changes --

    M apis/cloudstack/src/test/java/org/jclouds/cloudstack/compute/CloudStackComputeServiceAdapterExpectTest.java (22)
    M apis/cloudstack/src/test/java/org/jclouds/cloudstack/compute/extensions/CloudStackImageExtensionExpectTest.java (24)
    M apis/cloudstack/src/test/java/org/jclouds/cloudstack/compute/extensions/CloudStackSecurityGroupExtensionExpectTest.java (36)
    M apis/cloudstack/src/test/java/org/jclouds/cloudstack/features/FirewallApiExpectTest.java (380)
    M apis/cloudstack/src/test/java/org/jclouds/cloudstack/features/GlobalHostApiExpectTest.java (6)
    M apis/cloudstack/src/test/java/org/jclouds/cloudstack/features/GlobalUserApiTest.java (2)
    M apis/cloudstack/src/test/java/org/jclouds/cloudstack/features/GlobalZoneApiExpectTest.java (45)
    M apis/cloudstack/src/test/java/org/jclouds/cloudstack/features/ISOApiExpectTest.java (18)
    M apis/cloudstack/src/test/java/org/jclouds/cloudstack/features/SecurityGroupApiTest.java (24)
    M apis/cloudstack/src/test/java/org/jclouds/cloudstack/features/SnapshotApiTest.java (12)
    M apis/cloudstack/src/test/java/org/jclouds/cloudstack/features/TagApiExpectTest.java (40)
    M apis/cloudstack/src/test/java/org/jclouds/cloudstack/features/TemplateApiTest.java (6)
    M apis/cloudstack/src/test/java/org/jclouds/cloudstack/features/VirtualMachineApiExpectTest.java (19)
    M apis/cloudstack/src/test/java/org/jclouds/cloudstack/features/ZoneApiExpectTest.java (19)
    M apis/cloudstack/src/test/java/org/jclouds/cloudstack/internal/BaseCloudStackComputeServiceContextExpectTest.java (52)
    M apis/swift/src/main/java/org/jclouds/openstack/swift/functions/ParseObjectInfoListFromJsonResponse.java (5)
    M core/src/main/java/org/jclouds/http/Uris.java (65)
    M core/src/main/java/org/jclouds/util/Strings2.java (9)
    M core/src/test/java/org/jclouds/http/HttpRequestTest.java (5)
    M core/src/test/java/org/jclouds/util/Strings2Test.java (10)
    M providers/aws-s3/src/main/java/org/jclouds/aws/s3/blobstore/AWSS3BlobRequestSigner.java (11)
    M providers/aws-s3/src/test/java/org/jclouds/aws/s3/blobstore/AWSS3BlobSignerExpectTest.java (23)
    M providers/azureblob/src/test/java/org/jclouds/azureblob/blobstore/integration/AzureBlobIntegrationLiveTest.java (6)

-- Patch Links --

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

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

Re: [jclouds] JCLOUDS-217: do not double decode strings. (#856)

Posted by Timur Alperovich <no...@github.com>.

Rebased. I'll look into GCS in labs and see if anything else may be broken.

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

Re: [jclouds] JCLOUDS-217: do not double decode strings. (#856)

Posted by Andrew Gaul <no...@github.com>.

:+1: 

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

Re: [jclouds] JCLOUDS-217: do not double decode strings. (#856)

Posted by Ignasi Barrera <no...@github.com>.

+1 Thanks for all this fixes @timuralp!
Are there required changes in the labs repos too, once this is merged?

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