Skip to content

Commit c6d1cb4

Browse files
os-zhuangclaude
andauthored
refactor(spec,drivers)!: retire IDataDriver.findStream (#4484) (#4652)
* refactor(spec,drivers)!: retire IDataDriver.findStream — required, uncalled, and inverted in two of three impls (#4484) `findStream` was a REQUIRED method on the driver contract, documented as the read "optimized for large datasets to avoid memory overflow". Three things were true of it at once: - Nothing called it. Repo-wide, outside the declaration and the three driver implementations, every hit was a test double — and ~20 of those satisfied the required method by throwing `not implemented`. No test ever went red. - `SqlDriver` and `InMemoryDriver` awaited `find()` for the ENTIRE result set and then yielded row by row, so the memory peak it promised to avoid was reached before the first yield. SqlDriver carried a `TODO: Use Knex .stream()`. - `MongoDBDriver._findStream` did stream, but was the one read there never routed through `buildFindOptions`, hardcoding `projection: { _id: 0 }` and silently dropping `query.fields` (the divergence #4459 recorded; subsumed, not fixed). Removed from `IDataDriver` and `DriverInterfaceSchema`, all three implementations deleted, and the ~38 stub lines that existed only to satisfy a required method. Registered as the `data-driver-find-stream-retired` semantic entry on the protocol-17 chain step (ADR-0087 D3) — a TS/API surface, never stored metadata, so no source rewrite and, deliberately, no tombstone: nothing ever `.parse()`d a driver object, so tsc is the only channel that can carry the prescription, and it carries it at the call site. `DriverCapabilities.streaming`, the unread flag whose only referent was this method, is left standing and filed as #4634 — removing it breaks every driver's capability literal, third-party included, and that audit should cover all ~30 flags in one pass. Fixes #4484 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012C2cd7tL8QDoZ2QKN3djJ5 * fix(spec,objectql): changeset is a MAJOR bump, and sweep the last findStream stub Two defects found reviewing the retirement against the `spec-property-retirement` checklist: - The changeset declared `minor` for all four packages. Removing a REQUIRED method from a published contract interface is breaking — the skill says `major` for `@objectstack/spec`, and it is the house convention for every other `!` spec change in this major (`session-dual-source-c4`, `notification-dual-source-c3`). The driver packages drop a public method too, so they go major with it. - `protocol-batch-atomic.test.ts` still carried a `findStream() { throw new Error('not implemented'); }` stub. It is typed `any`, so it compiles and is simply dead — but it is exactly the stub this issue exists to sweep, and leaving one behind lets the next reader infer the method still exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012C2cd7tL8QDoZ2QKN3djJ5 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 666f542 commit c6d1cb4

55 files changed

Lines changed: 298 additions & 196 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
---
2+
"@objectstack/spec": major
3+
"@objectstack/driver-sql": major
4+
"@objectstack/driver-memory": major
5+
"@objectstack/driver-mongodb": major
6+
---
7+
8+
refactor(spec,drivers)!: retire `IDataDriver.findStream` — a required method with no caller, whose two main implementations did the opposite of what it promised (#4484, ADR-0049 enforce-or-remove)
9+
10+
`findStream` was a **required** method on the driver contract — every driver and
11+
every test double had to implement it — documented as the read
12+
13+
> Optimized for large datasets to avoid memory overflow.
14+
15+
Three things were true about it at once, and each is worse in the light of the
16+
others.
17+
18+
**Nothing called it.** Not the query engine (there is no `stream` entry on it),
19+
not REST export, not import, not any bulk-read path. Repo-wide, outside the
20+
contract declaration and the three driver implementations, every single hit was
21+
a test double — and roughly twenty of those satisfied the required method like
22+
this:
23+
24+
```ts
25+
findStream() { throw new Error('not implemented'); }
26+
```
27+
28+
Twenty stubs that throw, across four packages, for years, and no test ever went
29+
red. That is not an anecdote about test hygiene; it is the proof of absence. A
30+
method whose every double throws is a method nothing reaches.
31+
32+
**Two of the three implementations inverted its one guarantee.** `SqlDriver` and
33+
`InMemoryDriver` both did this:
34+
35+
```ts
36+
const results = await this.find(object, query, options); // ← the entire result set
37+
for (const row of results) yield row;
38+
```
39+
40+
The whole table is resident in memory before the first `yield`. A caller who
41+
believed the doc comment and reached for `findStream` precisely because a result
42+
set was too large would have hit the overflow it existed to prevent, at exactly
43+
the scale where it mattered. `SqlDriver` carried a `TODO: Use Knex .stream()`
44+
admitting it.
45+
46+
**The one real implementation dropped a parameter.** `MongoDBDriver._findStream`
47+
did walk a cursor — but it was the only read in that driver never routed through
48+
`buildFindOptions`, so it hardcoded `projection: { _id: 0 }` and silently
49+
discarded `query.fields`. (#4459 unified `find`/`findOne` onto `buildFindOptions`
50+
and recorded in its TSDoc that `_findStream` was left out. This removal subsumes
51+
that divergence rather than fixing it — there is nothing left to fix it for.)
52+
53+
Rather than manufacture a caller to justify three implementations, the method is
54+
retired. If a cursor-based read is wanted, it should arrive **with** the caller
55+
that needs it, so the contract can be shaped by a real requirement instead of
56+
being reverse-engineered from a doc comment nobody could test.
57+
58+
**Migration.**
59+
60+
| Wrote | Write instead |
61+
| --- | --- |
62+
| `for await (const row of driver.findStream(obj, q)) { … }` | page `driver.find(obj, { ...q, limit, offset })` in a loop |
63+
| `findStream(…) { … }` on your own driver | delete the method (see below) |
64+
| `findStream() { throw new Error('ni'); }` in a test double | delete the line |
65+
66+
Paging `find()` is not a downgrade from what `findStream` actually did: on SQL
67+
and memory it is strictly better (bounded pages instead of one full
68+
materialisation), and the paged read is the one with an **enforced** guarantee —
69+
`IDataDriver.find` requires a total order across the whole walk, checked by the
70+
shared `PAGINATION_CASES` / `PAGINATION_UNORDERED_CASES` fixtures in
71+
`data/pagination-conformance.ts`. `findStream` never had a conformance case at
72+
all.
73+
74+
**Driver authors: nothing breaks on you.** An implementation left in place still
75+
compiles — an extra method is not an error on a class or a widened object — it is
76+
simply never reached, so deleting it is cleanup you can do whenever. The break is
77+
on the **caller** side: `driver.findStream(...)` no longer type-checks, and there
78+
were no callers.
79+
80+
**No tombstone, deliberately.** The other v17 retirements tombstone their key so
81+
authoring it fails loudly with a prescription. That would be noise here.
82+
`DriverInterfaceSchema` describes a contract that code *implements*; nothing in
83+
either repository ever ran a driver object through `.parse()`, so a
84+
`retiredKey()` there would carry its prescription to no one. The channel that can
85+
carry it is `tsc`, and `tsc` reports it where it is actionable — at a call site.
86+
The key is removed from the schema and from `IDataDriver`, and the retirement is
87+
registered as the `data-driver-find-stream-retired` semantic entry in the
88+
protocol-17 chain step (ADR-0087 D3), so `spec-changes.json`, the generated
89+
upgrade guide and the `spec_changes` MCP tool all carry it. There is no
90+
`os migrate meta` step: a driver is code, never stack metadata, so the chain has
91+
no source to rewrite.
92+
93+
**Left standing on purpose:** `DriverCapabilities.streaming`, the capability flag
94+
whose only referent was this method. It has no readers either (and the values
95+
written into it were already wrong — `SqlDriver` declared `streaming: false`
96+
while implementing `findStream`, `InMemoryDriver` declared `true` for the
97+
copy-everything version), but removing a key from the capabilities literal breaks
98+
every driver that writes it, third-party included, and the same audit should
99+
cover the other ~30 flags in one pass rather than one at a time. Tracked as
100+
#4634.

docs/design/driver-turso.md

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,6 @@ This does NOT require changes to existing client packages — it would be a new,
215215
| `getPoolStats()` | 🟡 | Concurrency tracking (no traditional pool) |
216216
| `execute()` || `client.execute(sql, args)` |
217217
| `find()` || SQL SELECT with QueryAST→SQL compiler |
218-
| `findStream()` | 🟡 | Cursor-based pagination (no native streaming) |
219218
| `findOne()` || `SELECT ... LIMIT 1` |
220219
| `create()` || `INSERT INTO ... RETURNING *` |
221220
| `update()` || `UPDATE ... WHERE id = ? RETURNING *` |

docs/protocol-upgrade-guide.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,8 @@ The datasource close-out also graduates the four legacy `datasource.config` spel
162162

163163
The `script` flow node converges on its one real path (#4343). It had four ways to name what it ran and only one of them ran anything: `config.actionType: 'email' | 'slack'` were logger-backed stubs that wrote a line, reported success and delivered nothing under any configuration — with `config.template` / `.recipients` / `.variables` feeding a message no channel ever sent; inline `config.script` was recognized and never executed (the built-in runtime has no server-side JS sandbox), so the node warned and no-op'd; and every other `actionType` value was shorthand for a registered-function name, a second spelling of `config.function`. All five keys are retired and `function` becomes required, which is also what finally made the contract PARSEABLE: while the legal key set depended on `actionType`, a flat parse would either reject valid shapes or wave everything through, so `script` (with `subflow`) now runs through the same execute-time contract parse #4277 gave the flat builtins. A shorthand `actionType` CONVERTS into `function` — that is what it meant — unless `function` is already set, in which case it was dead metadata the executor never reached. The other four are dropped outright: nothing read them, so there is no value to preserve, and rebuilding the intent is an authoring decision the tombstones prescribe per branch (a `notify` node for mail — it delivers through the messaging service, the in-app inbox by default and real email once `@objectstack/plugin-email` is installed; a `connector_action` with the Slack connector, or an `http` node posting to a webhook, for Slack; a registered function for an inline body). Retired from the load path for the same reason as the rest: absorbing `actionType: 'email'` silently would let an author keep believing the flow sends mail.
164164

165+
The same audit reaches the driver contract itself: `IDataDriver.findStream` is removed (#4484). It was REQUIRED — every driver and every test double had to implement it — and documented as the read "optimized for large datasets to avoid memory overflow", while two of its three implementations awaited `find()` for the whole result set and then yielded it row by row, reaching exactly the peak it promised to avoid; the third streamed for real but was the one read in that driver that skipped `buildFindOptions`, so it dropped `query.fields`. Nothing anywhere called it, which is why a contract method could carry an inverted guarantee for this long and why ~20 test doubles could satisfy it by throwing `not implemented`. Paged `find()` is the read that exists and is enforced (its total-order guarantee is checked by the shared pagination-conformance cases); a cursor-based read is worth building when a caller asks for one, which is the honest order. A TS/API surface, never stored — one semantic TODO for driver authors, no source rewrite, and no tombstone: `DriverInterfaceSchema` describes a contract that code IMPLEMENTS and nothing ever `.parse()`d a driver, so tsc is the only channel that could carry the prescription, and it carries it where it matters — at a call site.
166+
165167
### Mechanical (applied for you)
166168

167169
| Conversion | Surface | Change | Load window |
@@ -230,6 +232,9 @@ The `script` flow node converges on its one real path (#4343). It had four ways
230232
- **`workflow-service-slot-retired`**`CoreServiceName 'workflow' / IWorkflowService / WorkflowProtocol / discovery routes.workflow / RestApiRouteCategory workflow` → the live mechanisms the slot only ever pointed at: `state_machine` validation rules for record state machines, approval flow nodes on the approvals runtime (ADR-0019) for approvals, lifecycle hooks + `record_change` flows (service-automation) for record-triggered automation
231233
- Why not automatic: The workflow slot was declared end to end and implemented nowhere: no code in either repository ever registered or resolved it (ADR-0115 Evidence 5 — the only touches were plugin-dev's retired stub probe and the generic discovery walk), no implementation of any WorkflowProtocol method ever existed, and no host ever mounted `/api/v1/workflow` (the pre-#3586 DEFAULT_DISPATCHER_ROUTES listed it among routes that never existed). Every part of it was ADR-0078's silently-inert declaration: a CoreServiceName nothing filled, a contract nothing implemented, a protocol nothing served, a discovery route field no builder could truthfully populate. These are TS/API surfaces and a discovery RESPONSE field — never stored in stack metadata, so there is no source for the chain to rewrite; consumers of the deleted types move their imports themselves. ADR-0049 / ADR-0078, #4451.
232234
- Done when: No import of IWorkflowService, WorkflowProtocol or the Get/WorkflowState/Config/Transition types resolves; no code calls getService('workflow') or reads discovery `routes.workflow` / `services.workflow`; record state machines, approvals and record-triggered automation go through the replacement mechanisms. Discovery output on a default boot is unchanged (the slot was always reported unavailable; now it is simply absent).
235+
- **`data-driver-find-stream-retired`**`contracts.IDataDriver.findStream / data.DriverInterfaceSchema.findStream` → find() with limit/offset — the paged read whose determinism IS enforced (IDataDriver.find, data/pagination-conformance.ts)
236+
- Why not automatic: `findStream` was a REQUIRED contract method documented as "optimized for large datasets to avoid memory overflow", and in two of its three implementations it delivered the opposite: `SqlDriver` and `InMemoryDriver` both awaited `find()` for the ENTIRE result set and then yielded it row by row, so the peak memory a caller was promised protection from was already reached before the first yield. The third (`MongoDBDriver._findStream`) did walk a cursor, but it was the one read path in that driver never routed through `buildFindOptions`, so it hardcoded `projection: { _id: 0 }` and silently discarded `query.fields`. None of it was ever observed, because the method had NO caller in either repository: the engine exposes no stream entry, and the REST export, import and bulk-read paths all go through `find()`. The ~20 driver test doubles that existed only to satisfy a required method almost all threw `not implemented`, and nothing ever noticed — which is the proof, not the anecdote. Being REQUIRED, it also taxed every new driver and every test double with an implementation of a capability the platform does not have. Rather than build a caller to justify three implementations, the method is retired; a real cursor-based read should return WITH the caller that needs it (ADR-0049 enforce-or-remove). This is a TS/API contract surface — a driver is CODE, never stack metadata — so there is no source for the chain to rewrite, and deliberately no schema tombstone either: nothing ever ran a driver object through `DriverInterfaceSchema.parse()`, so a prescription there would have no one to reach. The enforced channel is tsc, and it points at callers. ADR-0049 / ADR-0078, #4484.
237+
- Done when: No code calls `driver.findStream(...)`; large reads page through `find()` with `limit`/`offset` (which guarantees a total order across the whole walk) or go through the export surface. Drivers and test doubles no longer implement the method — one left behind still compiles and is simply never reached, so removing it is cleanup rather than a break, while a CALLER of it no longer type-checks.
233238

234239
---
235240

packages/metadata/src/loaders/database-loader.test.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,6 @@ function createMockDriver(): IDataDriver {
9797
return Promise.resolve(null);
9898
}),
9999

100-
findStream: vi.fn(),
101100

102101
create: vi.fn().mockImplementation((tableName: string, data: Record<string, unknown>) => {
103102
const table = getTable(tableName);

packages/objectql/src/datasource-mapping.test.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ const createMockDriver = (name: string) => ({
2121
bulkUpdate: async () => [],
2222
bulkDelete: async () => {},
2323
execute: async () => ({}),
24-
findStream: async function* () {},
2524
upsert: async (obj: string, data: any) => ({ id: '1', ...data }),
2625
beginTransaction: async () => ({}),
2726
commit: async () => {},

packages/objectql/src/engine-aggregate-having.test.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ function makeNativeDriver(rows: any[]) {
3030
supports: {},
3131
async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; },
3232
async find() { return rows.slice(); },
33-
findStream() { throw new Error('ni'); },
3433
async findOne() { return rows[0] ?? null; },
3534
async create(_o: string, d: any) { return d; },
3635
async update(_o: string, _id: string, d: any) { return d; },
@@ -66,7 +65,6 @@ function makeRawDriver(rows: any[]) {
6665
supports: {},
6766
async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; },
6867
async find() { return rows.slice(); },
69-
findStream() { throw new Error('ni'); },
7068
async findOne() { return rows[0] ?? null; },
7169
async create(_o: string, d: any) { return d; },
7270
async update(_o: string, _id: string, d: any) { return d; },

packages/objectql/src/engine-aggregate-timezone.test.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ function makeBucketingDriver(rows: any[]) {
2424
supports: { queryDateGranularity: { day: true, week: true, month: true, quarter: true, year: true } },
2525
async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; },
2626
async find() { return rows.slice(); },
27-
findStream() { throw new Error('ni'); },
2827
async findOne() { return rows[0] ?? null; },
2928
async create(_o: string, d: any) { return d; },
3029
async update(_o: string, _id: string, d: any) { return d; },

packages/objectql/src/engine-ambient-transaction.test.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@ function makeRecordingDriver() {
4040
seen.find.push({ object, transaction: options?.transaction });
4141
return Array.from(storeFor(object).values());
4242
},
43-
findStream() { throw new Error('not implemented'); },
4443
async findOne(object: string) {
4544
for (const r of storeFor(object).values()) return r;
4645
return null;

packages/objectql/src/engine-audit-anchor-write.test.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,6 @@ function makeMemoryDriver() {
4747
async find(object: string, ast: any) {
4848
return Array.from(storeFor(object).values()).filter((r) => matchesWhere(r, ast?.where));
4949
},
50-
findStream() { throw new Error('not implemented'); },
5150
async findOne(object: string, ast: any) {
5251
for (const r of storeFor(object).values()) if (matchesWhere(r, ast?.where)) return r;
5352
return null;

packages/objectql/src/engine-autonumber-batch.test.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ function makeDriver() {
2020
name: 'memory', version: '0.0.0', supports: {},
2121
async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; },
2222
async find(object: string) { return Array.from(storeFor(object).values()); },
23-
findStream() { throw new Error('ni'); },
2423
async findOne() { return null; },
2524
async create(object: string, data: Record<string, unknown>) {
2625
n += 1;

0 commit comments

Comments
 (0)