Skip to content

Commit ebdb6f2

Browse files
fix(driver-turso,driver-sql): a business-key upsert keeps the stored id on the remote face, and both faces answer the stored row (#21166) (#21184)
Fixes #21166 Clause-②: no ## What this does **Remote face (`driver-turso`).** `TursoDriver.upsert`'s remote branch now passes `SqlDriver.insertOnlyUpsertColumns(object)` to `RemoteTransport.upsert`. This is the same list the local faces build their merge set from, so it names `id`, `created_at` and the `auto_number` columns. Before, the branch passed only the autonumber columns, from a lookup of its own (`remoteAutoNumberColumns`, deleted here). The merge set then carried `"id" = excluded."id"`, and an upsert keyed on a business column replaced the stored row's primary key. This is the re-key the `SqlDriver.upsert` docblock forbids: "`id` is insert-only … the moment `conflictKeys` names a business key the merged row's identity is silently replaced". There is now one list and no second copy. A remote object never renames a column, because `remoteTableFor` refuses a renaming column map before any statement. So the list's physical names are the names the transport writes. **The answer (both faces).** With `id` insert-only, the remote read-back by the payload's `id` stopped finding the merged row and answered the payload instead. The local face already behaved that way: H3 below. Both read-backs now look the landed row up by its conflict-key values, which identify it on both legs. When a conflict key is empty in the payload, nothing can have matched, so the row was inserted and the read is by `id`. On the default `['id']` target, both readings are the same query. ## Hypotheses, measured - **H1: confirmed.** I reproduced it on `origin/main` `f3b16fc2f` with the card's table on the libsql-sqlite stub (`turso-local-remote-upsert-identity-parity.test.ts`, run before the fix: `8 failed | 5 passed`): - On the remote face, `upsert({ id: 'row-NEW', email: a, title: 'edited' }, ['email'])` stored `id: 'row-NEW'`. - `upsert({ email: b, title: 'edited too' }, ['email'])` stored `id: 'u9ZJdBf6xIsx0URs'`, a minted nanoid. - The `_id` alias stored `'row-ALIAS'`. - The local face kept `row-a` and `row-b`. - **H2: the raise rule is NOT met.** `runImport`'s `writeMode: 'upsert'` never reaches `driver.upsert`: - It finds the record by `matchFields` through `findExisting` → `p.findData` (`packages/core/src/utils/import-runner.ts:574`, `:583`). - Then it updates by id, `p.updateData({ id: target.id })` (`:941`), or it creates through `p.createData` (`:965`), `createManyData` or `insertManyData` (`:757`). - `ImportProtocolLike` (`:132`) declares no upsert member, and `bulk-write.ts` has zero `upsert` references. - The connector pull hands `artifact.upsertKey` to that runner as `matchFields` (`packages/services/service-automation/src/connector-pull.ts:291`, `:413`). - The only in-repo `driver.upsert` caller is still `LifecycleService` with `['id']` (`packages/objectql/src/lifecycle/lifecycle-service.ts:1405`). The sandbox `ql.upsert` falls back to `insert`, because the engine has no `upsert`. - **H3: measured on SQLite and on a private live PostgreSQL 16. It is the same list's consequence, so it is fixed here.** `SqlDriver.upsert` stored `os21166_seed` and answered `id: 'os21166_new'`: the read by the payload's `id` missed, and `|| toUpsert` answered the payload. That is the failing ablation leg A1 below, on both cells. MySQL was NOT MEASURED, because no server is in this container. The code path is dialect-independent. **File-surface increment (declared).** The claim listed `sql-driver.ts` as read-only unless lifted. H3's ruling arm adds three things: - the read-back in `packages/drivers/driver-sql/src/sql-driver.ts` (nothing else in that file); - one pin in `sql-driver-upsert-conflict-target-dialects.test.ts`; - `@objectstack/driver-sql: patch` beside `@objectstack/driver-turso: patch` in the changeset. ## Pins - `driver-turso/src/turso-local-remote-upsert-identity-parity.test.ts`, 13 cases on both faces: - the card's table: each stored id is kept, the merge still writes `title`, and the answer names the stored id; - the `_id` alias; - `created_at` is insert-only, the shared list's second member; - controls: an `id`-keyed upsert, by default and with `['id']` named, and a business-key upsert that inserts; - a parity case that compares the two faces with each other and with the right answer. - `driver-sql/src/sql-driver-upsert-conflict-target-dialects.test.ts` adds one case to the existing pins that keep the merged row's identity. It runs on the SQLite and live-PG cells: the answer names the stored id for a supplied and for a minted payload id, and the insert-leg answer is the control. ## Ablations Each fix was committed first. Each mutation went through `scripts/ablation-replace.mjs`, which proved it on disk, and was restored to a HEAD-equal blob. | leg | mutation | red | green | |:--|:--|:--|:--| | A1 | local read-back back to `where('id', toUpsert.id)` (driver-sql rebuilt; `ablation-dist-preflight` saw the marker in 2 dist files, then `--absent` after the restore build) | turso pins: the 3 local answer cases + parity (`4 failed`); driver-sql pin on **sqlite and live postgres** (`2 failed`, PG answered `'os21166_new'`) | every remote case | | A2 | remote set filtered back to autonumber-only | the 3 remote stored-id cases, remote `created_at`, parity (`5 failed`) | local face, controls | | A2b | only `created_at` dropped from the remote set | remote `created_at` alone (`1 failed`; stored `'2001-01-01…'`) | the rest | | A3 | remote read-back back to `"id" = ?` | the 3 remote answer cases + parity (`4 failed`; answered `'row-NEW'`) | remote stored-id cases | ## Tests (head `f80af709ec`, merge of `origin/main` `fbcc05f40`; all under `os-verify-lock.sh`) The test runs below ran on the fix commit `7a597948b3`. The merge brought only `plugin-audit` and docs files, so they were not re-run. The gate union ran on `f80af709ec`. - `driver-sql` build exit 0. Closure `pnpm --filter '@objectstack/driver-turso^...' build` exit 0. - `driver-turso` full `vitest run`: `83 passed (83)` files, `2231 passed | 33 skipped`. - `driver-sql` full suite in two halves of 109 and 108 files: `104 passed | 5 skipped`, `1440 passed | 100 skipped`; `102 passed | 6 skipped`, `1915 passed | 88 skipped`. - The 8 `driver-sql` files that call `.upsert(` with `OS_TEST_POSTGRES_URL` on a private PG 16: `8 passed`, `125 passed | 3 skipped`, live postgres RAN, live mysql NOT RUN. - `driver-sqlite-wasm` (`extends SqlDriver`) upsert file: `3 passed`. - `driver-turso` and `driver-sql` typecheck exit 0. `--listFiles` includes both test files. - Driver conformance ledger, before (`f3b16fc2f`) and after: identical, `50 covered cell(s), 0 in the DEBT ledger, 0 exempt`. - Gate union, re-derived with no paths on `f80af709ec`: 63 commands plus 4 roster gates named as ⛔ for these paths. All exit 0. `check:dual-build-cjs-loads` and `check:lean-entry-closure` first exited 3 (PREREQUISITE NOT MET) and exited 0 after the builds they name. `dispatch-gates --ran`: `63 derived, 63 run, 0 NOT-MEASURED, 0 UNRUN`. ## Acceptance notes - **Pin sweep.** No other test pinned the payload answer. `driver-memory` already answers the stored id on a business-key merge (`memory-unique-constraint.test.ts:174` expects `'1'`), so the SQL faces now agree with it. - The answer pin sits in the `sqlite` + `pg` sweep, so the MySQL cell does not run it. The MySQL read-back takes the same branch. - A measured cross-tenant behaviour of business-key upserts on the local face goes to the seat in the report. It was not changed here. --- _Generated by [Claude Code](https://claude.ai/code/session_01Ujdtvqs7ree7WyQmEDwEnG)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2c1cef3 commit ebdb6f2

6 files changed

Lines changed: 354 additions & 16 deletions

File tree

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
'@objectstack/driver-turso': patch
3+
'@objectstack/driver-sql': patch
4+
---
5+
6+
An `upsert` keyed on a business column keeps the stored row's primary key on the Turso remote face, and both drivers answer the stored row.
7+
8+
Clause-②: no
9+
10+
**Remote face (`@objectstack/driver-turso`).** `upsert(object, data, ['email'])` on a remote (hosted) database used to replace the matched row's `id`: with the payload's `id` when it carried one, else with a freshly generated one. Every reference to the old id was left pointing at nothing, and no error was raised. The merge now leaves `id` and `created_at` alone, as the local and embedded-replica faces already do. It reads the columns to leave alone from the same list the local faces use, so `id`, `created_at` and the `auto_number` columns are kept on a merge on every face. An upsert on the primary key (no `conflictKeys`, or `['id']`) is unchanged, and an upsert that inserts still writes the payload's `id`, or a generated one.
11+
12+
**The answer (`@objectstack/driver-sql`, and the remote face).** On such a merge, `upsert` returned the payload instead of the stored row, so the answer carried the payload's `id` (or the generated one), an id no stored row has. It now returns the stored row: its own `id`, with the merged values. The row is read back by the conflict-key values. When a conflict key is empty in the payload, nothing can have matched it, so the row was inserted and it is read back by its `id`, as before.
13+
14+
To change a row's `id` on purpose, use `update()`. An `upsert` never changes it.

‎packages/drivers/driver-sql/src/sql-driver-upsert-conflict-target-dialects.test.ts‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -466,6 +466,42 @@ function declareRefusalSweep(cell: DialectCell): void {
466466
expect(rows[0].title, 'the alias call must still have merged its other columns').toBe('third');
467467
});
468468

469+
/**
470+
* [#21166] The pins above read the STORE. This one reads the ANSWER, which
471+
* they never did: the call kept the stored `id` and then answered the
472+
* payload's. The read-back looked the row up by the payload's `id`, which a
473+
* merge on a business key never writes, found nothing, and fell back to
474+
* the payload. Measured before the fix, on both cells:
475+
*
476+
* ```
477+
* upsert({ id: 'os21166_new', email, title: 'second' }, ['email'])
478+
* stored id 'os21166_seed' answered id 'os21166_new'
479+
* upsert({ email, title: 'third' }, ['email'])
480+
* stored id 'os21166_seed' answered id = the nanoid minted for the insert that lost
481+
* ```
482+
*
483+
* The insert leg is the control: there the payload's `id` IS the stored
484+
* one, and a fix that answered the wrong row on that leg goes red here.
485+
*/
486+
it('answers the row the merge landed on: its stored `id`, not the payload’s', async () => {
487+
await driver.upsert(BACKED.name, { id: 'os21166_seed', email: 'ans@b.com', title: 'first' }, ['email']);
488+
489+
const supplied = await driver.upsert(BACKED.name, { id: 'os21166_new', email: 'ans@b.com', title: 'second' }, ['email']);
490+
expect(supplied.id, 'the answer names an id no stored row has').toBe('os21166_seed');
491+
expect(supplied.title, 'the answer must be the merged row, merged columns included').toBe('second');
492+
493+
const minted = await driver.upsert(BACKED.name, { email: 'ans@b.com', title: 'third' }, ['email']);
494+
expect(minted.id, 'the answer names the nanoid minted for the insert that lost').toBe('os21166_seed');
495+
expect(minted.title).toBe('third');
496+
497+
const stored = await driver.find(BACKED.name, { where: { email: 'ans@b.com' } });
498+
expect(stored.map((r: any) => ({ id: r.id, title: r.title }))).toEqual([{ id: 'os21166_seed', title: 'third' }]);
499+
500+
const inserted = await driver.upsert(BACKED.name, { id: 'os21166_ins', email: 'ins@b.com', title: 'new' }, ['email']);
501+
expect(inserted.id, 'the insert leg answers the id it wrote').toBe('os21166_ins');
502+
expect(inserted.email).toBe('ins@b.com');
503+
});
504+
469505
/**
470506
* The counterweight, and the reason the three pins above are a repair rather
471507
* than a capability removal: re-keying a row is still possible, through the

‎packages/drivers/driver-sql/src/sql-driver.ts‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8865,10 +8865,14 @@ export class SqlDriver implements IDataDriver {
88658865
// transaction (inside one the sequence UPDATE rolls back with the refused
88668866
// INSERT, so nothing is burned and there is nothing to repair — measured).
88678867
const mayRetry = options?.transaction === undefined;
8868+
// The row the last attempt SENT, in storage form: the read-back below
8869+
// looks the landed row up by its conflict-key values.
8870+
let sent: Record<string, any> = {};
88688871
for (let attempt = 0; ; attempt++) {
88698872
const reservations = await this.fillAutoNumberFields(object, toUpsert, options);
88708873

88718874
const formatted = this.applyWriteColumnMap(object, this.formatInput(object, toUpsert));
8875+
sent = formatted;
88728876
this.stampInsertTimestamps(object, formatted);
88738877
// [#11176] …and the same slot filled on Postgres/MySQL, where the line
88748878
// above returns early. Without it `updated_at` is not in `formatted`, so
@@ -9052,7 +9056,23 @@ export class SqlDriver implements IDataDriver {
90529056
}
90539057
}
90549058

9055-
const readback = this.getBuilder(object, options).where('id', toUpsert.id);
9059+
// [#21166] Read back the row the statement landed on, by the identity it
9060+
// MATCHED on: the conflict-key values. Reading it back by `toUpsert.id`
9061+
// answers the wrong row on exactly the call #8622 protects: a merge on a
9062+
// business key keeps the stored row's `id`, so the payload's `id` (or the
9063+
// nanoid minted above) names no row, the read finds nothing, and the
9064+
// fallback below answered the PAYLOAD. Measured on SQLite and live
9065+
// Postgres 16: the row stored as `row-a` answered `id: 'row-NEW'`, an id
9066+
// no stored row has. The conflict-key values name the landed row on both
9067+
// legs: the inserted row carries them, and the merged row is the one that
9068+
// matched them. A conflict key the row leaves empty cannot have matched
9069+
// (NULL never conflicts), so the statement inserted and the row carries
9070+
// `toUpsert.id`. On the default `['id']` target the two readings are the
9071+
// same query. The tenant scope is applied to either, as before.
9072+
const matchedOn = mergeKeys.every((k) => sent[k] !== undefined && sent[k] !== null);
9073+
const readback = this.getBuilder(object, options);
9074+
if (matchedOn) for (const k of mergeKeys) readback.where(k, sent[k]);
9075+
else readback.where('id', toUpsert.id);
90569076
this.applyTenantScope(readback, object, options);
90579077
const result = await readback.first();
90589078
return this.formatOutput(object, result) || toUpsert;

‎packages/drivers/driver-turso/src/remote-transport.ts‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1651,10 +1651,21 @@ export class RemoteTransport {
16511651
throw e;
16521652
}
16531653

1654-
// Fetch the result row
1654+
// Fetch the row the statement landed on, by the identity it MATCHED on:
1655+
// the conflict-key values. [#21166] Reading it back by the payload's `id`
1656+
// answers the wrong row once `id` is insert-only: a merge on a business
1657+
// key keeps the stored row's `id`, so the payload's `id` (or the nanoid
1658+
// minted above) names no row, the read finds nothing, and the fallback
1659+
// below answered the PAYLOAD as if it had been stored. The conflict-key
1660+
// values name the landed row on both legs: the inserted row carries them,
1661+
// and the merged row is the one that matched them. A conflict key the
1662+
// payload leaves empty cannot have matched (NULL never conflicts), so the
1663+
// statement inserted and the row carries this call's `id`. On the default
1664+
// `['id']` target the two readings are the same statement.
1665+
const keyColumns = mergeKeys.every((k) => toUpsert[k] !== undefined && toUpsert[k] !== null) ? mergeKeys : ['id'];
16551666
const result = await this.client!.execute({
1656-
sql: `SELECT * FROM ${this.tableSql(table)} WHERE "id" = ?`,
1657-
args: [toUpsert.id],
1667+
sql: `SELECT * FROM ${this.tableSql(table)} WHERE ${keyColumns.map((k) => `"${k}" = ?`).join(' AND ')}`,
1668+
args: keyColumns.map((k) => this.serializeValue(toUpsert[k])),
16581669
});
16591670
const rows = this.mapRows(result);
16601671
return rows[0] || toUpsert;

‎packages/drivers/driver-turso/src/turso-driver.ts‎

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2615,17 +2615,6 @@ export class TursoDriver extends SqlDriver {
26152615
}
26162616
}
26172617

2618-
/**
2619-
* The `auto_number` columns of `object`, looked up as `fillAutoNumberFields`
2620-
* looks them up (object name first, then the physical table it maps to), so
2621-
* the columns named insert-only to the transport are exactly the ones a
2622-
* number was issued for.
2623-
*/
2624-
private remoteAutoNumberColumns(object: string, table: string): string[] {
2625-
const cfgs = this.autoNumberFields[object] || this.autoNumberFields[table];
2626-
return cfgs ? cfgs.map((cfg) => cfg.name) : [];
2627-
}
2628-
26292618
// [#15267] The override declares the contract's type, as both of its branches
26302619
// already do: `RemoteTransport.create()` answers `Record<string, unknown>`
26312620
// through the generic `formatRemoteRow`, and the local branch forwards to
@@ -2683,7 +2672,19 @@ export class TursoDriver extends SqlDriver {
26832672
// going unused exactly as it does on the local faces (a gap in the
26842673
// sequence, never a renumbering). An explicit payload value does not
26852674
// renumber a merged row either; `update()` is the renumbering path.
2686-
const insertOnly = this.remoteAutoNumberColumns(object, table);
2675+
//
2676+
// [#21166] The insert-only set is `insertOnlyUpsertColumns` itself, the
2677+
// list the local faces build their merge set from, so it names `id` and
2678+
// `created_at` beside the autonumber columns. This face once handed the
2679+
// transport the autonumber columns alone, from a lookup of its own, so a
2680+
// merge on a business key (`conflictKeys: ['email']`) wrote
2681+
// `"id" = excluded."id"` and replaced the stored row's primary key with
2682+
// the payload's, or with the nanoid minted for the insert that lost
2683+
// (#8622's re-key, on this face). One list, so the faces cannot drift on
2684+
// which columns a merge may write. A remote object never renames a
2685+
// column (`remoteTableFor` refuses a renaming column map first), so the
2686+
// list's physical names are the names the transport writes.
2687+
const insertOnly = [...this.insertOnlyUpsertColumns(object)];
26872688
const written = await this.writeRemoteRowWithAutoNumbers(object, { ...data }, options, (filled) =>
26882689
this.remoteTransport!.upsert(object, this.toRemoteWriteForms(object, filled), conflictKeys, table, insertOnly),
26892690
);

0 commit comments

Comments
 (0)