Implement paging for listing apps and flights - #172
Implement paging for listing apps and flights#172Dave Smits (davesmits) wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The current diff contains a C# compile-time error (named+positional args) and the paging iterator can stop early or paginate incorrectly because it doesn’t fully follow the server-provided NextLink.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Implements pagination support in the packaged Dev Center API client so CLI scenarios (e.g., listing apps / flights) aren’t capped at the first 100 items, addressing the reported limitation in #170.
Changes:
- Replace single-page application listing with an async pagination helper that aggregates all pages.
- Replace single-page flight listing with the same pagination approach.
- Add JSON mapping for the paging continuation field (
@nextLink) onPagedResponse<T>.
File summaries
| File | Description |
|---|---|
| MSStore.API/Packaged/StorePackagedAPI.cs | Switches app/flight listing to fetch all pages and adds a shared GetAllPagesAsync iterator. |
| MSStore.API/Packaged/Models/PagedResponse.cs | Adds JsonPropertyName("@nextLink") so paging metadata deserializes correctly. |
Review details
Suppressed comments (1)
MSStore.API/Packaged/StorePackagedAPI.cs:684
- Pagination currently ignores the server-provided
NextLinkwhen computing the next page offset and also gates continuation onValue.Count == top. If the service returns a validNextLinkwith a page size different fromtop(or fewer thantopitems before the final page), this can skip remaining pages or stop early. Consider drivingskip/topfromNextLinkwhen present and looping solely based onNextLinkbeing non-empty.
while (lastPage is { NextLink.Length: > 0, Value.Count: top });
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
The pagination loop can terminate early because it requires both @nextLink and Value.Count == top, which may miss remaining pages when the service provides @nextLink but returns fewer than top items.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
MSStore.API/Packaged/StorePackagedAPI.cs:685
- The pagination loop stops only when both
@nextLinkis present andValue.Count == top. If the service returns fewer thantopitems while still providing@nextLink(which the PR description implies is the authoritative “has more pages” signal), this will terminate early and miss remaining items. Consider looping based solely on@nextLink(optionally with an empty-page guard) so pagination continues whenever the server indicates more pages.
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
| ct); | ||
| } | ||
|
|
||
| private static async IAsyncEnumerable<T> GetAllPagesAsync<T>(Func<int, int, CancellationToken, Task<PagedResponse<T>>> pageFunc, [EnumeratorCancellation] CancellationToken ct = default) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
🟡 Changes recommended
The new paging loop can prematurely stop (or potentially spin) due to relying on TotalCount and hard-coding a very small page size, which impacts correctness and performance.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
MSStore.API/Packaged/StorePackagedAPI.cs:686
- The loop termination condition depends on TotalCount. If the service doesn’t populate TotalCount (it will remain 0), paging will stop after the first page even when
@nextLinkindicates more results. Also, if a page ever returns 0 items while still providing nextlink, skip won’t advance and the loop can spin forever. Rely on@nextLinkfor continuation and guard against empty pages.
while (!string.IsNullOrEmpty(lastPage.NextLink) && skip < lastPage.TotalCount);
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new paging loop can terminate early or spin indefinitely in certain response shapes and should be hardened before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new paging loop can prematurely stop when TotalCount is 0/unset even if @nextLink indicates more pages.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| yield return item; | ||
| } | ||
| } | ||
| while (!string.IsNullOrEmpty(lastPage.NextLink) && lastPage.Value?.Count > 0 && skip < lastPage.TotalCount); |
There was a problem hiding this comment.
Copilot is right about this one, and it's a real truncation bug — I verified it rather than taking either of our words for it.
TotalCount is a non-nullable int, so an absent totalCount in the payload deserializes to 0, indistinguishable from a genuine zero. I ran that exact payload through the real SourceGenerationContext:
{ "@nextLink": "applications/9NBLGGH4R315/listflights/?skip=2&top=2", "value": [ {...}, {...} ] }
-> TotalCount=0, NextLink='applications/...?skip=2&top=2', Value.Count=2
skip < 0 is immediately false, so we stop after page one even though @nextLink says there's more. Simulating this loop against a service that pages correctly but doesn't populate totalCount:
scenario | expected | CURRENT | TotalCount optional
------------------------------------------|----------|---------------|--------------------
honest server, 250 items | 250 | 250 (3 calls) | 250 (3 calls)
honest, page capped at 50, 250 items | 250 | 250 (5 calls) | 250 (5 calls)
service omits totalCount, 250 items | 250 | 100 (1 call) | 250 (3 calls) <-- truncates
service omits totalCount, 37 items | 37 | 37 (1 call) | 37 (1 call)
100 instead of 250 — the same class of silent truncation this PR exists to fix.
Making it an optional guard is a one-clause change:
while (!string.IsNullOrEmpty(lastPage.NextLink)
&& lastPage.Value?.Count > 0
&& (lastPage.TotalCount <= 0 || skip < lastPage.TotalCount));Worth noting this costs nothing on the genuine-zero path: if the account really has 0 apps, Value comes back empty and your Value?.Count > 0 guard already stops the loop. So TotalCount <= 0 can safely mean "unknown, fall back to @nextLink" without weakening termination.
To be clear on where I landed vs. my last comment: TotalCount is still the right bound when present — I'm not walking that back. Both get-all-apps and get-flights-for-an-app document totalCount as always returned, so this is belt-and-braces. But since the field is non-nullable and the failure mode is silent data loss rather than a loud error, I'd rather not have it be load-bearing.
Everything else here is good — skip += received, the empty-page guard, and IsNullOrEmpty all landed correctly, and the capped-page-size case now works (250 across 5 calls where the old Count == top version returned 50).
There was a problem hiding this comment.
Not sure what scenario you are want to cover;
If its the scenario that the API might not return a TotalCount field isn't the fix better to make the property nullable instead?
If it's a wrong 0 value from the API this might better.
(for now changed it)
| ct); | ||
| } | ||
|
|
||
| private static async IAsyncEnumerable<T> GetAllPagesAsync<T>(Func<int, int, CancellationToken, Task<PagedResponse<T>>> pageFunc, [EnumeratorCancellation] CancellationToken ct = default) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
🟢 Approval recommended
The pagination logic is contained, consistent with existing API calling patterns, and resolves the reported 100-item limit without introducing breaking surface-area changes.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
fixes #170 and removes some todo's from code
current problem
paging is not implement and people report hitting the existing default of 100 apps.
Implementation
Using the next link to decide if new page need to be retrieved but not using it as a template as it doesn't fit very well in the existing way of templating urls (nextlink for application is
applications?top=100&skip=100while the existing templates assume application is coming from code;