You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Ignasi Barrera <no...@github.com> on 2013/07/22 16:31:05 UTC
[jclouds] Generalized the Arg0ToPagedIterable to propagate all args
(#70)
Generalized the `Arg0ToPagedIterable` to allow to propagating all arguments. This will help building PagedIterables for api methods that require more than one argument when requesting the next page of the result set.
This will help completing [JCLOUDS-198](https://issues.apache.org/jira/browse/JCLOUDS-198).
You can merge this Pull Request by running:
git pull https://github.com/nacx/jclouds pagediterable-args
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/70
-- Commit Summary --
* Generalized the Arg0ToPagedIterable to propagate all args
-- File Changes --
M core/src/main/java/org/jclouds/collect/internal/Arg0ToPagedIterable.java (28)
A core/src/main/java/org/jclouds/collect/internal/ArgsToPagedIterable.java (77)
A core/src/test/java/org/jclouds/collect/internal/ArgsToPagedIterableTest.java (168)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/70.patch
https://github.com/jclouds/jclouds/pull/70.diff
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Andrew Phillips <no...@github.com>.
> + GeneratedHttpRequest request = callerArgs(ImmutableList.of());
> + final IterableWithMarker<String> next = IterableWithMarkers.from(ImmutableSet.of("baz"));
> +
> + TestCallerArgs converter = new TestCallerArgs(request) {
> +
> + @Override
> + protected Function<Object, IterableWithMarker<String>> markerToNextForArgs(List<Object> args) {
> + assertTrue(args.isEmpty());
> + return Functions.constant(next);
> + }
> +
> + };
> +
> + assertEquals(converter.apply(IterableWithMarkers.from(ImmutableSet.of("foo", "bar"), "marker")).concat().toSet(),
> + ImmutableSet.of("foo", "bar", "baz"));
> + }
> The first one propagates the arguments of the invoked method directly, the FromCaller propagates the arguments of the caller of the
> method.
OK...hence the tests looking the same. The difference is in `args(...` vs. `callerArgs(...` in the test setup, I gather.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70/files#r5351127
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #73](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/73/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70#issuecomment-21421968
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #216](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/216/) 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/70#issuecomment-21421616
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #530](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/530/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70#issuecomment-21422372
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Ignasi Barrera <no...@github.com>.
> + GeneratedHttpRequest request = callerArgs(ImmutableList.of());
> + final IterableWithMarker<String> next = IterableWithMarkers.from(ImmutableSet.of("baz"));
> +
> + TestCallerArgs converter = new TestCallerArgs(request) {
> +
> + @Override
> + protected Function<Object, IterableWithMarker<String>> markerToNextForArgs(List<Object> args) {
> + assertTrue(args.isEmpty());
> + return Functions.constant(next);
> + }
> +
> + };
> +
> + assertEquals(converter.apply(IterableWithMarkers.from(ImmutableSet.of("foo", "bar"), "marker")).concat().toSet(),
> + ImmutableSet.of("foo", "bar", "baz"));
> + }
The logic is essentially the same; the only difference is where they pick the arguments to propagate from. The first one propagates the arguments of the invoked method directly, the FromCaller propagates the arguments of the caller of the method.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70/files#r5345187
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #532](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/532/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70#issuecomment-21467411
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #211](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/211/) 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/70#issuecomment-21351215
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Andrew Phillips <no...@github.com>.
> + @SuppressWarnings("unchecked")
> + @Override
> + public I setContext(HttpRequest request) {
> + this.request = GeneratedHttpRequest.class.cast(request);
> + return (I) this;
> + }
> +
> + /**
> + * Used to propagate the invoked method arguments during an advance in a
> + * {@link PagedIterable}. For example, in the call
> + * {@code api.getUserApiForZone(zone).list()}, the caller arg0 is the value
> + * of {@code zone}, and the callee is {@code UserApi.list()}
> + *
> + * @author Adrian Cole
> + */
> + public abstract static class FromCaller<T, I extends FromCaller<T, I>> extends ArgsToPagedIterable<T, I> {
Thanks for explaining. Perhaps a Javadoc comment similar to "see `ParseImages` for a usage example"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70/files#r5351026
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Ignasi Barrera <no...@github.com>.
Reopened #70.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Andrew Phillips <no...@github.com>.
> +
> + private abstract static class TestArgs extends ArgsToPagedIterable<String, TestArgs> {
> + private TestArgs(GeneratedHttpRequest in) {
> + this.setContext(in);
> + }
> + }
> +
> + @Test
> + public void testWhenNextMarkerAbsentDoesntAdvance() {
> + GeneratedHttpRequest request = args(ImmutableList.of());
> +
> + TestArgs converter = new TestArgs(request) {
> +
> + @Override
> + protected Function<Object, IterableWithMarker<String>> markerToNextForArgs(List<Object> args) {
> + fail();
Add a message to `fail(...)`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70/files#r5324173
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Andrew Phillips <no...@github.com>.
> +
> + TestArgs converter = new TestArgs(request) {
> +
> + @Override
> + protected Function<Object, IterableWithMarker<String>> markerToNextForArgs(List<Object> args) {
> + assertEquals(args.get(0), "path");
> + return Functions.constant(next);
> + }
> +
> + };
> +
> + assertEquals(converter.apply(IterableWithMarkers.from(ImmutableSet.of("foo", "bar"), "marker")).concat().toSet(),
> + ImmutableSet.of("foo", "bar", "baz"));
> + }
> +
> + private GeneratedHttpRequest args(ImmutableList<Object> args) {
OK. Dang checked reflection exceptions ;-) I guess my personal style preference would be to add `throws Exception` to the test method, but am fine with this, too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70/files#r5351216
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #218](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/218/) 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/70#issuecomment-21468851
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Ignasi Barrera <no...@github.com>.
Closed #70.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Andrew Phillips <no...@github.com>.
> The Java7 failure seems unrelated to this change
Looks like a reasonable guess ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70#issuecomment-21358744
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Ignasi Barrera <no...@github.com>.
The Java7 failure seems unrelated to this change, but I'm closing and re-opening just to be sure.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70#issuecomment-21352954
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Andrew Phillips <no...@github.com>.
> +
> + @Override
> + public PagedIterable<T> apply(IterableWithMarker<T> input) {
> + return input.nextMarker().isPresent() ? advance(input, markerToNextForArgs(getArgs(request))) : onlyPage(input);
> + }
> +
> + protected List<Object> getArgs(GeneratedHttpRequest request) {
> + return request.getInvocation().getArgs();
> + }
> +
> + protected abstract Function<Object, IterableWithMarker<T>> markerToNextForArgs(List<Object> args);
> +
> + @SuppressWarnings("unchecked")
> + @Override
> + public I setContext(HttpRequest request) {
> + this.request = GeneratedHttpRequest.class.cast(request);
Check for illegal arg or are we happy with a `ClassCastException` here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70/files#r5324027
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @Override
> + public PagedIterable<T> apply(IterableWithMarker<T> input) {
> + return input.nextMarker().isPresent() ? advance(input, markerToNextForArgs(getArgs(request))) : onlyPage(input);
> + }
> +
> + protected List<Object> getArgs(GeneratedHttpRequest request) {
> + return request.getInvocation().getArgs();
> + }
> +
> + protected abstract Function<Object, IterableWithMarker<T>> markerToNextForArgs(List<Object> args);
> +
> + @SuppressWarnings("unchecked")
> + @Override
> + public I setContext(HttpRequest request) {
> + this.request = GeneratedHttpRequest.class.cast(request);
I'll better add the check :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70/files#r5345233
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Andrew Phillips <no...@github.com>.
> +
> + TestArgs converter = new TestArgs(request) {
> +
> + @Override
> + protected Function<Object, IterableWithMarker<String>> markerToNextForArgs(List<Object> args) {
> + assertEquals(args.get(0), "path");
> + return Functions.constant(next);
> + }
> +
> + };
> +
> + assertEquals(converter.apply(IterableWithMarkers.from(ImmutableSet.of("foo", "bar"), "marker")).concat().toSet(),
> + ImmutableSet.of("foo", "bar", "baz"));
> + }
> +
> + private GeneratedHttpRequest args(ImmutableList<Object> args) {
Why the `try...catch`? Add "throws XYZException" to the various test methods instead?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70/files#r5324199
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #70](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/70/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70#issuecomment-21351625
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Andrew Phillips <no...@github.com>.
> + @SuppressWarnings("unchecked")
> + @Override
> + public I setContext(HttpRequest request) {
> + this.request = GeneratedHttpRequest.class.cast(request);
> + return (I) this;
> + }
> +
> + /**
> + * Used to propagate the invoked method arguments during an advance in a
> + * {@link PagedIterable}. For example, in the call
> + * {@code api.getUserApiForZone(zone).list()}, the caller arg0 is the value
> + * of {@code zone}, and the callee is {@code UserApi.list()}
> + *
> + * @author Adrian Cole
> + */
> + public abstract static class FromCaller<T, I extends FromCaller<T, I>> extends ArgsToPagedIterable<T, I> {
Add an example of how this is to be used? Is it intentional that this is not actually used in `ArgsToPagedIterable` anywhere?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70/files#r5324156
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #528](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/528/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70#issuecomment-21356860
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + private abstract static class TestArgs extends ArgsToPagedIterable<String, TestArgs> {
> + private TestArgs(GeneratedHttpRequest in) {
> + this.setContext(in);
> + }
> + }
> +
> + @Test
> + public void testWhenNextMarkerAbsentDoesntAdvance() {
> + GeneratedHttpRequest request = args(ImmutableList.of());
> +
> + TestArgs converter = new TestArgs(request) {
> +
> + @Override
> + protected Function<Object, IterableWithMarker<String>> markerToNextForArgs(List<Object> args) {
> + fail();
Sure!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70/files#r5345800
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Ignasi Barrera <no...@github.com>.
> + @SuppressWarnings("unchecked")
> + @Override
> + public I setContext(HttpRequest request) {
> + this.request = GeneratedHttpRequest.class.cast(request);
> + return (I) this;
> + }
> +
> + /**
> + * Used to propagate the invoked method arguments during an advance in a
> + * {@link PagedIterable}. For example, in the call
> + * {@code api.getUserApiForZone(zone).list()}, the caller arg0 is the value
> + * of {@code zone}, and the callee is {@code UserApi.list()}
> + *
> + * @author Adrian Cole
> + */
> + public abstract static class FromCaller<T, I extends FromCaller<T, I>> extends ArgsToPagedIterable<T, I> {
This class has the same behavior than the containing class: it implements the advance operation in paginated collections. Both have an abstract method that given a page marker returns the next page of the result set, but concrete implementations must be created in order to properly advance each paginated collection. These abstract classes just provide the common logic.
The `FromCaller` class is just a helper to, instead of propagating the arguments of the invoked method, propagate the arguments of the caller of that method, but essentially does the same than the containing class. You can have a look at the [ParseImages](https://github.com/jclouds/jclouds-labs-openstack/blob/master/openstack-glance/src/main/java/org/jclouds/openstack/glance/v1_0/functions/internal/ParseImages.java#L65-L91) class in openstack-quantum to see an example where this is used (the arg0 version).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70/files#r5345080
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Andrew Phillips <no...@github.com>.
> + GeneratedHttpRequest request = callerArgs(ImmutableList.of());
> + final IterableWithMarker<String> next = IterableWithMarkers.from(ImmutableSet.of("baz"));
> +
> + TestCallerArgs converter = new TestCallerArgs(request) {
> +
> + @Override
> + protected Function<Object, IterableWithMarker<String>> markerToNextForArgs(List<Object> args) {
> + assertTrue(args.isEmpty());
> + return Functions.constant(next);
> + }
> +
> + };
> +
> + assertEquals(converter.apply(IterableWithMarkers.from(ImmutableSet.of("foo", "bar"), "marker")).concat().toSet(),
> + ImmutableSet.of("foo", "bar", "baz"));
> + }
What is the difference between the `FromCaller` tests and the other ones? Sorry if I'm missing something here...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70/files#r5324245
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + TestArgs converter = new TestArgs(request) {
> +
> + @Override
> + protected Function<Object, IterableWithMarker<String>> markerToNextForArgs(List<Object> args) {
> + assertEquals(args.get(0), "path");
> + return Functions.constant(next);
> + }
> +
> + };
> +
> + assertEquals(converter.apply(IterableWithMarkers.from(ImmutableSet.of("foo", "bar"), "marker")).concat().toSet(),
> + ImmutableSet.of("foo", "bar", "baz"));
> + }
> +
> + private GeneratedHttpRequest args(ImmutableList<Object> args) {
The checked exceptions here are the ones generated by the `String.class.getMethod("toString")` call (`SecurityException` and `NoSuchMethodException`). As this method is just a helper that is used to build a dummy GeneratedHttpRequest and is not supposed to fail (all objects have the `toString` method), I preferreed not to add those exceptions to the test signatures, as they are not related to the test itself.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70/files#r5345718
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Ignasi Barrera <no...@github.com>.
Closed #70.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Ignasi Barrera <no...@github.com>.
Merged!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70#issuecomment-21540824
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #75](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/75/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70#issuecomment-21469089
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #527](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/527/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70#issuecomment-21351810
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #212](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/212/) 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/70#issuecomment-21356438
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by Andrew Phillips <no...@github.com>.
> jclouds-java-7-pull-requests #532 FAILURE
Test failure looks unrelated. The added commit (thanks, @nacx!) only updates documentation, so I'm pretty sure the successful results from the [previous jclouds-java-7-pull-requests invocation](https://github.com/jclouds/jclouds/pull/70#issuecomment-21422372) are still valid.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70#issuecomment-21478844
Re: [jclouds] Generalized the Arg0ToPagedIterable to propagate all
args (#70)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #71](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/71/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/70#issuecomment-21356828