Skip to content

Commit a13827e

Browse files
os-zhuangclaude
andauthored
fix(data): paging a sorted read is a partition of the result set, not five queries that share a WHERE clause (objectui#3106) (#4367)
`ORDER BY status LIMIT 50 OFFSET 50` names a sort key that does not identify a row, and no backend promises that rows with equal keys keep the same relative arrangement between two queries. MongoDB documents this outright: `sort` + `skip`/`limit` on a non-unique key may return the same document more than once. Page 2 then repeats a row page 1 already showed and skips one nobody ever sees — with every page full, every row real, and the two halves of the symptom several screens apart. SqlDriver and MongoDBDriver append a unique tie-breaker to any non-empty `orderBy`, in the last requested key's direction: determinism holds either way, but a same-direction suffix is the one an index can still walk in a single pass. SqlDriver applies it only to objects it created itself (`initObjects` records those in `managedObjectFields`). A federated table (ADR-0015) may carry no `id` column, and guessing there would be worse than doing nothing — the resulting unknown-column error is answered by #3821's recovery ladder retrying with NO ORDER BY at all, trading a reshuffle among ties for the loss of the caller's whole sort. driver-memory needed no change: `Array#sort` is stable and the backing table's order does not move between reads. It gets a suite anyway, because that guarantee is implicit and is exactly what a refactor that looks like a speed-up (a hand-rolled sort, or sorting the array in place) would silently remove. The obligation is normative on `IDataDriver.find` and the cases are shared (`PAGINATION_CASES` in `@objectstack/spec/data`), so a future driver is held to it by a gate rather than by remembering. A paged read with NO `orderBy` is deliberately out of scope and filed as #4363. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent b5f9397 commit a13827e

10 files changed

Lines changed: 629 additions & 19 deletions

File tree

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
---
2+
"@objectstack/spec": patch
3+
"@objectstack/driver-sql": patch
4+
"@objectstack/driver-mongodb": patch
5+
---
6+
7+
fix(data): paging a sorted read is a partition of the result set, not five queries that share a WHERE clause (objectui#3106)
8+
9+
`ORDER BY status LIMIT 50 OFFSET 50` names a sort key that does not identify a
10+
row, and no backend promises that rows with equal keys keep the same relative
11+
arrangement between two queries. MongoDB documents this outright — `sort` +
12+
`skip`/`limit` on a non-unique key "may return the same document more than
13+
once". So page 2 could repeat a row page 1 already showed and skip one nobody
14+
ever saw:
15+
16+
```
17+
page 1: ORDER BY status LIMIT 5 OFFSET 0 -> [r05 r07 r11 r04 …]
18+
page 2: ORDER BY status LIMIT 5 OFFSET 5 -> [r04 …] r04 again; one row never served
19+
```
20+
21+
Every page is full, every row is real and belongs, and the duplicate sits
22+
several screens from the omission — which is why this is found by a user
23+
counting records, never by reading a response.
24+
25+
`SqlDriver` and `MongoDBDriver` now append a unique tie-breaker to any non-empty
26+
`orderBy`, in the last requested key's direction (determinism holds either way,
27+
but a same-direction suffix is the one an index can still walk in one pass).
28+
`driver-memory` already conformed — `Array#sort` is stable over a table whose
29+
order does not move — and now has a suite saying so, because that property is
30+
implicit and easy to lose in a refactor that looks like a speed-up.
31+
32+
`SqlDriver` adds it only for objects it created itself (`initObjects` records
33+
those). A federated table (ADR-0015) may have no `id` column, and guessing there
34+
would be worse than doing nothing: the unknown-column error is answered by
35+
#3821's ladder retrying with **no ORDER BY at all**, trading a reshuffle among
36+
ties for the loss of the caller's whole sort.
37+
38+
The obligation is now normative on `IDataDriver.find`, with shared cases in
39+
`@objectstack/spec/data` (`PAGINATION_CASES`) that all three drivers run — so a
40+
future driver is held to it by a gate rather than by remembering.
41+
42+
Deliberately not covered: a paged read with **no** `orderBy`. That is
43+
non-deterministic on every backend by definition and imposing an order on
44+
callers who asked for none changes plan selection far more broadly; filed as
45+
#4363.
Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Deterministic paged reads for the in-memory driver (objectui#3106) — the
5+
* contract on `IDataDriver.find`, checked against the shared cases in
6+
* `@objectstack/spec/data`.
7+
*
8+
* This driver needed **no change** to satisfy it, and that is worth a suite
9+
* rather than a shrug. It sorts with `Array#sort`, which ES2019 onward
10+
* guarantees is stable, over a table array whose order does not move between
11+
* two reads — so equal keys keep the same relative arrangement on page 2 that
12+
* they had on page 1, which is precisely what the contract asks for.
13+
*
14+
* The guarantee is therefore load-bearing but implicit: it rests on `sort`
15+
* being stable and on `applySort` copying rather than reordering the table in
16+
* place. Both are easy to lose in a refactor that looks like a speed-up — a
17+
* hand-rolled quicksort, or sorting the backing array directly — and neither
18+
* loss would fail any other test in this package. That is what this file is
19+
* for: it holds the property against the day the implementation changes, which
20+
* is the only day it could break.
21+
*/
22+
23+
import { describe, it, expect, beforeEach } from 'vitest';
24+
import { PAGINATION_ALL_IDS, PAGINATION_CASES, PAGINATION_ROWS } from '@objectstack/spec/data';
25+
import { InMemoryDriver } from './memory-driver.js';
26+
27+
describe('InMemoryDriver — paged reads are a partition of the result set (objectui#3106)', () => {
28+
let driver: InMemoryDriver;
29+
30+
beforeEach(async () => {
31+
driver = new InMemoryDriver({ persistence: false });
32+
await driver.connect();
33+
for (const row of PAGINATION_ROWS) {
34+
await driver.create('ticket', { ...row });
35+
}
36+
});
37+
38+
for (const testCase of PAGINATION_CASES) {
39+
it(`visits every row exactly once — ${testCase.name}`, async () => {
40+
const seen: string[] = [];
41+
for (let offset = 0; offset < PAGINATION_ROWS.length; offset += testCase.pageSize) {
42+
const page = await driver.find('ticket', {
43+
orderBy: [...testCase.orderBy],
44+
limit: testCase.pageSize,
45+
offset,
46+
} as any);
47+
seen.push(...page.map((r: any) => String(r.id)));
48+
}
49+
50+
expect(seen).toHaveLength(PAGINATION_ALL_IDS.length);
51+
expect(new Set(seen).size).toBe(PAGINATION_ALL_IDS.length);
52+
expect([...seen].sort()).toEqual([...PAGINATION_ALL_IDS].sort());
53+
});
54+
55+
it(`page boundaries are invisible — ${testCase.name}`, async () => {
56+
const paged: any[] = [];
57+
for (let offset = 0; offset < PAGINATION_ROWS.length; offset += testCase.pageSize) {
58+
const page = await driver.find('ticket', {
59+
orderBy: [...testCase.orderBy],
60+
limit: testCase.pageSize,
61+
offset,
62+
} as any);
63+
paged.push(...page);
64+
}
65+
66+
const whole = await driver.find('ticket', { orderBy: [...testCase.orderBy] } as any);
67+
expect(paged.map((r) => r.id)).toEqual(whole.map((r: any) => r.id));
68+
});
69+
}
70+
});

packages/plugins/driver-mongodb/src/mongodb-driver.ts

Lines changed: 40 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -238,15 +238,8 @@ export class MongoDBDriver implements IDataDriver {
238238
}
239239

240240
// Sorting
241-
if (query.orderBy && Array.isArray(query.orderBy)) {
242-
const sort: Document = {};
243-
for (const item of query.orderBy) {
244-
if (item.field) {
245-
sort[this.mapFieldName(item.field)] = item.order === 'desc' ? -1 : 1;
246-
}
247-
}
248-
findOptions.sort = sort;
249-
}
241+
const sort = this.buildSortSpec(query.orderBy);
242+
if (sort) findOptions.sort = sort;
250243

251244
// Pagination
252245
if (query.offset !== undefined) findOptions.skip = query.offset;
@@ -284,15 +277,8 @@ export class MongoDBDriver implements IDataDriver {
284277
projection: { _id: 0 },
285278
};
286279

287-
if (query.orderBy && Array.isArray(query.orderBy)) {
288-
const sort: Document = {};
289-
for (const item of query.orderBy) {
290-
if (item.field) {
291-
sort[this.mapFieldName(item.field)] = item.order === 'desc' ? -1 : 1;
292-
}
293-
}
294-
findOptions.sort = sort;
295-
}
280+
const sort = this.buildSortSpec(query.orderBy);
281+
if (sort) findOptions.sort = sort;
296282

297283
if (query.offset !== undefined) findOptions.skip = query.offset;
298284
if (query.limit !== undefined) findOptions.limit = query.limit;
@@ -611,6 +597,42 @@ export class MongoDBDriver implements IDataDriver {
611597
return field;
612598
}
613599

600+
/**
601+
* The `sort` spec for a `find`, with a unique tie-breaker appended so that
602+
* paging is a partition of the result set rather than a series of unrelated
603+
* queries (objectui#3106, contract on `IDataDriver.find`).
604+
*
605+
* MongoDB is explicit that this is not free: `sort` on a non-unique key
606+
* combined with `skip`/`limit` "may return the same document more than once"
607+
* because equal keys have no defined relative order and nothing holds that
608+
* order steady between two executions. Page 2 repeats a row from page 1 and
609+
* silently drops another — with every page full, every row real, and the two
610+
* halves of the symptom too far apart for anyone to notice.
611+
*
612+
* `id` is always present (`create()` fills it when the caller omits one), so
613+
* unlike the SQL driver there is no table this cannot apply to. It is
614+
* appended in the LAST requested key's direction: determinism holds either
615+
* way, but a same-direction suffix is the one a compound index can still walk
616+
* in a single pass.
617+
*
618+
* Returns `undefined` when nothing was requested — an unordered read stays
619+
* unordered (see the contract's explicit carve-out).
620+
*/
621+
private buildSortSpec(orderBy: QueryAST['orderBy']): Document | undefined {
622+
if (!orderBy || !Array.isArray(orderBy)) return undefined;
623+
const sort: Document = {};
624+
let lastDirection: 1 | -1 = 1;
625+
for (const item of orderBy) {
626+
if (item.field) {
627+
lastDirection = item.order === 'desc' ? -1 : 1;
628+
sort[this.mapFieldName(item.field)] = lastDirection;
629+
}
630+
}
631+
if (Object.keys(sort).length === 0) return undefined;
632+
if (sort.id === undefined) sort.id = lastDirection;
633+
return sort;
634+
}
635+
614636
// ── Temporal storage form (#4047) ─────────────────────────────────────────
615637

616638
/**
Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Deterministic paged reads for the MongoDB driver (objectui#3106) — the
5+
* contract on `IDataDriver.find`, checked against the shared cases in
6+
* `@objectstack/spec/data` against a real MongoDB via `mongodb-memory-server`.
7+
*
8+
* This is the backend where the defect is not theoretical. MongoDB documents
9+
* that `sort` on a non-unique key combined with `skip`/`limit` may return the
10+
* same document more than once — equal keys have no defined relative order, and
11+
* nothing holds one execution's arrangement steady for the next. So the driver
12+
* appends `id` to every requested sort, and these cases walk the page
13+
* boundaries that would otherwise be where a row is served twice while another
14+
* is never served at all.
15+
*
16+
* The sort-spec assertions at the end are deliberately about the spec object
17+
* rather than the rows: they are what fails if the tie-breaker is removed on a
18+
* day the fixture happens to come back in a stable order anyway.
19+
*/
20+
21+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
22+
import { MongoMemoryServer } from 'mongodb-memory-server';
23+
import { PAGINATION_ALL_IDS, PAGINATION_CASES, PAGINATION_ROWS } from '@objectstack/spec/data';
24+
import { MongoDBDriver } from './mongodb-driver.js';
25+
26+
let sharedMongod: MongoMemoryServer | undefined;
27+
try {
28+
sharedMongod = await MongoMemoryServer.create({ instance: { launchTimeout: 60_000 } });
29+
} catch (err) {
30+
console.warn(
31+
'[driver-mongodb] Skipping pagination conformance — mongodb-memory-server could not start: ' +
32+
`${(err as Error)?.message ?? String(err)}`,
33+
);
34+
}
35+
36+
describe.skipIf(!sharedMongod)('driver-mongodb — paged reads are a partition of the result set', () => {
37+
const mongod = sharedMongod as MongoMemoryServer;
38+
let driver: MongoDBDriver;
39+
40+
beforeAll(async () => {
41+
driver = new MongoDBDriver({ url: mongod.getUri(), database: 'pagination_conformance' });
42+
await driver.connect();
43+
for (const row of PAGINATION_ROWS) {
44+
await driver.create('ticket', { ...row });
45+
}
46+
}, 90_000);
47+
48+
afterAll(async () => {
49+
if (driver) await driver.disconnect();
50+
if (sharedMongod) await sharedMongod.stop();
51+
});
52+
53+
for (const testCase of PAGINATION_CASES) {
54+
it(`visits every row exactly once — ${testCase.name}`, async () => {
55+
const seen: string[] = [];
56+
for (let offset = 0; offset < PAGINATION_ROWS.length; offset += testCase.pageSize) {
57+
const page = await driver.find('ticket', {
58+
orderBy: [...testCase.orderBy],
59+
limit: testCase.pageSize,
60+
offset,
61+
} as any);
62+
seen.push(...page.map((r) => String(r.id)));
63+
}
64+
65+
expect(seen).toHaveLength(PAGINATION_ALL_IDS.length);
66+
expect(new Set(seen).size).toBe(PAGINATION_ALL_IDS.length);
67+
expect([...seen].sort()).toEqual([...PAGINATION_ALL_IDS].sort());
68+
});
69+
70+
it(`page boundaries are invisible — ${testCase.name}`, async () => {
71+
const paged: any[] = [];
72+
for (let offset = 0; offset < PAGINATION_ROWS.length; offset += testCase.pageSize) {
73+
const page = await driver.find('ticket', {
74+
orderBy: [...testCase.orderBy],
75+
limit: testCase.pageSize,
76+
offset,
77+
} as any);
78+
paged.push(...page);
79+
}
80+
81+
const whole = await driver.find('ticket', { orderBy: [...testCase.orderBy] } as any);
82+
expect(paged.map((r) => r.id)).toEqual((whole as any[]).map((r) => r.id));
83+
});
84+
}
85+
86+
it('leaves an unordered read alone — no sort is imposed on a caller who asked for none', () => {
87+
expect(driver['buildSortSpec'](undefined)).toBeUndefined();
88+
expect(driver['buildSortSpec']([])).toBeUndefined();
89+
});
90+
91+
it('appends `id` in the LAST key\'s direction', () => {
92+
expect(driver['buildSortSpec']([{ field: 'status', order: 'asc' }])).toEqual({
93+
status: 1,
94+
id: 1,
95+
});
96+
expect(
97+
driver['buildSortSpec']([
98+
{ field: 'status', order: 'asc' },
99+
{ field: 'rank', order: 'desc' },
100+
]),
101+
).toEqual({ status: 1, rank: -1, id: -1 });
102+
});
103+
104+
it('does not override `id` when the caller already sorted by it', () => {
105+
expect(driver['buildSortSpec']([{ field: 'id', order: 'desc' }])).toEqual({ id: -1 });
106+
});
107+
});

0 commit comments

Comments
 (0)