diff --git a/.changeset/turso-remote-upsert-null-autonumber-warning.md b/.changeset/turso-remote-upsert-null-autonumber-warning.md new file mode 100644 index 0000000000..f5f5ab6841 --- /dev/null +++ b/.changeset/turso-remote-upsert-null-autonumber-warning.md @@ -0,0 +1,47 @@ +--- +"@objectstack/driver-turso": patch +--- + +fix(driver-turso): a remote upsert that lands a NULL record number now says so (#7099) + +#6944 made the Turso REMOTE face refuse an `auto_number` write it cannot fulfil +instead of silently writing NULL, but one leg was left uncovered and declared as +known residue: an `upsert` carrying an `id` or explicit `conflictKeys` that +matches nothing still inserts, and the record-number slot still lands NULL. The +refusal is raised deliberately BEFORE the statement is built — that is what makes +a refused write cost zero round trips — and whether an `INSERT … ON CONFLICT` +merges or inserts is not knowable at that point. + +That outcome is UNCHANGED here. What changes is that it is no longer silent: + +``` +upsert({ id: 'never-seen', … }) -> RESOLVED case_number=null (before: nothing said) + + logger.warn naming the object, + the column and the row id (now) +``` + +The residue was recorded under the premise that detecting this leg would cost +"the round trip this refusal exists to avoid". Measured, that round trip is +already paid unconditionally: `RemoteTransport.upsert` follows its +`INSERT … ON CONFLICT` with `SELECT * FROM "" WHERE "id" = ?` and returns +the mapped row. So the NULL was in hand all along, one field read away, on the +layer that knows which column is an `auto_number` — and the leg is made loud +without buying anything and without a probe query. + +Scope, stated because it is deliberately narrow: + +- **No accept/reject change.** The same writes are accepted and refused as + before, and the returned row is byte-identical. Refusing after the write has + landed is a different act from the pre-write gate, and it is untouched. +- **Not a generator.** Issuing record numbers on the remote transport remains + deferred (#6944 disposition A). +- **Reported at `warn`, not `error`.** Everything the caller submitted persisted + and the returned row carries the `null` in plain sight; what is missing is a + derived value this face declares it does not issue. Per AGENTS.md + §Degradation log levels that is a functional degradation, not a durability one. +- **Not throttled.** Unlike the tenant-audit warning it sits beside, the row `id` + IS the payload — one line per unnumbered row is the list an operator repairs. + +Operators of remote Turso deployments carrying `auto_number` objects will see a +new `warn` line on this path. It reports a condition that was already happening +silently; no behaviour changed to produce it. diff --git a/packages/drivers/driver-turso/src/turso-driver.ts b/packages/drivers/driver-turso/src/turso-driver.ts index 96af6ac048..a0dbc48c12 100644 --- a/packages/drivers/driver-turso/src/turso-driver.ts +++ b/packages/drivers/driver-turso/src/turso-driver.ts @@ -189,6 +189,19 @@ export interface TursoDriverConfig { * break a path that works today and would take the two faces further apart, not * closer (#6203). */ +/** + * `SqlDriver.fillAutoNumberFields`'s own generate predicate: a slot holding + * `undefined` / `null` / `''` is one the driver would have had to fill. + * + * Stated once, read twice — by the pre-write refusal below and by the + * post-write report on the one leg that refusal provably cannot classify + * (#7099). Two spellings of "empty" would be two answers to one question, and + * the second one would drift. + */ +function isEmptyAutoNumberSlot(held: unknown): boolean { + return held === undefined || held === null || held === ''; +} + function refuseRemoteAutonumber(object: string, fields: string[], path: string): never { const err = new Error( `Object "${object}" declares auto_number field(s) [${fields.join(', ')}] left empty for this ` + @@ -653,16 +666,85 @@ export class TursoDriver extends SqlDriver { for (const row of rows) { if (!row || typeof row !== 'object') continue; for (const cfg of cfgs) { - // `fillAutoNumberFields`'s own generate predicate, verbatim: a slot - // holding undefined / null / '' is one the driver would have had to - // fill. - const held = row[cfg.name]; - if (held === undefined || held === null || held === '') empty.add(cfg.name); + if (isEmptyAutoNumberSlot(row[cfg.name])) empty.add(cfg.name); } } if (empty.size > 0) refuseRemoteAutonumber(object, [...empty], path); } + /** + * [#7099] Say out loud that a remote write landed a row whose declared + * `auto_number` column holds no record number. + * + * # Why this exists beside the refusal, rather than inside it + * + * {@link refuseUngeneratableRemoteAutonumber} runs BEFORE the statement is + * built, which is what makes a refused write cost zero round trips — and is + * also the reason it cannot cover every leg. An upsert carrying an `id` or + * explicit `conflictKeys` may merge (safe: the row keeps the number already + * in its column) or may insert (the slot lands NULL), and which one it did is + * not knowable before it runs. #6944 left that leg as declared residue. + * + * What has since been measured is that the round trip the residue was + * attributed to is **already paid**: `RemoteTransport.upsert` follows its + * `INSERT … ON CONFLICT` with `SELECT * FROM "" WHERE "id" = ?` and + * returns the mapped row, unconditionally. So the NULL is in hand at no extra + * cost, on the layer that knows which column is an `auto_number` — and the + * leg can be made loud without buying anything. Note the check is not "did it + * insert or merge": it is one field read on a row this method already holds. + * + * # Report, not refuse — deliberately + * + * The write has already happened when this runs. Refusing here would be a + * different act from the pre-write gate (it would have to undo a landed row), + * and that trade-off is untouched by this change: nothing about what the + * remote face accepts or rejects moves. Generating the number on remote stays + * behind the same appetite door (#6944 disposition A) it always has. + * + * # `warn`, not `error` + * + * By AGENTS.md §Degradation log levels this is a FUNCTIONAL degradation, not + * a durability one: everything the caller submitted persisted, nothing it + * claims to have stored is missing, and the returned row carries the `null` + * in plain sight rather than reporting a success that did not happen. What is + * absent is a DERIVED value this face declares it does not issue — the same + * capability gap the sibling legs answer with `NOT_IMPLEMENTED`/501. Grading + * it `error` would file a known capability gap beside real data loss, which + * is the over-application that rule warns about by name. + * + * # Not throttled, unlike the tenant-audit warning + * + * `SqlDriver.tenantAuditWarned` collapses its warning per `{object}:{op}` + * because that one reports a CONFIGURATION mistake — the second occurrence + * carries no information the first did not. Here the row `id` IS the payload: + * each occurrence names a different row that landed without a record number, + * and that list is what an operator repairs. One line per unnumbered row is + * proportional, not noisy. + */ + private reportUnnumberedRemoteRow( + object: string, + row: Record | null | undefined, + path: string, + ): void { + const cfgs = this.autoNumberFields[object]; + if (!cfgs || cfgs.length === 0) return; + if (!row || typeof row !== 'object') return; + const unfilled = cfgs + .filter((cfg) => isEmptyAutoNumberSlot(row[cfg.name])) + .map((cfg) => cfg.name); + if (unfilled.length === 0) return; + this.logger.warn( + `[driver-turso] ${path} on "${object}" returned row id ${JSON.stringify(row.id)} with ` + + `auto_number field(s) [${unfilled.join(', ')}] left empty. The Turso REMOTE transport does ` + + `not generate record numbers, so an upsert that matches no existing row inserts one without ` + + `it — the row is persisted, its record number is not, and nothing else reports this. Supply ` + + `the value explicitly on this path (a seed replay or import keeps its own numbers and is ` + + `written unchanged), or use the local / embedded-replica transport, which do issue them ` + + `(#7099).`, + { object, fields: unfilled, id: row.id, path }, + ); + } + override async create(object: string, data: Record, options?: DriverOptions): Promise { if (this.isRemote) { this.refuseUngeneratableRemoteAutonumber(object, [data], 'create'); @@ -691,15 +773,25 @@ export class TursoDriver extends SqlDriver { // insert — measured, two such upserts produced two rows with different // ids. That is also the shape the engine sends for a new record. // - // ⚠️ Residue, stated rather than papered over: an upsert that DOES carry - // an id or conflict keys but matches nothing still inserts, and on that - // leg the slot is still written NULL. Classifying it needs the round trip - // this refusal exists to avoid, so it is left as a known gap of the same - // deferred half (A) rather than answered with a probe query. + // ⚠️ The leg the pre-write gate cannot classify: an upsert that DOES + // carry an id or conflict keys but matches nothing still inserts, and on + // that leg the slot is still written NULL. That outcome is UNCHANGED by + // #7099 — what changed is that it is no longer silent. The row comes back + // through this method, so the NULL can be read AFTER the write and + // reported, without the probe query the pre-write gate exists to avoid + // and without turning an accepted write into a refused one. Generating + // the number here stays behind the deferred half (A). const mayMerge = data?.id !== undefined || data?._id !== undefined || (Array.isArray(conflictKeys) && conflictKeys.length > 0); if (!mayMerge) this.refuseUngeneratableRemoteAutonumber(object, [data], 'upsert'); - return this.formatRemoteRow(object, await this.remoteTransport!.upsert(object, this.toRemoteWriteForms(object, data), conflictKeys)); + const row = this.formatRemoteRow(object, await this.remoteTransport!.upsert(object, this.toRemoteWriteForms(object, data), conflictKeys)); + // Judged on the row the CALLER receives, after read-coercion — reporting + // a different value than the one handed out would be its own defect. The + // `!mayMerge` leg reaches this too and is a no-op there by construction: + // an empty slot was already refused above, and a caller-supplied number + // comes back filled. + this.reportUnnumberedRemoteRow(object, row, 'upsert'); + return row; } return super.upsert(object, data, conflictKeys, options); } diff --git a/packages/drivers/driver-turso/src/turso-remote-autonumber-refusal.test.ts b/packages/drivers/driver-turso/src/turso-remote-autonumber-refusal.test.ts index f6d973e431..fba81d10b9 100644 --- a/packages/drivers/driver-turso/src/turso-remote-autonumber-refusal.test.ts +++ b/packages/drivers/driver-turso/src/turso-remote-autonumber-refusal.test.ts @@ -78,9 +78,60 @@ * One detail not predicted and worth keeping: the reverted suite does not merely * go red, it REPRINTS the defect — every failure message carries the resolved * row with `"case_number":null` (or `""` for the empty-string case) inside it. + * + * # [#7099] The leg the gate cannot classify is now LOUD — still not refused + * + * The residue pin below was recorded under a premise that has since been + * measured false. It said classifying this leg "needs the round trip this + * refusal exists to avoid"; in fact `RemoteTransport.upsert` already runs + * `SELECT * FROM "" WHERE "id" = ?` after every `INSERT … ON CONFLICT` + * and returns the mapped row — the pin's own `expect(inserted.case_number)` + * was reading that very round trip's result. So the NULL was in hand all along, + * one field read away, on the layer that knows which column is an + * `auto_number`. No probe query was added, and none is needed. + * + * What did NOT change: the row still lands, and it still lands with a NULL + * record number. Refusing after the write is a different act from the pre-write + * gate, and generating the number on remote is still the deferred half (A). The + * pin therefore asserts BOTH halves — the unchanged NULL and the new warning — + * so a future change that quietly converts this leg into a refusal goes red on + * the first half rather than sliding through on the second. + * + * ## Where the detection lives, and why not on `RemoteTransport` + * + * The card's promoted scope suggested handing the autonumber rule DOWN to + * `RemoteTransport` (the `setFilterColumnSql` / `setDiagnosticSink` plumbing + * precedent). Measured, the driver is the better layer and needs no plumbing at + * all: `TursoDriver.upsert` already receives the transport's returned row and + * already holds `autoNumberFields[object]`, and its `logger.warn` is the very + * sink `setDiagnosticSink` forwards to — so routing through the transport would + * be a longer path to the same log line. It would also contradict the layering + * decision the block below pins: "RemoteTransport cannot see a field type, so it + * cannot be the one to refuse" asserts no member of `RemoteTransport.prototype` + * matches `/autonumber|sequence/i`. Teaching the transport this rule would have + * gone red on that pin — correctly. + * + * ## Reverse verification — both directions predicted BEFORE they were run + * + * ① Fix reverted (driver source restored to `origin/main`, all new tests kept). + * Predicted: exactly one red — the residue pin, failing on the WARNING half + * (`expect(lines).toHaveLength(1)`), never on the NULL half asserted above + * it. Measured `1 failed | 19 passed`, `expected [] to have a length of 1`. + * The 19 green include the three silence controls, which an unfixed driver + * satisfies trivially — stated plainly because it is what ② is for. + * + * ② Fix broken the OTHER way (empty-slot predicate dropped, so the report fires + * on every declared `auto_number` column). Predicted: exactly two red — the + * MERGE-leg control and the caller-supplied-number control — with the + * residue pin itself STAYING GREEN, since an over-firing rule still emits + * its one line, and with the no-`auto_number`-field control also staying + * green, since the registry lookup returns before the predicate is reached. + * Measured `2 failed | 18 passed`, both `expected [ Array(1) ] to deeply + * equal []`. That is the half ① cannot prove: the pin alone cannot tell a + * correct warning from a wolf-crying one, and the controls are what do. */ -import { describe, it, expect, afterEach } from 'vitest'; +import { describe, it, expect, afterEach, vi } from 'vitest'; import { TursoDriver } from './index.js'; import { RemoteTransport } from './remote-transport.js'; import { makeLibsqlSqliteStub, type LibsqlSqliteStub } from './libsql-sqlite-stub.testkit.js'; @@ -154,6 +205,25 @@ async function makeReplica() { return driver; } +/** + * [#7099] Watch the sink the driver actually writes operator diagnostics to. + * + * `TursoDriver` inherits `SqlDriver.logger` and `TursoDriver`'s own remote + * constructor forwards it to `RemoteTransport.setDiagnosticSink`, so this ONE + * spy sees a warning raised on either layer — which is what makes the assertion + * a statement about the operator's experience rather than about a call site. + * Filtered to `auto_number` lines so an unrelated warning (a tenant audit, a + * backfill) can neither satisfy nor break the pin. + */ +function watchAutoNumberWarnings(driver: TursoDriver) { + const sink = (driver as unknown as { logger: { warn: (msg: string, meta?: unknown) => void } }).logger; + const spy = vi.spyOn(sink, 'warn'); + return { + lines: () => spy.mock.calls.map((c) => String(c[0])).filter((m) => /auto_number/.test(m)), + restore: () => spy.mockRestore(), + }; +} + const rowCount = (stub: LibsqlSqliteStub, table: string) => (stub.raw.prepare(`select count(*) as c from "${table}"`).all() as Array<{ c: number }>)[0].c; @@ -308,20 +378,88 @@ describe('[#6944] REMOTE: what is deliberately NOT refused', () => { expect(row.title).toBe('n'); }); - it('[known residue, not fixed] an id-bearing upsert that INSERTS still writes NULL', async () => { - // Stated as an assertion rather than a comment so it cannot quietly become - // untrue in either direction. `RemoteTransport.upsert` emits - // `INSERT … ON CONFLICT DO UPDATE`, and whether that statement merges or - // inserts is knowable only after it runs — a round trip this refusal exists - // to avoid. So the leg stays open, on the same deferred half (A) as the rest - // of remote autonumber generation. + it('[#7099] an id-bearing upsert that INSERTS still writes NULL — and now says so', async () => { + // Both halves are asserted, and the pairing is the point. + // + // ① The NULL is UNCHANGED. #7099 restored observability on this leg, it did + // not change what the remote face accepts: refusing after the write has + // landed is a different act from the pre-write gate, and it stays out of + // scope. If this half ever flips, the change was a refusal or a + // generator, not a warning, and it owes the appetite-door conversation + // (#6944 disposition A). + // + // ② The silence is GONE. The premise the residue was recorded under — + // "whether that statement merges or inserts is knowable only after it + // runs, a round trip this refusal exists to avoid" — was measured false: + // `RemoteTransport.upsert` already pays that `SELECT` unconditionally + // and hands the row back, so the driver reads the NULL off a row it + // already holds. The warning names the object, the column and the id, + // because "some row somewhere lost its number" is not repairable. const { driver } = await makeRemote(); + const warnings = watchAutoNumberWarnings(driver); const inserted = await driver.upsert('crm_case', { id: 'never-seen', organization_id: 'orgA', title: 'inserted by upsert', }); expect(inserted.case_number).toBeNull(); + + const lines = warnings.lines(); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain('"crm_case"'); + expect(lines[0]).toContain('case_number'); + expect(lines[0]).toContain('never-seen'); + warnings.restore(); + }); + + it('[#7099] the MERGE leg stays silent — the warning does not cry wolf on a working path', async () => { + // The false-positive control, and the reason the check reads the RESULT row + // rather than the request: a merging upsert leaves the slot empty in the + // request too, so a request-shaped rule would warn on the one leg #6944 + // measured as correct. Nothing is wrong here — the row kept the number it + // already had — and an operator must not be told otherwise. + const { driver } = await makeRemote(); + await driver.create('crm_case', { + id: 'fixed3', + organization_id: 'orgA', + title: 'seeded', + case_number: 'CASE-00044', + }); + const warnings = watchAutoNumberWarnings(driver); + const merged = await driver.upsert('crm_case', { id: 'fixed3', organization_id: 'orgA', title: 'edited' }); + expect(merged.case_number).toBe('CASE-00044'); + expect(warnings.lines()).toEqual([]); + warnings.restore(); + }); + + it('[#7099] an object with no auto_number field never reaches the report', async () => { + // The other half of "no wolf": the registry lookup is keyed by object, so + // the overwhelming majority of remote upserts — objects with no record + // number at all — cost one map read and say nothing. A rule that warned on + // any NULL column would fire here, on a column that is simply absent. + const { driver } = await makeRemote(); + const warnings = watchAutoNumberWarnings(driver); + const row = await driver.upsert('crm_note', { id: 'note1', organization_id: 'orgA', title: 'n' }); + expect(row.title).toBe('n'); + expect(warnings.lines()).toEqual([]); + warnings.restore(); + }); + + it('[#7099] a caller-supplied number on the same leg is silent too', async () => { + // The seed-replay / import path crosses exactly this leg (id-bearing, no + // matching row) and is deliberately not refused. It must not be warned + // about either: the slot is filled, so there is nothing to report. + const { driver } = await makeRemote(); + const warnings = watchAutoNumberWarnings(driver); + const imported = await driver.upsert('crm_case', { + id: 'imported-1', + organization_id: 'orgA', + title: 'replayed', + case_number: 'CASE-00777', + }); + expect(imported.case_number).toBe('CASE-00777'); + expect(warnings.lines()).toEqual([]); + warnings.restore(); }); });