fix(documents): name the journal_entries/fiscal_periods relationship so supplier-invoice underlag can anchor (#2109)
Prod has three foreign keys between journal_entries and fiscal_periods, so
PostgREST answers PGRST201 to any embed of that pair that does not name the
relationship. pickAnchorEntry() destructured only data, so the error was
dropped and the helper returned null on every call since it shipped on
2026-07-27: supplier-invoice underlag has never once anchored in production.
Users see "Underlag saknas" on a verifikat that plainly shows the invoice PDF.
Names the constraint, matching the already-merged sibling fix in
lib/transactions/inbox-underlag.ts (6a40b3c0e), and handles the error instead
of dropping it.
Adds scripts/checks/ambiguous-embed.mjs to the ratchet guard, because neither
test layer can see this class: a mocked Supabase client never resolves a
relationship, and pg-real bypasses PostgREST entirely. The check derives the
ambiguous table pairs by parsing supabase/migrations, so a migration adding a
second foreign key between two tables arms the guard on the same commit; the
derived list reproduces prod's pg_constraint output exactly. It accepts both
PostgREST hint forms (constraint name and FK column name, both in use here) and
parses aliased embeds, which is the shape the real bug took.
Claude-Session: https://claude.ai/code/session_016ifKg6Ec67A39oxfGPU1yc
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Jakob Wennberg
@@ -1427,6 +1427,7 @@ One line per decision: `[YYYY-MM-DD] <decision>: <why>`. Appended by agents and
|
||||
[2026-09-01] multi_user skeptic fixes: Stripe cancel EXPIRES the multi_user stripe grant instead of deleting it (grace anchor; other grants still deleted per freeze-and-retain); app-side state checks go RPC-first via SECURITY DEFINER company_multi_user_state (capability_grants RLS hides team rows from non-team users, byrå clients would misread as frozen); byra-kind teams get a standing team-scoped multi_user grant via backfill + teams trigger (WL-10 assumption made real; partner billing is out-of-band); PGRST202 on resolution fails OPEN (pre-migration DB has no multi_user rows: gated fallback would freeze all non-owners); /api/v1 got the same dormancy gate as MCP. RLS-level enforcement and the mid-session API fallback write-back window stay v2 follow-ups (documented, same class as pre-existing stale-preference fallback).
|
||||
[2026-09-01] Declined CodeRabbit's UpgradeNote suggestion (PR #1758 follow-up) to append the self-host connector sentence to children instead of replacing them: every caller's children is hosted subscription copy ("... kräver ett abonnemang"), so appending would show subscription wording on a self-host, the exact thing the branch exists to avoid; the "CSV/SIE import stays free" text it cited is a code comment in BankSyncNowButton, not children. Replace-on-self-host stays; a dedicated selfHosted children prop can come when a caller actually needs per-panel reassurance there.
|
||||
[2026-09-01] getConnectorConfig() rebuilds baseUrl as origin + path (userinfo/query/fragment stripped, warn-logged without the raw value): /api/connector/status echoes baseUrl to the operator and the sync/proxy URLs get paths appended, so nothing secret-shaped pasted into GNUBOK_CONNECT_URL may survive; the stripped parts were never meaningful in a base URL. The status route is also Cache-Control: no-store (key prefix + wiring layout out of shared browser caches).
|
||||
[2026-09-01] PostgREST embeds between a table pair with more than one foreign key must name the relationship. Enforced by scripts/checks/ambiguous-embed.mjs in the ratchet guard, because neither test layer can see this class: a mocked Supabase client never resolves a relationship, and pg-real bypasses PostgREST entirely. Third appearance, second shipped instance.
|
||||
[2026-09-01] WORM integrity check moved to a new document_integrity_checks table instead of narrowing enforce_period_lock_documents(). CLAUDE.md declares migration 017's enforcement triggers legally required and never-touch, and the nightly stamp does not need to write document_attachments at all. Trades one extra table for leaving the BFL enforcement surface untouched.
|
||||
[2026-09-01] processing_history PII: the sender address and mail subject are dropped outright from RateLimitedDropped and AttachmentsTruncated rather than hashed, and the strip ships in the same commit as, and ahead of, the migration registering those event types. Registering first would start persisting PII into an append-only table the archive's erasure path excludes.
|
||||
[2026-09-01] White-label backend guard inverted from an allowlist of protected hosts to an assertion that any customer-facing production host is served by the production Supabase project. This reverses the earlier explicit-allowlist decision: that model failed open for improveone.accounted.se, which was serving a byra login page wired to the staging project with no alert, because it was never added to the list.
|
||||
|
||||
@@ -254,6 +254,73 @@ describe('anchorSupplierInvoiceDocument', () => {
|
||||
).resolves.toBeNull()
|
||||
})
|
||||
|
||||
it('names the foreign key in the fiscal_periods embed', async () => {
|
||||
// fiscal_periods points back at journal_entries twice (closing_entry_id,
|
||||
// opening_balance_entry_id), so the bare embed is ambiguous and PostgREST
|
||||
// answers PGRST201. The bare form shipped 2026-07-27 and this helper
|
||||
// anchored nothing in production until 2026-09-01. A mocked client resolves
|
||||
// no relationship, so asserting the string is the only thing a unit test
|
||||
// can do here; scripts/checks/ambiguous-embed.mjs is the repo-wide guard.
|
||||
const { supabase, enqueueMany, findCall } = createQueuedMockSupabase()
|
||||
enqueueMany([
|
||||
{
|
||||
data: {
|
||||
id: 'si-1',
|
||||
document_id: 'doc-1',
|
||||
registration_journal_entry_id: 'je-reg',
|
||||
payment_journal_entry_id: null,
|
||||
},
|
||||
},
|
||||
{ data: { id: 'doc-1', journal_entry_id: null, is_current_version: true } },
|
||||
{ data: [] },
|
||||
{ data: [{ id: 'je-reg', status: 'posted', fiscal_period: openPeriod }] },
|
||||
{ data: [{ id: 'doc-1' }] },
|
||||
])
|
||||
|
||||
await anchorSupplierInvoiceDocument(supabase as unknown as SupabaseClient, 'company-1', 'si-1')
|
||||
|
||||
expect(findCall('journal_entries', 'select')?.[0]).toContain(
|
||||
'fiscal_periods!journal_entries_fiscal_period_id_fkey',
|
||||
)
|
||||
})
|
||||
|
||||
it('logs the reason when the period lock state cannot be read, and anchors nothing', async () => {
|
||||
// Failing closed is right: never anchor on a lock state we could not read.
|
||||
// Failing SILENTLY is what let the PGRST201 above sit dead for five weeks,
|
||||
// with the caller's "no verifikat can anchor it" warning as the only
|
||||
// signal, naming a cause that had nothing to do with it.
|
||||
const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||
const { supabase, enqueueMany } = createQueuedMockSupabase()
|
||||
enqueueMany([
|
||||
{
|
||||
data: {
|
||||
id: 'si-1',
|
||||
document_id: 'doc-1',
|
||||
registration_journal_entry_id: 'je-reg',
|
||||
payment_journal_entry_id: null,
|
||||
},
|
||||
},
|
||||
{ data: { id: 'doc-1', journal_entry_id: null, is_current_version: true } },
|
||||
{ data: [] },
|
||||
{
|
||||
error: {
|
||||
message:
|
||||
"Could not embed because more than one relationship was found for 'journal_entries' and 'fiscal_periods'",
|
||||
},
|
||||
},
|
||||
])
|
||||
|
||||
await expect(
|
||||
anchorSupplierInvoiceDocument(supabase as unknown as SupabaseClient, 'company-1', 'si-1'),
|
||||
).resolves.toBeNull()
|
||||
// Stopped at the failed read: no UPDATE was attempted.
|
||||
expect(supabase.from).toHaveBeenCalledTimes(4)
|
||||
const logged = consoleError.mock.calls.flat().join(' ')
|
||||
expect(logged).toContain('failed to resolve period lock state for supplier invoice anchoring')
|
||||
expect(logged).toContain('more than one relationship')
|
||||
consoleError.mockRestore()
|
||||
})
|
||||
|
||||
it('reports failure as null instead of throwing at the caller', async () => {
|
||||
const { supabase, enqueueMany } = createQueuedMockSupabase()
|
||||
enqueueMany([
|
||||
|
||||
@@ -176,12 +176,31 @@ async function pickAnchorEntry(
|
||||
}
|
||||
if (candidates.length === 0) return null
|
||||
|
||||
const { data: entries } = await supabase
|
||||
const { data: entries, error } = await supabase
|
||||
.from('journal_entries')
|
||||
.select('id, status, fiscal_period:fiscal_periods(is_closed, locked_at)')
|
||||
// fiscal_periods also points back at journal_entries (closing_entry_id,
|
||||
// opening_balance_entry_id), so PostgREST refuses the bare embed as
|
||||
// ambiguous; name the FK explicitly.
|
||||
.select(
|
||||
'id, status, fiscal_period:fiscal_periods!journal_entries_fiscal_period_id_fkey(is_closed, locked_at)',
|
||||
)
|
||||
.eq('company_id', companyId)
|
||||
.in('id', candidates)
|
||||
|
||||
if (error) {
|
||||
// Fail closed: never anchor on a lock state we could not read. But say so.
|
||||
// The bare embed above returned PGRST201 on every call from 2026-07-27
|
||||
// onwards and the result was dropped on the floor, so the caller's "no
|
||||
// verifikat can anchor it" warning was the only signal, and it named the
|
||||
// wrong cause.
|
||||
log.error('failed to resolve period lock state for supplier invoice anchoring', {
|
||||
companyId,
|
||||
supplierInvoiceId,
|
||||
reason: error.message,
|
||||
})
|
||||
return null
|
||||
}
|
||||
|
||||
type EntryRow = {
|
||||
id: string
|
||||
status: string
|
||||
|
||||
@@ -0,0 +1,307 @@
|
||||
/**
|
||||
* Proof that the ambiguous-embed ratchet catches the shape that shipped twice
|
||||
* and leaves both legitimate hint forms alone. Offending fixtures live only in
|
||||
* these strings and in an OS temp directory the end-to-end cases create and
|
||||
* delete.
|
||||
*/
|
||||
import { describe, it, expect, afterAll } from 'vitest'
|
||||
import fs from 'node:fs'
|
||||
import os from 'node:os'
|
||||
import path from 'node:path'
|
||||
import {
|
||||
deriveAmbiguousPairs,
|
||||
findAmbiguousEmbeds,
|
||||
findAmbiguousEmbedsInSource,
|
||||
pairKey,
|
||||
parseEmbeds,
|
||||
} from '../ambiguous-embed.mjs'
|
||||
|
||||
const tempDirs: string[] = []
|
||||
afterAll(() => {
|
||||
for (const dir of tempDirs) fs.rmSync(dir, { recursive: true, force: true })
|
||||
})
|
||||
|
||||
const tempRoot = (prefix: string) => {
|
||||
const root = fs.mkdtempSync(path.join(os.tmpdir(), prefix))
|
||||
tempDirs.push(root)
|
||||
return root
|
||||
}
|
||||
|
||||
const write = (root: string, rel: string, content: string) => {
|
||||
const full = path.join(root, rel)
|
||||
fs.mkdirSync(path.dirname(full), { recursive: true })
|
||||
fs.writeFileSync(full, content)
|
||||
}
|
||||
|
||||
// Only the pair this guard exists for. Every other pair the fixtures embed
|
||||
// (journal_entries -> journal_entry_lines, transactions -> journal_entries) is
|
||||
// absent from the set, which is exactly how a single-foreign-key pair looks to
|
||||
// the scanner: it must never be flagged.
|
||||
const PAIRS = new Set([pairKey('journal_entries', 'fiscal_periods')])
|
||||
|
||||
const scan = (source: string) =>
|
||||
findAmbiguousEmbedsInSource(source, 'fixture.ts', PAIRS).map(
|
||||
(f: { from: string; target: string }) => `${f.from}->${f.target}`,
|
||||
)
|
||||
|
||||
describe('ambiguous-embed: the shape that shipped', () => {
|
||||
it('flags an ALIASED embed with no relationship named', () => {
|
||||
// lib/core/documents/supplier-invoice-underlag.ts as it shipped 2026-07-27.
|
||||
// A tokenizer that only looks for `<table>(` after a comma, space or paren
|
||||
// misses this, which is the whole reason the bug survived two reviews.
|
||||
expect(
|
||||
scan(
|
||||
`const { data } = await supabase
|
||||
.from('journal_entries')
|
||||
.select('id, status, fiscal_period:fiscal_periods(is_closed, locked_at)')`,
|
||||
),
|
||||
).toEqual(['journal_entries->fiscal_periods'])
|
||||
})
|
||||
|
||||
it('flags the bare, unaliased embed too', () => {
|
||||
expect(scan(`supabase.from('journal_entries').select('id, fiscal_periods(is_closed)')`)).toEqual(
|
||||
['journal_entries->fiscal_periods'],
|
||||
)
|
||||
})
|
||||
|
||||
it('flags the embed written in the other direction', () => {
|
||||
expect(
|
||||
scan(`supabase.from('fiscal_periods').select('id, journal_entries(voucher_number)')`),
|
||||
).toEqual(['fiscal_periods->journal_entries'])
|
||||
})
|
||||
})
|
||||
|
||||
describe('ambiguous-embed: both hint forms PostgREST accepts', () => {
|
||||
it('accepts the constraint name', () => {
|
||||
// lib/transactions/inbox-underlag.ts, lib/pending-operations/commit.ts.
|
||||
expect(
|
||||
scan(
|
||||
`supabase.from('journal_entries').select('id, fiscal_period:fiscal_periods!journal_entries_fiscal_period_id_fkey(is_closed, locked_at)')`,
|
||||
),
|
||||
).toEqual([])
|
||||
})
|
||||
|
||||
it('accepts the foreign key COLUMN name', () => {
|
||||
// lib/import/opening-balance/cascade.ts, lib/invoices/invoice-matching.ts.
|
||||
// A `*_fkey`-only matcher would hard-fail six legitimate lines here.
|
||||
expect(
|
||||
scan(
|
||||
`supabase.from('fiscal_periods').select('id, opening_balance_entry:journal_entries!opening_balance_entry_id(voucher_series, voucher_number)')`,
|
||||
),
|
||||
).toEqual([])
|
||||
})
|
||||
|
||||
it('accepts a hint combined with a join modifier, in either order', () => {
|
||||
expect(
|
||||
scan(
|
||||
`supabase.from('journal_entries').select('id, fiscal_periods!journal_entries_fiscal_period_id_fkey!inner(is_closed)')`,
|
||||
),
|
||||
).toEqual([])
|
||||
expect(
|
||||
scan(
|
||||
`supabase.from('journal_entries').select('id, fiscal_periods!inner!journal_entries_fiscal_period_id_fkey(is_closed)')`,
|
||||
),
|
||||
).toEqual([])
|
||||
})
|
||||
|
||||
it('does NOT accept a bare join modifier: !inner disambiguates nothing', () => {
|
||||
expect(
|
||||
scan(`supabase.from('journal_entries').select('id, fiscal_periods!inner(is_closed)')`),
|
||||
).toEqual(['journal_entries->fiscal_periods'])
|
||||
expect(
|
||||
scan(`supabase.from('journal_entries').select('id, fiscal_periods!left(is_closed)')`),
|
||||
).toEqual(['journal_entries->fiscal_periods'])
|
||||
})
|
||||
})
|
||||
|
||||
describe('ambiguous-embed: chain resolution', () => {
|
||||
it('resolves .from() through its OWN chain, not the nearest one above', () => {
|
||||
// scripts/seed-demo-account.ts: a journal_entries!inner embed from
|
||||
// journal_entry_lines (a single-foreign-key pair) sits below an unrelated
|
||||
// .from('journal_entries'). Pairing by proximity flags it wrongly.
|
||||
const source = `
|
||||
await supabase.from('journal_entries').select('id, voucher_number')
|
||||
await supabase
|
||||
.from('journal_entry_lines')
|
||||
.select('account_number, debit, journal_entries!inner(entry_date)')
|
||||
.eq('company_id', companyId)
|
||||
`
|
||||
expect(scan(source)).toEqual([])
|
||||
})
|
||||
|
||||
it('reads a select anywhere in the chain, before or after the filters', () => {
|
||||
expect(
|
||||
scan(
|
||||
`supabase.from('journal_entries').select('id, fiscal_periods(is_closed)').eq('company_id', id).in('id', ids)`,
|
||||
),
|
||||
).toEqual(['journal_entries->fiscal_periods'])
|
||||
})
|
||||
|
||||
it('ignores a select whose from-table cannot be resolved statically', () => {
|
||||
expect(scan(`query.select('id, fiscal_periods(is_closed)')`)).toEqual([])
|
||||
expect(scan(`supabase.from(tableName).select('id, fiscal_periods(is_closed)')`)).toEqual([])
|
||||
})
|
||||
})
|
||||
|
||||
describe('ambiguous-embed: nesting', () => {
|
||||
it('resolves a nested embed against its enclosing embed, not the root', () => {
|
||||
// transactions -> journal_entries is a single-foreign-key pair here, so the
|
||||
// inner fiscal_periods embed is what must be judged, against journal_entries.
|
||||
const nested = `supabase.from('transactions').select('id, journal_entries(id, fiscal_periods(is_closed))')`
|
||||
expect(scan(nested)).toEqual(['journal_entries->fiscal_periods'])
|
||||
})
|
||||
|
||||
it('leaves the enclosing embed alone when the inner one is hinted', () => {
|
||||
expect(
|
||||
scan(
|
||||
`supabase.from('transactions').select('id, journal_entries(id, fiscal_periods!journal_entries_fiscal_period_id_fkey(is_closed))')`,
|
||||
),
|
||||
).toEqual([])
|
||||
})
|
||||
|
||||
it('parses plain columns, casts and json paths without inventing embeds', () => {
|
||||
expect(parseEmbeds('id, amount::text, metadata->>ref, *', 'journal_entries')).toEqual([])
|
||||
})
|
||||
})
|
||||
|
||||
describe('ambiguous-embed: pair derivation from the migration history', () => {
|
||||
const migrationsRoot = (files: Record<string, string>) => {
|
||||
const root = tempRoot('ambiguous-embed-sql-')
|
||||
for (const [name, sql] of Object.entries(files)) write(root, name, sql)
|
||||
return root
|
||||
}
|
||||
|
||||
it('counts a pair as ambiguous once a second foreign key joins it', () => {
|
||||
const dir = migrationsRoot({
|
||||
'20240101000001_core.sql': `
|
||||
CREATE TABLE public.journal_entries (
|
||||
id UUID PRIMARY KEY,
|
||||
fiscal_period_id UUID REFERENCES public.fiscal_periods(id)
|
||||
);
|
||||
`,
|
||||
})
|
||||
expect(deriveAmbiguousPairs(dir).size).toBe(0)
|
||||
|
||||
const dir2 = migrationsRoot({
|
||||
'20240101000001_core.sql': `
|
||||
CREATE TABLE public.journal_entries (
|
||||
id UUID PRIMARY KEY,
|
||||
fiscal_period_id UUID REFERENCES public.fiscal_periods(id)
|
||||
);
|
||||
`,
|
||||
'20240101000019_period_closing.sql': `
|
||||
ALTER TABLE public.fiscal_periods
|
||||
ADD COLUMN closing_entry_id UUID REFERENCES public.journal_entries(id);
|
||||
`,
|
||||
})
|
||||
expect([...deriveAmbiguousPairs(dir2)]).toEqual([pairKey('fiscal_periods', 'journal_entries')])
|
||||
})
|
||||
|
||||
it('collapses the idempotent re-adds the migration history is full of', () => {
|
||||
const dir = migrationsRoot({
|
||||
'20240101000001_a.sql': `
|
||||
ALTER TABLE public.journal_entry_lines
|
||||
ADD CONSTRAINT journal_entry_lines_cost_center_id_fkey
|
||||
FOREIGN KEY (cost_center_id) REFERENCES public.cost_centers(id);
|
||||
`,
|
||||
'20240101000002_b.sql': `
|
||||
ALTER TABLE public.journal_entry_lines
|
||||
ADD CONSTRAINT journal_entry_lines_cost_center_id_fkey
|
||||
FOREIGN KEY (cost_center_id) REFERENCES public.cost_centers(id);
|
||||
`,
|
||||
})
|
||||
expect(deriveAmbiguousPairs(dir).size).toBe(0)
|
||||
})
|
||||
|
||||
it('honours DROP COLUMN, DROP CONSTRAINT and DROP TABLE', () => {
|
||||
const base = `
|
||||
CREATE TABLE public.a (
|
||||
id UUID PRIMARY KEY,
|
||||
b_one_id UUID REFERENCES public.b(id),
|
||||
b_two_id UUID REFERENCES public.b(id)
|
||||
);
|
||||
`
|
||||
expect([...deriveAmbiguousPairs(migrationsRoot({ '1_a.sql': base }))]).toEqual([pairKey('a', 'b')])
|
||||
|
||||
expect(
|
||||
deriveAmbiguousPairs(
|
||||
migrationsRoot({ '1_a.sql': base, '2_b.sql': `ALTER TABLE public.a DROP COLUMN b_two_id;` }),
|
||||
).size,
|
||||
).toBe(0)
|
||||
|
||||
expect(
|
||||
deriveAmbiguousPairs(
|
||||
migrationsRoot({
|
||||
'1_a.sql': base,
|
||||
'2_b.sql': `ALTER TABLE public.a DROP CONSTRAINT IF EXISTS a_b_two_id_fkey;`,
|
||||
}),
|
||||
).size,
|
||||
).toBe(0)
|
||||
|
||||
expect(
|
||||
deriveAmbiguousPairs(
|
||||
migrationsRoot({ '1_a.sql': base, '2_b.sql': `DROP TABLE IF EXISTS public.b;` }),
|
||||
).size,
|
||||
).toBe(0)
|
||||
})
|
||||
|
||||
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.
|
||||
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)
|
||||
// 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)
|
||||
expect(pairs.has(pairKey('tax_assessment_notices', 'fiscal_periods'))).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
describe('ambiguous-embed: file scan', () => {
|
||||
it('reports offenders relative to the root and skips test files', () => {
|
||||
const root = tempRoot('ambiguous-embed-')
|
||||
write(
|
||||
root,
|
||||
'supabase/migrations/20240101000019_period_closing.sql',
|
||||
`
|
||||
CREATE TABLE public.journal_entries (
|
||||
id UUID PRIMARY KEY,
|
||||
fiscal_period_id UUID REFERENCES public.fiscal_periods(id)
|
||||
);
|
||||
ALTER TABLE public.fiscal_periods
|
||||
ADD COLUMN closing_entry_id UUID REFERENCES public.journal_entries(id);
|
||||
`,
|
||||
)
|
||||
const bad = `supabase.from('journal_entries').select('id, fiscal_period:fiscal_periods(is_closed)')`
|
||||
const good = `supabase.from('journal_entries').select('id, fiscal_periods!journal_entries_fiscal_period_id_fkey(is_closed)')`
|
||||
write(root, 'lib/bad.ts', bad)
|
||||
write(root, 'lib/good.ts', good)
|
||||
write(root, 'lib/__tests__/bad.test.ts', bad)
|
||||
write(root, 'lib/bad.test.ts', bad)
|
||||
write(root, 'app/api/x/route.ts', `${good}\n${bad}`)
|
||||
|
||||
expect(findAmbiguousEmbeds(root)).toEqual([
|
||||
{ where: 'app/api/x/route.ts:2', from: 'journal_entries', target: 'fiscal_periods' },
|
||||
{ where: 'lib/bad.ts:1', from: 'journal_entries', target: 'fiscal_periods' },
|
||||
])
|
||||
})
|
||||
|
||||
it('finds nothing when no pair is ambiguous, whatever the code embeds', () => {
|
||||
const root = tempRoot('ambiguous-embed-single-')
|
||||
write(
|
||||
root,
|
||||
'supabase/migrations/1_core.sql',
|
||||
`CREATE TABLE public.journal_entries (id UUID PRIMARY KEY, fiscal_period_id UUID REFERENCES public.fiscal_periods(id));`,
|
||||
)
|
||||
write(
|
||||
root,
|
||||
'lib/fine.ts',
|
||||
`supabase.from('journal_entries').select('id, fiscal_period:fiscal_periods(is_closed)')`,
|
||||
)
|
||||
expect(findAmbiguousEmbeds(root)).toEqual([])
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,327 @@
|
||||
#!/usr/bin/env node
|
||||
/**
|
||||
* Guard: an un-hinted PostgREST embed between two tables that share more than
|
||||
* one foreign key.
|
||||
*
|
||||
* PostgREST resolves `.from('a').select('b(...)')` by looking for THE foreign
|
||||
* key between a and b, in either direction. When several exist it refuses the
|
||||
* request with PGRST201 ("Could not embed because more than one relationship
|
||||
* was found") instead of picking one. The embed must then name the
|
||||
* relationship, and PostgREST accepts two spellings for that:
|
||||
*
|
||||
* b!a_b_id_fkey(...) the constraint name
|
||||
* b!b_column_id(...) the foreign key COLUMN name
|
||||
*
|
||||
* Both are in use here (`fiscal_periods!journal_entries_fiscal_period_id_fkey`
|
||||
* in lib/transactions/inbox-underlag.ts, `journal_entries!opening_balance_entry_id`
|
||||
* in lib/import/opening-balance/cascade.ts), so either counts as hinted.
|
||||
* `!inner` / `!left` are join modifiers, not hints, and do not disambiguate.
|
||||
*
|
||||
* Why a static guard is the only thing that can catch this: the failure is a
|
||||
* runtime PostgREST decision. Unit tests mock the Supabase client and hand back
|
||||
* an already-joined row shape, so they never resolve a relationship; pg-real
|
||||
* tests talk to Postgres directly and never go through PostgREST at all. The
|
||||
* class has now been reasoned about three times and shipped broken twice, both
|
||||
* on journal_entries -> fiscal_periods, ambiguous since migration
|
||||
* 20240101000019 added fiscal_periods.closing_entry_id and
|
||||
* fiscal_periods.opening_balance_entry_id:
|
||||
* - lib/transactions/inbox-underlag.ts (fixed 2026-08-31, PR #2022): the
|
||||
* nightly underlag cron read an empty locked-set and then tried to link
|
||||
* documents into locked periods, ~60 trigger rejections a night.
|
||||
* - lib/core/documents/supplier-invoice-underlag.ts (fixed 2026-09-01): the
|
||||
* error was dropped on the floor, so supplier-invoice underlag never once
|
||||
* anchored in production and users kept seeing "Underlag saknas" on a
|
||||
* verifikat that plainly showed the invoice PDF.
|
||||
*
|
||||
* The ambiguous pairs are DERIVED from supabase/migrations/*.sql rather than
|
||||
* hardcoded, so a migration that adds a second foreign key between two tables
|
||||
* arms the guard for that pair on the same commit. To check the derivation
|
||||
* against the live schema:
|
||||
*
|
||||
* select least(s.relname, t.relname) a, greatest(s.relname, t.relname) b, count(*)
|
||||
* from pg_constraint c
|
||||
* join pg_class s on s.oid = c.conrelid
|
||||
* join pg_class t on t.oid = c.confrelid
|
||||
* join pg_namespace ns on ns.oid = s.relnamespace and ns.nspname = 'public'
|
||||
* join pg_namespace nt on nt.oid = t.relnamespace and nt.nspname = 'public'
|
||||
* 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.
|
||||
*
|
||||
* No baseline: the count is 0, any new un-hinted ambiguous embed is a hard
|
||||
* failure.
|
||||
*/
|
||||
|
||||
import fs from 'node:fs'
|
||||
import path from 'node:path'
|
||||
import ts from 'typescript'
|
||||
|
||||
const SCAN_DIRS = ['app', 'components', 'contexts', 'extensions', 'lib', 'scripts']
|
||||
const IGNORE_DIRS = new Set([
|
||||
'node_modules',
|
||||
'.next',
|
||||
'.git',
|
||||
'dist',
|
||||
'build',
|
||||
'coverage',
|
||||
'__tests__',
|
||||
])
|
||||
|
||||
/** Undirected pair key: PostgREST considers foreign keys in both directions. */
|
||||
export const pairKey = (a, b) => (a < b ? `${a}|${b}` : `${b}|${a}`)
|
||||
|
||||
const stripSql = (sql) => sql.replace(/--[^\n]*/g, '').replace(/\/\*[\s\S]*?\*\//g, '')
|
||||
const normIdent = (t) => t.replace(/"/g, '').replace(/^public\./i, '').toLowerCase()
|
||||
|
||||
/**
|
||||
* Foreign keys declared across the migration history, as a map of
|
||||
* `<table>.<column>` -> target table. Keying on the column (rather than
|
||||
* counting statements) makes the repeated idempotent re-adds this repo is full
|
||||
* of collapse onto one edge, which is what Postgres ends up with.
|
||||
*
|
||||
* Only the shapes that actually appear in supabase/migrations are parsed:
|
||||
* inline `REFERENCES` in a CREATE TABLE column, table-level
|
||||
* `FOREIGN KEY (col) REFERENCES`, `ADD COLUMN ... REFERENCES`, and
|
||||
* `ADD CONSTRAINT ... FOREIGN KEY (col) REFERENCES`. Drops are honoured so a
|
||||
* relationship that was removed again does not keep a pair armed forever.
|
||||
*/
|
||||
export function deriveForeignKeys(migrationsDir) {
|
||||
const edges = new Map()
|
||||
// Constraint name -> edge key, so DROP CONSTRAINT can find what it removes.
|
||||
// Unnamed foreign keys get Postgres's default `<table>_<column>_fkey`.
|
||||
const byConstraint = new Map()
|
||||
|
||||
const addEdge = (table, column, target, constraintName) => {
|
||||
const key = `${table}.${column}`
|
||||
edges.set(key, target)
|
||||
byConstraint.set(constraintName ?? `${table}_${column}_fkey`, key)
|
||||
}
|
||||
|
||||
let files
|
||||
try {
|
||||
files = fs.readdirSync(migrationsDir).filter((f) => f.endsWith('.sql')).sort()
|
||||
} catch {
|
||||
return edges
|
||||
}
|
||||
|
||||
for (const file of files) {
|
||||
const sql = stripSql(fs.readFileSync(path.join(migrationsDir, file), 'utf8'))
|
||||
for (const raw of sql.split(';')) {
|
||||
const stmt = raw.trim()
|
||||
if (!stmt) continue
|
||||
|
||||
const created = stmt.match(/create\s+table\s+(?:if\s+not\s+exists\s+)?([\w".]+)\s*\(/i)
|
||||
const altered = stmt.match(/alter\s+table\s+(?:if\s+exists\s+)?(?:only\s+)?([\w".]+)/i)
|
||||
const dropped = stmt.match(/drop\s+table\s+(?:if\s+exists\s+)?([\w".]+)/i)
|
||||
|
||||
if (created) {
|
||||
const table = normIdent(created[1])
|
||||
// `col uuid references public.other(id)` inside the column list.
|
||||
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.
|
||||
for (const m of stmt.matchAll(
|
||||
/foreign\s+key\s*\(\s*([\w"]+)\s*\)\s*references\s+([\w".]+)/gi,
|
||||
)) {
|
||||
addEdge(table, normIdent(m[1]), normIdent(m[2]))
|
||||
}
|
||||
continue
|
||||
}
|
||||
|
||||
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,
|
||||
)) {
|
||||
addEdge(table, normIdent(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,
|
||||
)) {
|
||||
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])}`)
|
||||
}
|
||||
for (const m of stmt.matchAll(/drop\s+constraint\s+(?:if\s+exists\s+)?([\w"]+)/gi)) {
|
||||
const edge = byConstraint.get(normIdent(m[1]))
|
||||
if (edge) edges.delete(edge)
|
||||
}
|
||||
continue
|
||||
}
|
||||
|
||||
if (dropped) {
|
||||
const table = normIdent(dropped[1])
|
||||
for (const [key, target] of [...edges]) {
|
||||
if (key.startsWith(`${table}.`) || target === table) edges.delete(key)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return edges
|
||||
}
|
||||
|
||||
/** Table pairs joined by more than one foreign key, as `pairKey` strings. */
|
||||
export function deriveAmbiguousPairs(migrationsDir) {
|
||||
const counts = new Map()
|
||||
for (const [key, target] of deriveForeignKeys(migrationsDir)) {
|
||||
const source = key.slice(0, key.indexOf('.'))
|
||||
// auth.users and other non-public tables are never embedded through PostgREST here.
|
||||
if (source.includes('.') || target.includes('.')) continue
|
||||
const pair = pairKey(source, target)
|
||||
counts.set(pair, (counts.get(pair) ?? 0) + 1)
|
||||
}
|
||||
return new Set([...counts].filter(([, n]) => n > 1).map(([pair]) => pair))
|
||||
}
|
||||
|
||||
const HEAD_CHARS = /[\w!]/
|
||||
// An embed head is preceded by the start of the select string, a separator, or
|
||||
// the `:` of an alias. `fiscal_period:fiscal_periods(...)` is the shape both
|
||||
// production bugs were written in, so `:` MUST be a delimiter here.
|
||||
const HEAD_DELIMITERS = new Set([',', ':', '(', ' ', '\t', '\n', '\r'])
|
||||
const JOIN_MODIFIERS = new Set(['inner', 'left'])
|
||||
|
||||
/**
|
||||
* The embed head immediately before `index` (which points at its `(`), or null
|
||||
* when the paren does not open an embed.
|
||||
*/
|
||||
function headBefore(select, index) {
|
||||
let end = index
|
||||
while (end > 0 && /\s/.test(select[end - 1])) end--
|
||||
let start = end
|
||||
while (start > 0 && HEAD_CHARS.test(select[start - 1])) start--
|
||||
if (start === end) return null
|
||||
if (start > 0 && !HEAD_DELIMITERS.has(select[start - 1])) return null
|
||||
|
||||
const [target, ...modifiers] = select.slice(start, end).split('!')
|
||||
if (!target) return null
|
||||
return { target: target.toLowerCase(), hinted: modifiers.some((m) => !JOIN_MODIFIERS.has(m)) }
|
||||
}
|
||||
|
||||
/**
|
||||
* Every embed in one select string, paired with the table it is embedded FROM.
|
||||
* Nested embeds resolve against their enclosing embed, not against the root.
|
||||
*/
|
||||
export function parseEmbeds(select, rootTable) {
|
||||
const embeds = []
|
||||
const stack = [rootTable]
|
||||
for (let i = 0; i < select.length; i++) {
|
||||
if (select[i] === '(') {
|
||||
const head = headBefore(select, i)
|
||||
const from = stack[stack.length - 1]
|
||||
if (head && from) embeds.push({ from, target: head.target, hinted: head.hinted })
|
||||
stack.push(head ? head.target : null)
|
||||
} else if (select[i] === ')') {
|
||||
if (stack.length > 1) stack.pop()
|
||||
}
|
||||
}
|
||||
return embeds
|
||||
}
|
||||
|
||||
/**
|
||||
* The table a `.select()` call reads from, resolved by walking ITS OWN method
|
||||
* chain back to `.from()`. Never the nearest preceding `.from()` in the file:
|
||||
* `.from('journal_entry_lines').select('... journal_entries!inner(...)')` sits
|
||||
* a few lines below an unrelated `.from('journal_entries')` in
|
||||
* scripts/seed-demo-account.ts, and pairing by proximity flags it wrongly.
|
||||
*/
|
||||
function fromTableOfChain(selectCall) {
|
||||
let node = selectCall.expression.expression
|
||||
while (node) {
|
||||
if (ts.isCallExpression(node)) {
|
||||
const callee = node.expression
|
||||
if (ts.isPropertyAccessExpression(callee) && callee.name.text === 'from') {
|
||||
const arg = node.arguments[0]
|
||||
return arg && ts.isStringLiteralLike(arg) ? arg.text.toLowerCase() : null
|
||||
}
|
||||
node = ts.isPropertyAccessExpression(callee) ? callee.expression : callee
|
||||
continue
|
||||
}
|
||||
if (ts.isPropertyAccessExpression(node)) {
|
||||
node = node.expression
|
||||
continue
|
||||
}
|
||||
return null
|
||||
}
|
||||
return null
|
||||
}
|
||||
|
||||
/**
|
||||
* Un-hinted embeds of an ambiguous pair in one file's source, as
|
||||
* `{ line, from, target }`. Exported for the unit test.
|
||||
*/
|
||||
export function findAmbiguousEmbedsInSource(sourceText, fileName, ambiguousPairs) {
|
||||
const source = ts.createSourceFile(
|
||||
fileName,
|
||||
sourceText,
|
||||
ts.ScriptTarget.Latest,
|
||||
true,
|
||||
fileName.endsWith('.tsx') ? ts.ScriptKind.TSX : ts.ScriptKind.TS,
|
||||
)
|
||||
const findings = []
|
||||
|
||||
const visit = (node) => {
|
||||
if (
|
||||
ts.isCallExpression(node) &&
|
||||
ts.isPropertyAccessExpression(node.expression) &&
|
||||
node.expression.name.text === 'select' &&
|
||||
node.arguments.length > 0 &&
|
||||
ts.isStringLiteralLike(node.arguments[0])
|
||||
) {
|
||||
const from = fromTableOfChain(node)
|
||||
if (from) {
|
||||
for (const embed of parseEmbeds(node.arguments[0].text, from)) {
|
||||
if (embed.hinted) continue
|
||||
if (!ambiguousPairs.has(pairKey(embed.from, embed.target))) continue
|
||||
const pos = source.getLineAndCharacterOfPosition(node.getStart(source))
|
||||
findings.push({ line: pos.line + 1, from: embed.from, target: embed.target })
|
||||
}
|
||||
}
|
||||
}
|
||||
ts.forEachChild(node, visit)
|
||||
}
|
||||
visit(source)
|
||||
return findings
|
||||
}
|
||||
|
||||
function walk(dir, out) {
|
||||
let entries
|
||||
try {
|
||||
entries = fs.readdirSync(dir, { withFileTypes: true })
|
||||
} catch {
|
||||
return out
|
||||
}
|
||||
for (const entry of entries) {
|
||||
if (entry.name.startsWith('.') || IGNORE_DIRS.has(entry.name)) continue
|
||||
const full = path.join(dir, entry.name)
|
||||
if (entry.isDirectory()) walk(full, out)
|
||||
else if (/\.tsx?$/.test(entry.name) && !/\.test\.tsx?$/.test(entry.name)) out.push(full)
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
/** Findings across the repo, as `{ where, from, target }`, sorted. */
|
||||
export function findAmbiguousEmbeds(root) {
|
||||
const ambiguousPairs = deriveAmbiguousPairs(path.join(root, 'supabase', 'migrations'))
|
||||
if (ambiguousPairs.size === 0) return []
|
||||
|
||||
const findings = []
|
||||
for (const dir of SCAN_DIRS) {
|
||||
for (const file of walk(path.join(root, dir), [])) {
|
||||
const sourceText = fs.readFileSync(file, 'utf8')
|
||||
if (!sourceText.includes('.select(')) continue
|
||||
const relPath = path.relative(root, file).split(path.sep).join('/')
|
||||
for (const finding of findAmbiguousEmbedsInSource(sourceText, file, ambiguousPairs)) {
|
||||
findings.push({
|
||||
where: `${relPath}:${finding.line}`,
|
||||
from: finding.from,
|
||||
target: finding.target,
|
||||
})
|
||||
}
|
||||
}
|
||||
}
|
||||
return findings.sort((a, b) => a.where.localeCompare(b.where))
|
||||
}
|
||||
@@ -102,6 +102,19 @@
|
||||
* horizontal scrollbar instead of repositioning (AccountCombobox's
|
||||
* dropdown pre-2026-08-19). Tracked as a per-file baseline set that may
|
||||
* only shrink.
|
||||
* 12. ambiguous-embed: a PostgREST `.select()` that embeds a table joined to
|
||||
* the from-table by more than one foreign key, without naming the
|
||||
* relationship. PostgREST answers PGRST201 instead of picking one, and
|
||||
* neither a mocked-Supabase unit test (a mock never resolves a
|
||||
* relationship) nor a pg-real test (it bypasses PostgREST) can see that,
|
||||
* so a static guard is the only thing that catches the class. Two sites
|
||||
* shipped the same journal_entries -> fiscal_periods embed: the nightly
|
||||
* underlag cron (fixed 2026-08-31, ~60 period-lock trigger rejections a
|
||||
* night) and supplier-invoice underlag anchoring, which swallowed the
|
||||
* error and therefore never anchored a single document in production.
|
||||
* The ambiguous pairs are derived from supabase/migrations; both hint
|
||||
* forms PostgREST accepts count as disambiguated. Implementation and
|
||||
* rationale in ambiguous-embed.mjs. No baseline: the count is 0 today.
|
||||
*
|
||||
* Usage:
|
||||
* node scripts/checks/no-new-antipatterns.mjs # check (CI)
|
||||
@@ -123,6 +136,7 @@ import ts from 'typescript'
|
||||
import { findSekLabelledFxAmounts } from './format-currency-sek-label.mjs'
|
||||
import { findRawReferenceFetches } from './raw-reference-fetch.mjs'
|
||||
import { findClientNodeBuiltins } from './client-node-builtin.mjs'
|
||||
import { findAmbiguousEmbeds } from './ambiguous-embed.mjs'
|
||||
import {
|
||||
findExtensionRouteFindings,
|
||||
UNGATED_EXTENSION_ROUTES,
|
||||
@@ -1027,6 +1041,7 @@ const current = {
|
||||
directAiClients: findDirectAiClients(),
|
||||
rawReferenceFetch: findRawReferenceFetches(ROOT),
|
||||
clientNodeBuiltins: findClientNodeBuiltins(ROOT),
|
||||
ambiguousEmbeds: findAmbiguousEmbeds(ROOT),
|
||||
}
|
||||
|
||||
const dialogOverflowFiles = [...new Set(current.dialogOverflowRisk.map((f) => f.file))].sort()
|
||||
@@ -1118,6 +1133,28 @@ if (current.clientNodeBuiltins.length) {
|
||||
)
|
||||
}
|
||||
|
||||
// 1b4. ambiguous-embed: an embed between two tables joined by more than one
|
||||
// foreign key must name the relationship, or PostgREST answers PGRST201 at
|
||||
// runtime. No baseline: the count is 0 today, any new one is a hard failure.
|
||||
if (current.ambiguousEmbeds.length) {
|
||||
failed = true
|
||||
console.error(
|
||||
`\n✗ ambiguous-embed: ${current.ambiguousEmbeds.length} PostgREST embed(s) between a table pair ` +
|
||||
`that shares more than one foreign key, with no relationship named:`,
|
||||
)
|
||||
current.ambiguousEmbeds.forEach((f) =>
|
||||
console.error(` ${f.where} ${f.from} -> ${f.target}`),
|
||||
)
|
||||
console.error(
|
||||
' → name the relationship in the embed, either by constraint\n' +
|
||||
" (.select('fiscal_period:fiscal_periods!journal_entries_fiscal_period_id_fkey(...)'))\n" +
|
||||
" or by foreign key column (.select('journal_entries!opening_balance_entry_id(...)')).\n" +
|
||||
' Without it PostgREST returns PGRST201 for every call, and no mocked-Supabase test\n' +
|
||||
' or pg-real test can see it: a mock never resolves a relationship and pg-real does\n' +
|
||||
' not go through PostgREST at all.',
|
||||
)
|
||||
}
|
||||
|
||||
// 1b2. leaky-supabase-client: server code must construct clients through
|
||||
// createServiceRoleClient(). No baseline: the count is 0 today.
|
||||
if (current.leakySupabaseClients.length) {
|
||||
@@ -1410,5 +1447,5 @@ if (failed) {
|
||||
process.exit(1)
|
||||
}
|
||||
console.log(
|
||||
`\n✓ Antipattern guard passed (raw-route-auth: ${current.rawRouteAuth.length}, naive-ore-round: ${current.naiveOreRound}, hand-rolled-invariant: ${current.handRolledInvariants}, ledger-scanning-report: ${current.ledgerScanningReports.length}, direct-jel-insert: 0, leaky-supabase-client: 0, pinned-dep: 0, raw-user-error: 0, sek-labelled-amount: 0, off-ladder-radius: 0, folded-public-flag: 0, cross-extension-import: 0, ungated-extension-route: ${current.extensionRoutes.ungated.length}/${UNGATED_EXTENSION_ROUTES.size} allowlisted, dialog-overflow-risk: ${dialogOverflowFiles.length} file(s), raw-reference-fetch: ${current.rawReferenceFetch.length} file(s), client-node-builtin: ${current.clientNodeBuiltins.length}, direct-ai-client: ${current.directAiClients.length}/${DIRECT_AI_CLIENT_ALLOWED.size} allowlisted).`,
|
||||
`\n✓ Antipattern guard passed (raw-route-auth: ${current.rawRouteAuth.length}, naive-ore-round: ${current.naiveOreRound}, hand-rolled-invariant: ${current.handRolledInvariants}, ledger-scanning-report: ${current.ledgerScanningReports.length}, direct-jel-insert: 0, leaky-supabase-client: 0, pinned-dep: 0, raw-user-error: 0, sek-labelled-amount: 0, off-ladder-radius: 0, folded-public-flag: 0, cross-extension-import: 0, ungated-extension-route: ${current.extensionRoutes.ungated.length}/${UNGATED_EXTENSION_ROUTES.size} allowlisted, dialog-overflow-risk: ${dialogOverflowFiles.length} file(s), raw-reference-fetch: ${current.rawReferenceFetch.length} file(s), client-node-builtin: ${current.clientNodeBuiltins.length}, ambiguous-embed: ${current.ambiguousEmbeds.length}, direct-ai-client: ${current.directAiClients.length}/${DIRECT_AI_CLIENT_ALLOWED.size} allowlisted).`,
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user