private/mode/api: refactoring paginators #119

xibz · 2018-02-07T19:10:09Z

Pagination will be typed now and users will be able to grab a aws.Pager from the API requests.

svc := s3.New(cfg)
req := svc.ListObjectsRequest(input)
p := req.Paginate()

for p.Next() {
    page := p.CurrentPage()
    fmt.Println("List Objects' contents", len(page.Contents))
}

if err := p.Err(); err != nil {
    fmt.Println("unexpected error occurred", err.Error())
}

jasdel

Looks like a few merge issues in the codebase, the protocol benchmark files have merge issues, or not updated like they should of been with the gen marshaler change.

jasdel · 2018-02-08T00:21:29Z

aws/request_pagination.go

 //
 // Will always return true if Next has not been called yet.
-func (p *Pagination) HasNextPage() bool {
+func (p *Pager) hasNextPage() bool {


out of curiosity why not export this?

With the new design, I couldn't think of a good reason to export it.

jasdel · 2018-02-08T00:22:34Z

aws/request_pagination_test.go


 	"github.com/aws/aws-sdk-go-v2/aws"
-	"github.com/aws/aws-sdk-go-v2/internal/awstesting"
+	"github.com/aws/aws-sdk-go-v2/aws/defaults"


so much red! :)

jasdel · 2018-02-08T00:25:43Z

aws/request_pagination_test.go


-// Use DynamoDB methods for simplicity
-func TestPaginationEachPage(t *testing.T) {
+/*// Use DynamoDB methods for simplicity


large block of commented code here.

jasdel · 2018-02-08T00:26:14Z

aws/request_pagination_test.go

 		})
 	}
-}
+}*/


can this block be uncommented, or tested directly?

jasdel · 2018-02-08T00:27:14Z

private/model/api/operation.go

 	"GetCrosslinkURL": GetCrosslinkURL,
 }).Parse(`
 {{ $reqType := printf "%sRequest" .ExportedName -}}
+{{ $pagerType := printf "%sPager" .ExportedName -}}


We want to make sure this __Pager type won't collide with modeled shapes.

Ah! Good point. Will fix.

jasdel · 2018-02-08T00:28:07Z

private/model/api/operation.go

+				}
+
+				var output {{ .OutputRef.GoTypeElem }}
+				req := aws.New(p.Request.Config, p.Request.Metadata, p.Request.Handlers.Copy(), p.Request.Retryer, p.Request.Operation, inCpy, &output)


Not directly related to this PR, but we should rename the aws.New method to aws.NewRequest

jasdel · 2018-02-08T00:29:54Z

private/model/api/operation.go

+				}
+
+				var output {{ .OutputRef.GoTypeElem }}
+				req := aws.New(p.Request.Config, p.Request.Metadata, p.Request.Handlers.Copy(), p.Request.Retryer, p.Request.Operation, inCpy, &output)


If there are in custom code generated into the API's __Request method this will bypass that code. e.g. Signing logic, ect.

For example its possible for the __Request logic to have a closure referencing some instance value such as a output shape. This code probably should just use the base __Request instead of bypasing it.

Okay, that makes sense.

jasdel · 2018-02-08T00:36:39Z

private/protocol/ec2query/build_bench_test.go

@@ -1,85 +0,0 @@
-// +build bench


Think this is a merge issue, these benchmark files have been removed.

jasdel · 2018-02-08T00:37:28Z

private/protocol/restjson/build_bench_test.go

 	"net/http"
 	"net/http/httptest"
 	"os"
 	"testing"


Shouldn't these files of been updated in the generated marshaler change?

Yea, this is a rebase issue.

jasdel

Looks good few minor things. specifically with the s3manager batch utility using the Paginate() value provided instead of duplicating the logic, and a test that would be good to update.

jasdel · 2018-02-09T00:07:22Z

service/s3/s3manager/batch.go

@@ -162,7 +161,7 @@ func (iter *DeleteListIterator) Next() bool {
 	}

 	if len(iter.objects) == 0 && iter.Paginator.Next() {
-		iter.objects = iter.Paginator.Page().(s3.ListObjectsOutput).Contents
+		iter.objects = iter.Paginator.CurrentPage().(s3.ListObjectsOutput).Contents


The s3/s3manager/DeleteBatch can be updated to use the Pager created by S3.ListObjectsRequest.Paginate() method.

This would also get rid of the closure here. If there are other places that have the aws.Pager{NewRequest:...} like this they probably can be updated as well.

func NewDeleteListIterator(... pager := svc.ListObjectsRequest(input).Paginate() iter := &DeleteListIterator{ Bucket: input.Bucket, Paginator: pager.Pager, } //... }

jasdel · 2018-02-09T00:08:29Z

example/service/s3/listObjects/listObjects.go

-
-		for _, obj := range p.Contents {
-			fmt.Println("Object:", *obj.Key)
+	req := svc.ListObjectsRequest(&s3.ListObjectsInput{Bucket: &os.Args[1]})


Noticed the listObjectsConcurrently example isn't using the paginator like it should be. Only the first page is used in that example. If you don't mind would be good to update that one as well.

jasdel · 2018-02-09T00:11:02Z

service/s3/s3manager/batch_test.go

-		Paginator: request.Pagination{
-			NewRequest: func() (*request.Request, error) {
+		Paginator: aws.Pager{
+			NewRequest: func() (*aws.Request, error) {


These tests can be updated to use the ListObjectsRequest(input).Paginate() returned pager value.

Services --- * Synced the V2 SDK with latests AWS service API definitions. Breaking Changes --- * `private/mode/api`: Refactor service paginator helpers to iterator pattern ([aws#119](aws#119)) * Refactors the generated service client paginators to be an iterator pattern. This pattern improves usability while removing the need for callbacks. * See the linked PR for an example. * `private/model/api`: Removes setter helpers from service API types ([aws#101](aws#101)) * Removes the setter helper methods from service API types. Removing clutter and noise from the API type's signature. * Based on feedback [aws#81][aws#81] * `aws`: Rename CanceledErrorCode to ErrCodeRequestCanceled ([aws#131](aws#131)) * Renames CanceledErrorCode to correct naming scheme. SDK Bugs --- * `internal/awsutil`: Fix DeepEqual to consider string alias type equal to string ([aws#102](aws#102)) * Fixes SDK waiters not detecting the correct condition is met. [aws#92](aws#92) * `aws/external`: Fix EnvConfig misspelled container endpoint path getter ([aws#106](aws#106)) * This caused the type to not satisfy the ContainerCredentialsEndpointPathProvider interface. * Fixes [aws#105](aws#105) * `service/s3/s3crypto`: Fix S3Crypto's handling of TagLen ([aws#107](aws#107)) * Fixes the S3Crypto's handling of TagLen to only be set if present. * V2 Fix for [aws/aws-sdk-go#1742](aws/aws-sdk-go#1742) * `private/model/api`: Update SDK service client initialization documentation ([aws#141](aws#141)) * Updates the SDK's service initialization doc template to reflect the v2 SDK's configuration update change from v1. * Related to [aws#136](aws#136) SDK Enhancements --- * `aws`: Improve pagination unit tests ([aws#97](aws#97)) * V2 port of [aws/aws-sdk-go#1733](aws/aws-sdk-go#1733) * `aws/external`: Add example for shared config and static credential helper ([aws#109](aws#109)) * Adds examples for the config helpers; WithSharedConfigProfile, WithCredentialsValue, WithMFATokenFunc. * `private/model/api`: Add validation to prevent collision of api defintions ([aws#112](aws#112)) * V2 port of [aws/aws-sdk-go#1758](aws/aws-sdk-go#1758) * `aws/ec2metadata`: Add support for AWS_EC2_METADATA_DISABLED env var ([aws#128](aws#128)) * When this environment variable is set. The SDK's EC2 Metadata Client will not attempt to make requests. All requests made with the EC2 Metadata Client will fail. * V2 port of [aws/aws-sdk-go#1799](aws/aws-sdk-go#1799) * Add code of conduct ([aws#138](aws#138)) * Update SDK README dep usage ([aws#140](aws#140))

Services --- * Synced the V2 SDK with latests AWS service API definitions. Breaking Changes --- * `private/mode/api`: Refactor service paginator helpers to iterator pattern (aws#119) * Refactors the generated service client paginators to be an iterator pattern. This pattern improves usability while removing the need for callbacks. * See the linked PR for an example. * `private/model/api`: Removes setter helpers from service API types (aws#101) * Removes the setter helper methods from service API types. Removing clutter and noise from the API type's signature. * Based on feedback aws#81 * `aws`: Rename CanceledErrorCode to ErrCodeRequestCanceled (aws#131) * Renames CanceledErrorCode to correct naming scheme. SDK Bugs --- * `internal/awsutil`: Fix DeepEqual to consider string alias type equal to string (aws#102) * Fixes SDK waiters not detecting the correct condition is met. aws#92 * `aws/external`: Fix EnvConfig misspelled container endpoint path getter (aws#106) * This caused the type to not satisfy the ContainerCredentialsEndpointPathProvider interface. * Fixes aws#105 * `service/s3/s3crypto`: Fix S3Crypto's handling of TagLen (aws#107) * Fixes the S3Crypto's handling of TagLen to only be set if present. * V2 Fix for aws/aws-sdk-go#1742 * `private/model/api`: Update SDK service client initialization documentation (aws#141) * Updates the SDK's service initialization doc template to reflect the v2 SDK's configuration update change from v1. * Related to aws#136 SDK Enhancements --- * `aws`: Improve pagination unit tests (aws#97) * V2 port of aws/aws-sdk-go#1733 * `aws/external`: Add example for shared config and static credential helper (aws#109) * Adds examples for the config helpers; WithSharedConfigProfile, WithCredentialsValue, WithMFATokenFunc. * `private/model/api`: Add validation to prevent collision of api defintions (aws#112) * V2 port of aws/aws-sdk-go#1758 * `aws/ec2metadata`: Add support for AWS_EC2_METADATA_DISABLED env var (aws#128) * When this environment variable is set. The SDK's EC2 Metadata Client will not attempt to make requests. All requests made with the EC2 Metadata Client will fail. * V2 port of aws/aws-sdk-go#1799 * Add code of conduct (aws#138) * Update SDK README dep usage (aws#140)

Services --- * Synced the V2 SDK with latests AWS service API definitions. Breaking Changes --- * `private/mode/api`: Refactor service paginator helpers to iterator pattern (#119) * Refactors the generated service client paginators to be an iterator pattern. This pattern improves usability while removing the need for callbacks. * See the linked PR for an example. * `private/model/api`: Removes setter helpers from service API types (#101) * Removes the setter helper methods from service API types. Removing clutter and noise from the API type's signature. * Based on feedback #81 * `aws`: Rename CanceledErrorCode to ErrCodeRequestCanceled (#131) * Renames CanceledErrorCode to correct naming scheme. SDK Bugs --- * `internal/awsutil`: Fix DeepEqual to consider string alias type equal to string (#102) * Fixes SDK waiters not detecting the correct condition is met. #92 * `aws/external`: Fix EnvConfig misspelled container endpoint path getter (#106) * This caused the type to not satisfy the ContainerCredentialsEndpointPathProvider interface. * Fixes #105 * `service/s3/s3crypto`: Fix S3Crypto's handling of TagLen (#107) * Fixes the S3Crypto's handling of TagLen to only be set if present. * V2 Fix for aws/aws-sdk-go#1742 * `private/model/api`: Update SDK service client initialization documentation (#141) * Updates the SDK's service initialization doc template to reflect the v2 SDK's configuration update change from v1. * Related to #136 SDK Enhancements --- * `aws`: Improve pagination unit tests (#97) * V2 port of aws/aws-sdk-go#1733 * `aws/external`: Add example for shared config and static credential helper (#109) * Adds examples for the config helpers; WithSharedConfigProfile, WithCredentialsValue, WithMFATokenFunc. * `private/model/api`: Add validation to prevent collision of api defintions (#112) * V2 port of aws/aws-sdk-go#1758 * `aws/ec2metadata`: Add support for AWS_EC2_METADATA_DISABLED env var (#128) * When this environment variable is set. The SDK's EC2 Metadata Client will not attempt to make requests. All requests made with the EC2 Metadata Client will fail. * V2 port of aws/aws-sdk-go#1799 * Add code of conduct (#138) * Update SDK README dep usage (#140)

xibz requested a review from jasdel February 7, 2018 19:10

jasdel suggested changes Feb 8, 2018

View reviewed changes

xibz added 2 commits February 8, 2018 09:45

Refactoring paginator

cfa76ce

Fixing docs and tests

4e45d95

xibz force-pushed the paginator branch 2 times, most recently from 9e4ae2d to 6babf84 Compare February 8, 2018 17:47

regenerating services

08b77fa

xibz force-pushed the paginator branch from 6babf84 to 08b77fa Compare February 8, 2018 17:57

xibz added 2 commits February 8, 2018 13:47

Introducing Copy method on API Requests

fec3bf9

regenerating services

b09aee5

jasdel suggested changes Feb 9, 2018

View reviewed changes

Updating tests to use new paginator and add mocking of paginators

0bd144d

jasdel approved these changes Feb 13, 2018

View reviewed changes

xibz merged commit 5e8e36f into aws:master Feb 13, 2018

jasdel mentioned this pull request Mar 8, 2018

Release v2.0.0-preview.3 (2018-03-08) #147

Merged

jasdel mentioned this pull request Jun 26, 2018

User should be able to paginate a supported API operation from the API's Request type. #36

Closed

private/mode/api: refactoring paginators #119

private/mode/api: refactoring paginators #119

Uh oh!

Conversation

xibz commented Feb 7, 2018

Uh oh!

jasdel 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!

jasdel 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants