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

Add support for auto-pagination #1414

Merged
merged 1 commit into from
Jan 4, 2019
Merged

Conversation

ob-stripe
Copy link
Contributor

@ob-stripe ob-stripe commented Dec 7, 2018

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

Adds support for auto-pagination.

Example usages:

var service = new BalanceTransactionService();
var listOptions = new BalanceTransactionListOptions
{
  Created = new DateRangeOptions
  {
    GreatherThanOrEqual = DateTime.Parse("2018-11-01T00:00:00Z"),
    LessThan = DateTime.Parse("2018-12-01T00:00:00Z"),
  },
  Limit = 100,
};

foreach (var balanceTransaction in service.ListAutoPaging(listOptions))
{
  // Do something with balanceTransaction
}
var service = new InvoiceService();
var lineItems = service.ListLineItemsAutoPaging("in_123").ToList();

The second example uses the LINQ method ToList() to directly convert the IEnumerable<LineItem> returned by InvoiceService.ListLineItemsAutoPaging() into a List<LineItem>.

Fixes #1038.

"n/a",
RequestDate = response.Headers.Contains("Date") ?
Convert.ToDateTime(response.Headers.GetValues("Date").First(), CultureInfo.InvariantCulture) :
default(DateTime),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find a way to mock response headers, so I made the Date header optional. I think that's better anyway.

@@ -4,7 +4,7 @@ namespace Stripe
using System.Threading.Tasks;

public interface IListable<T, O>
where T : IStripeEntity
where T : IStripeEntity, IHasId
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the resource to have an ID in order to be able to paginate. I also added IHasId as a constraint on other interfaces where it makes sense to do so (i.e. everything except create).

src/StripeTests/BaseStripeTest.cs Outdated Show resolved Hide resolved
using Stripe;
using Xunit;

public class AutoPagingTest : BaseStripeTest
Copy link
Contributor Author

@ob-stripe ob-stripe Dec 7, 2018

Choose a reason for hiding this comment

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

This test seems pretty complex, but it's actually pretty straightforward:

  1. set up 3 stubbed requests with file fixtures
  2. call ListAutoPaging
  3. check invocations to make sure the correct parameters were sent

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 high level feedback. This looks mostly okay to me though it's still beyond what I'm comfortable actively reviewing so also cc-in @anelder-stripe

Is there a way we don't have to add new prototypes for this? I assume not but was hoping you could do an autoPaginate = true that defaults to false?

],
"has_more": true,
"object": "list",
"url": "/v1/pageable_models"
Copy link
Contributor

Choose a reason for hiding this comment

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

OOC aren't we supposed to have ?starting_after=pm_124 in this URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the API doesn't include starting_after parameters in url attributes.

Assert.Equal("pm_124", models[1].Id);
Assert.Equal("pm_125", models[2].Id);
Assert.Equal("pm_126", models[3].Id);
Assert.Equal("pm_127", models[4].Id);
Copy link
Contributor

Choose a reason for hiding this comment

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

So auto-paging basically builds the entire list in ram no matter what? You can't auto page while also getting items one by one and not filling your RAM with all your BTs history?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can. In these examples, I used ToList() to directly convert the iterator into a list, but you can also iterate over the objects one by one:

foreach (var balanceTransaction in balanceTransactionService.ListAutoPaging(listOptions))
{
  // Do something with balanceTransaction
}

Copy link
Contributor

Choose a reason for hiding this comment

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

ah neat! Can you put a test for this in our test suite? I know people refer to the test suite a lot to find example. I guess this is where your yield applies. Fun how similar it is in each language you've shown me this approach!

@ob-stripe
Copy link
Contributor Author

Is there a way we don't have to add new prototypes for this? I assume not but was hoping you could do an autoPaginate = true that defaults to false?

No, because the types that are returned are different. When you use the existing List method, you get a StripeList<Resource> object. Auto-pagination lets you iterate directly over the Resource objects, bypassing the StripeList wrapper.

@ob-stripe ob-stripe changed the base branch from master to integration-v23 December 10, 2018 17:19
@ob-stripe ob-stripe mentioned this pull request Dec 10, 2018
29 tasks
@ob-stripe ob-stripe changed the title [wip] Add support for auto-pagination Add support for auto-pagination Dec 10, 2018
@ob-stripe
Copy link
Contributor Author

Okay, I think this is ready to be merged. ptal @remi-stripe @anelder-stripe

@remi-stripe remi-stripe removed their assignment Dec 10, 2018
@ob-stripe ob-stripe force-pushed the integration-v23 branch 2 times, most recently from 65edf0a to 7d6d6cc Compare December 10, 2018 22:58
@ob-stripe ob-stripe force-pushed the ob-auto-paging branch 2 times, most recently from dbe68b3 to db53d82 Compare December 11, 2018 17:34
@ob-stripe ob-stripe force-pushed the ob-auto-paging branch 4 times, most recently from c243a24 to 31bc80e Compare December 13, 2018 18:13
@ob-stripe ob-stripe force-pushed the ob-auto-paging branch 4 times, most recently from 1bd903a to 4603a3c Compare December 17, 2018 09:02
@ob-stripe ob-stripe force-pushed the ob-auto-paging branch 2 times, most recently from 8ede288 to 74d2144 Compare December 21, 2018 15:04
@ob-stripe
Copy link
Contributor Author

Timing out and self-approving this one :)

@ob-stripe ob-stripe merged commit 9ac4ef7 into integration-v23 Jan 4, 2019
@ob-stripe ob-stripe deleted the ob-auto-paging branch January 4, 2019 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants