Repository navigation
Add StreamPage DataLoader extensions and hash IncludeItems in branch keys - #5
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
One or more issues must be addressed before approval.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Adds paging, selection, filtering, and sorting extensions for StreamPage<T> DataLoaders, while centralizing paging branch-key hashing.
Changes:
- Added StreamPage DataLoader extensions and tests.
- Shared Page/StreamPage branch creation and hashing logic.
- Added
IncludeItemsto branch-key hashing.
| File | Description |
|---|---|
| src/GreenDonut/test/GreenDonut.Data.Tests/Internal/ExpressionHasherTests.cs | Updated as part of this pull request. |
| src/GreenDonut/test/GreenDonut.Data.Tests/Extensions/StreamPagingDataLoaderExtensionsTests.cs | Updated as part of this pull request. |
| src/GreenDonut/src/GreenDonut.Data/Internal/DataLoaderStateHelper.cs | Updated as part of this pull request. |
| src/GreenDonut/src/GreenDonut.Data/Extensions/GreenDonutStreamPaginationBatchingDataLoaderExtensions.cs | Updated as part of this pull request. |
| src/GreenDonut/src/GreenDonut.Data/Extensions/GreenDonutPaginationBatchingDataLoaderExtensions.cs | Updated as part of this pull request. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| requiredBufferSize += EstimateIntLength(pagingArguments.Last); | ||
|
|
||
| if (pagingArguments.Before is not null) | ||
| { | ||
| requiredBufferSize += pagingArguments.Before?.Length ?? 0; | ||
| requiredBufferSize += 2; | ||
| } | ||
|
|
||
| // Only reserve space for the marker when items are excluded, so the hash of every | ||
| // existing argument combination (IncludeItems true) stays byte-identical. | ||
| if (!pagingArguments.IncludeItems) |
There was a problem hiding this comment.
Agree. IncludeTotalCount, EnableRelativeCursors and NullOrdering will be encoded in the branch key for non-default values, keeping today's default combination byte-identical. Follow-up PR (task hc-fork-1-oxx.2).
| (DataLoaderBase<TKey, StreamPage<TValue>>)dataLoader, | ||
| branchKey); | ||
|
|
||
| ApplyPagingState(branch, state); |
There was a problem hiding this comment.
Agree. A fetch-based test will execute a branched stream loader and assert the captured GetPagingArguments()/GetQueryContext() (selector, predicate, sorting). Follow-up PR (task hc-fork-1-7v0.8).
1bd6943 to
c1e3fd7
Compare
27f3368 to
bc337f2
Compare
bc337f2 to
c716c14
Compare
This gives DataLoaders that return
StreamPage<T>the sameWith,Select,WhereandOrderByextensions thatPage<T>DataLoaders have. Both use the same branch keys and state keys, soGetPagingArgumentsandGetQueryContextwork unchanged inside a stream fetch. The branch-key hashing now lives in one shared helper, and it includesPagingArguments.IncludeItems, so a count-only request no longer shares a branch with a rows request. Then:marker is written only whenIncludeItemsis false, so every existing branch key stays byte-identical; that is worth a second look since cached branches depend on it.Closed tickets: hc-fork-1-oxx.1 (b901f6a, 1bd6943).
Stack: sits on #4 (ToStreamPageAsync and end cursors in EF paging,
mst/ef-paging-streampage).🤖 Generated with Claude Code