Repository navigation
fix(driver-sql): create and bulkCreate answer the stored row on MySQL (#21227) - #21239
objectstack-fleet[bot] merged 4 commits into
Conversation
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>
…per dialect Claude-Session: https://claude.ai/code/session_017xfMoEjKUuSh2xYB8sCozp Co-authored-by: Claude <noreply@anthropic.com>
…row on MySQL Claude-Session: https://claude.ai/code/session_017xfMoEjKUuSh2xYB8sCozp Co-authored-by: Claude <noreply@anthropic.com>
…eate-reads-back-stored-row
📓 Docs Drift CheckThis PR changes 1 package(s): 9 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 2 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 11 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 0d61a56aa6b419d817fecebb169b933a388b404c && git checkout 0d61a56aa6b419d817fecebb169b933a388b404c
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 3a7b6eb0635827442fa248baffa14187a60f5a22 ee7c024b92bc03e388eac0de377b78117cbfeeef && git checkout -B drift-repro 3a7b6eb0635827442fa248baffa14187a60f5a22 && git merge --no-ff ee7c024b92bc03e388eac0de377b78117cbfeeef
node scripts/docs-audit/affected-docs.mjs --json 3a7b6eb0635827442fa248baffa14187a60f5a22
|
Contract reviewServed-tier: Inputs: card #21227 (body; comments 5938387083, 5938943912, 5940143677, 5940180378), PR #21239 (body, 3-file list, net diff against merge base ① Derived judgments
② Semver level
③ Boundary flags
Implemented-by: VERDICT: PASS Generated by Claude Code |
…precision (objectstack-ai#21252) Fixes objectstack-ai#21241 Clause-②: no ## What was wrong On MySQL a declared `Field.datetime` is built as `DATETIME(3)`. For its `defaultValue: 'NOW()'` default, `SqlDriver.nowColumnDefault('datetime')` fell through to a bare `knex.fn.now()`, which is `CURRENT_TIMESTAMP` at precision 0. MySQL 8.0 refuses a `CURRENT_TIMESTAMP` default whose precision differs from its column's, so the whole `CREATE TABLE` failed and so did `ALTER TABLE … ADD`. The builtin `created_at` / `updated_at` columns beside it carried their own literal `now(3)` and were accepted. Measured at the base `be5a83cf` against a throwaway MySQL 8.0.46 (server zone `+08:00`): | boot | schema sync | refused DDL | data door | |:--|:--|:--|:--| | `pnpm dev:crm -- --fresh --database mysql://…` | `synced 80, skipped 0, failed 1` | `sys_activity`: `` `timestamp` datetime(3) default CURRENT_TIMESTAMP `` → `Invalid default value for 'timestamp'` | `GET /api/v1/data/sys_activity?limit=1` → `500 DATABASE_ERROR` | | `pnpm dev:showcase -- --fresh --database mysql://…` | `synced 102, skipped 2, failed 2` | `sys_activity` as above, and `sys_presence`: `` `last_seen` datetime(3) default CURRENT_TIMESTAMP `` → `Invalid default value for 'last_seen'` | `GET /api/v1/data/sys_presence?limit=1` → `500`, `sys_activity` → `500` | ## The fix - **One source for the precision.** `MYSQL_DATETIME_PRECISION` is a module constant in `sql-driver.ts`. Five sites read it: the declared datetime column (`createColumn`), the builtin audit columns (`createAuditTimestampColumn`), the `NOW()` default (`nowColumnDefault`), the UPDATE stamp (`updatedAtStamp`) and the legacy `TIMESTAMP` widening (`migrateMysqlDatetimeColumns`). Before this PR, five literal `3`s lived at those sites. Now there is one, and no new literal was added. - **`nowColumnDefault('datetime')` on MySQL** returns `knex.fn.now(MYSQL_DATETIME_PRECISION)`, which renders `CURRENT_TIMESTAMP(3)`. - **The builtin audit column's MySQL default is routed through `nowColumnDefault('datetime')`**, the way the SQLite branch already is. The declared column and the builtin column now share one definition. The emitted DDL for the audit columns is byte-identical: `datetime(3) default CURRENT_TIMESTAMP(3)`. - PostgreSQL and SQLite emit unchanged SQL. On PostgreSQL the driver builds `timestamptz`, and `CURRENT_TIMESTAMP` on it was measured accepted on PostgreSQL 16.14. `timestamp(3) default CURRENT_TIMESTAMP` is also accepted there, rounding silently. So the mismatch does not exist on that dialect. ### Bounded in-place fix in the same statement: the `TIMESTAMP` widening dropped a declared `NOW()` default The widening's `ALTER … MODIFY` line now reads the constant, so this PR touches it. That same statement restated the default of `created_at` / `updated_at` and dropped the default of a declared `NOW()` column. The `TIME` twin (`migrateMysqlTimeColumns`) already restates it. Measured on MySQL 8.0.46 with the driver built at this branch before the change: a legacy `stamped_at timestamp null default current_timestamp` column came out of schema sync as `datetime(3)` with `COLUMN_DEFAULT` `null`, and `create` without the field answered `stamped_at = null`. The widening now restates `nowColumnDefault('datetime')` for a declared `NOW()` column as well. All four conditions for an in-place fix hold: - same family (a MySQL `NOW()` datetime default that is not honoured); - mechanical, with the shape fixed by the `TIME` twin; - same file and claim; - same gate family. The changed lines are `sql-driver.ts` `migrateMysqlDatetimeColumns`, and a MySQL-only pin, §3. ## Pins: `packages/drivers/driver-sql/src/sql-driver-21241-mysql-now-default-precision.test.ts` - **§1**, run on every runner with no server: for `mysql2`, `pg` and `better-sqlite3`, the compiled DDL of a declared `NOW()` datetime column is byte-identical to the builtin audit column's. A second case reads the `mysql2` DDL of the required `sys_activity.timestamp` shape and checks that its `CURRENT_TIMESTAMP(n)` names the column's own `datetime(n)`. This compares against the column, not against a literal. - **§2**, one cell per dialect through `declareDialectCell`: - the table syncs, both on create and on add-column for a table that already exists; - `create` without the field answers the instant the column `DEFAULT` stored. The check uses a window and confirms that the answer equals `findOne`; - a raw insert that never names the column is filled by the `DEFAULT` alone; - the server's own catalogue (`information_schema` / `pragma table_info`) reports the declared column's type and default equal to `created_at`'s. - **§3**, MySQL only: a legacy `TIMESTAMP` `NOW()` column keeps a default through the widening, equal to `created_at`'s. ## Verification (HEAD `d334fe314b`) Every reading below was re-run at `d334fe314b`: the branch plus one merge of `origin/main` `4727fcb22a`, which touches no `driver-sql` file and no lockfile. The readings match the earlier head `e5cab5f20b`, except the `driver-turso` count, which moved because objectstack-ai#21226 landed on `main`. (Seat edit.) The live servers were local: MySQL 8.0.46 at `+08:00` and PostgreSQL 16.14 at `Asia/Shanghai`, with `TZ=America/New_York`. - Pins: `vitest run --reporter=verbose src/sql-driver-21241-mysql-now-default-precision.test.ts` with both URLs set → **17 passed (17)**. - **Reverse verification.** `sql-driver.ts` was written back to the base blob `e65a0f08` while the HEAD blob `533a790b` stayed committed. On-disk hash equal to the base blob, `grep -c MYSQL_DATETIME_PRECISION` → 0. Result: **7 failed | 10 passed (17)**. Every failure is MySQL: §1 `mysql2` ×2, §2 live mysql ×4 (`Invalid default value for 'stamped_at'` at create and at add-column), and §3. The SQLite and PostgreSQL cells stayed green (the control). The restore was checked: hash equal to the HEAD blob and `git diff HEAD` empty. - **Ablation of the widening half.** Through `scripts/ablation-replace.mjs`: anchor 1 → 0, blob `533a790b` → `16057202`. Only §3 went red, `dflt: null` against `CURRENT_TIMESTAMP(3)`; the other 16 stayed green. Restored: blob equal to HEAD and `git diff HEAD` empty. - `@objectstack/driver-sql`, the whole suite against both live servers with `OS_EXPECT_LIVE_DIALECT_MATRIX=1`: **224 files passed, 5386 passed | 1 skipped**. The live-dialect reporter printed "all 3 dialects were exercised". - `@objectstack/driver-sql`, the SQLite tier (`pnpm --filter @objectstack/driver-sql test`, no URLs, at `04b23d2791` before the widening commit): 213 files passed | 11 skipped, 3570 passed | 200 skipped. - Typecheck of `@objectstack/driver-sql`, `@objectstack/driver-sqlite-wasm` and `@objectstack/driver-turso` (the inheritors): all three `Done`. `tsc --listFiles` includes the new test file once. - Tests of the inheritors: `driver-sqlite-wasm` 36 files / 675 passed; `driver-turso` 87 files / 2349 passed | 33 skipped. - `pnpm check:driver-conformance` gave the same reading before the first edit and after the last commit: `OK — 50 covered cell(s), 0 in the DEBT ledger, 0 exempt`, with 0 in the DIALECT ledger. - Gates: `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands` derived 63 families. All 63 ran with exit 0. `--ran` reconciliation: `63 derived, 63 run, 0 NOT-MEASURED, 0 UNRUN`, with every line recording its exit code. The derivation warned that the tree is one `main` commit behind (`3dc33b2d13`, file-disjoint from this diff, and it touches `check-route-envelope.mjs` / `engine-double-contract.pinned.json`). CI re-derives on the merge ref. - Lint, narrowed: - `eslint --no-inline-config --format json` over the two changed `.ts` files: 2 files, 0 errors, 0 warnings. - Population: both files are linted, not ignored. - Invariance: the config's `parserOptions` carry no `project` / `projectService`, so linting is not type-aware and this diff cannot move a verdict on an untouched file. - Repo-wide `pnpm lint` is left to CI. ### After the fix, same boot `pnpm dev:showcase -- --fresh --database mysql://…` at `e5cab5f20b`: - no `Schema sync FAILED`; - `GET /api/v1/data/sys_presence` → 200 and `sys_activity` → 200; - `information_schema` reports `sys_activity.timestamp` and `sys_presence.last_seen` as `datetime(3)` / `CURRENT_TIMESTAMP(3)`, equal to their `created_at`; - a `POST` / `PATCH` / `DELETE` on `showcase_account` left three `sys_activity` rows (`created`, `updated`, `deleted`). ## Raise-rule reading (triage) At the base on MySQL, with `sys_activity` absent, a missing table does not break a record write. `POST /api/v1/data/crm_account` → 201, `PATCH` → 200 (a read-back showed the new name), `DELETE` → 200 (a read-back answered 404). `sys_audit_log` holds 3 rows for the record. Each mutation lost its activity row: the server logged `Insert operation failed {object: sys_activity …}` at `warn` and `Audit write FAILED (ER_NO_SUCH_TABLE …)` at `error`. The API answers carried nothing about it. Applying the rule is the seat's decision. ## Acceptance notes - **Existing tables / migration (H5).** On MySQL no table could have been created with the refused default, so a fresh boot after this change creates the missing tables and no migration is owed. A column that an earlier release's `TIMESTAMP` widening already left without a default does not get one back from this change. The widening only touches columns that are still `timestamp`. This is a read-only inference beyond the measurement above. - **MariaDB not measured.** No MariaDB server here. `client: 'mariadb'` is not in the driver's MySQL family, but `mysql2` pointed at a MariaDB server is. Whether MariaDB accepted the bare default, and so holds tables with a precision-0 default, is NOT MEASURED. - **Operator text.** At the base, the `Audit write FAILED` line for the `sys_activity` insert names `sys_audit_log` as the row that "never landed" and as the table to check. It printed 4 times despite "reported ONCE". Observation only, nothing filed. - **`sys_packages`.** Its raw DDL (`created_at TEXT DEFAULT CURRENT_TIMESTAMP`) is still refused on MySQL in both boots. This was already recorded in PR objectstack-ai#21239's acceptance notes; it is `domain:services`, and no door was measured. - **Not filed from this PR, handed to the seat in the report.** At the base on MySQL, `GET /api/v1/auth/jwks` and `GET /api/v1/auth/token` answered 500. An insert into `sys_jwks` is refused with `Incorrect datetime value … for column 'updated_at'`, and the server logs `JWT signing failed with alg "EdDSA"`. --- _Generated by [Claude Code](https://claude.ai/code/session_017xfMoEjKUuSh2xYB8sCozp)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
Fixes #21227
Clause-②: no
What changes
SqlDriver.createandSqlDriver.bulkCreateranbuilder.insert(...).returning('*')and answered what the statement answered. MySQL has noRETURNING: 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, and0for this driver's string primary key.Both doors now answer the stored row on every dialect:
RETURNINGin one statement..returning(), and the rows are read back by the ids that were written. That is oneSELECTpercreate, and one perbulkCreatebatch.The switch is one protected capability getter,
insertReturnsStoredRows(isSqlite || isPostgres), next toisMysql. 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/main62b90d74, driver called directly)create, generated id0create, supplied id0bulkCreate, 3 rows[0](length 1)bulkCreate, 1 row[0]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)62b90d74POST /api/v1/auth/sign-up/email, first user (--no-seed-admin)400 FAILED_TO_CREATE_USER;sys_userrow stored, nosys_accountrow200; user,credentialaccount and session storedPOST /api/v1/auth/sign-in/email, same credentials401 INVALID_EMAIL_OR_PASSWORD200--seed-adminat bootdev admin seed skipped: Failed to create user; orphanedsys_user200POST /api/v1/data/crm_accountas the seeded admin201, with the full record including the stampedorganization_idcurated capability ... has no platform row and could not be seededwarningsDecisions the card left open
H3: read back only where
RETURNINGdoes not answer the stored row. Reading back on every dialect would add one round trip to everycreateon SQLite and PostgreSQL, whereRETURNINGalready answers the stored row (measured above). It would also move the control cells onto new code. Cost on MySQL: +1SELECTper 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.RETURNINGis the shortcut that only a dialect known to answer the stored row gets.driver-sqlite-wasmoverridesisSqlitetotrue, so it keepsRETURNING.H2: one helper for
createandbulkCreateonly.updateandupsertare byte-unchanged. The four read-backs answer different things:updatereads by id under the caller's scope and answersnullon a miss, which its contract allows.upsertreads by the conflict-key values it matched on and falls back to the payload.createmust answer a row, and is keyed on ids it wrote.Sharing one helper would change one of those answers. It would also touch the
upsertregion, which the card fences off.H4: the read key is the written id, and that id always exists.
createandbulkCreategive every row its id before the statement is built: the caller'sid, else_id, else a minted nanoid. The managedidcolumn isvarchar(255)PRIMARY KEY with no AUTO_INCREMENT, so no insert id is ever read. That is the only kind of key this path produces.remoteColumn, so an externalcolumnMapthat renamesidis read by its physical column.applyTenantScope, scoped to the tenant(s) the rows were WRITTEN under (asassertMergeLandedOnSuppliedIdentityscopes its probe). For a batch that is the union throughtenantIds. So an admin write that names another tenant in the row data is answered rather than missed.idis the PRIMARY KEY, so the read cannot answer another organization's row.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'stry, so a read fault can never reach the autonumber collision retry.One conclusion per face of the invariant (
IDataDriver.createanswers the inserted record)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).driver-sqlite-wasmand LOCAL-modedriver-turso: inheritSqlDriver.create/bulkCreateand stay onRETURNING. Wasm overridesisSqlitetotrue; Turso local usesbetter-sqlite3. Their suites are green: wasm 36 files / 675 tests, turso 86 files / 2313 passed, 33 skipped.driver-turso: already conformant.RemoteTransport.createissues its INSERT and thenSELECT * ... WHERE "id" = ?and answers that row (remote-transport.ts). ItsbulkCreateloops the driver's owncreate.driver-memory: already conformant.createpushes the built record and answers a copy of it, andbulkCreateanswers the pending records it pushed.driver-mongodb: already conformant.createanswers the document it inserted (minus_id), andbulkCreateanswers the inserted docs in order.Pins
packages/drivers/driver-sql/src/sql-driver-21227-create-answers-stored-row.test.ts, throughdeclareDialectCell: SQLite always, and live PostgreSQL and MySQL where provisioned. TheTemporal 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:createwith a generated id and with a supplied id;bulkCreateof 3 rows, which answers 3 rows in written order from ONE insert, plus ONE read on the read-back path;bulkCreateof 1 row;codeandstatusand that the insert is not re-issued;Every answer is compared with the driver's own
findOneand must carry the DEFAULT-onlydone: false.Reverse verification, both legs run with live PostgreSQL 16.14 and MySQL 8.0.46:
sql-driver.tsat62b90d74, worktree only, restore by trap with a hash check):13 failed | 20 passed.expected +0 to deeply equal {...}, andexpected [ +0 ] to have a length of 3 but got 1.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.tsand 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 mergingorigin/mainwith #21225 in it)pnpm --filter @objectstack/driver-sql exec vitest run --maxWorkers=2withOS_TEST_POSTGRES_URLandOS_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, inschema-drift.base-type-mismatch.test.ts.pnpm --filter @objectstack/driver-sqlite-wasm test(36 / 675 passed) andpnpm --filter @objectstack/driver-turso test(86 files, 2313 passed, 33 skipped): exit 0.typecheckfor driver-sql, driver-sqlite-wasm and driver-turso: exit 0.tsc --listFilesincludes the new test file.node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commandsderived 63 commands atee7c024b92, and all 63 exited 0.--ranreconciliation:63 derived, 63 run, 0 NOT-MEASURED, 0 UNRUN, all with recorded exit codes. This includescheck: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 axis8 conformance suite(s) ... 0 in the DIALECT ledger; after the last commit (ee7c024b92), identical.eslint --no-inline-config --format jsonwas run on the two touched TypeScript files.--print-configresolves rules for both (6 and 5 rules; neither file is ignored).eslint.config.mjsenables no type-aware linting (parserOptions.projectandprojectServiceare null for both files), so this diff cannot move a verdict on an untouched file.pnpm lintis left to CI.Acceptance notes
sql-driver-21163-autonumber-prefix-like-escape.test.ts's header says its cases read the stored row "Not fromcreate'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.CREATE TABLE IF NOT EXISTS sys_packages (... created_at TEXT DEFAULT CURRENT_TIMESTAMP ...)is refused withER_INVALID_DEFAULT(Invalid default value for 'created_at'), and laterSELECT * FROM sys_packagesreads answerER_NO_SUCH_TABLE. No door was measured for it, so it is noted here and not filed.sys_activityboot failure is reported to the PM with a measured door, for the seat to file. It is not touched here.Generated by Claude Code