Support pagination #247
Support pagination #247
Conversation
This change adds a <Operation>Pages() operation to each operation
with pagination configuration to return an iterator that can be
used like slice iteration:
db := dynamodb.New(nil)
for resp := range db.ListTablesPages() {
fmt.Println(resp)
}
References #58
New API:
func main() {
db := dynamodb.New(nil)
params := &dynamodb.ListTablesInput{Limit: aws.Long(2)}
i := 0
db.ListTablesPages(params, func(p *dynamodb.ListTablesOutput, err error) bool {
if err != nil {
fmt.Println("An error occurred", err)
} else if p != nil {
i++
fmt.Println(i, awsutil.StringValue(p))
} else {
fmt.Println("Finished paging!")
}
return true // return false to stop paging
})
}
Also adds pagination tests.
References #58
New API:
numPages := 0
err := req.EachPage(func(p *dynamodb.ListTablesOutput, last bool) {
// Print a page of data
numPages++
fmt.Printf("Page %d: %x\n", numPages, p.TableNames)
if last { // We are on the last page
fmt.Println("Got to last page")
}
})
if err != nil {
panic(err)
}
- Document use of nil reflect.Value with Call() - Rename "result" to "shouldContinue" - Use "SetValueAtAnyPath" to match case insensitive matches - Copy parameters when sending new request (add test assertion for this)
| func (r *Request) EachPage(fn interface{}) error { | ||
| valfn := reflect.ValueOf(fn) | ||
| if valfn.Kind() != reflect.Func { | ||
| panic("expected function for EachPage()") |
There was a problem hiding this comment.
Wouldn't returning error be more appropriate than panicing?
There was a problem hiding this comment.
Not passing a function to this method would be a programmer error, not some kind of runtime error. It would never make sense to handle an error like that, since there would be no sensible recovery. Basically this should be a compile-time error but Go cannot quite represent the type here. That said, EachPage() is a low-level call that is typically not used unless you know what you're doing. The *Pages() methods have the correct type checking in them.
There was a problem hiding this comment.
That argument makes sense, and I agree with the differentiation between programmer and runtime errors. But the downside of this form of runtime type checking is that you only catch the code paths you execute in your testing. 99% of the time you have a simple case and you hit the panic in development and fix it right away. But a common issue when accepting an interface{} argument is someone passing a pointer when they shouldn't have, or vice versa. And if this happens in an edge case that escaped your testing, you start panicing when releasing to production. Or worst case, code has been live and stable and some external change causes a behavior change which starts executing this bad path.
Of course the dream is 100% test coverage, and recoverer middleware can often stop panics from bubbling to the top for web servers (unless the panic is in a goroutine). But in reality edge cases can go untested, and it's nice when library code is more forgiving.
The Go authors seem to support that stance:
The convention in the Go libraries is that even when a package uses panic internally, its external API still presents explicit error return values.
The above excerpt is from http://blog.golang.org/defer-panic-and-recover.
All that said, I think it's extremely unlikely anyone would hit these particular panics in production, for the reasons you mentioned. But I'm always a little uncomfortable when I see the possibility of panics escaping libraries.
There was a problem hiding this comment.
The unfortunate reality is that simply using reflect already opens us up to arbitrary panics (Go is by definition ignoring its own above stance here), few of which we are guarding from because they are all equally edge-cased programmer errors, but there are cases where, for instance, fn.Call() could fail for various reasons, etc. -- all of those are panics.
Just to reinforce a few things though: the likelihood of an actual panic in production code is really low. You would have to do bad things to create such a state such that your code could not have ever worked. These are mostly just edge case guards to generate "nicer" panics than the ones reflect will emit. The goal isn't to try to defensively guard against these scenarios.
There was a problem hiding this comment.
An option would be to use a recoverer, like the encoding/json package.
But do I agree that this would be extremely rare in production.
Also refactor and add tests for truncation logic
| } | ||
| if valfn.Type().NumOut() > 0 && valfn.Type().Out(0).Kind() != reflect.Bool { | ||
| panic("EachPage(fn) function must return bool if it returns a value") | ||
| } |
There was a problem hiding this comment.
You might also want to check the input types here:
- takes two values
- the first one must be ???
- the second one must be a boolean
Based on a suggestion from @nightlyone we were able to remove the use of reflection. This change subtly changes the API on the low-level Request.EachPage() operation. This does not change the wrapped pager functions for each operation on the service, specifically the "*Pages()" pagination operations. The new EachPage API is: EachPage(fn func(data interface{}, isLastPage bool) (shouldContinue bool)) error Note that "data" is no longer the response type structure but a simple interface{} type. In order to use the structure, the value must be manually asserted to the response type: // Get the tables in a page of data tables := *data.(*dynamodb.ListTablesOutput).Tables It is also no longer possible to omit the "shouldContinue" return value to the function. References #247
Sometimes CI leaks untagged subnets. Because we are allowed to remove all resources from within a cluster-owned VPC, add a ByVPC walker to remove these indirectly-owned subnets. DescribeSubnetsPages has a strange history. It was initially added in aws/aws-sdk-go@3664ecc7da (Add initial implementation of pagination, 2015-03-23, aws/aws-sdk-go#247) but was removed in aws/aws-sdk-go@bad551feb8 (Add support for multi-token pagination rules, 2015-03-27) as a later step in that same pull request. It finally landed in master via aws/aws-sdk-go@52cd98f1ed (Release v1.19.30, 2019-05-14, aws/aws-sdk-go#2599), but we only vendor v1.16.14. It doesn't seem like "zounds of untagged subnets" is a high-probability thing, so I'm just using the unpaginated DescribeSubnets instead of bumping the vendor to pick up DescribeSubnetsPages. Even if we do overflow DescribeSubnets with untagged subnets, VPC deletion will fail and we'll get another pass at deleting tagged subnets when we come around to the next deleteEC2VPC attempt.
Sometimes CI leaks untagged subnets. Because we are allowed to remove all resources from within a cluster-owned VPC, add a ByVPC walker to remove these indirectly-owned subnets. DescribeSubnetsPages has a strange history. It was initially added in aws/aws-sdk-go@3664ecc7da (Add initial implementation of pagination, 2015-03-23, aws/aws-sdk-go#247) but was removed in aws/aws-sdk-go@bad551feb8 (Add support for multi-token pagination rules, 2015-03-27) as a later step in that same pull request. It finally landed in master via aws/aws-sdk-go@52cd98f1ed (Release v1.19.30, 2019-05-14, aws/aws-sdk-go#2599), but we only vendor v1.16.14. It doesn't seem like "zounds of untagged subnets" is a high-probability thing, so I'm just using the unpaginated DescribeSubnets instead of bumping the vendor to pick up DescribeSubnetsPages. Even if we do overflow DescribeSubnets with untagged subnets, VPC deletion will fail and we'll get another pass at deleting tagged subnets when we come around to the next deleteEC2VPC attempt.
Updates the v2 SDK to use the latest go-jmespath. Fix aws#247
See #58