-
Notifications
You must be signed in to change notification settings - Fork 670
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
S3 custom paginators #1999
S3 custom paginators #1999
Conversation
service/s3/handwritten-paginators.go
Outdated
|
||
// HasMorePages returns a boolean indicating whether more pages are available | ||
func (p *ListObjectVersionsPaginator) HasMorePages() bool { | ||
return p.firstPage || (p.KeyMarker != nil && len(*p.KeyMarker) != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correctness: Should this be looking at VersionIdMarker
in addition to KeyMarker
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not 100% sure about this.
Each object has a version ID, whether or not S3 Versioning is enabled. If S3 Versioning is not enabled, Amazon S3 sets the value of the version ID to null. If you enable S3 Versioning, Amazon S3 assigns a version ID value for the object. This value distinguishes that object from other versions of the same key.
I can add that field to the validation as a "just in case"
func (p *ListObjectVersionsPaginator) HasMorePages() bool {
return p.firstPage || (p.keyMarker != nil && len(*p.keyMarker) != 0) && (p.versionIdMarker != nil && len(*p.versionIdMarker) != 0)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would take a look at one of the SDKs that already has these paginators (e.g. Java) to understand the correct behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is where this is determined in the Java SDK. It only looks at isTruncated
to determine if there are more pages.
I think it makes sense and is more elegant.
service/s3/handwritten-paginators.go
Outdated
p.KeyMarker = result.NextKeyMarker | ||
p.VersionIdMarker = result.NextVersionIdMarker | ||
|
||
if p.options.StopOnDuplicateToken && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correctness: This doesn't account for VersionIdMarker
being duplicate, should it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the S3 docs:
Only Amazon S3 generates version IDs, and they cannot be edited. Version IDs are Unicode, UTF-8 encoded, URL-ready, opaque strings that are no more than 1,024 bytes long. The following is an example:
3sL4kqtJlcpXroDTDmJ+rmSpXd3dIbrHY+MTRCxf3vjVBH40Nr8X8gdRQBpUMLUo
Since they are unique identifiers, and cannot be set by the user, the chance of collision within a specific bucket are tiny.
What are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think it's more about any one parameter being a duplicate rather than collision being the concern. I'm not 100% sure though, maybe another thing to look to other SDKs w.r.t behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple suggestions to improve the overall tests and paginator implementation.
service/s3/handwritten_paginators.go
Outdated
} | ||
} | ||
|
||
// HasMorePages returns a boolean indicating whether more pages are available | ||
func (p *ListObjectVersionsPaginator) HasMorePages() bool { | ||
return p.firstPage || (p.keyMarker != nil && len(*p.keyMarker) != 0) && (p.versionIdMarker != nil && len(*p.versionIdMarker) != 0) | ||
return p.firstPage || (p.keyMarker != nil && len(*p.keyMarker) != 0) && (p.versionIDMarker != nil && len(*p.versionIDMarker) != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: I think this may have been discussed offline at one point but we should be able to just store and use the IsTruncated
flag on the output which is what Java does.
e.g.
return p.firstPage || p.isTruncated
service/s3/handwritten_paginators.go
Outdated
} | ||
} | ||
|
||
// HasMorePages returns a boolean indicating whether more pages are available | ||
func (p *ListMultipartUploadsPaginator) HasMorePages() bool { | ||
return p.firstPage || (p.keyMarker != nil && len(*p.keyMarker) != 0) && (p.uploadIdMarker != nil && len(*p.uploadIdMarker) != 0) | ||
return p.firstPage || (p.keyMarker != nil && len(*p.keyMarker) != 0) && (p.uploadIDMarker != nil && len(*p.uploadIDMarker) != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: Same thing, store and use the IsTruncated
flag and predicate off that
return fmt.Sprintf("mock middleware %d", m.id) | ||
} | ||
|
||
func (m *testListMPUMiddleware) HandleInitialize(ctx context.Context, input middleware.InitializeInput, next middleware.InitializeHandler) ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: This shouldn't be necessary. The paginator takes an interface for the client type:
type ListObjectVersionsAPIClient interface {
ListObjectVersions(context.Context, *ListObjectVersionsInput, ...func(*Options)) (*ListObjectVersionsOutput, error)
}
You can just implement a test/mock version of this interface which removes the need for middleware or constructing a real client and allows you to directly control the outputs and observe/record the inputs to make assertions.
e.g.
// this is just an example
type mockListObjectVersionsClient struct {
responses []*ListObjectVersionsOutput
inputs []*ListObjectVersionsOutput
requestCount int
}
func (c *mockListObjectVersionsClient) ListObjectVersions(ctx context.Context, input *ListObjectVersionsInput, options ...func(*Options)) (*ListObjectVersionsOutput, error) {
idx := c.requestCount + 1
if idx > len(c.resopnses) { // error }
c.requestCount = idx
// record the input observed
c.inputs = append(c.inputs, input)
return c.responses[idx], nil
}
Added handwritten paginators for S3's
ListMultipartUploads
andListObjectVersions
.Both are not code generated because of multiple pagination tokens that Smithy does not support.
Related: #1775