diff --git a/.changeset/migration-affected-tables-declared-gate.md b/.changeset/migration-affected-tables-declared-gate.md new file mode 100644 index 0000000000..eb6bed28a4 --- /dev/null +++ b/.changeset/migration-affected-tables-declared-gate.md @@ -0,0 +1,35 @@ +--- +"@objectstack/metadata": patch +--- + +fix(metadata): stop `migrateProjectIdToEnvironmentId` renaming into a column no declaration knows about (#13205) + +`AFFECTED_TABLES` was a hand-written list, and it outlived the declarations it +described. The branch/project-removal amendment (M1) took `environment_id` out +of `sys_metadata_history`'s declaration in `@objectstack/metadata-core`; the +migration kept naming that table. Its only guard asks whether `project_id` is +present **physically** (`_columnExists`) — which says nothing about the target +column being **declared** — so against any database whose physical +`sys_metadata_history` still carried the pre-v5 column, the migration renamed it +to `environment_id`: a fresh orphan column that no declaration, no `syncSchema` +and no reader knows about. + +The list is now **derived from the declarations** rather than restated beside +them. A candidate table is migrated only if its current declaration carries the +target column, so the two cannot drift apart again — the derivation and the +declaration are the same fact. `@objectstack/metadata-core` was already a +dependency of this package, so this adds no dependency edge. + +A candidate that does not declare the target column is now **reported** as +`status: 'skipped_not_declared'` (an additive member of the result union) +instead of silently vanishing from the result array: an operator reading the +results can tell "considered and deliberately skipped" from "forgotten again", +which is the state this defect started in. + +The sibling `migrateEnvIdToProjectId` is deliberately left alone: its target +(`project_id`) is an intermediate column that no current declaration carries by +design, so the "target must be declared" rule is sound only for the terminal +migration in the chain. + +No behaviour changes for `sys_metadata`, whose declaration does carry +`environment_id`: it is renamed exactly as before. 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 new file mode 100644 index 0000000000..977cea41e6 --- /dev/null +++ b/packages/metadata/src/migrations/migrate-project-id-to-environment-id.test.ts @@ -0,0 +1,199 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #13205 — the migration's table list must never outlive the declarations. + * + * `migrateProjectIdToEnvironmentId` is the TERMINAL step of the tenancy-column + * chain, so its target column (`environment_id`) is by definition the current + * declared shape. A table on its list whose declaration no longer carries that + * column is therefore not a stale comment — it is a rename that MINTS AN ORPHAN + * COLUMN, on a real database, that no declaration knows about. + * + * `sys_metadata_history` was exactly that: the branch/project-removal amendment + * (M1) took `environment_id` out of its declaration and the hand-written + * `AFFECTED_TABLES` kept naming it. The old `_columnExists` guard could not see + * it — it gates on `project_id` being PHYSICALLY present, which says nothing + * about the target being DECLARED. + * + * So these are pins on the PROPERTY, not on today's answer: + * 1. every table the migration will touch declares the column it produces + * (quantified over `AFFECTED_TABLES`, so a hand-added entry is caught); + * 2. membership TRACKS the declaration in both directions (so the derivation + * cannot be replaced by a literal that happens to match today, and cannot + * quietly degrade to nothing); + * 3. the migration issues no SQL AT ALL against a table it must not rename, + * even when that table physically still carries `project_id` — which is + * the shape the defect actually took. + */ + +import { describe, expect, it } from 'vitest'; +import { + SysMetadataObject, + SysMetadataHistoryObject, + SysMetadataAuditObject, + SysMetadataCommitObject, + SysViewDefinitionObject, +} from '@objectstack/metadata-core'; + +import { + AFFECTED_TABLES, + migrateProjectIdToEnvironmentId, +} from './migrate-project-id-to-environment-id.js'; + +/** The column the migration produces. Renaming into it is only legal if it is declared. */ +const TARGET_COLUMN = 'environment_id'; +/** The column the migration renames away from. */ +const SOURCE_COLUMN = 'project_id'; + +type DeclaredObject = { name: string; fields?: Record }; + +/** + * Every metadata storage object declared by `@objectstack/metadata-core`. Used + * to RESOLVE a table named by the migration back to its declaration — a name + * with no declaration at all is itself drift. + */ +const DECLARED_OBJECTS: readonly DeclaredObject[] = [ + SysMetadataObject, + SysMetadataHistoryObject, + SysMetadataAuditObject, + SysMetadataCommitObject, + SysViewDefinitionObject, +] as unknown as readonly DeclaredObject[]; + +/** + * The tables this migration may ever consider: the metadata storage tables that + * historically carried the tenancy column. This is the migration's UNIVERSE + * (fixed by history), not its answer — the answer is what the assertions below + * derive from the declarations. + */ +const CANDIDATE_TABLES = ['sys_metadata', 'sys_metadata_history'] as const; + +function declaration(table: string): DeclaredObject | undefined { + return DECLARED_OBJECTS.find((o) => o.name === table); +} + +function declaresTarget(table: string): boolean { + const object = declaration(table); + return !!object && Object.prototype.hasOwnProperty.call(object.fields ?? {}, TARGET_COLUMN); +} + +/** Records every statement the migration issues, and answers column probes. */ +function fakeDriver(physicalColumns: Record) { + const statements: string[] = []; + const raw = async (sql: string, _bindings?: unknown[]) => { + statements.push(sql); + const pragma = /^PRAGMA table_info\("(.+)"\)$/.exec(sql); + if (pragma) { + const columns = physicalColumns[pragma[1]!] ?? []; + return columns.map((name) => ({ name })); + } + if (sql.startsWith('SELECT column_name')) return []; + return []; + }; + return { driver: { raw } as any, statements }; +} + +describe('migrateProjectIdToEnvironmentId — AFFECTED_TABLES is pinned to declared reality', () => { + it('every table it will rename DECLARES the column the rename produces', () => { + // The safety property, quantified over the list itself: whatever ends up + // in AFFECTED_TABLES — derived today, hand-written tomorrow — must be a + // table whose CURRENT declaration carries `environment_id`. This is the + // assertion that would have failed on `sys_metadata_history`. + const orphanMinting = AFFECTED_TABLES.filter((table) => !declaresTarget(table)); + expect(orphanMinting).toEqual([]); + }); + + it('names only tables that have a declaration at all', () => { + const undeclared = AFFECTED_TABLES.filter((table) => declaration(table) === undefined); + expect(undeclared).toEqual([]); + }); + + it('membership TRACKS the declaration, in both directions', () => { + // Not `toEqual(['sys_metadata'])` — that would re-state today's answer and + // pin nothing. Each candidate's membership is compared against its own + // declaration, so this keeps holding (and keeps meaning something) if a + // declaration legitimately regains or loses `environment_id`. + for (const table of CANDIDATE_TABLES) { + expect( + AFFECTED_TABLES.includes(table), + `${table}: list membership must equal "declares ${TARGET_COLUMN}"`, + ).toBe(declaresTarget(table)); + } + }); + + it('is not vacuous — the derivation still selects the tables that do declare it', () => { + // Guards the other failure direction: a derivation that silently degrades + // to an empty list would satisfy every assertion above. + const expected = CANDIDATE_TABLES.filter(declaresTarget); + expect(expected.length).toBeGreaterThan(0); + expect([...AFFECTED_TABLES].sort()).toEqual([...expected].sort()); + }); + + it('considers no table outside the historical candidate set', () => { + expect(AFFECTED_TABLES.every((t) => (CANDIDATE_TABLES as readonly string[]).includes(t))).toBe(true); + }); +}); + +describe('migrateProjectIdToEnvironmentId — behaviour against a physically-stale database', () => { + /** + * The defect's exact shape: BOTH physical tables still carry `project_id`. + * The pre-fix code renamed both, because it only asked whether `project_id` + * was physically there. + */ + const stale = { + sys_metadata: ['id', 'name', 'type', SOURCE_COLUMN], + sys_metadata_history: ['id', 'name', 'type', SOURCE_COLUMN], + }; + + it('renames only the declared table and issues NO statement against the undeclared one', async () => { + const { driver, statements } = fakeDriver(stale); + + const results = await migrateProjectIdToEnvironmentId(driver); + + const renamed = results.filter((r) => r.status === 'renamed').map((r) => r.table); + expect(renamed).toEqual(CANDIDATE_TABLES.filter(declaresTarget)); + + const alters = statements.filter((s) => s.startsWith('ALTER TABLE')); + expect(alters).toEqual([ + `ALTER TABLE "sys_metadata" RENAME COLUMN ${SOURCE_COLUMN} TO ${TARGET_COLUMN}`, + ]); + + // Sharper than "no ALTER": a table the migration must not touch is never + // even probed. Pre-fix this list held a PRAGMA and an ALTER. + const undeclared = CANDIDATE_TABLES.filter((t) => !declaresTarget(t)); + for (const table of undeclared) { + expect(statements.filter((s) => s.includes(table))).toEqual([]); + } + }); + + it('reports the skipped table instead of dropping it silently', async () => { + const { driver } = fakeDriver(stale); + + const results = await migrateProjectIdToEnvironmentId(driver); + + // One entry per candidate — an operator can tell "considered and skipped" + // from "forgotten again", which is the state this card started in. + expect(results.map((r) => r.table).sort()).toEqual([...CANDIDATE_TABLES].sort()); + for (const table of CANDIDATE_TABLES.filter((t) => !declaresTarget(t))) { + expect(results.find((r) => r.table === table)?.status).toBe('skipped_not_declared'); + } + }); + + it('is idempotent on an already-migrated database', async () => { + const { driver, statements } = fakeDriver({ + sys_metadata: ['id', 'name', 'type', TARGET_COLUMN], + sys_metadata_history: ['id', 'name', 'type'], + }); + + const results = await migrateProjectIdToEnvironmentId(driver); + + expect(results.find((r) => r.table === 'sys_metadata')?.status).toBe('already_done'); + expect(statements.filter((s) => s.startsWith('ALTER TABLE'))).toEqual([]); + }); + + it('still refuses a driver without .raw()', async () => { + await expect(migrateProjectIdToEnvironmentId({} as any)).rejects.toThrow( + /must expose a \.raw\(sql, bindings\?\) method/, + ); + }); +}); 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 27601b4ec5..21013e4410 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 @@ -3,10 +3,9 @@ /** * Migration: project_id → environment_id * - * Renames the `project_id` column to `environment_id` on the metadata - * storage tables: - * - sys_metadata - * - sys_metadata_history + * Renames the `project_id` column to `environment_id` on the metadata storage + * tables — but only on the tables whose CURRENT declaration actually knows + * `environment_id`. * * Forward counterpart of {@link migrateEnvIdToProjectId} (which performed the * earlier `env_id → project_id` rename). Together they let an operator walk an @@ -15,6 +14,35 @@ * migrateEnvIdToProjectId(driver); // env_id → project_id (legacy) * migrateProjectIdToEnvironmentId(driver); // project_id → environment_id (v5) * + * ───────────────────────────────────────────────────────────────────── + * Why the table list is DERIVED and not written out (#13205) + * + * This migration is the terminal step of that chain: its target column is + * the CURRENT declared shape, so "should this table be renamed?" is not an + * independent fact — it is `does this object still declare environment_id?`. + * Written out by hand, the two drifted apart: `sys_metadata_history` stayed + * on the list after the branch/project-removal amendment (M1) removed + * `environment_id` from its declaration, so against a database whose + * physical `sys_metadata_history` still carried `project_id` this migration + * renamed it to a column NO declaration knows about — minting exactly the + * orphan column class the metadata drift audit exists to remove. + * + * The old guard could not catch it: the loop gates on `project_id` existing + * PHYSICALLY (`_columnExists`), which says nothing about the target column + * being DECLARED. So the list is now computed from the declarations in + * `@objectstack/metadata-core` (already a dependency of this package — no + * new edge), and a candidate that does not declare the target column is + * reported as `skipped_not_declared` rather than dropped silently: an + * operator reading the result sees the table was considered and why nothing + * happened, instead of having to guess whether it was forgotten again. + * + * ⚠️ The sibling `migrate-env-id-to-project-id.ts` is deliberately NOT + * changed this way. Its target (`project_id`) is an INTERMEDIATE column that + * no current declaration carries by design — gating it on today's + * declarations would disable the chain's first step entirely. The rule + * "target must be declared" is sound only for the terminal migration. + * ───────────────────────────────────────────────────────────────────── + * * (The per-type projection tables `sys_object` / `sys_view` / `sys_flow` / * `sys_agent` / `sys_tool` were removed in 2026-05 along with the projection * pipeline — see ADR 0005 addendum. They are intentionally not included.) @@ -29,24 +57,59 @@ */ import type { IDataDriver } from '@objectstack/spec/contracts'; +import { SysMetadataObject, SysMetadataHistoryObject } from '@objectstack/metadata-core'; + +/** The column this migration RENAMES AWAY FROM. */ +const SOURCE_COLUMN = 'project_id'; -const AFFECTED_TABLES = [ - 'sys_metadata', - 'sys_metadata_history', -] as const; +/** The column this migration PRODUCES. Must be declared, or the rename mints an orphan. */ +const TARGET_COLUMN = 'environment_id'; + +/** + * Every metadata storage table this migration considers. Membership here says + * "this table has, historically, carried the tenancy column" — whether the + * rename actually runs is decided by {@link AFFECTED_TABLES} below, from the + * declaration. + */ +const CANDIDATE_OBJECTS = [SysMetadataObject, SysMetadataHistoryObject] as const; + +function declaresColumn(object: { fields?: Record }, column: string): boolean { + return Object.prototype.hasOwnProperty.call(object.fields ?? {}, column); +} + +/** Candidate table names, in declaration order. */ +const CANDIDATE_TABLES: readonly string[] = CANDIDATE_OBJECTS.map((o) => o.name); + +/** + * The tables this migration will actually rename: the candidates whose CURRENT + * declaration carries {@link TARGET_COLUMN}. + * + * Exported for the pin in `migrate-project-id-to-environment-id.test.ts` (not + * re-exported from `./index.ts` — this is not package surface). + */ +export const AFFECTED_TABLES: readonly string[] = CANDIDATE_OBJECTS + .filter((o) => declaresColumn(o, TARGET_COLUMN)) + .map((o) => o.name); export interface ProjectIdToEnvironmentIdResult { table: string; - status: 'renamed' | 'already_done' | 'table_missing' | 'error'; + /** + * `skipped_not_declared` — the table is a known metadata storage table, but + * its current declaration has no `environment_id`, so renaming into it + * would create a column nothing declares. Nothing was executed. + */ + status: 'renamed' | 'already_done' | 'table_missing' | 'skipped_not_declared' | 'error'; error?: string; } /** - * Rename `project_id` → `environment_id` on all metadata tables. + * 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?)`. - * @returns Per-table migration results. + * @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, @@ -63,10 +126,18 @@ export async function migrateProjectIdToEnvironmentId( const results: ProjectIdToEnvironmentIdResult[] = []; - for (const table of AFFECTED_TABLES) { + for (const table of CANDIDATE_TABLES) { + // The declared-target gate, ahead of every physical probe: a table whose + // declaration lost `environment_id` must never be renamed INTO it, no + // matter what the physical schema still carries (#13205). + if (!AFFECTED_TABLES.includes(table)) { + results.push({ table, status: 'skipped_not_declared' }); + continue; + } + try { - const hasColumn = await _columnExists(driverAny, table, 'project_id'); - const alreadyMigrated = await _columnExists(driverAny, table, 'environment_id'); + const hasColumn = await _columnExists(driverAny, table, SOURCE_COLUMN); + const alreadyMigrated = await _columnExists(driverAny, table, TARGET_COLUMN); if (alreadyMigrated && !hasColumn) { results.push({ table, status: 'already_done' }); @@ -79,7 +150,7 @@ export async function migrateProjectIdToEnvironmentId( } await driverAny.raw( - `ALTER TABLE "${table}" RENAME COLUMN project_id TO environment_id`, + `ALTER TABLE "${table}" RENAME COLUMN ${SOURCE_COLUMN} TO ${TARGET_COLUMN}`, ); results.push({ table, status: 'renamed' });