feat(rest): total-count pagination via Prefer: count= (Content-Range) - #2147
feat(rest): total-count pagination via Prefer: count= (Content-Range)#2147cb1kenobi wants to merge 10 commits into
Prefer: count= (Content-Range)#2147Conversation
Adds opt-in total-record-count for REST collection queries so a client can paginate
("1–25 of 1,234") without a second round-trip or a custom resource.
- `Prefer: count=exact` — Table.search drains the full matched set once, windowing the
requested page in the same pass (O(matched) filter evals, O(limit) memory), bounded by
MAX_EXACT_COUNT_SCAN so a page fetch can't turn into an unbounded scan.
- `Prefer: count=estimated` — returns just the page plus a cheap planner/table estimate
(estimateCondition / estimatedEntryCount, now exported), no full scan.
- No default: without the header nothing is computed and no header is emitted.
- REST emits `Content-Range: items <start>-<end>/<total>` (200, not 206), `Range-Unit:
items`, and `Preference-Applied: count=exact|estimated|none`, and adds them to
`Access-Control-Expose-Headers` so browser (CORS) clients can read them. HEAD returns
the headers with no body — a cheap "how many match?" pre-flight.
Tests: resources-level unit (exact/estimated/window/filtered/default streaming) and REST
integration (Content-Range/Range-Unit/Preference-Applied/CORS/HEAD/opt-in).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cross-model review (Codex) of the count feature surfaced several correctness, resource, and disclosure issues, all fixed here: - Read-txn leak: the count drain now releases the read transaction in a `finally`, so a throw mid-iteration (record load, rowFilter policy error) can't leak a pinned snapshot. - Guardrail no longer truncates the page: the requested [offset, end) window is always collected in full; the row cap only abandons the running total. Added a wall-clock budget (MAX_EXACT_COUNT_MS) alongside the row cap so an exact count of a large match set can't run unbounded — on exhaustion the total is reported unknown (Content-Range .../*), never a short page. - Estimated totals no longer corrupted by the planner's synthetic `sort` pseudo-condition: hasUserConditions now reads the raw request conditions, and the estimate drops `sort` pseudo-conditions. A clamp keeps a non-empty page's Content-Range valid when an estimate undershoots (exact totals stay authoritative). - Estimated totals return unknown (null -> .../*) when an opaque rowFilter/vectorFilter participates, instead of a misleading estimate that could disclose hidden cardinality. - Spurious headers: the REST gate now requires an array result, so a single-record GET whose record carries a `recordCount` attribute can't be mistaken for a count page. - CORS: Access-Control-Expose-Headers is appended (not overwritten), preserving a resource's own exposed headers. Adds regression tests for the sorted-estimate, filter-aware estimate, and filtered-exact paths. Resources unit 8 passing; REST integration 21 passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds an operator control for the expensive exact-count scan: `rest: { exactCount:
false }` on a REST mount serves a `Prefer: count=exact` request as a cheap estimate
instead (signaled back via `Preference-Applied: count=estimated`), rather than
rejecting it. Default enabled. Read from httpOptions in the same per-mount way as the
existing `includeExpensiveRecordCountEstimates` option.
This is the operator-facing half of the DoS mitigation for exact counts: the in-code
guardrails (row cap + time budget) bound a single request, and this lets a deployment
turn exact counts off entirely on a sensitive/public mount. It is a per-REST-mount
policy — components exporting at the shared root path share one mount's options.
Integration: a dedicated suite (its own instance, since a gated component would
otherwise share the root mount with the main suite) verifies count=exact downgrades to
estimated while count=estimated is unchanged. 23 REST integration tests passing.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A code-review concern held that the count path releases its read transaction before the page is serialized, so a Bytes/Blob field decoded as a zero-copy view of the read buffer could be corrupted by later reads/writes. Verified it does NOT occur: the count drain reads every record eagerly while the txn is open and returns owned copies (Bytes come back as standalone Buffers, byteOffset 0), so releasing before serialize is safe — unlike the streaming path, which reads lazily during serialization and must hold the txn. This test churns writes/reads after an exact count and asserts the returned Bytes are unchanged, on both storage engines. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…actCount gate Addresses two review findings: - #2: Preference-Applied now echoes the count mode the server applied (exact|estimated, after any per-mount downgrade) instead of `count=none` when the total is unavailable. A `Content-Range: items x-y/*` now reads as "that mode was applied but the total is unavailable" (guardrail hit, or an estimate suppressed by an opaque filter / Infinity estimate) rather than "no count was requested". Added an integration case: a `ne` condition (Infinity estimate) yields items 0-.../* with count=estimated. - #4: the exactCount disable check also accepts the string "false", since not every config source coerces to a boolean. 24 REST integration tests passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for REST pagination total-count using the Prefer: count=exact|estimated header. It updates Table.search to return materialized pages with exact or estimated record counts, incorporating guardrails to prevent unbounded scans. The REST layer is updated to parse the preference, handle configuration-based downgrades, and emit RFC 7233-style headers (Content-Range, Range-Unit, and Preference-Applied). Additionally, new unit and integration tests are added to verify the functionality and ensure read-buffer safety. There are no review comments, and I have no feedback to provide.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reviewed; no blockers found. This push (5ec0d38) addresses all 5 outstanding review threads: bounded count-page validation (finite/non-negative/<=10k limit), exact counting now opt-in per mount (default off), |
Review (claude[bot] on #2147) found the exact-count guardrail (row cap + time budget) and the estimated early-exit only applied when the request included a limit(): both live inside `if (end !== undefined ...)`. A count=exact/estimated request with no limit() therefore drained AND materialized the entire matched set with no cap — the exact unbounded-scan/-memory DoS the guardrail was built to prevent, on the most likely-hit path (a bare collection GET), and it bypassed the exactCount gate too. Counting is a pagination feature, so it now requires a limit(): a count request without one falls through to the normal streaming path (no count emitted), which keeps the guardrail always applied to a bounded page. Updated the unit test that documented the no-limit drain as intentional, and added a test asserting a no-limit count streams (does not materialize). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
This is a really cool idea, I like the interface.
However, I agree with the comments; I think we should probably have exactCount disabled by default, it could pretty expensive. I also think this really increases the need for HarperFast/rocksdb-js#311 or some better solution for estimating range counts (maybe should be a prerequisite for this?)
And do we have an issue for this? I'd like to get priorities associated with these (especially if this starts entraining dependency PRs).
🤖 Reviewed with Codex
| // likely-hit path (a bare collection GET). A count request without a limit therefore falls | ||
| // through to the normal streaming path (no count emitted), so the guardrail below always | ||
| // applies to a bounded page rather than being skipped when `end` is undefined. | ||
| if (target.count && target.limit !== undefined) { |
There was a problem hiding this comment.
The new check closes the undefined-limit case, but it does not establish that the page is actually bounded. REST parsing assigns +args[0] at resources/search.ts:1370-1377, so limit(Infinity) makes pageEnd infinite and scanned >= pageEnd never becomes true; a huge finite limit similarly materializes the whole match set before any guard applies. limit(foo) produces NaN, causing both page and guard comparisons to remain false while exact counting drains the full set. Please validate a finite, non-negative limit and enforce a maximum count-page size before entering this materializing branch, with tests for non-finite and oversized values.
— KrAIs (GPT-5)
| // A mount can disable the expensive exact scan with `rest: { exactCount: false }` (default | ||
| // enabled); a count=exact request is then served as a cheap estimate. Accept a string | ||
| // `"false"` too, since not every config source coerces to a boolean. | ||
| const exactDisabled = (httpOptions as any).exactCount === false || (httpOptions as any).exactCount === 'false'; |
There was a problem hiding this comment.
Exact counting is effectively enabled by default here. A request such as GET ...?limit(1) with Prefer: count=exact returns one row but then drains up to one million matches or one second in resources/Table.ts:3528-3535. Those limits are per request, so concurrent requests can each consume the full budget and hold read snapshots, making a public exported table an inexpensive worker-saturation path. Please make exact counting opt-in (exactCount: true) or enforce a shared concurrency/cancellation budget; estimated counting can remain the safe default.
— KrAIs (GPT-5)
| const wantExact = target.count === 'exact'; | ||
| const pageEnd = offset + (target.limit as number); | ||
| const countStart = performance.now(); | ||
| return (async () => { |
There was a problem hiding this comment.
This conditionally changes the declared search() contract from AsyncIterable (resources/Table.ts:3098) to Promise<Array>. That still breaks an internal path: REST attaches target.count for every HTTP method, and collection deletion immediately does for await (const entry of this.search(scanTarget)) at resources/Table.ts:2997-3003; a DELETE carrying both limit(...) and Prefer: count=exact therefore receives a Promise and throws instead of deleting. Please restrict the REST preference to GET/HEAD and either preserve the iterable contract or expose counting through an explicit, accurately typed API/overload with all callers updated.
— KrAIs (GPT-5)
| const range = len > 0 ? `${offset}-${offset + len - 1}` : '*'; | ||
| const totalStr = typeof total === 'number' ? String(total) : '*'; | ||
| headers.set('Range-Unit', 'items'); | ||
| headers.set('Content-Range', `items ${range}/${totalStr}`); |
There was a problem hiding this comment.
These response headers depend on the request's Prefer value, but the URL and body do not, and no final Vary: Prefer is emitted. A shared cache can therefore serve count headers to a request without the preference, or serve a cached non-count response to a client that requested totals. Please add Prefer to the final Vary value after serialization—serialize() later replaces Vary at server/REST.ts:406, so adding it only inside this helper would still be lost.
— KrAIs (GPT-5)
| // Append (don't overwrite) so a resource that already exposed its own headers keeps them. | ||
| const exposed = headers.get('Access-Control-Expose-Headers'); | ||
| for (const name of ['Content-Range', 'Range-Unit', 'Preference-Applied']) { | ||
| if (!exposed || !String(exposed).toLowerCase().includes(name.toLowerCase())) { |
There was a problem hiding this comment.
Please compare comma-delimited exposure names as case-insensitive tokens rather than substrings. For example, an existing Access-Control-Expose-Headers: X-Content-Range-Metadata currently suppresses the actual Content-Range token, leaving it unreadable to browser clients. Splitting and trimming the existing value, as the Vary helper does, avoids that collision.
— KrAIs (GPT-5)
… only, Vary/CORS Addresses kriszyp's review on #2147: - Bound the count page: the count path now requires a finite, non-negative integer limit no larger than MAX_COUNT_PAGE (10k). limit(Infinity), limit(foo)->NaN, a negative, or an oversized limit fall through to streaming with no count, so a count request can't be coerced into materializing an unbounded page. - Exact counting is now opt-in per mount (`rest: { exactCount: true }`, default off); count=exact is otherwise served as an estimate. Estimated stays the safe default, removing the default worker-saturation surface on public tables. - Only honor Prefer: count on GET/HEAD. It was set for every method, so a collection DELETE carrying limit()+Prefer received a materialized array from search() (declared AsyncIterable) and threw instead of deleting. - Emit `Vary: Prefer` on collection reads (after serialize, which resets Vary) so a shared cache can't serve count headers to a request that didn't ask, or a cached non-count response to one that did. - Compare Access-Control-Expose-Headers as case-insensitive comma tokens, not substrings, so an unrelated existing token (e.g. X-Content-Range-Metadata) no longer suppresses the real Content-Range token. Tests: unit 11 passing (added invalid/oversized-limit fall-through); integration 27 passing (oversized-limit fall-through, Vary: Prefer, DELETE-not-misrouted, and the new opt-in default via exactCount: true / default-off suites). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @kriszyp — really helpful review, all five points were spot on. Addressed in 5ec0d38: Design
Fixes
Docs updated in HarperFast/documentation#623. Tests: unit 11, integration 27 green. Ready for another look when you have a moment. — Claude Opus 4.8 |
3 participants
Summary
Tracking issue: #2162
Adds opt-in total-record-count for REST collection queries so a client can paginate ("1-25 of 1,234") without a second round-trip or a custom resource.
A
GET/HEADon a collection with aPrefer: count=header gets the total in RFC 7233-style response headers:count=exact—Table.searchdrains the full matched set once, windowing the requested page in the same pass (O(matched) filter evals, O(limit) memory).count=estimated— returns just the page plus a cheap planner/table estimate (estimateCondition/estimatedEntryCount), no full scan.200(Content-Rangeis informational, not206).HEADreturns the headers with no body — a cheap "how many match?" pre-flight. The three headers are added toAccess-Control-Expose-Headersso browsers can read them cross-origin.Guardrails and operator control
items x-y/*) rather than truncating the page.rest: { exactCount: false }servescount=exactas an estimate instead (default enabled) for sensitive/public mounts.Note on the base branch
Stacked on
fix/sql-engine-top-limit-normalization(#2124) so this PR's diff is pagination-only. I'll retarget it tomainonce #2124 merges.Review findings addressed
Cross-model (Codex) and Harper-domain review surfaced and fixed: read-transaction release on the count path (
finally), guardrail no longer truncating the page (+ time budget), estimate corruption by the planner's syntheticsortcondition, filter-aware estimates (unknown total instead of a misleading / cardinality-disclosing one), a spurious-header guard for single-record responses, valid-range clamping, and CORS append-not-overwrite. A flagged read-buffer-aliasing concern was verified a non-issue (records return owned copies) and is guarded by a regression test.Testing
unitTests/resources/queryCount*.test.js): exact/estimated/window/filtered/default-streaming, plus a Bytes read-buffer-safety guard on both storage engines.integrationTests/apiTests/rest.test.mjs): Content-Range/Range-Unit/Preference-Applied, offset window, filtered, estimated, unavailable-total (/*), opt-in, HEAD, and theexactCountgate (its own instance). 24 REST integration + 11 unit passing;tscclean.Docs
REST reference docs (Pagination and Total Count, the
exactCountoption, thePreferheader): HarperFast/documentation#623.🤖 Generated with Claude Code