diff --git a/.changeset/break-inline-fk-cycles.md b/.changeset/break-inline-fk-cycles.md new file mode 100644 index 000000000..2e4dcffba --- /dev/null +++ b/.changeset/break-inline-fk-cycles.md @@ -0,0 +1,22 @@ +--- +"@supabase/pg-topo": patch +--- + +fix(pg-topo): break inline foreign-key cycles instead of silently dropping statements + +A mutual foreign-key reference between two tables (or any FK loop spanning +several tables) is a real dependency cycle: neither table can be created with +its FK declared inline. Previously the topological sort dropped the cycle +participants — and every statement depending on them — from the `ordered` +result, leaving only a `CYCLE_DETECTED` diagnostic. Declarative apply then +built an incomplete schema from the truncated list, which surfaced downstream +as spurious `DROP` statements for objects that still existed +(supabase/pg-toolbelt#311). + +`analyzeAndSort` now mirrors `pg_dump`: when a cycle is detected it strips the +cross-cycle FK constraints from the offending `CREATE TABLE` statements and +re-emits them as standalone `ALTER TABLE ... ADD CONSTRAINT` statements, +preserving the constraint name, columns, and `ON DELETE`/`ON UPDATE` actions. +Self-referential FKs are kept inline. As a safety net, any statement still +left in an unbreakable cycle is appended to `ordered` in deterministic order +rather than being dropped, so consumers always receive every statement. diff --git a/packages/pg-topo/src/analyze-and-sort.ts b/packages/pg-topo/src/analyze-and-sort.ts index 3c4bc2b77..db9e60458 100644 --- a/packages/pg-topo/src/analyze-and-sort.ts +++ b/packages/pg-topo/src/analyze-and-sort.ts @@ -16,6 +16,7 @@ import type { GraphReport, StatementNode, } from "./model/types.ts"; +import { breakForeignKeyCycles } from "./rewrite/break-fk-cycles.ts"; const dedupeDiagnostics = (diagnostics: Diagnostic[]): Diagnostic[] => { const map = new Map(); @@ -114,6 +115,59 @@ const EMPTY_RESULT: AnalyzeResult = { }, }; +/** + * Classify and extract dependencies for a parsed statement, producing a + * StatementNode plus any classification diagnostics. Factored out so the + * pipeline can rebuild nodes after FK-cycle rewriting without duplicating + * the logic. + */ +const buildStatementNode = ( + statement: ParsedStatement, +): { node: StatementNode; diagnostics: Diagnostic[] } => { + const diagnostics: Diagnostic[] = []; + const statementClass = classifyStatement(statement.ast); + if (statementClass === "UNKNOWN") { + diagnostics.push({ + code: "UNKNOWN_STATEMENT_CLASS", + message: `Unsupported statement AST root '${statementClassAstNode(statement.ast) ?? "unknown"}'.`, + statementId: statement.id, + }); + } + + const extraction = extractDependencies( + statementClass, + statement.ast, + statement.annotations, + ); + + return { + node: { + id: statement.id, + sql: statement.sql, + statementClass, + provides: extraction.provides, + requires: extraction.requires, + phase: + statement.annotations.phase ?? phaseForStatementClass(statementClass), + annotations: statement.annotations, + }, + diagnostics, + }; +}; + +const buildStatementNodes = ( + statements: readonly ParsedStatement[], +): { nodes: StatementNode[]; diagnostics: Diagnostic[] } => { + const nodes: StatementNode[] = []; + const diagnostics: Diagnostic[] = []; + for (const statement of statements) { + const built = buildStatementNode(statement); + nodes.push(built.node); + diagnostics.push(...built.diagnostics); + } + return { nodes, diagnostics }; +}; + export const analyzeAndSort = async ( sql: string[], options?: AnalyzeOptions, @@ -130,49 +184,45 @@ export const analyzeAndSort = async ( }; } - const diagnostics: Diagnostic[] = []; - const parsedStatements: ParsedStatement[] = []; + const parseDiagnostics: Diagnostic[] = []; + let workingStatements: ParsedStatement[] = []; for (let i = 0; i < sql.length; i += 1) { const parsed = await parseSqlContent(sql[i], ``); - parsedStatements.push(...parsed.statements); - diagnostics.push(...parsed.diagnostics); + workingStatements.push(...parsed.statements); + parseDiagnostics.push(...parsed.diagnostics); } - const statementNodes: StatementNode[] = []; - for (const parsedStatement of parsedStatements) { - const statementClass = classifyStatement(parsedStatement.ast); - if (statementClass === "UNKNOWN") { - diagnostics.push({ - code: "UNKNOWN_STATEMENT_CLASS", - message: `Unsupported statement AST root '${statementClassAstNode(parsedStatement.ast) ?? "unknown"}'.`, - statementId: parsedStatement.id, - }); - } + let built = buildStatementNodes(workingStatements); + let statementNodes = built.nodes; + let graphState = buildGraph(statementNodes, options?.externalProviders); + let topoResult = topoSort(statementNodes, graphState.edges); - const extraction = extractDependencies( - statementClass, - parsedStatement.ast, - parsedStatement.annotations, + // Break inline foreign-key cycles (pg_dump style) by deferring the + // cross-cycle FK into a standalone ALTER TABLE ... ADD CONSTRAINT. Only + // runs when a cycle is actually present, and re-derives the graph from the + // rewritten statement set so downstream consumers see an acyclic order. + if (topoResult.cycleGroups.length > 0) { + const rewritten = await breakForeignKeyCycles( + workingStatements, + statementNodes, + topoResult.cycleGroups, ); - - statementNodes.push({ - id: parsedStatement.id, - sql: parsedStatement.sql, - statementClass, - provides: extraction.provides, - requires: extraction.requires, - phase: - parsedStatement.annotations.phase ?? - phaseForStatementClass(statementClass), - annotations: parsedStatement.annotations, - }); + if (rewritten) { + workingStatements = rewritten; + built = buildStatementNodes(workingStatements); + statementNodes = built.nodes; + graphState = buildGraph(statementNodes, options?.externalProviders); + topoResult = topoSort(statementNodes, graphState.edges); + } } - const graphState = buildGraph(statementNodes, options?.externalProviders); - diagnostics.push(...graphState.diagnostics); + const diagnostics: Diagnostic[] = [ + ...parseDiagnostics, + ...built.diagnostics, + ...graphState.diagnostics, + ]; - const topoResult = topoSort(statementNodes, graphState.edges); if (topoResult.cycleGroups.length > 0) { for (const cycleGroup of topoResult.cycleGroups) { const firstCycleIndex = cycleGroup[0]; @@ -221,7 +271,25 @@ export const analyzeAndSort = async ( } } - const ordered = topoResult.orderedIndices + // A statement must never be silently dropped from the result. Kahn's + // algorithm omits any node left in a cycle (and everything downstream of + // one), so append those trailing nodes in a deterministic order after the + // acyclic prefix. The CYCLE_DETECTED diagnostic above still flags the + // unresolved cycle; consumers (e.g. the declarative apply engine) then see + // every statement and can surface a real error instead of building an + // incomplete schema from a truncated list. + const orderedIndexSet = new Set(topoResult.orderedIndices); + const trailingIndices: number[] = []; + for (let index = 0; index < statementNodes.length; index += 1) { + if (!orderedIndexSet.has(index)) { + trailingIndices.push(index); + } + } + trailingIndices.sort((left, right) => + compareStatementIndices(left, right, statementNodes), + ); + + const ordered = [...topoResult.orderedIndices, ...trailingIndices] .map((index) => statementNodes[index]) .filter((statementNode): statementNode is StatementNode => Boolean(statementNode), diff --git a/packages/pg-topo/src/rewrite/break-fk-cycles.ts b/packages/pg-topo/src/rewrite/break-fk-cycles.ts new file mode 100644 index 000000000..9611978c0 --- /dev/null +++ b/packages/pg-topo/src/rewrite/break-fk-cycles.ts @@ -0,0 +1,284 @@ +/** + * Foreign-key cycle breaking (pg_dump style). + * + * A mutual FK reference between two `CREATE TABLE` statements (or any FK + * loop spanning several tables) is a real dependency cycle: neither table + * can be created first with its FK declared inline. PostgreSQL's own + * pg_dump handles this by creating the tables without the offending FK and + * re-adding it afterwards via `ALTER TABLE ... ADD CONSTRAINT`. + * + * This module performs the same rewrite on the parsed statement set. Given + * the cycle groups discovered by the topological sort, it: + * + * 1. Finds the `CREATE TABLE` statements participating in each cycle. + * 2. Strips every FK constraint that points at *another* table in the same + * cycle (self-referential FKs are left inline — PostgreSQL accepts those + * within a single `CREATE TABLE`). + * 3. Re-emits each stripped FK as a standalone `ALTER TABLE ... ADD + * CONSTRAINT` statement, preserving the original constraint definition + * (name, columns, `ON DELETE`/`ON UPDATE` actions, match type, etc.). + * + * The rewritten statement set is acyclic and applies cleanly in a single + * forward pass. The constraint AST node is moved verbatim into the ALTER + * command, so no FK option is lost; column-level FKs (which carry no + * `fk_attrs` because the column is implicit) gain an explicit `fk_attrs` + * entry naming the owning column. + */ + +import { deparseSql } from "plpgsql-parser"; +import type { ParsedStatement } from "../ingest/parse.ts"; +import type { StatementId, StatementNode } from "../model/types.ts"; +import { asRecord } from "../utils/ast.ts"; + +const DEFAULT_SCHEMA = "public"; + +const tableKeyFromRelation = ( + relation: Record | undefined, +): string | undefined => { + const relname = + typeof relation?.relname === "string" ? relation.relname : undefined; + if (!relname) { + return undefined; + } + const schema = + typeof relation?.schemaname === "string" + ? relation.schemaname + : DEFAULT_SCHEMA; + return `${schema}.${relname}`; +}; + +const createStmtOf = (ast: unknown): Record | undefined => + asRecord(asRecord(ast)?.CreateStmt); + +const isForeignKeyConstraint = ( + constraint: Record | undefined, +): boolean => constraint?.contype === "CONSTR_FOREIGN"; + +const referencedTableKey = ( + constraint: Record, +): string | undefined => tableKeyFromRelation(asRecord(constraint.pktable)); + +const buildAddConstraintAlter = ( + relation: unknown, + constraint: Record, +): unknown => ({ + AlterTableStmt: { + relation, + objtype: "OBJECT_TABLE", + cmds: [ + { + AlterTableCmd: { + subtype: "AT_AddConstraint", + def: { Constraint: constraint }, + }, + }, + ], + }, +}); + +type StripResult = { + /** Cloned CreateStmt AST with cross-cycle FK constraints removed. */ + createAst: unknown; + /** ALTER TABLE ADD CONSTRAINT AST nodes for each deferred FK. */ + alterAsts: unknown[]; +}; + +/** + * Remove FK constraints pointing at other tables in `cycleTableKeys` from a + * `CREATE TABLE` AST and return the deferred FKs as ALTER statements. + * Returns null when there is nothing to defer. + */ +const stripCrossCycleForeignKeys = ( + ast: unknown, + ownTableKey: string, + cycleTableKeys: ReadonlySet, +): StripResult | null => { + const clonedAst = structuredClone(ast); + const create = createStmtOf(clonedAst); + const relation = create?.relation; + if (!create || !Array.isArray(create.tableElts)) { + return null; + } + + const alterAsts: unknown[] = []; + const keptElements: unknown[] = []; + + for (const element of create.tableElts) { + const elementRecord = asRecord(element); + + // Table-level constraint, e.g. `CONSTRAINT fk FOREIGN KEY (a) REFERENCES ...` + const tableConstraint = asRecord(elementRecord?.Constraint); + if (isForeignKeyConstraint(tableConstraint) && tableConstraint) { + const refKey = referencedTableKey(tableConstraint); + if (refKey && refKey !== ownTableKey && cycleTableKeys.has(refKey)) { + alterAsts.push(buildAddConstraintAlter(relation, tableConstraint)); + continue; + } + } + + // Column-level constraint, e.g. `col uuid REFERENCES other (id)` + const columnDefinition = asRecord(elementRecord?.ColumnDef); + if (columnDefinition && Array.isArray(columnDefinition.constraints)) { + const columnName = + typeof columnDefinition.colname === "string" + ? columnDefinition.colname + : undefined; + const keptConstraints: unknown[] = []; + for (const constraintItem of columnDefinition.constraints) { + const constraint = asRecord(asRecord(constraintItem)?.Constraint); + if (isForeignKeyConstraint(constraint) && constraint && columnName) { + const refKey = referencedTableKey(constraint); + if (refKey && refKey !== ownTableKey && cycleTableKeys.has(refKey)) { + // A column-level FK omits fk_attrs (the column is implicit); make + // it explicit so the standalone ALTER knows the referencing column. + const tableLevelConstraint = { + ...constraint, + fk_attrs: [{ String: { sval: columnName } }], + }; + alterAsts.push( + buildAddConstraintAlter(relation, tableLevelConstraint), + ); + continue; + } + } + keptConstraints.push(constraintItem); + } + // Reassign so the column keeps its remaining constraints (type, NOT + // NULL, DEFAULT, self-FK, ...) while shedding the deferred FK. + columnDefinition.constraints = + keptConstraints.length > 0 ? keptConstraints : undefined; + } + + keptElements.push(element); + } + + if (alterAsts.length === 0) { + return null; + } + + create.tableElts = keptElements; + return { createAst: clonedAst, alterAsts }; +}; + +/** + * Break foreign-key dependency cycles by deferring cross-cycle FK + * constraints into standalone `ALTER TABLE ... ADD CONSTRAINT` statements. + * + * @param statements - The full parsed statement set, in input order. + * @param nodes - Statement nodes aligned 1:1 with `statements` (provides + * the classification used to pick `CREATE TABLE` cycle members). + * @param cycleGroups - Cycle node-index groups from the topological sort. + * @returns A rewritten statement set, or null if no FK could be deferred + * (e.g. the cycle is not made of inline table FKs). + */ +export const breakForeignKeyCycles = async ( + statements: readonly ParsedStatement[], + nodes: readonly StatementNode[], + cycleGroups: readonly number[][], +): Promise => { + // Map each cyclic CREATE TABLE statement index to its table key, grouped + // by the cycle it belongs to. Only FKs between two tables in the same + // cycle must be deferred. + const tableKeyByIndex = new Map(); + const cycleTableKeysByIndex = new Map>(); + + for (const group of cycleGroups) { + const tableKeys = new Set(); + const tableIndexes: number[] = []; + for (const index of group) { + if (nodes[index]?.statementClass !== "CREATE_TABLE") { + continue; + } + const create = createStmtOf(statements[index]?.ast); + const key = tableKeyFromRelation(asRecord(create?.relation)); + if (!key) { + continue; + } + tableKeyByIndex.set(index, key); + tableKeys.add(key); + tableIndexes.push(index); + } + for (const index of tableIndexes) { + cycleTableKeysByIndex.set(index, tableKeys); + } + } + + if (tableKeyByIndex.size === 0) { + return null; + } + + // Synthetic ALTER statements reuse the owning statement's file path with a + // fresh, collision-free statementIndex appended after that file's originals. + const nextIndexByFile = new Map(); + for (const statement of statements) { + const current = nextIndexByFile.get(statement.id.filePath) ?? 0; + nextIndexByFile.set( + statement.id.filePath, + Math.max(current, statement.id.statementIndex + 1), + ); + } + const allocateSyntheticId = (owner: ParsedStatement): StatementId => { + const filePath = owner.id.filePath; + const statementIndex = nextIndexByFile.get(filePath) ?? 0; + nextIndexByFile.set(filePath, statementIndex + 1); + return { + filePath, + statementIndex, + sourceOffset: owner.id.sourceOffset, + }; + }; + + const rewritten: ParsedStatement[] = []; + const appendedAlters: ParsedStatement[] = []; + let didDefer = false; + + for (let index = 0; index < statements.length; index += 1) { + const statement = statements[index]; + const ownTableKey = tableKeyByIndex.get(index); + const cycleTableKeys = cycleTableKeysByIndex.get(index); + + if (!ownTableKey || !cycleTableKeys) { + rewritten.push(statement); + continue; + } + + const stripped = stripCrossCycleForeignKeys( + statement.ast, + ownTableKey, + cycleTableKeys, + ); + if (!stripped) { + rewritten.push(statement); + continue; + } + + didDefer = true; + const createSql = await deparseSql(stripped.createAst as object); + rewritten.push({ + ...statement, + ast: stripped.createAst, + sql: createSql.trimEnd().endsWith(";") ? createSql : `${createSql};`, + }); + + for (const alterAst of stripped.alterAsts) { + const alterSql = await deparseSql(alterAst as object); + appendedAlters.push({ + id: allocateSyntheticId(statement), + ast: alterAst, + sql: alterSql.trimEnd().endsWith(";") ? alterSql : `${alterSql};`, + // ADD CONSTRAINT statements have no annotation hints of their own. + annotations: { + dependsOn: [], + requires: [], + provides: [], + }, + }); + } + } + + if (!didDefer) { + return null; + } + + return [...rewritten, ...appendedAlters]; +}; diff --git a/packages/pg-topo/test/cycle-fk-split.test.ts b/packages/pg-topo/test/cycle-fk-split.test.ts new file mode 100644 index 000000000..424a1ebc8 --- /dev/null +++ b/packages/pg-topo/test/cycle-fk-split.test.ts @@ -0,0 +1,104 @@ +import { describe, expect, test } from "bun:test"; +import { analyzeAndSort } from "../src/analyze-and-sort"; + +// Regression coverage for https://github.com/supabase/pg-toolbelt/issues/311 +// +// A mutual foreign-key cycle between two tables previously made the topo sort +// drop the cycle participants AND every statement depending on them from the +// `ordered` result. Declarative apply then built an incomplete shadow schema, +// and pg-delta emitted spurious DROPs for the missing-but-still-present +// objects. pg-topo must instead break the cycle by deferring the +// cross-table FK into a standalone ALTER TABLE ... ADD CONSTRAINT (pg_dump +// style) so every statement survives and the order is applyable. +describe("foreign-key cycle splitting (issue #311)", () => { + const ISSUE_SCHEMA = [ + `CREATE TABLE public.note ( + id uuid NOT NULL PRIMARY KEY DEFAULT gen_random_uuid(), + active_note_version_id uuid NULL REFERENCES public.note_version (id) + );`, + `CREATE TABLE public.note_version ( + id uuid NOT NULL PRIMARY KEY DEFAULT gen_random_uuid(), + content text NOT NULL, + note_id uuid NOT NULL REFERENCES public.note (id) ON DELETE CASCADE, + previous_note_version_id uuid REFERENCES public.note_version (id) + );`, + `CREATE POLICY "Note select policy" ON public.note FOR SELECT USING (true);`, + `CREATE TABLE public.note_note_link ( + id uuid PRIMARY KEY DEFAULT gen_random_uuid(), + note_id uuid NOT NULL REFERENCES public.note (id) + );`, + `CREATE TABLE public.unrelated (id int PRIMARY KEY);`, + ]; + + test("keeps every original statement in the ordered output", async () => { + const result = await analyzeAndSort(ISSUE_SCHEMA); + + // No original statement may be silently dropped. The cycle is broken by + // deferring the two cross-table FKs into standalone ALTER statements, so + // the result is the 5 originals (minus the 2 inline FKs) plus 2 ALTERs. + const nonAlter = result.ordered.filter( + (n) => n.statementClass !== "ALTER_TABLE", + ); + expect(nonAlter.length).toBe(ISSUE_SCHEMA.length); + expect(result.ordered.length).toBe(ISSUE_SCHEMA.length + 2); + + const orderedSql = result.ordered.map((n) => n.sql.toLowerCase()); + for (const fragment of [ + "create table public.note ", + "create table public.note_version", + "create policy", + "create table public.note_note_link", + "create table public.unrelated", + ]) { + expect(orderedSql.some((s) => s.includes(fragment))).toBe(true); + } + }); + + test("breaks the FK cycle so no CYCLE_DETECTED diagnostic remains", async () => { + const result = await analyzeAndSort(ISSUE_SCHEMA); + + expect( + result.diagnostics.filter((d) => d.code === "CYCLE_DETECTED"), + ).toHaveLength(0); + expect(result.graph.cycleGroups).toHaveLength(0); + }); + + test("defers cross-table FKs to ALTER TABLE but keeps self-referential FK inline", async () => { + const result = await analyzeAndSort(ISSUE_SCHEMA); + + const alterTables = result.ordered.filter( + (n) => n.statementClass === "ALTER_TABLE", + ); + // note.active_note_version_id -> note_version and + // note_version.note_id -> note are the two cross-table cycle FKs. + expect(alterTables.length).toBe(2); + const alterSql = alterTables.map((n) => n.sql.toLowerCase()).join("\n"); + expect(alterSql).toContain("add"); + expect(alterSql).toContain("foreign key"); + // ON DELETE CASCADE on note_version.note_id must be preserved. + expect(alterSql).toContain("on delete cascade"); + + // The self-referential FK (previous_note_version_id -> note_version) stays + // inline on the CREATE TABLE; it does not need deferral. + const noteVersionCreate = result.ordered.find( + (n) => + n.statementClass === "CREATE_TABLE" && + n.sql.toLowerCase().includes("create table public.note_version"), + ); + expect(noteVersionCreate?.sql.toLowerCase()).toContain( + "previous_note_version_id", + ); + expect(noteVersionCreate?.sql.toLowerCase()).toContain("references"); + }); + + test("orders both base tables before the deferred FK alters", async () => { + const result = await analyzeAndSort(ISSUE_SCHEMA); + const classes = result.ordered.map((n) => n.statementClass); + const lastCreateTable = classes.lastIndexOf("CREATE_TABLE"); + const firstAlter = classes.indexOf("ALTER_TABLE"); + expect(firstAlter).toBeGreaterThan(-1); + // every ALTER (deferred FK) comes after the cyclic CREATE TABLEs it links + expect(firstAlter).toBeGreaterThan(classes.indexOf("CREATE_TABLE")); + expect(lastCreateTable).toBeGreaterThan(-1); + }); +}); diff --git a/packages/pg-topo/test/cycle-fk-split.validation.test.ts b/packages/pg-topo/test/cycle-fk-split.validation.test.ts new file mode 100644 index 000000000..a2c296554 --- /dev/null +++ b/packages/pg-topo/test/cycle-fk-split.validation.test.ts @@ -0,0 +1,34 @@ +import { describe, expect, test } from "bun:test"; +import { analyzeAndSort } from "../src/analyze-and-sort"; +import { validateAnalyzeResultWithPostgres } from "./support/postgres-validation"; + +// Docker-backed validation for the FK cycle splitting fix (issue #311). +// Confirms the rewritten statement order actually applies against a real +// PostgreSQL instance with no runtime diagnostics. +describe("foreign-key cycle splitting applies on PostgreSQL (issue #311)", () => { + test("mutual FK cycle schema applies cleanly", async () => { + const result = await analyzeAndSort([ + `CREATE TABLE public.note ( + id uuid NOT NULL PRIMARY KEY DEFAULT gen_random_uuid(), + active_note_version_id uuid NULL REFERENCES public.note_version (id) + );`, + `CREATE TABLE public.note_version ( + id uuid NOT NULL PRIMARY KEY DEFAULT gen_random_uuid(), + content text NOT NULL, + note_id uuid NOT NULL REFERENCES public.note (id) ON DELETE CASCADE, + previous_note_version_id uuid REFERENCES public.note_version (id) + );`, + `CREATE TABLE public.note_note_link ( + id uuid PRIMARY KEY DEFAULT gen_random_uuid(), + note_id uuid NOT NULL REFERENCES public.note (id) + );`, + ]); + + expect( + result.diagnostics.filter((d) => d.code === "CYCLE_DETECTED"), + ).toHaveLength(0); + + const validation = await validateAnalyzeResultWithPostgres(result); + expect(validation.diagnostics).toHaveLength(0); + }, 120000); +});