Skip to content
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

Support pagination #247

Merged
merged 14 commits into from
May 28, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ generate-test: generate-protocol-test

generate:
go generate ./aws
make service
make services

service:
services:
go generate ./service

integration: deps
Expand Down
106 changes: 106 additions & 0 deletions aws/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (
"reflect"
"strings"
"time"

"github.com/awslabs/aws-sdk-go/aws/awsutil"
)

// A Request is the service request to be made.
Expand Down Expand Up @@ -39,6 +41,14 @@ type Operation struct {
Name string
HTTPMethod string
HTTPPath string
*Paginator
}

type Paginator struct {
InputTokens []string
OutputTokens []string
LimitToken string
TruncationToken string
}

// NewRequest returns a new Request pointer for the service API
Expand Down Expand Up @@ -217,3 +227,99 @@ func (r *Request) Send() error {

return nil
}

func (r *Request) HasNextPage() bool {
return r.nextPageTokens() != nil
}

func (r *Request) nextPageTokens() []interface{} {
if r.Operation.Paginator == nil {
return nil
}

if r.Operation.TruncationToken != "" {
tr := awsutil.ValuesAtAnyPath(r.Data, r.Operation.TruncationToken)
if tr == nil || len(tr) == 0 {
return nil
}
switch v := tr[0].(type) {
case bool:
if v == false {
return nil
}
}
}

found := false
tokens := make([]interface{}, len(r.Operation.OutputTokens))

for i, outtok := range r.Operation.OutputTokens {
v := awsutil.ValuesAtAnyPath(r.Data, outtok)
if v != nil && len(v) > 0 {
found = true
tokens[i] = v[0]
}
}

if found {
return tokens
}
return nil
}

func (r *Request) NextPage() *Request {
tokens := r.nextPageTokens()
if tokens == nil {
return nil
}

data := reflect.New(reflect.TypeOf(r.Data).Elem()).Interface()
nr := NewRequest(r.Service, r.Operation, awsutil.CopyOf(r.Params), data)
for i, intok := range nr.Operation.InputTokens {
awsutil.SetValueAtAnyPath(nr.Params, intok, tokens[i])
}
return nr
}

// This nil value is needed to work around passing nil into reflect.Value#Call()
// See https://groups.google.com/forum/#!topic/golang-nuts/apNcACpl_fI
var nilval interface{}
var rnil = reflect.ValueOf(&nilval).Elem()

func (r *Request) EachPage(fn interface{}) error {
valfn := reflect.ValueOf(fn)
if valfn.Kind() != reflect.Func {
panic("expected function for EachPage()")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't returning error be more appropriate than panicing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
if valfn.Type().NumOut() > 0 && valfn.Type().Out(0).Kind() != reflect.Bool {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would check for "exactly one output parameter" (valfn.Type().NumOut() == 1) to enforce stronger typing.

While think about it, the valid type for this function is actually

func (data SomeType, more bool) bool

Only SomeType cannot be represented properly by Go at the moment.

An alternate design might be to use

func (data interface{}, more bool) bool

That would make this function much safer to use, but maybe a little less convenient.
But it will also get rid of reflection here.

It might be possible to hide this better by providing type asserting helpers for the specific use cases and/or more code generation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoke about this with @jasdel and I agree that we can make this change. I'll take a look at this now.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lsegal I looked at the results and the manual coded parts got easier, the hard parts are luckily generated by the api scripts (which I didn't anticipate). So it looks awesome now 😍

panic("EachPage(fn) function must return bool if it returns a value")
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


for page := r; page != nil; page = page.NextPage() {
page.Send()

shouldContinue := true
args := []reflect.Value{
reflect.ValueOf(page.Data),
reflect.ValueOf(!page.HasNextPage()),
}

// Use rnil (see above) when page.Data is nil to work around
// reflect.Value#Call() on nil values.
if page.Data == nil {
args[0] = rnil
}

out := valfn.Call(args)

if len(out) > 0 {
shouldContinue = out[0].Bool()
}

if page.Error != nil || !shouldContinue {
return page.Error
}
}

return nil
}
Loading