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

Make API response accessible on returned API structs #1054

Merged
merged 4 commits into from
Apr 3, 2020

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Mar 30, 2020

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
Copy link
Contributor

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 brandur-stripe force-pushed the brandur-last-response branch 2 times, most recently from 8867576 to 6798855 Compare March 31, 2020 17:35
@brandur-stripe
Copy link
Contributor

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.

Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

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

One nit, but looks great otherwise!

stripe.go Outdated Show resolved Hide resolved
@brandur-stripe
Copy link
Contributor

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

Copy link
Contributor

@remi-stripe remi-stripe left a comment

Choose a reason for hiding this comment

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

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?

@@ -197,6 +197,8 @@ func (p *CardListParams) AppendTo(body *form.Values, keyParts []string) {
// Card is the resource representing a Stripe credit/debit card.
// For more details see https://stripe.com/docs/api#cards.
type Card struct {
APIResource

Copy link
Contributor

Choose a reason for hiding this comment

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

why the extra line on that one?

Copy link
Contributor

Choose a reason for hiding this comment

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

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"`
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@@ -111,6 +111,8 @@ type OAuthTokenParams struct {
// OAuthToken is the value of the OAuthToken from OAuth flow.
// https://stripe.com/docs/connect/oauth-reference#post-token
type OAuthToken struct {
APIResource
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Roger. Will leave as APIResource for now then.

params.go Outdated
@@ -70,6 +70,7 @@ func (f Filters) AppendTo(body *form.Values, keyParts []string) {
// of List iterators. The Count property is only populated if the
// total_count include option is passed in (see tests for example).
type ListMeta struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -87,6 +87,7 @@ func SourceParamsFor(obj interface{}) (*SourceParams, error) {
// The Type should indicate which object is fleshed out (eg. BitcoinReceiver or Card)
// For more details see https://stripe.com/docs/api#retrieve_charge
type PaymentSource struct {
APIResource
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@@ -50,6 +50,7 @@ type ReportRunParameters struct {

// ReportRun is the resource representing a report run.
type ReportRun struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

damn it, this should be named ReportingReportRun instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

RequestID string

// Status is a status code and message. e.g. "200 OK"
Status string
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need that one?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Header: res.Header,
IdempotencyKey: res.Header.Get("Idempotency-Key"),
RawJSON: resBody,
RequestID: res.Header.Get("Request-Id"),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

@brandur-stripe
Copy link
Contributor

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
Copy link
Contributor

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

@remi-stripe
Copy link
Contributor

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 and others added 4 commits April 1, 2020 14:37
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:

    ``` go
    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.
Co-Authored-By: Olivier Bellone <ob@stripe.com>
@brandur-stripe brandur-stripe changed the base branch from master to integration-v71 April 1, 2020 21:37
@brandur brandur mentioned this pull request Apr 1, 2020
7 tasks
@brandur-stripe
Copy link
Contributor

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
Copy link
Contributor

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

@brandur-stripe brandur-stripe merged commit 8d70710 into integration-v71 Apr 3, 2020
remi-stripe pushed a commit that referenced this pull request Apr 16, 2020
* Make API response accessible on returned API structs

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:

    ``` go
    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.

* Rename `LastResponseGetter` to `LastResponseSetter` and remove a function

* Update stripe.go

Co-Authored-By: Olivier Bellone <ob@stripe.com>

* Move `APIResource` onto individual list structs instead of having it in `ListMeta`

Co-authored-by: Brandur <brandur@stripe.com>
Co-authored-by: Olivier Bellone <ob@stripe.com>
brandur-stripe added a commit that referenced this pull request Apr 17, 2020
* Make API response accessible on returned API structs (#1054)

* Make API response accessible on returned API structs

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:

    ``` go
    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.

* Rename `LastResponseGetter` to `LastResponseSetter` and remove a function

* Update stripe.go

Co-Authored-By: Olivier Bellone <ob@stripe.com>

* Move `APIResource` onto individual list structs instead of having it in `ListMeta`

Co-authored-by: Brandur <brandur@stripe.com>
Co-authored-by: Olivier Bellone <ob@stripe.com>

* Remove all beta features from Issuing APIs

* Multiple breaking API changes

* `PaymentIntent` is now expandable on `Charge`
* `Percentage` was removed as a filter when listing `TaxRate`
* Removed `RenewalInterval` on `SubscriptionSchedule`
* Removed `Country` and `RoutingNumber` from `ChargePaymentMethodDetailsAcssDebit`

* Start using Go Modules

Similar to the original implementation for Go Modules in #712, here we
add a `go.mod` and `go.sum`, then proceed to use Go Modules style
import paths everywhere that include the current major revision.

Unfortunately, this may still cause trouble for Dep users who are trying
to upgrade stripe-go, but the project's now had two years to help its
users with basic Go Module awareness, and has chosen not to do so. It's
received no commits of any kind since August 2019, and therefore would
be considered unmaintained by most definitions. Elsewhere, Go Modules
now seem to be the only and obvious way forward, so we're likely to see
more and more users on them.

`scripts/check_api_clients/main.go` is also updated to be smarter about
breaking down package paths which may now include the major.

[1] golang/dep#1963

* Change v71 back to v70

Move back down to current major version so that we can test that our
release script will bump it to v71 properly when the time comes.

Co-authored-by: Brandur <brandur@stripe.com>
Co-authored-by: Olivier Bellone <ob@stripe.com>
Co-authored-by: Remi Jannel <remi@stripe.com>
@remi-stripe remi-stripe deleted the brandur-last-response branch April 29, 2020 23:47
nadaismail-stripe pushed a commit that referenced this pull request Oct 18, 2024
* WIP for iframe calc, stuck on AWS keys

* More tweaks

* Revert changes to systemconnectionstep
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants