From a06f8da9279be4cca12022911fcfda1903b572e7 Mon Sep 17 00:00:00 2001 From: pjpj Date: Fri, 26 Jun 2026 12:32:50 +0800 Subject: [PATCH] fix(pg-delta): order leading enum value additions RED: the new enum.diff unit test produced ADD 'a' BEFORE 'b' before b existed, and the new PG17 integration test failed with 22023: "b" is not an existing enum label.\n\nGREEN: the planner now only emits enum ADD VALUE statements whose BEFORE/AFTER anchors are already present in the simulated working label list. --- .changeset/fix-leading-enum-values.md | 5 ++ .../core/objects/type/enum/enum.diff.test.ts | 62 ++++++++++++++ .../src/core/objects/type/enum/enum.diff.ts | 81 ++++++++++++------- .../tests/integration/type-operations.test.ts | 40 +++++++++ 4 files changed, 158 insertions(+), 30 deletions(-) create mode 100644 .changeset/fix-leading-enum-values.md diff --git a/.changeset/fix-leading-enum-values.md b/.changeset/fix-leading-enum-values.md new file mode 100644 index 000000000..a11e83c63 --- /dev/null +++ b/.changeset/fix-leading-enum-values.md @@ -0,0 +1,5 @@ +--- +"@supabase/pg-delta": patch +--- + +Order enum value additions so `BEFORE` and `AFTER` anchors always reference labels that already exist when the statement runs. diff --git a/packages/pg-delta/src/core/objects/type/enum/enum.diff.test.ts b/packages/pg-delta/src/core/objects/type/enum/enum.diff.test.ts index 280cb3205..aa9c3693d 100644 --- a/packages/pg-delta/src/core/objects/type/enum/enum.diff.test.ts +++ b/packages/pg-delta/src/core/objects/type/enum/enum.diff.test.ts @@ -25,6 +25,20 @@ const testContext = { mainRoles: {}, }; +function testEnum(labels: string[]) { + return new Enum({ + schema: "public", + name: "e1", + owner: "o1", + labels: labels.map((label, index) => ({ + label, + sort_order: index + 1, + })), + comment: null, + privileges: [], + }); +} + describe.concurrent("enum.diff", () => { test("create and drop", () => { const props: EnumProps = { @@ -122,6 +136,30 @@ describe.concurrent("enum.diff", () => { expect(add?.position?.after).toBeUndefined(); }); + test("add multiple leading values from right to left using existing anchors", () => { + const main = testEnum(["c"]); + const branch = testEnum(["a", "b", "c"]); + + const changes = diffEnums( + testContext, + { [main.stableId]: main }, + { [branch.stableId]: branch }, + ); + const adds = changes.filter( + (c): c is AlterEnumAddValue => c instanceof AlterEnumAddValue, + ); + + expect( + adds.map((add) => ({ + newValue: add.newValue, + position: add.position, + })), + ).toEqual([ + { newValue: "b", position: { before: "c" } }, + { newValue: "a", position: { before: "b" } }, + ]); + }); + test("add value in middle (AFTER previous)", () => { const main = new Enum({ schema: "public", @@ -160,6 +198,30 @@ describe.concurrent("enum.diff", () => { expect(add?.position?.before).toBeUndefined(); }); + test("add multiple middle values left to right after existing anchor", () => { + const main = testEnum(["a", "d"]); + const branch = testEnum(["a", "b", "c", "d"]); + + const changes = diffEnums( + testContext, + { [main.stableId]: main }, + { [branch.stableId]: branch }, + ); + const adds = changes.filter( + (c): c is AlterEnumAddValue => c instanceof AlterEnumAddValue, + ); + + expect( + adds.map((add) => ({ + newValue: add.newValue, + position: add.position, + })), + ).toEqual([ + { newValue: "b", position: { after: "a" } }, + { newValue: "c", position: { after: "b" } }, + ]); + }); + test("add value at end (AFTER last)", () => { const main = new Enum({ schema: "public", diff --git a/packages/pg-delta/src/core/objects/type/enum/enum.diff.ts b/packages/pg-delta/src/core/objects/type/enum/enum.diff.ts index 3a44b6caa..54a419d1c 100644 --- a/packages/pg-delta/src/core/objects/type/enum/enum.diff.ts +++ b/packages/pg-delta/src/core/objects/type/enum/enum.diff.ts @@ -299,39 +299,60 @@ function diffEnumLabels(mainEnum: Enum, branchEnum: Enum): EnumChange[] { (a, b) => a.sort_order - b.sort_order, ); const workingLabels = [...mainEnum.labels].map((l) => l.label); + const pendingValues = new Set(addedValues); - for (const newValue of addedValues) { - const branchIdx = branchOrdered.findIndex((l) => l.label === newValue); - if (branchIdx === -1) continue; - - const prevBranch = branchOrdered[branchIdx - 1]?.label; - const nextBranch = branchOrdered[branchIdx + 1]?.label; - - let position: { before?: string; after?: string } | undefined; - - // Prefer AFTER when prevBranch exists in workingLabels (more natural for sequential additions) - // Use BEFORE only when we need to insert before the first value or when prevBranch doesn't exist - if (prevBranch && workingLabels.includes(prevBranch)) { - position = { after: prevBranch }; - // Insert after the previous label in our working list - const prevIdx = workingLabels.indexOf(prevBranch); - workingLabels.splice(prevIdx + 1, 0, newValue); - } else if (nextBranch && workingLabels.includes(nextBranch)) { - // Insert before nextBranch when prevBranch doesn't exist (e.g., adding at beginning) - position = { before: nextBranch }; - const nextIdx = workingLabels.indexOf(nextBranch); - workingLabels.splice(nextIdx, 0, newValue); - } else if (nextBranch) { - // nextBranch exists but not in workingLabels yet (shouldn't happen in normal flow) - position = { before: nextBranch }; - workingLabels.push(newValue); - } else { - // Fallback: append to the end - position = { after: workingLabels[workingLabels.length - 1] }; - workingLabels.push(newValue); + while (pendingValues.size > 0) { + let emitted = false; + + for (const newValue of pendingValues) { + const branchIdx = branchOrdered.findIndex((l) => l.label === newValue); + if (branchIdx === -1) { + pendingValues.delete(newValue); + emitted = true; + continue; + } + + const prevBranch = branchOrdered[branchIdx - 1]?.label; + const nextBranch = branchOrdered[branchIdx + 1]?.label; + + let position: { before?: string; after?: string } | undefined; + let insertIdx: number | undefined; + + // Prefer AFTER when prevBranch exists in workingLabels (more natural for sequential additions) + // Use BEFORE only when we need to insert before the first value or when prevBranch doesn't exist + if (prevBranch && workingLabels.includes(prevBranch)) { + position = { after: prevBranch }; + // Insert after the previous label in our working list + insertIdx = workingLabels.indexOf(prevBranch) + 1; + } else if (nextBranch && workingLabels.includes(nextBranch)) { + // Insert before nextBranch when prevBranch doesn't exist (e.g., adding at beginning) + position = { before: nextBranch }; + insertIdx = workingLabels.indexOf(nextBranch); + } else if (nextBranch) { + // nextBranch exists but not in workingLabels yet (shouldn't happen in normal flow) + continue; + } else { + // Fallback: append to the end + if (prevBranch) continue; + if (workingLabels.length === 0) continue; + position = { after: workingLabels[workingLabels.length - 1] }; + insertIdx = workingLabels.length; + } + + if (insertIdx === undefined) continue; + workingLabels.splice(insertIdx, 0, newValue); + changes.push( + new AlterEnumAddValue({ enum: mainEnum, newValue, position }), + ); + pendingValues.delete(newValue); + emitted = true; } - changes.push(new AlterEnumAddValue({ enum: mainEnum, newValue, position })); + if (!emitted) { + throw new Error( + `Unable to plan enum label additions for ${mainEnum.schema}.${mainEnum.name}: no pending value has an existing enum label anchor`, + ); + } } // Complex changes (removals, resorting) are currently not auto-handled. diff --git a/packages/pg-delta/tests/integration/type-operations.test.ts b/packages/pg-delta/tests/integration/type-operations.test.ts index 145aeebca..b413f611e 100644 --- a/packages/pg-delta/tests/integration/type-operations.test.ts +++ b/packages/pg-delta/tests/integration/type-operations.test.ts @@ -27,6 +27,46 @@ for (const pgVersion of POSTGRES_VERSIONS) { }); }), ); + test( + "add multiple leading enum values before the first existing value", + withDb(pgVersion, async (db) => { + await roundtripFidelityTest({ + mainSession: db.main, + branchSession: db.branch, + initialSetup: ` + CREATE SCHEMA test_schema; + CREATE TYPE test_schema.status AS ENUM ('c'); + `, + testSql: ` + DROP TYPE test_schema.status; + CREATE TYPE test_schema.status AS ENUM ('a', 'b', 'c'); + `, + }); + }), + ); + test( + "add leading and trailing enum values around an existing value", + withDb(pgVersion, async (db) => { + await roundtripFidelityTest({ + mainSession: db.main, + branchSession: db.branch, + initialSetup: ` + CREATE SCHEMA test_schema; + CREATE TYPE test_schema.status AS ENUM ('b'); + `, + testSql: ` + DROP TYPE test_schema.status; + CREATE TYPE test_schema.status AS ENUM ('a', 'b', 'c'); + `, + assertSqlStatements: (sqlStatements) => { + expect(sqlStatements).toEqual([ + "ALTER TYPE test_schema.status ADD VALUE 'a' BEFORE 'b'", + "ALTER TYPE test_schema.status ADD VALUE 'c' AFTER 'b'", + ]); + }, + }); + }), + ); test( "add enum value before setting default to the new value", withDb(pgVersion, async (db) => {