Skip to content

Commit d254421

Browse files
fix(driver-sql): a merge-path upsert no longer rewrites an existing row's autonumber (#7011) (#7059)
An autonumber is an immutable business identifier once assigned (triage ruling on the card). fillAutoNumberFields reserves a fresh number before the statement knows whether it will insert or merge, and the autonumber column sat in mergeColumns — so every ON CONFLICT ... DO UPDATE wrote the freshly reserved number over the existing row's, measured on a healthy counter: create -> CASE-00001, two upserts of the same id -> CASE-00002 then CASE-00003, one row throughout. auto_number columns are now excluded from the merge column list, exactly like created_at (insert-only facts about the row's birth). The exclusion is unconditional — an explicit payload value does not renumber on merge either; update() remains the deliberate renumbering path. Insert-path upserts still assign fresh numbers; every non-autonumber column (including updated_at) merges as before. Out of scope, deliberately (#6943's reseed family): the reservation still happens before insert-vs-merge is known, so a merge-only upsert still consumes one sequence value per call — a permanent gap now, no longer a rewrite (measured post-fix: last_value 1 -> 2 -> 3, next fresh row gets CASE-00004). Faces: SqliteWasmDriver inherits upsert; TursoDriver local routes its override to super — both pinned by their own tests. Turso remote never enters fillAutoNumberFields (neither defect nor fix). Claude-Session: https://claude.ai/code/session_01LGRN2cSRfggfX9B2L83bQc Co-authored-by: Claude <noreply@anthropic.com>
1 parent b3fe0f9 commit d254421

5 files changed

Lines changed: 278 additions & 3 deletions

File tree

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
---
2+
"@objectstack/driver-sql": patch
3+
---
4+
5+
fix(driver-sql): a merge-path `upsert` no longer rewrites an existing row's autonumber (#7011)
6+
7+
Measured on a completely healthy counter, single row throughout:
8+
9+
```
10+
create → CASE-00001 last_value 1
11+
upsert same id (1st time) → CASE-00002 last_value 2
12+
upsert same id (2nd time) → CASE-00003 last_value 3
13+
```
14+
15+
`fillAutoNumberFields` reserves a number before the statement knows whether it
16+
will insert or merge, and the autonumber column sat in `mergeColumns` — so
17+
every `ON CONFLICT … DO UPDATE` wrote the freshly reserved number over the
18+
row's existing one, silently replacing an externally visible business
19+
identifier the caller never asked to change.
20+
21+
Per the triage ruling on the card: an autonumber is an **immutable business
22+
identifier once assigned**. `auto_number` columns are now excluded from the
23+
merge column list, exactly like `created_at` (both are insert-only facts about
24+
the row's birth). After the fix the same sequence keeps `CASE-00001` through
25+
both upserts. The exclusion is unconditional — an explicit autonumber value in
26+
the upsert payload does not renumber an existing row on the merge branch
27+
either; `update()` writes what it is given and remains the deliberate
28+
renumbering path. Insert-path upserts still assign fresh numbers, and every
29+
non-autonumber column (including `updated_at`) merges as before.
30+
31+
Deliberately out of scope (#6943's reseed family): the reservation itself still
32+
happens before insert-vs-merge is known, so a merge-only upsert still consumes
33+
one sequence value per call — now a permanent gap in the sequence rather than a
34+
rewrite of the row (measured post-fix: row keeps `CASE-00001`, `last_value`
35+
walks 1 → 2 → 3, the next inserted row gets `CASE-00004`).
36+
37+
Covered faces: `SqliteWasmDriver` inherits `upsert` unchanged; `TursoDriver`
38+
local/replica routes its override to `super` — both pinned by their own tests.
39+
Turso remote (`RemoteTransport.upsert`) never enters `fillAutoNumberFields` and
40+
has neither the defect nor the fix. Rows already renumbered by past merges
41+
cannot be restored from the driver side.
Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#7011] A merge-path upsert must never rewrite an existing row's autonumber.
5+
*
6+
* # The defect, measured on a HEALTHY counter (no staleness involved)
7+
*
8+
* ```
9+
* create → CASE-00001 last_value 1
10+
* upsert same id (1st time) → CASE-00002 last_value 2
11+
* upsert same id (2nd time) → CASE-00003 last_value 3
12+
* ```
13+
*
14+
* One row throughout — and its `case_number` was rewritten twice. The cause:
15+
* `fillAutoNumberFields` reserves a number before the statement knows whether
16+
* it will insert or merge, and the autonumber column sat in `mergeColumns`, so
17+
* the `ON CONFLICT … DO UPDATE` branch wrote the freshly reserved number over
18+
* the row's existing one.
19+
*
20+
* # The ruling this file pins (triage, 2026-08-09, on the card)
21+
*
22+
* An autonumber is an **immutable business identifier once assigned** — it is
23+
* usually an externally visible document number (`CASE-00001`), and a caller
24+
* asking to update a row did not ask for a new one. The fix excludes
25+
* `auto_number` columns from `mergeColumns`, exactly like `created_at` (both
26+
* are insert-only facts about the row's birth). The exclusion is unconditional:
27+
* even an EXPLICIT autonumber value in the upsert payload does not rewrite an
28+
* existing row's number on the merge branch — `update()` writes whatever it is
29+
* given and remains the deliberate renumbering path.
30+
*
31+
* # Out of scope here, deliberately
32+
*
33+
* The reservation itself still happens before insert-vs-merge is known, so a
34+
* merge-only upsert still consumes a sequence value (leaves a gap). That
35+
* pre-burn half belongs to #6943's reseed family and is NOT pinned by this
36+
* file — no test here asserts `last_value` on the merge path, so deferring the
37+
* reservation later cannot turn this file red.
38+
*
39+
* # Reverse verification (direction predicted before running)
40+
*
41+
* Restoring the deleted limb — removing the autonumber exclusion from
42+
* `mergeColumns` — turns exactly the merge-path pins below red, with the
43+
* filing's own values (received `CASE-00002` / `CASE-00003` where `CASE-00001`
44+
* was asserted). The insert-path and non-autonumber-merge cases stay green
45+
* either way; they are here to pin that the exclusion does not over-reach.
46+
*/
47+
48+
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
49+
import { SqlDriver } from './index.js';
50+
51+
const CRM_CASE = {
52+
name: 'crm_case',
53+
fields: {
54+
organization_id: { type: 'string' },
55+
case_number: { type: 'autonumber', format: 'CASE-{00000}', unique: true },
56+
title: { type: 'string' },
57+
status: { type: 'string' },
58+
},
59+
} as any;
60+
61+
describe('[#7011] merge-path upsert keeps the assigned autonumber', () => {
62+
let driver: SqlDriver;
63+
64+
const knex = () => (driver as any).knex;
65+
66+
const rowCount = async () => Number((await knex()('crm_case').count({ c: '*' }).first()).c);
67+
68+
beforeEach(async () => {
69+
driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true });
70+
await driver.initObjects([CRM_CASE]);
71+
});
72+
73+
afterEach(async () => {
74+
await driver.disconnect();
75+
});
76+
77+
it('keeps the existing number across repeated merges to the same row (the filing repro)', async () => {
78+
const created = await driver.create('crm_case', { organization_id: 'orgA', title: 'original' });
79+
expect(created.case_number).toBe('CASE-00001');
80+
81+
const first = await driver.upsert('crm_case', { id: created.id, organization_id: 'orgA', title: 'edit 1' });
82+
expect(first.case_number).toBe('CASE-00001'); // was CASE-00002 before the fix
83+
expect(first.title).toBe('edit 1');
84+
85+
const second = await driver.upsert('crm_case', { id: created.id, organization_id: 'orgA', title: 'edit 2' });
86+
expect(second.case_number).toBe('CASE-00001'); // was CASE-00003 before the fix
87+
expect(second.title).toBe('edit 2');
88+
89+
// One row throughout, and the stored value agrees with the returned one.
90+
expect(await rowCount()).toBe(1);
91+
const stored = await knex()('crm_case').where({ id: created.id }).first();
92+
expect(stored.case_number).toBe('CASE-00001');
93+
});
94+
95+
it('does not rewrite the number even when the payload carries an explicit one', async () => {
96+
const created = await driver.create('crm_case', { organization_id: 'orgA', title: 'original' });
97+
expect(created.case_number).toBe('CASE-00001');
98+
99+
// An explicit value is skipped by fillAutoNumberFields (caller-supplied),
100+
// but it still sits in the INSERT's column list — the exclusion is what
101+
// keeps it off the merge branch. `update()` remains the deliberate
102+
// renumbering path for the caller who really means it.
103+
const merged = await driver.upsert('crm_case', {
104+
id: created.id,
105+
organization_id: 'orgA',
106+
case_number: 'CASE-99999',
107+
title: 'tried to renumber',
108+
});
109+
expect(merged.case_number).toBe('CASE-00001');
110+
expect(merged.title).toBe('tried to renumber');
111+
});
112+
113+
it('insert-path upsert still assigns a fresh number', async () => {
114+
const first = await driver.upsert('crm_case', { organization_id: 'orgA', title: 'new row' });
115+
expect(first.case_number).toBe('CASE-00001');
116+
117+
const second = await driver.upsert('crm_case', { organization_id: 'orgA', title: 'another new row' });
118+
expect(second.case_number).toBe('CASE-00002');
119+
120+
expect(await rowCount()).toBe(2);
121+
});
122+
123+
it('still merges every non-autonumber column normally', async () => {
124+
const created = await driver.create('crm_case', { organization_id: 'orgA', title: 'original', status: 'open' });
125+
126+
const merged = await driver.upsert('crm_case', {
127+
id: created.id,
128+
organization_id: 'orgA',
129+
title: 'renamed',
130+
status: 'closed',
131+
});
132+
133+
expect(merged.title).toBe('renamed');
134+
expect(merged.status).toBe('closed');
135+
expect(merged.case_number).toBe('CASE-00001');
136+
expect(await rowCount()).toBe(1);
137+
});
138+
139+
it('a merge on a non-id conflict key keeps the number too', async () => {
140+
// The exclusion is per-column, not per-conflict-target: merging on a
141+
// business key instead of `id` must protect the number the same way.
142+
await driver.initObjects([
143+
{
144+
name: 'crm_ticket',
145+
fields: {
146+
organization_id: { type: 'string' },
147+
ticket_number: { type: 'autonumber', format: 'TKT-{0000}', unique: true },
148+
external_ref: { type: 'string', unique: 'global' },
149+
title: { type: 'string' },
150+
},
151+
} as any,
152+
]);
153+
154+
const created = await driver.create('crm_ticket', { organization_id: 'orgA', external_ref: 'ext-1', title: 'first' });
155+
expect(created.ticket_number).toBe('TKT-0001');
156+
157+
const merged = await driver.upsert(
158+
'crm_ticket',
159+
{ id: created.id, organization_id: 'orgA', external_ref: 'ext-1', title: 'merged by ref' },
160+
['external_ref'],
161+
);
162+
expect(merged.title).toBe('merged by ref');
163+
164+
const stored = await knex()('crm_ticket').where({ external_ref: 'ext-1' }).first();
165+
expect(stored.ticket_number).toBe('TKT-0001');
166+
});
167+
});

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

Lines changed: 37 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3983,6 +3983,27 @@ export class SqlDriver implements IDataDriver {
39833983
return this.formatOutput(object, updated) || null;
39843984
}
39853985

3986+
/**
3987+
* Columns `upsert`'s merge branch must never write ([#7011]): `created_at`
3988+
* (the row's birth timestamp belongs to the original insert) and every
3989+
* `auto_number` column — an autonumber is an immutable business identifier
3990+
* once assigned, so an upsert that lands on an existing row keeps that row's
3991+
* number (see the fuller rationale at the merge site). Autonumber columns
3992+
* are returned under their PHYSICAL names (an external object can remap
3993+
* logical fields via `external.columnMap`), matching the
3994+
* `applyWriteColumnMap`-processed row the merge column list is derived from;
3995+
* `created_at` stays the literal post-map key it has always been filtered as.
3996+
*/
3997+
protected insertOnlyUpsertColumns(object: string): Set<string> {
3998+
// Same config resolution as `fillAutoNumberFields`: object name first,
3999+
// then the storage-mapped table name.
4000+
const tableName = this.physicalTableByObject[object] ?? StorageNameMapping.resolveTableName({ name: object } as any);
4001+
const cfgs = this.autoNumberFields[object] || this.autoNumberFields[tableName] || [];
4002+
const columns = new Set<string>(['created_at']);
4003+
for (const cfg of cfgs) columns.add(this.remoteColumn(object, cfg.name, cfg.name));
4004+
return columns;
4005+
}
4006+
39864007
async upsert(object: string, data: Record<string, any>, conflictKeys?: string[], options?: DriverOptions): Promise<Record<string, any>> {
39874008
const { _id, ...rest } = data;
39884009
const toUpsert = { ...rest };
@@ -4022,9 +4043,22 @@ export class SqlDriver implements IDataDriver {
40224043
const builder = this.getBuilder(this.rotationWriteTarget(object) ?? object, options);
40234044
// `created_at` is insert-only — never overwrite it when an existing row is
40244045
// merged on conflict (the stamped/seeded value belongs to the original
4025-
// insert). Everything else (incl. `updated_at`) merges as before, so an
4026-
// upsert that updates a row still advances `updated_at`.
4027-
const mergeColumns = Object.keys(formatted).filter((c) => c !== 'created_at');
4046+
// insert). [#7011] `auto_number` columns are insert-only for the same
4047+
// reason, and a stronger one: the number is an externally visible
4048+
// business identifier once assigned (`CASE-00001`), and
4049+
// `fillAutoNumberFields` above reserved a FRESH value before the
4050+
// statement could know it would merge — leaving these columns in the
4051+
// merge set rewrote the existing row's number on every merge-path upsert
4052+
// (measured on a healthy counter: create → CASE-00001, two upserts of
4053+
// the same id → CASE-00002 then CASE-00003, one row throughout). The
4054+
// exclusion is unconditional — an explicit payload value does not
4055+
// renumber on merge either; `update()` is the deliberate renumbering
4056+
// path. The reservation itself still happens on the merge path (a gap,
4057+
// not a rewrite) — that pre-burn half is #6943's reseed family, not
4058+
// this exclusion's. Everything else (incl. `updated_at`) merges as
4059+
// before, so an upsert that updates a row still advances `updated_at`.
4060+
const insertOnlyColumns = this.insertOnlyUpsertColumns(object);
4061+
const mergeColumns = Object.keys(formatted).filter((c) => !insertOnlyColumns.has(c));
40284062
const insertion = builder.insert(formatted).onConflict(mergeKeys);
40294063

40304064
try {

packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-autonumber-batch-resync.test.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,4 +76,16 @@ describe('[#6943] driver-sqlite-wasm inherits the batch/upsert autonumber re-see
7676
const upserted = await driver.upsert('crm_case', { organization_id: 'orgA', title: 'u1' });
7777
expect(upserted.case_number).toBe('CASE-00031');
7878
});
79+
80+
it('[#7011] a merge-path upsert keeps the existing autonumber on this transport too', async () => {
81+
// Inherited from `SqlDriver.upsert` (no override) — pinned here because the
82+
// exclusion is applied to the ON CONFLICT merge column list, and this
83+
// driver swaps the transport under that statement.
84+
const created = await driver.create('crm_case', { organization_id: 'orgA', title: 'original' });
85+
expect(created.case_number).toBe('CASE-00001');
86+
87+
const merged = await driver.upsert('crm_case', { id: created.id, organization_id: 'orgA', title: 'edited' });
88+
expect(merged.case_number).toBe('CASE-00001');
89+
expect(merged.title).toBe('edited');
90+
});
7991
});

packages/drivers/driver-turso/src/turso-autonumber-batch-resync.test.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,27 @@ describe('[#6943] TursoDriver batch/upsert autonumber re-seed', () => {
8989
expect(upserted.case_number).toBe('CASE-00031');
9090
});
9191

92+
it('LOCAL: [#7011] a merge-path upsert keeps the existing autonumber', async () => {
93+
// Turso OVERRIDES `upsert` (remote → RemoteTransport, else `super`), so
94+
// "the base class was fixed" is not on its own an answer about this face —
95+
// the local route through `super.upsert` is pinned here.
96+
const created = await driver.create(
97+
'crm_case',
98+
{ organization_id: 'orgA', title: 'original' },
99+
{ bypassTenantAudit: true },
100+
);
101+
expect(created.case_number).toBe('CASE-00001');
102+
103+
const merged = await driver.upsert(
104+
'crm_case',
105+
{ id: created.id, organization_id: 'orgA', title: 'edited' },
106+
undefined,
107+
{ bypassTenantAudit: true } as any,
108+
);
109+
expect(merged.case_number).toBe('CASE-00001');
110+
expect(merged.title).toBe('edited');
111+
});
112+
92113
it('REMOTE: the transport that bypasses this path has no autonumber machinery to re-seed', async () => {
93114
const remote = new TursoDriver({ url: 'libsql://example.turso.io', authToken: 'placeholder' });
94115
expect(remote.transportMode).toBe('remote');

0 commit comments

Comments
 (0)