From d90d28bf5d2fc9e541b23ea6df03e74b60f0ceaf Mon Sep 17 00:00:00 2001 From: Brandur Date: Tue, 24 Jul 2018 12:43:23 -0700 Subject: [PATCH] Fix encoding of list params for bank accounts and cards Fixes a bug discovered in #632 whereby because we were manually adding a filter like `?object=bank_account` to the end of paths, we'd clash with the `form` package's attempt at an encoding if both came into play. This would have the effect of producing an invalid URL like this: ``` /accounts/acc_xxxxxx/external_accounts?object=bank_account?limit=1&starting_after=ba_xxxxxx ``` (Note the multiple `?`s in the query string.) Here we stop manually adding filters to URL paths and move them into a custom `AppendTo` override for the `form` package. Bank accounts and cards were the only places that we were doing this, so only they were affected. We also add tests to make sure that we're doing the right thing. --- bankaccount.go | 4 ++++ bankaccount/client.go | 8 ++++++-- bankaccount_test.go | 20 ++++++++++++++++++++ card.go | 6 ++++++ card/client.go | 7 +++++-- card_test.go | 31 +++++++++++++++++++++++++++++++ 6 files changed, 72 insertions(+), 4 deletions(-) diff --git a/bankaccount.go b/bankaccount.go index 0f25ef2353..0056d21e05 100644 --- a/bankaccount.go +++ b/bankaccount.go @@ -125,6 +125,10 @@ type BankAccountListParams struct { Customer *string `form:"-"` } +func (p *BankAccountListParams) AppendTo(body *form.Values, keyParts []string) { + body.Add(form.FormatKey(append(keyParts, "object")), "bank_account") +} + // BankAccount represents a Stripe bank account. type BankAccount struct { AccountHolderName string `json:"account_holder_name"` diff --git a/bankaccount/client.go b/bankaccount/client.go index da4b0e6942..66c1057ce1 100644 --- a/bankaccount/client.go +++ b/bankaccount/client.go @@ -135,13 +135,17 @@ func (c Client) List(listParams *stripe.BankAccountListParams) *Iter { var path string var outerErr error + // There's no bank accounts list URL, so we use one sources or external + // accounts. An override on BankAccountListParam's `AppendTo` will add the + // filter `object=bank_account` to make sure that only bank accounts come + // back with the response. if listParams == nil { outerErr = errors.New("params should not be nil") } else if listParams.Customer != nil { - path = stripe.FormatURLPath("/customers/%s/sources?object=bank_account", + path = stripe.FormatURLPath("/customers/%s/sources", stripe.StringValue(listParams.Customer)) } else if listParams.Account != nil { - path = stripe.FormatURLPath("/accounts/%s/external_accounts?object=bank_account", + path = stripe.FormatURLPath("/accounts/%s/external_accounts", stripe.StringValue(listParams.Account)) } else { outerErr = errors.New("Invalid bank account params: either Customer or Account need to be set") diff --git a/bankaccount_test.go b/bankaccount_test.go index e5aee1c385..a4d9e9369c 100644 --- a/bankaccount_test.go +++ b/bankaccount_test.go @@ -29,6 +29,26 @@ func TestBankAccount_UnmarshalJSON(t *testing.T) { } } +func TestBankAccountListParams_AppendTo(t *testing.T) { + // Adds `object` for account (this will hit the customer sources endpoint) + { + params := &BankAccountListParams{Account: String("acct_123")} + body := &form.Values{} + form.AppendTo(body, params) + t.Logf("body = %+v", body) + assert.Equal(t, []string{"bank_account"}, body.Get("object")) + } + + // Adds `object` for customer (this will hit the external accounts endpoint) + { + params := &BankAccountListParams{Customer: String("cus_123")} + body := &form.Values{} + form.AppendTo(body, params) + t.Logf("body = %+v", body) + assert.Equal(t, []string{"bank_account"}, body.Get("object")) + } +} + func TestBankAccountParams_AppendToAsSourceOrExternalAccount(t *testing.T) { // We should add more tests for all the various corner cases here ... diff --git a/card.go b/card.go index 5d85e22234..2ebe29a13a 100644 --- a/card.go +++ b/card.go @@ -168,6 +168,12 @@ type CardListParams struct { Recipient *string `form:"-"` } +func (p *CardListParams) AppendTo(body *form.Values, keyParts []string) { + if p.Account != nil || p.Customer != nil { + body.Add(form.FormatKey(append(keyParts, "object")), "card") + } +} + // Card is the resource representing a Stripe credit/debit card. // For more details see https://stripe.com/docs/api#cards. type Card struct { diff --git a/card/client.go b/card/client.go index 45d04612fe..ff4fca8faa 100644 --- a/card/client.go +++ b/card/client.go @@ -152,13 +152,16 @@ func (c Client) List(listParams *stripe.CardListParams) *Iter { var path string var outerErr error + // Because the external accounts and sources endpoints can return non-card + // objects, CardListParam's `AppendTo` will add the filter `object=card` to + // make sure that only cards come back with the response. if listParams == nil { outerErr = errors.New("params should not be nil") } else if listParams.Account != nil { - path = stripe.FormatURLPath("/accounts/%s/external_accounts?object=card", + path = stripe.FormatURLPath("/accounts/%s/external_accounts", stripe.StringValue(listParams.Account)) } else if listParams.Customer != nil { - path = stripe.FormatURLPath("/customers/%s/sources?object=card", + path = stripe.FormatURLPath("/customers/%s/sources", stripe.StringValue(listParams.Customer)) } else if listParams.Recipient != nil { path = stripe.FormatURLPath("/recipients/%s/cards", stripe.StringValue(listParams.Recipient)) diff --git a/card_test.go b/card_test.go index be9995a601..a2fcc17dd7 100644 --- a/card_test.go +++ b/card_test.go @@ -5,8 +5,39 @@ import ( "testing" assert "github.com/stretchr/testify/require" + "github.com/stripe/stripe-go/form" ) +func TestCardListParams_AppendTo(t *testing.T) { + // Adds `object` for account (this will hit the external accounts endpoint) + { + params := &CardListParams{Account: String("acct_123")} + body := &form.Values{} + form.AppendTo(body, params) + t.Logf("body = %+v", body) + assert.Equal(t, []string{"card"}, body.Get("object")) + } + + // Adds `object` for customer (this will hit the sources endpoint) + { + params := &CardListParams{Customer: String("cus_123")} + body := &form.Values{} + form.AppendTo(body, params) + t.Logf("body = %+v", body) + assert.Equal(t, []string{"card"}, body.Get("object")) + } + + // *Doesn't* add `object` for recipient (this will hit the recipient cards + // endpoint, so all possible resources are cards) + { + params := &CardListParams{Recipient: String("rp_123")} + body := &form.Values{} + form.AppendTo(body, params) + t.Logf("body = %+v", body) + assert.Equal(t, []string(nil), body.Get("object")) + } +} + func TestCard_UnmarshalJSON(t *testing.T) { // Unmarshals from a JSON string {