Skip to content

Commit

Permalink
Fix encoding of list params for bank accounts and cards
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
brandur committed Jul 24, 2018
1 parent caa0c20 commit d90d28b
Show file tree
Hide file tree
Showing 6 changed files with 72 additions and 4 deletions.
4 changes: 4 additions & 0 deletions bankaccount.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down
8 changes: 6 additions & 2 deletions bankaccount/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
20 changes: 20 additions & 0 deletions bankaccount_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 ...

Expand Down
6 changes: 6 additions & 0 deletions card.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
7 changes: 5 additions & 2 deletions card/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
31 changes: 31 additions & 0 deletions card_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down

0 comments on commit d90d28b

Please sign in to comment.