Skip to content

Commit be5a83c

Browse files
fix(driver-sql): create and bulkCreate answer the stored row on MySQL (#21227) (#21239)
Fixes #21227 Clause-②: no ## What changes `SqlDriver.create` and `SqlDriver.bulkCreate` ran `builder.insert(...).returning('*')` and answered what the statement answered. MySQL has no `RETURNING`: knex's MySQL compiler drops the clause (it logs `.returning() is not supported by mysql`) and answers `[insertId]`. That is ONE element whatever the row count, and `0` for this driver's string primary key. Both doors now answer the stored row on every dialect: - **SQLite and PostgreSQL families**: unchanged. They answer from `RETURNING` in one statement. - **MySQL family, and any client the driver recognises as neither family**: the INSERT is issued without `.returning()`, and the rows are read back by the ids that were written. That is one `SELECT` per `create`, and one per `bulkCreate` batch. The switch is one protected capability getter, `insertReturnsStoredRows` (`isSqlite || isPostgres`), next to `isMysql`. One private helper, `readBackInsertedRows`, serves both doors. No caller was patched: the auth adapter, the engine and the protocol are untouched. ### Measured before the fix (live MySQL 8.0.46, `origin/main` `62b90d74`, driver called directly) | call | answered | stored | |:--|:--|:--| | `create`, generated id | `0` | the row | | `create`, supplied id | `0` | the row | | `bulkCreate`, 3 rows | `[0]` (length 1) | all 3 rows | | `bulkCreate`, 1 row | `[0]` | the row | SQLite and live PostgreSQL 16.14 answered the full stored rows in the same probe, including `done: false`, which only the column DEFAULT supplies. That is the control. ### The doors, before and after (`pnpm dev:crm -- --fresh --database mysql://...` on that MySQL 8.0.46) | door | at `62b90d74` | with this change | |:--|:--|:--| | `POST /api/v1/auth/sign-up/email`, first user (`--no-seed-admin`) | `400 FAILED_TO_CREATE_USER`; `sys_user` row stored, no `sys_account` row | `200`; user, `credential` account and session stored | | `POST /api/v1/auth/sign-in/email`, same credentials | `401 INVALID_EMAIL_OR_PASSWORD` | `200` | | `--seed-admin` at boot | `dev admin seed skipped: Failed to create user`; orphaned `sys_user` | seeded; admin sign-in `200` | | `POST /api/v1/data/crm_account` as the seeded admin | not reachable (no session could exist) | `201`, with the full record including the stamped `organization_id` | | boot: `curated capability ... has no platform row and could not be seeded` warnings | 9 | 0 | ## Decisions the card left open - **H3: read back only where `RETURNING` does not answer the stored row.** Reading back on every dialect would add one round trip to every `create` on SQLite and PostgreSQL, where `RETURNING` already answers the stored row (measured above). It would also move the control cells onto new code. Cost on MySQL: +1 `SELECT` per statement. Cost on SQLite and PostgreSQL: 0. The pin file counts the statements on every cell. The getter is a positive list on purpose. A client the driver does not recognise (a Client constructor, `redshift`, `mariadb`) reads back, which is correct on every dialect. `RETURNING` is the shortcut that only a dialect known to answer the stored row gets. `driver-sqlite-wasm` overrides `isSqlite` to `true`, so it keeps `RETURNING`. - **H2: one helper for `create` and `bulkCreate` only. `update` and `upsert` are byte-unchanged.** The four read-backs answer different things: - `update` reads by id under the caller's scope and answers `null` on a miss, which its contract allows. - `upsert` reads by the conflict-key values it matched on and falls back to the payload. - `create` must answer a row, and is keyed on ids it wrote. Sharing one helper would change one of those answers. It would also touch the `upsert` region, which the card fences off. - **H4: the read key is the written id, and that id always exists.** `create` and `bulkCreate` give every row its id before the statement is built: the caller's `id`, else `_id`, else a minted nanoid. The managed `id` column is `varchar(255)` PRIMARY KEY with no AUTO_INCREMENT, so no insert id is ever read. That is the only kind of key this path produces. - The column goes through `remoteColumn`, so an external `columnMap` that renames `id` is read by its physical column. - The table is the write target, a rotation shard included. - The tenant scope goes through `applyTenantScope`, scoped to the tenant(s) the rows were WRITTEN under (as `assertMergeLandedOnSuppliedIdentity` scopes its probe). For a batch that is the union through `tenantIds`. So an admin write that names another tenant in the row data is answered rather than missed. - The ids are this call's own and `id` is the PRIMARY KEY, so the read cannot answer another organization's row. - The read uses the caller's transaction when there is one. - **A written row that is gone before the read-back** (a concurrent delete, or a trigger) is refused with `DATABASE_ERROR` / 500. It is not answered with the payload, and the insert is not re-issued. The read-back runs outside the insert's `try`, so a read fault can never reach the autonumber collision retry. ## One conclusion per face of the invariant (`IDataDriver.create` answers the inserted record) 1. **`driver-sql`**: changed for the MySQL family. SQLite and PostgreSQL are already conformant and unchanged (evidence: the pin's control cells, green before and after). 2. **`driver-sqlite-wasm` and LOCAL-mode `driver-turso`**: inherit `SqlDriver.create` / `bulkCreate` and stay on `RETURNING`. Wasm overrides `isSqlite` to `true`; Turso local uses `better-sqlite3`. Their suites are green: wasm 36 files / 675 tests, turso 86 files / 2313 passed, 33 skipped. 3. **REMOTE-mode `driver-turso`**: already conformant. `RemoteTransport.create` issues its INSERT and then `SELECT * ... WHERE "id" = ?` and answers that row (`remote-transport.ts`). Its `bulkCreate` loops the driver's own `create`. 4. **`driver-memory`**: already conformant. `create` pushes the built record and answers a copy of it, and `bulkCreate` answers the pending records it pushed. 5. **`driver-mongodb`**: already conformant. `create` answers the document it inserted (minus `_id`), and `bulkCreate` answers the inserted docs in order. ## Pins `packages/drivers/driver-sql/src/sql-driver-21227-create-answers-stored-row.test.ts`, through `declareDialectCell`: SQLite always, and live PostgreSQL and MySQL where provisioned. The `Temporal Conformance (live PG + MySQL)` job runs them on both. There is also one cell that always runs: SQLite with the read-back path forced. It puts the read-back's ordering, tenant scope, transaction and refusal into every CI run, not only the job with a MySQL server. Each test pins one behaviour: - `create` with a generated id and with a supplied id; - `bulkCreate` of 3 rows, which answers 3 rows in written order from ONE insert, plus ONE read on the read-back path; - `bulkCreate` of 1 row; - a tenanted create; - an admin write naming another tenant (single and batch); - create and bulkCreate inside a rolled-back caller transaction; - the vanished-row refusal (forced cell only), asserting `code` and `status` and that the insert is not re-issued; - a per-cell check that the cell measures the path it claims to. Every answer is compared with the driver's own `findOne` and must carry the DEFAULT-only `done: false`. Reverse verification, both legs run with live PostgreSQL 16.14 and MySQL 8.0.46: - **Fix reverted** (`sql-driver.ts` at `62b90d74`, worktree only, restore by trap with a hash check): `13 failed | 20 passed`. - All 8 MySQL tests are red: `expected +0 to deeply equal {...}`, and `expected [ +0 ] to have a length of 3 but got 1`. - 3 forced-cell path tests are red. - The SQLite and PostgreSQL behaviour tests stay green (the control). Their path checks are red only because the getter does not exist before the fix. - **Fix in place** (HEAD `f42d0354c3`): `33 passed`. **Sign-up door pin: not added, declared.** No live-dialect harness runs the real auth stack against a datasource URL. The plugin-auth real-engine harness (`signup-existing-address-refusal.test.ts` and its siblings) hard-codes better-sqlite3. A MySQL door pin there would need a per-file database isolation helper in plugin-auth and a new CI step in the live job. Without that step, the pin is a named skip that never runs in CI. The door is measured above instead. The options are in the report for the PM. ## Verification (HEAD `ee7c024b92`, after merging `origin/main` with #21225 in it) - `pnpm --filter @objectstack/driver-sql exec vitest run --maxWorkers=2` with `OS_TEST_POSTGRES_URL` and `OS_TEST_MYSQL_URL` (PG 16.14, MySQL 8.0.46), `OS_EXPECT_LIVE_DIALECT_MATRIX=1`, `TZ=America/New_York`: **223 files passed, 5369 passed, 1 skipped**. The skip is pre-existing, in `schema-drift.base-type-mismatch.test.ts`. - `pnpm --filter @objectstack/driver-sqlite-wasm test` (36 / 675 passed) and `pnpm --filter @objectstack/driver-turso test` (86 files, 2313 passed, 33 skipped): exit 0. - `typecheck` for driver-sql, driver-sqlite-wasm and driver-turso: exit 0. `tsc --listFiles` includes the new test file. - `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands` derived 63 commands at `ee7c024b92`, and all 63 exited 0. `--ran` reconciliation: `63 derived, 63 run, 0 NOT-MEASURED, 0 UNRUN`, all with recorded exit codes. This includes `check:tenant-chokepoint` ("every read builder routes through applyTenantScope()"). - `pnpm check:driver-conformance`: before the first edit (`62b90d74`) `50 covered cell(s), 0 in the DEBT ledger, 0 exempt`, dialect axis `8 conformance suite(s) ... 0 in the DIALECT ledger`; after the last commit (`ee7c024b92`), identical. - **Lint, a declared narrowing.** `eslint --no-inline-config --format json` was run on the two touched TypeScript files. - Population: eslint's own `--print-config` resolves rules for both (6 and 5 rules; neither file is ignored). - Count: 2 files, 0 errors, 0 warnings. - Invariance: `eslint.config.mjs` enables no type-aware linting (`parserOptions.project` and `projectService` are null for both files), so this diff cannot move a verdict on an untouched file. - The repo-wide `pnpm lint` is left to CI. ## Acceptance notes - `sql-driver-21163-autonumber-prefix-like-escape.test.ts`'s header says its cases read the stored row "Not from `create`'s return value: on MySQL that is not the row". After this change that sentence is stale. It is a test comment, not published; it is left for whoever next edits that file. - In the same MySQL boot, service-package's raw `CREATE TABLE IF NOT EXISTS sys_packages (... created_at TEXT DEFAULT CURRENT_TIMESTAMP ...)` is refused with `ER_INVALID_DEFAULT` (`Invalid default value for 'created_at'`), and later `SELECT * FROM sys_packages` reads answer `ER_NO_SUCH_TABLE`. No door was measured for it, so it is noted here and not filed. - The `sys_activity` boot failure is reported to the PM with a measured door, for the seat to file. It is not touched here. --- _Generated by [Claude Code](https://claude.ai/code/session_017xfMoEjKUuSh2xYB8sCozp)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent ef96c9e commit be5a83c

3 files changed

Lines changed: 473 additions & 10 deletions

File tree

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
'@objectstack/driver-sql': patch
3+
---
4+
5+
On MySQL, `SqlDriver.create` and `SqlDriver.bulkCreate` now answer the rows they stored (#21227).
6+
7+
Clause-②: no
8+
9+
MySQL has no `INSERT … RETURNING`. knex drops the clause on the MySQL family and answers the insert id instead, so `create` answered `0` and `bulkCreate` answered a one-element array whatever the row count, although every row was stored. Callers that use the answer failed one layer up: on a MySQL datasource, sign-up answered `400 FAILED_TO_CREATE_USER` with the user stored and no account, the dev admin seed failed, and a multi-row `bulkCreate` through the engine was refused after its rows had landed.
10+
11+
On the MySQL family both doors now read the rows back by the ids they wrote, under the tenant the rows were written with, inside the caller's transaction when there is one: one extra `SELECT` per `create` and per `bulkCreate` batch. SQLite and PostgreSQL still answer from `RETURNING`, with no extra statement and no change in what they answer. If a written row is gone before it can be read back (deleted in between by another statement or a trigger), the call throws `DATABASE_ERROR` (500) and does not retry the insert.
12+
13+
Nothing to change in a project. Code that read the record from the result now gets it on MySQL as on the other dialects.
Lines changed: 257 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,257 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#21227] `create` and `bulkCreate` answer the rows they STORED, on every
5+
* dialect: one record per written row, in the written order.
6+
*
7+
* # The defect
8+
*
9+
* Both doors ran `builder.insert(...).returning('*')` and answered what the
10+
* statement answered. MySQL has no `RETURNING`: knex's MySQL compiler drops the
11+
* clause with a `.returning() is not supported by mysql` warning and answers
12+
* `[insertId]`, which is `0` for this driver's string primary key and ONE
13+
* element whatever the row count. Measured on live MySQL 8.0.46 at `62b90d74`,
14+
* every row stored correctly each time:
15+
*
16+
* | call | answered |
17+
* |:--|:--|
18+
* | `create` (generated id) | `0` |
19+
* | `create` (supplied id) | `0` |
20+
* | `bulkCreate`, 3 rows | `[0]` (length 1) |
21+
* | `bulkCreate`, 1 row | `[0]` |
22+
*
23+
* One layer up, the auth adapter answers what `create` answers, so sign-up on
24+
* MySQL answered `400 FAILED_TO_CREATE_USER` with the user row stored and no
25+
* account row; and the engine's one-result-per-row guard refuses a multi-row
26+
* `bulkCreate` AFTER every row has landed.
27+
*
28+
* # What is asserted, per cell
29+
*
30+
* The answer is compared with the row read back through the driver's own
31+
* `findOne`, and it must carry `done: false`, a value only the column's
32+
* DEFAULT supplies (the payload never names `done`): an answer that echoed the
33+
* payload fails here, and so does `0`.
34+
*
35+
* Cells: SQLite always; live PostgreSQL and MySQL where provisioned (the
36+
* `Temporal Conformance (live PG + MySQL)` job runs this package against both)
37+
* and declared un-run otherwise. SQLite and PostgreSQL answer from `RETURNING`,
38+
* unchanged by the fix, and are the control: they passed before it and must
39+
* pass after it, in ONE statement each. MySQL reads back, in two.
40+
*
41+
* One more cell, always run: SQLite with the read-back path forced
42+
* (`insertReturnsStoredRows` overridden to `false`), so the read-back's own
43+
* logic (written order, the written tenant scope, the caller's transaction,
44+
* the refusal when a written row is gone) is measured on every CI run, not
45+
* only on the job that has a MySQL server.
46+
*/
47+
48+
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
49+
import { SqlDriver, type SqlDriverConfig } from '../src/index.js';
50+
import { DIALECT_CELLS, declareDialectCell, type DialectCell } from './live-dialect-matrix.testkit.js';
51+
52+
const TABLE = 'os21227_stored_row';
53+
54+
const OBJECT = {
55+
name: TABLE,
56+
fields: {
57+
organization_id: { type: 'string' },
58+
title: { type: 'string' },
59+
qty: { type: 'number' },
60+
// Filled by the column DEFAULT, never by a payload in this file: the field
61+
// that tells a stored row from an echo of what was sent.
62+
done: { type: 'boolean', defaultValue: false },
63+
meta: { type: 'json' },
64+
},
65+
} as any;
66+
67+
/** SQLite, with the read-back path the MySQL family takes. */
68+
class ReadBackPathSqlDriver extends SqlDriver {
69+
protected override get insertReturnsStoredRows(): boolean {
70+
return false;
71+
}
72+
}
73+
74+
interface Face {
75+
label: string;
76+
make(): SqlDriver;
77+
/** Whether this face answers by reading back (two statements) rather than from RETURNING. */
78+
readsBack: boolean;
79+
/** Whether a trigger can delete the row its own INSERT wrote (SQLite only). */
80+
sqliteTriggers: boolean;
81+
}
82+
83+
function suite(face: Face) {
84+
describe(`sql-driver — create / bulkCreate answer the stored row (${face.label}) [#21227]`, () => {
85+
let driver: SqlDriver;
86+
const knex = () => driver.getKnex();
87+
88+
/** Statements this test issued against the fixture table, in order. */
89+
let statements: string[] = [];
90+
const record = (q: { sql?: string }) => {
91+
const sql = String(q?.sql ?? '');
92+
if (sql.includes(TABLE)) statements.push(sql.trim().split(/\s+/)[0].toLowerCase());
93+
};
94+
95+
const stored = async (id: unknown) => {
96+
const row = await driver.findOne(TABLE, { where: { id } });
97+
expect(row, `row ${String(id)} was not stored`).not.toBeNull();
98+
return row!;
99+
};
100+
101+
beforeEach(async () => {
102+
driver = face.make();
103+
await knex().schema.dropTableIfExists(TABLE);
104+
await driver.initObjects([OBJECT]);
105+
statements = [];
106+
knex().on('query', record);
107+
});
108+
109+
afterEach(async () => {
110+
knex().removeListener('query', record);
111+
await knex().schema.dropTableIfExists(TABLE);
112+
await driver.disconnect();
113+
});
114+
115+
it('measures the path it claims to (the cell is not vacuous)', () => {
116+
expect((driver as any).insertReturnsStoredRows).toBe(!face.readsBack);
117+
});
118+
119+
it('create with a generated id answers the stored row', async () => {
120+
const answer = await driver.create(TABLE, { title: 'generated', qty: 1, meta: { k: [1, 2] } });
121+
122+
expect(typeof answer.id).toBe('string');
123+
expect(answer.id).not.toBe('');
124+
expect(answer.done).toBe(false);
125+
expect(answer.meta).toEqual({ k: [1, 2] });
126+
expect(answer).toEqual(await stored(answer.id));
127+
});
128+
129+
it('create with a supplied id answers that row, in one statement or two by path', async () => {
130+
const answer = await driver.create(TABLE, { id: 'given-1', title: 'supplied', qty: 2 });
131+
132+
expect(answer).toEqual(await stored('given-1'));
133+
expect(answer.title).toBe('supplied');
134+
expect(answer.done).toBe(false);
135+
// The findOne above is the test's own read; the door's statements precede it.
136+
expect(statements.slice(0, face.readsBack ? 2 : 1)).toEqual(face.readsBack ? ['insert', 'select'] : ['insert']);
137+
expect(statements[face.readsBack ? 2 : 1]).toBe('select'); // the test's findOne
138+
});
139+
140+
it('bulkCreate answers one stored row per written row, in the written order', async () => {
141+
const answer = await driver.bulkCreate(TABLE, [
142+
{ title: 'b1' },
143+
{ id: 'given-b2', title: 'b2', qty: 2 },
144+
{ title: 'b3', qty: 3 },
145+
]);
146+
147+
expect(answer).toHaveLength(3);
148+
expect(answer.map((r) => r.title)).toEqual(['b1', 'b2', 'b3']);
149+
expect(answer[1].id).toBe('given-b2');
150+
// The batch is ONE insert, and on the read-back path ONE read.
151+
expect(statements).toEqual(face.readsBack ? ['insert', 'select'] : ['insert']);
152+
for (const row of answer) {
153+
expect(row.done).toBe(false);
154+
expect(row).toEqual(await stored(row.id));
155+
}
156+
});
157+
158+
it('bulkCreate of one row answers one stored row', async () => {
159+
const answer = await driver.bulkCreate(TABLE, [{ id: 'only', title: 'single' }]);
160+
161+
expect(answer).toHaveLength(1);
162+
expect(answer[0]).toEqual(await stored('only'));
163+
});
164+
165+
it('a tenanted create answers the row stamped with the caller tenant', async () => {
166+
const answer = await driver.create(TABLE, { id: 't-own', title: 'own' }, { tenantId: 'org_a' });
167+
168+
expect(answer.organization_id).toBe('org_a');
169+
expect(answer).toEqual(await stored('t-own'));
170+
});
171+
172+
it('an admin write naming another tenant answers the row it stored, not a refusal', async () => {
173+
// `injectTenantOnInsert` never overwrites an explicit tenant, so the row
174+
// lands under org_b while the call carries org_a. A read-back scoped to
175+
// the caller's ACTIVE org would miss it.
176+
const answer = await driver.create(
177+
TABLE,
178+
{ id: 't-named', title: 'named', organization_id: 'org_b' },
179+
{ tenantId: 'org_a' },
180+
);
181+
expect(answer.organization_id).toBe('org_b');
182+
expect(answer).toEqual(await stored('t-named'));
183+
184+
const batch = await driver.bulkCreate(
185+
TABLE,
186+
[
187+
{ id: 't-b1', title: 'stamped' },
188+
{ id: 't-b2', title: 'named', organization_id: 'org_c' },
189+
],
190+
{ tenantId: 'org_a' },
191+
);
192+
expect(batch.map((r) => [r.id, r.organization_id])).toEqual([
193+
['t-b1', 'org_a'],
194+
['t-b2', 'org_c'],
195+
]);
196+
});
197+
198+
it('inside a caller transaction, create and bulkCreate answer the uncommitted rows and the rollback keeps', async () => {
199+
const trx = await driver.beginTransaction();
200+
let single!: Record<string, unknown>;
201+
let batch!: Record<string, unknown>[];
202+
try {
203+
single = await driver.create(TABLE, { id: 'tx-1', title: 'in tx' }, { transaction: trx });
204+
batch = await driver.bulkCreate(TABLE, [{ id: 'tx-2', title: 'in tx' }, { id: 'tx-3', title: 'in tx' }], {
205+
transaction: trx,
206+
});
207+
} finally {
208+
await driver.rollback(trx);
209+
}
210+
expect(single.id).toBe('tx-1');
211+
expect(single.done).toBe(false);
212+
expect(batch.map((r) => r.id)).toEqual(['tx-2', 'tx-3']);
213+
expect(await driver.find(TABLE, {})).toEqual([]);
214+
});
215+
216+
if (face.readsBack && face.sqliteTriggers) {
217+
it('a written row that is gone before the read-back is refused, once, and never re-issued', async () => {
218+
await knex().raw(
219+
`create trigger os21227_vanish after insert on ${TABLE} begin delete from ${TABLE} where id = new.id; end`,
220+
);
221+
statements = [];
222+
223+
const err = await driver.create(TABLE, { id: 'gone', title: 'vanishes' }).then(
224+
() => null,
225+
(e: unknown) => e as { code?: string; status?: number },
226+
);
227+
expect(err).not.toBeNull();
228+
expect(err!.code).toBe('DATABASE_ERROR');
229+
expect(err!.status).toBe(500);
230+
// One insert, one read, no retry: re-issuing would duplicate a row that landed.
231+
expect(statements).toEqual(['insert', 'select']);
232+
});
233+
}
234+
});
235+
}
236+
237+
for (const cell of DIALECT_CELLS) {
238+
declareDialectCell(cell, 'create / bulkCreate stored row (#21227)', (c: DialectCell) =>
239+
suite({
240+
label: c.label,
241+
make: () => new SqlDriver(c.config()),
242+
readsBack: c.id === 'mysql',
243+
sqliteTriggers: c.id === 'sqlite',
244+
}),
245+
);
246+
}
247+
248+
suite({
249+
label: 'sqlite, read-back path forced',
250+
make: () => new ReadBackPathSqlDriver(dialectSqliteConfig()),
251+
readsBack: true,
252+
sqliteTriggers: true,
253+
});
254+
255+
function dialectSqliteConfig(): SqlDriverConfig {
256+
return DIALECT_CELLS.find((c) => c.id === 'sqlite')!.config();
257+
}

0 commit comments

Comments
 (0)