-
Notifications
You must be signed in to change notification settings - Fork 11
feat(rest): total-count pagination via Prefer: count= (Content-Range)
#2147
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
base: main
Are you sure you want to change the base?
Changes from all commits
eaa6cf3
1520757
8d21379
8e502cf
b3d2e58
0a12465
cac3ae2
289bfdf
128fcb2
5ec0d38
57caa5e
beb062d
0a2b9ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,6 +55,7 @@ import { | |
| searchByIndex, | ||
| findAttribute, | ||
| estimateCondition, | ||
| estimatedEntryCount, | ||
| flattenKey, | ||
| COERCIBLE_OPERATORS, | ||
| executeConditions, | ||
|
|
@@ -128,6 +129,17 @@ const EVICTION_BATCH_SIZE = 100; | |
| // letting an unbounded number of open transactions (and their snapshots) accumulate. | ||
| const MAX_INFLIGHT_EVICTION_BATCHES = 4; | ||
| const CACHEABLE_STATUS_CODES = new Set([200, 203, 204, 206, 300, 301, 308, 404, 405, 410, 414, 501]); | ||
| // Guardrails for `Prefer: count=exact`: once the requested page has been collected, counting the rest | ||
| // of the match set is bounded by BOTH a row cap and a wall-clock budget, so a paginated read can't turn | ||
| // into an unbounded scan. Exceeding either reports an unknown total (Content-Range `.../*`) rather than | ||
| // truncating the page. These bound the count tail, not the page itself; a genuinely expensive query | ||
| // (large filtered full-scan, in-memory sort) should still be gated by config before broad exposure. | ||
| const MAX_EXACT_COUNT_SCAN = 1_000_000; | ||
| const MAX_EXACT_COUNT_MS = 1_000; | ||
| // Largest page a `Prefer: count=` request will materialize. A request whose limit exceeds this (or is | ||
| // not a finite, non-negative integer, e.g. `limit(Infinity)`/`limit(foo)`) falls through to the normal | ||
| // streaming path with no count, so a count request can't be coerced into buffering an unbounded page. | ||
| const MAX_COUNT_PAGE = 10_000; | ||
| envMngr.initSync(); | ||
| const LMDB_PREFETCH_WRITES = envMngr.get(CONFIG_PARAMS.STORAGE_PREFETCHWRITES); | ||
| const LOCK_TIMEOUT = 10000; | ||
|
|
@@ -3430,6 +3442,10 @@ export function makeTable(options) { | |
| } | ||
| } | ||
| const select = target.select; | ||
| // Whether the caller supplied real filter conditions — read from the raw request, NOT the | ||
| // planner-augmented `conditions` (which by now may carry a synthetic `sort` pseudo-condition and | ||
| // injected full-scan condition). Used to pick the count-estimate source below. | ||
| const hasUserConditions = Array.isArray(target.conditions) && target.conditions.length > 0; | ||
| if (conditions.length === 0) { | ||
| conditions = [{ attribute: primaryKey, comparator: 'greater_than', value: true }]; | ||
| } | ||
|
|
@@ -3513,12 +3529,91 @@ export function makeTable(options) { | |
| readTxn, | ||
| transformToRecord | ||
| ); | ||
| const offset = target.offset || 0; | ||
| const end = target.limit !== undefined ? offset + (target.limit as number) : undefined; | ||
| // `Prefer: count=` (REST pagination): materialize the requested page and attach a total record | ||
| // count so the HTTP layer can emit a Content-Range. `exact` drains the full matched set once, | ||
| // windowing the page in the same pass; `estimated` returns just the page plus a cheap planner/ | ||
| // table estimate. Opt-in only — the default streaming path below is untouched. | ||
| // | ||
| // Requires a bounded page AND window. Counting is a pagination feature; both the limit and the | ||
| // offset must be finite, non-negative integers, the limit no larger than MAX_COUNT_PAGE, and the | ||
| // window (offset + limit) no larger than MAX_EXACT_COUNT_SCAN. Anything else — a missing/ | ||
| // oversized/non-finite/negative limit or offset (a bare collection GET, limit(Infinity), | ||
| // limit(foo), limit(-5,10)) or a deep-page window past the scan budget — falls through to the | ||
| // normal streaming path with no count. This bounds the offset too: without it a huge offset would | ||
| // postpone the exact guardrail (which only engages past the page) until that offset was scanned. | ||
| const pageLimit = target.limit as number; | ||
| if ( | ||
| target.count && | ||
| Number.isInteger(pageLimit) && | ||
| pageLimit >= 0 && | ||
| pageLimit <= MAX_COUNT_PAGE && | ||
| Number.isInteger(offset) && | ||
| offset >= 0 && | ||
| offset + pageLimit <= MAX_EXACT_COUNT_SCAN | ||
| ) { | ||
| const wantExact = target.count === 'exact'; | ||
| const pageEnd = offset + pageLimit; | ||
| const countStart = performance.now(); | ||
| return (async () => { | ||
| const page: any = []; | ||
| let scanned = 0; | ||
| let exact = true; | ||
| try { | ||
| for await (const record of results) { | ||
| if (scanned >= offset && scanned < pageEnd) page.push(record); | ||
| scanned++; | ||
| // The page window [offset, pageEnd) is always collected in full first — the guardrail | ||
| // only ever abandons the running TOTAL, never truncates the page body. | ||
| if (scanned >= pageEnd) { | ||
| if (!wantExact) break; // `estimated` needs nothing past the page | ||
| // `exact` keeps counting the tail, bounded by a row cap AND a time budget so a | ||
| // large match set can't turn a bounded page fetch into an unbounded scan. | ||
| if (scanned > MAX_EXACT_COUNT_SCAN || performance.now() - countStart > MAX_EXACT_COUNT_MS) { | ||
| exact = false; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } finally { | ||
| // We own the iteration here (no results.onDone consumer), so release the read | ||
| // transaction unconditionally — including when the drain throws — or the snapshot leaks. | ||
| txn.doneReadTxn(); | ||
| } | ||
| let total: number | null; | ||
| if (wantExact) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Please avoid deriving an exact total from an approximate-index result iterator. Either count through a separate non-candidate-limited path with clearly defined vector/missing-vector semantics, or report the total as unavailable/downgrade the applied mode for approximate searches. A regression test should run the same vector query with two page sizes or offsets and verify that an advertised exact total is authoritative and stable. — KrAIs (GPT-5) |
||
| total = exact ? scanned : null; | ||
| } else if (boundRowFilter || typeof target.vectorFilter === 'function') { | ||
| // An opaque row/vector filter shapes the result but isn't reflected in the index/condition | ||
| // estimate; guessing would both mislead and disclose cardinality the filter hides. | ||
| total = null; | ||
| } else if (!hasUserConditions) { | ||
| total = estimatedEntryCount(primaryStore); | ||
| } else { | ||
| // Estimate from the real conditions only — drop the planner's synthetic `sort` | ||
| // pseudo-condition, which otherwise contributes a bogus (entryCount/2) cardinality. | ||
| const est = estimateCondition(TableResource)({ | ||
| conditions: conditions.filter((c: any) => c.comparator !== 'sort'), | ||
| operator: operator ? String(operator).toLowerCase() : 'and', | ||
| }); | ||
| total = isFinite(est) ? Math.round(est) : null; | ||
| } | ||
| // For an estimate, never report a total below the last row actually returned — keeps the | ||
| // Content-Range valid (start-end/total) when an estimate undershoots a non-empty page. | ||
| // Exact totals are authoritative (and an empty page past the end must not be clamped up). | ||
| if (!wantExact && total != null && page.length > 0 && total < offset + page.length) { | ||
| total = offset + page.length; | ||
| } | ||
| page.recordCount = total; | ||
| page.recordCountExact = wantExact && exact; | ||
| page.selectApplied = true; | ||
| page.getColumns = getColumns; | ||
| return page; | ||
| })() as any; | ||
| } | ||
| // apply any offset/limit after all the sorting and filtering | ||
| if (target.offset || target.limit !== undefined) | ||
| results = results.slice( | ||
| target.offset, | ||
| target.limit !== undefined ? (target.offset || 0) + target.limit : undefined | ||
| ); | ||
| if (target.offset || target.limit !== undefined) results = results.slice(offset, end); | ||
| results.onDone = () => { | ||
| results.onDone = null; // ensure that it isn't called twice | ||
| txn.doneReadTxn(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conditionally changes the declared
search()contract fromAsyncIterable(resources/Table.ts:3098) toPromise<Array>. That still breaks an internal path: REST attachestarget.countfor every HTTP method, and collection deletion immediately doesfor await (const entry of this.search(scanTarget))atresources/Table.ts:2997-3003; a DELETE carrying bothlimit(...)andPrefer: count=exacttherefore 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)