Skip to content

Commit bf927bb

Browse files
committed
fix(driver-sql): create and bulkCreate answer the stored row on MySQL
On the MySQL family knex drops RETURNING and answers [insertId], so create answered 0 and bulkCreate answered one element for the whole batch. Where the dialect's INSERT answers no rows, both doors now read back what they wrote, by the ids they wrote, under the tenant scope the rows were written with. SQLite and PostgreSQL keep RETURNING unchanged. Claude-Session: https://claude.ai/code/session_017xfMoEjKUuSh2xYB8sCozp Co-authored-by: Claude <noreply@anthropic.com>
1 parent 62b90d7 commit bf927bb

1 file changed

Lines changed: 203 additions & 10 deletions

File tree

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

Lines changed: 203 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1109,6 +1109,45 @@ function rawStatementFaultError(cause: unknown): Error {
11091109
return err;
11101110
}
11111111

1112+
/**
1113+
* [#21227] The refusal {@link SqlDriver.readBackInsertedRows} raises when an
1114+
* INSERT was accepted but a row it wrote is not there to be read back.
1115+
*
1116+
* `create` and `bulkCreate` answer the stored record (`IDataDriver.create`).
1117+
* On a dialect whose INSERT returns no rows the driver reads what it wrote
1118+
* back by the id it wrote, and only a row removed between the two statements
1119+
* (a concurrent delete, a trigger) can be missing. There is then no stored
1120+
* record to answer. Answering the caller's payload in its place would look
1121+
* like a success while answering a row that is not stored, and would hide a
1122+
* read-back keyed on the wrong column for good, so the door refuses instead.
1123+
*
1124+
* `DATABASE_ERROR` / 500, the pair this file's other terminals declare for a
1125+
* fault the request did not cause. The message is composed and names only the
1126+
* object the caller passed. The ids, which are the driver's own values (the
1127+
* caller's `id` / `_id` or a minted nanoid, never a business column), travel
1128+
* under a non-enumerable `cause` for the server log, as in
1129+
* {@link backendStatementFaultError}.
1130+
*/
1131+
function insertedRowsNotReadBackError(object: string, missingIds: unknown[], writtenCount: number): Error {
1132+
const err = new Error(
1133+
`The database accepted the insert into object '${object}', but ${missingIds.length} of its ` +
1134+
`${writtenCount} row(s) could not be read back by the id this driver wrote, so the stored ` +
1135+
'record cannot be answered. This database returns no rows from an INSERT, so the driver ' +
1136+
'reads each written row back by its id, and a row removed between the two statements (a ' +
1137+
'concurrent delete or a trigger) leaves nothing to read. The write was not retried, because ' +
1138+
're-issuing it could duplicate a row that did land.',
1139+
) as Error & { code?: string; status?: number };
1140+
err.code = StandardErrorCode.enum.DATABASE_ERROR;
1141+
err.status = 500;
1142+
Object.defineProperty(err, 'cause', {
1143+
value: new Error(`no row carries the written id(s) ${JSON.stringify(missingIds)} after the insert`),
1144+
enumerable: false,
1145+
writable: true,
1146+
configurable: true,
1147+
});
1148+
return err;
1149+
}
1150+
11121151
/**
11131152
* [#9354] How long a widening ALTER waits for a metadata lock, in seconds.
11141153
*
@@ -5830,6 +5869,38 @@ export class SqlDriver implements IDataDriver {
58305869
return SqlDriver.MYSQL_EMIT_CLIENTS.has(SqlDriver.clientSpelling(this.config));
58315870
}
58325871

5872+
/**
5873+
* [#21227] Whether this dialect's `INSERT … RETURNING *` answers the rows it
5874+
* STORED.
5875+
*
5876+
* `create` and `bulkCreate` answer the inserted record (`IDataDriver.create`).
5877+
* Where this is true they take it from the statement, in one round trip.
5878+
* Where it is false the statement answers no rows, and both doors read back
5879+
* what they wrote, by the ids they wrote ({@link readBackInsertedRows}).
5880+
*
5881+
* True for the SQLite and PostgreSQL families. knex compiles `RETURNING` for
5882+
* both, and the row it answers is the stored one, server-side column defaults
5883+
* included (measured on better-sqlite3 and on live PostgreSQL 16.14).
5884+
*
5885+
* False for the MySQL family, which has no `RETURNING`. knex's MySQL compiler
5886+
* drops the clause with a `.returning() is not supported by mysql` warning and
5887+
* answers `[insertId]`: ONE element whatever the row count, and `0` for this
5888+
* driver's string primary key. Measured on live MySQL 8.0.46 before this
5889+
* change: `create` answered `0`, a three-row `bulkCreate` answered `[0]`, and
5890+
* every row was stored. The auth adapter answers what `create` answers, so
5891+
* sign-up failed on MySQL with the user row stored and no account row.
5892+
*
5893+
* False, too, for a client this driver recognises as neither family (a Client
5894+
* constructor, a wire-only spelling such as `redshift`): reading back is
5895+
* correct on every dialect, and `RETURNING` is only the shortcut a dialect
5896+
* known to answer the stored row is given. The SQLite and PostgreSQL doors
5897+
* therefore pay no extra round trip, and the MySQL doors pay one SELECT per
5898+
* statement (per `create`, and per `bulkCreate` batch).
5899+
*/
5900+
protected get insertReturnsStoredRows(): boolean {
5901+
return this.isSqlite || this.isPostgres;
5902+
}
5903+
58335904
/**
58345905
* Per-granularity native SQL bucket support, computed from dialect.
58355906
*
@@ -7142,10 +7213,15 @@ export class SqlDriver implements IDataDriver {
71427213

71437214
/**
71447215
* [#15267] Declared as `IDataDriver.create()` declares it: the inserted
7145-
* record, `formatOutput(...)` over the `returning('*')` row. The annotation
7216+
* record, `formatOutput(...)` over the stored row. The annotation
71467217
* used to be an explicit `Promise<any>`, so the published `.d.ts` let a
71477218
* caller read any member off the result; it is the contract's type now,
71487219
* pinned by `sql-driver-doors-declared-types.test.ts`.
7220+
*
7221+
* [#21227] The stored row comes from the statement's own `returning('*')`
7222+
* where the dialect answers one, and from {@link readBackInsertedRows}
7223+
* where it does not ({@link insertReturnsStoredRows}: the MySQL family). The
7224+
* type was the contract's all along; on MySQL the VALUE was the insert id.
71497225
*/
71507226
async create(object: string, data: Record<string, any>, options?: DriverOptions): Promise<Record<string, unknown>> {
71517227
const { _id, ...rest } = data;
@@ -7186,8 +7262,11 @@ export class SqlDriver implements IDataDriver {
71867262
this.stampInsertTimestamps(object, formatted);
71877263

71887264
try {
7189-
const result = await builder.insert(formatted).returning('*');
7190-
return this.formatOutput(object, result[0]);
7265+
if (this.insertReturnsStoredRows) {
7266+
const result = await builder.insert(formatted).returning('*');
7267+
return this.formatOutput(object, result[0]);
7268+
}
7269+
await builder.insert(formatted);
71917270
} catch (error) {
71927271
// #11627: on a table whose UNIQUE index is carried by a hash shadow,
71937272
// `ER_DUP_ENTRY` quotes a binary digest and names the shadow index, so
@@ -7212,10 +7291,108 @@ export class SqlDriver implements IDataDriver {
72127291
// rather than burning a second number for nothing.
72137292
delete toInsert[reservation.field];
72147293
}
7294+
continue;
72157295
}
7296+
7297+
// [#21227] Reached only when the INSERT landed and answered no rows. The
7298+
// read sits OUTSIDE the try on purpose: a fault in it is not an insert
7299+
// failure, so it must never reach the collision re-seed above, whose
7300+
// retry would re-issue a write that already landed.
7301+
const [stored] = await this.readBackInsertedRows(
7302+
object,
7303+
this.rotationWriteTarget(object) ?? object,
7304+
[toInsert],
7305+
options,
7306+
);
7307+
return stored;
72167308
}
72177309
}
72187310

7311+
/**
7312+
* [#21227] Read back the rows an INSERT on this call just wrote, by the ids
7313+
* it wrote: what `create` and `bulkCreate` answer on a dialect whose INSERT
7314+
* answers no rows ({@link insertReturnsStoredRows}).
7315+
*
7316+
* One row per written row, in the written order (`IN (…)` promises no
7317+
* order), each through `formatOutput()`, the same presentation
7318+
* `returning('*')` gets on the other dialects and `update`'s own read-back
7319+
* gets on every dialect.
7320+
*
7321+
* # The read key: the written id, which this driver always holds
7322+
*
7323+
* Every row reaches the INSERT carrying the id this driver gave it: the
7324+
* caller's `id`, else its `_id`, else a nanoid minted in `create` /
7325+
* `bulkCreate` before the statement is built. The id is never asked of the
7326+
* database, so no insert id is read: the managed `id` column is a
7327+
* `varchar(255)` PRIMARY KEY with no AUTO_INCREMENT, and the insert id
7328+
* MySQL reports for it is `0`. The column is resolved through
7329+
* {@link remoteColumn}, so an external object whose `columnMap` renames
7330+
* `id` is read by its physical column, as its INSERT was written.
7331+
*
7332+
* # The table: the write target
7333+
*
7334+
* The table the statement wrote, a rotation shard included, so the read
7335+
* looks where the row landed rather than at the base name.
7336+
*
7337+
* # Tenant scope: the tenants the rows were WRITTEN under
7338+
*
7339+
* Routed through {@link applyTenantScope} like every read door in this class
7340+
* (`check:tenant-chokepoint`), scoped as `upsert`'s identity probe
7341+
* (`assertMergeLandedOnSuppliedIdentity`) scopes its own read: to the tenant
7342+
* each row was written under. On an ordinary tenanted call that is the
7343+
* caller's org, which `injectTenantOnInsert` stamped. On an admin write that
7344+
* names a tenant in the row data (a documented authority: explicit values
7345+
* are never overwritten), a read scoped to the caller's ACTIVE org would miss
7346+
* a row that really landed. A batch may name several tenants, so the scope is
7347+
* their union through `tenantIds`, which `applyTenantScope` already compiles
7348+
* as `IN (…) OR IS NULL`; the caller's own `tenantIds` membership set is
7349+
* replaced, not widened. With no tenant field, or no tenant on the rows and
7350+
* none on the call, `applyTenantScope` leaves the read unscoped by its own
7351+
* contract. The ids are this call's own and `id` is the PRIMARY KEY, so the
7352+
* read cannot answer a row this call did not write, in any organization.
7353+
*
7354+
* # A row that is not there
7355+
*
7356+
* Refused with {@link insertedRowsNotReadBackError}, never answered with the
7357+
* payload: see that function for why.
7358+
*/
7359+
private async readBackInsertedRows(
7360+
object: string,
7361+
writeTable: string,
7362+
written: Record<string, any>[],
7363+
options?: DriverOptions,
7364+
): Promise<Record<string, unknown>[]> {
7365+
if (written.length === 0) return [];
7366+
const idColumn = this.remoteColumn(object, 'id', 'id');
7367+
const tenantField = this.resolveTenantField(object);
7368+
const writtenTenants = tenantField
7369+
? [
7370+
...new Set(
7371+
written
7372+
.map((row) => row[tenantField])
7373+
.filter((value) => value !== undefined && value !== null && value !== '')
7374+
.map(String),
7375+
),
7376+
]
7377+
: [];
7378+
const scopeOptions: DriverOptions = {
7379+
...options,
7380+
tenantId: writtenTenants[0] ?? options?.tenantId,
7381+
tenantIds: writtenTenants.length > 0 ? writtenTenants : undefined,
7382+
};
7383+
const builder = this.getBuilder(writeTable, options);
7384+
this.applyTenantScope(builder, object, scopeOptions);
7385+
const stored: Record<string, any>[] = await builder.whereIn(
7386+
idColumn,
7387+
written.map((row) => row.id),
7388+
);
7389+
const byId = new Map<string, Record<string, any>>();
7390+
for (const row of stored) byId.set(String(row[idColumn]), row);
7391+
const missing = written.filter((row) => !byId.has(String(row.id))).map((row) => row.id);
7392+
if (missing.length > 0) throw insertedRowsNotReadBackError(object, missing, written.length);
7393+
return written.map((row) => this.formatOutput(object, byId.get(String(row.id))));
7394+
}
7395+
72197396
/**
72207397
* Ensure the sequence-counter table exists. Idempotent and cheap after
72217398
* the first call (cached via `sequencesTableReady`).
@@ -9185,13 +9362,16 @@ export class SqlDriver implements IDataDriver {
91859362
const builder = this.getBuilder(this.rotationWriteTarget(object) ?? object, options);
91869363

91879364
try {
9188-
const result = await builder.insert(formattedRows).returning('*');
9189-
// Read-back parity with create(): JSON columns come back as their stored
9190-
// strings from `returning('*')` — decode them so batch callers see the
9191-
// same shapes single-insert callers do.
9192-
return Array.isArray(result)
9193-
? result.map((r) => this.formatOutput(object, r))
9194-
: result;
9365+
if (this.insertReturnsStoredRows) {
9366+
const result = await builder.insert(formattedRows).returning('*');
9367+
// Read-back parity with create(): JSON columns come back as their stored
9368+
// strings from `returning('*')` — decode them so batch callers see the
9369+
// same shapes single-insert callers do.
9370+
return Array.isArray(result)
9371+
? result.map((r) => this.formatOutput(object, r))
9372+
: result;
9373+
}
9374+
await builder.insert(formattedRows);
91959375
} catch (error) {
91969376
if (!mayRetry || attempt >= AUTONUMBER_COLLISION_RETRIES) throw error;
91979377
const colliding = await this.collidingAutoNumberReservations(error, reservationsPerRow.flat(), options);
@@ -9223,7 +9403,20 @@ export class SqlDriver implements IDataDriver {
92239403
if (stale.has(this.autoNumberCounterKey(reservation))) delete rows[i][reservation.field];
92249404
}
92259405
}
9406+
continue;
92269407
}
9408+
9409+
// [#21227] Reached only when the INSERT landed and answered no rows (the
9410+
// MySQL family answered `[insertId]`: one element for the whole batch,
9411+
// which the engine's one-result-per-row guard then refused after every
9412+
// row had been stored). ONE read for the batch, by the ids written, in
9413+
// the written order. Outside the try for the reason `create` gives.
9414+
return this.readBackInsertedRows(
9415+
object,
9416+
this.rotationWriteTarget(object) ?? object,
9417+
rows,
9418+
options,
9419+
);
92279420
}
92289421
}
92299422

0 commit comments

Comments
 (0)