Skip to content

Commit 2a18117

Browse files
claude[bot]claude
andauthored
fix(metadata): bind the published migrations to the driver surface IDataDriver declares, and pin it with a real driver (#14084)
* fix(metadata): bind the migrations to the driver surface the contract declares All four helpers in `packages/metadata/src/migrations/` guarded on and drove through `driver.raw(sql, bindings?)`, a method no data driver in this repo defines. `IDataDriver` declares `execute(command, parameters?, options?)` non-optionally and has never declared `raw`, so the guard was enforcing a surface the contract does not have — and refused every driver the platform ships, quietly, through a returned `{ status: 'error' }`. A shared resolver (`driver-exec.ts`) now tries `execute` first and falls back to `raw`, applied uniformly across all four members. The refusal fires only for a driver offering neither surface, and still states its remedy exactly once. Adds `real-driver-exec-surface.test.ts`: the suite's every pre-existing case built a double carrying `raw` — including the one asserting the guard fires — so it pinned the wording while never exercising a shipped driver. The new file drives all four migrations through a real `SqliteWasmDriver` on real in-process SQLite and asserts the physical schema, not the returned status. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L * test(metadata): re-point the loader fixtures at the declared driver surface `database-loader.test.ts` bolted a `raw` method onto its `IDataDriver` mock through an `as unknown as { raw: unknown }` cast, in the two cases that observe the post-sync migration. The cast was the tell: `createMockDriver` already carries `execute` without one, because `IDataDriver` declares it non-optionally and has never declared `raw`. Both cases now observe the mock's own `execute`. The overlay-index case gains a non-vacuity assertion first. It asserts that no statement matched a pattern, which a run issuing no statements at all satisfies equally well — the state the file was actually in while the migration refused every driver. Pins the new file's engine double in the retained ledger (add-only). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L * fix(metadata): drop a wrong destructuring annotation in the loader fixtures `mock.calls` is `any[][]`, so `([sql]: [unknown])` is not assignable to the callback `some`/`map` expect. Two errors, both mine, both caught by `check:type-check-debt` re-measuring @objectstack/metadata at 91 against a shrink-only ledger recording 89. Back to 89 with the annotations removed; the parameter is inferred as `any[]` and carries no implicit-any. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 56cc64d commit 2a18117

11 files changed

Lines changed: 597 additions & 62 deletions
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
---
2+
"@objectstack/metadata": patch
3+
---
4+
5+
fix(metadata): every migration in `@objectstack/metadata/migrations` refused every driver this repo ships (#14023)
6+
7+
All four helpers exported from `@objectstack/metadata/migrations` guarded on —
8+
and drove through — `driver.raw(sql, bindings?)`. **No data driver in this repo
9+
defines `raw`.** `SqlDriver` keeps its knex handle `protected` and declares no
10+
`raw` member, and `SqliteWasmDriver` inherits that; the only `raw(` member
11+
anywhere outside a test double is an HTTP harness in `packages/verify` whose
12+
signature is `(path, init)`. So an operator who passed their platform driver was
13+
refused by all four:
14+
15+
```
16+
migrateSysNotificationToEvent({ driver, data }) -> { status: 'error', migrated: 0 }
17+
```
18+
19+
The failure was quiet in the shape that matters. `migrateSysNotificationToEvent`
20+
*returns* `{ status: 'error' }` rather than throwing, and the message blamed the
21+
caller's driver for lacking a method instead of saying the migration had not
22+
run — so someone following the ADR-0030 cut-over runbook, which names this call
23+
as the supported way to preserve users' existing bell notifications, would read
24+
it as a problem with their own driver.
25+
26+
It was not only an operator-facing path. `DatabaseLoader` calls
27+
`migrateProjectIdToEnvironmentId(driver)` on bootstrap with a real driver, at
28+
two call sites, each wrapped in a catch — so the v5.0 `project_id` ->
29+
`environment_id` forward migration threw and was swallowed on every boot.
30+
31+
The four helpers now resolve their raw-SQL entry point through one shared
32+
resolver (`src/migrations/driver-exec.ts`) that tries `execute` first and falls
33+
back to `raw`. `execute` goes first because it is the surface the contract
34+
declares: `IDataDriver` (`@objectstack/spec/contracts`) declares
35+
`execute(command, parameters?, options?)` **non-optionally**, with bound
36+
parameters as the second positional argument — exactly the shape `raw(sql,
37+
bindings?)` was being called in — and has never declared `raw`. `raw` is kept as
38+
a fallback so a host or third-party driver that does define it keeps working;
39+
nothing that worked before stops working, and the refusal now fires only for a
40+
driver offering neither surface.
41+
42+
Two sibling directories already resolved both surfaces instead of assuming one,
43+
in opposite orders (`metadata-protocol`'s `partial-index-probe` tries `raw`
44+
first, its `seed-tenancy-backfill` tries `execute` first, and `protocol.ts`'s
45+
`ensureOverlayIndex` is a third). One operation with three implementations and
46+
two behaviours resolves to the declaration-bound side, which is why this
47+
directory adopts `execute`-first uniformly rather than copying either precedent.
48+
49+
The refusal message now names both surfaces. It keeps the properties pinned
50+
after the doubled-sentence defect: the remedy is stated exactly once, the
51+
sentences stay separated, and a conforming driver is still named.
52+
53+
Tests: every pre-existing case in this directory built its own double carrying a
54+
`raw` method — including the case asserting the guard fires — so the suite
55+
pinned the guard's wording while never exercising a driver the platform ships.
56+
Swapping `raw` for `execute` in the helpers and in the doubles would have moved
57+
that hole rather than closed it. A new `real-driver-exec-surface.test.ts` drives
58+
all four migrations through a real `SqliteWasmDriver` against real in-process
59+
SQLite, asserting the physical schema rather than the returned status, and pins
60+
the surface reality the file exists for: the real driver has no `raw` and does
61+
have `execute`.

packages/metadata/src/loaders/database-loader.test.ts

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -619,20 +619,27 @@ describe('DatabaseLoader schema-sync failure reporting (#4728)', () => {
619619
});
620620

621621
it('still runs the post-sync migrations (the table exists, so they apply)', async () => {
622+
// #14023 — this used to bolt a `raw` method onto the mock through an
623+
// `as unknown as { raw: unknown }` cast, because that was the only
624+
// surface the migration accepted. The cast was the tell: it reached PAST
625+
// the declared contract. `IDataDriver` declares `execute` non-optionally
626+
// and has never declared `raw`, which is why `createMockDriver` already
627+
// carries `execute` and needed no cast to carry it. The migration now
628+
// drives the declared surface, so this case observes the mock's own
629+
// `execute` and the cast is gone.
622630
const driver = createMockDriver();
623631
driver.syncSchema = vi.fn().mockRejectedValue(alreadyExists());
624-
const raw = vi.fn().mockResolvedValue(undefined);
625-
(driver as unknown as { raw: unknown }).raw = raw;
632+
const execute = driver.execute as ReturnType<typeof vi.fn>;
626633
const loader = new DatabaseLoader({ driver });
627634

628635
await loader.list('object');
629636

630637
// The `project_id` → `environment_id` forward migration still runs; it
631638
// probes the column list before touching anything.
632-
expect(raw).toHaveBeenCalled();
633-
expect(raw.mock.calls.some(([sql]) => /table_info|information_schema/i.test(String(sql)))).toBe(
634-
true,
635-
);
639+
expect(execute).toHaveBeenCalled();
640+
expect(
641+
execute.mock.calls.some(([sql]) => /table_info|information_schema/i.test(String(sql))),
642+
).toBe(true);
636643
});
637644

638645
/**
@@ -646,15 +653,20 @@ describe('DatabaseLoader schema-sync failure reporting (#4728)', () => {
646653
it('issues NO overlay-index DDL — this package is not a producer of that name', async () => {
647654
const driver = createMockDriver();
648655
driver.syncSchema = vi.fn().mockRejectedValue(alreadyExists());
649-
const raw = vi.fn().mockResolvedValue(undefined);
650-
(driver as unknown as { raw: unknown }).raw = raw;
656+
const execute = driver.execute as ReturnType<typeof vi.fn>;
651657
const loader = new DatabaseLoader({ driver });
652658

653659
await loader.list('object');
654660

655-
const overlayDdl = raw.mock.calls
661+
// Non-vacuity FIRST (#14023). This assertion is "no statement matched a
662+
// pattern", which a run that issued NO statements at all satisfies just
663+
// as well — and that is exactly the state this file was in while the
664+
// migration refused every driver. Observe that SQL really flowed before
665+
// reading anything into the absence of that one statement.
666+
expect(execute, 'nothing ran — the emptiness below would prove nothing').toHaveBeenCalled();
667+
const overlayDdl = execute.mock.calls
656668
.map(([sql]) => String(sql))
657-
.filter((sql) => /idx_sys_metadata_overlay_active/i.test(sql));
669+
.filter((sql: string) => /idx_sys_metadata_overlay_active/i.test(sql));
658670
expect(overlayDdl).toEqual([]);
659671
});
660672
});
Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* How the migrations in this directory obtain a raw-SQL entry point.
5+
*
6+
* Every helper here used to guard on — and drive through — `driver.raw(sql,
7+
* bindings?)`. **No data driver in this repo defines `raw`.** Measured on
8+
* `origin/main`, the only `raw(` member anywhere outside a test double is
9+
* `packages/verify/src/harness.ts`, an HTTP harness whose signature is
10+
* `(path, init)`. `SqlDriver` keeps its knex handle `protected`, so
11+
* `driver.raw` is `undefined` there too, and `SqliteWasmDriver` inherits that.
12+
* The result was a published, operator-documented migration path that refused
13+
* every driver the platform ships — quietly, because
14+
* `migrateSysNotificationToEvent` *returns* `{ status: 'error' }` rather than
15+
* throwing, and the message blamed the operator's driver instead of saying the
16+
* migration did not run.
17+
*
18+
* ## Why `execute` is tried FIRST
19+
*
20+
* `IDataDriver` (`@objectstack/spec/contracts`, `data-driver.ts`) declares
21+
*
22+
* ```ts
23+
* execute(command: unknown, parameters?: unknown[], options?: DriverOptions): Promise<unknown>;
24+
* ```
25+
*
26+
* — **non-optional**, with bound parameters as the second POSITIONAL argument,
27+
* which is the exact shape `raw(sql, bindings?)` was being called in. `raw` has
28+
* never appeared on that interface. So `execute` is not merely the surface the
29+
* shipped drivers happen to have; it is the only raw-execution surface the
30+
* contract guarantees at all, and a driver that satisfies `IDataDriver` always
31+
* has it. Trying it first is therefore the order that matches the declaration.
32+
*
33+
* ⚠️ `IDataEngine.execute?(command, options?)` (`data-engine.ts`) is a DIFFERENT
34+
* member on a different interface — its second parameter is an options bag, not
35+
* bindings. These helpers take an `IDataDriver`, so `data-driver.ts` governs.
36+
* Do not reason about this call from the engine declaration.
37+
*
38+
* ## Prior art, and why the order had to be chosen rather than copied
39+
*
40+
* `packages/metadata-protocol/src/migrations/` already resolves both surfaces
41+
* instead of assuming one — twice, and **in opposite orders**:
42+
* `partial-index-probe.ts` tries `raw` first, `seed-tenancy-backfill.ts` tries
43+
* `execute` first. `metadata-protocol/src/protocol.ts` (`ensureOverlayIndex`)
44+
* is a third, raw-first. One operation with three implementations and two
45+
* behaviours resolves to the declaration-bound side, so this directory adopts
46+
* `execute`-first uniformly across all four of its members.
47+
*
48+
* `raw` is kept as a fallback rather than dropped: nothing in this repo defines
49+
* it, but a host or a third-party driver may, and removing a surface that
50+
* currently works is not what this repair is for. The refusal below therefore
51+
* fires only for a driver that has NEITHER.
52+
*
53+
* ## Known limitation, deliberately not papered over here
54+
*
55+
* Two shipped drivers satisfy `typeof driver.execute === 'function'` without
56+
* being able to run SQL: `MemoryDriver.execute` logs a warning and returns
57+
* `null` for every command, and `MongoDbDriver.execute` returns a string
58+
* command back verbatim. Both are selected by the probe below and then answer
59+
* every column probe with "absent", so a migration reports `not_applicable` /
60+
* `table_missing` instead of refusing. `IDataDriver` exposes no capability flag
61+
* that would separate "implements the escape hatch" from "can run SQL"
62+
* (`DriverCapabilities` has no such member), so distinguishing them is a
63+
* contract question, not something to guess at with a driver-name sniff.
64+
* Filed separately.
65+
*/
66+
67+
import type { IDataDriver } from '@objectstack/spec/contracts';
68+
69+
/**
70+
* A raw-SQL entry point resolved off a driver. `bindings` are passed
71+
* positionally, matching `IDataDriver.execute`'s declared `parameters`.
72+
*/
73+
export type DriverExec = (sql: string, bindings?: readonly unknown[]) => Promise<any>;
74+
75+
/**
76+
* Resolve the raw-SQL entry point of `driver`, or `undefined` when it offers
77+
* neither surface.
78+
*
79+
* Callers that must refuse should pair this with {@link driverExecRefusal} so
80+
* every member of this directory states the same remedy.
81+
*/
82+
export function resolveDriverExec(driver: IDataDriver | null | undefined): DriverExec | undefined {
83+
const candidate = driver as any;
84+
if (!candidate) return undefined;
85+
// Declared surface first — see the header.
86+
if (typeof candidate.execute === 'function') {
87+
return (sql, bindings) => candidate.execute(sql, bindings ? [...bindings] : []);
88+
}
89+
if (typeof candidate.raw === 'function') {
90+
return (sql, bindings) => candidate.raw(sql, bindings ? [...bindings] : []);
91+
}
92+
return undefined;
93+
}
94+
95+
/**
96+
* The single refusal sentence used by every migration in this directory, for a
97+
* driver that offers neither surface.
98+
*
99+
* Assembled in one place because the wording carries pinned properties: the
100+
* remedy is stated exactly ONCE (a guard here once concatenated its instruction
101+
* twice), the two sentences stay separated rather than running together, and a
102+
* conforming driver is named so the operator has something to act on.
103+
*/
104+
export function driverExecRefusal(helper: string): string {
105+
return (
106+
`${helper}: driver must expose an .execute(sql, bindings?) or .raw(sql, bindings?) method. ` +
107+
'SqlDriver (better-sqlite3/knex) exposes .execute(), as does its SqliteWasmDriver subclass; ' +
108+
'cloud-side TursoDriver also conforms.'
109+
);
110+
}

packages/metadata/src/migrations/drop-projection-tables.ts

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919

2020
import type { IDataDriver } from '@objectstack/spec/contracts';
2121

22+
import { driverExecRefusal, resolveDriverExec } from './driver-exec.js';
23+
2224
const DEPRECATED_TABLES = [
2325
'sys_object',
2426
'sys_view',
@@ -36,19 +38,21 @@ export interface DropProjectionResult {
3638
/**
3739
* Drop the deprecated per-type metadata projection tables.
3840
*
39-
* @param driver An `IDataDriver` with `driver.raw(sql, bindings?)` access.
41+
* @param driver An `IDataDriver`. Raw SQL is issued through the surface
42+
* `IDataDriver` declares — `execute(sql, bindings?)` — falling
43+
* back to `raw(sql, bindings?)`; see `./driver-exec.ts`.
4044
* @returns Per-table results.
4145
*/
4246
export async function dropProjectionTables(driver: IDataDriver): Promise<DropProjectionResult[]> {
43-
const driverAny = driver as any;
44-
if (typeof driverAny.raw !== 'function') {
45-
throw new Error('dropProjectionTables: driver must expose a raw(sql) method');
47+
const exec = resolveDriverExec(driver);
48+
if (!exec) {
49+
throw new Error(driverExecRefusal('dropProjectionTables'));
4650
}
4751

4852
const results: DropProjectionResult[] = [];
4953
for (const table of DEPRECATED_TABLES) {
5054
try {
51-
await driverAny.raw(`DROP TABLE IF EXISTS ${table}`);
55+
await exec(`DROP TABLE IF EXISTS ${table}`);
5256
results.push({ table, status: 'dropped' });
5357
} catch (error) {
5458
results.push({

packages/metadata/src/migrations/migrate-env-id-to-project-id.ts

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121

2222
import type { IDataDriver } from '@objectstack/spec/contracts';
2323

24+
import { type DriverExec, driverExecRefusal, resolveDriverExec } from './driver-exec.js';
25+
2426
const AFFECTED_TABLES = [
2527
'sys_metadata',
2628
'sys_metadata_history',
@@ -35,27 +37,26 @@ export interface MigrationResult {
3537
/**
3638
* Rename `env_id` → `project_id` on all metadata tables.
3739
*
38-
* @param driver An IDataDriver with access to the target database.
39-
* Must expose a raw query method: `driver.raw(sql, bindings?)`.
40+
* @param driver An IDataDriver with access to the target database. Raw SQL is
41+
* issued through the surface `IDataDriver` declares —
42+
* `execute(sql, bindings?)` — falling back to
43+
* `raw(sql, bindings?)`; see `./driver-exec.ts`.
4044
* @returns Per-table migration results.
4145
*/
4246
export async function migrateEnvIdToProjectId(driver: IDataDriver): Promise<MigrationResult[]> {
43-
const driverAny = driver as any;
47+
const exec = resolveDriverExec(driver);
4448

45-
if (typeof driverAny.raw !== 'function') {
46-
throw new Error(
47-
'migrateEnvIdToProjectId: driver must expose a .raw(sql, bindings?) method. ' +
48-
'SqlDriver (better-sqlite3/knex) supports this; cloud-side TursoDriver also conforms.'
49-
);
49+
if (!exec) {
50+
throw new Error(driverExecRefusal('migrateEnvIdToProjectId'));
5051
}
5152

5253
const results: MigrationResult[] = [];
5354

5455
for (const table of AFFECTED_TABLES) {
5556
try {
5657
// Detect dialect: SQLite uses PRAGMA, others use information_schema.
57-
const hasColumn = await _columnExists(driverAny, table, 'env_id');
58-
const alreadyMigrated = await _columnExists(driverAny, table, 'project_id');
58+
const hasColumn = await _columnExists(exec, table, 'env_id');
59+
const alreadyMigrated = await _columnExists(exec, table, 'project_id');
5960

6061
if (alreadyMigrated && !hasColumn) {
6162
results.push({ table, status: 'already_done' });
@@ -69,7 +70,7 @@ export async function migrateEnvIdToProjectId(driver: IDataDriver): Promise<Migr
6970
}
7071

7172
// Perform the rename. SQLite ≥ 3.25.0 supports ALTER TABLE RENAME COLUMN.
72-
await driverAny.raw(`ALTER TABLE "${table}" RENAME COLUMN env_id TO project_id`);
73+
await exec(`ALTER TABLE "${table}" RENAME COLUMN env_id TO project_id`);
7374

7475
results.push({ table, status: 'renamed' });
7576
} catch (err: any) {
@@ -84,18 +85,18 @@ export async function migrateEnvIdToProjectId(driver: IDataDriver): Promise<Migr
8485
// Internal helpers
8586
// ---------------------------------------------------------------------------
8687

87-
async function _columnExists(driver: any, table: string, column: string): Promise<boolean> {
88+
async function _columnExists(exec: DriverExec, table: string, column: string): Promise<boolean> {
8889
try {
8990
// SQLite: PRAGMA table_info returns rows with `name` column.
90-
const rows: any[] = await driver.raw(`PRAGMA table_info("${table}")`);
91+
const rows: any[] = await exec(`PRAGMA table_info("${table}")`);
9192
if (Array.isArray(rows) && rows.length > 0) {
9293
// knex wraps PRAGMA result; handle both `rows` and `rows[0]` shapes.
9394
const list: any[] = Array.isArray(rows[0]) ? rows[0] : rows;
9495
return list.some((r: any) => r?.name === column);
9596
}
9697

9798
// Fallback for non-SQLite: query information_schema.
98-
const result: any[] = await driver.raw(
99+
const result: any[] = await exec(
99100
`SELECT column_name FROM information_schema.columns WHERE table_name = ? AND column_name = ?`,
100101
[table, column]
101102
);

0 commit comments

Comments
 (0)