Skip to content

Commit ca3fd4b

Browse files
os-muskclaudeos-musk
authored
fix(drivers): update() on a missing id answers null on MongoDB and Turso's remote face (#14914)
* fix(drivers): update() on a missing id answers null on MongoDB and Turso remote `IDataDriver.update()` declares `Promise<Record<string, unknown> | null>`, and four of six shipped implementations return `null` for an id that names no row. `MongoDBDriver.update()` and `RemoteTransport.update()` fabricated a record instead — the caller's own payload with the id stapled on (and, on Mongo, the `updated_at` the driver had just stamped). Through the engine's by-id door that surfaced as a 200 with a record that does not exist. Both now return `null`. `TursoDriver.update()`'s remote branch needed no edit: `formatRemoteRow` already guards `row && typeof row === 'object'`, so the two faces of that driver converge. `RemoteTransport.bulkUpdate()`'s `if (updated) results.push(updated)` skip stops being dead code. `upsert()` is untouched on both: an upsert never answers "not found". Regression pins added per driver (net-new — no landed test pinned the fabricating posture), each with a positive control so "return null always" cannot pass, plus a local/remote parity pin on TursoDriver. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68 * fix(driver-mongodb): narrow the three found-arm `update()` reads in mongodb-driver.test.ts The widened `update(): Promise<Record<string, unknown> | null>` declaration made `expect(result.title|.status|.id)` three TS18047 errors. The package tsconfig excludes `**/*.test.ts`, so `pnpm typecheck` could not see them; the type-check debt ratchet re-measures with the tests un-hidden and caught the +3 (10 -> 13). Narrowed at the three sites with the file's own `findOne` idiom -- assert the found arm, then read through it. Re-measured with the ratchet's own project shape: back to exactly 10 (TS1309 x7 + TS2550 x3), the ledger's frozen entry. The ledger is NOT raised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68 * docs(drivers): correct the changeset semver row and the two test headers to what was measured Contract-review round on PR #14914. Changeset: `patch`/`patch` understated a published TYPE-surface narrowing plus a runtime behaviour change on two published drivers. Now `minor`/`minor` with a `**BREAKING**` sentence naming what breaks for TypeScript consumers, and exactly one ADR-0087 disposition -- `not-required (no-migration-prescription)`, the same shape and reasoning as `.changeset/driver-memory-update-upsert-honest-types.md` (PR #14434) one day earlier in this series. `type-surface-only` is not claimable: its predicate 4 (narrowed-from-erased) is false, and runtime behaviour moves too. `mongodb-update-missing-id.test.ts`: the header claimed a type-level pin here would be "never checked by anything". False -- `pnpm check:type-check-debt` re-measures this package with its tests un-hidden, which is exactly how CI caught the three TS18047 the widened declaration introduced. The section now states both programs, names the tsconfig exclusion as the filed defect (#14917), and gives the real reason the pin lives in the turso twin instead. Its reverse-verification paragraph now reports the OBSERVED leg (2 failed | 2 passed (4), all four ran) rather than predicting a compile-time red for a type pin this file does not have. `turso-update-missing-id.test.ts`: same correction. Restoring the fabricating EXPRESSION leaves the declaration untouched, so the `Equals` const cannot red and nothing fails at compile time; the parity pin is one assertion, not two halves; and the no-fabrication pin, omitted before, does red. The paragraph now names all five reds and all five greens from the run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68 --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: os-musk <elon@objectstack.ai>
1 parent b1d49b3 commit ca3fd4b

6 files changed

Lines changed: 597 additions & 7 deletions

File tree

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
---
2+
"@objectstack/driver-mongodb": minor
3+
"@objectstack/driver-turso": minor
4+
---
5+
6+
fix(drivers): `update()` on a missing id answers `null` on MongoDB and on Turso's remote face
7+
8+
**BREAKING** for TypeScript consumers — a published TYPE-surface narrowing alongside a
9+
runtime behaviour change, shipped as `minor` under the launch-window convention. Two
10+
published declared returns move: `MongoDBDriver.update()` and `RemoteTransport.update()`
11+
(both exported from their package index) now declare
12+
`Promise<Record<string, unknown> | null>` where they declared
13+
`Promise<Record<string, unknown>>`. A caller that reads fields off either result —
14+
`result.id`, `result.title` — no longer compiles until it narrows the `null` arm first.
15+
The narrowing is delivered by the compiler at every call site, and it is the honest
16+
declaration: the value that arm carries has always been reachable, it was simply being
17+
answered with a fabricated record instead.
18+
19+
`IDataDriver.update()` declares `Promise<Record<string, unknown> | null>` — the
20+
not-found arm landed with the ruling on the contract (`packages/spec` is untouched here),
21+
and it is the answer `InMemoryDriver`, `SqlDriver`, `SqliteWasmDriver` and `TursoDriver`'s
22+
local face have always given. Two implementations did not honour it. They **invented a
23+
record** instead:
24+
25+
- `MongoDBDriver.update()` ran `updateOne({ id })`, then `findOne({ id })`, and
26+
when nothing came back returned
27+
`withoutUndefinedOwnKeys({ id: String(id), ...updateData })` — a row assembled
28+
from the caller's own payload plus the `updated_at` it had just stamped, under
29+
an id that names no document.
30+
- `RemoteTransport.update()` ran `UPDATE … WHERE "id" = ?`, then
31+
`SELECT * … WHERE "id" = ?`, and when no row came back returned
32+
`{ id, ...data }` — the caller's payload with the id stapled on.
33+
34+
Both now return `null`. That is the runtime half of this change, and it is why this
35+
release is not a pure type-surface move: the value a caller receives for a missing id is
36+
different at run time, not only in the `.d.ts`.
37+
38+
This is the expensive direction of wrong, not merely the wrong answer: the
39+
fabricated row said **succeeded** where the truth was **not found**, and said it
40+
in a shape carrying the caller's own fields back, so nothing about it looked
41+
wrong. Through the engine's by-id door a REST / SDK / MCP `update` against a
42+
deleted or mistyped id answered **200 with a record that does not exist** — on
43+
these two implementations only. A caller, human or agent, read that as a landed
44+
write and did not retry, alert or roll back.
45+
46+
Two things downstream become correct rather than merely different:
47+
48+
- **One `TursoDriver`, one answer.** Its remote branch passes the transport
49+
result through `formatRemoteRow`, which already guards
50+
`row && typeof row === 'object'`, so `null` reaches the engine untouched and
51+
the two faces converge with no edit at that seam. Previously the same driver
52+
answered the same missing id two ways, chosen by `isRemote`.
53+
- **`RemoteTransport.bulkUpdate()`'s skip stops being dead code.**
54+
`if (updated) results.push(updated)` is the cross-driver convention
55+
`SqlDriver.bulkUpdate` follows; on this transport `updated` could never be
56+
falsy, so a batch over N missing ids answered N invented rows. It now answers
57+
the rows that exist.
58+
59+
`upsert()` is untouched on both drivers: an upsert never answers "not found".
60+
61+
No landed test pinned the fabricating posture on either driver, so the
62+
regression pins added here are net-new coverage rather than a changed baseline.
63+
64+
<!-- adr-0087: not-required (no-migration-prescription) No metadata key is removed, renamed or re-shaped: the moving surfaces are two driver methods' declared return types and the value they answer for an id that names no row, so there is nothing for `objectstack migrate meta`, `spec-changes.json` or the upgrade guide to project, and this changeset prescribes no rewrite. The consumer obligation is a TypeScript narrowing at the call site, delivered by the compiler. `type-surface-only` is NOT claimable here: its predicate 4 (narrowed-from-erased) is false — neither declared return was `any` at the merge base, they were the non-null `Promise<Record<string, unknown>>` — and, independently, runtime behaviour moves in the same diff, which is more than a type surface. Same disposition and reasoning as `.changeset/driver-memory-update-upsert-honest-types.md` (PR #14434), one day earlier in this same series. -->

packages/drivers/driver-mongodb/src/mongodb-driver.test.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -155,9 +155,13 @@ describe.skipIf(!sharedMongod)('MongoDBDriver', () => {
155155
it('should update a record and return updated data', async () => {
156156
await driver.create('task', { id: 'upd-1', title: 'Original', status: 'new' });
157157
const result = await driver.update('task', 'upd-1', { title: 'Updated', status: 'done' });
158-
expect(result.title).toBe('Updated');
159-
expect(result.status).toBe('done');
160-
expect(result.id).toBe('upd-1');
158+
// `update()` declares `Record<string, unknown> | null` (#14428): a miss
159+
// answers `null`. This case is the FOUND arm, so pin that first and read
160+
// the fields through it -- same idiom as `findOne` above.
161+
expect(result).not.toBeNull();
162+
expect(result!.title).toBe('Updated');
163+
expect(result!.status).toBe('done');
164+
expect(result!.id).toBe('upd-1');
161165
expect(result).not.toHaveProperty('_id');
162166
});
163167

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

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -400,7 +400,34 @@ export class MongoDBDriver implements IDataDriver {
400400
return result;
401401
}
402402

403-
async update(object: string, id: string | number, data: Record<string, unknown>, options?: DriverOptions): Promise<Record<string, unknown>> {
403+
/**
404+
* [#14428] A miss answers `null` — the arm `IDataDriver.update()` declares
405+
* (#13878) and the one `InMemoryDriver`, `SqlDriver`, `SqliteWasmDriver` and
406+
* `TursoDriver`'s local face already return.
407+
*
408+
* This door used to answer a missing id with a row ASSEMBLED from the
409+
* caller's own payload plus the `updated_at` it had just stamped:
410+
*
411+
* return (updated as Record[string, unknown]) || withoutUndefinedOwnKeys({ id: String(id), ...updateData });
412+
*
413+
* `updateOne` matched nothing, `findOne` came back `null`, and the caller was
414+
* handed a record for an id that names no document. The reason that was ever
415+
* written — "the declaration does not permit `null`, so something has to come
416+
* back" — was removed by #13878; the posture outlived it. The maintainer
417+
* ruled it out on 2026-09-03 (「同意」, decision batch #15 item 1, posture A).
418+
*
419+
* Why the fabricated row is the expensive direction, not merely the wrong
420+
* one: it says SUCCEEDED where the truth is NOT FOUND, and it says so in a
421+
* shape that carries the caller's own fields back, so nothing about it looks
422+
* wrong. A caller — human or agent — reads it as a landed write and does not
423+
* retry, alert or roll back. Through the engine's by-id door
424+
* (`engine.ts` → `driver.update`) it surfaced as a REST/SDK/MCP `200` with a
425+
* record that does not exist, on this driver and Turso's remote face only.
426+
*
427+
* ⚠️ `upsert()` is deliberately untouched: an upsert never answers
428+
* "not found" (it inserts instead), so it has no not-found arm to declare.
429+
*/
430+
async update(object: string, id: string | number, data: Record<string, unknown>, options?: DriverOptions): Promise<Record<string, unknown> | null> {
404431
const collection = this.getCollection(object);
405432
const session = this.getSession(options);
406433

@@ -419,7 +446,7 @@ export class MongoDBDriver implements IDataDriver {
419446
{ session, projection: { _id: 0 } },
420447
);
421448

422-
return (updated as Record<string, unknown>) || withoutUndefinedOwnKeys({ id: String(id), ...updateData });
449+
return (updated as Record<string, unknown> | null) ?? null;
423450
}
424451

425452
async upsert(object: string, data: Record<string, unknown>, conflictKeys?: string[], options?: DriverOptions): Promise<Record<string, unknown>> {
Lines changed: 202 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,202 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#14428] `MongoDBDriver.update()` answers a missing id with `null`, not with
5+
* a record it made up.
6+
*
7+
* # What was broken
8+
*
9+
* The door read:
10+
*
11+
* return (updated as Record[string, unknown]) || withoutUndefinedOwnKeys({ id: String(id), ...updateData });
12+
*
13+
* `updateOne({ id })` matching nothing and `findOne({ id })` coming back `null`
14+
* still produced a row — the caller's own payload plus the `updated_at` this
15+
* driver had just stamped, under an id that names no document. Since #13878
16+
* (PR #14434) `IDataDriver.update()` declares `Promise[Record[string, unknown]
17+
* | null]`, so "a row for an id that does not exist" is no longer a way of
18+
* satisfying the declaration: it is a value the declaration distinguishes from.
19+
* Four of six shipped implementations already answered `null`; this one and
20+
* Turso's remote face answered "updated". Maintainer ruling 2026-09-03,
21+
* posture A.
22+
*
23+
* # Why this file exists at all
24+
*
25+
* The card measured that NO landed test pinned the miss posture on this driver
26+
* — `mongodb-driver.test.ts:157,175` read `update()` results over rows that
27+
* EXIST. So this is net-new coverage, and the fabricating posture could have
28+
* come back without reddening anything.
29+
*
30+
* # Why it does not live in `mongodb-driver.test.ts`
31+
*
32+
* That suite is `describe.skipIf(!sharedMongod)` and `createTestMongod` skips
33+
* it unless `OS_TEST_MONGODB_MEMORY_SERVER_ENABLED=1` (#5517 retired the
34+
* 123 MB binary download that was ejecting unrelated PRs from the merge queue).
35+
* A pin added there would be GREEN-BY-SKIP on every CI run — a phantom pin,
36+
* which is worse than none: it reads as coverage in the file list and can
37+
* never fail. The fake `Db` below is the pattern
38+
* `mongodb-findone-options.test.ts` established and
39+
* `mongodb-own-key-undefined.test.ts` reuses: `getCollection` is
40+
* `this.db.collection(name)`, so replacing `db` observes every call the real
41+
* code path makes, with no server and no download.
42+
*
43+
* # The pins, and what each alone would miss
44+
*
45+
* - **The miss pin** is the defect: `findOne` empty ⇒ `null`.
46+
* - **The positive control** is what stops the fix from being "return `null`
47+
* always". A driver that had simply deleted the read-back would pass the
48+
* miss pin and break every update that works.
49+
* - **The no-fabrication pin** asserts the specific shape that used to come
50+
* back (the caller's fields, the stamped `updated_at`). `toBeNull()` alone
51+
* would also be satisfied by a driver that threw and was caught elsewhere;
52+
* this states what must NOT be synthesized.
53+
* - **The write-still-issued pin** holds the other half of the contract: the
54+
* `updateOne` is still sent. A "fix" that short-circuited on a miss by
55+
* reading FIRST would answer `null` correctly and quietly stop writing.
56+
*
57+
* # ⚠️ There is deliberately NO type-level pin in this file — and NOT because
58+
* # nothing would read one
59+
*
60+
* #13878's `memory-update-declared-null.test.ts` pins the declared return type
61+
* with `Equals`/`IsAny` consts. That instrument reaches this file through only
62+
* ONE program, and it is not the one whose name is on the package:
63+
*
64+
* - **This package's own `typecheck` script cannot see it.** `tsconfig.json`
65+
* here carries `"exclude": [..., "**\/*.test.ts"]` (escaped so this comment
66+
* does not terminate early), and `tsc --noEmit` reads that config. Measured,
67+
* not assumed: `tsc --noEmit --listFiles` in this package lists 0 files
68+
* ending `.test.ts` (the sibling `driver-turso`, whose tsconfig excludes
69+
* only `node_modules`/`dist`, lists 43). vitest transpiles without
70+
* typechecking, and the root `tsconfig.json` excludes `packages` entirely,
71+
* so neither of those picks it up either. That exclusion is itself a filed
72+
* defect (#14917), not a design.
73+
* - **`pnpm check:type-check-debt` DOES compile it.** The ratchet's
74+
* `--re-measure` leg generates a project that drops the test exclusion and
75+
* runs `tsc` over this package with its tests un-hidden, then compares the
76+
* error count to the frozen `TEST_DEBT['@objectstack/driver-mongodb']` entry
77+
* (10: `TS1309` x7 + `TS2550` x3). ⭐ Not theory: the first head of this
78+
* branch widened `update()`'s declaration, the three found-arm reads in
79+
* `mongodb-driver.test.ts` became `TS18047 'result' is possibly 'null'`, and
80+
* that lane went red at 13 (+3) while `pnpm typecheck` stayed green. CI
81+
* caught in this file's layer exactly what the package's own typecheck is
82+
* blind to.
83+
*
84+
* So a type pin here would not be a phantom — it would be checked, once, in a
85+
* lane that reports a break as a ledger COUNT moving rather than as a named
86+
* assertion failure, and that reports it only when someone runs the whole-repo
87+
* re-measure. The declaration is pinned by better instruments instead, both of
88+
* which run in this package's own `typecheck`:
89+
*
90+
* - **narrowing the declaration back** to `Promise[Record[string, unknown]]`
91+
* is a `tsc` error in `mongodb-driver.ts` itself — that file IS in the
92+
* program, and the body's `?? null` then returns `Record[string, unknown] |
93+
* null` from a non-null signature. `pnpm --filter @objectstack/driver-mongodb
94+
* typecheck` reds.
95+
* - **losing the contract linkage** (should `IDataDriver.update()` drop its
96+
* `| null` arm) reds the same typecheck through `implements IDataDriver`.
97+
* - **reverting the behaviour** while keeping the signature reds the runtime
98+
* pins below.
99+
*
100+
* # Reverse verification — predicted direction, then what was OBSERVED
101+
*
102+
* Predicted: restoring the `|| withoutUndefinedOwnKeys({ id: String(id),
103+
* ...updateData })` fallback reds the miss pin and the no-fabrication pin,
104+
* while the positive control and the write-still-issued pin stay GREEN — they
105+
* exercise the found arm, which the revert does not touch.
106+
*
107+
* Observed, with the mutation proved on disk (injected text counted, deleted
108+
* text absent) and the restore proved by a `git hash-object` match against the
109+
* HEAD blob: `Test Files 1 failed (1)`, `Tests 2 failed | 2 passed (4)`. The
110+
* two reds are the miss pin and the no-fabrication pin, by name. ⚠️ All FOUR
111+
* cases ran — there is no compile-time leg here, because there is no type pin
112+
* in this file to red; a prediction that the file would fail to typecheck as a
113+
* whole would have been wrong for exactly that reason.
114+
*/
115+
116+
import { describe, it, expect } from 'vitest';
117+
118+
import { MongoDBDriver } from './mongodb-driver.js';
119+
120+
/** What the fake collection recorded, so the WRITE half stays observable. */
121+
interface Recorded {
122+
updateOne: Array<{ filter: Record<string, unknown>; update: Record<string, unknown> }>;
123+
findOne: Array<Record<string, unknown>>;
124+
}
125+
126+
/**
127+
* A driver wired to a recording fake `Db` — no `connect()`, no server.
128+
*
129+
* `stored` is the document `findOne` answers with; `null` models the miss (a
130+
* real `findOne` resolves `null` when nothing matches), and an object models
131+
* the row that exists.
132+
*/
133+
function makeDriver(stored: Record<string, unknown> | null) {
134+
const recorded: Recorded = { updateOne: [], findOne: [] };
135+
const collection = {
136+
async updateOne(filter: Record<string, unknown>, update: Record<string, unknown>) {
137+
recorded.updateOne.push({ filter, update });
138+
return { matchedCount: stored ? 1 : 0, modifiedCount: stored ? 1 : 0 };
139+
},
140+
async findOne(filter: Record<string, unknown>) {
141+
recorded.findOne.push(filter);
142+
return stored;
143+
},
144+
};
145+
const driver = new MongoDBDriver({ url: 'mongodb://127.0.0.1:1/probe' });
146+
(driver as any).db = { collection: () => collection };
147+
return { driver, recorded };
148+
}
149+
150+
describe('[#14428] MongoDBDriver.update() on a missing id', () => {
151+
it('resolves null when no document carries that id', async () => {
152+
const { driver } = makeDriver(null);
153+
154+
const result = await driver.update('task', 'no-such-id', { title: 'edited' });
155+
156+
expect(result).toBeNull();
157+
// The narrowing the declared type demands of every caller.
158+
const title = result === null ? 'absent' : result.title;
159+
expect(title).toBe('absent');
160+
});
161+
162+
it('fabricates nothing — no id, no payload echo, no stamped updated_at', async () => {
163+
const { driver } = makeDriver(null);
164+
165+
const result = await driver.update('task', 'no-such-id', { title: 'edited', owner: 'u1' });
166+
167+
// The exact shape the old fallback produced: `{ id, ...updateData }` with
168+
// `updated_at` stamped a moment earlier. Asserted as a NON-match against a
169+
// reconstruction of it, so the pin names the thing it forbids rather than
170+
// only the thing it wants — `toBeNull()` alone would also be satisfied by a
171+
// driver that threw and was caught somewhere up the stack.
172+
//
173+
// ⚠️ NOT written as `expect(result).not.toBeTypeOf('object')`: `typeof
174+
// null` IS `'object'` in JS, so that assertion fails on the correct value.
175+
expect(result).toBeNull();
176+
expect(result).not.toMatchObject({ id: 'no-such-id' });
177+
expect(Object.keys((result as Record<string, unknown> | null) ?? {})).toEqual([]);
178+
});
179+
180+
it('still ISSUES the write — the miss is discovered by reading back, not by refusing', async () => {
181+
const { driver, recorded } = makeDriver(null);
182+
183+
await driver.update('task', 'no-such-id', { title: 'edited' });
184+
185+
expect(recorded.updateOne).toHaveLength(1);
186+
expect(recorded.updateOne[0].filter).toEqual({ id: 'no-such-id' });
187+
expect((recorded.updateOne[0].update as any).$set.title).toBe('edited');
188+
expect(recorded.findOne).toHaveLength(1);
189+
expect(recorded.findOne[0]).toEqual({ id: 'no-such-id' });
190+
});
191+
192+
it('POSITIVE CONTROL — an id that DOES exist still returns the stored row', async () => {
193+
const stored = { id: 'task-1', title: 'edited', owner: 'u1', updated_at: new Date('2026-01-01T00:00:00Z') };
194+
const { driver } = makeDriver(stored);
195+
196+
const result = await driver.update('task', 'task-1', { title: 'edited' });
197+
198+
expect(result).not.toBeNull();
199+
expect(result!.id).toBe('task-1');
200+
expect(result!.title).toBe('edited');
201+
});
202+
});

0 commit comments

Comments
 (0)