Sync pagination implementation #207

varunnvs92 · 2017-10-12T22:23:11Z

Description

Implementation for Sync automatic de-pagination. Supports lists, nested shapes in paginators.json file
Tracking Issue: #26

For all paginated operations, there will be two APIs - one with automatic depagination and one that returns a single page. Example: For listTables operation in DynamoDB, the following APIs are present in the client:

ListTablesResponse listTables(ListTablesRequest listTablesRequest) - Returns only a single page. similar to v1
ListTablesPaginator listTablesIterable(ListTablesRequest listTablesRequest)
This is the auto paginated API in which SDK will make service calls for the customer.

As per recent api review discussions and inconsistencies in paginator files, going with this option is safe in terms of backwards compatibility. Coming to discoverability, these APIs are highly discoverable compared to high level utilities as they are in same client.

Recent changes

Added more unit tests
Added Javadocs for generated classes

Testing

mvn clean install
Added an integ test class

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have read the README document
I have added tests to cover my changes
All new and existing tests passed
A short description of the change has been added to the CHANGELOG

License

I confirm that this pull request can be released under the Apache 2 license

shorea

No major concerns so far. It would be nice to see the generated code to get a sense for what that looks like.

shorea · 2017-10-13T14:30:09Z

...en/src/main/java/software/amazon/awssdk/codegen/emitters/tasks/PaginatorsGeneratorTasks.java

+        info("Emitting paginator classes");
+        List<GeneratorTask> tasks = new ArrayList<>();
+
+        model.getPaginators().entrySet().stream()


You can just collect into a list here.

+1. changed

shorea · 2017-10-13T14:31:35Z

codegen/src/main/java/software/amazon/awssdk/codegen/model/intermediate/IntermediateModel.java

@@ -70,14 +75,16 @@ public IntermediateModel(
            CustomizationConfig customizationConfig,
            ServiceExamples examples,
            Map<String, WaiterDefinitionModel> waiters,
-            Map<String, AuthorizerModel> customAuthorizers) {
+            Map<String, AuthorizerModel> customAuthorizers,


Builder might be nice here.

shorea · 2017-10-13T14:32:12Z

codegen/src/main/java/software/amazon/awssdk/codegen/model/intermediate/IntermediateModel.java

@@ -169,6 +180,10 @@ public boolean getHasWaiters() {
        return waiters.size() > 0;
    }

+    public boolean getHasPaginators() {


This weird naming seems like something we would do for FTL. Do we need it for the poet approach?

waiters has the same naming. I thought it was used for serialization/deserialization. We might not need it. I can change it to hasPaginators()

shorea · 2017-10-13T14:36:19Z

codegen/src/main/java/software/amazon/awssdk/codegen/utils/PaginatorUtils.java

+     * @return the name of the pagination enabled operation
+     *
+     */
+    public static String getSyncMethodName(String methodName) {


Would this make more sense in NamingStrategy?

DefaultNamingStrategy constructor is coupled to ServiceModel and CustomizationConfig. But the method doesn't require any of them. The classes where I use this method don't have enough information to create a NamingStrategy object.

shorea · 2017-10-13T14:37:18Z

core/src/main/java/software/amazon/awssdk/pagination/Paginated.java

+ */
+public interface Paginated<PageT, ItemT> extends SdkIterable<PageT> {
+
+    PageT firstPage();


If we are going with generating both request/response and paginated methods I question the usefulness of this.

Customers can use this as return type instead of operation specific class.
For example,
Paginated<ListTablesResponse, String> response = dbClient.listTables();
Instead of
ListTablesPaginator response = dbClient.listTables();

Sorry I meant the firstPage specifically. That was added mainly when we were thinking of only exposing the paginator but if that's no longer true I don't think it holds it's weight.

We are eagerly loading the responses. So if a customer want a single page and uses next() method, SDK makes two service calls. This method might help in that case.

If they just want a single page they can use the method that returns a single page. The only reason this was added was because we weren't planning on exposing the low level API method. Also I'm a little unsure about making service calls the customer didn't ask for, can we make it lazy loaded? You may still have to preemptively fetch the next page if hasNext is called (and you are at the end of a page boundary).

You may still have to preemptively fetch the next page if hasNext is called (and you are at the end of a page boundary).

We don't want to make service calls in hasNext(). It can be called multiple times before next() method is called. I didn't get the statement about end of page boundary.

shorea · 2017-10-13T14:39:32Z

core/src/main/java/software/amazon/awssdk/pagination/PaginatedItemsIterable.java

+
+public class PaginatedItemsIterable<ResponseT, ItemT> implements SdkIterable<ItemT> {
+
+    private final Iterator<ResponseT> responseIterator;


Should these be iterables too? Otherwise it seems you can only iterate it once.

Everytime this object is constructed, a new object of iterator is passed. So if customer tries to use the API to iterate through response/paginated member multiple items, it works fine. I have added a sample paginator class in the description.

Hmm I'll have to see how this is used in the generated code but I assume it goes something like this.

// I assume this creates the PaginatedItemsIterable SdkIterable<Reservation> reservations = ec2.describeInstances().allItems(); // Iterate once for(Reservation r : reserverations) {...} // What happens when I iterate again? for(Reservation r : reserverations) {...}

edit - Okay I see the generated code, yeah I think that will be a problem then right?

Good catch. As discussed offline, will expose a PaginatedResponsesIterator instead of the PaginatedResponsesIterable and pass the paginator ref to this class

shorea · 2017-10-13T14:40:30Z

core/src/main/java/software/amazon/awssdk/pagination/PaginatedItemsIterable.java

+        return new ItemIterator(responseIterator);
+    }
+
+    private class ItemIterator implements Iterator<ItemT> {


Might as well make this static and pass in the getPaginatedItemIterator too

The itemsIterator in the class is separate for each instance. I am not able to use the generics (ItemT) if I make the class static.

Oh right, can you just reference the outer field rather than pass in responseIterator? Just feels weird to pass one in but not the other.

shorea · 2017-10-13T14:44:02Z

core/src/main/java/software/amazon/awssdk/pagination/PaginatedItemsIterable.java

+                throw new NoSuchElementException();
+            }
+
+            if (itemsIterator == null || !itemsIterator.hasNext()) {


itemsIterator can never be null here right?

I can't think of a null case. Added it as a safety incase I overlooked any edge case.

shorea · 2017-10-13T14:48:36Z

core/src/main/java/software/amazon/awssdk/pagination/PaginatedItemsIterable.java

+
+        @Override
+        public boolean hasNext() {
+            return (itemsIterator != null && itemsIterator.hasNext()) ||


For items iteration it's possible for the responseIterator to have a next page but that next page have an empty collection of items. We should make sure we are handling that case correctly in accordance with the Iterator interface.

Is it possible for a page to have empty items? Then what is the use of that page? At this point in code, we will throw an exception as there is no next element. Its tricky to handle this case as we don't know if the page with empty items is the last page or not.

Yeah it's possible, at least for Dynamo and I suspect for other services too. I think it's just an implementation limitation, the service may not know what the last item is until it goes to fetch it so if you end a page on the last item it thinks there might be more when in reality there's not. The only way to check for sure is to make another call and get an emtpy response with a empty paging token.

http://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_Query.html

The primary key of the item where the operation stopped, inclusive of the previous result set. Use this value to start a new operation, excluding this value in the new request.
If LastEvaluatedKey is empty, then the "last page" of results has been processed and there is no more data to be retrieved.
If LastEvaluatedKey is not empty, it does not necessarily mean that there is more data in the result set. The only way to know when you have reached the end of the result set is when LastEvaluatedKey is empty.

dagnir · 2017-10-13T15:34:21Z

core/src/main/java/software/amazon/awssdk/pagination/PaginatedResponsesIterable.java

+    private final Function<ResponseT, ResponseT> getNextResponse;
+
+    public PaginatedResponsesIterable(ResponseT firstResponse,
+                                      Function<ResponseT, ResponseT> getNextResponse) {


Instead of Function can we create an interface like NextResponseSupplier and annotate with @FunctionalInterface instead? That way we can have a more descriptive method like nextResponse instead of apply

Ditto for PaginatedItemsIterable.

I am okay with this change. Will do it

Added an interface for NextResponseSupplier.
I am not sure if I want to do the same for getIterator in PaginatedItemsIterable class. The result_key can be a list and we need to generate multiple classes for each key in the list. This will bloat the response class for paginated APIs.

kiiadi · 2017-10-18T18:17:44Z

re: naming, instead of listTablesAutoPaginated() can we call it something shorter? Maybe listTablesPaged() or listTablesPaginated() (I don't love these either but I think shorter is better)

millems · 2017-10-18T22:05:08Z

To join the naming discussion, is it possible to come up with a name that follows our service API standards, since this will be on the service clients?

listTablesPaginated or listTablesPaged don't follow them, since the adjective would need to be before the noun. We don't have a way for separating the verb from the noun (and it would hurt discoverability), so could we possibly come up with a noun to use at the end instead of an adjective?

I'm probably just throwing fuel on the fire, though since I can't come up with an ending noun that doesn't require us to depluralize the resource's noun. listTablesPages seems weird (even if it's technically correct - it's a List<List<Tables>>).

kiiadi · 2017-10-18T23:20:48Z

listTablesIterable

varunnvs92 · 2017-10-19T00:12:13Z

listTablesIterable

Sounds better as per english semantics. But It doesn't convey that the operation is paginated.

spfink · 2017-10-27T16:37:16Z

codegen/src/main/java/software/amazon/awssdk/codegen/internal/Jackson.java

@@ -43,6 +43,8 @@
        MAPPER.configure(JsonParser.Feature.ALLOW_COMMENTS, true);
        // TODO: Un comment this once we know for sure, we capture everything in C2j model.
        MAPPER.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
+        // TODO: Risky to enable for all deserialiation?
+        MAPPER.enable(DeserializationFeature.ACCEPT_SINGLE_VALUE_AS_ARRAY);


What's this for?

In paginators.json, the values for input_token, output_token and result_key can be a single value or a list of values.
Example:
"result_key": ["Folders", "Documents"]
"result_key": "Users"

I am representing these fields as List in PaginatorDefinition class. When deserializing the json file into PaginatorDefinition class, this property has to be set so mapper knows to accept a single value into a list.

spfink · 2017-10-27T16:37:59Z

codegen/src/main/java/software/amazon/awssdk/codegen/model/intermediate/IntermediateModel.java

@@ -52,6 +53,9 @@
    @JsonIgnore
    private final Map<String, WaiterDefinitionModel> waiters;

+    @JsonIgnore
+    private final Map<String, PaginatorDefinition> paginators;


Can we keep consistent naming? For waiters, its WaiterDefinitionModel.

Waiters are removed. Keep it as PaginatorDefinition.

spfink · 2017-10-27T16:39:23Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/SyncClientClass.java

@@ -147,14 +148,30 @@ private MethodSpec constructorWithAdvancedConfiguration() {
        List<MethodSpec> methods = new ArrayList<>();
        ClassName returnType = poetExtensions.getModelClass(opModel.getReturnType().getReturnType());

-        methods.add(SyncClientInterface.operationMethodSignature(model, opModel)
+        methods.add(SyncClientInterface.operationMethodSignature(model, opModel, opModel.getMethodName())


Why the change here?

In operationMethodSignature method body, opModel.getMethodName() was being used operation name. For paginators enabled APIs, the name is different. I added the extra parameter in method signature so I can reuse operationMethodSignature() for both normal and pagination enabled APIs.

spfink · 2017-10-27T16:39:49Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/SyncClientClass.java

+                                                                     PaginatorUtils.getSyncMethodName(opModel.getMethodName()))
+                    .addAnnotation(Override.class)
+                    .returns(poetExtensions.getResponseClassForPaginatedSyncOperation(opModel.getOperationName()))
+                    .addCode("\n")


Why do we need this?

If the comment in only for adding new line, I don't see it in my latest revision. I might have added to have clear distinction if the body was complex.
Let me know if I misunderstood the comment.

Actually the code is still there, I was looking at a different class. The new lines are to make the method body look clear in the generated class. I don't have a strong opinion on them.

spfink · 2017-10-27T16:40:21Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/SyncClientClass.java

                                  .addAnnotation(Override.class)
                                  .addCode(getCustomResponseHandler(opModel, returnType)
                                               .orElseGet(() -> protocolSpec.responseHandler(opModel)))
                                  .addCode(protocolSpec.errorResponseHandler(opModel))
                                  .addCode(protocolSpec.executionHandler(opModel))
                                  .build());

+        // Add extra methods for paginated operations
+        if (model.getPaginators().keySet().contains(opModel.getOperationName())) {


Can we move this to it's own method?

spfink · 2017-10-27T16:40:33Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/SyncClientInterface.java

                            .addModifiers(Modifier.DEFAULT)
                            .addStatement("throw new $T()", UnsupportedOperationException.class)
                            .build());

        methods.addAll(streamingSimpleMethods(opModel));

+        // Add extra methods for paginated operations
+        if (model.getPaginators().keySet().contains(opModel.getOperationName())) {


Move code to its own method

spfink · 2017-10-27T16:43:31Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/model/PaginatorResponseClassSpec.java

+
+        public SdkIterable&lt;String> tableNames() {
+            Function&lt;ListTablesResponse, Iterator&lt;String>> getPaginatedMemberIterator = response -> response != null
+                            ? response.tableNames().iterator() : null;


If response is null, I am returning null. This is used in PaginatedItemsIterable which checks has a check to handle null values. Are you suggesting to use empty iterator?

varunnvs92 · 2017-10-27T18:54:05Z

Made changes based on spfink@ feedback

zoewangg · 2017-10-31T19:34:04Z

...amodb/src/it/java/software/amazon/awssdk/services/dynamodb/ScanPaginatorIntegrationTest.java

+                                             .build());
+
+        TableUtils.waitUntilActive(dynamo, TABLE_NAME);
+//        new DynamoDBClientWaiters(dynamo)


let's remove the commented-out codes

zoewangg · 2017-10-31T19:34:09Z

codegen/src/main/java/software/amazon/awssdk/codegen/model/service/PaginatorDefinition.java

+    @JsonProperty("limit_key")
+    private String limitKey;
+
+    public PaginatorDefinition() {


default constructor probably not required.

This is the code style we follow for classes with just default constructor.

zoewangg · 2017-10-31T19:34:13Z

codegen/src/main/java/software/amazon/awssdk/codegen/model/service/Paginators.java

+ */
+public class Paginators {
+
+    public static final Paginators NONE = new Paginators(Collections.emptyMap());


maybe rename it to EMPTY_PAGINATOR or NONE_PAGINATOR?

I'm a fan of NONE since it's clear from the qualifier, Paginators.NONE

zoewangg · 2017-10-31T19:34:17Z

codegen/src/main/java/software/amazon/awssdk/codegen/utils/PaginatorUtils.java

+
+package software.amazon.awssdk.codegen.utils;
+
+public class PaginatorUtils {


final on class and private constructor?

shorea · 2017-10-31T21:41:35Z

codegen/src/main/java/software/amazon/awssdk/codegen/emitters/tasks/AwsGeneratorTasks.java

@@ -33,7 +33,8 @@ public AwsGeneratorTasks(GeneratorTaskParams params) {
    }

    private Iterable<GeneratorTask> createAwsTasks(GeneratorTaskParams params) {
-        return new CompositeIterable<>(new AsyncClientGeneratorTasks(params));
+        return new CompositeIterable<>(new AsyncClientGeneratorTasks(params),


Can you add a TODO for the async generator tasks. It's something we'll defintely wanna push up to the generic tasks.

shorea · 2017-10-31T21:42:53Z

codegen/src/main/java/software/amazon/awssdk/codegen/internal/Jackson.java

@@ -43,6 +43,8 @@
        MAPPER.configure(JsonParser.Feature.ALLOW_COMMENTS, true);
        // TODO: Un comment this once we know for sure, we capture everything in C2j model.
        MAPPER.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
+        // TODO: Risky to enable for all deserialiation?


I think it should be fine.

Okay, removed the TODO. Thanks

shorea · 2017-10-31T21:47:25Z

codegen/src/main/java/software/amazon/awssdk/codegen/model/service/Paginators.java

+ */
+public class Paginators {
+
+    public static final Paginators NONE = new Paginators(Collections.emptyMap());


I'm a fan of NONE since it's clear from the qualifier, Paginators.NONE

shorea · 2017-10-31T21:51:05Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/model/PaginatorResponseClassSpec.java

+ *
+ * Sample of a generated class with annotations:
+
+    public final class ListTablesPaginator implements Paginated&lt;ListTablesResponse, String> {


Probably would remove this from the javadocs and just have it in the diff test.

Ok, what is diff test? Do you mean a new test class to test the generation of sample paginators file?

shorea · 2017-10-31T21:52:28Z

core/src/main/java/software/amazon/awssdk/pagination/NextPageSupplier.java

+package software.amazon.awssdk.pagination;
+
+@FunctionalInterface
+public interface NextPageSupplier<ResponseT> {


Feels more internal.

shorea · 2017-10-31T21:53:07Z

core/src/main/java/software/amazon/awssdk/pagination/PaginatedItemsIterable.java

+@SdkProtectedApi
+public class PaginatedItemsIterable<ResponseT, ItemT> implements SdkIterable<ItemT> {
+
+    private final Paginated paginator;


Should this have some kind of generics?

Yep. I actually removed the Paginated interface as we are removing the main allItems() method from it. Not the response class directly extends SdkIterable instead of Paginated.
Ex:
public final class ListTablesPaginator implements SdkIterable
instead of
public final class ListTablesPaginator implements Paginated<ListTablesResponse, String>

Let me know if you have any concerns?

shorea · 2017-10-31T21:54:37Z

core/src/main/java/software/amazon/awssdk/pagination/PaginatedItemsIterable.java

+                throw new NoSuchElementException();
+            }
+
+            // Using while loop here to handle response pages with empty collection of items


Is this possible?

This is to handle the case you mentioned that there can be pages with empty collection of items. In this code, we retrieve the next page until there is a next element in itemsIterator or no more response pages.
If there is next element in itemsIterator, we return it.
If there is no response page, it means there are no more elements to return and so we return null. I am not satisfied with this. But I don't like throwing error as hasNext() before this next() call would have returned true.

shorea · 2017-10-31T21:54:54Z

core/src/main/java/software/amazon/awssdk/pagination/PaginatedItemsIterable.java

+
+
+            if (!itemsIterator.hasNext()) {
+                // TODO throw error or return null


Somethings fishy here.

See above comment. I am not satisfied with it too. This happens only in cases with empty item collection in last page. I don't see a good way to handle it. Any ideas?

shorea · 2017-10-31T21:56:00Z

core/src/main/java/software/amazon/awssdk/pagination/PaginatedResponsesIterator.java

+ */
+public class PaginatedResponsesIterator<ResponseT> implements Iterator<ResponseT> {
+
+    private final NextPageSupplier<ResponseT> nextPageSupplier;


Supplier makes me think of a factory. This is really more of a function. NextPageFetcher?

Yep, this is a function. Based on Dongie's comment to have better naming instead of apply(), created this NextPageSupplier functional interface. I am okay with both names.

@aws/aws-java-sdk
Anyone else want to chime in their opinion with naming - NextPageSupplier vs NextPageFetcher?

NextPageFetcher +1

shorea · 2017-10-31T21:56:56Z

core/src/main/java/software/amazon/awssdk/pagination/PaginatedResponsesIterator.java

+
+        ResponseT oldValue = currentResponse;
+
+        currentResponse = nextPageSupplier.nextPage(currentResponse);


The preemptive fetch seems weird to me. I'm hestitant to make service calls the customer didn't really ask for.

kiiadi

Probably want a change log entry for this too since it's a sizable feature.

shorea · 2017-11-02T15:51:45Z

core/src/main/java/software/amazon/awssdk/core/pagination/PaginatedResponsesIterator.java

+    @Override
+    public ResponseT next() {
+        if (!hasNext()) {
+            throw new NoSuchElementException();


Perhaps a message here like "No more pages left".

shorea · 2017-11-02T15:53:14Z

core/src/main/java/software/amazon/awssdk/core/pagination/PaginatedItemsIterable.java

+
+        @Override
+        public boolean hasNext() {
+            return (singlePageItemsIterator != null && singlePageItemsIterator.hasNext()) ||


I think to preserve the semantics of Iterator interface you need to do the loop here. You can't know if there are more items just by the presence of another page.

Thats true. We have to retrieve the next page to verify that right? But we discussed that we should not make service calls in hasNext() method.

For Items iteration I can't see another way to do it. I think it's more acceptable in that case since you have given up control of when service calls are made already (i.e. there is no contract of where page boundaries are and when service calls are made). If there's a way to not make that call and still preserve the Iterator contract I'm all for it, but I don't think thats possible.

shorea · 2017-11-02T15:55:01Z

...rc/test/resources/software/amazon/awssdk/codegen/poet/client/test-json-client-interface.java

+     * @see <a href="http://docs.aws.amazon.com/goto/WebAPI/json-service-2010-05-08/PaginatedOperationWithoutResultKey"
+     *      target="_top">AWS API Documentation</a>
+     */
+    default PaginatedOperationWithoutResultKeyPaginator paginatedOperationWithoutResultKeyIterable(


If we are going with the Iterable naming can we change Paginator to just Iterable? I.E. ListTablesIterable.

I had same idea but thought keeping paginator in return type will be more clear to customer that it is a paginated (or atleast some special) API.

shorea · 2017-11-02T16:09:17Z

...rces/software/amazon/awssdk/codegen/poet/model/PaginatedOperationWithResultKeyPaginator.java

+import software.amazon.awssdk.services.jsonprotocoltests.model.SimpleStruct;
+
+/**
+ * Represents the output for the


This is some really nice Javadoc! Would it be more discoverable on the operation method itself?

Will add docs in client too. Was figuring out some formatting issues

shorea · 2017-11-02T16:09:54Z

...rces/software/amazon/awssdk/codegen/poet/model/PaginatedOperationWithResultKeyPaginator.java

+ * guarantee that your request is valid. As you iterate through this iterable, SDK will lazily load response pages by
+ * making service calls until there are no pages left or your iteration stops.
+ *
+ * Following are few ways to iterate through the response pages:


The following
Here are

shorea · 2017-11-02T16:17:35Z

...rces/software/amazon/awssdk/codegen/poet/model/PaginatedOperationWithResultKeyPaginator.java

+            if (oldPage == null) {
+                return client.paginatedOperationWithResultKey(firstRequest);
+            } else {
+                return client.paginatedOperationWithResultKey(firstRequest.toBuilder().nextToken(oldPage.nextToken()).build());


Minor but perhaps just a ternary here.

return client.paginatedOperationWithResultKey(firstRequest.toBuilder() .nextToken(oldPage == null ? null : oldPage.nextToken()) .build();

The output_token is a list i.e., there might be multiple values in response that is needed to get the next page. I feel ternary becomes convoluted to read and i prefer to keep the if-else.

return client.paginatedOperationWithResultKey(firstRequest.toBuilder() .nextToken1(oldPage == null ? null : oldPage.nextToken1()) .nextToken2(oldPage == null ? null : oldPage.nextToken2()) .build();

Minor, if using if-else, It might be cleaner to remove the else.

if (oldPage == null) { return client.paginatedOperationWithResultKey(firstRequest); } return client.paginatedOperationWithResultKey(firstRequest.toBuilder().nextToken(oldPage.nextToken()).build());

shorea · 2017-11-02T16:18:46Z

...software/amazon/awssdk/codegen/poet/paginators/PaginatedOperationWithResultKeyPaginator.java

+ * {@link JsonProtocolTestsClient#paginatedOperationWithResultKey(PaginatedOperationWithResultKeyRequest)} API.
+ */
+@Generated("software.amazon.awssdk:codegen")
+public final class PaginatedOperationWithResultKeyPaginator implements SdkIterable<PaginatedOperationWithResultKeyResponse> {


Looks like there are two of these?

Yep was aware of it. Some issue with loading the file in paginators package. Will fix it.

shorea · 2017-11-02T16:19:06Z

...tware/amazon/awssdk/codegen/poet/paginators/PaginatedOperationWithoutResultKeyPaginator.java

+
+    private final NextPageFetcher nextPageFetcher;
+
+    public PaginatedOperationWithoutResultKeyPaginator(final JsonProtocolTestsClient client,


Looks like there are two of these?

same as above.

shorea · 2017-11-02T16:31:40Z

core/src/test/java/software/amazon/awssdk/core/pagination/PaginatedItemsIterableTest.java

+
+public class PaginatedItemsIterableTest {
+
+    private SdkIterable pagesIterable = Mockito.mock(SdkIterable.class);


If you do the RunWith mockito runner you can annotate these with just Mock

Oh thats it. I tried with @mock annotations but I was getting NPE. I forgot to annotate the class with MockitoJUnitRunner.

shorea · 2017-11-02T16:37:35Z

core/src/test/java/software/amazon/awssdk/core/pagination/PaginatedItemsIterableTest.java

+    }
+
+    @Test
+    public void nextMethodDoesnotRetrieveNextPage_WhenItemsIteratorHasAnElement() {


spfink · 2017-11-09T21:35:53Z

codegen/src/main/java/software/amazon/awssdk/codegen/docs/PaginationDocs.java

+    /**
+     * Constructs additional documentation on the client operation that is appended to the service documentation.
+     */
+    public String getDocsForSyncOperation() {


Add a TODO here to link to our developer guide. We'll probably want to go into a little more depth in the guide so a link to it when available would be nice.

spfink · 2017-11-09T21:46:37Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/SyncClientClass.java

                                  .addAnnotation(Override.class)
                                  .addCode(getCustomResponseHandler(opModel, returnType)
                                               .orElseGet(() -> protocolSpec.responseHandler(opModel)))
                                  .addCode(protocolSpec.errorResponseHandler(opModel))
                                  .addCode(protocolSpec.executionHandler(opModel))
                                  .build());

+        // Add extra methods for paginated operations
+        if (opModel.isPaginated()) {
+            methods.add(SyncClientInterface.operationMethodSignature(model, opModel,


If new lining, we typically new line all arguments

spfink · 2017-11-09T21:46:55Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/SyncClientClass.java

+                    .addAnnotation(Override.class)
+                    .returns(poetExtensions.getResponseClassForPaginatedSyncOperation(opModel.getOperationName()))
+                    .addStatement("return new $T(this, $L)",
+                            poetExtensions.getResponseClassForPaginatedSyncOperation(opModel.getOperationName()),


Line these lines up?

spfink · 2017-11-09T21:48:31Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/SyncClientInterface.java

                            .addModifiers(Modifier.DEFAULT)
                            .addStatement("throw new $T()", UnsupportedOperationException.class)
                            .build());

        methods.addAll(streamingSimpleMethods(opModel));

+        // Add extra methods for paginated operations
+        if (opModel.isPaginated()) {


Can we move all specific logic to other methods?

Basically end up with:

methods.addAll(paginatedSimpleMethods());
methods.addAll(paginatedMethods());

I don't know if we want to have separate methods that go through all operations agains to find the paginated operations.

I moved the new code to a new method that takes in an operation model and return the method specs if its paginated operation.
List paginatedMethods(OperationModel opModel)

the call will end up with
methods.addAll(paginatedMethods(opModel));

spfink · 2017-11-09T21:49:06Z

...src/main/java/software/amazon/awssdk/codegen/poet/paginators/PaginatorResponseClassSpec.java

+    private static final String NEXT_PAGE_FETCHER_MEMBER = "nextPageFetcher";
+    private static final String HAS_NEXT_PAGE_METHOD = "hasNextPage";
+    private static final String NEXT_PAGE_METHOD = "nextPage";
+    private static final String OLD_PAGE_METHOD_ARGUMENT = "oldPage";


Previous page?

spfink · 2017-11-09T21:49:16Z

...src/main/java/software/amazon/awssdk/codegen/poet/paginators/PaginatorResponseClassSpec.java

+    public PaginatorResponseClassSpec(IntermediateModel intermediateModel,
+                                      String c2jOperationName,
+                                      PaginatorDefinition paginatorDefinition) {
+


Extra new line

spfink · 2017-11-09T21:52:04Z

...n/src/test/resources/software/amazon/awssdk/codegen/poet/client/test-async-client-class.java

@@ -82,14 +90,14 @@ protected DefaultJsonAsyncClient(AsyncClientConfiguration clientConfiguration) {
    public CompletableFuture<APostOperationResponse> aPostOperation(APostOperationRequest aPostOperationRequest) {

        HttpResponseHandler<APostOperationResponse> responseHandler = protocolFactory.createResponseHandler(
-                new JsonOperationMetadata().withPayloadJson(true).withHasStreamingSuccessResponse(false),


Why the changes in formatting throughout this file?

I copied the expected file when the unit test failed. Then checked if new code looks as expected. I guess the generated file in unit test has different formatting. I tried to auto-format the file now which is having lot more changes

spfink · 2017-11-09T21:52:41Z

...n/src/test/resources/software/amazon/awssdk/codegen/poet/client/test-async-client-class.java

+     *         of this type.</li>
+     *         </ul>
+     * @sample JsonAsyncClient.PaginatedOperationWithResultKey
+     * @see <a href="http://docs.aws.amazon.com/goto/WebAPI/json-service-2010-05-08/PaginatedOperationWithResultKey"


Will this link work?

This is generated for all operations in this class. All of them redirect to AWS documentation official page.

spfink · 2017-11-09T21:53:51Z

...en/src/test/resources/software/amazon/awssdk/codegen/poet/client/test-json-client-class.java

+    }
+
+    /**
+     * Some paginated operation with result_key in paginators.json file


Have you generated the javadoc and made sure this all looks good?

Yes, looks good

spfink · 2017-11-09T21:56:08Z

...amodb/src/it/java/software/amazon/awssdk/services/dynamodb/ScanPaginatorIntegrationTest.java

+    @Test
+    public void test_MultipleIteration_On_Responses_Iterable() {
+        ScanRequest request = ScanRequest.builder().tableName(TABLE_NAME).limit(2).build();
+        ScanPaginator scanResponses = dynamo.scanIterable(request);


Seeing it definitely don't know if I like "scanIterable"

I don't think we will go with "Iterable". We would want to have consistent naming across sync and Async clients. Based on #185, Iterable doesn't make sense for Async operations. So we need change them for sure.

spfink · 2017-11-09T21:56:40Z

Added a few comments. Mostly minor stuff. Def need to figure out naming.

spfink

Fine with where this is now. We might make changes as we start to using the paginators (probably in naming) but think this is good to ship out

shorea reviewed Oct 13, 2017

View reviewed changes

dagnir reviewed Oct 13, 2017

View reviewed changes

varunnvs92 force-pushed the varunkn/SyncPagination branch 3 times, most recently from 860a2bb to 7aefbeb Compare October 18, 2017 17:47

varunnvs92 force-pushed the varunkn/SyncPagination branch 2 times, most recently from 0bb679d to ab338c2 Compare October 19, 2017 06:53

spfink reviewed Oct 27, 2017

View reviewed changes

varunnvs92 force-pushed the varunkn/SyncPagination branch from ab338c2 to d70afcb Compare October 27, 2017 18:53

varunnvs92 changed the title ~~Sync pagination implementation to get initial feedback~~ Sync pagination implementation Oct 27, 2017

varunnvs92 force-pushed the varunkn/SyncPagination branch from d70afcb to b7fe511 Compare October 31, 2017 18:14

zoewangg reviewed Oct 31, 2017

View reviewed changes

shorea reviewed Oct 31, 2017

View reviewed changes

kiiadi reviewed Oct 31, 2017

View reviewed changes

millems mentioned this pull request Nov 1, 2017

Pt1 Remove legacy request/response classes #152

Closed

varunnvs92 force-pushed the varunkn/SyncPagination branch from f3cc14c to db87043 Compare November 2, 2017 08:45

shorea reviewed Nov 2, 2017

View reviewed changes

varunnvs92 force-pushed the varunkn/SyncPagination branch 4 times, most recently from 8aa0122 to af55b8d Compare November 8, 2017 17:57

spfink reviewed Nov 9, 2017

View reviewed changes

varunnvs92 force-pushed the varunkn/SyncPagination branch 2 times, most recently from f39cc29 to 3930680 Compare November 10, 2017 23:33

spfink approved these changes Nov 13, 2017

View reviewed changes

varunnvs92 force-pushed the varunkn/SyncPagination branch 5 times, most recently from 5f024f6 to 3d360ee Compare November 14, 2017 01:39

Added support for Sync pagination

dc0a9cc

varunnvs92 force-pushed the varunkn/SyncPagination branch from 3d360ee to dc0a9cc Compare November 14, 2017 01:46

varunnvs92 merged commit 75c1ba7 into master Nov 14, 2017

zoewangg deleted the varunkn/SyncPagination branch December 6, 2017 19:16


		public class PaginatedItemsIterable<ResponseT, ItemT> implements SdkIterable<ItemT> {

		private final Iterator<ResponseT> responseIterator;


		package software.amazon.awssdk.codegen.utils;

		public class PaginatorUtils {



		if (!itemsIterator.hasNext()) {
		// TODO throw error or return null


		ResponseT oldValue = currentResponse;

		currentResponse = nextPageSupplier.nextPage(currentResponse);


		private final NextPageFetcher nextPageFetcher;

		public PaginatedOperationWithoutResultKeyPaginator(final JsonProtocolTestsClient client,


		public class PaginatedItemsIterableTest {

		private SdkIterable pagesIterable = Mockito.mock(SdkIterable.class);

Sync pagination implementation #207

Sync pagination implementation #207

Uh oh!

Conversation

varunnvs92 commented Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Recent changes

Testing

Types of changes

Checklist

License

Uh oh!

shorea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shorea Oct 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiiadi commented Oct 18, 2017

Uh oh!

millems commented Oct 18, 2017

Uh oh!

kiiadi commented Oct 18, 2017

Uh oh!

varunnvs92 commented Oct 12, 2017 •

edited

Loading

shorea Oct 13, 2017 •

edited

Loading

varunnvs92 commented Oct 19, 2017 •

edited

Loading