You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Bill Branan <no...@github.com> on 2014/05/30 22:43:42 UTC
[jclouds] Resolves https://issues.apache.org/jira/browse/JCLOUDS-278
(#388)
This patch resolves https://issues.apache.org/jira/browse/JCLOUDS-278, with changes made against the master branch. Code is the same as in https://github.com/jclouds/jclouds/pull/387 except for the test which is tweaked slightly based on changes that have been made to GeneratedHttpRequest since 1.5.x.
You can merge this Pull Request by running:
git pull https://github.com/duracloud/jclouds bugfix/jclouds-278-master
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/388
-- Commit Summary --
* Resolves https://issues.apache.org/jira/browse/JCLOUDS-278 by not attempting to perform URL decoding on the content item names when generating the URIs which are created as part of parsing a container content listing from JSON.
-- File Changes --
M apis/swift/src/main/java/org/jclouds/openstack/swift/functions/ParseObjectInfoListFromJsonResponse.java (2)
A apis/swift/src/test/java/org/jclouds/openstack/swift/functions/ParseObjectInfoListFromJsonResponseTest.java (79)
M core/src/main/java/org/jclouds/http/Uris.java (23)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/388.patch
https://github.com/jclouds/jclouds/pull/388.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by Bill Branan <no...@github.com>.
> + args.add("container-name");
> + args.add(new ListContainerOptions[0]);
> + GeneratedHttpRequest.Builder builder = new GeneratedHttpRequest.Builder();
> + builder.method("method")
> + .endpoint("http://test.org/test")
> + .invocation(Invocation.create(Invokable.from(Object.class.getMethod("toString", null)), args))
> + .caller(null);
> +
> + GeneratedHttpRequest request = builder.build();
> +
> + parser.setContext(request);
> +
> + PageSet<ObjectInfo> objectSet = parser.apply(stream);
> + assertNotNull(objectSet);
> + ObjectInfo objectInfo = objectSet.iterator().next();
> + assertEquals(contentName, objectInfo.getName());
Yes, I used JUnit style, which is assertEquals(expected, actual). I can swap it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388/files#r13253738
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by Bill Branan <no...@github.com>.
@demobox I think the test should stick with %x (see my comment in https://github.com/jclouds/jclouds/pull/389). The test succeeds with a non-URL-encoded string including "%" because that's the purpose of the fix.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388#issuecomment-44748899
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by Bill Branan <no...@github.com>.
> @@ -138,20 +138,35 @@ public UriBuilder host(String host) {
> }
>
> public UriBuilder path(@Nullable String path) {
> + return path(path, true);
> + }
> +
> + public UriBuilder path(@Nullable String path, boolean urlDecode) {
I intentionally kept the changes in this pull request to the minimum required to solve the issue, making sure that code paths other than the one I was specifically changing would not be affected. While the change to UriBuilder that you suggest seems reasonable, I'd be inclined to handle it as a next step, so that it's clear in the commit logs which changes fix the known issue, and which are refactoring to reduce fragility.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388/files#r13261217
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by Andrew Phillips <no...@github.com>.
> + String contentName = "swift-test-content-%x-1401395399020";
> +
> + String jsonObjectList =
> + "[{\"last_modified\": \"2014-05-29T20:30:03.845660\", " +
> + "\"bytes\": 19, " +
> + "\"name\": \"" + contentName + "\", " +
> + "\"content_type\": \"application/unknown\"}]";
> +
> + InputStream stream = Strings2.toInputStream(jsonObjectList);
> +
> + ParseObjectInfoListFromJsonResponse parser =
> + new ParseObjectInfoListFromJsonResponse(new GsonWrapper(new Gson()));
> +
> + List<Object> args = new ArrayList<Object>();
> + args.add("container-name");
> + args.add(new ListContainerOptions[0]);
`ImmutableList.of("container-name", new ListContainerOptions[0])` and inline this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388/files#r13251316
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by Andrew Phillips <no...@github.com>.
> I've updated the test case to use %x...thanks for explaining!
The test case also succeeds (without code changes) using `%x` on master - see [this comment](https://github.com/jclouds/jclouds/pull/389#issuecomment-44761426)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388#issuecomment-44829487
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by Andrew Phillips <no...@github.com>.
Closed #388.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388#event-129835406
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1321](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1321/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388#issuecomment-44710004
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #849](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/849/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388#issuecomment-44703447
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by Andrew Phillips <no...@github.com>.
Looks like the test case here succeeds against master _without_ a code change (see #389)? At least, the test passes for me locally.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388#issuecomment-44718422
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by Andrew Phillips <no...@github.com>.
> I think the test should stick with %x (see my comment in #389).
I've updated the test case to use `%x`...thanks for explaining!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388#issuecomment-44759642
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by Andrew Phillips <no...@github.com>.
@andrewgaul @bbranan Thoughts on closing this? The test has been committed to master, and passes already. So the code change here should not be required?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388#issuecomment-45566696
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by Andrew Phillips <no...@github.com>.
> +
> + List<Object> args = new ArrayList<Object>();
> + args.add("container-name");
> + args.add(new ListContainerOptions[0]);
> + GeneratedHttpRequest.Builder builder = new GeneratedHttpRequest.Builder();
> + builder.method("method")
> + .endpoint("http://test.org/test")
> + .invocation(Invocation.create(Invokable.from(Object.class.getMethod("toString", null)), args))
> + .caller(null);
> +
> + GeneratedHttpRequest request = builder.build();
> +
> + parser.setContext(request);
> +
> + PageSet<ObjectInfo> objectSet = parser.apply(stream);
> + assertNotNull(objectSet);
Don't need this assertion? We will blow up if this turns out to be null in any case, but we're really only interested in the name here? E.g.
```
ObjectInfo info = parser.apply(stream).iterator().next();
assertEquals(objectInfo.getName(), contentName);
```
?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388/files#r13251514
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by Andrew Phillips <no...@github.com>.
> +
> + ParseObjectInfoListFromJsonResponse parser =
> + new ParseObjectInfoListFromJsonResponse(new GsonWrapper(new Gson()));
> +
> + List<Object> args = new ArrayList<Object>();
> + args.add("container-name");
> + args.add(new ListContainerOptions[0]);
> + GeneratedHttpRequest.Builder builder = new GeneratedHttpRequest.Builder();
> + builder.method("method")
> + .endpoint("http://test.org/test")
> + .invocation(Invocation.create(Invokable.from(Object.class.getMethod("toString", null)), args))
> + .caller(null);
> +
> + GeneratedHttpRequest request = builder.build();
> +
> + parser.setContext(request);
[minor] Inline? I.e. `parser.setContext(builder.build());`? But also happy to leave as-is
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388/files#r13251435
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1175](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1175/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388#issuecomment-44712337
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by Bill Branan <no...@github.com>.
I'm fine with closing this, the issue appears to have been resolved in master, and the test verifies it. Thanks for your help in getting this resolved!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388#issuecomment-45612665
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by Andrew Gaul <no...@github.com>.
> @@ -138,20 +138,35 @@ public UriBuilder host(String host) {
> }
>
> public UriBuilder path(@Nullable String path) {
> + return path(path, true);
> + }
> +
> + public UriBuilder path(@Nullable String path, boolean urlDecode) {
Can we change `UriBuilder` to not encode these paths until `build()`? We keep tweaking this behavior, the last time in 12f29fd8a9e50203acc12ccf794cf73c0abf5703, and the fragility of this code gives me pause.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388/files#r13255411
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by Andrew Phillips <no...@github.com>.
Thanks, @bbranan!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388#issuecomment-45628390
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1320](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1320/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388#issuecomment-44703720
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by Andrew Phillips <no...@github.com>.
> +import java.io.InputStream;
> +import java.util.ArrayList;
> +import java.util.List;
> +
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertNotNull;
> +
> +/**
> + * @author Bill Branan
> + * Date: 5/29/14
> + */
> +@Test(groups = "unit")
> +public class ParseObjectInfoListFromJsonResponseTest {
> +
> + @Test
> + public void testApplyInputStream() throws Exception {
Rename the test so it describes what we're testing, e.g. "testNoUrlDecodingOfResponse" or so?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388/files#r13251370
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by Andrew Phillips <no...@github.com>.
> + args.add("container-name");
> + args.add(new ListContainerOptions[0]);
> + GeneratedHttpRequest.Builder builder = new GeneratedHttpRequest.Builder();
> + builder.method("method")
> + .endpoint("http://test.org/test")
> + .invocation(Invocation.create(Invokable.from(Object.class.getMethod("toString", null)), args))
> + .caller(null);
> +
> + GeneratedHttpRequest request = builder.build();
> +
> + parser.setContext(request);
> +
> + PageSet<ObjectInfo> objectSet = parser.apply(stream);
> + assertNotNull(objectSet);
> + ObjectInfo objectInfo = objectSet.iterator().next();
> + assertEquals(contentName, objectInfo.getName());
Invert? TestNG uses `assertEquals(actual, expected)`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388/files#r13251534
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1172](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1172/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388#issuecomment-44702382
Re: [jclouds] Resolves
https://issues.apache.org/jira/browse/JCLOUDS-278 (#388)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #850](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/850/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/388#issuecomment-44708669