diff --git a/.changeset/migration-driver-exec-surface.md b/.changeset/migration-driver-exec-surface.md new file mode 100644 index 0000000000..b9a33548f3 --- /dev/null +++ b/.changeset/migration-driver-exec-surface.md @@ -0,0 +1,61 @@ +--- +"@objectstack/metadata": patch +--- + +fix(metadata): every migration in `@objectstack/metadata/migrations` refused every driver this repo ships (#14023) + +All four helpers exported from `@objectstack/metadata/migrations` guarded on — +and drove through — `driver.raw(sql, bindings?)`. **No data driver in this repo +defines `raw`.** `SqlDriver` keeps its knex handle `protected` and declares no +`raw` member, and `SqliteWasmDriver` inherits that; the only `raw(` member +anywhere outside a test double is an HTTP harness in `packages/verify` whose +signature is `(path, init)`. So an operator who passed their platform driver was +refused by all four: + +``` +migrateSysNotificationToEvent({ driver, data }) -> { status: 'error', migrated: 0 } +``` + +The failure was quiet in the shape that matters. `migrateSysNotificationToEvent` +*returns* `{ status: 'error' }` rather than throwing, and the message blamed the +caller's driver for lacking a method instead of saying the migration had not +run — so someone following the ADR-0030 cut-over runbook, which names this call +as the supported way to preserve users' existing bell notifications, would read +it as a problem with their own driver. + +It was not only an operator-facing path. `DatabaseLoader` calls +`migrateProjectIdToEnvironmentId(driver)` on bootstrap with a real driver, at +two call sites, each wrapped in a catch — so the v5.0 `project_id` -> +`environment_id` forward migration threw and was swallowed on every boot. + +The four helpers now resolve their raw-SQL entry point through one shared +resolver (`src/migrations/driver-exec.ts`) that tries `execute` first and falls +back to `raw`. `execute` goes first because it is the surface the contract +declares: `IDataDriver` (`@objectstack/spec/contracts`) declares +`execute(command, parameters?, options?)` **non-optionally**, with bound +parameters as the second positional argument — exactly the shape `raw(sql, +bindings?)` was being called in — and has never declared `raw`. `raw` is kept as +a fallback so a host or third-party driver that does define it keeps working; +nothing that worked before stops working, and the refusal now fires only for a +driver offering neither surface. + +Two sibling directories already resolved both surfaces instead of assuming one, +in opposite orders (`metadata-protocol`'s `partial-index-probe` tries `raw` +first, its `seed-tenancy-backfill` tries `execute` first, and `protocol.ts`'s +`ensureOverlayIndex` is a third). One operation with three implementations and +two behaviours resolves to the declaration-bound side, which is why this +directory adopts `execute`-first uniformly rather than copying either precedent. + +The refusal message now names both surfaces. It keeps the properties pinned +after the doubled-sentence defect: the remedy is stated exactly once, the +sentences stay separated, and a conforming driver is still named. + +Tests: every pre-existing case in this directory built its own double carrying a +`raw` method — including the case asserting the guard fires — so the suite +pinned the guard's wording while never exercising a driver the platform ships. +Swapping `raw` for `execute` in the helpers and in the doubles would have moved +that hole rather than closed it. A new `real-driver-exec-surface.test.ts` drives +all four migrations through a real `SqliteWasmDriver` against real in-process +SQLite, asserting the physical schema rather than the returned status, and pins +the surface reality the file exists for: the real driver has no `raw` and does +have `execute`. diff --git a/packages/metadata/src/loaders/database-loader.test.ts b/packages/metadata/src/loaders/database-loader.test.ts index ae917c7ce0..832042b655 100644 --- a/packages/metadata/src/loaders/database-loader.test.ts +++ b/packages/metadata/src/loaders/database-loader.test.ts @@ -619,20 +619,27 @@ describe('DatabaseLoader schema-sync failure reporting (#4728)', () => { }); it('still runs the post-sync migrations (the table exists, so they apply)', async () => { + // #14023 — this used to bolt a `raw` method onto the mock through an + // `as unknown as { raw: unknown }` cast, because that was the only + // surface the migration accepted. The cast was the tell: it reached PAST + // the declared contract. `IDataDriver` declares `execute` non-optionally + // and has never declared `raw`, which is why `createMockDriver` already + // carries `execute` and needed no cast to carry it. The migration now + // drives the declared surface, so this case observes the mock's own + // `execute` and the cast is gone. const driver = createMockDriver(); driver.syncSchema = vi.fn().mockRejectedValue(alreadyExists()); - const raw = vi.fn().mockResolvedValue(undefined); - (driver as unknown as { raw: unknown }).raw = raw; + const execute = driver.execute as ReturnType; const loader = new DatabaseLoader({ driver }); await loader.list('object'); // The `project_id` → `environment_id` forward migration still runs; it // probes the column list before touching anything. - expect(raw).toHaveBeenCalled(); - expect(raw.mock.calls.some(([sql]) => /table_info|information_schema/i.test(String(sql)))).toBe( - true, - ); + expect(execute).toHaveBeenCalled(); + expect( + execute.mock.calls.some(([sql]) => /table_info|information_schema/i.test(String(sql))), + ).toBe(true); }); /** @@ -646,15 +653,20 @@ describe('DatabaseLoader schema-sync failure reporting (#4728)', () => { it('issues NO overlay-index DDL — this package is not a producer of that name', async () => { const driver = createMockDriver(); driver.syncSchema = vi.fn().mockRejectedValue(alreadyExists()); - const raw = vi.fn().mockResolvedValue(undefined); - (driver as unknown as { raw: unknown }).raw = raw; + const execute = driver.execute as ReturnType; const loader = new DatabaseLoader({ driver }); await loader.list('object'); - const overlayDdl = raw.mock.calls + // Non-vacuity FIRST (#14023). This assertion is "no statement matched a + // pattern", which a run that issued NO statements at all satisfies just + // as well — and that is exactly the state this file was in while the + // migration refused every driver. Observe that SQL really flowed before + // reading anything into the absence of that one statement. + expect(execute, 'nothing ran — the emptiness below would prove nothing').toHaveBeenCalled(); + const overlayDdl = execute.mock.calls .map(([sql]) => String(sql)) - .filter((sql) => /idx_sys_metadata_overlay_active/i.test(sql)); + .filter((sql: string) => /idx_sys_metadata_overlay_active/i.test(sql)); expect(overlayDdl).toEqual([]); }); }); diff --git a/packages/metadata/src/migrations/driver-exec.ts b/packages/metadata/src/migrations/driver-exec.ts new file mode 100644 index 0000000000..32aaee4cd1 --- /dev/null +++ b/packages/metadata/src/migrations/driver-exec.ts @@ -0,0 +1,110 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * How the migrations in this directory obtain a raw-SQL entry point. + * + * Every helper here used to guard on — and drive through — `driver.raw(sql, + * bindings?)`. **No data driver in this repo defines `raw`.** Measured on + * `origin/main`, the only `raw(` member anywhere outside a test double is + * `packages/verify/src/harness.ts`, an HTTP harness whose signature is + * `(path, init)`. `SqlDriver` keeps its knex handle `protected`, so + * `driver.raw` is `undefined` there too, and `SqliteWasmDriver` inherits that. + * The result was a published, operator-documented migration path that refused + * every driver the platform ships — quietly, because + * `migrateSysNotificationToEvent` *returns* `{ status: 'error' }` rather than + * throwing, and the message blamed the operator's driver instead of saying the + * migration did not run. + * + * ## Why `execute` is tried FIRST + * + * `IDataDriver` (`@objectstack/spec/contracts`, `data-driver.ts`) declares + * + * ```ts + * execute(command: unknown, parameters?: unknown[], options?: DriverOptions): Promise; + * ``` + * + * — **non-optional**, with bound parameters as the second POSITIONAL argument, + * which is the exact shape `raw(sql, bindings?)` was being called in. `raw` has + * never appeared on that interface. So `execute` is not merely the surface the + * shipped drivers happen to have; it is the only raw-execution surface the + * contract guarantees at all, and a driver that satisfies `IDataDriver` always + * has it. Trying it first is therefore the order that matches the declaration. + * + * ⚠️ `IDataEngine.execute?(command, options?)` (`data-engine.ts`) is a DIFFERENT + * member on a different interface — its second parameter is an options bag, not + * bindings. These helpers take an `IDataDriver`, so `data-driver.ts` governs. + * Do not reason about this call from the engine declaration. + * + * ## Prior art, and why the order had to be chosen rather than copied + * + * `packages/metadata-protocol/src/migrations/` already resolves both surfaces + * instead of assuming one — twice, and **in opposite orders**: + * `partial-index-probe.ts` tries `raw` first, `seed-tenancy-backfill.ts` tries + * `execute` first. `metadata-protocol/src/protocol.ts` (`ensureOverlayIndex`) + * is a third, raw-first. One operation with three implementations and two + * behaviours resolves to the declaration-bound side, so this directory adopts + * `execute`-first uniformly across all four of its members. + * + * `raw` is kept as a fallback rather than dropped: nothing in this repo defines + * it, but a host or a third-party driver may, and removing a surface that + * currently works is not what this repair is for. The refusal below therefore + * fires only for a driver that has NEITHER. + * + * ## Known limitation, deliberately not papered over here + * + * Two shipped drivers satisfy `typeof driver.execute === 'function'` without + * being able to run SQL: `MemoryDriver.execute` logs a warning and returns + * `null` for every command, and `MongoDbDriver.execute` returns a string + * command back verbatim. Both are selected by the probe below and then answer + * every column probe with "absent", so a migration reports `not_applicable` / + * `table_missing` instead of refusing. `IDataDriver` exposes no capability flag + * that would separate "implements the escape hatch" from "can run SQL" + * (`DriverCapabilities` has no such member), so distinguishing them is a + * contract question, not something to guess at with a driver-name sniff. + * Filed separately. + */ + +import type { IDataDriver } from '@objectstack/spec/contracts'; + +/** + * A raw-SQL entry point resolved off a driver. `bindings` are passed + * positionally, matching `IDataDriver.execute`'s declared `parameters`. + */ +export type DriverExec = (sql: string, bindings?: readonly unknown[]) => Promise; + +/** + * Resolve the raw-SQL entry point of `driver`, or `undefined` when it offers + * neither surface. + * + * Callers that must refuse should pair this with {@link driverExecRefusal} so + * every member of this directory states the same remedy. + */ +export function resolveDriverExec(driver: IDataDriver | null | undefined): DriverExec | undefined { + const candidate = driver as any; + if (!candidate) return undefined; + // Declared surface first — see the header. + if (typeof candidate.execute === 'function') { + return (sql, bindings) => candidate.execute(sql, bindings ? [...bindings] : []); + } + if (typeof candidate.raw === 'function') { + return (sql, bindings) => candidate.raw(sql, bindings ? [...bindings] : []); + } + return undefined; +} + +/** + * The single refusal sentence used by every migration in this directory, for a + * driver that offers neither surface. + * + * Assembled in one place because the wording carries pinned properties: the + * remedy is stated exactly ONCE (a guard here once concatenated its instruction + * twice), the two sentences stay separated rather than running together, and a + * conforming driver is named so the operator has something to act on. + */ +export function driverExecRefusal(helper: string): string { + return ( + `${helper}: driver must expose an .execute(sql, bindings?) or .raw(sql, bindings?) method. ` + + 'SqlDriver (better-sqlite3/knex) exposes .execute(), as does its SqliteWasmDriver subclass; ' + + 'cloud-side TursoDriver also conforms.' + ); +} diff --git a/packages/metadata/src/migrations/drop-projection-tables.ts b/packages/metadata/src/migrations/drop-projection-tables.ts index 0bdfa863a3..4d51e7693c 100644 --- a/packages/metadata/src/migrations/drop-projection-tables.ts +++ b/packages/metadata/src/migrations/drop-projection-tables.ts @@ -19,6 +19,8 @@ import type { IDataDriver } from '@objectstack/spec/contracts'; +import { driverExecRefusal, resolveDriverExec } from './driver-exec.js'; + const DEPRECATED_TABLES = [ 'sys_object', 'sys_view', @@ -36,19 +38,21 @@ export interface DropProjectionResult { /** * Drop the deprecated per-type metadata projection tables. * - * @param driver An `IDataDriver` with `driver.raw(sql, bindings?)` access. + * @param driver An `IDataDriver`. Raw SQL is issued through the surface + * `IDataDriver` declares — `execute(sql, bindings?)` — falling + * back to `raw(sql, bindings?)`; see `./driver-exec.ts`. * @returns Per-table results. */ export async function dropProjectionTables(driver: IDataDriver): Promise { - const driverAny = driver as any; - if (typeof driverAny.raw !== 'function') { - throw new Error('dropProjectionTables: driver must expose a raw(sql) method'); + const exec = resolveDriverExec(driver); + if (!exec) { + throw new Error(driverExecRefusal('dropProjectionTables')); } const results: DropProjectionResult[] = []; for (const table of DEPRECATED_TABLES) { try { - await driverAny.raw(`DROP TABLE IF EXISTS ${table}`); + await exec(`DROP TABLE IF EXISTS ${table}`); results.push({ table, status: 'dropped' }); } catch (error) { results.push({ diff --git a/packages/metadata/src/migrations/migrate-env-id-to-project-id.ts b/packages/metadata/src/migrations/migrate-env-id-to-project-id.ts index 7d419c4215..263f90764d 100644 --- a/packages/metadata/src/migrations/migrate-env-id-to-project-id.ts +++ b/packages/metadata/src/migrations/migrate-env-id-to-project-id.ts @@ -21,6 +21,8 @@ import type { IDataDriver } from '@objectstack/spec/contracts'; +import { type DriverExec, driverExecRefusal, resolveDriverExec } from './driver-exec.js'; + const AFFECTED_TABLES = [ 'sys_metadata', 'sys_metadata_history', @@ -35,18 +37,17 @@ export interface MigrationResult { /** * Rename `env_id` → `project_id` on all metadata tables. * - * @param driver An IDataDriver with access to the target database. - * Must expose a raw query method: `driver.raw(sql, bindings?)`. + * @param driver An IDataDriver with access to the target database. Raw SQL is + * issued through the surface `IDataDriver` declares — + * `execute(sql, bindings?)` — falling back to + * `raw(sql, bindings?)`; see `./driver-exec.ts`. * @returns Per-table migration results. */ export async function migrateEnvIdToProjectId(driver: IDataDriver): Promise { - const driverAny = driver as any; + const exec = resolveDriverExec(driver); - if (typeof driverAny.raw !== 'function') { - throw new Error( - 'migrateEnvIdToProjectId: driver must expose a .raw(sql, bindings?) method. ' + - 'SqlDriver (better-sqlite3/knex) supports this; cloud-side TursoDriver also conforms.' - ); + if (!exec) { + throw new Error(driverExecRefusal('migrateEnvIdToProjectId')); } const results: MigrationResult[] = []; @@ -54,8 +55,8 @@ export async function migrateEnvIdToProjectId(driver: IDataDriver): Promise { +async function _columnExists(exec: DriverExec, table: string, column: string): Promise { try { // SQLite: PRAGMA table_info returns rows with `name` column. - const rows: any[] = await driver.raw(`PRAGMA table_info("${table}")`); + const rows: any[] = await exec(`PRAGMA table_info("${table}")`); if (Array.isArray(rows) && rows.length > 0) { // knex wraps PRAGMA result; handle both `rows` and `rows[0]` shapes. const list: any[] = Array.isArray(rows[0]) ? rows[0] : rows; @@ -95,7 +96,7 @@ async function _columnExists(driver: any, table: string, column: string): Promis } // Fallback for non-SQLite: query information_schema. - const result: any[] = await driver.raw( + const result: any[] = await exec( `SELECT column_name FROM information_schema.columns WHERE table_name = ? AND column_name = ?`, [table, column] ); diff --git a/packages/metadata/src/migrations/migrate-project-id-to-environment-id.test.ts b/packages/metadata/src/migrations/migrate-project-id-to-environment-id.test.ts index 69b51145fa..a1f66f2b2c 100644 --- a/packages/metadata/src/migrations/migrate-project-id-to-environment-id.test.ts +++ b/packages/metadata/src/migrations/migrate-project-id-to-environment-id.test.ts @@ -191,24 +191,30 @@ describe('migrateProjectIdToEnvironmentId — behaviour against a physically-sta expect(statements.filter((s) => s.startsWith('ALTER TABLE'))).toEqual([]); }); - it('still refuses a driver without .raw(), stating the remedy exactly once', async () => { + it('still refuses a driver with NEITHER surface, stating the remedy exactly once', async () => { // #13219 — the guard concatenated its instruction sentence TWICE, so an - // operator with a raw-less driver read the same remedy twice in one + // operator with an unusable driver read the same remedy twice in one // message. The assertion that used to stand here // (`rejects.toThrow(/must expose a \.raw\(sql, bindings\?\) method/)`) // passed either way: a substring match cannot see a second copy. So // these pin the PROPERTIES of the assembled message, not a full-string // copy of today's wording. + // + // The refusal now names BOTH surfaces, because the guard now accepts + // both — `execute` first, which is the one `IDataDriver` declares. The + // properties below are unchanged; only the sentence they are counted + // over moved. That the guard still fires AT ALL is the totality floor: + // widening what is accepted must not mean accepting everything. const outcome: unknown = await migrateProjectIdToEnvironmentId({} as any).then( (value) => value, (error: unknown) => error, ); - expect(outcome, 'a driver with no .raw() must be refused').toBeInstanceOf(Error); + expect(outcome, 'a driver with neither .execute() nor .raw() must be refused').toBeInstanceOf(Error); const message = (outcome as Error).message; // 1. The remedy is stated exactly ONCE. Counted, not compared, so a // later rewording of the sentence still leaves this asserting. - const instruction = /driver must expose a \.raw\(sql, bindings\?\) method\./g; + const instruction = /driver must expose an \.execute\(sql, bindings\?\) or \.raw\(sql, bindings\?\) method\./g; expect(message.match(instruction) ?? []).toHaveLength(1); // 2. ...and the sentences stay SEPARATED. Deleting the duplicate by @@ -224,4 +230,38 @@ describe('migrateProjectIdToEnvironmentId — behaviour against a physically-sta // is the half of the message an operator acts on. expect(message).toMatch(/SqlDriver/); }); + + it('accepts a driver that offers only execute(), and binds through it', async () => { + // The counterpart of the refusal above, and the reason the guard was + // wrong rather than merely strict: `IDataDriver` declares + // `execute(command, parameters?, options?)` NON-optionally and has never + // declared `raw`, so this double is the CONFORMING shape — and the + // pre-repair guard rejected it. Real-driver coverage is in + // `real-driver-exec-surface.test.ts`; this case pins the resolution + // itself, with no `raw` anywhere to fall back to. + const statements: Array<{ sql: string; bindings: unknown }> = []; + const executeOnly = { + async execute(sql: string, bindings?: unknown[]) { + statements.push({ sql, bindings }); + const pragma = /^PRAGMA table_info\("(.+)"\)$/.exec(sql); + if (pragma) { + const columns = pragma[1] === 'sys_metadata' ? ['id', SOURCE_COLUMN] : []; + return columns.map((name) => ({ name })); + } + return []; + }, + } as any; + expect(typeof executeOnly.raw, 'the double must NOT carry a raw()').not.toBe('function'); + + const results = await migrateProjectIdToEnvironmentId(executeOnly); + + expect(results.find((r) => r.table === 'sys_metadata')?.status).toBe('renamed'); + expect(statements.filter((c) => c.sql.startsWith('ALTER TABLE')).map((c) => c.sql)).toEqual([ + `ALTER TABLE "sys_metadata" RENAME COLUMN ${SOURCE_COLUMN} TO ${TARGET_COLUMN}`, + ]); + // The information_schema fallback is the one call site here that binds + // values; every other statement must still receive an empty array + // rather than `undefined`. + expect(statements.every((c) => Array.isArray(c.bindings))).toBe(true); + }); }); diff --git a/packages/metadata/src/migrations/migrate-project-id-to-environment-id.ts b/packages/metadata/src/migrations/migrate-project-id-to-environment-id.ts index 0105db21f0..5cc918a730 100644 --- a/packages/metadata/src/migrations/migrate-project-id-to-environment-id.ts +++ b/packages/metadata/src/migrations/migrate-project-id-to-environment-id.ts @@ -57,6 +57,8 @@ */ import type { IDataDriver } from '@objectstack/spec/contracts'; + +import { type DriverExec, driverExecRefusal, resolveDriverExec } from './driver-exec.js'; import { SysMetadataObject, SysMetadataHistoryObject } from '@objectstack/metadata-core'; /** The column this migration RENAMES AWAY FROM. */ @@ -106,21 +108,20 @@ export interface ProjectIdToEnvironmentIdResult { * Rename `project_id` → `environment_id` on all metadata tables that still * declare `environment_id`. * - * @param driver An IDataDriver with access to the target database. - * Must expose a raw query method: `driver.raw(sql, bindings?)`. + * @param driver An IDataDriver with access to the target database. Raw SQL is + * issued through the surface `IDataDriver` declares — + * `execute(sql, bindings?)` — falling back to + * `raw(sql, bindings?)`; see `./driver-exec.ts`. * @returns Per-table migration results — one entry per candidate table, * including the ones skipped for lacking the declared target. */ export async function migrateProjectIdToEnvironmentId( driver: IDataDriver, ): Promise { - const driverAny = driver as any; + const exec = resolveDriverExec(driver); - if (typeof driverAny.raw !== 'function') { - throw new Error( - 'migrateProjectIdToEnvironmentId: driver must expose a .raw(sql, bindings?) method. ' + - 'SqlDriver (better-sqlite3/knex) supports this; cloud-side TursoDriver also conforms.' - ); + if (!exec) { + throw new Error(driverExecRefusal('migrateProjectIdToEnvironmentId')); } const results: ProjectIdToEnvironmentIdResult[] = []; @@ -135,8 +136,8 @@ export async function migrateProjectIdToEnvironmentId( } try { - const hasColumn = await _columnExists(driverAny, table, SOURCE_COLUMN); - const alreadyMigrated = await _columnExists(driverAny, table, TARGET_COLUMN); + const hasColumn = await _columnExists(exec, table, SOURCE_COLUMN); + const alreadyMigrated = await _columnExists(exec, table, TARGET_COLUMN); if (alreadyMigrated && !hasColumn) { results.push({ table, status: 'already_done' }); @@ -148,7 +149,7 @@ export async function migrateProjectIdToEnvironmentId( continue; } - await driverAny.raw( + await exec( `ALTER TABLE "${table}" RENAME COLUMN ${SOURCE_COLUMN} TO ${TARGET_COLUMN}`, ); @@ -165,15 +166,15 @@ export async function migrateProjectIdToEnvironmentId( // Internal helpers // --------------------------------------------------------------------------- -async function _columnExists(driver: any, table: string, column: string): Promise { +async function _columnExists(exec: DriverExec, table: string, column: string): Promise { try { - const rows: any[] = await driver.raw(`PRAGMA table_info("${table}")`); + const rows: any[] = await exec(`PRAGMA table_info("${table}")`); if (Array.isArray(rows) && rows.length > 0) { const list: any[] = Array.isArray(rows[0]) ? rows[0] : rows; return list.some((r: any) => r?.name === column); } - const result: any[] = await driver.raw( + const result: any[] = await exec( `SELECT column_name FROM information_schema.columns WHERE table_name = ? AND column_name = ?`, [table, column], ); diff --git a/packages/metadata/src/migrations/migrate-sys-notification-to-event.test.ts b/packages/metadata/src/migrations/migrate-sys-notification-to-event.test.ts index 3d51e57b7d..d88f8ddcd4 100644 --- a/packages/metadata/src/migrations/migrate-sys-notification-to-event.test.ts +++ b/packages/metadata/src/migrations/migrate-sys-notification-to-event.test.ts @@ -145,11 +145,51 @@ describe('migrateSysNotificationToEvent', () => { expect(result.status).toBe('not_applicable'); }); - it('errors cleanly when the driver has no raw()', async () => { + it('errors cleanly when the driver has NEITHER raw() nor execute()', async () => { + // The totality floor. A driver offering no raw-SQL surface at all must + // still be refused — that half of the guard is not what was wrong with + // it. What was wrong is that `raw` was the ONLY surface it accepted, so + // it also refused every driver this repo ships; see + // `real-driver-exec-surface.test.ts` for the other half. const e = fakeEngine(); const result = await migrateSysNotificationToEvent({ driver: {} as any, data: e.engine }); expect(result.status).toBe('error'); - expect(result.error).toContain('.raw'); + expect(result.error).toContain('.execute(sql, bindings?)'); + expect(result.error).toContain('.raw(sql, bindings?)'); + }); + + it('drives a driver that offers only execute(), passing bindings positionally', async () => { + // A double deliberately shaped like the DECLARED contract + // (`IDataDriver.execute(command, parameters?, options?)`) rather than + // like the helper's old assumption. The real-driver coverage lives in + // `real-driver-exec-surface.test.ts`; this case additionally pins that + // the second argument arrives as the bindings ARRAY, which is the part a + // mechanical `raw`->`execute` rename could get wrong silently. + const seen: Array<{ sql: string; bindings: unknown }> = []; + const executeOnly = { + async execute(sql: string, bindings?: unknown[]) { + seen.push({ sql, bindings }); + if (sql.startsWith('PRAGMA table_info')) { + return LEGACY_TABLE_COLUMNS.map((name) => ({ name })); + } + if (sql.startsWith('SELECT id, recipient_id')) { + return [{ id: 'n1', recipient_id: 'u1', type: 'mention', title: 't', body: null, url: null, actor_name: null, is_read: 0, read_at: null, created_at: '2026-01-01T00:00:00.000Z', organization_id: 'org_1' }]; + } + return []; + }, + } as any; + expect(typeof executeOnly.raw, 'the double must NOT carry a raw()').not.toBe('function'); + const e = fakeEngine(); + + const result = await migrateSysNotificationToEvent({ driver: executeOnly, data: e.engine }); + + expect(result.status).toBe('migrated'); + expect(result.migrated).toBe(1); + const update = seen.find((c) => c.sql.startsWith('UPDATE')); + expect(update?.bindings).toEqual(['n1']); + // An unbound statement gets an empty array, never `undefined` — the + // shape `SqlDriver.execute` and TursoDriver both normalize to anyway. + expect(seen.find((c) => c.sql.startsWith('PRAGMA'))?.bindings).toEqual([]); }); }); diff --git a/packages/metadata/src/migrations/migrate-sys-notification-to-event.ts b/packages/metadata/src/migrations/migrate-sys-notification-to-event.ts index 1f9ef52ee8..4357eabe7b 100644 --- a/packages/metadata/src/migrations/migrate-sys-notification-to-event.ts +++ b/packages/metadata/src/migrations/migrate-sys-notification-to-event.ts @@ -27,13 +27,17 @@ * await migrateSysNotificationToEvent({ driver, data }); * * `driver` provides raw access to read legacy columns the re-modeled schema no - * longer projects and to clear them; `data` (IDataEngine) performs the + * longer projects and to clear them — through the surface `IDataDriver` + * declares, `execute(sql, bindings?)`, falling back to `raw(sql, bindings?)` + * (see `./driver-exec.ts`); `data` (IDataEngine) performs the * structured inbox/receipt writes and the event rewrite so ids, JSON fields and * tenant stamping are handled uniformly across drivers. */ import type { IDataDriver, IDataEngine } from '@objectstack/spec/contracts'; +import { type DriverExec, driverExecRefusal, resolveDriverExec } from './driver-exec.js'; + const EVENT_OBJECT = 'sys_notification'; const INBOX_OBJECT = 'sys_inbox_message'; const RECEIPT_OBJECT = 'sys_notification_receipt'; @@ -67,32 +71,32 @@ export interface SysNotificationMigrationOptions { export async function migrateSysNotificationToEvent( opts: SysNotificationMigrationOptions, ): Promise { - const driver = opts.driver as any; const { data } = opts; const now = opts.now ?? (() => new Date().toISOString()); - if (typeof driver?.raw !== 'function') { + const exec = resolveDriverExec(opts.driver); + if (!exec) { return { status: 'error', migrated: 0, - error: 'migrateSysNotificationToEvent: driver must expose a .raw(sql, bindings?) method.', + error: driverExecRefusal('migrateSysNotificationToEvent'), }; } // No legacy `recipient_id` column → the table never held the inbox shape. - if (!(await columnExists(driver, EVENT_OBJECT, 'recipient_id'))) { + if (!(await columnExists(exec, EVENT_OBJECT, 'recipient_id'))) { return { status: 'not_applicable', migrated: 0 }; } // Only null-out columns that actually exist on this deployment. const presentLegacy: string[] = []; for (const col of LEGACY_COLUMNS) { - if (await columnExists(driver, EVENT_OBJECT, col)) presentLegacy.push(col); + if (await columnExists(exec, EVENT_OBJECT, col)) presentLegacy.push(col); } let migrated = 0; try { - const rows = await selectLegacyRows(driver); + const rows = await selectLegacyRows(exec); if (rows.length === 0) return { status: 'already_done', migrated: 0 }; for (const row of rows) { @@ -154,7 +158,7 @@ export async function migrateSysNotificationToEvent( // migration filter (idempotency) and carries no stale recipient. if (presentLegacy.length > 0) { const setClause = presentLegacy.map((c) => `"${c}" = NULL`).join(', '); - await driver.raw(`UPDATE "${EVENT_OBJECT}" SET ${setClause} WHERE id = ?`, [id]); + await exec(`UPDATE "${EVENT_OBJECT}" SET ${setClause} WHERE id = ?`, [id]); } migrated += 1; @@ -216,8 +220,8 @@ function canonicalTimestampText(value: unknown): string { return String(value); } -async function selectLegacyRows(driver: any): Promise { - const result: any[] = await driver.raw( +async function selectLegacyRows(exec: DriverExec): Promise { + const result: any[] = await exec( `SELECT id, recipient_id, type, title, body, url, actor_name, is_read, read_at, created_at, organization_id ` + `FROM "${EVENT_OBJECT}" WHERE recipient_id IS NOT NULL`, ); @@ -228,13 +232,13 @@ async function selectLegacyRows(driver: any): Promise { return Array.isArray(result) ? result : []; } -async function columnExists(driver: any, table: string, column: string): Promise { +async function columnExists(exec: DriverExec, table: string, column: string): Promise { // SQLite path: PRAGMA table_info. On Postgres/others this raises a syntax // error — swallow it *locally* and fall through to information_schema (the // outer-catch version of this would never reach the fallback, making the // migration silently no-op on every non-SQLite DB). try { - const rows: any = await driver.raw(`PRAGMA table_info("${table}")`); + const rows: any = await exec(`PRAGMA table_info("${table}")`); const list: any[] = Array.isArray(rows) ? (Array.isArray(rows[0]) ? rows[0] : rows) : []; @@ -246,7 +250,7 @@ async function columnExists(driver: any, table: string, column: string): Promise } // Postgres / others. try { - const result: any = await driver.raw( + const result: any = await exec( `SELECT column_name FROM information_schema.columns WHERE table_name = ? AND column_name = ?`, [table, column], ); diff --git a/packages/metadata/src/migrations/real-driver-exec-surface.test.ts b/packages/metadata/src/migrations/real-driver-exec-surface.test.ts new file mode 100644 index 0000000000..ff0134621c --- /dev/null +++ b/packages/metadata/src/migrations/real-driver-exec-surface.test.ts @@ -0,0 +1,247 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The migrations in this directory, driven through a driver THIS REPO DEFINES. + * + * Every pre-existing case in this directory builds its own double carrying a + * `raw(sql, bindings?)` method — **including the case that asserts the guard + * fires**. So the suite pinned the guard's wording while never once exercising + * a driver the platform ships, and a helper that refused all four of them sat + * green. Swapping `raw` for `execute` in the helpers AND in the doubles would + * have moved that hole rather than closed it: a double shaped to the helper's + * own assumption can only ever agree with it. + * + * This file is the closure. `SqliteWasmDriver` is a real driver + * (`@objectstack/driver-sqlite-wasm`, already a devDependency here and already + * used by `metadata-history.test.ts`), it extends `SqlDriver`, it runs real + * SQLite in-process with no server, and it is constructed here the same way an + * operator constructs one. Nothing below stubs a driver method. + * + * ⭐ The load-bearing case is `pins the surface reality this file exists for`: + * it asserts the real driver has NO `raw` and DOES have `execute`. Without it + * every case here would keep passing if someone re-introduced a `raw`-only + * guard and quietly re-added `raw` to the driver — and it is the assertion that + * fails first if the shipped surface ever moves back. + */ + +import { afterEach, describe, expect, it } from 'vitest'; +import { SqliteWasmDriver } from '@objectstack/driver-sqlite-wasm'; +// The engine double's write verbs route through the producer's OWN dispatch +// predicates, so it cannot accept a call `ObjectQL.` would refuse — the +// same pinning the sibling suite in this directory carries. Imported from +// `@objectstack/metadata-core` (a `dependencies` entry here) and not from +// `@objectstack/objectql`, which depends on this package: that edge would close +// a cycle turbo rejects. +import { + assertEngineDeleteDispatch, + assertEngineFindOnePredicate, + assertEngineUpdateDispatch, + type EngineFindOneQueryInput, +} from '@objectstack/metadata-core'; + +import { dropProjectionTables } from './drop-projection-tables.js'; +import { migrateEnvIdToProjectId } from './migrate-env-id-to-project-id.js'; +import { migrateProjectIdToEnvironmentId } from './migrate-project-id-to-environment-id.js'; +import { migrateSysNotificationToEvent } from './migrate-sys-notification-to-event.js'; + +/** Every driver made here, torn down in `afterEach` (sql.js holds a WASM heap). */ +const live: SqliteWasmDriver[] = []; + +async function realDriver(): Promise { + const driver = new SqliteWasmDriver({ filename: ':memory:' }); + await driver.connect(); + live.push(driver); + return driver; +} + +/** Run SQL the way an operator's setup would — through the driver's own surface. */ +function sql(driver: SqliteWasmDriver): (statement: string, bindings?: unknown[]) => Promise { + return (statement, bindings) => (driver as any).execute(statement, bindings ?? []); +} + +/** + * Engine double for the ONE helper that also needs an `IDataEngine`. The driver + * under test is real; this stands in only for the structured-write half, which + * is not what this file is about. + */ +function recordingEngine() { + const inserts: Array<{ object: string; row: any }> = []; + const updates: Array<{ object: string; data: any }> = []; + return { + inserts, + updates, + engine: { + async insert(object: string, row: any) { + inserts.push({ object, row }); + return { id: `${object}_${inserts.length}`, ...row }; + }, + async update(object: string, data: any, options?: Record) { + assertEngineUpdateDispatch(data, options); + updates.push({ object, data }); + return data; + }, + async find() { return []; }, + async findOne(object: string, query?: EngineFindOneQueryInput) { + assertEngineFindOnePredicate(object, query); + return null; + }, + async delete(_object?: string, options?: Record) { + assertEngineDeleteDispatch(options); + return {}; + }, + async count() { return 0; }, + async aggregate() { return []; }, + } as any, + }; +} + +afterEach(async () => { + while (live.length > 0) { + await live.pop()!.disconnect().catch(() => undefined); + } +}); + +describe('migrations against a driver this repo actually defines', () => { + it('pins the surface reality this file exists for: real drivers have `execute`, not `raw`', async () => { + const driver = await realDriver(); + + // The defect in one line. `SqlDriver` keeps its knex handle `protected` + // and declares no `raw` member, so the old `typeof driver.raw === + // 'function'` guard was false for every driver the platform ships. + expect( + typeof (driver as any).raw, + 'if a real driver grows a .raw() member, every other case in this file stops proving anything', + ).not.toBe('function'); + + // ...and the surface `IDataDriver` declares (non-optionally, with bound + // parameters as its second POSITIONAL argument) is present. + expect(typeof (driver as any).execute).toBe('function'); + + // Non-vacuity for the binding half: `execute` really carries bindings + // positionally, which is what the migrations' `(sql, bindings)` calls + // assume. A driver that accepted the array and ignored it would answer + // `1` here. + const rows: any = await (driver as any).execute('SELECT ? AS bound', [7]); + const list: any[] = Array.isArray(rows) ? (Array.isArray(rows[0]) ? rows[0] : rows) : []; + expect(list[0]?.bound).toBe(7); + }); + + it('migrateProjectIdToEnvironmentId renames the column on a real database', async () => { + const driver = await realDriver(); + const run = sql(driver); + await run('CREATE TABLE "sys_metadata" (id TEXT PRIMARY KEY, name TEXT, project_id TEXT)'); + await run('INSERT INTO "sys_metadata" (id, name, project_id) VALUES (?, ?, ?)', ['m1', 'n', 'env_a']); + + const results = await migrateProjectIdToEnvironmentId(driver); + + expect(results.find((r) => r.table === 'sys_metadata')?.status).toBe('renamed'); + + // Read the physical schema back, not the return value: the return value + // is what reported `error` for years while nothing happened. + const info: any = await run('PRAGMA table_info("sys_metadata")'); + const columns: any[] = Array.isArray(info) ? (Array.isArray(info[0]) ? info[0] : info) : []; + const names = columns.map((c: any) => c.name); + expect(names).toContain('environment_id'); + expect(names).not.toContain('project_id'); + + // The row survived the rename with its value intact. + const after: any = await run('SELECT environment_id FROM "sys_metadata" WHERE id = ?', ['m1']); + const afterRows: any[] = Array.isArray(after) ? (Array.isArray(after[0]) ? after[0] : after) : []; + expect(afterRows[0]?.environment_id).toBe('env_a'); + }); + + it('migrateProjectIdToEnvironmentId is idempotent on a real already-migrated database', async () => { + const driver = await realDriver(); + await sql(driver)('CREATE TABLE "sys_metadata" (id TEXT PRIMARY KEY, environment_id TEXT)'); + + const results = await migrateProjectIdToEnvironmentId(driver); + + expect(results.find((r) => r.table === 'sys_metadata')?.status).toBe('already_done'); + }); + + it('migrateEnvIdToProjectId renames the column on a real database', async () => { + const driver = await realDriver(); + const run = sql(driver); + await run('CREATE TABLE "sys_metadata" (id TEXT PRIMARY KEY, env_id TEXT)'); + await run('CREATE TABLE "sys_metadata_history" (id TEXT PRIMARY KEY, env_id TEXT)'); + + const results = await migrateEnvIdToProjectId(driver); + + expect(results.map((r) => r.status)).toEqual(['renamed', 'renamed']); + for (const table of ['sys_metadata', 'sys_metadata_history']) { + const info: any = await run(`PRAGMA table_info("${table}")`); + const columns: any[] = Array.isArray(info) ? (Array.isArray(info[0]) ? info[0] : info) : []; + expect(columns.map((c: any) => c.name)).toContain('project_id'); + } + }); + + it('dropProjectionTables drops the deprecated tables on a real database', async () => { + const driver = await realDriver(); + const run = sql(driver); + await run('CREATE TABLE sys_object (id TEXT PRIMARY KEY)'); + await run('CREATE TABLE sys_view (id TEXT PRIMARY KEY)'); + + const results = await dropProjectionTables(driver); + + expect(results.every((r) => r.status === 'dropped')).toBe(true); + + // Physical proof — `DROP TABLE IF EXISTS` reports success either way, so + // the return value alone cannot tell "dropped" from "never ran". + const master: any = await run("SELECT name FROM sqlite_master WHERE type = 'table'"); + const tables: any[] = Array.isArray(master) ? (Array.isArray(master[0]) ? master[0] : master) : []; + const names = tables.map((t: any) => t.name); + expect(names).not.toContain('sys_object'); + expect(names).not.toContain('sys_view'); + }); + + it('migrateSysNotificationToEvent carries legacy rows across on a real database', async () => { + const driver = await realDriver(); + const run = sql(driver); + await run( + 'CREATE TABLE "sys_notification" (' + + 'id TEXT PRIMARY KEY, recipient_id TEXT, type TEXT, title TEXT, body TEXT, url TEXT, ' + + 'actor_name TEXT, is_read INTEGER, read_at TEXT, created_at TEXT, organization_id TEXT, ' + + 'topic TEXT, payload TEXT, severity TEXT)', + ); + await run( + 'INSERT INTO "sys_notification" ' + + '(id, recipient_id, type, title, body, url, actor_name, is_read, read_at, created_at, organization_id) ' + + 'VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)', + ['n1', 'u1', 'mention', 'You were mentioned', 'hi', '/x', 'Ada', 0, null, '2026-01-01T00:00:00.000Z', 'org_1'], + ); + const e = recordingEngine(); + + const result = await migrateSysNotificationToEvent({ driver, data: e.engine }); + + // This is the assertion the card is about: an operator following + // `docs/handoff/adr-0030-notification-convergence.md` step 2 with their + // platform driver used to get `{ status: 'error', migrated: 0 }` here. + expect(result.status).toBe('migrated'); + expect(result.migrated).toBe(1); + expect(e.inserts.map((i) => i.object)).toEqual(['sys_inbox_message', 'sys_notification_receipt']); + expect(e.inserts[0]!.row).toMatchObject({ user_id: 'u1', notification_id: 'n1', action_url: '/x' }); + + // The legacy columns were really cleared, through the real driver, with + // the id passed as a BINDING — the one call site that binds a value. + const rows: any = await run('SELECT recipient_id, title FROM "sys_notification" WHERE id = ?', ['n1']); + const list: any[] = Array.isArray(rows) ? (Array.isArray(rows[0]) ? rows[0] : rows) : []; + expect(list[0]?.recipient_id).toBeNull(); + expect(list[0]?.title).toBeNull(); + }); + + it('migrateSysNotificationToEvent reports not_applicable on a real post-cut-over table', async () => { + const driver = await realDriver(); + await sql(driver)( + 'CREATE TABLE "sys_notification" (id TEXT PRIMARY KEY, topic TEXT, payload TEXT, severity TEXT, created_at TEXT)', + ); + const e = recordingEngine(); + + const result = await migrateSysNotificationToEvent({ driver, data: e.engine }); + + // Distinguishes the repair from "accepts anything": a real driver whose + // table never held the inbox shape must still be told apart from one the + // migration could not drive at all. + expect(result.status).toBe('not_applicable'); + expect(e.inserts).toHaveLength(0); + }); +}); diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 3d2d1bad3b..23ae38b7fa 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -1606,6 +1606,21 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/metadata/src/migrations/real-driver-exec-surface.test.ts", + "verb": "delete", + "pinned": 1 + }, + { + "file": "packages/metadata/src/migrations/real-driver-exec-surface.test.ts", + "verb": "findOne", + "pinned": 1 + }, + { + "file": "packages/metadata/src/migrations/real-driver-exec-surface.test.ts", + "verb": "update", + "pinned": 1 + }, { "file": "packages/objectql/src/action-activation.test.ts", "verb": "update",