From 228662e2f6e561adc88000a095d5cddc178e2bb5 Mon Sep 17 00:00:00 2001 From: Brandur Date: Fri, 8 Jun 2018 20:55:32 -0700 Subject: [PATCH] Push parameter encoding into `BackendConfiguration.Call` Now that we have a little more consistency throughout the library, here we modify `BackendConfiguration.Call` (temporarily renamed to `Call2` while we refactor) so that it can take parameter structs directly, then encode them. This allows us to remove this common boilerplate from every API call throughout the entire library: ``` go if params != nil { commonParams = ¶ms.Params body = &form.Values{} form.AppendTo(body, params) } ``` Temporarily though, only the `charge/` package has been converted over to show what it looks like before we convert everything. There is a little bit of type trickiness that allows this to happen, and which requires a somewhat advanced understanding of some Go language semantics. We define a `ParamsContainer` interface as this: ``` go type ParamsContainer interface { GetParams() *Params } ``` Then on the `Params` struct itself, we define an implementation: ``` go func (p *Params) GetParams() *Params { return p } ``` In Go, whenever a struct is embedded in another struct, the host inherits the function implementation of the struct which was embedded. Because every parameter struct in the library embeds `Params`, they all get a `GetParams` implementation and thus implement `ParamsContainer` automatically: ``` go type ChargeParams struct { Params `form:"*"` ... } ``` This allows us to take a `ParamsContainer` from `Call`. Most of this applies similarly for `ListParams`, and similarly `iter.go` also gets a `Query2` and `GetIter2` to keep everything compiling while we refactor everything. I did a little more refactoring in `iter.go` as well just because there was quite a few very bad names in there. If this patch lands, I'll probably refactor even more of it in a follow up. --- charge/client.go | 66 +++++++-------------------------------- iter.go | 80 ++++++++++++++++++++++++++++++++++++------------ params.go | 29 ++++++++++++++++++ stripe.go | 35 +++++++++++++++++++++ 4 files changed, 135 insertions(+), 75 deletions(-) diff --git a/charge/client.go b/charge/client.go index 2219e70653..fe301b3c75 100644 --- a/charge/client.go +++ b/charge/client.go @@ -19,12 +19,8 @@ func New(params *stripe.ChargeParams) (*stripe.Charge, error) { } func (c Client) New(params *stripe.ChargeParams) (*stripe.Charge, error) { - body := &form.Values{} - form.AppendTo(body, params) - charge := &stripe.Charge{} - err := c.B.Call("POST", "/charges", c.Key, body, ¶ms.Params, charge) - + err := c.B.Call2("POST", "/charges", c.Key, params, charge) return charge, err } @@ -35,18 +31,9 @@ func Get(id string, params *stripe.ChargeParams) (*stripe.Charge, error) { } func (c Client) Get(id string, params *stripe.ChargeParams) (*stripe.Charge, error) { - var body *form.Values - var commonParams *stripe.Params - - if params != nil { - commonParams = ¶ms.Params - body = &form.Values{} - form.AppendTo(body, params) - } - + path := stripe.FormatURLPath("/charges/%s", id) charge := &stripe.Charge{} - err := c.B.Call("GET", stripe.FormatURLPath("/charges/%s", id), c.Key, body, commonParams, charge) - + err := c.B.Call2("GET", path, c.Key, params, charge) return charge, err } @@ -57,18 +44,9 @@ func Update(id string, params *stripe.ChargeParams) (*stripe.Charge, error) { } func (c Client) Update(id string, params *stripe.ChargeParams) (*stripe.Charge, error) { - var body *form.Values - var commonParams *stripe.Params - - if params != nil { - commonParams = ¶ms.Params - body = &form.Values{} - form.AppendTo(body, params) - } - + path := stripe.FormatURLPath("/charges/%s", id) charge := &stripe.Charge{} - err := c.B.Call("POST", stripe.FormatURLPath("/charges/%s", id), c.Key, body, commonParams, charge) - + err := c.B.Call2("POST", path, c.Key, params, charge) return charge, err } @@ -79,19 +57,9 @@ func Capture(id string, params *stripe.CaptureParams) (*stripe.Charge, error) { } func (c Client) Capture(id string, params *stripe.CaptureParams) (*stripe.Charge, error) { - var body *form.Values - var commonParams *stripe.Params - - if params != nil { - commonParams = ¶ms.Params - body = &form.Values{} - form.AppendTo(body, params) - } - + path := stripe.FormatURLPath("/charges/%s/capture", id) charge := &stripe.Charge{} - - err := c.B.Call("POST", stripe.FormatURLPath("/charges/%s/capture", id), c.Key, body, commonParams, charge) - + err := c.B.Call2("POST", path, c.Key, params, charge) return charge, err } @@ -101,21 +69,10 @@ func List(params *stripe.ChargeListParams) *Iter { return getC().List(params) } -func (c Client) List(params *stripe.ChargeListParams) *Iter { - var body *form.Values - var lp *stripe.ListParams - var p *stripe.Params - - if params != nil { - body = &form.Values{} - form.AppendTo(body, params) - lp = ¶ms.ListParams - p = params.ToParams() - } - - return &Iter{stripe.GetIter(lp, body, func(b *form.Values) ([]interface{}, stripe.ListMeta, error) { +func (c Client) List(listParams *stripe.ChargeListParams) *Iter { + return &Iter{stripe.GetIter2(listParams, func(p *stripe.Params, b *form.Values) ([]interface{}, stripe.ListMeta, error) { list := &stripe.ChargeList{} - err := c.B.Call("GET", "/charges", c.Key, b, p, list) + err := c.B.CallRaw("GET", "/charges", c.Key, b, p, list) ret := make([]interface{}, len(list.Data)) for i, v := range list.Data { @@ -188,8 +145,7 @@ func CloseDispute(id string) (*stripe.Dispute, error) { func (c Client) CloseDispute(id string) (*stripe.Dispute, error) { dispute := &stripe.Dispute{} - err := c.B.Call("POST", stripe.FormatURLPath("/charges/%s/dispute/close", id), c.Key, nil, nil, dispute) - + err := c.B.Call2("POST", stripe.FormatURLPath("/charges/%s/dispute/close", id), c.Key, nil, dispute) return dispute, err } diff --git a/iter.go b/iter.go index def6d45c44..a9e6b8c8ac 100644 --- a/iter.go +++ b/iter.go @@ -9,6 +9,9 @@ import ( // Query is the function used to get a page listing. type Query func(*form.Values) ([]interface{}, ListMeta, error) +// Query2 is the function used to get a page listing. +type Query2 func(*Params, *form.Values) ([]interface{}, ListMeta, error) + // Iter provides a convenient interface // for iterating over the elements // returned from paginated list API calls. @@ -18,39 +21,76 @@ type Query func(*form.Values) ([]interface{}, ListMeta, error) // Iterators are not thread-safe, so they should not be consumed // across multiple goroutines. type Iter struct { - cur interface{} - err error - meta ListMeta - params ListParams - qs *form.Values - query Query - values []interface{} + cur interface{} + err error + formValues *form.Values + listParams ListParams + meta ListMeta + query Query + query2 Query2 + values []interface{} } // GetIter returns a new Iter for a given query and its options. -func GetIter(params *ListParams, qs *form.Values, query Query) *Iter { +func GetIter(listParams *ListParams, formValues *form.Values, query Query) *Iter { iter := &Iter{} iter.query = query - p := params + p := listParams if p == nil { p = &ListParams{} } - iter.params = *p + iter.listParams = *p - q := qs + q := formValues if q == nil { q = &form.Values{} } - iter.qs = q + iter.formValues = q + + iter.getPage() + return iter +} + +// TODO: After every list API call uses GetIter2, remove GetIter, then rename +// all instances of GetIter2 to GetIter. This only exists as a separate method +// to keep the build/tests working while we refactor. +func GetIter2(container ListParamsContainer, query Query2) *Iter { + var listParams *ListParams + formValues := &form.Values{} + + if container != nil { + reflectValue := reflect.ValueOf(container) + + // See the comment on Call in stripe.go. + if reflectValue.Kind() == reflect.Ptr && !reflectValue.IsNil() { + listParams = container.GetListParams() + form.AppendTo(formValues, container) + } + } + + if listParams == nil { + listParams = &ListParams{} + } + iter := &Iter{ + formValues: formValues, + listParams: *listParams, + query2: query, + } iter.getPage() + return iter } func (it *Iter) getPage() { - it.values, it.meta, it.err = it.query(it.qs) - if it.params.EndingBefore != nil { + if it.query != nil { + it.values, it.meta, it.err = it.query(it.formValues) + } else { + it.values, it.meta, it.err = it.query2(it.listParams.GetParams(), it.formValues) + } + + if it.listParams.EndingBefore != nil { // We are moving backward, // but items arrive in forward order. reverse(it.values) @@ -63,14 +103,14 @@ func (it *Iter) getPage() { // It returns false when the iterator stops // at the end of the list. func (it *Iter) Next() bool { - if len(it.values) == 0 && it.meta.HasMore && !it.params.Single { + if len(it.values) == 0 && it.meta.HasMore && !it.listParams.Single { // determine if we're moving forward or backwards in paging - if it.params.EndingBefore != nil { - it.params.EndingBefore = String(listItemID(it.cur)) - it.qs.Set(EndingBefore, *it.params.EndingBefore) + if it.listParams.EndingBefore != nil { + it.listParams.EndingBefore = String(listItemID(it.cur)) + it.formValues.Set(EndingBefore, *it.listParams.EndingBefore) } else { - it.params.StartingAfter = String(listItemID(it.cur)) - it.qs.Set(StartingAfter, *it.params.StartingAfter) + it.listParams.StartingAfter = String(listItemID(it.cur)) + it.formValues.Set(StartingAfter, *it.listParams.StartingAfter) } it.getPage() } diff --git a/params.go b/params.go index e8b94f1f68..9fdcd3c618 100644 --- a/params.go +++ b/params.go @@ -17,6 +17,14 @@ const ( StartingAfter = "starting_after" ) +type ParamsContainer interface { + GetParams() *Params +} + +type ListParamsContainer interface { + GetListParams() *ListParams +} + // Params is the structure that contains the common properties // of any *Params structure. type Params struct { @@ -46,6 +54,13 @@ type Params struct { StripeAccount *string `form:"-"` // Passed as header } +// GetParams returns a Params struct (itself). It exists because any structs +// that embed Params will inherit it, and thus implement the ParamsContainer +// interface. +func (p *Params) GetParams() *Params { + return p +} + // ExtraValues are extra parameters that are attached to an API request. // They're implemented as a custom type so that they can have their own // AppendTo implementation. @@ -200,6 +215,20 @@ func (p *ListParams) AddExpand(f string) { p.Expand = append(p.Expand, &f) } +// GetListParams returns a ListParams struct (itself). It exists because any +// structs that embed ListParams will inherit it, and thus implement the +// ListParamsContainer interface. +func (p *ListParams) GetListParams() *ListParams { + return p +} + +// GetParams returns ListParams as a Params struct. It exists because any +// structs that embed Params will inherit it, and thus implement the +// ParamsContainer interface. +func (p *ListParams) GetParams() *Params { + return p.ToParams() +} + // SetStripeAccount sets a value for the Stripe-Account header. func (p *ListParams) SetStripeAccount(val string) { p.StripeAccount = &val diff --git a/stripe.go b/stripe.go index b81bfd2e62..cfb679f439 100644 --- a/stripe.go +++ b/stripe.go @@ -13,6 +13,7 @@ import ( "net/url" "os" "os/exec" + "reflect" "runtime" "strings" "sync" @@ -72,6 +73,8 @@ func (a *AppInfo) formatUserAgent() string { // This interface exists to enable mocking for during testing if needed. type Backend interface { Call(method, path, key string, body *form.Values, params *Params, v interface{}) error + Call2(method, path, key string, params ParamsContainer, v interface{}) error + CallRaw(method, path, key string, body *form.Values, params *Params, v interface{}) error CallMultipart(method, path, key, boundary string, body io.Reader, params *Params, v interface{}) error } @@ -210,6 +213,38 @@ func SetBackend(backend SupportedBackend, b Backend) { // Call is the Backend.Call implementation for invoking Stripe APIs. func (s *BackendConfiguration) Call(method, path, key string, form *form.Values, params *Params, v interface{}) error { + return s.CallRaw(method, path, key, form, params, v) +} + +// TODO: After every API call uses Call2, remove Call, then rename all +// instances of Call2 to Call. This only exists as a separate method to keep +// the build/tests working while we refactor. +func (s *BackendConfiguration) Call2(method, path, key string, params ParamsContainer, v interface{}) error { + var body *form.Values + var commonParams *Params + + if params != nil { + // This is a little unfortunate, but Go makes it impossible to compare + // an interface value to nil without the use of the reflect package and + // its true disciples insist that this is a feature and not a bug. + // + // Here we do invoke reflect because (1) we have to reflect anyway to + // use encode with the form package, and (2) the corresponding removal + // of boilerplate that this enables makes the small performance penalty + // worth it. + reflectValue := reflect.ValueOf(params) + + if reflectValue.Kind() == reflect.Ptr && !reflectValue.IsNil() { + commonParams = params.GetParams() + body = &form.Values{} + form.AppendTo(body, params) + } + } + + return s.Call(method, path, key, body, commonParams, v) +} + +func (s *BackendConfiguration) CallRaw(method, path, key string, form *form.Values, params *Params, v interface{}) error { var body io.Reader if form != nil && !form.Empty() { data := form.Encode()