Skip to content

Implement paging for listing apps and flights #172

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.

Sign up for GitHub

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

Open
Dave Smits (davesmits) wants to merge 10 commits into
base: main
Choose a base branch
from
+28 −4
Open

Implement paging for listing apps and flights #172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7a59d02
implement paging for listing apps and flights
davesmits Sep 1, 2026
f21f813
addresss feedback
davesmits Sep 3, 2026
df235bd
change top to const
davesmits Sep 3, 2026
9be1ab3
change const back to 100
davesmits Sep 3, 2026
cccd4c1
fix naming
davesmits Sep 6, 2026
5bfd1d4
rename named argument
davesmits Sep 6, 2026
6951553
fix
davesmits Sep 9, 2026
0f5f4bb
back to 100
davesmits Sep 9, 2026
02aec90
improve
davesmits Sep 9, 2026
7da0006
change guard
davesmits Sep 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions MSStore.API/Packaged/Models/PagedResponse.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@
// Licensed under the MIT License.

using System.Collections.Generic;
using System.Text.Json.Serialization;

namespace MSStore.API.Packaged.Models
{
public class PagedResponse<T>
{
[JsonPropertyName("@nextLink")]
public string? NextLink { get; set; }
public List<T>? Value { get; set; }
public int TotalCount { get; set; }
Expand Down
30 changes: 26 additions & 4 deletions MSStore.API/Packaged/StorePackagedAPI.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Linq;
using System.Net.Http;
using System.Runtime.CompilerServices;
using System.Security.Cryptography.X509Certificates;
using System.Text;
using System.Text.Json;
Expand Down Expand Up @@ -219,8 +221,7 @@ public async Task<List<DevCenterApplication>> GetApplicationsAsync(CancellationT
{
try
{
var devCenterApplicationsResponse = await GetDevCenterApplicationsAsync(0, 100, ct); // TODO: pagination
return devCenterApplicationsResponse.Value ?? [];
return await GetAllPagesAsync<DevCenterApplication>(GetDevCenterApplicationsAsync, ct).ToListAsync(ct);
}
catch (Exception error)
{
Expand Down Expand Up @@ -384,8 +385,7 @@ public async Task<List<DevCenterFlight>> GetFlightsAsync(string productId, Cance
{
try
{
var devCenterFlightsResponse = await GetFlightsAsync(productId, 0, 100, ct); // TODO: pagination
return devCenterFlightsResponse.Value ?? [];
return await GetAllPagesAsync<DevCenterFlight>((skip, top, token) => GetFlightsAsync(productId, skip, top, token), ct).ToListAsync(ct);
}
catch (Exception error)
{
Expand Down Expand Up @@ -663,5 +663,27 @@ public async Task<PackageRollout> FinalizePackageRolloutAsync(string productId,
SourceGenerationContext.GetCustom().PackageRollout,
ct);
}

private static async IAsyncEnumerable<T> GetAllPagesAsync<T>(Func<int, int, CancellationToken, Task<PagedResponse<T>>> pageFunc, [EnumeratorCancellation] CancellationToken ct = default)

@azchohfi Alexandre Zollinger Chohfi (azchohfi) Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Both call sites materialize immediately with ToListAsync, and the public signatures still return Task<List<T>> — so the IAsyncEnumerable machinery (iterator state machine, [EnumeratorCancellation], System.Linq.AsyncEnumerable) buys us nothing today.

There's also a subtle hazard in making this lazy: it only stays inside the try/catch because ToListAsync is awaited there. If anyone later returns the enumerable to a caller, or adds a .Where(...) that defers enumeration past the try, exceptions stop getting wrapped in MSStoreException and the error contract silently changes.

A plain private static async Task<List<T>> helper would be simpler and keeps enumeration eagerly bound to the error handling. Happy to defer if you're planning to expose streaming overloads.

@davesmits Dave Smits (davesmits) Sep 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Making it now asyncenumerable gives the consumer the choice what he wants to do, making it more future proof and using logic from the base library (instead of maintaining a own list result); there for went this approach. But yea might be easier to just keep a list. Tell me what you prefer

@azchohfi Alexandre Zollinger Chohfi (azchohfi) Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fair argument, and I don't feel strongly enough to block on it — your call. Since you asked what I'd prefer: I'd keep the IAsyncEnumerable, with one caveat.

What changed my mind is that you've now made the helper genuinely lazy-friendly (skip advances by items actually received, empty-page guard), so it would stream correctly if a caller ever wanted it to. That wasn't true of the earlier version.

The caveat is the one I raised: the only reason exceptions still get wrapped in MSStoreException is that ToListAsync is awaited inside the try. That holds today — I checked, both call sites enumerate eagerly inside the block. But it's a tripwire. The day someone returns the enumerable to a caller, or inserts a .Where(...) that defers enumeration past the try, the wrapping silently stops and the error contract changes with no compiler complaint.

If you keep it, worth a short comment on the helper noting that callers must enumerate inside the try for exception wrapping to hold. That way the constraint is written down rather than implicit.

Not blocking either way — the TotalCount issue on the loop condition is the one I'd actually like fixed before this merges.

@azchohfi Alexandre Zollinger Chohfi (azchohfi) Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Answering your question from the last round on this (Dave Smits (@davesmits) asked whether to keep IAsyncEnumerable or go back to a list) — keep it. I don't feel strongly enough to block, and your reasoning holds up.

What tips it is that the helper is now genuinely lazy-safe in a way the earlier version wasn't: skip advances by items actually received and there's an empty-page guard, so it really would stream correctly if a caller wanted it to. Deferring to the BCL's ToListAsync instead of hand-rolling accumulation is also the right instinct.

One caveat worth writing down. The only reason exceptions still get wrapped in MSStoreException is that ToListAsync is awaited inside the try. That's true at both call sites today — I checked. But it's a tripwire: the day someone returns the enumerable to a caller, or slips in a .Where(...) that defers enumeration past the try, the wrapping silently stops and the error contract changes with nothing failing to compile.

A one-line comment on the helper saying callers must enumerate within the try for exception wrapping to hold would make that constraint explicit rather than accidental.

Also minor, now that this is [EnumeratorCancellation]-annotated and both callers pass ct into ToListAsync: the ct.ThrowIfCancellationRequested() you added inside the foreach is largely redundant — await foreach already observes the token per iteration. Harmless, just noise.

{
int skip = 0;
const int top = 100;
PagedResponse<T>? lastPage;
do
{
ct.ThrowIfCancellationRequested();

lastPage = await pageFunc(skip, top, ct);
skip += lastPage.Value?.Count ?? 0;

foreach (var item in lastPage.Value ?? [])
{
ct.ThrowIfCancellationRequested();

yield return item;
}
}
while (!string.IsNullOrEmpty(lastPage.NextLink) && lastPage.Value?.Count > 0 && (lastPage.TotalCount <= 0 || skip < lastPage.TotalCount));
}
}
}