fix(sales-orders): pin the sales_order_items embed FK and teach the embed guard composite keys (#2207)
* fix(sales-orders): pin the sales_order_items embed FK and teach the embed guard composite keys Migration 20260902180000_sales_orders_hardening added a composite (sales_order_id, company_id) foreign key from sales_order_items to sales_orders next to the original single-column one. PostgREST then saw two relationships and answered every `items:sales_order_items(*)` embed with HTTP 300 / PGRST201, so kundorder list, detail, create and the MCP list tool all failed on prod and staging with "Oväntat serverfel". - Hint the three embeds with `!sales_order_items_sales_order_id_fkey` (route, load service, MCP list tool). - scripts/checks/ambiguous-embed.mjs only parsed single-column `FOREIGN KEY (col)`, which is why the ratchet reported 0 for this pair. It now reads composite column lists (named or default constraint name) in both CREATE TABLE and ALTER TABLE, derives the same 17 ambiguous pairs prod's pg_constraint reports, and flags all three shipped sites on main. - Unit tests for the composite shapes: alongside a single-column key, replacing one, and inline in CREATE TABLE. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q7xJQL2aZo6iRHxCZNntUq * fix(checks): drop composite embed edges when DROP COLUMN removes a member column Postgres drops every foreign key a column takes part in, so the ambiguous-embed parser must release a composite edge (and its constraint name) when one of its columns is dropped, not only the single-column key. Otherwise a later migration would keep a pair armed for a relationship that no longer exists and reject valid embeds. Regression case added. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q7xJQL2aZo6iRHxCZNntUq --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Mattsson
co-authored by
Claude Fable 5.1
Claude Fable 5.1
parent
4c6feea64d
commit
fefef038c5
@@ -245,15 +245,119 @@ describe('ambiguous-embed: pair derivation from the migration history', () => {
|
||||
).toBe(0)
|
||||
})
|
||||
|
||||
it('counts a composite foreign key next to a single-column one', () => {
|
||||
// 20260902130000_sales_orders.sql + 20260902180000_sales_orders_hardening.sql:
|
||||
// the composite (sales_order_id, company_id) guard is a second relationship
|
||||
// in PostgREST's eyes. A single-column-only parser derived one edge here and
|
||||
// let `items:sales_order_items(*)` ship un-hinted; prod answered PGRST201
|
||||
// on every kundorder load (2026-09-03).
|
||||
const dir = migrationsRoot({
|
||||
'20260902130000_sales_orders.sql': `
|
||||
CREATE TABLE public.sales_order_items (
|
||||
id uuid PRIMARY KEY,
|
||||
company_id uuid NOT NULL REFERENCES public.companies(id) ON DELETE CASCADE,
|
||||
sales_order_id uuid NOT NULL REFERENCES public.sales_orders(id) ON DELETE CASCADE
|
||||
);
|
||||
`,
|
||||
'20260902180000_sales_orders_hardening.sql': `
|
||||
ALTER TABLE public.sales_order_items
|
||||
DROP CONSTRAINT IF EXISTS sales_order_items_order_company_fkey;
|
||||
ALTER TABLE public.sales_order_items
|
||||
ADD CONSTRAINT sales_order_items_order_company_fkey
|
||||
FOREIGN KEY (sales_order_id, company_id)
|
||||
REFERENCES public.sales_orders (id, company_id)
|
||||
ON DELETE CASCADE;
|
||||
`,
|
||||
})
|
||||
expect([...deriveAmbiguousPairs(dir)]).toEqual([pairKey('sales_order_items', 'sales_orders')])
|
||||
})
|
||||
|
||||
it('does not count a composite foreign key that REPLACED the single-column one', () => {
|
||||
// 20260721101500_harden_tax_assessment_notices.sql shape: the old
|
||||
// single-column constraint is dropped and the composite added, leaving one
|
||||
// relationship. Prod agrees: tax_assessment_notices|fiscal_periods is not
|
||||
// in the pg_constraint list.
|
||||
const dir = migrationsRoot({
|
||||
'1_base.sql': `
|
||||
CREATE TABLE public.tax_assessment_notices (
|
||||
id uuid PRIMARY KEY,
|
||||
company_id uuid NOT NULL REFERENCES public.companies(id),
|
||||
fiscal_period_id uuid REFERENCES public.fiscal_periods(id)
|
||||
);
|
||||
`,
|
||||
'2_harden.sql': `
|
||||
ALTER TABLE public.tax_assessment_notices
|
||||
DROP CONSTRAINT IF EXISTS tax_assessment_notices_fiscal_period_id_fkey;
|
||||
ALTER TABLE public.tax_assessment_notices
|
||||
ADD CONSTRAINT tax_assessment_notices_fiscal_period_company_fkey
|
||||
FOREIGN KEY (fiscal_period_id, company_id)
|
||||
REFERENCES public.fiscal_periods (id, company_id);
|
||||
`,
|
||||
})
|
||||
expect(deriveAmbiguousPairs(dir).size).toBe(0)
|
||||
})
|
||||
|
||||
it('drops a composite edge when DROP COLUMN removes one of its columns', () => {
|
||||
// Postgres drops every foreign key a column takes part in. Keeping the
|
||||
// composite edge would arm a pair that no longer exists and reject valid
|
||||
// embeds (CodeRabbit on PR #2207).
|
||||
const base = `
|
||||
CREATE TABLE public.sales_order_items (
|
||||
id uuid PRIMARY KEY,
|
||||
company_id uuid NOT NULL,
|
||||
sales_order_id uuid NOT NULL REFERENCES public.sales_orders(id),
|
||||
CONSTRAINT sales_order_items_order_company_fkey
|
||||
FOREIGN KEY (sales_order_id, company_id) REFERENCES public.sales_orders(id, company_id)
|
||||
);
|
||||
`
|
||||
expect([...deriveAmbiguousPairs(migrationsRoot({ '1_a.sql': base }))]).toEqual([
|
||||
pairKey('sales_order_items', 'sales_orders'),
|
||||
])
|
||||
expect(
|
||||
deriveAmbiguousPairs(
|
||||
migrationsRoot({
|
||||
'1_a.sql': base,
|
||||
'2_b.sql': `ALTER TABLE public.sales_order_items DROP COLUMN company_id;`,
|
||||
}),
|
||||
).size,
|
||||
).toBe(0)
|
||||
// The dropped constraint's name is released too: a later DROP CONSTRAINT
|
||||
// by that name must not delete an unrelated edge.
|
||||
expect(
|
||||
deriveAmbiguousPairs(
|
||||
migrationsRoot({
|
||||
'1_a.sql': base,
|
||||
'2_b.sql': `ALTER TABLE public.sales_order_items DROP COLUMN company_id;`,
|
||||
'3_c.sql': `ALTER TABLE public.sales_order_items DROP CONSTRAINT IF EXISTS sales_order_items_order_company_fkey;`,
|
||||
}),
|
||||
).size,
|
||||
).toBe(0)
|
||||
})
|
||||
|
||||
it('reads a composite table-level constraint inside CREATE TABLE', () => {
|
||||
const dir = migrationsRoot({
|
||||
'1_a.sql': `
|
||||
CREATE TABLE public.a (
|
||||
id uuid PRIMARY KEY,
|
||||
company_id uuid NOT NULL,
|
||||
b_id uuid REFERENCES public.b(id),
|
||||
CONSTRAINT a_b_company_fkey FOREIGN KEY (b_id, company_id) REFERENCES public.b(id, company_id)
|
||||
);
|
||||
`,
|
||||
})
|
||||
expect([...deriveAmbiguousPairs(dir)]).toEqual([pairKey('a', 'b')])
|
||||
})
|
||||
|
||||
it('matches the live schema on the real migration history', () => {
|
||||
// Verified against prod (pwxtzglxptnnvjrpixpg) on 2026-09-01 with the
|
||||
// pg_constraint query in ambiguous-embed.mjs: the same 15 pairs.
|
||||
// Verified against prod (pwxtzglxptnnvjrpixpg) on 2026-09-03 with the
|
||||
// pg_constraint query in ambiguous-embed.mjs: the same 17 pairs.
|
||||
const pairs = deriveAmbiguousPairs(
|
||||
path.join(__dirname, '..', '..', '..', 'supabase', 'migrations'),
|
||||
)
|
||||
expect(pairs.has(pairKey('journal_entries', 'fiscal_periods'))).toBe(true)
|
||||
expect(pairs.has(pairKey('journal_entries', 'salary_runs'))).toBe(true)
|
||||
expect(pairs.has(pairKey('supplier_invoices', 'transactions'))).toBe(true)
|
||||
expect(pairs.has(pairKey('sales_order_items', 'sales_orders'))).toBe(true)
|
||||
// Single-foreign-key pairs that legitimate code embeds without a hint.
|
||||
expect(pairs.has(pairKey('journal_entries', 'journal_entry_lines'))).toBe(false)
|
||||
expect(pairs.has(pairKey('supplier_invoice_payments', 'supplier_invoices'))).toBe(false)
|
||||
|
||||
@@ -47,7 +47,12 @@
|
||||
* where c.contype = 'f'
|
||||
* group by 1, 2 having count(*) > 1 order by 1, 2;
|
||||
*
|
||||
* On 2026-09-01 that returned the same 15 pairs the parser derives.
|
||||
* On 2026-09-03 that returned the same 17 pairs the parser derives. Composite
|
||||
* foreign keys count: until 2026-09-03 the parser only read single-column
|
||||
* `FOREIGN KEY (col)`, so the composite (sales_order_id, company_id) guard in
|
||||
* 20260902180000_sales_orders_hardening.sql was invisible to it and the
|
||||
* un-hinted `items:sales_order_items(*)` embeds shipped, taking every
|
||||
* kundorder list and detail load down with PGRST201 (fixed 2026-09-03).
|
||||
*
|
||||
* No baseline: the count is 0, any new un-hinted ambiguous embed is a hard
|
||||
* failure.
|
||||
@@ -92,10 +97,16 @@ export function deriveForeignKeys(migrationsDir) {
|
||||
// Unnamed foreign keys get Postgres's default `<table>_<column>_fkey`.
|
||||
const byConstraint = new Map()
|
||||
|
||||
// `column` is one column or a composite list ("sales_order_id, company_id").
|
||||
// A composite foreign key is its own edge, distinct from a single-column one
|
||||
// on its leading column: PostgREST counts both, which is what made
|
||||
// sales_order_items -> sales_orders ambiguous (migration 20260902180000)
|
||||
// while this parser, then single-column only, still derived one edge.
|
||||
const addEdge = (table, column, target, constraintName) => {
|
||||
const key = `${table}.${column}`
|
||||
const columns = column.split(',').map((c) => normIdent(c.trim())).filter(Boolean)
|
||||
const key = `${table}.${columns.join(',')}`
|
||||
edges.set(key, target)
|
||||
byConstraint.set(constraintName ?? `${table}_${column}_fkey`, key)
|
||||
byConstraint.set(constraintName ?? `${table}_${columns.join('_')}_fkey`, key)
|
||||
}
|
||||
|
||||
let files
|
||||
@@ -121,11 +132,12 @@ export function deriveForeignKeys(migrationsDir) {
|
||||
for (const m of stmt.matchAll(/(?:^|,)\s*([\w"]+)\s+[^,()]*?references\s+([\w".]+)/gis)) {
|
||||
addEdge(table, normIdent(m[1]), normIdent(m[2]))
|
||||
}
|
||||
// `foreign key (col) references public.other(id)` as a table constraint.
|
||||
// `foreign key (col[, col]) references public.other(id[, id])` as a
|
||||
// table constraint, named or not.
|
||||
for (const m of stmt.matchAll(
|
||||
/foreign\s+key\s*\(\s*([\w"]+)\s*\)\s*references\s+([\w".]+)/gi,
|
||||
/(?:constraint\s+([\w"]+)\s+)?foreign\s+key\s*\(\s*([\w",\s]+?)\s*\)\s*references\s+([\w".]+)/gi,
|
||||
)) {
|
||||
addEdge(table, normIdent(m[1]), normIdent(m[2]))
|
||||
addEdge(table, m[2], normIdent(m[3]), m[1] && normIdent(m[1]))
|
||||
}
|
||||
continue
|
||||
}
|
||||
@@ -133,9 +145,9 @@ export function deriveForeignKeys(migrationsDir) {
|
||||
if (altered) {
|
||||
const table = normIdent(altered[1])
|
||||
for (const m of stmt.matchAll(
|
||||
/(?:add\s+constraint\s+([\w"]+)\s+)?foreign\s+key\s*\(\s*([\w"]+)\s*\)\s*references\s+([\w".]+)/gi,
|
||||
/(?:add\s+constraint\s+([\w"]+)\s+)?foreign\s+key\s*\(\s*([\w",\s]+?)\s*\)\s*references\s+([\w".]+)/gi,
|
||||
)) {
|
||||
addEdge(table, normIdent(m[2]), normIdent(m[3]), m[1] && normIdent(m[1]))
|
||||
addEdge(table, m[2], normIdent(m[3]), m[1] && normIdent(m[1]))
|
||||
}
|
||||
for (const m of stmt.matchAll(
|
||||
/add\s+column\s+(?:if\s+not\s+exists\s+)?([\w"]+)\s+[^,;]*?references\s+([\w".]+)/gi,
|
||||
@@ -143,7 +155,17 @@ export function deriveForeignKeys(migrationsDir) {
|
||||
addEdge(table, normIdent(m[1]), normIdent(m[2]))
|
||||
}
|
||||
for (const m of stmt.matchAll(/drop\s+column\s+(?:if\s+exists\s+)?([\w"]+)/gi)) {
|
||||
edges.delete(`${table}.${normIdent(m[1])}`)
|
||||
// Postgres drops every foreign key the column takes part in, so a
|
||||
// composite edge listing it goes too, not only the single-column key.
|
||||
const column = normIdent(m[1])
|
||||
for (const key of [...edges.keys()]) {
|
||||
if (!key.startsWith(`${table}.`)) continue
|
||||
if (!key.slice(table.length + 1).split(',').includes(column)) continue
|
||||
edges.delete(key)
|
||||
for (const [name, edge] of [...byConstraint]) {
|
||||
if (edge === key) byConstraint.delete(name)
|
||||
}
|
||||
}
|
||||
}
|
||||
for (const m of stmt.matchAll(/drop\s+constraint\s+(?:if\s+exists\s+)?([\w"]+)/gi)) {
|
||||
const edge = byConstraint.get(normIdent(m[1]))
|
||||
|
||||
Reference in New Issue
Block a user