Skip to content

Commit 2034975

Browse files
fix(driver-turso): a Turso remote database enforces the storage.notNull a field declares (#23893)
Fixes #23847 Clause-②: no ## What was wrong `RemoteTransport`, driver-turso's remote face and the `libsql://` transport that hosted environments use, built every declared column without `NOT NULL`. `buildCreateTableSQL` and both `ALTER TABLE … ADD COLUMN` sites (in `syncSchema` and in `syncSchemasBatch`) wrote only the storage type. So a field declared `storage: { notNull: true }` was unenforced on every Turso remote database. The local face's `SqlDriver.createColumn` has always created that column `NOT NULL` (ADR-0113). The shared differ then read each such column as destructive `tighten_not_null` drift, on a database the remote sync had only just created. ## What changes - **`RemoteTransport.columnDeclarationSQL(field)`** is now the one renderer for every statement that materializes a declared field's column on this face. It writes the storage type from `mapFieldTypeToSQL`, then ` NOT NULL` when `declaresColumnNotNull(field)` holds. The predicate asks what `createColumn`'s own `declaresColumnNotNull` asks: whether `storage.notNull` is truthy. That holds on every field type, multi-value JSON columns included (the #17231 case), and `required` alone never triggers it. - **All three sites render through it:** `buildCreateTableSQL`, the `syncSchema` ADD COLUMN and the `syncSchemasBatch` ADD COLUMN. The three remote schema doors (`syncSchemasBatch`, `syncSchema`, `initObjects`) all reach those statements. - **A refused remote `ADD COLUMN` from `syncSchema` now names its statement.** It uses knex's `SQL - message` form, as the local face's error already does. libSQL's own message names no table and no column, and the engine's per-object report names only the object. The error object, its class and its code are unchanged. - **`turso-driver.ts`, the caveat text only.** The remote face's `driftReportCaveat` sentence, and the header docblock above it, said in the present tense that the remote sync creates no `NOT NULL`. This change makes that false, so those NOT NULL sentences now say that tables created by an earlier release keep their nullable columns. The caveat stays non-null. Every line carrying the "a destructive entry has no remedy on this transport" clause, and its docblock twin, is byte-identical to `main`; #23848 owns that clause (relay 6213357665, accepted in 6213866355). ## Measurements The committed test uses the instrument `turso-remote-drift-detection.test.ts` established: a remote-mode `TursoDriver` over a real `@libsql/client` `file:` client (libSQL 3.45.1), with the local face as the control. A second leg ran against a real server: the official `sqld 0.24.32` (tursodatabase/libsql release `libsql-server-v0.24.32`, sha256 `71720fc8…336879f`, matching its published checksum), reached as `libsql://127.0.0.1:PORT?tls=0`. That leg is a scratch suite and is not committed, because CI has no sqld. **Positive control on unchanged code (`f7e836da1`):** - sqld: each of the three doors creates `title` and `tags` (both `storage.notNull`) with `notnull=0`, and `detectManagedDrift` answers `tags:tighten_not_null` and `title:tighten_not_null`. The ADD COLUMN path behaves the same. A populated table accepts the "notNull" column silently, as nullable. - `file:` client: the remote reader and the local face reading the same file both report the same two `tighten_not_null` entries. A table the local face created reads `[]`. **On the fix (sqld re-run at `5a41f2167`):** every door gives `title:nn=1 tags:nn=1 plain:nn=0` and drift `[]`, on CREATE TABLE and on ADD COLUMN. A populated table refuses with `ALTER TABLE "…" ADD COLUMN "added" TEXT NOT NULL - SQLITE_UNKNOWN: SQLite error: Cannot add a NOT NULL column with default value NULL`, and the column stays absent. **Ablation:** `scripts/ablation-replace.mjs` in WRAP mode replaced `return declaresColumnNotNull(field) ? … NOT NULL … : colType;` with `return colType;` (anchor 1 to 0, blob `039d3cd8` to `b133667a`), and the new suite ran while mutated. Result: **10 of 10 red**. The restore is proven: the blob equals HEAD's `039d3cd8` and `git diff HEAD` is empty. The test reaches the subject through relative imports, so it reads `src/` and no build sits between the mutation and the run. ## `turso-remote-not-null.test.ts`, 10 cases 1. CREATE TABLE through `syncSchemasBatch` / `syncSchema` / `initObjects`: the remote `notnull` flags equal the local face's for the same declarations (`title`, `amount`, `active` and multi-value `tags` are 1; `required`-only `note`, `notNull: false` and plain are 0). The remote and the local reader both report drift `[]`. 2. ADD COLUMN onto an existing empty table, through the same three doors: the same result. 3. Enforcement: an insert that leaves `title` NULL is refused by the database on both faces, and no row lands. 4. A populated table with no DEFAULT: both faces refuse (`Cannot add a NOT NULL column with default value NULL`). The remote `syncSchema` refusal names `ALTER TABLE "rows_t" ADD COLUMN "added" TEXT NOT NULL`, and no nullable column is slipped in. 5. A **recorded asymmetry**, described in the next section. ## ADD COLUMN on a table that already holds rows Once a table has a row, SQLite refuses `ADD COLUMN … NOT NULL` without a non-NULL DEFAULT. An empty table accepts it. Both were measured on libSQL 3.45.1 and on sqld 0.24.32. The local face sends the same statement and gets the same refusal, measured as `alter table … add column … text not null - Cannot add a NOT NULL column with default value NULL`. So that object's sync fails loudly on both faces, and the engine reports it per object: `ObjectQLPlugin` replays a failed batch per object at warn first, so only the failing object loses its DDL. The `NOT NULL` is never dropped to let the statement pass. One case still differs, and it is about defaults rather than nullability. The remote face emits no declared column DEFAULT for any field, while `createColumn` emits a literal `defaultValue` (`applyDeclaredColumnDefault`). So a `storage.notNull` field with a literal `defaultValue`, added to a populated table, is accepted locally (existing rows take the default) and refused here. Case 5 pins that as a named asymmetry, to be inverted when this face emits declared defaults. Whether it should, and from which shared source, goes back to the dispatching seat as an open question. A third copy of the default rules (the CLI's `declaredColumnDefault` is already the second) is a choice for that seat, not for this PR. ## Not in this PR - **Data at rest.** A remote database created before this release keeps its nullable columns. An additive sync never tightens an existing column, so that database's drift report still names those columns `tighten_not_null`. No existing database is migrated here. That is why the caveat stays, and why the `OS_ARTIFACT_URL` boot gate still does not act on a remote report (#23848 is the other precondition). ## Acceptance notes - **File surface.** The claim's surface was amended in 6213866355 to cover the `turso-driver.ts` NOT NULL sentences. The caveat's existing pin (`turso-remote-drift-detection.test.ts`, which expects the text to contain `NOT NULL`) still holds. - **Stale CLI comment.** `packages/cli/src/utils/artifact-boot-migration.ts` has a code comment whose present tense is now stale in the same way: "its schema sync does not create a declared NOT NULL". It is a comment in another package, so it is not edited here. carrier: #23848, which reworks that gate's remote branch. - **The predicate has two spellings.** driver-sql's `declaresColumnNotNull` is module-private, so this transport spells the same question itself. The new test pins their parity: remote `notnull` flags equal the local face's for one declaration set. Exporting the driver-sql predicate would give it one source; noted, not filed. - **Builtin timestamp defaults differ by face (observation, reach not measured).** From the dev's measurement run at `f7e836da1`: the remote CREATE TABLE gives `created_at` / `updated_at` `TEXT DEFAULT (datetime('now'))` (the zone-naive `YYYY-MM-DD HH:MM:SS` form), while the local face gives `DEFAULT strftime('%Y-%m-%dT%H:%M:%fZ', 'now')`. Whether any write path leaves those columns to the database default was not measured, so this is noted, not filed. (Added by the `domain:engine` seat 2 from the dev's report.) - **Conformance ledger:** 55 covered, 0 DEBT, 0 exempt, both before (`f7e836da1`) and after (`fcc038d2f`). ## Tests (head `fcc038d2f`) - `pnpm --filter @objectstack/driver-turso test`: 91 files, 2425 passed, 33 skipped (VERDICT command-exit 0). - `pnpm --filter @objectstack/driver-turso typecheck`: exit 0. `tsc --listFiles` includes all 91 test files, the new one among them. - eslint over the 3 changed `.ts` files: exit 0. ## Gates (head `fcc038d2f`) - `node scripts/pm/dispatch-gates.mjs --commands` derived 67 families, and all 67 exit 0. `--ran` reports 67 derived, 67 run, 0 NOT-MEASURED, 0 UNRUN. - The four artifact-roster families whose roster sits in a directory this diff touches were also run, and each exits 0: `check-changeset-fixed`, `check:authz-resolver`, `check:error-code-casing`, `check:filter-alias-parity`. - The declared wide-population families, the type-check lanes and the rest of the full battery are CI's to run. --- _Generated by [Claude Code](https://claude.ai/code/session_01TQLy3Yq6vF4SDpTXdr3Uwi)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 570f01d commit 2034975

4 files changed

Lines changed: 344 additions & 9 deletions

File tree

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
---
2+
'@objectstack/driver-turso': patch
3+
---
4+
5+
fix(driver-turso): a Turso remote database enforces the `storage.notNull` a field declares, on the tables and columns its schema sync creates
6+
7+
Clause-②: no
8+
9+
**What was wrong.** On a Turso database reached over the remote transport (`libsql://…`, the transport hosted environments use), schema sync created every declared column nullable. A field declared `storage: { notNull: true }` was not enforced there: an insert that left it empty was stored. The same declaration on a local SQLite file (`file:`) has always produced a `NOT NULL` column. The drift report, which compares the database with the declarations through the shared differ, also read each such column as destructive `tighten_not_null` drift on a database the remote sync had just created.
10+
11+
**What changes.** The remote face's `CREATE TABLE` and `ALTER TABLE … ADD COLUMN` add `NOT NULL` to a column whose field declares `storage.notNull`, wherever the local face adds it: on every field type, multi-value fields included, and never for `required` alone. On a remote database synced after this release, an insert that leaves such a field empty is refused by the database, and the drift report names no `tighten_not_null` for it.
12+
13+
**What does not change.**
14+
15+
- A column that already exists is not touched. A remote database synced by an earlier release keeps its nullable columns, and its drift report still names them `tighten_not_null`; no data at rest is migrated. So the remote face still declares its drift-report caveat (reworded to say this), and the `OS_ARTIFACT_URL` boot gate still does not act on a remote report.
16+
- Adding a `storage.notNull` field to a remote table that already holds rows is refused by SQLite (`Cannot add a NOT NULL column with default value NULL`), as it is on a local SQLite file, and that object's schema sync fails loudly. The remote error now names the refused statement. One case still differs: when the field also declares a literal `defaultValue`, the local face creates the column with that DEFAULT and accepts it, while the remote face emits no declared column DEFAULT and refuses it.

‎packages/drivers/driver-turso/src/remote-transport.ts‎

Lines changed: 86 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,17 @@ const DEFAULT_ID_LENGTH = 16;
9999
*/
100100
const BUILTIN_COLUMNS = new Set(['id', 'created_at', 'updated_at']);
101101

102+
/**
103+
* [#23847] ADR-0113's physical column constraint: does this field declare a
104+
* NOT NULL column? Asked exactly as the local face's `declaresColumnNotNull`
105+
* (`driver-sql`'s `sql-driver.ts`) asks it — truthiness of `storage.notNull`,
106+
* whatever the field's type, `multiple: true` included, and never `required`.
107+
* See {@link RemoteTransport.columnDeclarationSQL}, its one reader here.
108+
*/
109+
function declaresColumnNotNull(field: unknown): boolean {
110+
return Boolean((field as { storage?: { notNull?: unknown } } | null | undefined)?.storage?.notNull);
111+
}
112+
102113
/**
103114
* [#17609] Every index name the database already carries — ONE statement for a
104115
* whole schema sync, read beside the column probe. SQLite index names are
@@ -2098,8 +2109,7 @@ export class RemoteTransport {
20982109
const type = (field as any).type || 'string';
20992110
if (type === 'formula') continue; // Virtual — no column
21002111
this.assertSafeIdentifier(name);
2101-
const colType = this.mapFieldTypeToSQL(field);
2102-
await this.client!.execute(`ALTER TABLE "${tableName}" ADD COLUMN "${name}" ${colType}`);
2112+
await this.executeAddColumn(`ALTER TABLE "${tableName}" ADD COLUMN "${name}" ${this.columnDeclarationSQL(field)}`);
21032113
materialized.add(name);
21042114
}
21052115
}
@@ -2230,8 +2240,7 @@ export class RemoteTransport {
22302240
const type = (field as any).type || 'string';
22312241
if (type === 'formula') continue;
22322242
this.assertSafeIdentifier(name);
2233-
const colType = this.mapFieldTypeToSQL(field);
2234-
ddlStatements.push(`ALTER TABLE "${object}" ADD COLUMN "${name}" ${colType}`);
2243+
ddlStatements.push(`ALTER TABLE "${object}" ADD COLUMN "${name}" ${this.columnDeclarationSQL(field)}`);
22352244
materialized.add(name);
22362245
}
22372246

@@ -2396,15 +2405,86 @@ export class RemoteTransport {
23962405
const type = (field as any).type || 'string';
23972406
if (type === 'formula') continue; // Virtual — no column
23982407
this.assertSafeIdentifier(name);
2399-
const colType = this.mapFieldTypeToSQL(field);
2400-
sql += `, "${name}" ${colType}`;
2408+
sql += `, "${name}" ${this.columnDeclarationSQL(field)}`;
24012409
}
24022410
}
24032411

24042412
sql += ')';
24052413
return sql;
24062414
}
24072415

2416+
/**
2417+
* [#23847] What follows a declared field's column NAME in this transport's
2418+
* DDL: the storage type {@link mapFieldTypeToSQL} decides, then ` NOT NULL`
2419+
* when the field declares `storage: { notNull: true }`. The ONE renderer for
2420+
* every statement that materializes a declared field's column here — the
2421+
* `CREATE TABLE` {@link buildCreateTableSQL} builds and the `ALTER TABLE …
2422+
* ADD COLUMN` of {@link syncSchema} and {@link syncSchemasBatch} — so no door
2423+
* can create the column without the constraint another door creates it with.
2424+
*
2425+
* # The reference: `SqlDriver.createColumn`, this driver's local face
2426+
*
2427+
* ADR-0113: the physical NOT NULL comes from the EXPLICIT storage
2428+
* constraint, never from `required` (the write-time contract the record
2429+
* validator enforces). `createColumn` asks one question for it,
2430+
* `declaresColumnNotNull` — truthiness of `storage.notNull` — on BOTH of its
2431+
* exits, the multi-value JSON column included (#17231), and runs on `CREATE
2432+
* TABLE` and on `ALTER TABLE ADD COLUMN` alike. {@link declaresColumnNotNull}
2433+
* below asks the same question, the same way, on the same two statements.
2434+
* `turso-remote-not-null.test.ts` holds the two faces to one answer: the
2435+
* columns a remote sync creates carry the `notnull` flag the local face's
2436+
* columns carry for the same declarations.
2437+
*
2438+
* Until this renderer the remote face never emitted the constraint, so every
2439+
* `storage.notNull` was unenforced on a Turso remote database, and the shared
2440+
* drift differ read each such column as DESTRUCTIVE `tighten_not_null` drift
2441+
* against a table this transport had itself just created.
2442+
*
2443+
* # ADD COLUMN on a table that already holds rows
2444+
*
2445+
* SQLite refuses `ADD COLUMN … NOT NULL` without a non-NULL `DEFAULT` once
2446+
* the table has a row (`Cannot add a NOT NULL column with default value
2447+
* NULL`); an empty table accepts it. The local face sends the same
2448+
* statement and gets the same refusal, so the sync of that object fails
2449+
* loudly on both faces, and the engine reports it per object. ⛔ Never drop
2450+
* the ` NOT NULL` to let such a statement through: a nullable column under a
2451+
* `storage.notNull` declaration is exactly the silent, unenforced state this
2452+
* renderer replaced. Imposing NOT NULL on a column that already exists is
2453+
* not done here at all — that is `tighten_not_null`, destructive, and
2454+
* belongs to the migration path, not to an additive sync.
2455+
*
2456+
* ⚠️ One case still differs, and it is a DEFAULT question, not a nullability
2457+
* one: this transport emits no declared column DEFAULT for any field, while
2458+
* `createColumn` emits a literal `defaultValue` (`applyDeclaredColumnDefault`).
2459+
* So a `storage.notNull` field that also declares a literal `defaultValue`,
2460+
* added to a populated table, is accepted locally (existing rows take the
2461+
* default) and refused here. `turso-remote-not-null.test.ts` records that
2462+
* asymmetry; closing it means deciding whether, and from which shared source,
2463+
* this face emits declared defaults.
2464+
*/
2465+
private columnDeclarationSQL(field: any): string {
2466+
const colType = this.mapFieldTypeToSQL(field);
2467+
return declaresColumnNotNull(field) ? `${colType} NOT NULL` : colType;
2468+
}
2469+
2470+
/**
2471+
* [#23847] Send one `ALTER TABLE … ADD COLUMN`, and when the database refuses
2472+
* it, say which statement was refused. The local face's error already names
2473+
* it — knex prefixes the failing SQL to the driver's message — while libSQL's
2474+
* own message (`Cannot add a NOT NULL column with default value NULL`) names
2475+
* no table and no column, and the engine's per-object report says only which
2476+
* object failed. Same error object, same class and code; only its message
2477+
* gains the statement, in knex's `<sql> - <message>` form.
2478+
*/
2479+
private async executeAddColumn(ddl: string): Promise<void> {
2480+
try {
2481+
await this.client!.execute(ddl);
2482+
} catch (e) {
2483+
if (e instanceof Error) e.message = `${ddl} - ${e.message}`;
2484+
throw e;
2485+
}
2486+
}
2487+
24082488
/**
24092489
* [#8413 · #17609] Every index an object's metadata declares, as executable
24102490
* DDL: field-level `unique` (tenancy-aware) PLUS the object's own

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -603,8 +603,10 @@ function refuseRemoteDeferredDdl(): never {
603603
* # What this face does NOT claim: a boot may not act on its report
604604
*
605605
* The report is accurate about the database, but not every finding is real
606-
* drift. This face's schema sync (`RemoteTransport`) does not create the NOT NULL
607-
* constraint a field declares, so a table it created reads as `tighten_not_null`
606+
* drift. A table this face's schema sync (`RemoteTransport`) created before
607+
* #23847 has no NOT NULL constraint for the fields that declare one: the sync
608+
* creates it now, but an additive sync never tightens a column that already
609+
* exists. So such a table reads as `tighten_not_null`
608610
* (destructive) against its own declaration. Nor is there a remedy for a
609611
* destructive entry on this transport: `os migrate apply` arms deferred DDL,
610612
* which this face refuses. So {@link TursoDriver.driftReportCaveat} says so,
@@ -615,7 +617,8 @@ function refuseRemoteDeferredDdl(): never {
615617

616618
/** [#23816] {@link TursoDriver.driftReportCaveat} on the REMOTE face. */
617619
const REMOTE_DRIFT_REPORT_CAVEAT =
618-
'the Turso REMOTE transport\'s schema sync does not create the NOT NULL constraint a field declares, so ' +
620+
'a table the Turso REMOTE transport\'s schema sync created in an earlier release has no NOT NULL ' +
621+
'constraint on the fields that declare one (the sync creates it now, but never tightens an existing column), so ' +
619622
'this driver\'s drift report can name a tightening its own sync produced, and a destructive entry has no ' +
620623
'remedy on this transport (`os migrate apply` refuses against it). The report is for reading; nothing is ' +
621624
'applied or refused on it unattended. To review the drift, run `os migrate plan` against a local SQLite ' +

0 commit comments

Comments
 (0)