diff --git a/.changeset/sqlite-rebuild-applied-honesty.md b/.changeset/sqlite-rebuild-applied-honesty.md new file mode 100644 index 0000000000..06a2040c3d --- /dev/null +++ b/.changeset/sqlite-rebuild-applied-honesty.md @@ -0,0 +1,17 @@ +--- +"@objectstack/driver-sql": patch +--- + +**Fix:** on SQLite, `applyMigrationEntries` no longer reports an op as **applied** just because a table rebuild ran (#11722). + +`SqlDriver.applyMigrationEntries` splits by dialect, and the two arms disagreed about what `applied` means. The in-place arm (Postgres / MySQL) asks per entry and believes the answer — `applyDriftOpInPlace` returns `false` for an op its dialect cannot perform, and the entry goes to `skipped`. The SQLite arm did not ask at all: it called `rebuildSqliteTablePatched(table, ents)` and then `applied.push(...ents)`, every entry, unconditionally. But that rebuild honours exactly four op types — `relax_not_null`, `tighten_not_null`, `drop_column`, `drop_column_default` — and silently ignores everything else; its own docblock already said so for the varchar ops. An ignored op was still reported applied. + +**The failure mode is a false green, not an error.** Nothing throws and nothing is skipped, so every consumer announces work that never happened: `reconcileAndWarnDrift` logs `auto-reconciled on `, and the artifact boot gate prints `↪ migrated `. The finding is still physically present, so the next boot detects it again, reports drift again, and "migrates" it again — a loop with no failing signal anywhere in it. + +**What changes.** `rebuildSqliteTablePatched` now returns the entries it actually acted on, built in the same pass that fills the four column sets it already partitioned into — deliberately not a second list of op types to keep in sync, so the returned set cannot drift from the work done. The caller reports those as `applied` and routes the remainder to `skipped`, logging it in the **same sentence** the in-place arm uses for an op its dialect cannot do (` on
. is unsupported on dialect 'sqlite' — skipped`), so one greppable line covers all three dialects. `@objectstack/driver-sqlite-wasm` and `@objectstack/driver-turso` extend `SqlDriver` without overriding either method, so both inherit the correction. + +**What deliberately does not change.** No op does anything different — this moves only what is *reported*. In particular the rebuild still runs for the whole table even when it honours nothing: it re-materializes every kept column's default (#11321, #4560) and the full declared index set from metadata (#3696), so it is not a no-op, and suppressing it would change what the reconciler DOES rather than what it says it did. `applied`/`skipped` remains a reported partition consumed by log lines and CLI counts; it is not an accept/reject door, and no public surface widens. + +**Latent when found, and fixed anyway.** The gap was unreachable at the time of the fix, held closed from two independent directions neither of which knew it was holding it: `enforcesVarcharLength` excludes SQLite, so the differ never emits `widen_varchar`/`narrow_varchar` there, and `multiValueColumnTypeIsLoadBearing` excludes SQLite for an unrelated measured reason, so #11535's `manual_column_type_change` is never emitted there either. The next column op that is not SQLite-rebuildable would have opened it silently. `manual_column_type_change`'s own docblock states that `applyMigrationEntries` reports it "skipped, never applied" — measured on Postgres and MySQL; that sentence is now also true on SQLite, and the docblock says so. + +Pinned by `packages/drivers/driver-sql/src/sql-driver-11722-sqlite-rebuild-applied-honesty.test.ts`, which constructs the reachability rather than waiting for it — it substitutes only the differ's dialect guard, handing entries straight to the public `applyMigrationEntries` seam that `os migrate apply` and the artifact boot gate both call, with a real driver, dialect and database throughout. All five cases fail on the pre-fix tree, including the consumer-level one that catches `auto-reconciled` being logged for an op that never happened. diff --git a/packages/drivers/driver-sql/src/schema-drift.ts b/packages/drivers/driver-sql/src/schema-drift.ts index 00e154b8c7..eb68d9ab90 100644 --- a/packages/drivers/driver-sql/src/schema-drift.ts +++ b/packages/drivers/driver-sql/src/schema-drift.ts @@ -161,7 +161,11 @@ export type DriftOp = * Measured consequence of having no arm, on live Postgres 16.13 (the same on * MySQL 8.0.46): `applyDriftOpInPlace` matches no case and returns `false`, * so `applyMigrationEntries` reports the entry as **skipped, never applied**, - * and logs it. That is the intended behaviour, not a gap to close. + * and logs it. That is the intended behaviour, not a gap to close. SQLite + * reaches the same verdict by a different road (#11722): its arm rebuilds the + * table and reports as applied only the entries the rebuild actually acted + * on, so an op with no arm is skipped there too rather than announced as + * migrated. * * `from`/`to` are the physical and declared type words, carried so a renderer * can show the divergence without re-deriving it from the message. diff --git a/packages/drivers/driver-sql/src/sql-driver-11722-sqlite-rebuild-applied-honesty.test.ts b/packages/drivers/driver-sql/src/sql-driver-11722-sqlite-rebuild-applied-honesty.test.ts new file mode 100644 index 0000000000..2d0a4da729 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-11722-sqlite-rebuild-applied-honesty.test.ts @@ -0,0 +1,307 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#11722] On SQLite, `applied` must mean "this op happened" — not "a rebuild ran". + * + * ## The defect + * + * `SqlDriver.applyMigrationEntries` splits by dialect and the two arms + * disagreed about what `applied` means. The in-place arm asks per entry and + * believes the answer (`applyDriftOpInPlace` returns `false` for an op its + * dialect cannot do, and the entry goes to `skipped`). The SQLite arm did not + * ask at all: + * + * ```ts + * await this.rebuildSqliteTablePatched(table, ents); + * applied.push(...ents); // every entry, unconditionally + * ``` + * + * `rebuildSqliteTablePatched` honours exactly four op types — `relax_not_null`, + * `tighten_not_null`, `drop_column`, `drop_column_default` — and silently + * ignores everything else; its own docblock says so for the varchar ops. An + * ignored op was still pushed into `applied`. + * + * ## Why the failure mode is a FALSE GREEN, not an error + * + * Nothing throws and nothing is skipped, so every consumer announces work that + * never happened: `reconcileAndWarnDrift` logs `auto-reconciled on + *
`, and the artifact boot gate prints `↪ migrated `. The finding is + * still physically present, so the NEXT boot detects it again, reports drift + * again, and "migrates" it again — a loop with no failing signal anywhere in + * it. §4 pins that consumer-visible symptom directly. + * + * ## Why a test that passes on `main` would prove nothing here + * + * The gap is LATENT: it is held closed from two independent directions, neither + * aware it is holding it. `enforcesVarcharLength` excludes SQLite, so the differ + * never emits `widen_varchar`/`narrow_varchar` there; and + * `multiValueColumnTypeIsLoadBearing` excludes SQLite for an unrelated MEASURED + * reason (a stale textual column does not corrupt the value there), so #11535's + * `manual_column_type_change` is never emitted on SQLite either. + * + * So this suite CONSTRUCTS the reachability instead of waiting for it. It + * substitutes exactly one thing and nothing else — the differ's dialect guard — + * by handing the entries straight to `applyMigrationEntries`, which is the + * public seam `os migrate apply` (`packages/cli/src/commands/migrate/apply.ts`) + * and the artifact boot gate (`packages/cli/src/utils/artifact-boot-migration.ts`) + * both call with differ output. Driver, dialect and database are real + * throughout. Every assertion below FAILS on the pre-fix tree, where the + * ignored entries arrive in `applied`. + * + * ## What this suite deliberately does NOT assert + * + * That the rebuild is skipped when it honours nothing. The rebuild + * re-materializes every kept column's default and the whole declared index set + * from metadata, so it is not a no-op — suppressing it would change what the + * reconciler DOES rather than what it REPORTS, which is a different question + * from this one. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { SqlDriver } from './sql-driver.js'; +import type { DriftOp, ManagedDriftEntry } from './schema-drift.js'; + +describe('SqlDriver SQLite reconcile reports only what the rebuild honoured (#11722)', () => { + let knexInstance: any; + + const makeDriver = (opts: any = {}, Ctor: typeof SqlDriver = SqlDriver) => { + const d = new Ctor({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + ...opts, + }); + knexInstance = (d as any).knex; + (d as any).logger = { warn: vi.fn(), info: vi.fn() }; + return d; + }; + + afterEach(async () => { + await knexInstance?.destroy(); + knexInstance = undefined; + }); + + /** A drift entry carrying `op`, exactly as the differ would hand one over. */ + const entryFor = (op: DriftOp, category: ManagedDriftEntry['category']): ManagedDriftEntry => + ({ + kind: 'type_mismatch', + severity: 'error', + table: op.table, + column: (op as { column?: string }).column, + category, + op, + message: `${op.type} on ${op.table}.${(op as { column?: string }).column}`, + }) as ManagedDriftEntry; + + const logLines = (driver: SqlDriver): string[] => { + const l = (driver as any).logger; + return [...l.warn.mock.calls, ...l.info.mock.calls].map((c: any[]) => String(c[0])); + }; + + // ──────────────────────────────────────────────────────────────────── + // §1 — the op that already documents its own answer: no reconciler arm + // ──────────────────────────────────────────────────────────────────── + describe('§1 an op the rebuild does not honour is reported skipped, not applied', () => { + it('`manual_column_type_change` — the op whose docblock says "skipped, never applied" — is skipped on SQLite too', async () => { + const driver = makeDriver(); + await driver.initObjects([{ name: 'proj_task', fields: { tags: { type: 'string' } } }]); + await knexInstance('proj_task').insert({ id: '1', tags: '["a","b"]' }); + + const before = await knexInstance('proj_task').columnInfo(); + + const op: DriftOp = { + type: 'manual_column_type_change', + table: 'proj_task', + column: 'tags', + from: 'varchar', + to: 'json', + }; + const { applied, skipped } = await driver.applyMigrationEntries([entryFor(op, 'needs_confirm')], { + allowDestructive: false, + }); + + // The whole point: the entry is reported for what actually happened to it. + expect(applied).toHaveLength(0); + expect(skipped.map((d) => d.op.type)).toEqual(['manual_column_type_change']); + + // …and "what actually happened to it" is: nothing. The physical column is + // byte-for-byte the one it was, which is why reporting it applied was a + // lie rather than a naming quibble. + expect(await knexInstance('proj_task').columnInfo()).toEqual(before); + + // Reported in the SAME words the in-place arm uses for an op its dialect + // cannot perform — one greppable sentence across all three dialects. + expect(logLines(driver).some((m) => /manual_column_type_change on proj_task\.tags is unsupported on dialect 'sqlite' — skipped/.test(m))).toBe(true); + }); + + it('`widen_varchar` — a SAFE op the rebuild ignores by design — is skipped, so dev auto-reconcile cannot announce it', async () => { + const driver = makeDriver(); + await driver.initObjects([{ name: 'proj_task', fields: { title: { type: 'string', maxLength: 60 } } }]); + + const op: DriftOp = { type: 'widen_varchar', table: 'proj_task', column: 'title', from: 30, to: 60 }; + const { applied, skipped } = await driver.applyMigrationEntries([entryFor(op, 'safe')], { + allowDestructive: false, + }); + + expect(applied).toHaveLength(0); + expect(skipped.map((d) => d.op.type)).toEqual(['widen_varchar']); + }); + }); + + // ──────────────────────────────────────────────────────────────────── + // §2 — a MIXED batch: the rebuild still runs and still reports honestly + // ──────────────────────────────────────────────────────────────────── + describe('§2 a mixed batch splits — the rebuild runs, and only its own work is reported applied', () => { + it('relaxes NOT NULL for real while reporting the ignored sibling as skipped', async () => { + const driver = makeDriver(); + await knexInstance.schema.createTable('biz_unit', (t: any) => { + t.string('id').primary(); + t.timestamp('created_at'); + t.timestamp('updated_at'); + t.string('name'); + t.string('organization_id').notNullable(); + }); + await knexInstance('biz_unit').insert({ id: '1', name: 'Acme', organization_id: 'org1' }); + await driver.initObjects([ + { + name: 'biz_unit', + fields: { + name: { type: 'string' }, + organization_id: { type: 'string', required: false }, + }, + }, + ]); + + // The relax entry is REAL differ output — only the ignored sibling is + // constructed, and only because the differ's SQLite guard withholds it. + const drift = await driver.detectManagedDrift(); + const relax = drift.find((d) => d.op.type === 'relax_not_null'); + expect(relax, 'fixture must produce a real relax_not_null entry').toBeDefined(); + + const ignored = entryFor( + { type: 'narrow_varchar', table: 'biz_unit', column: 'name', from: 255, to: 40 }, + 'destructive', + ); + + const { applied, skipped } = await driver.applyMigrationEntries([relax!, ignored], { + allowDestructive: true, + }); + + expect(applied.map((d) => d.op.type)).toEqual(['relax_not_null']); + expect(skipped.map((d) => d.op.type)).toEqual(['narrow_varchar']); + + // The rebuild genuinely ran: the constraint is gone and the row survived. + const info = await knexInstance('biz_unit').columnInfo(); + expect(info.organization_id.nullable).toBe(true); + expect(info).toHaveProperty('name'); + expect(await knexInstance('biz_unit').select('*')).toMatchObject([ + { id: '1', name: 'Acme', organization_id: 'org1' }, + ]); + }); + }); + + // ──────────────────────────────────────────────────────────────────── + // §3 — the ratchet: the honoured set is EXACTLY the four the rebuild acts on + // ──────────────────────────────────────────────────────────────────── + describe('§3 every column op type is partitioned by what the rebuild actually does', () => { + it('honours exactly relax/tighten NOT NULL, drop_column, drop_column_default — and skips the rest', async () => { + const driver = makeDriver(); + await knexInstance.schema.createTable('ratchet_t', (t: any) => { + t.string('id').primary(); + t.string('a').notNullable(); + t.string('b'); + t.string('c'); + t.string('d').defaultTo('current_user'); + t.string('e'); + t.string('f'); + t.string('g'); + }); + await driver.initObjects([ + { + name: 'ratchet_t', + fields: { + a: { type: 'string' }, + b: { type: 'string' }, + d: { type: 'string' }, + e: { type: 'string' }, + f: { type: 'string' }, + g: { type: 'string' }, + }, + }, + ]); + + // One entry per COLUMN op type in `DriftOp`, each on its own column so no + // two can interfere. Index ops take a different path entirely and are not + // this partition's business. + const batch: Array<[DriftOp, ManagedDriftEntry['category']]> = [ + [{ type: 'relax_not_null', table: 'ratchet_t', column: 'a' }, 'safe'], + [{ type: 'tighten_not_null', table: 'ratchet_t', column: 'b' }, 'destructive'], + [{ type: 'drop_column', table: 'ratchet_t', column: 'c' }, 'destructive'], + [{ type: 'drop_column_default', table: 'ratchet_t', column: 'd' }, 'safe'], + [{ type: 'widen_varchar', table: 'ratchet_t', column: 'e', from: 30, to: 60 }, 'safe'], + [{ type: 'narrow_varchar', table: 'ratchet_t', column: 'f', from: 60, to: 30 }, 'destructive'], + [ + { type: 'manual_column_type_change', table: 'ratchet_t', column: 'g', from: 'varchar', to: 'json' }, + 'needs_confirm', + ], + ]; + + const { applied, skipped } = await driver.applyMigrationEntries( + batch.map(([op, cat]) => entryFor(op, cat)), + { allowDestructive: true }, + ); + + expect(applied.map((d) => d.op.type).sort()).toEqual( + ['drop_column', 'drop_column_default', 'relax_not_null', 'tighten_not_null'].sort(), + ); + expect(skipped.map((d) => d.op.type).sort()).toEqual( + ['manual_column_type_change', 'narrow_varchar', 'widen_varchar'].sort(), + ); + + // Positive control on the applied half — a partition that reported + // "applied" for work that did not happen is the defect itself, so the + // four honoured ops are checked against the physical table. + const info = await knexInstance('ratchet_t').columnInfo(); + expect(info).not.toHaveProperty('c'); + expect(info.a.nullable).toBe(true); + expect(info.b.nullable).toBe(false); + expect(info.d.defaultValue ?? null).toBeNull(); + }); + }); + + // ──────────────────────────────────────────────────────────────────── + // §4 — the consumer-visible symptom the card is actually about + // ──────────────────────────────────────────────────────────────────── + describe('§4 dev auto-reconcile no longer announces an op that never happened', () => { + /** + * The differ cannot emit a non-rebuildable column op on SQLite today, so + * the future in which it can is simulated at the ONE seam that holds it + * closed: `detectTableDrift`. Everything downstream of it — the reconcile, + * the rebuild, the logging, the re-detect — is the real code path. + */ + class FutureOpDriver extends SqlDriver { + public nextDrift: ManagedDriftEntry[] = []; + protected async detectTableDrift(): Promise { + return this.nextDrift; + } + } + + it('logs "unsupported … skipped" instead of "auto-reconciled", and the finding is still warned about', async () => { + const driver = makeDriver({ autoMigrate: 'safe', schemaMode: 'managed' }, FutureOpDriver) as FutureOpDriver; + await driver.initObjects([{ name: 'biz_unit', fields: { name: { type: 'string', maxLength: 60 } } }]); + + driver.nextDrift = [ + entryFor({ type: 'widen_varchar', table: 'biz_unit', column: 'name', from: 30, to: 60 }, 'safe'), + ]; + // Re-detect after the reconcile returns the SAME entry, because the op + // was never performed — which is precisely the loop this card describes. + await (driver as any).reconcileAndWarnDrift('biz_unit', { name: { type: 'string', maxLength: 60 } }); + + const lines = logLines(driver); + expect(lines.some((m) => m.includes('auto-reconciled'))).toBe(false); + expect(lines.some((m) => /widen_varchar on biz_unit\.name is unsupported on dialect 'sqlite' — skipped/.test(m))).toBe(true); + // Still surfaced as drift — reporting it skipped does not hide it. + expect(lines.some((m) => m.includes('widen_varchar on biz_unit.name'))).toBe(true); + }); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 6c12539f68..18c76de812 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -10152,7 +10152,9 @@ export class SqlDriver implements IDataDriver { * Apply a set of drift entries to the physical schema. Destructive entries * are skipped unless `allowDestructive` is set. Postgres/MySQL alter columns * in place; SQLite (which cannot alter constraints in place) rebuilds each - * affected table (copy → swap) applying only the requested edits. + * affected table (copy → swap) applying only those requested edits it can + * express as a rebuild — the rest are reported `skipped` and logged, exactly + * as the in-place arm reports an op its dialect cannot perform (#11722). * * @returns the entries actually applied and those skipped (e.g. destructive * without `allowDestructive`, or unsupported on the dialect). @@ -10193,8 +10195,29 @@ export class SqlDriver implements IDataDriver { for (const [table, ents] of byTable) { try { if (this.isSqlite) { - await this.rebuildSqliteTablePatched(table, ents); - applied.push(...ents); + // `applied` means "this op happened", not "a rebuild ran" — every + // consumer reads it that way (`reconcileAndWarnDrift` logs + // "auto-reconciled on
", the artifact boot gate prints + // "↪ migrated "). The rebuild honours four op types and + // IGNORES the rest (#11722), so reporting the whole batch applied + // turned an op that never happened into a green log line: the + // finding is still there on the next boot, reports as drift again, + // and is "migrated" again — a loop with no failing signal anywhere + // in it. So only the entries the rebuild actually acted on are + // reported applied, and the remainder is skipped and logged in the + // SAME words the in-place arm uses for an op its dialect cannot do + // ({@link applyDriftOpInPlace}). + const honoured = new Set(await this.rebuildSqliteTablePatched(table, ents)); + for (const d of ents) { + if (honoured.has(d)) { + applied.push(d); + continue; + } + this.logger.warn( + `[schema-drift] ${d.op.type} on ${d.table}.${d.column ?? ''} is unsupported on dialect '${this.dialectName}' — skipped`, + ); + skipped.push(d); + } } else { for (const d of ents) { const ok = await this.applyDriftOpInPlace(d.op); @@ -10401,6 +10424,18 @@ export class SqlDriver implements IDataDriver { * copy → drop → rename. varchar widen/narrow are no-ops on SQLite (dynamic * typing) and ignored. * + * @returns the entries this rebuild actually ACTED on. Anything else in + * `ents` was ignored, and the caller must report those as `skipped` rather + * than `applied` (#11722) — on SQLite `applied` otherwise meant only "a + * rebuild ran", while every consumer of it reads "this op happened". + * + * ⚠️ The rebuild still runs for the WHOLE table even when it honours + * nothing: it re-materializes every kept column's default and the full + * declared index set from metadata, so it is not a no-op, and skipping it in + * that case would change what the reconciler DOES rather than what it + * reports. Honesty about the report is this function's contract; deciding + * whether a rebuild is worth running is not. + * * SQLite cannot alter a column's DEFAULT in place, so `drop_column_default` * (#4560) is reconciled here too — by re-materializing every column's default * from METADATA through {@link applyDeclaredColumnDefault} and simply not @@ -10415,17 +10450,28 @@ export class SqlDriver implements IDataDriver { * by `lookup` fields are not re-added (ObjectStack enforces relationships at * the application layer, not via SQLite FK constraints). */ - protected async rebuildSqliteTablePatched(table: string, ents: ManagedDriftEntry[]): Promise { + protected async rebuildSqliteTablePatched(table: string, ents: ManagedDriftEntry[]): Promise { const relax = new Set(); const tighten = new Set(); const drop = new Set(); const dropDefault = new Set(); + // The entries this rebuild ACTS on, filled by the same pass that fills the + // four sets — deliberately not a second list of op types to keep in sync + // with the branches below. An entry is honoured if and only if one of those + // branches took it, so the returned set cannot drift from the work done. + const honoured: ManagedDriftEntry[] = []; for (const e of ents) { if (e.op.type === 'relax_not_null') relax.add(e.op.column); else if (e.op.type === 'tighten_not_null') tighten.add(e.op.column); else if (e.op.type === 'drop_column') drop.add(e.op.column); else if (e.op.type === 'drop_column_default') dropDefault.add(e.op.column); - // widen/narrow varchar: SQLite ignores declared length — nothing to do. + // Everything else is IGNORED here and must be reported as such (#11722): + // widen/narrow varchar (SQLite ignores declared length — nothing to do), + // `manual_column_type_change` (no reconciler arm on ANY dialect, by + // decision), and any column op added later, which lands in this branch by + // default rather than being silently absorbed into `honoured`. + else continue; + honoured.push(e); } const physical = await this.introspectColumns(table); @@ -10497,6 +10543,8 @@ export class SqlDriver implements IDataDriver { } catch (e: any) { this.logger.warn(`[schema-drift] could not fully recreate indexes for '${table}' after rebuild`, e?.message ?? e); } + + return honoured; } /** Map an introspected SQLite column to a knex builder for the rebuilt table. */