Skip to content

Commit 88f22e0

Browse files
fix(driver-sql): upsert with nothing left to merge is a plain INSERT, not merge-ALL (#26015)
<!-- claude-projects-attribution: {"github_login":"os-project-manager"} --> _Requested by **Leehom** · [project thread](https://claude.ai/code/project/chan_01TenzsRopZj4jMbvjX8uz1X?thread=cmsg_01TenzsRopZj4jMbvjX8uz1XENoZnG15Ne6zvnM6q3Fq5d)_ Fixes #8740 Clause-②: no (narrowing) Metadata-protocol: none ## What changed `SqlDriver.upsert` merges every payload column that is not insert-only, or, when there is none, the conflict-target columns the payload carries (a no-op). When the payload carried neither, the statement fell back to knex's bare `merge()`, which compiles to merge-ALL on every dialect (`do update set` / `on duplicate key update` over every inserted column, insert-only ones included). That fallback is removed. With nothing to merge, the statement is a plain `INSERT` with no conflict clause, so a collision surfaces as the driver's own unique-violation error and the stored row is left as it was. It is not `insert ignore` / `do nothing`, which would hide the collision. The reachable no-op merge on the conflict-target columns is unchanged. - `packages/drivers/driver-sql/src/upsert-conflict-clause.ts` (new, not exported from the package): `withUpsertConflictClause(insertion, mergeKeys, columnsToMerge)` returns the plain insertion when `columnsToMerge` is empty, else `onConflict(mergeKeys).merge(columnsToMerge)`. Kept free of the driver so its SQL is pinned per dialect with no server. - `sql-driver.ts`: the upsert statement goes through it; the in-statement merge predicate is applied only when the statement merges. - `.changeset/8740-upsert-nothing-to-merge.md`: `patch`, BREAKING banner (a served-set narrowing), ADR-0087 `not-required (no-migration-prescription)`. ## Tests New `src/sql-driver-upsert-empty-merge-set.test.ts`: - compiled SQL with nothing to merge, via `toSQL()` with no server, exact strings for `better-sqlite3`, `pg` and `mysql2`: no `on conflict`, `do update set`, `on duplicate key update` or `insert ignore`; plus the contrast (something to merge keeps the clause and merges only the named column); - per dialect cell (SQLite always; PostgreSQL and MySQL where `OS_TEST_POSTGRES_URL` / `OS_TEST_MYSQL_URL` are provisioned): a collision on a conflict-target column the payload omits (non-null default) is refused with a unique violation and rewrites nothing; a primary-key collision whose payload carries only insert-only columns is refused and rewrites nothing; controls: a payload that collides with nothing still inserts and answers the row, and a payload carrying its conflict target still upserts onto the stored row. The refusal is asserted by `isUniqueViolationError` AND the dialect's native code (`SQLITE_CONSTRAINT_*`, `23505`, `ER_DUP_ENTRY`). Red first, on the unfixed driver (commit 19a401b): 4 failed / 12 passed, the non-null-default collision red on all three cells, the primary-key collision red on the MySQL-dialect cell. Green after the fix (commit fc2ee27), with the four neighbouring upsert suites: 5 files, 119 tests passed, all three cells live. Local cells: SQLite in-process; PostgreSQL 16.15 local; the MySQL cell against MariaDB 10.11.14 (no MySQL 8 server in the container). CI's `Temporal Conformance (live PG + MySQL)` job runs this package's whole suite, so the new file's MySQL cell runs there on MySQL 8. ## Gates - `pnpm --filter @objectstack/driver-sql test`: 236 files passed, 12 skipped; 3795 tests passed (SQLite cells; live cells via CI). `typecheck`: exit 0. - With local live URLs the package run shows 49 failures in 24 files, all live PG/MariaDB cells and none in an upsert file; the same 24 files on the base commit `05f7758f05` show the same 49 failures (local MariaDB / PG environment, not this diff). - `eslint --no-inline-config` on the three touched source files: exit 0. - `dispatch-gates --commands`: 71 derived, 70 run, every exit 0; 1 NOT MEASURED: `check:dual-build-cjs-loads` (exit 3, needs the whole tree built; CI runs it). `--ran` reconciliation: 71 accounted, 0 unrun. ## Acceptance notes - An unbacked conflict target is still refused VALIDATION_ERROR / 400 on the nothing-to-merge path on every dialect, checked against the table keys before the plain INSERT (pinned per dialect cell). - **`isUniqueViolationError` reads a non-unique error as a unique violation when the error message echoes an `on duplicate key update` statement** (driver-level repro: a MySQL `ER_TRUNCATED_WRONG_VALUE` whose message carries the statement answers `true`; the same code without the statement text answers `false`). Not fixed here; the new tests pin the native code alongside it for that reason. No public door measured. - `driver-turso`'s remote transport is a separate statement builder (`DO NOTHING` on an empty merge set, SQLite dialect only) and is untouched; its local face inherits this fix through `super.upsert`. - Full dogfood battery (owed by the `(narrowing)` arm) is run on the final head and reported on #8740. --- _Generated by [Claude Code](https://claude.ai/code/session_01T6USnvRkjp4BPytcNttAuF)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent aa2f8e3 commit 88f22e0

4 files changed

Lines changed: 389 additions & 26 deletions

File tree

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
---
2+
'@objectstack/driver-sql': patch
3+
---
4+
5+
fix(driver-sql): an `upsert` with nothing left to merge is a plain INSERT, so a collision surfaces as the driver's unique violation instead of merging every column
6+
7+
Clause-②: no (narrowing)
8+
Metadata-protocol: none
9+
10+
<!-- adr-0087: not-required (no-migration-prescription) No key of any metadata schema is removed, renamed or re-shaped, so there is nothing for `objectstack migrate meta` to rewrite and no tombstone. What changes is the statement `SqlDriver.upsert` sends when its merge set is empty. -->
11+
12+
**BREAKING** (a served-set narrowing) for an `upsert` whose payload carries only insert-only columns (`id`, `created_at`, `auto_number` columns) and none of its conflict-target columns, when that row collides with a stored row. Such a call used to merge EVERY payload column into the stored row, insert-only columns included. It now raises the driver's unique-violation error and leaves the stored row as it was.
13+
14+
`SqlDriver.upsert` merges every payload column that is not insert-only, or, when there is none, the conflict-target columns the payload carries (a no-op). When the payload carries neither, the statement used to fall back to knex's bare `merge()`, which compiles to merge-ALL on every dialect (`do update set` / `on duplicate key update` over every inserted column). That fallback is removed: with nothing to merge, the statement is a plain `INSERT` with no conflict clause. It is not `insert ignore` / `do nothing` either, which would hide a collision.
15+
16+
Unchanged: a payload with a column to merge, or one that carries its conflict target, upserts exactly as before, and a payload that collides with nothing is inserted and answered as before.
17+
18+
A conflict target that no PRIMARY KEY or UNIQUE index backs is still refused on that path, with the same `VALIDATION_ERROR` / 400 every dialect answers when there is something to merge. A plain INSERT asks the database nothing about the target, so the driver checks it against the table's keys before the statement, on every dialect.
19+
20+
**What changes for you.** If you upsert a payload made only of insert-only columns under a business conflict key the payload omits, catch the unique-violation error on a collision, or use `update()` to change a stored row.
Lines changed: 261 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,261 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* An upsert with nothing left to merge is a plain INSERT.
5+
*
6+
* `upsert`'s merge leg writes every payload column that is not insert-only
7+
* (`insertOnlyUpsertColumns`), or, when that set is empty, the conflict-target
8+
* columns the payload carries (a no-op: a conflict means they matched). When
9+
* both are empty the statement used to fall back to knex's bare `merge()`,
10+
* which compiles to merge-ALL on every dialect: `do update set` /
11+
* `on duplicate key update` over EVERY inserted column, insert-only ones
12+
* included. That fallback is gone. With nothing to merge the statement carries
13+
* no conflict clause, so a collision surfaces as the driver's own
14+
* unique-violation error and the stored row is left as it was.
15+
*
16+
* Pinned here:
17+
* - the compiled SQL per dialect, with no server (`toSQL()`);
18+
* - on every provisioned cell: a collision on a conflict-target column that
19+
* the payload omits (the column has a non-null default) is refused and
20+
* rewrites nothing; a collision on the primary key with only insert-only
21+
* columns in the payload is refused and rewrites nothing;
22+
* - a conflict target that no PRIMARY KEY or UNIQUE index backs is still
23+
* refused (`VALIDATION_ERROR` / 400) on that path, and nothing is written —
24+
* the same answer every dialect gives when there IS something to merge;
25+
* - controls: a payload that collides with nothing still inserts and answers
26+
* the row, and a payload that carries its conflict target still upserts
27+
* onto the stored row.
28+
*
29+
* The tables are created by hand and registered DDL-free, so the driver has not
30+
* observed an `updated_at` column on them and the payloads below reach the
31+
* empty merge set. Live cells run where their URL is provisioned; elsewhere they
32+
* are declared un-run, never silently passed.
33+
*/
34+
35+
import { describe, it, expect, beforeAll, beforeEach, afterAll } from 'vitest';
36+
import knex from 'knex';
37+
import { isUniqueViolationError } from '@objectstack/types';
38+
import { SqlDriver } from '../src/index.js';
39+
import { withUpsertConflictClause } from './upsert-conflict-clause.js';
40+
import { DIALECT_CELLS, declareDialectCell, type DialectCell } from './live-dialect-matrix.testkit.js';
41+
42+
// ─────────────────────────────────────────────────────────────────────────
43+
// The compiled statement, per dialect — no server
44+
// ─────────────────────────────────────────────────────────────────────────
45+
46+
describe('SqlDriver.upsert — the statement compiled when nothing is left to merge', () => {
47+
const compile = (client: string, columnsToMerge: string[]): string => {
48+
const k = knex({ client, connection: {} } as any);
49+
try {
50+
return withUpsertConflictClause(k('os8740_t').insert({ id: '1', created_at: 'x' }), ['email'], columnsToMerge)
51+
.statement.toSQL().sql;
52+
} finally {
53+
void k.destroy();
54+
}
55+
};
56+
57+
const EXPECTED_PLAIN: Record<string, string> = {
58+
'better-sqlite3': 'insert into `os8740_t` (`created_at`, `id`) values (?, ?)',
59+
pg: 'insert into "os8740_t" ("created_at", "id") values (?, ?)',
60+
mysql2: 'insert into `os8740_t` (`created_at`, `id`) values (?, ?)',
61+
};
62+
63+
for (const [client, expected] of Object.entries(EXPECTED_PLAIN)) {
64+
it(`compiles a plain INSERT with no conflict clause (${client})`, () => {
65+
const sql = compile(client, []);
66+
expect(sql).not.toMatch(/do update set/i);
67+
expect(sql).not.toMatch(/on duplicate key update/i);
68+
expect(sql).not.toMatch(/on conflict/i);
69+
expect(sql).not.toMatch(/insert ignore/i);
70+
expect(sql).toBe(expected);
71+
});
72+
}
73+
74+
// The contrast: with a column to merge, the same helper still emits the
75+
// conflict clause and merges exactly that column, never every column.
76+
it('keeps the conflict clause, merging only the named columns, when there is something to merge', () => {
77+
expect(compile('better-sqlite3', ['title'])).toBe(
78+
'insert into `os8740_t` (`created_at`, `id`) values (?, ?) on conflict (`email`) do update set `title` = excluded.`title`',
79+
);
80+
expect(compile('pg', ['title'])).toBe(
81+
'insert into "os8740_t" ("created_at", "id") values (?, ?) on conflict ("email") do update set "title" = excluded."title"',
82+
);
83+
expect(compile('mysql2', ['title'])).toBe(
84+
'insert into `os8740_t` (`created_at`, `id`) values (?, ?) on duplicate key update `title` = values(`title`)',
85+
);
86+
});
87+
});
88+
89+
// ─────────────────────────────────────────────────────────────────────────
90+
// Behaviour, per provisioned dialect
91+
// ─────────────────────────────────────────────────────────────────────────
92+
93+
/** A conflict-target column with a non-null default, so an omitted value still collides. */
94+
const DEFAULTED = {
95+
name: 'os8740_defaulted',
96+
fields: { email: { type: 'string' }, title: { type: 'string' } },
97+
} as any;
98+
99+
/** A business key the payloads below omit, beside insert-only `created_at`. */
100+
const STAMPED = {
101+
name: 'os8740_stamped',
102+
fields: { email: { type: 'string' }, title: { type: 'string' } },
103+
} as any;
104+
105+
/** No index on `email` at all: a conflict target nothing backs. */
106+
const UNBACKED = {
107+
name: 'os8740_unbacked',
108+
fields: { email: { type: 'string' }, title: { type: 'string' } },
109+
} as any;
110+
111+
const KEPT_ID = 'os8740_kept_row';
112+
const KEPT_CREATED = '2020-01-01 00:00:00';
113+
114+
/** Each dialect's native unique-violation code, as its client library reports it. */
115+
const UNIQUE_VIOLATION_CODES: Record<string, string[]> = {
116+
sqlite: ['SQLITE_CONSTRAINT_PRIMARYKEY', 'SQLITE_CONSTRAINT_UNIQUE'],
117+
pg: ['23505'],
118+
mysql: ['ER_DUP_ENTRY'],
119+
};
120+
121+
for (const cell of DIALECT_CELLS) {
122+
declareDialectCell(cell, 'upsert with nothing left to merge', declareEmptyMergeSetPins);
123+
}
124+
125+
function declareEmptyMergeSetPins(cell: DialectCell): void {
126+
describe(`SqlDriver.upsert — nothing left to merge (${cell.label})`, () => {
127+
let driver: SqlDriver;
128+
let knexInstance: any;
129+
130+
const rows = async (table: string): Promise<Array<Record<string, unknown>>> => {
131+
const found = await knexInstance(table).select('id', 'email', 'title').orderBy('id');
132+
return found.map((r: Record<string, unknown>) => ({ ...r }));
133+
};
134+
135+
const createdAtOf = async (id: string): Promise<string> => {
136+
const row = await knexInstance(STAMPED.name).where('id', id).first();
137+
const value = row?.created_at;
138+
return value instanceof Date ? value.toISOString() : String(value);
139+
};
140+
141+
/**
142+
* The dialect's own unique-violation code, beside the shared predicate: the
143+
* predicate matches its vocabulary anywhere in the message, and a statement
144+
* echoed into an unrelated error's message can carry that vocabulary.
145+
*/
146+
const expectUniqueViolation = (err: unknown): void => {
147+
expect(err, 'the collision was merged instead of refused').not.toBeNull();
148+
const { code, message } = err as { code?: string; message?: string };
149+
expect(isUniqueViolationError(err), `not a unique violation: ${String(message)}`).toBe(true);
150+
expect(UNIQUE_VIOLATION_CODES[cell.id], `not a unique violation: ${String(message)}`).toContain(code);
151+
};
152+
153+
const captureError = async (run: () => Promise<unknown>): Promise<unknown> => {
154+
try {
155+
await run();
156+
return null;
157+
} catch (e) {
158+
return e;
159+
}
160+
};
161+
162+
beforeAll(async () => {
163+
driver = new SqlDriver(cell.config());
164+
knexInstance = (driver as any).knex;
165+
await knexInstance.schema.dropTableIfExists(DEFAULTED.name);
166+
await knexInstance.schema.dropTableIfExists(STAMPED.name);
167+
await knexInstance.schema.dropTableIfExists(UNBACKED.name);
168+
await knexInstance.schema.createTable(DEFAULTED.name, (t: any) => {
169+
t.string('id', 64).primary();
170+
t.string('email', 191).notNullable().defaultTo('unset').unique();
171+
t.string('title', 191).nullable();
172+
});
173+
await knexInstance.schema.createTable(STAMPED.name, (t: any) => {
174+
t.string('id', 64).primary();
175+
t.timestamp('created_at').nullable();
176+
t.string('email', 191).nullable().unique();
177+
t.string('title', 191).nullable();
178+
});
179+
driver.registerExternalObject({ ...DEFAULTED, external: { remoteName: DEFAULTED.name } });
180+
await knexInstance.schema.createTable(UNBACKED.name, (t: any) => {
181+
t.string('id', 64).primary();
182+
t.string('email', 191).nullable();
183+
t.string('title', 191).nullable();
184+
});
185+
driver.registerExternalObject({ ...STAMPED, external: { remoteName: STAMPED.name } });
186+
driver.registerExternalObject({ ...UNBACKED, external: { remoteName: UNBACKED.name } });
187+
});
188+
189+
afterAll(async () => {
190+
await knexInstance?.schema.dropTableIfExists(DEFAULTED.name).catch(() => {});
191+
await knexInstance?.schema.dropTableIfExists(STAMPED.name).catch(() => {});
192+
await knexInstance?.schema.dropTableIfExists(UNBACKED.name).catch(() => {});
193+
await driver?.disconnect?.();
194+
});
195+
196+
beforeEach(async () => {
197+
await knexInstance(DEFAULTED.name).delete();
198+
await knexInstance(STAMPED.name).delete();
199+
await knexInstance(UNBACKED.name).delete();
200+
await knexInstance(DEFAULTED.name).insert({ id: KEPT_ID, email: 'unset', title: 'kept' });
201+
await knexInstance(STAMPED.name).insert({
202+
id: KEPT_ID,
203+
created_at: new Date(`${KEPT_CREATED.replace(' ', 'T')}Z`),
204+
email: 'kept@os8740.test',
205+
title: 'kept',
206+
});
207+
});
208+
209+
it('refuses a collision on an omitted conflict-target column with a unique violation, and rewrites no column', async () => {
210+
const before = await rows(DEFAULTED.name);
211+
212+
const err = await captureError(() => driver.upsert(DEFAULTED.name, { id: 'os8740_second_row' }, ['email']));
213+
214+
expectUniqueViolation(err);
215+
expect(await rows(DEFAULTED.name), 'the stored row was rewritten, or a row was added').toEqual(before);
216+
});
217+
218+
it('refuses a primary-key collision whose payload carries only insert-only columns, and rewrites no column', async () => {
219+
const before = await rows(STAMPED.name);
220+
const createdBefore = await createdAtOf(KEPT_ID);
221+
222+
const err = await captureError(() =>
223+
driver.upsert(STAMPED.name, { id: KEPT_ID, created_at: '2030-06-01 00:00:00' }, ['email']),
224+
);
225+
226+
expectUniqueViolation(err);
227+
expect(await rows(STAMPED.name)).toEqual(before);
228+
expect(await createdAtOf(KEPT_ID), 'an insert-only column was rewritten').toBe(createdBefore);
229+
});
230+
231+
it('refuses a conflict target that no unique index backs, and writes nothing', async () => {
232+
const err = (await captureError(() => driver.upsert(UNBACKED.name, { id: 'os8740_unbacked_row' }, ['email']))) as {
233+
code?: string;
234+
status?: number;
235+
} | null;
236+
237+
expect(err, 'the unbacked conflict target was accepted').not.toBeNull();
238+
expect(err!.code).toBe('VALIDATION_ERROR');
239+
expect(err!.status).toBe(400);
240+
expect(await rows(UNBACKED.name), 'a row was written').toEqual([]);
241+
});
242+
243+
it('still inserts, and answers the row, when the payload collides with nothing', async () => {
244+
const written = await driver.upsert(STAMPED.name, { id: 'os8740_fresh_row' }, ['email']);
245+
246+
expect(written.id).toBe('os8740_fresh_row');
247+
expect((await rows(STAMPED.name)).map((r) => r.id).sort()).toEqual(['os8740_fresh_row', KEPT_ID].sort());
248+
});
249+
250+
it('still upserts onto the stored row when the payload carries its conflict target', async () => {
251+
const before = await rows(STAMPED.name);
252+
const createdBefore = await createdAtOf(KEPT_ID);
253+
254+
const written = await driver.upsert(STAMPED.name, { id: KEPT_ID }, ['id']);
255+
256+
expect(written.id).toBe(KEPT_ID);
257+
expect(await rows(STAMPED.name)).toEqual(before);
258+
expect(await createdAtOf(KEPT_ID)).toBe(createdBefore);
259+
});
260+
});
261+
}

0 commit comments

Comments
 (0)