Make API response accessible on returned API structs

[brandur]

Makes an API response struct containing niceties like the raw response
body, status, and request ID accessible via API resource structs
returned from client functions. For example:

customer, err := customer.New(params)
fmt.Printf("request ID = %s\n", customer.LastResponse.RequestID)

This is a feature that already exists in other language API libraries
and which is requested occasionally here, usually for various situations
involving more complex usage or desire for better observability.

Implementation

We introduce a few new types to make this work:

• APIResponse: Represents a response from the Stripe API and includes
  things like request ID, status, and headers. I elected to create my
  own object instead of reusing http.Response because it gives us a
  little more flexibility, and hides many of myriad of fields exposed by
  the http version, which will hopefully give us a little more API
  stability/forward compatibility.

• APIResource: A struct that contains LastResponse and is meant to
  represent any type that can we returned from a Stripe API endpoint. A
  coupon is an APIResource and so is a list object. This struct is
  embedded in response structs where appropriate across the whole API
  surface area (e.g. Coupon, ListMeta, etc.).

• LastResponseGetter: A very basic interface to an object that looks
  like an APIResource. This isn't strictly necessary, but gives us
  slightly more flexibility around the API and makes backward
  compatibility a little bit better for non-standard use cases (see the
  section on that below).

stripe.Do and other backend calls all start taking objects which are
LastResponseGetter instead of interface{}. This provides us with some
type safety around forgetting to include an embedded APIResource on
structs that should have it by making the compiler balk.

As stripe.Do finishes running a request, it generates an APIResponse
object and sets it onto the API resource type it's deserializing and
returning (e.g. a Coupon).

Errors also embed APIResource and similarly get access to the same set
of fields as response resources, although in their case some of the
fields provided in APIResponse are duplicates of what they had
already (see "Caveats" below).

Backwards compatibility

This is a minor breaking change in that backend implementations methods
like Do now take LastResponseGetter instead of interface{}, which
is more strict.

The good news though is that:

• Very few users should be using any of these even if they're
  technically public. The resource-specific clients packages tend to do
  all the work.

• Users who are broken should have a very easy time updating code.
  Mostly this will just involve adding APIResource to structs that were
  being passed in.

Naming

• APIResponse: Went with this instead of StripeResponse as we see in
  some other libraries because the linter will complain that it
  "stutters" when used outside of the package (meaning, uses the same
  word twice in a row), i.e. stripe.StripeResponse. APIResponse
  sorts nicely with APIResource though, so I think it's okay.

• LastResponse: Copied the "last" convention from other API libraries
  like stripe-python.

• LastResponseGetter: Given an "-er" name per Go convention around
  small interfaces that are basically one liners -- e.g. Reader,
  Writer, Formatter, CloseNotifier, etc. I can see the argument that this maybe should just be APIResourceInterface` or something
  like that in case we start adding new things, but I figure at that
  point we can either rename it, or create a parent interface that
  encapsulates it:

  type APIResourceInterface interface {
      LastResponseGetter
  }

Caveats

• We only set the last response for top-level returned objects. For
  example, an InvoiceItem is an API resource, but if it's returned
  under an Invoice, only Invoice has a non-nil LastResponse. The
  same applies for all resources under list objects. I figure that doing
  it this way is more performant and makes a little bit more intuitive
  sense. Users should be able to work around it if they need to.

• There is some duplication between LastResponse and some other fields
  that already existed on stripe.Error because the latter was already
  exposing some of this information, e.g. RequestID. I figure this is
  okay: it's nice that stripe.Error is a LastResponseGetter for
  consistency with other API resources. The duplication is a little
  unfortunate, but not that big of a deal.

r? @ob-stripe @remi-stripe
cc @stripe/api-libraries

---

Note: Targets major version integration branch in #1055.

[brandur-stripe]

Forgot to mention, but although the diff seems like a bit of a monster, it's mostly just adding APIResource to things. If you just look at stripe.go, error.go, and their tests, you'll get 95% of the relevant information.

[brandur-stripe]

Ended up making one more small tweak here: I realized that GetLastResponse on LastResponseGetter was never something that we actually used (I basically added it for symmetry with SetLastResponse). To simplify things, I removed that function and renamed the interface to LastResponseSetter instead. It's not a hugely significant change, but minimizes the interface a bit. Added as a separate commit for review purposes.

[ob-stripe]

One nit, but looks great otherwise!

[brandur-stripe]

Thanks @ob-stripe! Will leave this a bit longer in case Remi has opinions.

[remi-stripe]

Left some comments. Also wanted to flag 2 high level things
1/ Since it's potentially breaking and a major is coming soon we should wait for the major to be extra safe.

2/ I didn't fully grasp how this works for Errors today in the API. Should Error have the last response too?

[remi-stripe]

why the extra line on that one?

[brandur-stripe]

Hah, I was thinking about leaving a comment about this, but didn't think anyone would notice. Should have known better!

It might look a little random whether I double-spaced after or not, but here's the rough heuristic:

1. If some effort has been made to "sectionize" the struct into various categories (i.e., there are other double newlines in it), then I left an extra newline after the APIResource inclusion.
2. If the struct is just a solid block of fields, then there's no newline.

An example of the first is Card:

type Card struct {
	APIResource

	AddressCity            string                      `json:"address_city"`
	AddressCountry         string                      `json:"address_country"`
	AddressLine1           string                      `json:"address_line1"`
	AddressLine1Check      CardVerification            `json:"address_line1_check"`
	AddressLine2           string                      `json:"address_line2"`
	AddressState           string                      `json:"address_state"`
	AddressZip             string                      `json:"address_zip"`
	AddressZipCheck        CardVerification            `json:"address_zip_check"`
	AvailablePayoutMethods []CardAvailablePayoutMethod `json:"available_payout_methods"`
	Brand                  CardBrand                   `json:"brand"`
	CVCCheck               CardVerification            `json:"cvc_check"`
	Country                string                      `json:"country"`
	Currency               Currency                    `json:"currency"`
	Customer               *Customer                   `json:"customer"`
	DefaultForCurrency     bool                        `json:"default_for_currency"`
	Deleted                bool                        `json:"deleted"`

	// Description is a succinct summary of the card's information.
	//
	// Please note that this field is for internal use only and is not returned
	// as part of standard API requests.
	Description string `json:"description"`

	DynamicLast4 string      `json:"dynamic_last4"`
	ExpMonth     uint8       `json:"exp_month"`
	ExpYear      uint16      `json:"exp_year"`
	Fingerprint  string      `json:"fingerprint"`
	Funding      CardFunding `json:"funding"`
	ID           string      `json:"id"`

...

An example of the second is Coupon:

type Coupon struct {
	APIResource
	AmountOff        int64             `json:"amount_off"`
	Created          int64             `json:"created"`
	Currency         Currency          `json:"currency"`
	Deleted          bool              `json:"deleted"`
	Duration         CouponDuration    `json:"duration"`
	DurationInMonths int64             `json:"duration_in_months"`
	ID               string            `json:"id"`
	Livemode         bool              `json:"livemode"`
	MaxRedemptions   int64             `json:"max_redemptions"`
	Metadata         map[string]string `json:"metadata"`
	Name             string            `json:"name"`
	PercentOff       float64           `json:"percent_off"`
	RedeemBy         int64             `json:"redeem_by"`
	TimesRedeemed    int64             `json:"times_redeemed"`
	Valid            bool              `json:"valid"`
}

[remi-stripe]

Mostly out of curiosity: what is the reasoning doing that? Since it seems to indent exactly the same way I would have expected Go to have an opinion on the only right way. Mostly asking because I'll need to remember when to add/remove lines depending on whether there are specific sections that get added/removed

[brandur-stripe]

Just a purely cosmetic thing for me :) If some attempt at spacing has already been made within a struct, I'll keep that going to give it some more breathing room. Gofmt doesn't have an opinion on it either way.

TBH though, it really doesn't matter that much so if we regress a little bit on this minor style I don't think it'll matter at all.

[remi-stripe]

Is this an API resource? OAuth is a bit different and I'm worried the shape might cause problems (not sure if it's plausible)

[brandur-stripe]

I think I see your meaning in that technically it's not an API resource, but it uses identical infrastructure to all the things that are for making the request, decoding, error handling, etc.:

func (c Client) New(params *stripe.OAuthTokenParams) (*stripe.OAuthToken, error) {
	// client_secret is sent in the post body for this endpoint.
	if stripe.StringValue(params.ClientSecret) == "" {
		params.ClientSecret = stripe.String(stripe.Key)
	}

	oauthToken := &stripe.OAuthToken{}
	err := c.B.Call(http.MethodPost, "/oauth/token", c.Key, params, oauthToken)

	return oauthToken, err
}

So the two alternatives would be:

1. Rename APIResource to something more generic so that it fits better with OAuthToken.
2. Have OAuth-related client calls use different request infrastructure.

I'm not too tied to APIResource — maybe a better name for now would be ResourceWithLastRequest or something, but couldn't think of a name that I liked any better. I also don't think it's too much big of a deal — if we add more API-specific stuff to APIResource in the future and it no longer fits with OAuth, we can always refactor OAuth at that time.

[remi-stripe]

I like APIResource personally because it represents a real API resource and it's a good way to find all "top-level classes" (without starting on a tangent I wish we had that at the API layer too in openapi)

I don't think we should change this I was flagging. cc @ob-stripe as we debated the same thing about OAuth in PHP last night

[brandur-stripe]

Roger. Will leave as APIResource for now then.

[remi-stripe]

Damn I kinda wish we renamed that one to StripeList instead.

[brandur-stripe]

I think ListMeta kind of makes sense because the Data parameter of a list is actually on the leaf struct, which makes ListMeta not a complete list. e.g.

type CouponList struct {
	ListMeta
	Data []*Coupon `json:"data"`
}

One alternative I thought about here was adding APIResource to each individual list instead, which in some ways might be a bit more logical. e.g.

type CouponList struct {
	APIResource
	ListMeta
	Data []*Coupon `json:"data"`
}

Don't think it'd make too much practical difference overall . Thoughts?

[brandur-stripe]

Actually, you know what, I think I'm going to change this. Feels cleaner/more consistent to me.

[remi-stripe]

In other languages this is not an API resource it's an interface. Curious how that fits with "only the top-level object has this set" and in which case that would be PaymentSource instead of Card for example that has APIResource

[brandur-stripe]

For better or worse, we use PaymentSource exactly like an API resource. e.g.:

func (s Client) New(params *stripe.CustomerSourceParams) (*stripe.PaymentSource, error) {
	if params == nil {
		return nil, errors.New("params should not be nil")
	}

	if params.Customer == nil {
		return nil, errors.New("Invalid source params: customer needs to be set")
	}

	path := stripe.FormatURLPath("/v1/customers/%s/sources", stripe.StringValue(params.Customer))
	source := &stripe.PaymentSource{}
	err := s.B.Call(http.MethodPost, path, s.Key, params, source)
	return source, err
}

[remi-stripe]

Yeah but my question was more about the limitation where only the top-level object has APIResource fields. So for example if you list Invoices, the List has the info but each Invoice (or invoice line items) doesn't.

I think that is fine but in the case of PaymentSource it is a bit stranger because the real API resource you are fetching is the Card, but the part that has APIResource filled would be PaymentSource and never Card right?

[brandur-stripe]

Ah, I see.

I think that is fine but in the case of PaymentSource it is a bit stranger because the real API resource you are fetching is the Card, but the part that has APIResource filled would be PaymentSource and never Card right?

Yeah, maybe. I think though in this case it's still okay because even if it's a little surprising if you know the API's domain very profoundly, it's not if you're just familiar with the stripe-go conventions. Basically:

1. The immediate object returned from a call like coupon.* or paymentsource.* will always have the APIResource fields set.
2. No other objects, including subobjects, will have the APIResource fields set. If you need them, you'll have to get them from the parent.

If you have an idea of an alternative that's more sane, I'm open to it, but personally I think it's okay like this.

And one last thought: if this turns out to be a bigger problem than I expect, we can always address this later by doing a recursive population of APIResource. It's easier to transition from nil to non-nil in those fields with a later change than it would be to go the other direction.

[remi-stripe]

damn it, this should be named ReportingReportRun instead.

[brandur-stripe]

Doh, well that major version change is coming anyway. Should we put it on the list?

[remi-stripe]

Yep we have 2 other PRs pending for the next major so it'll be easy to release!

[remi-stripe]

Do we need that one?

[brandur-stripe]

Not strictly, but included it just for consistency with http.Response. It's a little convenient because it makes for a more informational field to log. I don't care that much either way though.

[remi-stripe]

Minor but are those headers access safe? I'm asking because we had some weird things with HTTP2 last year and the case changing

[brandur-stripe]

Yep, Header.Get provides totally case-insensitive access. See docs here:

https://godoc.org/net/http#Header.Get

[brandur-stripe]

Thanks Remi! Going to take your feedback as non-blocking here (minus a small correction I'm making), but feel free to follow up on any particular thread that you feel extra strongly about.

1/ Since it's potentially breaking and a major is coming soon we should wait for the major to be extra safe.

Works for me. How do you want to handle this? Do you want to try and remember to merge or should we get an integration branch going?

2/ I didn't fully grasp how this works for Errors today in the API. Should Error have the last response too?

Yep! Errors will get a LastResponse exactly like any other API resource. See the error test in stripe_test.go:

stripe-go/stripe_test.go

Lines 907 to 910 in 6798855

	// Not exhaustive, but verify LastResponse is basically working as
	// expected.
	assert.Equal(t, res.Header.Get("Request-Id"), stripeErr.LastResponse.RequestID)
	assert.Equal(t, res.StatusCode, stripeErr.LastResponse.StatusCode)

[brandur-stripe]

Moved APIResource onto individual list structs in the latest commit 2bb8d8e.

[remi-stripe]

Works for me. How do you want to handle this? Do you want to try and remember to merge or should we get an integration branch going?

If you could do that that would be great and then I can go and start more changes for the next major!

[brandur-stripe]

If you could do that that would be great and then I can go and start more changes for the next major!

Cool! Started over at #1055.

[brandur-stripe]

Merging this into the v71 branch! Happy to make follow up changes too if anymore come up.