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

Feature request: Retrieve multipe objects by IDs #638

Open
adriaandotcom opened this issue Jun 3, 2019 · 10 comments
Open

Feature request: Retrieve multipe objects by IDs #638

adriaandotcom opened this issue Jun 3, 2019 · 10 comments
Labels

Comments

@adriaandotcom
Copy link

adriaandotcom commented Jun 3, 2019

I would be great to do this:

const customers = await stripe.customers.retrieve(['cus_12391239', 'cus_12392239'])

Instead of this:

const customers = await Promise.all([
  stripe.customers.retrieve('cus_12391239'),
  stripe.customers.retrieve('cus_12392239')
])
@ob-stripe
Copy link
Contributor

Hi @adriaanvanrossum, thanks for the suggestion! This is an interesting idea.

@rattrayalex-stripe What do you think? I'd like to keep the 1:1 mapping between existing methods and API requests (i.e. one call to stripe.foos.retrieve('foo_123') == one GET request to /v1/foos/foo_123), but we could have a different helper method for this e.g. retrieveMany. Or maybe overloading retrieve is fine too 🤷‍♂

Also cc @paulasjes-stripe @jlomas-stripe for Node.js opinions.

@rattrayalex-stripe
Copy link
Contributor

@adriaanvanrossum @wooDzu @sholladay can you say more about why you think this would be nice to have / what you would use it for / how often it comes up?

(in the meantime, this might be the helper I would write personally...)

const retrieveMany = (stripeResource, ids, opts = {}) => 
  Promise.all(ids.map(id) => stripeResource.retrieve(id, opts))

const customers = await retrieveMany(stripe.customers, ['cus_123', 'cus_456'])

@paulasjes-stripe
Copy link
Contributor

My main issue here is what would happen if you have a list of 100 customers you want to retrieve, but the request fails when processing the last one?

We might have to implement some sort of queuing system so you don't get rate limited when requesting a bunch of resources async.

I'm not sure if this is something that should be part of stripe-node as there are a few ways to handle the above problems and I don't think we should make assumptions on how people want those implemented.

@rattrayalex-stripe
Copy link
Contributor

Yeah, I think if we were to offer this, we'd want a configurable limit on the number of concurrent requests to be made at once, probably with a default of around 10.

Agreed, also not sure what to do with errors (my personal preference would be to return an error object in place of a successful object, but that isn't community standard and may be surprising).

We could keep things extremely simple (basically just do Promise.all as above) but would probably want a fairly tight limit on the number of id's you can retrieve (10~100) which I assume would largely hamstring the feature. So I'd lean away from that.

@remi-stripe
Copy link
Contributor

Lurk: Many users asking for this really just want us to do one request with all the ids. The point being to minimize the back and forth and error handling and getting a batch of object at once (though it still opens up the issue with errors and such).

@sholladay
Copy link

@sholladay can you say more about why you think this would be nice to have / what you would use it for / how often it comes up?

At Chords, we are building a system where a music fan can support an artist directly and they can also get an ongoing subscription discount by referring their friends to sign up and support that artist. There are some cases where we want to retrieve the customer object for multiple fans based on these referrals, such as the referring fan and the referred fan, in order to update their metadata, etc. It's generally only two customers at a time, but I can imagine our needs around this may grow in the future.

I was thinking of this feature request as just being sugar for Promise.all(), but some interesting points were raised about errors. Technically, any of the SDK's methods can receive a rate limit error from the REST API, albeit this one would certainly be more likely to if it allowed large input arrays.

How is rate limiting handled in the recently added async iterable feature for list resources? It auto-paginates the results and there could potentially be thousands of pages. Presumably many of the same concerns apply there (albeit to a lesser extent). Does the async iterator automatically wait until the rate limit is over and retry the request? That would be pretty cool, and if it does, then this should probably do the same for consistency. Otherwise, throwing an error that contains the rate limit data is a good approach.

Aside from whether rate limit errors are handled automatically, I would argue to keep the semantics of Promise.all(). That is, if one operation fails, the whole thing fails and an exception is thrown for the one that failed. If they all succeed, then all results are returned.

As for queuing the requests internally, there are some good tools out there for handling this. The got request library will automatically retry a request that gets rate limited as long as the REST API returns an appropriate status code (such as 429) and a Retry-After header that indicates when it is acceptable to retry. The SDK could also proactively limit the number of concurrent requests by using p-map or one of its related modules.

@rattrayalex-stripe
Copy link
Contributor

Thanks for sharing context & thoughts, @sholladay! Curious to hear from others in the community what their use-cases might be as well (similar or different).

You asked about the async iterable feature for list resources – it makes the requests serially, one after another, which reduces the risk of causing rate-limit problems. Promise.all, in contrast, would make all requests at once - we'd need to put limits on that, perhaps with an approach like p-map (thanks for sharing).

In any case, I think it's fairly unlikely that we'll build something for this in the short term, so for now I'd recommend a helper like the one I listed above.

I think if we were to do something, it might look more like a batch api (one request to retrieve or even create multiple things) or a generic utility to issue many requests to stripe "at once" (including updates) but I don't think either are likely anytime soon.

@adriaandotcom
Copy link
Author

adriaandotcom commented Jun 11, 2019

@adriaanvanrossum @WooDzu @sholladay can you say more about why you think this would be nice to have / what you would use it for / how often it comes up?

@rattrayalex-stripe We did built an admin for customer support of Simple Analytics. In that admin we search through our database with users and with the returned users we do a request to Stripe to add some Stripe user data to it. We limit the returned amount of users to 10 so employees/hackers can't pull out the full list of users from our database.

The code I use for retrieving Stripe customers
const appendStripeUsers = async (stripe, customers) => {
  const promises = []
  for (const customer of customers) promises.push(stripe.customers.retrieve(customer, { expand: ['data.plan'] }))
  const stripeCustomers = await Promise.all(promises)
  return customers.map(user => {
    const userId = getId(user)
    const stripeCustomer = stripeCustomers.find(({ id }) => id === userId)
    if (stripeCustomer) return { ...user, ...stripeCustomer }
    else return user
  })
}

I admit that the clean code gain is low but it would be great to combine the requests into one, or with pagination like we have with stripe.customers.list():

for await (const customer of stripe.customers.retrieve(['cus_123', 'cus_456'])) {
  // ...
}

For now I'm not running into rate limits and it's not too slow because I only request max 10 users at a time. I'm planning to run cron jobs which will update my users with Stripe data. For that I would maybe run into rate limits with my current Promise.all implementation.

@rattrayalex-stripe
Copy link
Contributor

Awesome, thanks!

I think we'd like to do something here, but I don't anticipate it happening anytime soon. In the meantime, feel free to upvote the PR and use a helper like this!

@LeadDreamer
Copy link

A couple years later, but a suggestion for rate limiting:

I've successfully used "promise-pool" with Firebase Firestore - the equivalent of a rate-limited, concurrency-limited "Promise.all()", which might be useful here.

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

No branches or pull requests

7 participants