diff --git a/.changeset/turso-remote-unique-ddl.md b/.changeset/turso-remote-unique-ddl.md new file mode 100644 index 0000000000..3b31ea8778 --- /dev/null +++ b/.changeset/turso-remote-unique-ddl.md @@ -0,0 +1,47 @@ +--- +"@objectstack/driver-turso": minor +--- + +fix(driver-turso): the remote face emits the declared `unique`, and an unbacked `conflictKeys` upsert refuses in an envelope (#8413) + +`RemoteTransport`'s DDL builder had no notion of `unique` at all, so a column +declared `{ type: 'string', unique: true }` reached a remote Turso endpoint as a +bare `"email" TEXT`. Two consequences of one cause, both fixed here: + +1. **Declared uniqueness was not enforced on the remote face.** Same driver, + same object definition, opposite answers: the local face rejected a duplicate + while the remote face accepted it and the duplicate landed. A remote + deployment that believed its `unique` declarations was accumulating + duplicates silently, and no read reported it. The remote face now emits a + companion `CREATE UNIQUE INDEX`, built from `uniqueIndexesFromFields` — the + same helper `SqlDriver` uses locally — so both faces produce the same index + name and the same key, including the ADR-0120 D1/D3 per-organization form. +2. **`conflictKeys` upserts could not work on remote at all.** SQLite requires + an `ON CONFLICT` target to be backed by a PRIMARY KEY or UNIQUE index; with + the index never created, every business-key upsert raised a raw `SqliteError` + (`code: 'SQLITE_ERROR'`, `status: undefined`) — not an ADR-0112 envelope. It + now answers `VALIDATION_ERROR` / 400 naming the object, the keys and the + remedy, with the SQLite text preserved as `cause`. + +**Who is affected.** Remote (`libsql://` / `https://`) Turso deployments only; +local and embedded-replica modes are unchanged. On a remote deployment a +duplicate write to a declared-unique column now **fails** where it previously +succeeded. That is the declaration being honoured rather than a new restriction +— the local face has always rejected it — but it is a real change in runtime +accept/reject and is the reason this is called out here rather than buried. + +**Existing tables are retrofitted, never migrated.** A table that already exists +gets its unique index created outside the schema-sync batch, so one table's +failure cannot roll back another object's DDL. If the table already holds the +duplicates the missing constraint admitted, the index cannot be created: that is +reported at `error` level naming the table and the remedy, the boot continues, +and `conflictKeys` upserts against it get the enveloped refusal above. **No +stored row is deleted, merged or rewritten** — resolving existing duplicates is +an operator decision, not a side effect of a driver booting. + +**On the bump.** The behaviour change alone would be `patch` by the usual +reading — it refuses a shape that was never entitled to an answer, since the +metadata declared the column unique. It is `minor` because `RemoteTransport` +(exported from the package root) gains two public wiring methods, +`setDurabilitySink` and `setTenantFieldResolver`. Nothing is removed, renamed or +narrowed. diff --git a/packages/drivers/driver-turso/src/remote-transport.ts b/packages/drivers/driver-turso/src/remote-transport.ts index 0c93c64ebf..be7f7cd05d 100644 --- a/packages/drivers/driver-turso/src/remote-transport.ts +++ b/packages/drivers/driver-turso/src/remote-transport.ts @@ -33,6 +33,21 @@ import { hasDanglingLikeEscape, likePatternToGlobPattern } from '@objectstack/sp // `AggregationNodeSchema.function` admits, nor from the local driver's twin. import { AggregationFunction } from '@objectstack/spec/data'; import type { DriverQuery } from '@objectstack/spec/contracts'; +// [#8413] What a `unique: true` FIELD becomes, from the one place that decides +// it. `uniqueIndexesFromFields`' own contract is that it is "the ONLY place +// field-level uniqueness becomes an index, so the create-table, alter-table, +// SQLite-rebuild and drift-detection paths cannot disagree about what a +// `unique: true` field is supposed to produce" — this transport is the fourth +// such path and now reads the same answer instead of re-deriving a second one. +// That is the whole anti-drift property: the index NAME and the KEY the remote +// face creates are byte-identical to the ones `SqlDriver` creates locally and +// the ones the drift differ looks for, so the two faces cannot fork on what the +// declaration meant (#6203, which this driver has already paid for twice). +import { + uniqueIndexesFromFields, + organizationKeyPartSql, + type ExpectedIndex, +} from '@objectstack/driver-sql'; import { nanoid } from 'nanoid'; /** @@ -547,6 +562,82 @@ function invalidFilterError(message: string): Error { return err; } +/** + * [#8413] Is this the "the conflict target is not a key" failure? + * + * The MESSAGE is the only channel SQLite fills for this condition — it is a + * plain `SQLITE_ERROR`, the same generic code a syntax error carries, so + * `code` cannot discriminate and a `code`-based test would either never fire or + * swallow every other statement failure as an unbacked conflict target. The + * text is SQLite's own, stable since the `ON CONFLICT` clause was introduced + * (3.24), and it is matched narrowly enough that no other failure shares it. + * + * ⚠️ Deliberately NOT `isUniqueViolationError` from `@objectstack/types`: that + * predicate answers the OPPOSITE condition (a unique index exists and the row + * violated it). Confusing the two would report a working constraint as a + * missing one. + */ +function isUnbackedConflictTargetError(error: unknown): boolean { + const message = error instanceof Error ? error.message : String(error ?? ''); + return /ON CONFLICT clause does not match any PRIMARY KEY or UNIQUE constraint/i.test(message); +} + +/** + * [#8413] A `conflictKeys` upsert whose target no unique index backs. + * + * # Why this refusal exists at all + * + * It is the safe answer for the case the DDL fix cannot reach: a remote table + * created BEFORE this driver emitted `unique` at all, whose rows may already + * contain the duplicates the missing constraint admitted. The index cannot be + * added over such a table without an operator first deciding what happens to + * those rows — a destructive migration this driver deliberately does not + * perform — so the honest outcome at the moment of use is a refusal that names + * the problem, rather than a crash the caller cannot classify. + * + * # Why `VALIDATION_ERROR` / 400 + * + * ADR-0112's own selection line is *ask what reads it — a client branching on + * why its call failed*. Such a client needs two facts: **do not retry** (the + * next attempt fails identically; nothing is transient, so a 5xx would be a + * lie that invites a retry storm), and **this is not a row-level conflict** + * (nothing collided — the merge could not be attempted at all, which is why it + * is not `RESOURCE_CONFLICT`/409 or `DUPLICATE_VALUE`). `VALIDATION_ERROR` is + * the catalogued generic for a request argument that did not validate against + * the target, and `conflictKeys` is exactly that argument. It is spelled from + * the enum rather than as a literal so a rename in `@objectstack/spec` breaks + * this build instead of shipping a code the schema no longer knows. + * + * `status: 400` is the other half, for the reason {@link invalidFilterError} + * records: it puts the rejection on `@objectstack/rest`'s expected-rejection + * list, so a caller's un-satisfiable upsert stops being logged as an unhandled + * SERVER error once per request. + * + * ⚠️ The original error is kept as `cause` rather than discarded — the SQLite + * text is the ground truth an operator debugging the table will want, and + * nothing above this layer can recover it once it is replaced. It is ASSIGNED + * rather than passed to the `Error` constructor: this package compiles against + * a lib target without the ES2022 `ErrorOptions` overload, so the two-argument + * form does not type-check here (measured — `tsc --noEmit` rejects it). + */ +function refuseUnbackedConflictTarget(object: string, mergeKeys: string[], cause: unknown): Error { + const keys = mergeKeys.map((k) => `"${k}"`).join(', '); + const err = new Error( + `Cannot upsert into "${object}" on conflict keys (${keys}): no PRIMARY KEY or UNIQUE index ` + + `backs them, so the merge target does not exist and SQLite refuses the statement. This is ` + + `usually a table created before its "unique" declaration was emitted as DDL, or conflict ` + + `keys naming columns that were never declared unique. Fix by declaring the column(s) ` + + `"unique: true" and re-running schema sync so the unique index is created — if the table ` + + `already holds duplicate values the index cannot be created until they are resolved, and ` + + `this driver does not rewrite stored rows to force it. Alternatively upsert on the primary ` + + `key by supplying "id" and omitting conflictKeys.`, + ) as Error & { code?: string; status?: number; cause?: unknown }; + err.code = StandardErrorCode.enum.VALIDATION_ERROR; + err.status = 400; + err.cause = cause; + return err; +} + /** * [#7929] The key `driver-sql` carries a REDACTED refusal's full diagnostic * under — the same global-registry symbol, deliberately, not a second one. @@ -868,6 +959,74 @@ export class RemoteTransport { this.diagnosticSink = sink; } + /** + * [#8413] Where a DURABILITY degradation is written — deliberately a SECOND + * sink, not a level flag on {@link diagnosticSink}. + * + * The two carry different classes and AGENTS.md's "Degradation log levels" + * rule grades them differently, by one question: *after the degradation, does + * the system still look normal from the outside while something it claims is + * enforced is not?* A redacted refusal's withheld half (#7929) answers **no** — + * the caller was told, loudly, and `warn` is right. A declared UNIQUE index + * that could not be created answers **yes**: every write keeps succeeding, + * every read keeps returning rows, and the only thing that changed is that a + * constraint the metadata declares is silently not enforced. That is the + * `error` class, and routing it through the `warn` sink would file it under + * exactly the level the rule exists to keep readable. + * + * Absent, the degradation is not lost: {@link syncUniqueIndexes} still skips + * the index and the condition resurfaces as the enveloped refusal in + * {@link upsert}. `TursoDriver` wires this to `logger.error` at construction, + * the same way it wires the connect factory and the temporal column rule. + */ + private durabilitySink: ((message: string) => void) | null = null; + + /** + * Register where this transport reports a declared constraint it could not + * materialize (#8413). See {@link durabilitySink} for why it is not the + * diagnostic sink. + */ + setDurabilitySink(sink: (message: string) => void): void { + this.durabilitySink = sink; + } + + /** + * [#8413] The driver's tenancy rule, asked rather than re-derived. + * + * `uniqueIndexesFromFields` scopes a field-level `unique` by the table's + * tenant column (ADR-0120 D1/D3): with one, `unique: true` means unique + * *within the organization* and materializes as the NULL-safe composite + * `(COALESCE(, '__global__'), )`; without one it is the plain + * single-column index. Getting that wrong in EITHER direction is a real + * defect — a platform-wide index where the local face builds a tenant-scoped + * one would reject two organizations legitimately holding the same value, + * which is a fresh divergence rather than the one being fixed. + * + * The answer lives on `SqlDriver.computeTenantField`, which is a pure + * function of the schema and already the single source of truth for + * `initObjects` and `registerExternalObject` (they inlined it once and + * drifted). This transport asks it instead of growing a third copy. + * + * ⚠️ It has to be a resolver over the SCHEMA, not a lookup by table name: + * remote DDL runs BEFORE `registerRemoteFieldMetadata` populates the driver's + * `tenantFieldByTable`, so at the moment this is called the by-table registry + * is still empty for the object being created. Absent (a transport driven + * standalone — not a supported composition), it degrades to `null`, which is + * the same "no tenant column" arm the shared helper already documents (S11). + */ + private tenantFieldResolver: ((schema: { fields?: Record; tenancy?: any }) => string | null) | null = + null; + + /** + * Register the driver's per-schema tenant-column rule (#8413) — see + * {@link tenantFieldResolver}. + */ + setTenantFieldResolver( + resolver: (schema: { fields?: Record; tenancy?: any }) => string | null, + ): void { + this.tenantFieldResolver = resolver; + } + /** * Set the @libsql/client instance used for all queries. */ @@ -1244,7 +1403,17 @@ export class RemoteTransport { sql += ` DO NOTHING`; } - await this.client!.execute({ sql, args: values }); + try { + await this.client!.execute({ sql, args: values }); + } catch (e) { + // [#8413] SQLite requires an `ON CONFLICT` target to be backed by a + // PRIMARY KEY or UNIQUE index, and answers a bare `SqliteError` + // (`code: 'SQLITE_ERROR'`, no `status`) when it is not — not an ADR-0112 + // envelope, so a caller could not branch on it and `mapDataError` shipped + // it as `{ "error": "" }` with no code at all. + if (isUnbackedConflictTargetError(e)) throw refuseUnbackedConflictTarget(object, mergeKeys, e); + throw e; + } // Fetch the result row const result = await this.client!.execute({ @@ -1377,14 +1546,22 @@ export class RemoteTransport { if (!exists) { await this.client!.execute(this.buildCreateTableSQL(tableName, objectDef)); + // [#8413] The table was created empty microseconds ago, so its unique + // indexes cannot fail on existing data — no isolation needed, and any + // error here is a real DDL fault that should surface. + for (const sql of this.buildUniqueIndexDDL(tableName, objectDef, this.materializedColumns(objectDef))) { + await this.client!.execute(sql); + } } else { // ALTER TABLE — add missing columns + const materialized = new Set(BUILTIN_COLUMNS); if (objectDef.fields) { const columnsResult = await this.client!.execute({ sql: `PRAGMA table_info("${tableName}")`, args: [], }); const existingColumns = new Set(columnsResult.rows.map((r: any) => r.name)); + for (const c of existingColumns) materialized.add(String(c)); for (const [name, field] of Object.entries(objectDef.fields)) { if (existingColumns.has(name)) continue; @@ -1393,9 +1570,30 @@ export class RemoteTransport { this.assertSafeIdentifier(name); const colType = this.mapFieldTypeToSQL(field); await this.client!.execute(`ALTER TABLE "${tableName}" ADD COLUMN "${name}" ${colType}`); + materialized.add(name); } } + // [#8413] The retrofit leg — this table may already hold the duplicates + // the missing constraint admitted, so it is isolated and reported, never + // forced. See {@link syncUniqueIndexes}. + await this.syncUniqueIndexes(this.buildUniqueIndexDDL(tableName, objectDef, materialized)); + } + } + + /** + * [#8413] The columns a freshly-built `CREATE TABLE` actually materializes — + * the builtins plus every non-virtual declared field, mirroring + * {@link buildCreateTableSQL} exactly. Kept beside it so a change to what the + * builder emits cannot leave the index path indexing a column that no longer + * exists. + */ + private materializedColumns(objectDef: { fields?: Record }): Set { + const columns = new Set(BUILTIN_COLUMNS); + for (const [name, field] of Object.entries(objectDef.fields ?? {})) { + if (((field as any)?.type || 'string') === 'formula') continue; + columns.add(name); } + return columns; } /** @@ -1445,8 +1643,25 @@ export class RemoteTransport { for (const { object, schema } of newSchemas) { const objectDef = schema as { name: string; fields?: Record }; ddlStatements.push(this.buildCreateTableSQL(object, objectDef)); + // [#8413] Rides the SAME batch, immediately behind its own CREATE TABLE + // (order matters — the index cannot precede the table). A brand-new table + // is empty, so these cannot fail on existing data and need none of the + // isolation the retrofit leg below gets: they belong in the batch, and + // cost this path zero extra round trips. + ddlStatements.push( + ...this.buildUniqueIndexDDL(object, objectDef, this.materializedColumns(objectDef)), + ); } + // [#8413] Existing tables' unique indexes are collected here and applied + // AFTER the main batch — they must follow their table's `ALTER TABLE ADD + // COLUMN` (the column may be brand new), and they must not share a + // transaction with it: on a libsql `write` batch one statement's failure + // rolls back every other statement in the batch, so a single table holding + // duplicates would silently undo the schema sync of every OTHER object in + // the boot. That is the blast radius the separation exists to prevent. + const retrofitStatements: string[] = []; + // Phase 2b: for existing tables, introspect columns in one batch if (existingSchemas.length > 0) { const pragmaStmts: InStatement[] = existingSchemas.map((s) => ({ @@ -1461,6 +1676,8 @@ export class RemoteTransport { if (!objectDef.fields) continue; const existingColumns = new Set(pragmaResults[i].rows.map((r: any) => r.name)); + const materialized = new Set(BUILTIN_COLUMNS); + for (const c of existingColumns) materialized.add(String(c)); for (const [name, field] of Object.entries(objectDef.fields)) { if (existingColumns.has(name)) continue; @@ -1469,7 +1686,10 @@ export class RemoteTransport { this.assertSafeIdentifier(name); const colType = this.mapFieldTypeToSQL(field); ddlStatements.push(`ALTER TABLE "${object}" ADD COLUMN "${name}" ${colType}`); + materialized.add(name); } + + retrofitStatements.push(...this.buildUniqueIndexDDL(object, objectDef, materialized)); } } @@ -1477,6 +1697,11 @@ export class RemoteTransport { if (ddlStatements.length > 0) { await this.client!.batch(ddlStatements, 'write'); } + + // Phase 4 [#8413]: retrofit the existing tables' declared unique indexes, + // outside the batch above for the blast-radius reason stated at its + // declaration. Failures here are reported, never forced and never repaired. + await this.syncUniqueIndexes(retrofitStatements); } async dropTable(object: string): Promise { @@ -1553,6 +1778,139 @@ export class RemoteTransport { return sql; } + /** + * [#8413] The UNIQUE indexes a schema's field-level `unique` declarations ask + * for, as executable DDL. + * + * # Why a companion index and not an inline `UNIQUE` column constraint + * + * Both spellings enforce the same thing on a NEW table, so the choice is + * decided by the two cases that are not new-table, and both point the same + * way: + * + * 1. **Retrofit.** SQLite cannot add a column constraint to an existing + * table — `ALTER TABLE` has no `ADD CONSTRAINT`, so an inline `UNIQUE` + * reaches an already-created table only through a full table rebuild + * (create-copy-drop-rename). A `CREATE UNIQUE INDEX` is a single + * statement that touches no row. Since the tables this defect has been + * filling with duplicates all already exist, the inline form would have + * made the fix unreachable exactly where it is needed. + * 2. **Parity.** `SqlDriver` materializes field-level `unique` as a UNIQUE + * INDEX (`syncDeclaredIndexes`), never inline. Matching it means the two + * faces converge on the same index NAME (`buildIndexName`) and the same + * key, so `sqlite_master` on a remote database and on a local one read + * alike — and the drift differ, which looks for those names, does not + * report a remote database as drifted from its own declaration. + * + * # NULL semantics are inherited, not chosen here + * + * SQL UNIQUE is NULL-distinct, so rows with a NULL in the key stay mutually + * unconstrained; the tenant-scoped arm gets the NULL-SAFE key part + * (`COALESCE(, '__global__')`) from the shared helper for the reason + * ADR-0120 D3 records. Neither rule is re-decided here. + * + * Columns that were never materialized (a virtual `formula` field) are + * skipped rather than emitted — the same choice `SqlDriver.syncDeclaredIndexes` + * makes, and for the same reason: DDL naming a column that does not exist + * fails the whole sync over an index nothing could have used. + */ + private buildUniqueIndexDDL( + tableName: string, + objectDef: { fields?: Record; tenancy?: any }, + materializedColumns: Set, + ): string[] { + const tenantField = this.tenantFieldResolver ? this.tenantFieldResolver(objectDef) : null; + const expected: ExpectedIndex[] = uniqueIndexesFromFields( + tableName, + objectDef.fields ?? {}, + tenantField, + ); + + const statements: string[] = []; + for (const index of expected) { + const missing = index.columns.filter((c) => !materializedColumns.has(c)); + if (missing.length > 0) { + this.diagnosticSink?.( + `[RemoteTransport] skipping declared unique index on "${tableName}" — ` + + `column(s) not materialized: ${missing.join(', ')}`, + ); + continue; + } + this.assertSafeIdentifier(index.name); + for (const column of index.columns) this.assertSafeIdentifier(column); + + const nullSafe = new Set(index.nullSafeColumns ?? []); + const parts = index.columns.map((c) => + nullSafe.has(c) ? organizationKeyPartSql(`"${c}"`) : `"${c}"`, + ); + // `IF NOT EXISTS` is what makes every sync after the first a no-op + // server-side, so re-syncing an object costs a statement rather than an + // error — and it is also what makes the per-statement retry in + // {@link syncUniqueIndexes} safe under EITHER libsql batch semantic + // (transactional: nothing was applied; non-transactional: re-applying is + // a no-op). + statements.push( + `CREATE UNIQUE INDEX IF NOT EXISTS "${index.name}" ON "${tableName}" (${parts.join(', ')})`, + ); + } + return statements; + } + + /** + * [#8413] Materialize unique indexes against a table that ALREADY EXISTS — + * the retrofit path, and the one that can legitimately fail. + * + * ⛔ **This must never repair data, and never gives up quietly.** Creating a + * UNIQUE index over a table that already holds duplicates fails, and those + * duplicates are precisely what this defect has been producing. Deleting, + * merging or rewriting any of those rows is a destructive migration and an + * operator's decision — never a side effect of a driver booting. So the only + * two outcomes here are *the index now exists* and *the index does not exist + * and somebody was told at `error`*, which is the level AGENTS.md's + * degradation rule requires for a declared constraint that is not enforced: + * from the outside nothing looks wrong, and the loss surfaces a release later. + * + * Reporting it is not the whole remedy, and is not meant to be — the operator + * with duplicates also gets {@link refuseUnbackedConflictTarget} on any + * `conflictKeys` upsert against the same table, which is a refusal at the + * moment of use rather than a log line at boot. + * + * **Round-trip cost, stated rather than hidden** (#7099 asked which trips are + * already paid): the happy path is ONE extra batch per sync that touches an + * existing table, and only when that table declares a unique field at all. + * The per-statement fallback runs only after a batch has already failed, i.e. + * only on a database that really does have a violated constraint — so the + * cost of precision is paid by the deployment that needs the diagnosis, not + * by every boot. + */ + private async syncUniqueIndexes(statements: string[]): Promise { + if (statements.length === 0) return; + try { + await this.client!.batch(statements, 'write'); + return; + } catch { + // The batch told us SOMETHING failed, not which. Re-issue one at a time + // so the report names the index an operator has to act on — `IF NOT + // EXISTS` makes the ones that already succeeded no-ops either way. + } + for (const sql of statements) { + try { + await this.client!.execute(sql); + } catch (e) { + this.durabilitySink?.( + `[RemoteTransport] could not create the declared unique index — ` + + `${sql}. The constraint is NOT enforced on this table: existing rows already ` + + `violate it (the duplicates this face accepted while it emitted no UNIQUE at all), ` + + `or the index is otherwise unbuildable. Nothing looks broken from the outside and ` + + `duplicates will keep accumulating. Fix by de-duplicating the column's existing ` + + `values and re-running schema sync — this driver deliberately does NOT rewrite ` + + `stored rows to force the index through. Until then a conflictKeys upsert on this ` + + `table is refused rather than crashing. Cause: ${e instanceof Error ? e.message : String(e)}`, + ); + } + } + } + /** * Map ObjectStack field types to SQLite column types for DDL. */ diff --git a/packages/drivers/driver-turso/src/turso-driver.ts b/packages/drivers/driver-turso/src/turso-driver.ts index a0dbc48c12..6804596e30 100644 --- a/packages/drivers/driver-turso/src/turso-driver.ts +++ b/packages/drivers/driver-turso/src/turso-driver.ts @@ -385,6 +385,27 @@ export class TursoDriver extends SqlDriver { // cannot lose them by changing its connection string. this.remoteTransport.setDiagnosticSink((message) => this.logger.warn(message)); + // [#8413] A declared UNIQUE index the remote face could not create is a + // DURABILITY degradation, not a functional one: writes keep succeeding, + // reads keep returning rows, and the only thing that changed is that a + // constraint the metadata declares is not enforced — the "looks normal + // from the outside" shape AGENTS.md grades at `error`. It is a SEPARATE + // sink from the `warn` one above precisely so this class does not have to + // share a level with the diagnostics that are merely informative. + this.remoteTransport.setDurabilitySink((message) => + (this.logger.error ?? this.logger.warn).call(this.logger, message), + ); + + // [#8413] The tenancy rule behind a field-level `unique`, handed down + // rather than re-derived. `computeTenantField` is `SqlDriver`'s single + // source of truth (ADR-0120 D1/D3) and is a pure function of the schema, + // which is what the transport needs: remote DDL runs BEFORE + // `registerRemoteFieldMetadata` fills `tenantFieldByTable`, so a lookup + // by table name would read empty at exactly the moment the index is built + // and would silently emit a platform-wide unique where the local face + // builds a per-organization one. + this.remoteTransport.setTenantFieldResolver((schema) => this.computeTenantField(schema)); + // Register a lazy-connect factory so the transport can self-heal when // connect() was never called, failed on first attempt, or the client // was lost (e.g. serverless cold-start, transient network error). diff --git a/packages/drivers/driver-turso/src/turso-local-remote-unique-parity.test.ts b/packages/drivers/driver-turso/src/turso-local-remote-unique-parity.test.ts new file mode 100644 index 0000000000..c101360ed8 --- /dev/null +++ b/packages/drivers/driver-turso/src/turso-local-remote-unique-parity.test.ts @@ -0,0 +1,280 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8413] ONE `TursoDriver`, ONE answer on `unique` — the two faces held + * against each other on a declared-unique column, and the `conflictKeys` + * upsert that rests on it. + * + * # What was broken + * + * `RemoteTransport.buildCreateTableSQL` had no notion of `unique` at all + * (`grep -ciE 'unique'` over the whole 2986-line file returned **zero**), so a + * column declared `{ type: 'string', unique: true }` reached a remote Turso + * endpoint as a bare `"email" TEXT`. Two consequences, one cause: + * + * 1. **Declared uniqueness was not enforced on this face.** The same object + * definition, the same duplicate write: LOCAL rejected it, REMOTE accepted + * it and the duplicate landed. A remote deployment that believed its + * `unique` declarations was accumulating duplicates silently, and no read + * reported it — Prime Directive #10's shape (declared ≠ enforced) on the + * load-bearing kind. + * 2. **`conflictKeys` upserts could not work on remote at all.** SQLite + * requires an `ON CONFLICT` target to be backed by a PRIMARY KEY or UNIQUE + * index; with the index never created, every business-key upsert raised a + * raw `SqliteError` (`code: 'SQLITE_ERROR'`, `status: undefined`) — not an + * ADR-0112 envelope, so a caller could not branch on it either. + * + * # Why this file, and not a case in the remote suite + * + * #6203's lesson, which this driver has already paid for twice (#5903 `$not` + * NULL-safety, #5769 `$`-operator keys): a fix that lands on one face is two + * answers. A per-transport suite cannot fail on the DIFFERENCE — a divergence + * shows up as one file red and the other green, in whichever order someone + * reads them. The divergence itself is the defect, so the divergence is what is + * pinned: same declaration, same write, both faces, one assertion. + * + * # The three pins, and what each one alone would miss + * + * - **The DDL pin** asserts the UNIQUE index the remote face emits for a + * `unique: true` column, by name and by key. Without it a future rewrite of + * the builder drops the key again and only the behavioural pins notice — + * from two layers away. + * - **The divergence pin** asserts both faces reject the duplicate AND that + * exactly one row survives. Asserting only "it throws" would be satisfied by + * a transport that threw for an unrelated reason; the row count is the + * contract, and it is the number that was actually wrong. + * - **The refusal pin** asserts `code` AND `status` (the ADR-0112 envelope) on + * an upsert whose conflict target has no backing unique index — the + * already-created-table case that (1) cannot retroactively cover. A bare + * `toThrow()` here is blind in both directions: it stays green on the raw + * `SqliteError` this card exists to remove. + * + * The refusal pin carries a **positive control** beside it: with the unique + * index present, the same `conflictKeys` upsert MERGES. Without that half, a + * transport that refused every `conflictKeys` upsert unconditionally would pass + * the refusal pin — and would have broken the capability instead of fixing it. + * + * # Reverse verification, direction predicted BEFORE it was run + * + * Predicted: reverting `buildCreateTableSQL`'s unique-index emission turns the + * DDL pin and the remote half of the divergence pin red, and — the direction + * worth predicting — turns the refusal pin's POSITIVE CONTROL red too, because + * the merge it asserts is precisely what the missing index made impossible. + * The refusal pin's negative half stays GREEN across the revert: its table + * never had a unique index to lose, which is what makes it the pin for the + * already-created-table case rather than a second copy of the DDL pin. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { StandardErrorCode } from '@objectstack/spec/api'; +import { TursoDriver } from './turso-driver.js'; +import { asLibsqlClient, makeLibsqlSqliteStub, type LibsqlSqliteStub } from './libsql-sqlite-stub.testkit.js'; + +/** + * The card's own object, verbatim: one declared-unique business key beside an + * ordinary column, and no tenant column — so `uniqueIndexesFromFields` resolves + * to the single-column `(email)` form and the fixture reads as the card wrote it. + */ +const CONTACT = { + name: 'crm_contact', + fields: { + email: { type: 'string', unique: true }, + title: { type: 'string' }, + }, +} as const; + +/** The same object with the declaration REMOVED — the un-backed conflict target. */ +const CONTACT_NO_UNIQUE = { + name: 'crm_contact_plain', + fields: { + email: { type: 'string' }, + title: { type: 'string' }, + }, +} as const; + +/** The error a refusal produced — never a bare `toThrow()` (ADR-0112). */ +interface WireBearingError extends Error { + code?: string; + status?: number; +} + +const captureError = async (fn: () => Promise): Promise => { + try { + await fn(); + return null; + } catch (e) { + return e as WireBearingError; + } +}; + +describe('[#8413] `unique` is enforced on BOTH TursoDriver faces', () => { + let local: TursoDriver; + let remote: TursoDriver; + let stub: LibsqlSqliteStub; + + beforeEach(async () => { + local = new TursoDriver({ url: ':memory:' }); + expect(local.transportMode).toBe('local'); + + stub = makeLibsqlSqliteStub(); + remote = new TursoDriver({ url: 'libsql://unique-parity.turso.io', client: asLibsqlClient(stub) }); + await remote.connect(); + expect(remote.transportMode).toBe('remote'); + }); + + afterEach(async () => { + await local.disconnect(); + await remote.disconnect(); + stub.close(); + }); + + // ───────────────────────────────────────────────────────────────────── + // Pin 1 — the DDL the remote face actually emits + // ───────────────────────────────────────────────────────────────────── + + describe('the remote DDL carries the declared uniqueness', () => { + it('emits a UNIQUE index over a `unique: true` column on CREATE', async () => { + await remote.initObjects([{ ...CONTACT, fields: { ...CONTACT.fields } }]); + + const indexes = stub.raw + .prepare(`SELECT name, sql FROM sqlite_master WHERE type='index' AND tbl_name='crm_contact'`) + .all() as Array<{ name: string; sql: string | null }>; + + // Named by the SHARED `buildIndexName` (driver-sql's `schema-drift`), so + // the two faces converge on one identifier rather than two spellings. + const unique = indexes.filter((i) => typeof i.sql === 'string' && /CREATE UNIQUE INDEX/i.test(i.sql!)); + expect(unique.map((i) => i.name)).toContain('uniq_crm_contact_email'); + expect(unique.find((i) => i.name === 'uniq_crm_contact_email')!.sql).toMatch(/\("email"\)/); + + // …and NOT over the column that never declared it. + expect(unique.some((i) => /"title"/.test(i.sql ?? ''))).toBe(false); + }); + + it('emits it on the single-object `syncSchema` path too, not just the batch path', async () => { + // Both paths build DDL; `syncSchema` is the one a lifecycle re-registration + // and the archive path reach. A fix that landed only in `syncSchemasBatch` + // would leave this face half-covered and look green on the suite above. + await remote.syncSchema(CONTACT.name, { ...CONTACT, fields: { ...CONTACT.fields } }); + + const names = ( + stub.raw + .prepare(`SELECT name FROM sqlite_master WHERE type='index' AND tbl_name='crm_contact'`) + .all() as Array<{ name: string }> + ).map((i) => i.name); + expect(names).toContain('uniq_crm_contact_email'); + }); + + it('is idempotent — re-syncing an already-synced object does not fail', async () => { + await remote.initObjects([{ ...CONTACT, fields: { ...CONTACT.fields } }]); + await expect( + remote.initObjects([{ ...CONTACT, fields: { ...CONTACT.fields } }]), + ).resolves.not.toThrow(); + }); + }); + + // ───────────────────────────────────────────────────────────────────── + // Pin 2 — the divergence itself: same declaration, same write, both faces + // ───────────────────────────────────────────────────────────────────── + + describe('a duplicate on a declared-unique column is refused on BOTH faces', () => { + it('rejects the duplicate and keeps exactly one row, local and remote alike', async () => { + await local.initObjects([{ ...CONTACT, fields: { ...CONTACT.fields } }]); + await remote.initObjects([{ ...CONTACT, fields: { ...CONTACT.fields } }]); + + await local.create(CONTACT.name, { email: 'a@b.com', title: 'first' }, { bypassTenantAudit: true }); + await remote.create(CONTACT.name, { email: 'a@b.com', title: 'first' }); + + const localErr = await captureError(() => + local.create(CONTACT.name, { email: 'a@b.com', title: 'second' }, { bypassTenantAudit: true }), + ); + const remoteErr = await captureError(() => remote.create(CONTACT.name, { email: 'a@b.com', title: 'second' })); + + // The divergence in one assertion: neither face may accept it. + expect({ local: localErr !== null, remote: remoteErr !== null }).toEqual({ local: true, remote: true }); + + // Both refusals are the UNIQUE constraint, not some unrelated failure that + // would satisfy "it threw" while the real defect walked past. + expect(String(localErr?.message)).toMatch(/UNIQUE constraint failed/i); + expect(String(remoteErr?.message)).toMatch(/UNIQUE constraint failed/i); + + // …and the number that was actually wrong: the duplicate did not land. + expect(await local.count(CONTACT.name, {})).toBe(1); + expect(await remote.count(CONTACT.name, {})).toBe(1); + }); + + it('still accepts distinct values on both faces — the constraint is not a blanket refusal', async () => { + await local.initObjects([{ ...CONTACT, fields: { ...CONTACT.fields } }]); + await remote.initObjects([{ ...CONTACT, fields: { ...CONTACT.fields } }]); + + for (const email of ['a@b.com', 'c@d.com', 'e@f.com']) { + await local.create(CONTACT.name, { email }, { bypassTenantAudit: true }); + await remote.create(CONTACT.name, { email }); + } + expect(await local.count(CONTACT.name, {})).toBe(3); + expect(await remote.count(CONTACT.name, {})).toBe(3); + }); + }); + + // ───────────────────────────────────────────────────────────────────── + // Pin 3 — the enveloped refusal, and the capability it guards + // ───────────────────────────────────────────────────────────────────── + + describe('a `conflictKeys` upsert with no backing unique index refuses in the ADR-0112 envelope', () => { + it('answers a real `code` and `status` — never a raw SqliteError', async () => { + await remote.initObjects([{ ...CONTACT_NO_UNIQUE, fields: { ...CONTACT_NO_UNIQUE.fields } }]); + + const err = await captureError(() => + remote.upsert(CONTACT_NO_UNIQUE.name, { email: 'a@b.com', title: 'x' }, ['email']), + ); + + expect(err).not.toBeNull(); + // The measured defect: `code: 'SQLITE_ERROR'`, `status: undefined`. + // Both halves of the envelope, never a bare `toThrow()` (ADR-0112). + expect(err!.code).toBe(StandardErrorCode.enum.VALIDATION_ERROR); + expect(err!.status).toBe(400); + expect(err!.code).not.toBe('SQLITE_ERROR'); + + // The SQLite text an operator debugging the table needs is preserved + // rather than replaced — the refusal adds a classification, it does not + // destroy the ground truth. + expect(String((err as unknown as { cause?: Error }).cause?.message)).toMatch( + /ON CONFLICT clause does not match/i, + ); + + // The message names the problem an operator has to act on: which object, + // which keys, and that the remedy is the index — not a retry. + expect(err!.message).toMatch(/crm_contact_plain/); + expect(err!.message).toMatch(/email/); + expect(err!.message).toMatch(/unique/i); + }); + + /** + * The positive control. Without it, a transport that refused EVERY + * `conflictKeys` upsert would pass the assertion above — having broken the + * capability rather than restored it. This is the half that proves the + * refusal is a diagnosis and not a blanket. + */ + it('MERGES when the declared unique index does back the conflict target', async () => { + await remote.initObjects([{ ...CONTACT, fields: { ...CONTACT.fields } }]); + + await remote.upsert(CONTACT.name, { email: 'a@b.com', title: 'first' }, ['email']); + await remote.upsert(CONTACT.name, { email: 'a@b.com', title: 'second' }, ['email']); + + const rows = await remote.find(CONTACT.name, {}); + expect(rows).toHaveLength(1); + expect(rows[0].title).toBe('second'); + }); + + it('leaves the default `id` merge key working — no conflictKeys, no refusal', async () => { + await remote.initObjects([{ ...CONTACT, fields: { ...CONTACT.fields } }]); + + const created = await remote.upsert(CONTACT.name, { id: 'fixed_id', email: 'a@b.com', title: 'first' }); + expect(created.id).toBe('fixed_id'); + await remote.upsert(CONTACT.name, { id: 'fixed_id', email: 'a@b.com', title: 'second' }); + + const rows = await remote.find(CONTACT.name, {}); + expect(rows).toHaveLength(1); + expect(rows[0].title).toBe('second'); + }); + }); +});