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 original list object accessible on iterators #1148

Merged
merged 5 commits into from
Aug 10, 2020

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Jul 30, 2020

This patch makes some tweaks to Iter so that it doesn't just retain
list metadata, but a reference to the entirety of the list object.

This is mainly useful so that it'd be possible to access the last API
response, which could be used for things like this:

requestID := it.CustomerList().LastResponse.RequestID

Currently, it's not possible to access a list operation's last response
because we only set LastResponse on the top level API resource (which
is the list in this case), and Iter provides no access to that.

I also introduce a new ListContainer interface that all lists inherit
through their embedded ListMeta just to convey more typing information
so that we don't have to use an untyped interface{}. I think this is
fine though because it's very nicely symmetric with what we already have
for ListParamsContainer which has a nearly identical implementation.

The downside is that this changes the definition of Query:

// Old definition
type Query func(*Params, *form.Values) ([]interface{}, ListMeta, error)

// New definition
type Query func(*Params, *form.Values) ([]interface{}, ListContainer, error)

This is technically a breaking change because Query is exported from
the stripe package, but it's one of those changes that's very unlikely
to affect that vast majority of users because it's not a type that you'd
ever be using most of the time.

We also have to make a slight change to every existing List function
to adopt the new Query signature. I've shown what this would look like
in the changes in customer/client.go. Unfortunately, I can't find many
solutions to getting access to LastResponse that don't involve
changing every List implementation to some degree. Similarly, we also
add a new function to every resource's Iter like CustomerList to get
back a typed version of the list object, similar to how each already has
a function to get a typed version of its API resource like Customer().

Fixes #1141.

r? @remi-stripe Can you take a perfunctory initial review for this one?
Also see my last comment in #1141.

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.

The changes look good, thanks for doing the work. I think I prefer this approach because it feels more future proof as you just track the "last response" for the most recent request instead of at an object level. At least it seems to solve the original issue more specifically than setting it on a per object in the page.

The code is fairly specific though in parts of the library I don't know really well so we should get @ob-stripe and @richardm-stripe to look too.

I flagged what I think might be a breaking change. Could you also add some tests to cover this behavior?

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

Thanks Remi.

I flagged what I think might be a breaking change.

Yeah, it's actually the change to Query that makes this breaking, but yes, it is technically breaking. It's only "mildly" breaking though in that while Query is exported, it's a type that we'd never expect a user to use. It's only exported so that each of the resource packages (customer/, plan/, etc.) can get at it.

Could you also add some tests to cover this behavior?

I added a basic test around the accessors on Iter. The code that sets the last response on an APIResource is extensively tested elsewhere already though, so I didn't bother testing any of that.

@ob-stripe / @richardm-stripe Definitely feel free to take a look as well. I'll push up the rest of the changes with all the other List functions changed tomorrow or Monday.

@@ -9,23 +9,23 @@ import (
)

func TestIterEmpty(t *testing.T) {
tq := testQuery{{nil, ListMeta{}, nil}}
tq := testQuery{{nil, &ListMeta{}, nil}}
Copy link
Contributor

Choose a reason for hiding this comment

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

These all changes so that I can have testQuery take a ListContainer instead of ListMeta. ListContainer is an interface, so it requires a pointer.

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

brandur-stripe commented Aug 3, 2020

Alright, open for a final round of feedback on this one everyone! Core code hasn't changed, but I've now modified List and Iter in every package (and added as a separate commit for reviewability). Tests are now passing.

@brandur-stripe
Copy link
Contributor

And did one final round of adding tests for all new accessors to get our coverage back up (which also caught a couple errors, phew).

This patch makes some tweaks to `Iter` so that it doesn't just retain
list metadata, but a reference to the entirety of the list object.

This is mainly useful so that it'd be possible to access the last API
response, which could be used for things like this:

``` go
requestID := it.CustomerList().LastResponse.RequestID
```

Currently, it's not possible to access a list operation's last response
because we only set `LastResponse` on the top level API resource (which
is the list in this case), and `Iter` provides no access to that.

I also introduce a new `ListContainer` interface that all lists inherit
through their embedded `ListMeta` just to convey more typing information
so that we don't have to use an untyped `interface{}`. I think this is
fine though because it's very nicely symmetric with what we already have
for `ListParamsContainer` which has a nearly identical implementation.

The downside is that this changes the definition of `Query`:

``` go
// Old definition
type Query func(*Params, *form.Values) ([]interface{}, ListMeta, error)

// New definition
type Query func(*Params, *form.Values) ([]interface{}, ListContainer, error)
```

This is technically a breaking change because `Query` is exported from
the `stripe` package, but it's one of those changes that's very unlikely
to affect that vast majority of users because it's not a type that you'd
ever be using most of the time.

We also have to make a slight change to every existing `List` function
to adopt the new `Query` signature. I've shown what this would look like
in the changes in `customer/client.go`. Unfortunately, I can't find many
solutions to getting access to `LastResponse` that _don't_ involve
changing every `List` implementation to some degree. Similarly, we also
add a new function to every resource's `Iter` like `CustomerList` to get
back a typed version of the list object, similar to how each already has
a function to get a typed version of its API resource like `Customer()`.

Fixes #1141.
@brandur-stripe
Copy link
Contributor

Just rebased and added the updates to promotioncode as well. Going to bring this in.

I talked this over with Remi, and although there is technically a breaking change here to an exported type (Query), because it's only supposed to be used internally for other stripe-go packages and therefore won't be breaking to the vast majority of users, I'm going to release this is a new minor version instead of going for a major. I'll add a more detailed note in the CHANGELOG for how to fix a breakage if you were one of the rare few to run into one.

@brandur-stripe brandur-stripe merged commit 9272de6 into master Aug 10, 2020
@brandur-stripe brandur-stripe deleted the brandur-list-objects branch August 10, 2020 21:03
@brandur-stripe
Copy link
Contributor

Released as 71.44.0.

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

Successfully merging this pull request may close these issues.

List based calls don't set RequestID
5 participants