From b516c7054811714ca51dc50496cf78aafb96a9b3 Mon Sep 17 00:00:00 2001 From: Jasdeep Date: Wed, 7 Oct 2026 21:14:16 +0000 Subject: [PATCH 01/15] fix(roles): a new table keeps the source's grants, not the target's defaults A table the source keeps away from the Data API (REVOKE ALL ... FROM anon, authenticated) and created on a Supabase target by the schema fix got everything the target's default privileges grant. Nothing took that back until a second sync, so one sync left it open to the anon key. The roles check now works out what the target's defaults will grant on each table it lacks, and revokes whatever the source does not grant. The revoke runs with the grants, after the table is created. An extra grant to anon, authenticated or PUBLIC is critical, and one to any other role a warning; both were info. Role, schema and table names in the generated SQL are now quoted with their own quotes doubled. Co-Authored-By: Claude Opus 5.5 --- packages/cli/src/checks/roles.ts | 104 +++++++++++++++--- packages/cli/test/checks/roles.test.ts | 78 ++++++++++++- .../cli/test/e2e/grants-on-new-tables.test.ts | 77 +++++++++++++ 3 files changed, 236 insertions(+), 23 deletions(-) create mode 100644 packages/cli/test/e2e/grants-on-new-tables.test.ts diff --git a/packages/cli/src/checks/roles.ts b/packages/cli/src/checks/roles.ts index c134e27..8037e43 100644 --- a/packages/cli/src/checks/roles.ts +++ b/packages/cli/src/checks/roles.ts @@ -3,6 +3,7 @@ import { pgQuery } from '../db.js' import type { DriftIssue } from '../types/drift.js' import { Check, type CheckContext } from './base.js' import { SUPABASE_PLATFORM_SCHEMAS } from '../defaults.js' +import { quoteName } from '../utils/sql.js' interface PgRole { rolname: string @@ -147,11 +148,12 @@ const columnGrantsSql = (ignoreSchemas: string[]) => ` ORDER BY 1, 2, 3, 4, 5 ` -/** The target's relations, to tell a grant on a table it lacks yet. */ -const RELATIONS_SQL = ` +/** Each side's relations, to tell a table the target lacks yet. */ +const relationsSql = (ignoreSchemas: string[]) => ` SELECT n.nspname AS table_schema, c.relname AS table_name FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace WHERE c.relkind IN ('r', 'p', 'v', 'm', 'f') + AND n.nspname ${grantSchemaFilter(ignoreSchemas)} ` /** @@ -166,8 +168,12 @@ const DEFAULT_TABLE_GRANTS_SQL = ` LEFT JOIN pg_namespace n ON n.oid = d.defaclnamespace CROSS JOIN LATERAL aclexplode(d.defaclacl) a WHERE d.defaclobjtype = 'r' AND d.defaclrole = (SELECT oid FROM pg_roles WHERE rolname = current_user) + AND a.grantee <> d.defaclrole ` +type Relation = { table_schema: string; table_name: string } +type DefaultGrant = { table_schema: string | null; grantee: string; privilege_type: string; is_grantable: boolean } + /** * Leave out a missing grant the target will make by itself: one on a table it * does not have yet, which its default privileges grant when the schema fix @@ -175,8 +181,8 @@ const DEFAULT_TABLE_GRANTS_SQL = ` * most of a report, and gone again after the first apply. */ export function grantedByDefault( - relations: Array<{ table_schema: string; table_name: string }>, - defaults: Array<{ table_schema: string | null; grantee: string; privilege_type: string; is_grantable: boolean }>, + relations: Relation[], + defaults: DefaultGrant[], ): (g: RoleGrant) => boolean { const present = new Set(relations.map(r => `${r.table_schema}.${r.table_name}`)) const granted = new Set(defaults.map(d => `${d.table_schema ?? '*'}|${d.grantee}|${d.privilege_type}|${Boolean(d.is_grantable)}`)) @@ -185,6 +191,67 @@ export function grantedByDefault( && [g.table_schema, '*'].some(schema => granted.has(`${schema}|${g.grantee}|${g.privilege_type}|${Boolean(g.is_grantable)}`)) } +/** + * What the target's default privileges will grant on a table the schema fix + * creates, and the source does not: the other half of `grantedByDefault`. + * + * A table kept away from the Data API (`REVOKE ALL ... FROM anon, + * authenticated`) and created on a Supabase target got everything the + * defaults give, and nothing took it back until a second sync, so one sync + * left it open to the anon key. The REVOKE runs with the grants, after the + * table is created. + */ +export function defaultGrantsToRevoke( + sourceRelations: Relation[], + targetRelations: Relation[], + defaults: DefaultGrant[], + sourceGrants: RoleGrant[], +): DriftIssue[] { + const present = new Set(targetRelations.map(r => `${r.table_schema}.${r.table_name}`)) + const held = new Set(sourceGrants.filter(g => !g.column_name) + .map(g => `${g.table_schema}.${g.table_name}|${g.grantee}|${g.privilege_type}`)) + const issues: DriftIssue[] = [] + for (const rel of sourceRelations) { + const name = `${rel.table_schema}.${rel.table_name}` + if (present.has(name)) continue + const extra = new Map>() + for (const d of defaults) { + if (d.table_schema !== null && d.table_schema !== rel.table_schema) continue + if (held.has(`${name}|${d.grantee}|${d.privilege_type}`)) continue + extra.set(d.grantee, (extra.get(d.grantee) ?? new Set()).add(d.privilege_type)) + } + for (const [grantee, privileges] of [...extra].sort(([a], [b]) => a.localeCompare(b))) { + const gs = [...privileges].sort().map(privilege_type => + ({ grantee, table_schema: rel.table_schema, table_name: rel.table_name, privilege_type, is_grantable: false })) + issues.push({ + id: `roles-grant-default-${grantee}.${name}`, + check: 'roles', + severity: extraGrantSeverity(grantee), + title: `Default grants to take back: ${describeGroup(gs)}`, + description: `Creating ${name} on the target gives ${grantee} what its default privileges grant. ` + + `The source does not grant "${describeGroup(gs)}", so it is revoked once the table exists.`, + targetValue: gs, + sql: { + up: `REVOKE ${groupTarget(gs)} FROM ${granteeSql(grantee)};`, + down: `GRANT ${groupTarget(gs)} TO ${granteeSql(grantee)};`, + }, + }) + } + } + return issues +} + +/** + * A grant the target has and the source does not widens access. To the roles + * the Data API serves (and PUBLIC) that is the anon key reaching a table, so + * critical; to any other role a warning. It was info, which a sync's summary + * does not lead with. + */ +const OPEN_GRANTEES = new Set(['anon', 'authenticated', 'PUBLIC']) +function extraGrantSeverity(grantee: string): 'critical' | 'warning' { + return OPEN_GRANTEES.has(grantee) ? 'critical' : 'warning' +} + export class RolesCheck extends Check { readonly name = 'roles' as const @@ -198,7 +265,7 @@ export class RolesCheck extends Check { Number((await this.queryFn(url, `SELECT current_setting('server_version_num') AS v`) as Array<{ v: string }>)[0]?.v ?? 0) const [ sourceRoles, targetRoles, sourceGrants, targetGrants, sourceColumns, targetColumns, - sourceVersion, targetVersion, targetRelations, targetDefaults, + sourceVersion, targetVersion, sourceRelations, targetRelations, targetDefaults, ] = await Promise.all([ this.queryFn(ctx.source.dbUrl, ROLES_SQL) as unknown as Promise, this.queryFn(ctx.target.dbUrl, ROLES_SQL) as unknown as Promise, @@ -208,9 +275,9 @@ export class RolesCheck extends Check { this.queryFn(ctx.target.dbUrl, columnGrantsSql(ignore)) as unknown as Promise, version(ctx.source.dbUrl), version(ctx.target.dbUrl), - this.queryFn(ctx.target.dbUrl, RELATIONS_SQL) as unknown as Promise>, - this.queryFn(ctx.target.dbUrl, DEFAULT_TABLE_GRANTS_SQL) as unknown as - Promise>, + this.queryFn(ctx.source.dbUrl, relationsSql(ignore)) as unknown as Promise, + this.queryFn(ctx.target.dbUrl, relationsSql(ignore)) as unknown as Promise, + this.queryFn(ctx.target.dbUrl, DEFAULT_TABLE_GRANTS_SQL) as unknown as Promise, ]) const comparable = comparablePrivileges(sourceVersion, targetVersion) const byDefault = grantedByDefault(targetRelations ?? [], targetDefaults ?? []) @@ -221,6 +288,9 @@ export class RolesCheck extends Check { [...(sourceGrants ?? []), ...(sourceColumns ?? [])].filter(g => comparable(g) && !byDefault(g)), [...(targetGrants ?? []), ...(targetColumns ?? [])].filter(comparable), ), + ...defaultGrantsToRevoke( + sourceRelations ?? [], targetRelations ?? [], + (targetDefaults ?? []).filter(d => comparable(d as RoleGrant)), sourceGrants ?? []), ] } } @@ -268,8 +338,8 @@ export function diffRoles(source: PgRole[], target: PgRole[]): DriftIssue[] { description: `Role "${name}" exists in source but is missing from target. Any RLS policies or grants referencing this role will be silently ineffective.`, sourceValue: sr, sql: { - up: `CREATE ROLE "${name}" ${roleAttrs(sr)};`, - down: `DROP ROLE IF EXISTS "${name}";`, + up: `CREATE ROLE ${quoteName(name)} ${roleAttrs(sr)};`, + down: `DROP ROLE IF EXISTS ${quoteName(name)};`, }, }) } @@ -285,8 +355,8 @@ export function diffRoles(source: PgRole[], target: PgRole[]): DriftIssue[] { description: `Role "${name}" exists in target but not in source.`, targetValue: tr, sql: { - up: `DROP ROLE IF EXISTS "${name}";`, - down: `CREATE ROLE "${name}" ${roleAttrs(tr)};`, + up: `DROP ROLE IF EXISTS ${quoteName(name)};`, + down: `CREATE ROLE ${quoteName(name)} ${roleAttrs(tr)};`, }, }) } @@ -304,8 +374,8 @@ export function diffRoles(source: PgRole[], target: PgRole[]): DriftIssue[] { sourceValue: sr, targetValue: tr, sql: { - up: `ALTER ROLE "${name}" ${roleAttrs(sr)};`, - down: `ALTER ROLE "${name}" ${roleAttrs(tr)};`, + up: `ALTER ROLE ${quoteName(name)} ${roleAttrs(sr)};`, + down: `ALTER ROLE ${quoteName(name)} ${roleAttrs(tr)};`, }, }) } @@ -330,7 +400,7 @@ function granteeSql(grantee: string): string { /** `SELECT ON "s"."t"`, or `UPDATE ("col") ON "s"."t"` for a column grant. */ function grantTarget(g: RoleGrant): string { const column = g.column_name ? ` ("${g.column_name.replace(/"/g, '""')}")` : '' - return `${g.privilege_type}${column} ON "${g.table_schema}"."${g.table_name}"` + return `${g.privilege_type}${column} ON ${quoteName(g.table_schema)}.${quoteName(g.table_name)}` } function describeGrant(g: RoleGrant): string { @@ -365,7 +435,7 @@ function grouped(grants: RoleGrant[]): RoleGrant[][] { /** `SELECT, INSERT ON "s"."t"` for a group of grants on one object. */ function groupTarget(gs: RoleGrant[]): string { const column = gs[0].column_name ? ` ("${gs[0].column_name.replace(/"/g, '""')}")` : '' - return `${gs.map(g => `${g.privilege_type}${column}`).join(', ')} ON "${gs[0].table_schema}"."${gs[0].table_name}"` + return `${gs.map(g => `${g.privilege_type}${column}`).join(', ')} ON ${quoteName(gs[0].table_schema)}.${quoteName(gs[0].table_name)}` } function describeGroup(gs: RoleGrant[]): string { @@ -435,7 +505,7 @@ export function diffGrants(source: RoleGrant[], target: RoleGrant[]): DriftIssue issues.push({ id: `roles-grant-extra-${groupKey(gs)}`, check: 'roles', - severity: 'info', + severity: extraGrantSeverity(gs[0].grantee), title: `Extra grant${plural}: ${describeGroup(gs)}`, description: `Grant${plural} "${describeGroup(gs)}" exist${gs.length > 1 ? '' : 's'} in target but not in source.`, targetValue: gs.length > 1 ? gs : gs[0], diff --git a/packages/cli/test/checks/roles.test.ts b/packages/cli/test/checks/roles.test.ts index 4f393d3..7de316c 100644 --- a/packages/cli/test/checks/roles.test.ts +++ b/packages/cli/test/checks/roles.test.ts @@ -132,11 +132,11 @@ describe('RolesCheck', () => { const issues = await check.scan(mockContext()) const grantIssue = issues.find(i => i.id.startsWith('roles-grant-extra')) expect(grantIssue).toBeTruthy() - expect(grantIssue!.severity).toBe('info') + expect(grantIssue!.severity).toBe('warning') expect(grantIssue!.sql?.up).toContain('REVOKE INSERT') }) - it('runs all 10 queries (roles, grants, column grants and version per side; the target\'s tables and default grants)', async () => { + it('runs all 11 queries (roles, grants, column grants, version and tables per side; the target\'s default grants)', async () => { const calls: string[] = [] const queryFn: QueryFn = async (_dbUrl, sql) => { calls.push(sql) @@ -144,7 +144,7 @@ describe('RolesCheck', () => { } const check = new RolesCheck(queryFn) await check.scan(mockContext()) - expect(calls).toHaveLength(10) + expect(calls).toHaveLength(11) expect(calls.filter(s => s.includes('server_version_num'))).toHaveLength(2) const roleQueries = calls.filter(s => s.includes('rolsuper')) const grantQueries = calls.filter(s => s.includes('c.relacl')) @@ -218,7 +218,7 @@ describe('diffGrants', () => { const issues = diffGrants([], [grant]) expect(issues[0].sql?.up).toBe('REVOKE DELETE ON "public"."users" FROM "app_readonly";') expect(issues[0].sql?.down).toBe('GRANT DELETE ON "public"."users" TO "app_readonly";') - expect(issues[0].severity).toBe('info') + expect(issues[0].severity).toBe('warning') }) it('returns empty when grants match', () => { @@ -237,12 +237,12 @@ describe('diffGrants', () => { expect(issues[0].sql?.up).toContain('posts') }) - it('uses correct severity: warning for missing, info for extra', () => { + it('uses correct severity: warning for missing, and for extra to a role the Data API does not serve', () => { const grant = makeGrant() const missingIssues = diffGrants([grant], []) const extraIssues = diffGrants([], [grant]) expect(missingIssues[0].severity).toBe('warning') - expect(extraIssues[0].severity).toBe('info') + expect(extraIssues[0].severity).toBe('warning') }) }) @@ -388,3 +388,69 @@ describe('grants across server versions and in groups', () => { }) }) + +// A table the source keeps away from the Data API, created on a Supabase +// target by the schema fix, got everything the target's default privileges +// grant: one sync left it readable and writable with the anon key. +describe('a new table keeps the source\'s grants, not the target\'s defaults', () => { + const ALL = ['DELETE', 'INSERT', 'REFERENCES', 'SELECT', 'TRIGGER', 'TRUNCATE', 'UPDATE'] + const grant = (grantee: string, table: string, privilege_type: string) => + ({ grantee, table_schema: 'public', table_name: table, privilege_type, is_grantable: false }) + + function target(sql: string, dbUrl: string): unknown[] { + const source = dbUrl.includes('source') + if (sql.includes('pg_default_acl')) { + return source ? [] : ['anon', 'authenticated'].flatMap(grantee => + ALL.map(privilege_type => ({ table_schema: 'public', grantee, privilege_type, is_grantable: false }))) + } + if (sql.includes('attacl') || sql.includes('rolsuper')) return [] + if (sql.includes('server_version_num')) return [{ v: '150008' }] + if (sql.includes('relacl')) { + return source ? [ + ...['SELECT', 'REFERENCES', 'TRIGGER'].map(p => grant('anon', 'ro_items', p)), + ...ALL.map(p => grant('authenticated', 'ro_items', p)), + ...ALL.map(p => grant('service_role', 'private_notes', p)), + ] : [] + } + // The relations each side has. + return source + ? ['private_notes', 'ro_items', 'shared'].map(table_name => ({ table_schema: 'public', table_name })) + : [{ table_schema: 'public', table_name: 'shared' }] + } + + async function issues() { + const check = new RolesCheck((async (dbUrl: string, sql: string) => target(sql, dbUrl)) as unknown as QueryFn) + return (await check.scan(mockContext())).filter(i => i.id.startsWith('roles-grant-default-')) + } + + it('revokes what the defaults would give a server-only table', async () => { + const found = await issues() + const notes = found.filter(i => i.id.endsWith('public.private_notes')) + expect(notes.map(i => i.sql?.up).sort()).toEqual([ + 'REVOKE DELETE, INSERT, REFERENCES, SELECT, TRIGGER, TRUNCATE, UPDATE ON "public"."private_notes" FROM "anon";', + 'REVOKE DELETE, INSERT, REFERENCES, SELECT, TRIGGER, TRUNCATE, UPDATE ON "public"."private_notes" FROM "authenticated";', + ]) + expect(notes.every(i => i.severity === 'critical')).toBe(true) + }) + + it('revokes only what the source took away from a read-only table', async () => { + const ro = (await issues()).filter(i => i.id.endsWith('public.ro_items')) + expect(ro.map(i => i.sql?.up)).toEqual(['REVOKE DELETE, INSERT, TRUNCATE, UPDATE ON "public"."ro_items" FROM "anon";']) + expect(ro[0].title).toBe('Default grants to take back: DELETE, INSERT, TRUNCATE, UPDATE ON public.ro_items TO anon') + }) + + it('says nothing about a table the target already has', async () => { + expect((await issues()).some(i => i.id.endsWith('public.shared'))).toBe(false) + }) +}) + +describe('an extra grant to a Data API role is not a footnote', () => { + const g = (grantee: string) => ({ grantee, table_schema: 'public', table_name: 'notes', privilege_type: 'SELECT', is_grantable: false }) + + it('is critical for anon, authenticated and PUBLIC, a warning for any other role', () => { + for (const grantee of ['anon', 'authenticated', 'PUBLIC']) { + expect(diffGrants([], [g(grantee)])[0].severity, grantee).toBe('critical') + } + expect(diffGrants([], [g('app_readonly')])[0].severity).toBe('warning') + }) +}) diff --git a/packages/cli/test/e2e/grants-on-new-tables.test.ts b/packages/cli/test/e2e/grants-on-new-tables.test.ts new file mode 100644 index 0000000..8000061 --- /dev/null +++ b/packages/cli/test/e2e/grants-on-new-tables.test.ts @@ -0,0 +1,77 @@ +/** + * A table the source keeps away from the Data API stays that way on the target + * after one sync. + * + * Supabase's default privileges give anon and authenticated everything on a + * table the moment it is created. A server-only table — `REVOKE ALL ... FROM + * anon, authenticated`, RLS off — created on the target by the schema fix was + * therefore readable and writable with the anon key until a second sync took + * the grants back. Both servers here carry the same defaults Supabase does. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest' +import { PgHarness } from '../harness/PgHarness.js' +import { describeWithContainers } from '../harness/containers.js' + +const describeE2E = describeWithContainers() + +/** What every Supabase database starts with, on both sides. */ +const PLATFORM = ` + DO $$ BEGIN CREATE ROLE anon NOLOGIN; EXCEPTION WHEN duplicate_object THEN NULL; END $$; + DO $$ BEGIN CREATE ROLE authenticated NOLOGIN; EXCEPTION WHEN duplicate_object THEN NULL; END $$; + GRANT USAGE ON SCHEMA public TO anon, authenticated; + ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT ALL ON TABLES TO anon, authenticated; +` + +const SOURCE = ` + CREATE TABLE public.private_notes (id int PRIMARY KEY, body text); + REVOKE ALL ON public.private_notes FROM anon, authenticated; + CREATE TABLE public.ro_items (id int PRIMARY KEY); + REVOKE INSERT, UPDATE, DELETE, TRUNCATE ON public.ro_items FROM anon; + CREATE TABLE public.open_items (id int PRIMARY KEY); +` + +/** Each table's grants to the two API roles, as one comparable line per grant. */ +const GRANTS = ` + SELECT coalesce(string_agg(format('%s %s %s', c.relname, a.grantee::regrole, a.privilege_type), E'\\n' + ORDER BY c.relname, a.grantee::regrole::text, a.privilege_type), '') + FROM pg_class c CROSS JOIN LATERAL aclexplode(c.relacl) a + WHERE c.relnamespace = 'public'::regnamespace AND c.relname IN ('private_notes', 'ro_items', 'open_items') + AND a.grantee IN ('anon'::regrole, 'authenticated'::regrole) +` + +describeE2E('e2e: a new table keeps the source\'s grants', () => { + let h: PgHarness + let ws: string + + beforeAll(async () => { + h = new PgHarness({ verbose: !!process.env.E2E_VERBOSE, keep: !!process.env.E2E_KEEP }) + await h.up() + await h.applySql('source', PLATFORM + SOURCE) + await h.applySql('target', PLATFORM) + ws = await h.workspace() + }, 300_000) + + afterAll(async () => { await h?.down() }, 120_000) + + it('reports, before the sync, what the target\'s defaults would open up', async () => { + const r = await h.cli(['diff', '--check', 'roles', '--json'], { cwd: ws }) + const scan = JSON.parse(r.stdout) as { checks: Array<{ issues: Array<{ id: string; severity: string }> }> } + const issues = scan.checks.flatMap(c => c.issues) + expect(issues.map(i => i.id).sort()).toEqual([ + 'roles-grant-default-anon.public.private_notes', + 'roles-grant-default-anon.public.ro_items', + 'roles-grant-default-authenticated.public.private_notes', + ]) + expect(issues.every(i => i.severity === 'critical')).toBe(true) + }, 120_000) + + it('leaves every new table with the source\'s grants after one sync', async () => { + const sync = await h.cli(['diff', '--apply'], { cwd: ws }) + expect(sync.stdout + sync.stderr).not.toMatch(/Rolled back/) + expect(await h.sql('target', GRANTS)).toBe(await h.sql('source', GRANTS)) + + const r = await h.cli(['diff', '--check', 'roles', '--json'], { cwd: ws }) + const scan = JSON.parse(r.stdout) as { checks: Array<{ issues: unknown[] }> } + expect(scan.checks.flatMap(c => c.issues), r.stdout).toEqual([]) + }, 300_000) +}) From 00d14a306b558df3e610f478821261ef20d3bdfd Mon Sep 17 00:00:00 2001 From: Jasdeep Date: Wed, 7 Oct 2026 21:32:35 +0000 Subject: [PATCH 02/15] fix: write every name into SQL with its own quotes doubled A policy named say "hi" there was written between plain quotes, a syntax error: a restore holding one rolled back entirely, and diff --apply applied nothing from that run. Policy SQL is now built in one place for the RLS, storage and Realtime checks and the snapshot, with the policy, schema, table and every role in TO quoted. Webhook, RLS coverage, extension, publication, grant and database names are quoted the same way. A role list read as an array literal ({"Admins \"A\"",anon}) is parsed properly, so such a role is not quoted twice, and titles read a name with doubled quotes as one name. Co-Authored-By: Claude Opus 5.5 --- packages/cli/src/branch.ts | 20 ++--- packages/cli/src/checks/rls-coverage.ts | 3 +- packages/cli/src/checks/rls.ts | 23 +---- packages/cli/src/checks/webhooks.ts | 6 +- packages/cli/src/dbdiff.ts | 5 +- packages/cli/src/prove.ts | 12 +-- packages/cli/src/restore.ts | 8 +- packages/cli/src/snapshot.ts | 42 +++------ packages/cli/src/utils/policy-equivalence.ts | 4 +- packages/cli/src/utils/schema-policies.ts | 29 +++++-- packages/cli/src/utils/strings.ts | 32 ++++++- packages/cli/test/checks/rls.test.ts | 4 +- packages/cli/test/dbdiff.test.ts | 12 +++ packages/cli/test/e2e/quoted-names.test.ts | 71 ++++++++++++++++ packages/cli/test/quoted-names.test.ts | 89 ++++++++++++++++++++ packages/cli/test/utils/strings.test.ts | 16 ++++ 16 files changed, 292 insertions(+), 84 deletions(-) create mode 100644 packages/cli/test/e2e/quoted-names.test.ts create mode 100644 packages/cli/test/quoted-names.test.ts diff --git a/packages/cli/src/branch.ts b/packages/cli/src/branch.ts index d045982..f60548b 100644 --- a/packages/cli/src/branch.ts +++ b/packages/cli/src/branch.ts @@ -8,7 +8,7 @@ import type { EnvironmentConfig, SupaForgeConfig } from './types/config' import { checkPgDumpCompat } from './pg-tools' import { BRANCHES_FILE, PG_PIPELINE_TIMEOUT_MS, CLONE_PROGRESS_INTERVAL_MS } from './constants' import { CLONE_STUBS_SQL } from './stubs' -import { quoteIdent } from './utils/sql' +import { quoteIdent, quoteName } from './utils/sql' import { SUPABASE_PLATFORM_SCHEMAS } from './defaults' /** Prefix for branch database names created by SupaForge. */ @@ -188,7 +188,7 @@ async function tryTemplateCopy(sourceUrl: string, newDb: string): Promise pg_backend_pid()`, [branch.dbName], ) - await client.query(`DROP DATABASE IF EXISTS "${branch.dbName}"`) + await client.query(`DROP DATABASE IF EXISTS ${quoteName(branch.dbName)}`) } finally { await client.end() } @@ -474,10 +474,10 @@ export async function cloneRemoteToLocal(opts: CloneRemoteOptions): Promise pg_backend_pid()`, [opts.localDbName], ) - await client.query(`DROP DATABASE IF EXISTS "${opts.localDbName}"`) + await client.query(`DROP DATABASE IF EXISTS ${quoteName(opts.localDbName)}`) } - await client.query(`CREATE DATABASE "${opts.localDbName}"`) + await client.query(`CREATE DATABASE ${quoteName(opts.localDbName)}`) } catch (err) { const msg = (err as Error).message ?? '' if (msg.includes('already exists')) { @@ -619,7 +619,7 @@ export async function cloneRemoteToLocal(opts: CloneRemoteOptions): Promise createPolicySql(p.schemaname, p) +const generateDropPolicySql = (p: RlsPolicy) => dropPolicySql(p.schemaname, p) export function diffPolicies(source: RlsPolicy[], target: RlsPolicy[]): DriftIssue[] { const issues: DriftIssue[] = [] diff --git a/packages/cli/src/checks/webhooks.ts b/packages/cli/src/checks/webhooks.ts index 42e006a..7eefb00 100644 --- a/packages/cli/src/checks/webhooks.ts +++ b/packages/cli/src/checks/webhooks.ts @@ -1,5 +1,6 @@ import type { QueryFn } from '../db' import { pgQuery } from '../db' +import { quoteName } from '../utils/sql' import type { DriftIssue } from '../types/drift' import { Check, type CheckContext } from './base' @@ -25,6 +26,8 @@ import { Check, type CheckContext } from './base' interface WebhookEntry { /** `schema.table`, so two webhooks of the same name stay distinct. */ table_name: string + /** The table as SQL names it, quoted where it needs to be. */ + table_ref?: string /** The trigger name, which is what the Dashboard calls the webhook. */ name: string /** `CREATE TRIGGER ...`, exactly as the server renders it. */ @@ -103,6 +106,7 @@ export class WebhooksCheck extends Check { */ const HOOKS_SQL = ` SELECT n.nspname || '.' || c.relname AS table_name, + format('%I.%I', n.nspname, c.relname) AS table_ref, t.tgname AS name, pg_get_triggerdef(t.oid) AS definition FROM pg_trigger t @@ -122,7 +126,7 @@ const PG_NET_CHECK_SQL = ` /** `DROP TRIGGER IF EXISTS "name" ON schema.table;` */ function dropStatement(entry: WebhookEntry): string { - return `DROP TRIGGER IF EXISTS "${entry.name}" ON ${entry.table_name};` + return `DROP TRIGGER IF EXISTS ${quoteName(entry.name)} ON ${entry.table_ref ?? entry.table_name};` } /** diff --git a/packages/cli/src/dbdiff.ts b/packages/cli/src/dbdiff.ts index 87b0513..171d53c 100644 --- a/packages/cli/src/dbdiff.ts +++ b/packages/cli/src/dbdiff.ts @@ -1625,9 +1625,10 @@ export function extractQualifiedName(sql: string, head: RegExp): string { const parts: string[] = [] for (let depth = 0; depth < 2; depth++) { - const ident = /^(?:"([^"]*)"|([\w$]+))/.exec(rest) + // A quoted name doubles any quote inside it: "say ""hi"" there". + const ident = /^(?:"((?:[^"]|"")*)"|([\w$]+))/.exec(rest) if (!ident) break - parts.push(ident[1] ?? ident[2]) + parts.push(ident[1] !== undefined ? ident[1].replace(/""/g, '"') : ident[2]) rest = rest.slice(ident[0].length) const dot = /^\s*\.\s*/.exec(rest) diff --git a/packages/cli/src/prove.ts b/packages/cli/src/prove.ts index b508100..d8da29b 100644 --- a/packages/cli/src/prove.ts +++ b/packages/cli/src/prove.ts @@ -241,7 +241,7 @@ export async function proveConvergence(opts: { let roundTripName: string | undefined try { try { - await pgQuery(adminUrl, `CREATE DATABASE "${cloneName}"`) + await pgQuery(adminUrl, `CREATE DATABASE ${quoteName(cloneName)}`) created = true } catch (err) { // Typically insufficient privilege. Not being able to prove is not the @@ -442,7 +442,7 @@ async function copyOfSource( adminUrl: string, sourceUrl: string, intoUrl: string, name: string, schemas: string[], tools: PgTools, ): Promise<{ ok: boolean } | null> { try { - await pgQuery(adminUrl, `CREATE DATABASE "${name}"`) + await pgQuery(adminUrl, `CREATE DATABASE ${quoteName(name)}`) } catch { return null } @@ -459,7 +459,7 @@ async function dropDatabase(adminUrl: string, name: string): Promise { await pgQuery(adminUrl, `SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE datname = '${name}' AND pid <> pg_backend_pid()`).catch(() => undefined) - await pgQuery(adminUrl, `DROP DATABASE IF EXISTS "${name}"`).catch(() => undefined) + await pgQuery(adminUrl, `DROP DATABASE IF EXISTS ${quoteName(name)}`).catch(() => undefined) } /** @@ -582,7 +582,7 @@ export async function prepareClone( cloneUrl: string, targetUrl: string, schemas: string[], queryFn: QueryFn = pgQuery, ): Promise { for (const schema of schemas) { - await queryFn(cloneUrl, `DROP SCHEMA IF EXISTS "${schema}" CASCADE`) + await queryFn(cloneUrl, `DROP SCHEMA IF EXISTS ${quoteName(schema)} CASCADE`) } const extensions = await queryFn(targetUrl, TARGET_EXTENSIONS_SQL) as unknown as @@ -593,13 +593,13 @@ export async function prepareClone( // this clone will not accept is not a reason to abandon the proof. If the // structure genuinely needed it, the replay fails next and says so — which // is a better error than this one (issue #94). - await queryFn(cloneUrl, `CREATE SCHEMA IF NOT EXISTS "${ext.schema}"`) + await queryFn(cloneUrl, `CREATE SCHEMA IF NOT EXISTS ${quoteName(ext.schema)}`) .catch(() => undefined) // Best-effort: an extension the server cannot offer this database is not a // reason to abandon the proof. If the schema genuinely needed it, the replay // fails next and says so. await queryFn(cloneUrl, - `CREATE EXTENSION IF NOT EXISTS "${ext.name}" WITH SCHEMA "${ext.schema}"`, + `CREATE EXTENSION IF NOT EXISTS ${quoteName(ext.name)} WITH SCHEMA ${quoteName(ext.schema)}`, ).catch(() => undefined) } } diff --git a/packages/cli/src/restore.ts b/packages/cli/src/restore.ts index 38d9d77..f4bea19 100644 --- a/packages/cli/src/restore.ts +++ b/packages/cli/src/restore.ts @@ -14,7 +14,7 @@ import { errMsg } from './utils/error' import { DEFAULT_IGNORE_SCHEMAS, SUPABASE_PLATFORM_SCHEMAS } from './defaults' import { dropUnsupportedSetStatements, knownParameters } from './prove' import { splitSqlStatements, isCommentOnly, isPsqlMetaCommand, stripPsqlMetaCommands } from './utils/sql-split' -import { quoteIdent, quoteLiteral } from './utils/sql.js' +import { quoteIdent, quoteLiteral, quoteName } from './utils/sql.js' import { sqlSkeleton, statementSubject, rolesNamedBy } from './sql-deps.js' import { ABSENT_ON_TARGET, EXTENSION_UNAVAILABLE } from './pg-errors.js' import { dataTablesInOrder, restoreBuckets, restoreTableRows } from './restore-data.js' @@ -843,7 +843,7 @@ async function applyStatement( // `CREATE EXTENSION ... WITH SCHEMA "extensions"` needs that schema to // exist, and on plain PostgreSQL it does not (issue #95). const needed = extensionTargetSchema(sql) - if (needed) await client.query(`CREATE SCHEMA IF NOT EXISTS ${quoteIdent(needed)}`) + if (needed) await client.query(`CREATE SCHEMA IF NOT EXISTS ${quoteName(needed)}`) // `GRANT … TO anon` and `CREATE POLICY … TO service_role` need the role to // exist, and on plain PostgreSQL none of Supabase's Data API roles do. @@ -934,7 +934,7 @@ export function extensionTargetSchema(sql: string): string | undefined { export function createRoleIfMissing(role: string): string { return `DO $$ BEGIN IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = ${quoteLiteral(role)}) THEN - CREATE ROLE ${quoteIdent(role)} NOLOGIN; + CREATE ROLE ${quoteName(role)} NOLOGIN; END IF; END $$;` } @@ -967,7 +967,7 @@ export function conditionalPublicationMembership(sql: string): string { AND schemaname = ${quoteLiteral(schema)} AND tablename = ${quoteLiteral(name)} ) THEN - EXECUTE ${quoteLiteral(`ALTER PUBLICATION ${quoteIdent(publication.replace(/"/g, ''))} ADD TABLE ${quoteIdent(schema)}.${quoteIdent(name)}`)}; + EXECUTE ${quoteLiteral(`ALTER PUBLICATION ${quoteIdent(publication.replace(/"/g, ''))} ADD TABLE ${quoteName(schema)}.${quoteName(name)}`)}; END IF; END $$;` } diff --git a/packages/cli/src/snapshot.ts b/packages/cli/src/snapshot.ts index 61a7776..b933588 100644 --- a/packages/cli/src/snapshot.ts +++ b/packages/cli/src/snapshot.ts @@ -5,7 +5,6 @@ import { promisify } from 'node:util' import type { QueryFn } from './db' import { pgQuery } from './db' import { quoteIdent, quoteLiteral, quoteName } from './utils/sql' -import { normalizeRoles } from './utils/strings' import type { EnvironmentConfig, SupaForgeConfig, SnapshotManifest, SnapshotLayerInfo } from './types/config' import { DEFAULT_IGNORE_SCHEMAS, RELATION_NOT_FOUND } from './defaults' import { introspectSchema } from './schema-introspect' @@ -539,7 +538,7 @@ async function captureRealtime( for (const row of rows) { const tables = byPublication.get(row.pubname) ?? [] if (row.schemaname && row.tablename) { - tables.push(`${quoteIdent(row.schemaname)}.${quoteIdent(row.tablename)}`) + tables.push(`${quoteName(row.schemaname)}.${quoteName(row.tablename)}`) } byPublication.set(row.pubname, tables) } @@ -549,11 +548,11 @@ async function captureRealtime( statements.push(`-- Publication: ${pubname}`) statements.push(`DO $$ BEGIN IF NOT EXISTS (SELECT 1 FROM pg_publication WHERE pubname = ${quoteLiteral(pubname)}) THEN - EXECUTE ${quoteLiteral(`CREATE PUBLICATION ${quoteIdent(pubname)}`)}; + EXECUTE ${quoteLiteral(`CREATE PUBLICATION ${quoteName(pubname)}`)}; END IF; END $$;`) for (const table of tables) { - statements.push(`ALTER PUBLICATION ${quoteIdent(pubname)} ADD TABLE ${table};`) + statements.push(`ALTER PUBLICATION ${quoteName(pubname)} ADD TABLE ${table};`) } } @@ -671,10 +670,10 @@ async function captureRoleGrants( const statements = rows.map(row => `GRANT ${row.privilege_type}` - + (row.column_name ? ` (${quoteIdent(row.column_name)})` : '') - + ` ON ${quoteIdent(row.table_schema)}.${quoteIdent(row.table_name)}` + + (row.column_name ? ` (${quoteName(row.column_name)})` : '') + + ` ON ${quoteName(row.table_schema)}.${quoteName(row.table_name)}` // PUBLIC is a keyword; quoted, it names a role that does not exist. - + ` TO ${row.grantee === 'PUBLIC' ? 'PUBLIC' : quoteIdent(row.grantee)}` + + ` TO ${row.grantee === 'PUBLIC' ? 'PUBLIC' : quoteName(row.grantee)}` + (row.is_grantable ? ' WITH GRANT OPTION' : '') + ';') @@ -702,7 +701,7 @@ async function captureExtensions( ORDER BY extname `) const statements = (rows as unknown as { extname: string; extversion: string; schema: string }[]).map(ext => { - return `CREATE EXTENSION IF NOT EXISTS "${ext.extname}" WITH SCHEMA "${ext.schema}";` + return `CREATE EXTENSION IF NOT EXISTS ${quoteName(ext.extname)} WITH SCHEMA ${quoteName(ext.schema)};` }) const output = statements.length > 0 ? `-- SupaForge Extensions Snapshot\n-- ${rows.length} extensions\n\n${statements.join('\n')}\n` @@ -853,27 +852,14 @@ interface RlsRow { comment?: string | null } +/** + * Dropped first so a snapshot can be restored twice: without it a second + * restore fails with `policy "…" already exists` and the layers after it + * never run (issue #80). Written by the shared builders, which set the + * comment after the policy. + */ function generateCreatePolicySql(p: RlsRow): string { - const roles = normalizeRoles(p.roles).join(', ') - const lines = [ - // Dropped first so a snapshot can be restored twice. Without it a second - // restore fails with `policy "…" already exists` and the layers after it - // never run (issue #80). The RLS check's own fix SQL already pairs the two - // this way. - `DROP POLICY IF EXISTS "${p.policyname}" ON "${p.schemaname}"."${p.tablename}";`, - `CREATE POLICY "${p.policyname}"`, - ` ON "${p.schemaname}"."${p.tablename}"`, - ` AS ${p.permissive}`, - ` FOR ${p.cmd}`, - ` TO ${roles}`, - ] - if (p.qual) lines.push(` USING (${p.qual})`) - if (p.with_check) lines.push(` WITH CHECK (${p.with_check})`) - lines.push(';') - if (p.comment) { - lines.push(`COMMENT ON POLICY "${p.policyname}" ON "${p.schemaname}"."${p.tablename}" IS ${quoteLiteral(p.comment)};`) - } - return lines.join('\n') + return `${dropPolicySql(p.schemaname, p)}\n${createPolicySql(p.schemaname, p)}` } interface CronRow { diff --git a/packages/cli/src/utils/policy-equivalence.ts b/packages/cli/src/utils/policy-equivalence.ts index a8db497..f3b6483 100644 --- a/packages/cli/src/utils/policy-equivalence.ts +++ b/packages/cli/src/utils/policy-equivalence.ts @@ -1,7 +1,7 @@ import type { CheckContext } from '../checks/base.js' import { inRolledBackTransaction, pgQuery, type QueryFn } from '../db.js' import type { DriftIssue } from '../types/drift.js' -import { quoteIdent } from './sql.js' +import { quoteName } from './sql.js' /** * The parts of a policy that decide whether two are the same. The RLS, storage @@ -43,7 +43,7 @@ export const canonicalPolicyExpressions: PolicyCanonicalizer = async (dbUrl, pol try { return await inRolledBackTransaction(dbUrl, async client => { await client.query( - `CREATE TEMP TABLE sf_canon (LIKE ${quoteIdent(policy.schemaname)}.${quoteIdent(policy.tablename)})`, + `CREATE TEMP TABLE sf_canon (LIKE ${quoteName(policy.schemaname)}.${quoteName(policy.tablename)})`, ) const command = (policy.cmd ?? 'ALL').toUpperCase() await client.query( diff --git a/packages/cli/src/utils/schema-policies.ts b/packages/cli/src/utils/schema-policies.ts index b8fbb8e..f457e31 100644 --- a/packages/cli/src/utils/schema-policies.ts +++ b/packages/cli/src/utils/schema-policies.ts @@ -15,13 +15,13 @@ */ import type { CheckName, DriftIssue } from '../types/drift' import { normalizeRoles } from './strings' -import { quoteLiteral } from './sql' +import { quoteLiteral, quoteName } from './sql' export interface SchemaPolicy { tablename: string policyname: string permissive: string - roles: string[] + roles: string[] | string cmd: string qual: string | null with_check: string | null @@ -47,11 +47,26 @@ export function policyKey(p: SchemaPolicy): string { return `${p.tablename}.${p.policyname}` } +/** `"schema"."table"`, each part quoted with its own quotes doubled. */ +const policyTable = (schema: string, p: Pick) => `${quoteName(schema)}.${quoteName(p.tablename)}` + +/** + * A role in a policy's TO list. pg_policies spells the PUBLIC pseudo-role + * `public`, which quoted would name a role that does not exist. + */ +const roleSql = (role: string) => role === 'public' ? 'PUBLIC' : quoteName(role) + +/** + * The SQL for a policy, used by every check and snapshot layer that writes one, + * so a name is quoted the same way everywhere. A policy named `say "hi" + * there`, written between plain quotes, was a syntax error that rolled back a + * whole restore or apply. + */ export function createPolicySql(schema: string, p: SchemaPolicy): string { - const roles = normalizeRoles(p.roles).join(', ') + const roles = normalizeRoles(p.roles).map(roleSql).join(', ') const lines = [ - `CREATE POLICY "${p.policyname}"`, - ` ON "${schema}"."${p.tablename}"`, + `CREATE POLICY ${quoteName(p.policyname)}`, + ` ON ${policyTable(schema, p)}`, ` AS ${p.permissive}`, ` FOR ${p.cmd}`, ` TO ${roles}`, @@ -65,11 +80,11 @@ export function createPolicySql(schema: string, p: SchemaPolicy): string { } export function commentPolicySql(schema: string, p: SchemaPolicy): string { - return `COMMENT ON POLICY "${p.policyname}" ON "${schema}"."${p.tablename}" IS ${p.comment ? quoteLiteral(p.comment) : 'NULL'};` + return `COMMENT ON POLICY ${quoteName(p.policyname)} ON ${policyTable(schema, p)} IS ${p.comment ? quoteLiteral(p.comment) : 'NULL'};` } export function dropPolicySql(schema: string, p: SchemaPolicy): string { - return `DROP POLICY IF EXISTS "${p.policyname}" ON "${schema}"."${p.tablename}";` + return `DROP POLICY IF EXISTS ${quoteName(p.policyname)} ON ${policyTable(schema, p)};` } export function policiesEqual(a: SchemaPolicy, b: SchemaPolicy): boolean { diff --git a/packages/cli/src/utils/strings.ts b/packages/cli/src/utils/strings.ts index 237edb5..66e8523 100644 --- a/packages/cli/src/utils/strings.ts +++ b/packages/cli/src/utils/strings.ts @@ -66,14 +66,42 @@ export function matchesGlob(value: string, pattern: string): boolean { /** * Parse Postgres name[] which may arrive as a JS array or Postgres literal `{a,b}`. * Returns a sorted, deduplicated array of role names. + * + * An element with a space, comma or quote in it is double-quoted in the + * literal, with `\\` escaping a quote or backslash: `{"Admins \\"A\\"",anon}`. + * Split on commas, such a role kept its literal quoting and was then quoted + * again as a name, so the policy went to a role that did not exist. */ export function normalizeRoles(roles: string[] | string): string[] { const arr = Array.isArray(roles) ? roles : [roles] return [...new Set( arr - .map(r => r.replace(/^\{|\}$/g, '')) - .flatMap(r => r.split(',')) + .flatMap(r => /^\{.*\}$/s.test(r) ? arrayLiteralElements(r) : [r]) .map(r => r.trim()) .filter(Boolean), )].sort() } + +/** The elements of a one-dimensional PostgreSQL array literal. */ +function arrayLiteralElements(literal: string): string[] { + const inner = literal.slice(1, -1) + const out: string[] = [] + let i = 0 + while (i < inner.length) { + let value = '' + if (inner[i] === '"') { + for (i++; i < inner.length && inner[i] !== '"'; i++) { + if (inner[i] === '\\') i++ + value += inner[i] + } + i++ + } else { + const end = inner.indexOf(',', i) + value = inner.slice(i, end === -1 ? inner.length : end) + i = end === -1 ? inner.length : end + } + out.push(value) + if (inner[i] === ',') i++ + } + return out +} diff --git a/packages/cli/test/checks/rls.test.ts b/packages/cli/test/checks/rls.test.ts index 4d067da..654ef84 100644 --- a/packages/cli/test/checks/rls.test.ts +++ b/packages/cli/test/checks/rls.test.ts @@ -148,7 +148,7 @@ describe('RlsCheck', () => { expect(sql).toContain('ON "public"."users"') expect(sql).toContain('AS PERMISSIVE') expect(sql).toContain('FOR SELECT') - expect(sql).toContain('TO authenticated') + expect(sql).toContain('TO "authenticated"') expect(sql).toContain('USING (') }) @@ -163,7 +163,7 @@ describe('RlsCheck', () => { const issues = await check.scan(mockContext()) const sql = issues[0].sql!.up - expect(sql).toContain('TO authenticated') + expect(sql).toContain('TO "authenticated"') expect(sql).not.toContain('{') }) }) diff --git a/packages/cli/test/dbdiff.test.ts b/packages/cli/test/dbdiff.test.ts index 1fadb81..2ed90b2 100644 --- a/packages/cli/test/dbdiff.test.ts +++ b/packages/cli/test/dbdiff.test.ts @@ -1499,3 +1499,15 @@ describe('a comment on an object the target lacks goes with its creation', () => expect(policy.sql?.up).toBe(`CREATE POLICY own ON t;\nCOMMENT ON POLICY own ON public.t IS 'x';`) }) }) + +// A name holding a double quote is written with it doubled. Read as two +// names, the title was "Policy missing: public.Odd .say". +describe('titles read names with doubled quotes', () => { + it.each([ + ['CREATE POLICY "say ""hi"" there" ON "Odd ""t""" FOR SELECT USING (true);', 'Policy missing: public.Odd "t".say "hi" there'], + ['CREATE TABLE "Odd ""t""" (id int);', 'Table missing: public.Odd "t"'], + ['DROP VIEW IF EXISTS "v ""w""";', 'Extra view: public.v "w"'], + ])('%s', (sql, title) => { + expect(summariseStatement(sql, 'schema')).toBe(title) + }) +}) diff --git a/packages/cli/test/e2e/quoted-names.test.ts b/packages/cli/test/e2e/quoted-names.test.ts new file mode 100644 index 0000000..5c51f67 --- /dev/null +++ b/packages/cli/test/e2e/quoted-names.test.ts @@ -0,0 +1,71 @@ +/** + * Names with double quotes in them survive a sync and a restore. + * + * A policy named `say "hi" there` was written between plain quotes, a syntax + * error: the restore holding it rolled back entirely, and `diff --apply` + * applied nothing from that run, unrelated fixes included. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest' +import { readdir } from 'node:fs/promises' +import { join } from 'node:path' +import { PgHarness } from '../harness/PgHarness.js' +import { describeWithContainers } from '../harness/containers.js' + +const describeE2E = describeWithContainers() + +const ROLE = `DO $$ BEGIN CREATE ROLE "Admins ""A""" NOLOGIN; EXCEPTION WHEN duplicate_object THEN NULL; END $$;` +const TABLE = `CREATE TABLE public."Odd ""table""" (id int PRIMARY KEY); + ALTER TABLE public."Odd ""table""" ENABLE ROW LEVEL SECURITY;` +const POLICIES = ` + CREATE POLICY "say ""hi"" there" ON public."Odd ""table""" FOR SELECT TO "Admins ""A""", authenticated USING (true); + COMMENT ON POLICY "say ""hi"" there" ON public."Odd ""table""" IS 'a "quoted" note'; +` +const PLATFORM = `DO $$ BEGIN CREATE ROLE authenticated NOLOGIN; EXCEPTION WHEN duplicate_object THEN NULL; END $$;` + +/** The policy as the catalog has it: name, roles and comment. */ +const POLICY = ` + SELECT format('%s | %s | %s', p.polname, + (SELECT string_agg(r::regrole::text, ',' ORDER BY r::regrole::text) FROM unnest(p.polroles) r), + obj_description(p.oid, 'pg_policy')) + FROM pg_policy p WHERE p.polrelid = to_regclass('public."Odd ""table"""') +` +const EXPECTED = `say "hi" there | "Admins ""A""",authenticated | a "quoted" note` + +describeE2E('e2e: names with double quotes in them', () => { + let h: PgHarness + let ws: string + + beforeAll(async () => { + h = new PgHarness({ verbose: !!process.env.E2E_VERBOSE, keep: !!process.env.E2E_KEEP }) + await h.up() + await h.applySql('source', PLATFORM + ROLE + TABLE + POLICIES) + await h.applySql('target', PLATFORM + ROLE + TABLE) + await h.createDatabase('target', 'sf_quoted_restore') + ws = await h.workspace({ + environments: { + source: { dbUrl: h.connectionString('source') }, + target: { dbUrl: h.connectionString('target') }, + restored: { dbUrl: h.urlFor('target', 'sf_quoted_restore') }, + }, + }) + }, 300_000) + + afterAll(async () => { await h?.down() }, 120_000) + + // The RLS check's own SQL. The schema check's comes from @dbdiff/cli, and + // the pg-conformance case quoted_identifiers covers it in the scenarios. + it('syncs the policy and its roles', async () => { + const sync = await h.cli(['diff', '--check', 'rls', '--apply'], { cwd: ws }) + expect(sync.stdout + sync.stderr).not.toMatch(/Rolled back|syntax error/) + expect(await h.sql('target', POLICY)).toBe(EXPECTED.replace(/ \| [^|]*$/, ' |')) + }, 300_000) + + it('restores it from a snapshot', async () => { + const snap = await h.cli(['snapshot', '-e', 'source', '--apply'], { cwd: ws }) + expect(snap.code, snap.stdout + snap.stderr).toBe(0) + const [snapshot] = await readdir(join(ws, '.supaforge', 'snapshots')) + const restore = await h.cli(['restore', '-e', 'restored', '--from-snapshot', snapshot, '--apply'], { cwd: ws }) + expect(restore.stdout + restore.stderr).not.toMatch(/Rolled back|syntax error/) + expect(await h.sqlIn('target', 'sf_quoted_restore', POLICY)).toBe(EXPECTED) + }, 300_000) +}) diff --git a/packages/cli/test/quoted-names.test.ts b/packages/cli/test/quoted-names.test.ts new file mode 100644 index 0000000..536093e --- /dev/null +++ b/packages/cli/test/quoted-names.test.ts @@ -0,0 +1,89 @@ +/** + * Every name SupaForge writes into SQL itself is quoted with its own quotes + * doubled. + * + * A policy named `say "hi" there` became `DROP POLICY IF EXISTS "say "hi" + * there"`, a syntax error: a restore holding one rolled back entirely, and a + * `diff --apply` holding one applied nothing at all. The SQL is read back here + * with PostgreSQL's own parser, so a name that does not survive the round trip + * fails the test whatever the text looks like. + */ +import { describe, it, expect, beforeEach, afterEach } from 'vitest' +import { mkdtemp, readFile, rm } from 'node:fs/promises' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { parseStatements } from '../src/sql-ast.js' +import { diffPolicies } from '../src/checks/rls.js' +import { createPolicySql, dropPolicySql, commentPolicySql, diffSchemaPolicies } from '../src/utils/schema-policies.js' +import { captureSnapshot } from '../src/snapshot.js' +import type { QueryFn } from '../src/db.js' + +const NAME = 'say "hi" there' +const TABLE = 'Odd "table"' + +/** The policy names, tables and roles each statement in `sql` names, as PostgreSQL reads them. */ +function read(sql: string) { + const statements = parseStatements(sql) + expect(statements, `does not parse:\n${sql}`).toBeDefined() + return statements!.map(({ kind, node }) => { + if (kind === 'CommentStmt') return { kind, name: node.object?.List?.items?.at(-1)?.String?.sval } + return { + kind, + name: node.policy_name, + table: node.table?.relname, + roles: (node.roles ?? []).map((r: { RoleSpec: { rolename?: string; roletype: string } }) => + r.RoleSpec.rolename ?? r.RoleSpec.roletype), + } + }) +} + +const policy = { + schemaname: 'public', tablename: TABLE, policyname: NAME, permissive: 'PERMISSIVE', + roles: ['Admins "A"', 'public'], cmd: 'SELECT', qual: 'true', with_check: null, comment: 'a "quoted" note', +} + +describe('policy SQL keeps names intact', () => { + it('in the shared builders', () => { + const sql = [createPolicySql('public', policy), dropPolicySql('public', policy), commentPolicySql('public', policy)].join('\n') + const [create, comment, drop] = read(sql) + expect(create).toMatchObject({ kind: 'CreatePolicyStmt', name: NAME, table: TABLE, roles: ['Admins "A"', 'ROLESPEC_PUBLIC'] }) + expect(comment).toMatchObject({ kind: 'CommentStmt', name: NAME }) + expect(drop.kind).toBe('DropStmt') + }) + + it('in the RLS check', () => { + const [issue] = diffPolicies([policy as never], []) + expect(read(issue.sql!.up)[0]).toMatchObject({ name: NAME, table: TABLE }) + expect(read(issue.sql!.down)[0].kind).toBe('DropStmt') + }) + + it('in the storage and Realtime checks', () => { + const [issue] = diffSchemaPolicies([{ ...policy, tablename: 'objects' }], [], { + schema: 'storage', check: 'storage', idPrefix: 'storage-policy', label: 'storage', + }) + expect(read(issue.sql!.up)[0]).toMatchObject({ name: NAME, table: 'objects' }) + }) +}) + +describe('snapshot files keep names intact', () => { + let dir: string + beforeEach(async () => { dir = await mkdtemp(join(tmpdir(), 'supaforge-quoted-')) }) + afterEach(async () => { await rm(dir, { recursive: true, force: true }) }) + + it('in rls.sql and storage-policies.sql', async () => { + const queryFn = (async (_url: string, sql: string) => + sql.includes('pg_policies') ? [{ ...policy, schemaname: sql.includes("'storage'") ? 'storage' : 'public' }] : []) as unknown as QueryFn + const result = await captureSnapshot({ + envName: 'prod', + env: { dbUrl: 'postgres://example' }, + config: { environments: { prod: { dbUrl: 'postgres://example' } } } as never, + outputDir: dir, + queryFn, + fetchFn: (async () => new Response('[]', { status: 200 })) as never, + }) + for (const file of ['rls.sql', 'storage-policies.sql']) { + const statements = read(await readFile(join(result.dir, file), 'utf8')) + expect(statements.find(s => s.kind === 'CreatePolicyStmt'), file).toMatchObject({ name: NAME, table: TABLE }) + } + }) +}) diff --git a/packages/cli/test/utils/strings.test.ts b/packages/cli/test/utils/strings.test.ts index a914945..6a3ff45 100644 --- a/packages/cli/test/utils/strings.test.ts +++ b/packages/cli/test/utils/strings.test.ts @@ -158,3 +158,19 @@ describe('escapeRegex', () => { expect(rx.test('axbbc')).toBe(false) }) }) + +describe('normalizeRoles: quoted elements of an array literal', () => { + it('reads a quoted role, with its escaped quotes, as the role itself', () => { + expect(normalizeRoles('{"Admins \\"A\\"",authenticated}')).toEqual(['Admins "A"', 'authenticated']) + }) + + it('keeps a comma or backslash inside a quoted role', () => { + expect(normalizeRoles('{"a,b","c\\\\d"}')).toEqual(['a,b', 'c\\d']) + }) + + it('reads plain literals and JS arrays as before', () => { + expect(normalizeRoles('{public}')).toEqual(['public']) + expect(normalizeRoles(['anon', 'authenticated'])).toEqual(['anon', 'authenticated']) + expect(normalizeRoles(['{anon,authenticated}'])).toEqual(['anon', 'authenticated']) + }) +}) From 74d727ecd690861a597b325594f12360a9caf659 Mon Sep 17 00:00:00 2001 From: Jasdeep Date: Wed, 7 Oct 2026 21:40:34 +0000 Subject: [PATCH 03/15] fix(diff): a view dropped and recreated is one modified view A changed view arrives from dbdiff as DROP VIEW + CREATE VIEW. Read by its DROP it was a critical 'Extra view' for a view both sides have. It is now one 'View modified' warning, materialized views alike, as triggers, policies and routines already are. Co-Authored-By: Claude Opus 5.5 --- packages/cli/src/dbdiff-units.ts | 4 +++- packages/cli/src/dbdiff.ts | 8 +++++++- packages/cli/test/dbdiff-units.test.ts | 16 ++++++++++++++++ packages/cli/test/dbdiff.test.ts | 8 ++++++++ 4 files changed, 34 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/dbdiff-units.ts b/packages/cli/src/dbdiff-units.ts index 3d05100..d8fef29 100644 --- a/packages/cli/src/dbdiff-units.ts +++ b/packages/cli/src/dbdiff-units.ts @@ -85,11 +85,13 @@ export function pairUnits( } /** The kinds dbdiff renders as dropping and recreating one object. */ -export const REPLACING_KINDS: Record = { +export const REPLACING_KINDS: Record = { AlterRoutine: 'routine', AlterEnum: 'type', AlterCompositeType: 'type', AlterDomain: 'type', AlterPolicy: 'policy', AlterTrigger: 'trigger', + AlterView: 'view', + AlterMatView: 'view', } diff --git a/packages/cli/src/dbdiff.ts b/packages/cli/src/dbdiff.ts index 171d53c..e82034d 100644 --- a/packages/cli/src/dbdiff.ts +++ b/packages/cli/src/dbdiff.ts @@ -924,7 +924,7 @@ interface MergedStatement { * same way — an enum whose values changed arrives as `DROP TYPE` + `CREATE * TYPE` — and needed the same treatment for the same two reasons (issue #81). */ -type ReplaceableKind = 'routine' | 'type' | 'sequence' | 'policy' | 'trigger' +type ReplaceableKind = 'routine' | 'type' | 'sequence' | 'policy' | 'trigger' | 'view' /** How each kind's identifier is read out of a statement. */ const REPLACEABLE_NAME: Record string> = { @@ -935,6 +935,7 @@ const REPLACEABLE_NAME: Record string> = { policy: (sql) => policyLabel(sql), // `schema.table.trigger`, for the same reason. trigger: (sql) => triggerLabel(sql), + view: (sql) => extractQualifiedName(sql, AFTER.view), } /** How a merged pair of each kind is reported. */ @@ -944,6 +945,7 @@ const REPLACEABLE_REPORT: Record { // A changed trigger is a DROP and a CREATE too. Read by its DROP, it was a // critical "Extra trigger" for a trigger both sides have. + // A changed view is a DROP and a CREATE: read by the DROP, a critical + // "Extra view" for a view both sides have. + it('names a replaced view or materialized view as modified', () => { + const issues = sqlToIssues({ + up: units( + ['AlterView', 'vt_v', 'DROP VIEW IF EXISTS "vt_v";\nCREATE VIEW "vt_v" AS SELECT vt.id FROM vt;\nREVOKE ALL ON "vt_v" FROM anon;'], + ['AlterMatView', 'vt_m', 'DROP MATERIALIZED VIEW IF EXISTS "vt_m";\nCREATE MATERIALIZED VIEW "vt_m" AS SELECT 1;'], + ), + down: '', + }, 'schema') + expect(issues.map(i => [i.title, i.severity])).toEqual([ + ['View modified: public.vt_v', 'warning'], + ['View modified: public.vt_m', 'warning'], + ]) + }) + it('names a replaced trigger as modified, by its table', () => { const [issue] = sqlToIssues({ up: units(['AlterTrigger', 'orders.note_changed', [ diff --git a/packages/cli/test/dbdiff.test.ts b/packages/cli/test/dbdiff.test.ts index 2ed90b2..64bd275 100644 --- a/packages/cli/test/dbdiff.test.ts +++ b/packages/cli/test/dbdiff.test.ts @@ -592,6 +592,14 @@ describe('sqlToIssues — programmable objects', () => { expect(issues[0].id).toBe('schema-drop-trigger-1') }) + it('merges a view dropped and recreated into one modified view', () => { + const issues = sqlToIssues({ + up: 'DROP VIEW IF EXISTS "vt_v";\nCREATE VIEW "vt_v" AS SELECT vt.id FROM vt;', + down: '', + }, 'schema') + expect(issues.map(i => [i.title, i.severity])).toEqual([['View modified: public.vt_v', 'warning']]) + }) + it('merges a trigger dropped and recreated into one modified trigger', () => { const issues = sqlToIssues({ up: [ From 265f8675e632b3412f92d1193894d88c5e78bb85 Mon Sep 17 00:00:00 2001 From: Jasdeep Date: Wed, 7 Oct 2026 21:55:14 +0000 Subject: [PATCH 04/15] fix(cli): --json prints JSON alone, and an unreadable history is not 'pending' Under --json every line a command prints besides its result now goes to stderr: migrate list and clone printed their preflight checks, and clone its transfer meter, on stdout before the JSON. Each command's JSON goes through one helper, and a clone preview prints a JSON result too. migrate list, connected as a role that cannot read the migration history, warned and then listed every migration as pending, exiting 0. It now shows the status as unknown (applied: null under --json) and exits 1. Co-Authored-By: Claude Opus 5.5 --- packages/cli/src/base-command.ts | 19 +++++ packages/cli/src/commands/clone.ts | 19 +++-- packages/cli/src/commands/diff.ts | 6 +- packages/cli/src/commands/migrate/list.ts | 24 +++--- packages/cli/src/commands/report.ts | 2 +- packages/cli/src/commands/restore.ts | 10 +-- packages/cli/src/commands/snapshot.ts | 12 +-- .../cli/test/e2e/json-and-history.test.ts | 85 +++++++++++++++++++ 8 files changed, 146 insertions(+), 31 deletions(-) create mode 100644 packages/cli/test/e2e/json-and-history.test.ts diff --git a/packages/cli/src/base-command.ts b/packages/cli/src/base-command.ts index 0cbba15..72477fd 100644 --- a/packages/cli/src/base-command.ts +++ b/packages/cli/src/base-command.ts @@ -1,3 +1,4 @@ +import { formatWithOptions } from 'node:util' import { Command } from '@oclif/core' import { loadConfig, validateConfig, validateSingleEnvConfig } from './config.js' import { Preflight } from './preflight.js' @@ -16,6 +17,24 @@ export abstract class BaseCommand extends Command { private _logWritten = false private _checkSummaries?: RunLogCheckSummary[] + /** + * Under --json, stdout carries the JSON and nothing else: every other line a + * command prints — preflight checks, progress — goes to stderr. Before, the + * text came first and the output did not parse. + */ + override log(message = '', ...args: unknown[]): void { + if (this.argv.includes('--json')) { + process.stderr.write(formatWithOptions({ colors: false }, message, ...args) + '\n') + return + } + super.log(message, ...args) + } + + /** The command's result as JSON, on stdout. */ + protected json(value: unknown): void { + process.stdout.write(JSON.stringify(value, null, 2) + '\n') + } + /** Call after scan() to attach per-check metadata to the run log entry. */ protected setCheckSummaries(summaries: RunLogCheckSummary[]): void { this._checkSummaries = summaries diff --git a/packages/cli/src/commands/clone.ts b/packages/cli/src/commands/clone.ts index 5c2bdb5..e61425c 100644 --- a/packages/cli/src/commands/clone.ts +++ b/packages/cli/src/commands/clone.ts @@ -124,7 +124,7 @@ export default class Clone extends BaseCommand { } if (flags.json) { - this.log(JSON.stringify(branches, null, 2)) + this.json(branches) return } @@ -247,6 +247,11 @@ export default class Clone extends BaseCommand { const report = await pre.run() if (!flags.apply) { + if (flags.json) { + this.json({ dryRun: true, passed: report.passed, localDb: localDbName }) + if (!report.passed) this.exit(1) + return + } if (report.passed) { this.log(` Steps that will be performed:`) this.log(' 1. Create local database via pg_dump | pg_restore') @@ -266,6 +271,8 @@ export default class Clone extends BaseCommand { // Execute clone this.log(`\n ${bold(`Cloning "${envName}" to local database "${localDbName}"...`)}\n`) + // The transfer meter, like every other line here, stays off stdout under --json. + const progressOut = flags.json ? process.stderr : process.stdout this.log(' [1/4] Creating local database...') let cloned: CloneResult @@ -280,10 +287,10 @@ export default class Clone extends BaseCommand { onProgress: (p) => { const mb = (p.bytesTransferred / 1024 / 1024).toFixed(1) const sec = Math.round(p.elapsedMs / 1000) - process.stdout.write(`\r ${dim(`pg_dump → pg_restore: ${mb} MB transferred (${sec}s)`)} `) + progressOut.write(`\r ${dim(`pg_dump → pg_restore: ${mb} MB transferred (${sec}s)`)} `) }, }) - process.stdout.write('\n') + progressOut.write('\n') this.log(` ${ok('✓')} Database created: ${bold(localDbName)}`) if (cloned.extensionsInstalled.length > 0) { this.log(` ${dim(`Installed first, in the schema the source keeps them in: ${cloned.extensionsInstalled.join(', ')}`)}`) @@ -291,7 +298,7 @@ export default class Clone extends BaseCommand { for (const line of formatUnavailable(cloned.unavailable)) this.log(line) for (const line of formatRestoreFailures(cloned.failures)) this.log(line) } catch (err) { - process.stdout.write('\n') + progressOut.write('\n') const msg = errMsg(err) this.log(` ${warn('✗')} Failed: ${msg}`) this.error('Clone aborted at step 1/4.', { exit: 1 }) @@ -354,10 +361,10 @@ export default class Clone extends BaseCommand { const missing = cloned.failures.length if (flags.json) { - this.log(JSON.stringify({ + this.json({ snapshot: snapshot.manifest, config: newConfig, restoreFailures: cloned.failures, unavailable: cloned.unavailable, - }, null, 2)) + }) if (missing > 0) this.exit(1) return } diff --git a/packages/cli/src/commands/diff.ts b/packages/cli/src/commands/diff.ts index 7fb2be4..8f7f96d 100644 --- a/packages/cli/src/commands/diff.ts +++ b/packages/cli/src/commands/diff.ts @@ -350,7 +350,7 @@ export default class Diff extends BaseCommand { // of an apply failed on the one run where there was nothing to do. if (flags.json) { const empty: PromoteResult = { applied: [], skipped: [], errors: [] } - this.log(JSON.stringify(empty, null, 2)) + this.json(empty) return } this.log(`${ok('No drift detected.')} Nothing to apply. ✓`) @@ -445,7 +445,7 @@ export default class Diff extends BaseCommand { }) if (flags.json) { - this.log(JSON.stringify(result, null, 2)) + this.json(result) return } @@ -517,7 +517,7 @@ export default class Diff extends BaseCommand { } if (flags.json) { - this.log(JSON.stringify(result, null, 2)) + this.json(result) } else if (flags.detail) { this.log(renderDetailed(result)) this.log(renderTip({ diff --git a/packages/cli/src/commands/migrate/list.ts b/packages/cli/src/commands/migrate/list.ts index 4cea810..69a9477 100644 --- a/packages/cli/src/commands/migrate/list.ts +++ b/packages/cli/src/commands/migrate/list.ts @@ -45,7 +45,7 @@ export default class MigrateList extends BaseCommand { if (flags.offline || !config.source) { // Offline: list files only if (flags.json) { - this.log(JSON.stringify(migrations, null, 2)) + this.json(migrations) return } if (migrations.length === 0) { @@ -67,22 +67,26 @@ export default class MigrateList extends BaseCommand { .addDatabase('Target', envName, env.dbUrl) await this.runPreflight(pre, 'Migrate list') - // Read only: a listing creates nothing. A history it cannot read is said - // so, rather than listing every migration as pending. - let applied: Set + // Read only: a listing creates nothing. A history it cannot read leaves + // every status unknown: listing them as pending read as "nothing has been + // applied", and exited 0. + let applied: Set | undefined try { applied = await getAppliedVersions(env.dbUrl, pgQuery) } catch (err) { this.warn(`Could not read which migrations are applied: ${errMsg(err)}`) - applied = new Set() + process.exitCode = 1 } if (flags.json) { - const result = migrations.map(m => ({ - ...m, - applied: applied.has(m.version), - })) - this.log(JSON.stringify(result, null, 2)) + this.json(migrations.map(m => ({ ...m, applied: applied ? applied.has(m.version) : null }))) + return + } + + if (!applied) { + this.log(`\n ${bold(`${migrations.length} migration(s)`)} ${dim(`→ ${envName}`)} · ${warn('status unknown')}\n`) + for (const m of migrations) this.log(` ${warn('?')} ${m.filename}`) + this.log('') return } diff --git a/packages/cli/src/commands/report.ts b/packages/cli/src/commands/report.ts index 111137b..41a9167 100644 --- a/packages/cli/src/commands/report.ts +++ b/packages/cli/src/commands/report.ts @@ -46,7 +46,7 @@ export default class Report extends BaseCommand { const entries = all.slice(-flags.last) if (flags.json) { - this.log(JSON.stringify(entries, null, 2)) + this.json(entries) return } diff --git a/packages/cli/src/commands/restore.ts b/packages/cli/src/commands/restore.ts index 74e29bf..472a27b 100644 --- a/packages/cli/src/commands/restore.ts +++ b/packages/cli/src/commands/restore.ts @@ -85,7 +85,7 @@ export default class Restore extends BaseCommand { const tables = await getPublicTables(env.dbUrl) if (tables.length > 0 && !flags.force) { if (flags.json) { - this.log(JSON.stringify({ error: 'Target database is not empty', tables }, null, 2)) + this.json({ error: 'Target database is not empty', tables }) this.exit(1) } this.log(`\n${warn('Target database is not empty.')} Found ${tables.length} table(s) in public schema:`) @@ -140,7 +140,7 @@ export default class Restore extends BaseCommand { if (!flags.apply) { const preview = await previewSnapshotRestore(snapshotDir) if (flags.json) { - this.log(JSON.stringify({ dryRun: true, snapshot: snapshotDir, layers: preview }, null, 2)) + this.json({ dryRun: true, snapshot: snapshotDir, layers: preview }) return } this.log('\nRestore preview (dry-run) -- from snapshot\n') @@ -195,7 +195,7 @@ export default class Restore extends BaseCommand { if (!flags.apply) { const migrations = await previewMigrationRestore(process.cwd(), toVersion, fromVersion) if (flags.json) { - this.log(JSON.stringify({ + this.json({ dryRun: true, migrations: migrations.map(m => ({ version: m.version, @@ -204,7 +204,7 @@ export default class Restore extends BaseCommand { sqlStatements: m.up.sql.length, apiActions: m.up.api.length, })), - }, null, 2)) + }) return } this.log('\nRestore preview (dry-run) -- from migrations\n') @@ -248,7 +248,7 @@ export default class Restore extends BaseCommand { // Only the JSON on stdout, so it parses, and the same exit code as the // text report: a restore that failed or left things out is not one a // script should read as success. - this.log(JSON.stringify(result, null, 2)) + this.json(result) if (result.errors.length > 0 || result.incomplete) this.exit(1) return } diff --git a/packages/cli/src/commands/snapshot.ts b/packages/cli/src/commands/snapshot.ts index 4a3846f..3356959 100644 --- a/packages/cli/src/commands/snapshot.ts +++ b/packages/cli/src/commands/snapshot.ts @@ -94,7 +94,7 @@ export default class Snapshot extends BaseCommand { return } if (flags.json) { - this.log(JSON.stringify(snapshots.map(s => s.manifest), null, 2)) + this.json(snapshots.map(s => s.manifest)) return } this.log(`\n${bold(`${snapshots.length} snapshot(s):`)}\n`) @@ -136,7 +136,7 @@ export default class Snapshot extends BaseCommand { const result = await pruneSnapshots(keepCount) if (flags.json) { - this.log(JSON.stringify(result, null, 2)) + this.json(result) return } @@ -164,7 +164,7 @@ export default class Snapshot extends BaseCommand { // Like clone / diff / restore, snapshot is a preview unless --apply is given. if (!flags.apply) { if (flags.json) { - this.log(JSON.stringify({ dryRun: true, environment: envName, migration: flags.migration }, null, 2)) + this.json({ dryRun: true, environment: envName, migration: flags.migration }) return } this.log(`\n ${bold('Snapshot preview')} ${dim('(dry-run)')}\n`) @@ -195,12 +195,12 @@ export default class Snapshot extends BaseCommand { }) if (flags.json) { - this.log(JSON.stringify({ + this.json({ isBaseline: result.isBaseline, snapshot: result.snapshot.manifest, migration: result.migration, migrationFile: result.migrationFile, - }, null, 2)) + }) return } @@ -233,7 +233,7 @@ export default class Snapshot extends BaseCommand { const snapshot = await captureSnapshot({ envName, env, config, outputDir: flags.output }) if (flags.json) { - this.log(JSON.stringify(snapshot.manifest, null, 2)) + this.json(snapshot.manifest) return } diff --git a/packages/cli/test/e2e/json-and-history.test.ts b/packages/cli/test/e2e/json-and-history.test.ts new file mode 100644 index 0000000..c677711 --- /dev/null +++ b/packages/cli/test/e2e/json-and-history.test.ts @@ -0,0 +1,85 @@ +/** + * `--json` means stdout is JSON, and a migration history that cannot be read + * is not reported as nothing applied. + * + * `migrate list --json` and `clone --json` printed their preflight checks on + * stdout first, so the output did not parse. And `migrate list`, connected as a + * role that cannot read the history table, warned and then listed every + * applied migration as pending, exiting 0. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest' +import { mkdir, writeFile } from 'node:fs/promises' +import { join } from 'node:path' +import { PgHarness } from '../harness/PgHarness.js' +import { describeWithContainers } from '../harness/containers.js' + +const describeE2E = describeWithContainers() + +const HISTORY = ` + CREATE SCHEMA supabase_migrations; + CREATE TABLE supabase_migrations.schema_migrations (version text PRIMARY KEY, statements text[], name text); + INSERT INTO supabase_migrations.schema_migrations (version, name) VALUES ('20261007000001', 'init'); + DO $$ BEGIN CREATE ROLE sf_mig_user LOGIN PASSWORD 'sf_mig_pass'; EXCEPTION WHEN duplicate_object THEN NULL; END $$; + GRANT CONNECT ON DATABASE postgres TO sf_mig_user; +` + +describeE2E('e2e: --json output, and a history the role cannot read', () => { + let h: PgHarness + let ws: string + + /** stdout parsed as JSON, or the test fails showing what was printed. */ + function parsed(stdout: string): unknown { + try { + return JSON.parse(stdout) + } catch { + throw new Error(`stdout is not JSON:\n${stdout.slice(0, 400)}`) + } + } + + beforeAll(async () => { + h = new PgHarness({ verbose: !!process.env.E2E_VERBOSE, keep: !!process.env.E2E_KEEP }) + await h.up() + await h.applySql('source', 'CREATE TABLE public.items (id int PRIMARY KEY);') + await h.applySql('target', HISTORY) + const limited = new URL(h.connectionString('target')) + limited.username = 'sf_mig_user' + limited.password = 'sf_mig_pass' + ws = await h.workspace({ + environments: { + source: { dbUrl: h.connectionString('source') }, + target: { dbUrl: h.connectionString('target') }, + limited: { dbUrl: limited.toString() }, + }, + }) + await mkdir(join(ws, 'supabase', 'migrations'), { recursive: true }) + await writeFile(join(ws, 'supabase', 'migrations', '20261007000001_init.sql'), 'SELECT 1;\n') + }, 300_000) + + afterAll(async () => { await h?.down() }, 120_000) + + it('migrate list --json prints JSON alone', async () => { + const r = await h.cli(['migrate', 'list', '--env', 'target', '--json'], { cwd: ws }) + expect(r.code, r.stderr).toBe(0) + expect(parsed(r.stdout)).toMatchObject([{ version: '20261007000001', applied: true }]) + }, 120_000) + + it('migrate list says the status is unknown, and exits 1, when the role cannot read the history', async () => { + const text = await h.cli(['migrate', 'list', '--env', 'limited'], { cwd: ws }) + expect(text.code).toBe(1) + expect(text.stdout).toMatch(/status unknown/) + expect(text.stdout).not.toMatch(/pending/) + + const json = await h.cli(['migrate', 'list', '--env', 'limited', '--json'], { cwd: ws }) + expect(json.code).toBe(1) + expect(parsed(json.stdout)).toMatchObject([{ version: '20261007000001', applied: null }]) + }, 120_000) + + it('clone --json prints JSON alone, previewed and applied', async () => { + const args = ['clone', '--env', 'source', '--local-url', h.urlFor('target', 'postgres'), '--local-db', 'sf_clone_json', '--schema-only', '--json'] + const preview = await h.cli(args, { cwd: ws }) + expect(parsed(preview.stdout)).toMatchObject({ dryRun: true, passed: true }) + + const applied = await h.cli([...args, '--apply'], { cwd: ws }) + expect(parsed(applied.stdout)).toHaveProperty('snapshot') + }, 300_000) +}) From f6069beffeddd10a6a367f16a024e48177948646 Mon Sep 17 00:00:00 2001 From: Jasdeep Date: Wed, 7 Oct 2026 21:55:14 +0000 Subject: [PATCH 05/15] fix(snapshot): keep json values and JSON null exactly in captured rows Rows were captured as JSON values, so a json column came back re-rendered (pretty-printed, keys reordered, a duplicate key collapsed) and a jsonb holding JSON null came back as SQL NULL. Each column is now captured as PostgreSQL's own text for it, which its type reads back exactly, and restored with a cast per column. Snapshots in the earlier format still restore. Co-Authored-By: Claude Opus 5.5 --- packages/cli/src/restore-data.ts | 19 +++++++---- packages/cli/src/snapshot.ts | 27 +++++++++++---- .../cli/test/e2e/snapshot-restore.test.ts | 34 +++++++++++++++++-- 3 files changed, 64 insertions(+), 16 deletions(-) diff --git a/packages/cli/src/restore-data.ts b/packages/cli/src/restore-data.ts index 8377ad8..c8a9946 100644 --- a/packages/cli/src/restore-data.ts +++ b/packages/cli/src/restore-data.ts @@ -10,7 +10,7 @@ import { readdir, readFile } from 'node:fs/promises' import { join } from 'node:path' import type pg from 'pg' -import { quoteIdent, quoteName } from './utils/sql' +import { quoteIdent, quoteLiteral, quoteName } from './utils/sql' /** The tables a snapshot holds rows for, by file name. */ export async function dataTablesIn(dataDir: string): Promise { @@ -58,11 +58,15 @@ export async function restoreBuckets(client: pg.Client, file: string): Promise { + // Each column's text, which the column's type reads back exactly (see + // capturedRows) — or, from a snapshot taken before that, JSON values. + const asText = rows.trimStart().startsWith('{') + const elements = asText ? `json_array_elements($1::json -> 'rows')` : `json_array_elements($1::json)` const { rows: keys } = await client.query<{ key: string }>( - `SELECT DISTINCT jsonb_object_keys(e) AS key FROM jsonb_array_elements($1::jsonb) e`, [rows]) + `SELECT DISTINCT json_object_keys(e) AS key FROM ${elements} e`, [rows]) if (keys.length === 0) return 0 - const { rows: columns } = await client.query<{ name: string; identity: string; generated: string }>( - `SELECT attname AS name, attidentity AS identity, attgenerated AS generated + const { rows: columns } = await client.query<{ name: string; identity: string; generated: string; type: string }>( + `SELECT attname AS name, attidentity AS identity, attgenerated AS generated, format_type(atttypid, atttypmod) AS type FROM pg_attribute WHERE attrelid = $1::regclass AND attnum > 0 AND NOT attisdropped ORDER BY attnum`, [table]) // The columns both the rows and the table have: a column added since the @@ -71,10 +75,11 @@ async function insertRows(client: pg.Client, table: string, rows: string): Promi const usable = columns.filter(c => c.generated === '' && captured.has(c.name)) const list = usable.map(c => quoteName(c.name)).join(', ') const overriding = usable.some(c => c.identity === 'a') ? ' OVERRIDING SYSTEM VALUE' : '' + const select = asText + ? `SELECT ${usable.map(c => `(e ->> ${quoteLiteral(c.name)})::${c.type}`).join(', ')} FROM ${elements} e` + : `SELECT ${list} FROM json_populate_recordset(NULL::${table}, $1::json)` const { rowCount } = await client.query( - `INSERT INTO ${table} (${list})${overriding} - SELECT ${list} FROM json_populate_recordset(NULL::${table}, $1::json) ON CONFLICT DO NOTHING`, - [rows]) + `INSERT INTO ${table} (${list})${overriding} ${select} ON CONFLICT DO NOTHING`, [rows]) return rowCount ?? 0 } diff --git a/packages/cli/src/snapshot.ts b/packages/cli/src/snapshot.ts index b933588..15b2172 100644 --- a/packages/cli/src/snapshot.ts +++ b/packages/cli/src/snapshot.ts @@ -444,12 +444,7 @@ async function captureData( for (const table of tables) { try { - // Rendered by the server, not by the driver: a bigint or numeric past - // 2^53 kept its digits, a timestamp its zone, a bytea its bytes — each - // as the text restore hands back to json_populate_recordset. - const [{ rows }] = await queryFn(dbUrl, `SELECT jsonb_pretty(coalesce(jsonb_agg(to_jsonb(t)), '[]'::jsonb)) AS rows - FROM (SELECT * FROM ${quoteIdent(table)} ORDER BY 1) t`) as unknown as Array<{ rows: string }> - await writeFile(join(dataDir, `${table}.json`), rows + '\n') + await writeFile(join(dataDir, `${table}.json`), await capturedRows(dbUrl, table, queryFn) + '\n') captured++ } catch (err) { errors.push(`${table}: ${errMsg(err)}`) @@ -465,6 +460,26 @@ async function captureData( } +/** + * A table's rows, each column as PostgreSQL's own text for it — the text its + * input function reads back exactly, as pg_dump relies on. A bigint past 2^53, + * a long numeric, a timestamp's zone, bytea and arrays come back as they were; + * so does a json value's text, spacing and duplicate keys included, and a JSON + * null stays distinct from an SQL NULL. Captured as JSON values instead, json + * was re-rendered and a JSON null read back as NULL. + */ +async function capturedRows(dbUrl: string, table: string, queryFn: QueryFn): Promise { + const columns = (await queryFn(dbUrl, `SELECT attname AS name FROM pg_attribute + WHERE attrelid = ${quoteLiteral(quoteIdent(table))}::regclass AND attnum > 0 AND NOT attisdropped ORDER BY attnum`) as unknown as Array<{ name: string }>) + .map(c => c.name) + const keys = columns.map(quoteLiteral).join(', ') + const values = columns.map(c => `t.${quoteName(c)}::text`).join(', ') + const [{ rows }] = await queryFn(dbUrl, `SELECT jsonb_pretty(jsonb_build_object('format', 'text', 'rows', + coalesce(jsonb_agg(json_object(ARRAY[${keys}]::text[], ARRAY[${values}]::text[])::jsonb), '[]'::jsonb))) AS rows + FROM (SELECT * FROM ${quoteIdent(table)} ORDER BY 1) t`) as unknown as Array<{ rows: string }> + return rows +} + async function captureWebhooks( dir: string, dbUrl: string, diff --git a/packages/cli/test/e2e/snapshot-restore.test.ts b/packages/cli/test/e2e/snapshot-restore.test.ts index 575138f..def02c3 100644 --- a/packages/cli/test/e2e/snapshot-restore.test.ts +++ b/packages/cli/test/e2e/snapshot-restore.test.ts @@ -58,12 +58,16 @@ const SOURCE = ` since timestamptz, badge bytea, tags text[], + raw json, + doc jsonb, doubled numeric GENERATED ALWAYS AS (price * 2) STORED ); CREATE TABLE features (id serial PRIMARY KEY, plan_id bigint NOT NULL REFERENCES plans (id), name text); - INSERT INTO plans (code, price, quota, since, badge, tags) VALUES - ('free', 0, 9007199254740993, '2026-01-01 00:00:00.123456+05:30', '\\x00ff10', '{a,"b c"}'), - ('pro', 12345678901234567890.0123456789, NULL, NULL, NULL, NULL); + -- json keeps its text exactly — spacing, key order, a duplicate key — and a + -- JSON null is not an SQL NULL. + INSERT INTO plans (code, price, quota, since, badge, tags, raw, doc) VALUES + ('free', 0, 9007199254740993, '2026-01-01 00:00:00.123456+05:30', '\\x00ff10', '{a,"b c"}', '{"dup": 1, "dup": 2, "b": []}', 'null'), + ('pro', 12345678901234567890.0123456789, NULL, NULL, NULL, NULL, '[]', NULL); INSERT INTO features (plan_id, name) VALUES (1, 'one'), (2, 'two'), (2, 'three'); ` @@ -83,6 +87,7 @@ describeE2E('e2e: snapshot restored into an empty database', () => { target: { dbUrl: h.connectionString('target') }, restored: { dbUrl: h.urlFor('target', restored) }, partial: { dbUrl: h.urlFor('target', 'sf_partial') }, + old: { dbUrl: h.urlFor('target', 'sf_old_format') }, }, checks: { data: { tables: ['public.features', 'public.plans'] } }, }) @@ -108,6 +113,10 @@ describeE2E('e2e: snapshot restored into an empty database', () => { expect(await h.sqlIn('target', restored, schemas)).toBe('billing,staging') // The rows, value for value, and sequences that carry on past them. + // A JSON null and an SQL NULL print alike in a row's text, so the json + // columns are compared on their own as well. + const json = `SELECT string_agg(coalesce(raw::text, 'SQL NULL') || ' / ' || coalesce(doc::text, 'SQL NULL'), ' | ' ORDER BY id) FROM public.plans` + expect(await h.sqlIn('target', restored, json)).toBe(await h.sql('source', json)) for (const table of ['plans', 'features']) { const rows = `SELECT string_agg(t::text, ' | ' ORDER BY id) FROM public.${table} t` expect(await h.sqlIn('target', restored, rows)).toBe(await h.sql('source', rows)) @@ -139,6 +148,25 @@ describeE2E('e2e: snapshot restored into an empty database', () => { expect(refusal.tables).toContain('orders') }, 120_000) + // Snapshots taken before rows were captured as each column's text hold JSON + // values instead; those still restore. + it('restores rows a snapshot holds in the earlier format', async () => { + const snapshots = join(ws, '.supaforge', 'snapshots') + const [taken] = (await readdir(snapshots)).sort() + const copy = '20980101T000000Z' + await cp(join(snapshots, taken), join(snapshots, copy), { recursive: true }) + const manifestFile = join(snapshots, copy, 'manifest.json') + await writeFile(manifestFile, JSON.stringify({ ...JSON.parse(await readFile(manifestFile, 'utf8')), timestamp: copy })) + await writeFile(join(snapshots, copy, 'data', 'public.features.json'), + JSON.stringify([{ id: 1, plan_id: 1, name: 'one' }, { id: 2, plan_id: 2, name: 'two' }])) + await writeFile(join(snapshots, copy, 'data', 'public.plans.json'), + JSON.stringify([{ id: 1, code: 'free', price: 0 }, { id: 2, code: 'pro', price: 1.5 }])) + await h.createDatabase('target', 'sf_old_format') + const r = await h.cli(['restore', '-e', 'old', '--from-snapshot', copy, '--apply'], { cwd: ws }) + expect(r.stdout + r.stderr).not.toMatch(/Rolled back/) + expect(await h.sqlIn('target', 'sf_old_format', `SELECT string_agg(name, ',' ORDER BY id) FROM public.features`)).toBe('one,two') + }, 300_000) + // A snapshot needing an extension this server does not ship. Everything // that needs it used to roll the whole restore back with it. it('restores what it can without a missing extension, says what it left out, and exits 1', async () => { From 221b2a9bbc341d3e791bd7cf9a55db8cd069a2f8 Mon Sep 17 00:00:00 2001 From: Jasdeep Date: Wed, 7 Oct 2026 22:01:31 +0000 Subject: [PATCH 06/15] fix: fold function comments and new tables' RLS flags; tidier restore reports - A comment on a new function folds into its 'Function missing' finding, matched by name when only one new routine has it: dbdiff spells the signature with argument types in the comment and with the parameter list in the create, so the two never matched. - A new table's ENABLE/FORCE ROW LEVEL SECURITY folds into its creation instead of being a 'Table altered' finding for a table not there yet. - A restore skip names only the missing extension or schema its error is about, and the snapshot groups a role's privileges on an object into one GRANT, so a skipped relation is one line per role, not one per privilege. - A restore that rolled back no longer also reports incomplete. Co-Authored-By: Claude Opus 5.5 --- packages/cli/src/dbdiff.ts | 56 ++++++++++++++++--- packages/cli/src/restore.ts | 12 +++- packages/cli/src/snapshot.ts | 13 ++++- packages/cli/test/dbdiff.test.ts | 37 ++++++++++++ .../cli/test/e2e/snapshot-restore.test.ts | 25 +++++++++ packages/cli/test/review-fixes.test.ts | 18 ++++++ packages/cli/test/snapshot.test.ts | 35 ++++++++++++ 7 files changed, 183 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/dbdiff.ts b/packages/cli/src/dbdiff.ts index e82034d..6b982f8 100644 --- a/packages/cli/src/dbdiff.ts +++ b/packages/cli/src/dbdiff.ts @@ -577,21 +577,22 @@ const ON_A_TABLE = new Set(['column', 'constraint', 'trigger', 'policy', 'rule'] */ export function foldCommentsIntoCreates(issues: DriftIssue[]): DriftIssue[] { const creates = new Map() + /** New routines by name alone, for a comment whose signature is spelled differently. */ + const routines = new Map() for (const issue of issues) { const m = CREATE_TITLE.exec(issue.title) - if (m) creates.set(`${m[1] === 'Schema' ? 'schema' : 'object'}:${m[2]}`, issue) + if (!m) continue + creates.set(`${m[1] === 'Schema' ? 'schema' : 'object'}:${m[2]}`, issue) + if (m[1] === 'Function' || m[1] === 'Procedure') { + const base = routineBaseName(m[2]) + routines.set(base, [...(routines.get(base) ?? []), issue]) + } } return issues.filter(issue => { - const m = COMMENT_TITLE.exec(issue.title) const up = issue.sql?.up - if (!m || !up || splitStatements(up).length !== 1) return true - - const [, schema, name, kind] = m - const candidates = schema !== undefined - ? [`schema:${schema}`] - : [`object:${name}`, ...(ON_A_TABLE.has(kind) ? [`object:${name.slice(0, name.lastIndexOf('.'))}`] : [])] - const owner = candidates.map(c => creates.get(c)).find(Boolean) + if (!up) return true + const owner = commentOwner(issue, up, creates, routines) ?? rlsFlagsOwner(issue, up, creates) if (!owner?.sql) return true owner.sql = { ...owner.sql, up: `${owner.sql.up.trimEnd()}\n${up.trim()}` } @@ -599,6 +600,43 @@ export function foldCommentsIntoCreates(issues: DriftIssue[]): DriftIssue[] { }) } +/** + * The finding creating the object a comment is on. dbdiff names a routine by + * its argument types in a comment (`f2(integer,pg_catalog.text)`) and by its + * parameter list in the create (`f2(p_x integer, p_y text DEFAULT 'a')`), so a + * routine is matched by name when only one new routine has it. + */ +function commentOwner( + issue: DriftIssue, up: string, creates: Map, routines: Map, +): DriftIssue | undefined { + const m = COMMENT_TITLE.exec(issue.title) + if (!m || splitStatements(up).length !== 1) return undefined + const [, schema, name, kind] = m + const candidates = schema !== undefined + ? [`schema:${schema}`] + : [`object:${name}`, ...(ON_A_TABLE.has(kind) ? [`object:${name.slice(0, name.lastIndexOf('.'))}`] : [])] + const exact = candidates.map(c => creates.get(c)).find(Boolean) + if (exact || (kind !== 'function' && kind !== 'procedure')) return exact + const sameName = routines.get(routineBaseName(name)) ?? [] + return sameName.length === 1 ? sameName[0] : undefined +} + +/** A routine's name without its argument list. */ +const routineBaseName = (name: string) => name.includes('(') ? name.slice(0, name.indexOf('(')) : name + +/** `ALTER TABLE t ENABLE ROW LEVEL SECURITY` and the like, the whole of a finding. */ +const RLS_FLAG = /^\s*ALTER\s+TABLE\s+(?:ONLY\s+)?\S+\s+(?:ENABLE|DISABLE|(?:NO\s+)?FORCE)\s+ROW\s+LEVEL\s+SECURITY\s*;?\s*$/i + +/** + * The finding creating a table whose RLS flags a finding sets: a table that + * does not exist yet had them as a separate "Table altered". + */ +function rlsFlagsOwner(issue: DriftIssue, up: string, creates: Map): DriftIssue | undefined { + const m = /^Table altered: (.+)$/.exec(issue.title) + if (!m || !splitStatements(up).every(sql => RLS_FLAG.test(sql))) return undefined + return creates.get(`object:${m[1]}`) +} + function unfoldedIssues( result: DbDiffResult, check: 'schema' | 'data', diff --git a/packages/cli/src/restore.ts b/packages/cli/src/restore.ts index f4bea19..8465ad6 100644 --- a/packages/cli/src/restore.ts +++ b/packages/cli/src/restore.ts @@ -288,6 +288,8 @@ export async function restoreFromSnapshot(options: RestoreOptions): Promise undefined) result.rolledBack = result.applied result.applied = [] + delete result.incomplete } // A RestoreAborted carries no message of its own: the statement that failed // has already been recorded, and adding a second entry for the same @@ -812,7 +815,14 @@ export function tolerableFailure( // indexes, keys and rows. Each is left out and named, and the rest is // restored — one missing extension rolled back everything before. if (missing.size > 0) { - return `${errMsg(err)}, as this target lacks ${[...missing].join(', ')}` + // The ones the error is about, when it names any: every missing extension + // listed against every failure said nothing about which one was needed. + const message = errMsg(err) + const named = [...missing].filter(what => { + const name = /^the (.+) (?:extension|schema)$/.exec(what)?.[1] + return name !== undefined && new RegExp(`\\b${name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\b`, 'i').test(message) + }) + return `${message}, as this target lacks ${(named.length > 0 ? named : [...missing]).join(', ')}` } return undefined } diff --git a/packages/cli/src/snapshot.ts b/packages/cli/src/snapshot.ts index 15b2172..22790d2 100644 --- a/packages/cli/src/snapshot.ts +++ b/packages/cli/src/snapshot.ts @@ -683,9 +683,16 @@ async function captureRoleGrants( column_name: string | null; privilege_type: string; is_grantable: boolean }> - const statements = rows.map(row => - `GRANT ${row.privilege_type}` - + (row.column_name ? ` (${quoteName(row.column_name)})` : '') + // One statement per role, object and grant option, listing the + // privileges: one per privilege made a restore that skipped a relation + // list it once for every privilege of every role. + const grouped = new Map() + for (const row of rows) { + const key = JSON.stringify([row.grantee, row.table_schema, row.table_name, row.column_name, Boolean(row.is_grantable)]) + grouped.set(key, [...(grouped.get(key) ?? []), row]) + } + const statements = [...grouped.values()].map(([row, ...rest]) => + `GRANT ${[row, ...rest].map(r => r.privilege_type + (r.column_name ? ` (${quoteName(r.column_name)})` : '')).join(', ')}` + ` ON ${quoteName(row.table_schema)}.${quoteName(row.table_name)}` // PUBLIC is a keyword; quoted, it names a role that does not exist. + ` TO ${row.grantee === 'PUBLIC' ? 'PUBLIC' : quoteName(row.grantee)}` diff --git a/packages/cli/test/dbdiff.test.ts b/packages/cli/test/dbdiff.test.ts index 64bd275..4c83bc1 100644 --- a/packages/cli/test/dbdiff.test.ts +++ b/packages/cli/test/dbdiff.test.ts @@ -1519,3 +1519,40 @@ describe('titles read names with doubled quotes', () => { expect(summariseStatement(sql, 'schema')).toBe(title) }) }) + +describe('folding into the finding that creates an object', () => { + const issue = (id: string, title: string, up: string) => + ({ id, check: 'schema' as const, severity: 'warning' as const, title, description: '', sql: { up, down: '' } }) + + // dbdiff names a function by its argument types in a comment and by its + // full parameter list in the create, so the two never matched. + it('folds a comment on a new function whose signature is written differently', () => { + const issues = foldCommentsIntoCreates([ + issue('f', "Function missing: public.f2(p_x integer, p_y text DEFAULT 'a')", 'CREATE FUNCTION public.f2(p_x integer, p_y text DEFAULT \'a\') RETURNS int LANGUAGE sql AS $$ SELECT 1 $$;'), + issue('c', 'Comment changed: public.f2(integer,pg_catalog.text) (function)', "COMMENT ON FUNCTION public.f2(integer,pg_catalog.text) IS 'f2';"), + ]) + expect(issues.map(i => i.id)).toEqual(['f']) + expect(issues[0].sql?.up).toMatch(/\nCOMMENT ON FUNCTION public\.f2\(integer,pg_catalog\.text\) IS 'f2';$/) + }) + + it('does not guess between overloads of one name', () => { + const issues = foldCommentsIntoCreates([ + issue('a', 'Function missing: public.f(a integer)', 'CREATE FUNCTION public.f(a integer) RETURNS int LANGUAGE sql AS $$ SELECT 1 $$;'), + issue('b', 'Function missing: public.f(a text)', 'CREATE FUNCTION public.f(a text) RETURNS int LANGUAGE sql AS $$ SELECT 1 $$;'), + issue('c', 'Comment changed: public.f(integer) (function)', "COMMENT ON FUNCTION public.f(integer) IS 'f';"), + ]) + expect(issues.map(i => i.id)).toEqual(['a', 'b', 'c']) + }) + + // A table that does not exist yet had its RLS flags as a separate + // "Table altered" finding. + it('folds a new table\'s row level security into its creation', () => { + const issues = foldCommentsIntoCreates([ + issue('t', 'Table missing: public.notes', 'CREATE TABLE public.notes (id int);'), + issue('r', 'Table altered: public.notes', 'ALTER TABLE "notes" ENABLE ROW LEVEL SECURITY;\nALTER TABLE "notes" NO FORCE ROW LEVEL SECURITY;'), + issue('k', 'Table altered: public.kept', 'ALTER TABLE "kept" ENABLE ROW LEVEL SECURITY;'), + ]) + expect(issues.map(i => i.id)).toEqual(['t', 'k']) + expect(issues[0].sql?.up).toBe('CREATE TABLE public.notes (id int);\nALTER TABLE "notes" ENABLE ROW LEVEL SECURITY;\nALTER TABLE "notes" NO FORCE ROW LEVEL SECURITY;') + }) +}) diff --git a/packages/cli/test/e2e/snapshot-restore.test.ts b/packages/cli/test/e2e/snapshot-restore.test.ts index def02c3..3b75af7 100644 --- a/packages/cli/test/e2e/snapshot-restore.test.ts +++ b/packages/cli/test/e2e/snapshot-restore.test.ts @@ -88,6 +88,7 @@ describeE2E('e2e: snapshot restored into an empty database', () => { restored: { dbUrl: h.urlFor('target', restored) }, partial: { dbUrl: h.urlFor('target', 'sf_partial') }, old: { dbUrl: h.urlFor('target', 'sf_old_format') }, + rolledback: { dbUrl: h.urlFor('target', 'sf_rolled_back') }, }, checks: { data: { tables: ['public.features', 'public.plans'] } }, }) @@ -167,6 +168,30 @@ describeE2E('e2e: snapshot restored into an empty database', () => { expect(await h.sqlIn('target', 'sf_old_format', `SELECT string_agg(name, ',' ORDER BY id) FROM public.features`)).toBe('one,two') }, 300_000) + // A restore that rolled back restored nothing, so it is not "incomplete": + // it said both at once. + it('reports a rolled-back restore as rolled back, not as incomplete', async () => { + const snapshots = join(ws, '.supaforge', 'snapshots') + const [taken] = (await readdir(snapshots)).sort() + const copy = '20970101T000000Z' + await cp(join(snapshots, taken), join(snapshots, copy), { recursive: true }) + const manifestFile = join(snapshots, copy, 'manifest.json') + await writeFile(manifestFile, JSON.stringify({ ...JSON.parse(await readFile(manifestFile, 'utf8')), timestamp: copy })) + const schemaFile = join(snapshots, copy, 'schema.sql') + await writeFile(schemaFile, (await readFile(schemaFile, 'utf8')) + ` +CREATE EXTENSION IF NOT EXISTS vector WITH SCHEMA public; +CREATE TABLE public.docs (id int PRIMARY KEY, embedding public.vector(3)); +CREATE TABLE public.orders (id int); +`) + await h.createDatabase('target', 'sf_rolled_back') + const r = await h.cli(['restore', '-e', 'rolledback', '--from-snapshot', copy, '--apply', '--json'], { cwd: ws }) + expect(r.code).toBe(1) + const result = JSON.parse(r.stdout) as { rolledBack?: unknown[]; incomplete?: boolean; errors: unknown[] } + expect(result.errors.length).toBeGreaterThan(0) + expect(result.rolledBack?.length).toBeGreaterThan(0) + expect(result.incomplete).toBeUndefined() + }, 300_000) + // A snapshot needing an extension this server does not ship. Everything // that needs it used to roll the whole restore back with it. it('restores what it can without a missing extension, says what it left out, and exits 1', async () => { diff --git a/packages/cli/test/review-fixes.test.ts b/packages/cli/test/review-fixes.test.ts index 270d599..937cfa4 100644 --- a/packages/cli/test/review-fixes.test.ts +++ b/packages/cli/test/review-fixes.test.ts @@ -182,3 +182,21 @@ describe('destructiveReason: from the parse tree', () => { expect(destructiveReason('DROP TABLE x; THIS IS NOT SQL')).toMatch(/drops a table/) }) }) + +describe('tolerableFailure names what the failure needs', () => { + const plain = new Set(['public', 'pg_catalog', 'information_schema']) + // Every missing extension was listed against every failure. + it('names only the missing extension the error is about', () => { + const lacks = new Set(['the pg_cron extension', 'the pg_net extension', 'the vector extension']) + expect(tolerableFailure('CREATE TABLE public.docs (e extensions.vector(3));', + { code: '42704', message: 'type "extensions.vector" does not exist' }, plain, lacks)) + .toBe('type "extensions.vector" does not exist, as this target lacks the vector extension') + }) + + it('names them all when the error names none of them', () => { + const lacks = new Set(['the pg_net extension', 'the vector extension']) + expect(tolerableFailure('CREATE VIEW public.v AS SELECT * FROM public.docs;', + { code: '42P01', message: 'relation "public.docs" does not exist' }, plain, lacks)) + .toBe('relation "public.docs" does not exist, as this target lacks the pg_net extension, the vector extension') + }) +}) diff --git a/packages/cli/test/snapshot.test.ts b/packages/cli/test/snapshot.test.ts index c773994..ec23ac3 100644 --- a/packages/cli/test/snapshot.test.ts +++ b/packages/cli/test/snapshot.test.ts @@ -482,3 +482,38 @@ describe('captureSnapshot: a policy keeps its comment', () => { expect(sql.indexOf('COMMENT ON POLICY')).toBeGreaterThan(sql.indexOf('CREATE POLICY')) }) }) + +// One GRANT per privilege made a restore that skipped a relation list eight +// lines per role for it. +describe('captureSnapshot: grants are captured one statement per role and object', () => { + let dir: string + beforeEach(async () => { dir = await mkdtemp(join(tmpdir(), 'supaforge-snap-')) }) + afterEach(async () => { await rm(dir, { recursive: true, force: true }) }) + + it('groups a role\'s privileges on a table, and keeps grant options and columns apart', async () => { + const g = (privilege_type: string, extra: Record = {}) => + ({ grantee: 'anon', table_schema: 'public', table_name: 'items', column_name: null, privilege_type, is_grantable: false, ...extra }) + const rows = [ + g('INSERT'), g('SELECT'), g('UPDATE'), + g('DELETE', { is_grantable: true }), + g('UPDATE', { column_name: 'name' }), + g('SELECT', { grantee: 'PUBLIC' }), + ] + const queryFn = (async (_url: string, sql: string) => sql.includes('aclexplode') ? rows : []) as unknown as QueryFn + const result = await captureSnapshot({ + envName: 'prod', + env: { dbUrl: 'postgres://example' }, + config: { environments: { prod: { dbUrl: 'postgres://example' } } } as never, + outputDir: dir, + queryFn, + fetchFn: (async () => new Response('[]', { status: 200 })) as never, + }) + const grants = (await readFile(join(result.dir, 'roles.sql'), 'utf8')).split('\n').filter(l => l.startsWith('GRANT')) + expect(grants.sort()).toEqual([ + 'GRANT DELETE ON "public"."items" TO "anon" WITH GRANT OPTION;', + 'GRANT INSERT, SELECT, UPDATE ON "public"."items" TO "anon";', + 'GRANT SELECT ON "public"."items" TO PUBLIC;', + 'GRANT UPDATE ("name") ON "public"."items" TO "anon";', + ]) + }) +}) From 4d51f4091c8d578b9d02671b88a6324bfa6c6e85 Mon Sep 17 00:00:00 2001 From: Jasdeep Date: Wed, 7 Oct 2026 22:12:17 +0000 Subject: [PATCH 07/15] test(scenarios): judge a cross-version target against the source built on its server PostgreSQL prints the same objects differently from one major version to the next (15 table-qualifies a view's columns, 16+ does not), so a migrated target never matched a source on another version however right it was. Across major versions the runner now builds the source's SQL on the target's server and compares the target with that, as --prove does. Co-Authored-By: Claude Opus 5.5 --- packages/cli/test/scenarios/runner.ts | 33 ++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/packages/cli/test/scenarios/runner.ts b/packages/cli/test/scenarios/runner.ts index 9f6d2ac..6292b11 100644 --- a/packages/cli/test/scenarios/runner.ts +++ b/packages/cli/test/scenarios/runner.ts @@ -21,6 +21,7 @@ * - **Nothing leaks.** No command touches the servers' roles or databases. */ import { diffState } from '../../src/state-diff.js' +import type { SchemaState } from '@akalforge/pg-conformance' import { DEFAULT_IGNORE_SCHEMAS } from '../../src/defaults.js' import type { PromoteResult } from '../../src/promote.js' import { isComparisonCheck, type ScanResult } from '../../src/types/drift.js' @@ -76,6 +77,36 @@ async function userSchemas(h: PgHarness, src: string, tgt: string): Promise>() +function differentMajors(h: PgHarness): Promise { + if (!majors.has(h)) { + const major = async (role: 'source' | 'target') => Math.floor(Number(await h.sql(role, 'SHOW server_version_num')) / 10000) + majors.set(h, Promise.all([major('source'), major('target')]).then(([a, b]) => a !== b)) + } + return majors.get(h)! +} + +/** + * What the target should hold once migrated: the source's state — or, across + * major versions, the source's SQL built on the target's server. PostgreSQL + * prints the same objects differently from one major version to the next (15 + * table-qualifies a view's columns, 16+ does not), so a migrated target never + * matched a source on another version however right it was. Built on the + * target's server, like is compared with like, as --prove does. + */ +async function expectedState(h: PgHarness, key: string, s: Scenario, src: string, schemas: string[]): Promise { + if (!await differentMajors(h)) return h.stateIn('source', src, schemas) + const ref = `sc_${key}_r` + await h.createDatabase('target', ref) + try { + await h.applySqlIn('target', ref, s.source) + return await h.stateIn('target', ref, schemas) + } finally { + await h.dropDatabase('target', ref) + } +} + async function rows(h: PgHarness, role: 'source' | 'target', db: string, queries: string[]): Promise { return Promise.all(queries.map(q => h.sqlIn(role, db, q))) } @@ -152,7 +183,7 @@ export async function runScenario(h: PgHarness, key: string, s: Scenario): Promi violations.push(`--prove did not run:\n${short(all)}`) } - outcome.residual = diffState(await h.stateIn('source', src, schemas), await h.stateIn('target', tgt, schemas)) + outcome.residual = diffState(await expectedState(h, key, s, src, schemas), await h.stateIn('target', tgt, schemas)) if (outcome.residual.length > 0) { violations.push(`the target does not match the source:\n ${outcome.residual.join('\n ')}`) } From abbd47ee55b793f85d940f8843d61b3a9d8d788d Mon Sep 17 00:00:00 2001 From: Jasdeep Date: Wed, 7 Oct 2026 22:28:13 +0000 Subject: [PATCH 08/15] fix(apply): key policies and triggers by their names as PostgreSQL reads them A policy name with a double quote in it is written with it doubled. Matched as text, the schema check's CREATE POLICY and the RLS check's for the same policy got different keys, so both were applied and the second failed with 'policy ... already exists'. Created and dropped policies and created triggers are now keyed from the parse tree, and the text fallback reads doubled quotes as part of one name. Co-Authored-By: Claude Opus 5.5 --- packages/cli/src/sql-deps.ts | 45 ++++++++++++++++++++++++------ packages/cli/test/sql-deps.test.ts | 20 +++++++++++++ 2 files changed, 57 insertions(+), 8 deletions(-) diff --git a/packages/cli/src/sql-deps.ts b/packages/cli/src/sql-deps.ts index 45f7f0f..9e59e90 100644 --- a/packages/cli/src/sql-deps.ts +++ b/packages/cli/src/sql-deps.ts @@ -170,23 +170,35 @@ export function sqlSkeleton(sql: string): string { // ─── Identifiers ───────────────────────────────────────────────────────────── +/** One name, quoted (its own quotes doubled) or not. */ +const NAME_PART = String.raw`"(?:[^"]|"")+"|[\w$]+` + /** An optionally schema-qualified, optionally quoted identifier. */ -const IDENT = String.raw`(?:"[^"]+"|[\w$]+)(?:\s*\.\s*(?:"[^"]+"|[\w$]+))?` +const IDENT = String.raw`(?:${NAME_PART})(?:\s*\.\s*(?:${NAME_PART}))?` /** Strip quoting and any schema qualifier, leaving a comparable bare name. */ export function bareName(identifier: string): string { - const last = identifier.split('.').pop() ?? identifier - return last.trim().replace(/^"|"$/g, '').toLowerCase() + const parts = identifier.match(new RegExp(NAME_PART, 'g')) ?? [identifier] + return unquote(parts[parts.length - 1]).toLowerCase() +} +/** A quoted identifier as the name it is, its doubled quotes made single. */ +const unquote = (part: string) => { + const p = part.trim() + return p.startsWith('"') ? p.slice(1, -1).replace(/""/g, '"') : p } +/** `schema.table.object`, lowercased, `public` where no schema is named: how policies are keyed. */ +const objectKey = (schema: string | undefined, table: string | undefined, name: string | undefined) => + `${(schema || 'public').toLowerCase()}.${(table ?? '').toLowerCase()}.${(name ?? '').toLowerCase()}` + /** * A relation's name with its schema, `public` where it names none: * `"app"."accounts"` → `app.accounts`, `users` → `public.users`. Folded the * way bareName folds, so the same relation written either way agrees. */ export function schemaQualified(identifier: string): string { - const parts = identifier.match(/"[^"]+"|[A-Za-z_][\w$]*/g) ?? [identifier] - const fold = (part: string) => part.trim().replace(/^"|"$/g, '').toLowerCase() + const parts = identifier.match(new RegExp(NAME_PART, 'g')) ?? [identifier] + const fold = (part: string) => unquote(part).toLowerCase() return `${parts.length > 1 ? fold(parts[0]) : 'public'}.${fold(parts[parts.length - 1])}` } @@ -281,8 +293,13 @@ const NOT_A_TABLE = new Set([ * qualifier is dropped for the same reason `referencedTables` drops it. */ export function createdPolicies(sql: string): string[] { + const tree = parseStatements(sql) + if (tree) { + return tree.filter(s => s.kind === 'CreatePolicyStmt') + .map(({ node }) => objectKey(node.table?.schemaname, node.table?.relname, node.policy_name)) + } const out: string[] = [] - const re = /CREATE\s+POLICY\s+("[^"]+"|[A-Za-z_][\w$]*)\s+ON\s+((?:"[^"]+"|[A-Za-z_][\w$]*)(?:\s*\.\s*(?:"[^"]+"|[A-Za-z_][\w$]*))?)/gi + const re = new RegExp(String.raw`CREATE\s+POLICY\s+(${NAME_PART})\s+ON\s+(${IDENT})`, 'gi') for (const m of sqlSkeleton(sql).matchAll(re)) { out.push(`${schemaQualified(m[2])}.${bareName(m[1])}`) } @@ -306,8 +323,15 @@ export function createdPolicies(sql: string): string[] { * same name, and that is not a loss of access control (issue #88). */ export function droppedPolicies(sql: string): string[] { + const tree = parseStatements(sql) + if (tree) { + return tree.filter(s => s.kind === 'DropStmt' && s.node.removeType === 'OBJECT_POLICY') + .flatMap(({ node }) => (node.objects as Array>) + .map(o => (o.List?.items ?? []).map((i: Record) => i.String?.sval as string)) + .map((parts: string[]) => objectKey(parts.at(-3), parts.at(-2), parts.at(-1)))) + } const out: string[] = [] - const re = /DROP\s+POLICY\s+(?:IF\s+EXISTS\s+)?("[^"]+"|[A-Za-z_][\w$]*)\s+ON\s+((?:"[^"]+"|[A-Za-z_][\w$]*)(?:\s*\.\s*(?:"[^"]+"|[A-Za-z_][\w$]*))?)/gi + const re = new RegExp(String.raw`DROP\s+POLICY\s+(?:IF\s+EXISTS\s+)?(${NAME_PART})\s+ON\s+(${IDENT})`, 'gi') for (const m of sqlSkeleton(sql).matchAll(re)) { out.push(`${schemaQualified(m[2])}.${bareName(m[1])}`) } @@ -351,8 +375,13 @@ export function createsOnlyPolicies(sql: string): boolean { * drops it: the two layers spell the same table differently. */ export function createdTriggers(sql: string): string[] { + const tree = parseStatements(sql) + if (tree) { + return tree.filter(s => s.kind === 'CreateTrigStmt') + .map(({ node }) => `${(node.relation?.relname ?? '').toLowerCase()}.${String(node.trigname).toLowerCase()}`) + } const out: string[] = [] - const re = /CREATE\s+(?:OR\s+REPLACE\s+)?(?:CONSTRAINT\s+)?TRIGGER\s+("[^"]+"|[A-Za-z_][\w$]*)[\s\S]*?\sON\s+((?:"[^"]+"|[A-Za-z_][\w$]*)(?:\s*\.\s*(?:"[^"]+"|[A-Za-z_][\w$]*))?)/gi + const re = new RegExp(String.raw`CREATE\s+(?:OR\s+REPLACE\s+)?(?:CONSTRAINT\s+)?TRIGGER\s+(${NAME_PART})[\s\S]*?\sON\s+(${IDENT})`, 'gi') for (const m of sqlSkeleton(sql).matchAll(re)) { out.push(`${bareName(m[2])}.${bareName(m[1])}`) } diff --git a/packages/cli/test/sql-deps.test.ts b/packages/cli/test/sql-deps.test.ts index 420cf13..b4a72cb 100644 --- a/packages/cli/test/sql-deps.test.ts +++ b/packages/cli/test/sql-deps.test.ts @@ -9,6 +9,8 @@ import { orderStatements, createdTriggers, createsOnlyTriggers, + createdPolicies, + droppedPolicies, } from '../src/sql-deps.js' /** @@ -476,3 +478,21 @@ describe('statementPhase: comments', () => { }) }) + +// A name with a double quote in it is written with it doubled, and read as +// two names the schema check's policy and the RLS check's never matched: both +// were applied, and the second failed with `policy ... already exists`. +describe('created and dropped objects, by names with doubled quotes', () => { + it('keys a policy the same however it is written', () => { + const dbdiff = 'CREATE POLICY "say ""hi"" there" ON "q1" FOR SELECT USING (true);' + const rls = 'CREATE POLICY "say ""hi"" there"\n ON "public"."q1"\n AS PERMISSIVE\n FOR SELECT\n TO PUBLIC\n USING (true)\n;' + expect(createdPolicies(dbdiff)).toEqual(['public.q1.say "hi" there']) + expect(createdPolicies(rls)).toEqual(createdPolicies(dbdiff)) + expect(droppedPolicies('DROP POLICY IF EXISTS "say ""hi"" there" ON "public"."q1";')).toEqual(['public.q1.say "hi" there']) + }) + + it('keys a trigger by its table, with doubled quotes read as one', () => { + expect(createdTriggers('CREATE TRIGGER "t ""x""" AFTER INSERT ON "Odd ""t""" FOR EACH ROW EXECUTE FUNCTION f();')) + .toEqual(['odd "t".t "x"']) + }) +}) From 52acd36745f5e3a141558126f414b0717b4fc5e0 Mon Sep 17 00:00:00 2001 From: Jasdeep Date: Thu, 8 Oct 2026 07:34:41 +0000 Subject: [PATCH 09/15] fix(diff): a recreated object's comments go with its modified finding dbdiff sets the comments of a view, function, policy or trigger it recreates again. Apart from the 'modified' finding they read as 'Comment changed' on a comment that had not. The grants e2e now covers a new locked-down view as well as tables. Co-Authored-By: Claude Opus 5.5 --- packages/cli/src/dbdiff.ts | 9 +++++++-- packages/cli/test/dbdiff.test.ts | 20 +++++++++++++++++++ .../cli/test/e2e/grants-on-new-tables.test.ts | 7 ++++++- 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/dbdiff.ts b/packages/cli/src/dbdiff.ts index 6b982f8..2964451 100644 --- a/packages/cli/src/dbdiff.ts +++ b/packages/cli/src/dbdiff.ts @@ -560,8 +560,13 @@ export function sqlToIssues( /** What a comment title names: `public.orders.total (column)` → the name and kind. */ const COMMENT_TITLE = /^(?:Comment changed|Comment missing): (?:schema (.+)|(.+) \(([a-z ]+)\))$/ -/** A finding that creates an object: `Table missing: public.orders`. */ -const CREATE_TITLE = /^(Schema|Table|View|Function|Procedure|Type|Domain|Sequence|Index|Policy|Trigger|Extension) missing: (.+)$/ +/** + * A finding that creates an object, `Table missing: public.orders`, or + * recreates one, `View modified: public.v`: dbdiff sets a recreated object's + * comments again, and apart those read as "Comment changed" on a comment that + * had not. + */ +const CREATE_TITLE = /^(Schema|Table|View|Function|Procedure|Type|Domain|Sequence|Index|Policy|Trigger|Extension) (?:missing|modified): (.+)$/ /** Kinds named after the table (or domain) they belong to. */ const ON_A_TABLE = new Set(['column', 'constraint', 'trigger', 'policy', 'rule']) diff --git a/packages/cli/test/dbdiff.test.ts b/packages/cli/test/dbdiff.test.ts index 4c83bc1..f19a33c 100644 --- a/packages/cli/test/dbdiff.test.ts +++ b/packages/cli/test/dbdiff.test.ts @@ -1556,3 +1556,23 @@ describe('folding into the finding that creates an object', () => { expect(issues[0].sql?.up).toBe('CREATE TABLE public.notes (id int);\nALTER TABLE "notes" ENABLE ROW LEVEL SECURITY;\nALTER TABLE "notes" NO FORCE ROW LEVEL SECURITY;') }) }) + +// A view, function, policy or trigger dbdiff recreates gets its comments set +// again. Apart, those read as "Comment changed" on a comment that had not. +describe('a recreated object\'s comments go with it', () => { + const units = (...parts: Array<[string, string, string]>) => + parts.map(([kind, object, sql]) => `-- dbdiff:unit ${kind} ${object}\n${sql}\n-- dbdiff:end`).join('\n') + + it('folds them into the modified finding', () => { + const issues = sqlToIssues({ + up: units( + ['AlterView', 'vt_v', 'DROP VIEW IF EXISTS "vt_v";\nCREATE VIEW "vt_v" AS SELECT 1 AS id;'], + ['AlterComment', 'vt_v', `COMMENT ON VIEW public.vt_v IS 'v';`], + ['AlterComment', 'vt_v.id', `COMMENT ON COLUMN public.vt_v.id IS 'vn';`], + ), + down: '', + }, 'schema') + expect(issues.map(i => i.title)).toEqual(['View modified: public.vt_v']) + expect(issues[0].sql?.up).toMatch(/CREATE VIEW[\s\S]*COMMENT ON VIEW public\.vt_v IS 'v';\nCOMMENT ON COLUMN public\.vt_v\.id IS 'vn';$/) + }) +}) diff --git a/packages/cli/test/e2e/grants-on-new-tables.test.ts b/packages/cli/test/e2e/grants-on-new-tables.test.ts index 8000061..5027866 100644 --- a/packages/cli/test/e2e/grants-on-new-tables.test.ts +++ b/packages/cli/test/e2e/grants-on-new-tables.test.ts @@ -28,6 +28,9 @@ const SOURCE = ` CREATE TABLE public.ro_items (id int PRIMARY KEY); REVOKE INSERT, UPDATE, DELETE, TRUNCATE ON public.ro_items FROM anon; CREATE TABLE public.open_items (id int PRIMARY KEY); + -- Views are tables to privileges, and get the same defaults. + CREATE VIEW public.private_v AS SELECT id FROM public.private_notes; + REVOKE ALL ON public.private_v FROM anon, authenticated; ` /** Each table's grants to the two API roles, as one comparable line per grant. */ @@ -35,7 +38,7 @@ const GRANTS = ` SELECT coalesce(string_agg(format('%s %s %s', c.relname, a.grantee::regrole, a.privilege_type), E'\\n' ORDER BY c.relname, a.grantee::regrole::text, a.privilege_type), '') FROM pg_class c CROSS JOIN LATERAL aclexplode(c.relacl) a - WHERE c.relnamespace = 'public'::regnamespace AND c.relname IN ('private_notes', 'ro_items', 'open_items') + WHERE c.relnamespace = 'public'::regnamespace AND c.relname IN ('private_notes', 'ro_items', 'open_items', 'private_v') AND a.grantee IN ('anon'::regrole, 'authenticated'::regrole) ` @@ -59,8 +62,10 @@ describeE2E('e2e: a new table keeps the source\'s grants', () => { const issues = scan.checks.flatMap(c => c.issues) expect(issues.map(i => i.id).sort()).toEqual([ 'roles-grant-default-anon.public.private_notes', + 'roles-grant-default-anon.public.private_v', 'roles-grant-default-anon.public.ro_items', 'roles-grant-default-authenticated.public.private_notes', + 'roles-grant-default-authenticated.public.private_v', ]) expect(issues.every(i => i.severity === 'critical')).toBe(true) }, 120_000) From 68b44e9255ec5d8015db48b1da4db5adb67da544 Mon Sep 17 00:00:00 2001 From: Jasdeep Date: Thu, 8 Oct 2026 07:50:44 +0000 Subject: [PATCH 10/15] test: run the suites against release candidates of all three packages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit scripts/test-release-candidates.sh builds DBDiff's PHAR and static binary from a checkout the way a release does, packs @dbdiff/cli, @dbdiff/cli-linux-x64 and pg-conformance, builds and packs SupaForge against them and installs it as a user would. It then runs the unit, integration, database e2e and scenario suites (twin servers and 17→15) against that installed package. SUPAFORGE_E2E_CLI points the harness at it. A fix spanning two repos can then be tested together before either is released. Co-Authored-By: Claude Opus 5.5 --- .../cli/scripts/test-release-candidates.sh | 189 ++++++++++++++++++ packages/cli/test/harness/PgHarness.ts | 5 +- 2 files changed, 193 insertions(+), 1 deletion(-) create mode 100755 packages/cli/scripts/test-release-candidates.sh diff --git a/packages/cli/scripts/test-release-candidates.sh b/packages/cli/scripts/test-release-candidates.sh new file mode 100755 index 0000000..8c24c79 --- /dev/null +++ b/packages/cli/scripts/test-release-candidates.sh @@ -0,0 +1,189 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Test SupaForge, DBDiff and pg-conformance together, as they would be +# released, before any of them is. +# +# Each repo's own CI tests it against the others' *published* versions, so a +# fix that needs a change in two of them is only ever tested together after +# both are released. This builds the release artifacts locally from the +# checkouts' current commits and runs SupaForge's suites through them: +# +# 1. DBDiff: the PHAR and the static linux-x64 binary, built the way the +# release workflow builds them (scripts/build-local.sh: Podman or Docker, +# no PHP needed; the first build takes 30-60 min, later ones minutes). +# 2. The npm packages @dbdiff/cli and @dbdiff/cli-linux-x64, versioned +# together and packed, and @akalforge/pg-conformance packed. +# 3. SupaForge built and packed against them, then installed into a scratch +# prefix the way a user installs it. +# 4. SupaForge's suites run against that installed package: unit and lint, +# integration, the database e2e suite and the scenario suites (twin +# servers, and PostgreSQL 17 → 15) drive its bin/run.js and its own +# @dbdiff/cli binary, not this checkout. +# +# Only committed work is tested: each checkout's HEAD is cloned. Linux x64. +# +# Usage: +# ./scripts/test-release-candidates.sh [--dbdiff PATH] [--pg-conformance PATH] +# [--work DIR] [--suites LIST] [--skip-build] +# +# --dbdiff PATH DBDiff checkout (default: ../../../DBDiff from here) +# --pg-conformance PATH pg-conformance checkout (default: the version in package.json) +# --work DIR Where to build (default: a new temp directory) +# --suites LIST Comma-separated: unit,integration,e2e,scenarios,cross-version +# (default: all) +# --skip-build Reuse the DBDiff artifacts already in --work +# +# Exit status is 0 only when every suite run passes. + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +CLI_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" +REPO_DIR="$(cd "$CLI_DIR/../.." && pwd)" + +DBDIFF_SRC="$(cd "$REPO_DIR/.." && pwd)/DBDiff" +PGC_SRC="" +WORK="" +SUITES="unit,integration,e2e,scenarios,cross-version" +SKIP_BUILD=false +while [ $# -gt 0 ]; do + case "$1" in + --dbdiff) DBDIFF_SRC="$2"; shift 2 ;; + --pg-conformance) PGC_SRC="$2"; shift 2 ;; + --work) WORK="$2"; shift 2 ;; + --suites) SUITES="$2"; shift 2 ;; + --skip-build) SKIP_BUILD=true; shift ;; + *) echo "unknown argument: $1" >&2; exit 2 ;; + esac +done +WORK="${WORK:-$(mktemp -d -t supaforge-rc-XXXXXX)}" +mkdir -p "$WORK/tgz" +WORK="$(cd "$WORK" && pwd)" +wants() { [[ ",$SUITES," == *",$1,"* ]]; } + +RT="" +for c in podman docker; do command -v "$c" >/dev/null 2>&1 && { RT="$c"; break; }; done +[ -n "$RT" ] || { echo "Podman or Docker is needed." >&2; exit 1; } + +say() { printf '\n\033[1m== %s\033[0m\n' "$*"; } +RESULTS=() +record() { RESULTS+=("$1"); } + +# ── 1. DBDiff: release artifacts from the checkout's HEAD ──────────────────── +DBDIFF_SHA="$(git -C "$DBDIFF_SRC" rev-parse --short HEAD)" +VERSION="0.0.0-local.$DBDIFF_SHA" +if ! $SKIP_BUILD; then + say "DBDiff $DBDIFF_SHA ($(git -C "$DBDIFF_SRC" rev-parse --abbrev-ref HEAD)): PHAR and static linux-x64 binary" + rm -rf "$WORK/dbdiff" + git clone -q --local "$DBDIFF_SRC" "$WORK/dbdiff" + git -C "$WORK/dbdiff" checkout -q "$(git -C "$DBDIFF_SRC" rev-parse HEAD)" + (cd "$WORK/dbdiff" && bash scripts/build-local.sh) +fi +BIN="$WORK/dbdiff/packages/@dbdiff/cli-linux-x64/dbdiff" +[ -x "$BIN" ] && [ -f "$WORK/dbdiff/dist/dbdiff.phar" ] || { echo "DBDiff artifacts missing in $WORK/dbdiff" >&2; exit 1; } +say "DBDiff binary: $("$BIN" --version)" + +# ── 2. npm packages, versioned together as a release stamps them ───────────── +say "Packing @dbdiff/cli $VERSION, @dbdiff/cli-linux-x64 $VERSION" +rm -f "$WORK"/tgz/*.tgz +cp "$WORK/dbdiff/dist/dbdiff.phar" "$WORK/dbdiff/packages/@dbdiff/cli/dbdiff.phar" +for pkg in cli cli-linux-x64; do + node -e ' + const fs = require("fs"), [file, v] = process.argv.slice(1) + const j = JSON.parse(fs.readFileSync(file, "utf8")) + j.version = v + for (const k of Object.keys(j.optionalDependencies ?? {})) j.optionalDependencies[k] = v + fs.writeFileSync(file, JSON.stringify(j, null, 2) + "\n") + ' "$WORK/dbdiff/packages/@dbdiff/$pkg/package.json" "$VERSION" + (cd "$WORK/dbdiff/packages/@dbdiff/$pkg" && npm pack --silent --pack-destination "$WORK/tgz" >/dev/null) +done +if [ -n "$PGC_SRC" ]; then + say "Packing @akalforge/pg-conformance from $PGC_SRC ($(git -C "$PGC_SRC" rev-parse --short HEAD))" + rm -rf "$WORK/pg-conformance" + git clone -q --local "$PGC_SRC" "$WORK/pg-conformance" + git -C "$WORK/pg-conformance" checkout -q "$(git -C "$PGC_SRC" rev-parse HEAD)" + (cd "$WORK/pg-conformance" && npm pack --silent --pack-destination "$WORK/tgz" >/dev/null) +fi +DEPS=("$WORK"/tgz/dbdiff-cli-*.tgz) +[ -n "$PGC_SRC" ] && DEPS+=("$WORK"/tgz/akalforge-pg-conformance-*.tgz) + +# ── 3. SupaForge built against them, packed and installed ─────────────────── +say "SupaForge $(git -C "$REPO_DIR" rev-parse --short HEAD) ($(git -C "$REPO_DIR" rev-parse --abbrev-ref HEAD)), built against those" +rm -rf "$WORK/supaforge" +git clone -q --local "$REPO_DIR" "$WORK/supaforge" +git -C "$WORK/supaforge" checkout -q "$(git -C "$REPO_DIR" rev-parse HEAD)" +SF="$WORK/supaforge/packages/cli" +# Pinned to the candidates as a release bump would pin them, so the packed +# package depends on these versions and not on local paths. +PGC_VERSION="$( [ -n "$PGC_SRC" ] && node -p 'require(process.argv[1]).version' "$WORK/pg-conformance/package.json" || true)" +(cd "$SF" && npm ci --silent --no-audit --no-fund) +node -e ' + const fs = require("fs"), [file, dbdiff, pgc] = process.argv.slice(1) + const j = JSON.parse(fs.readFileSync(file, "utf8")) + j.dependencies["@dbdiff/cli"] = dbdiff + if (pgc) j.dependencies["@akalforge/pg-conformance"] = pgc + fs.writeFileSync(file, JSON.stringify(j, null, 2) + "\n") +' "$SF/package.json" "$VERSION" "$PGC_VERSION" +(cd "$SF" && npm install --silent --no-audit --no-fund --no-save "${DEPS[@]}" && npm run build --silent) +(cd "$SF" && npm pack --silent --pack-destination "$WORK/tgz" >/dev/null) +rm -rf "$WORK/prefix" && mkdir -p "$WORK/prefix" +(cd "$WORK/prefix" && npm init -y >/dev/null && npm install --silent --no-audit --no-fund "$WORK"/tgz/akalforge-supaforge-*.tgz "${DEPS[@]}") +INSTALLED="$WORK/prefix/node_modules/@akalforge/supaforge" +say "Installed: $(node "$INSTALLED/bin/run.js" --version) with $("$WORK/prefix/node_modules/.bin/dbdiff" --version)" +export SUPAFORGE_E2E_CLI="$INSTALLED/bin/run.js" +cd "$SF" + +# A proof across versions copies the source with pg_dump, which must be at +# least as new as the source server. Use PostgreSQL 18's client tools from a +# container when the host's are older. +if wants scenarios || wants cross-version; then + if [ "$(pg_dump --version 2>/dev/null | grep -oE '[0-9]+' | head -1 || echo 0)" -lt 18 ]; then + mkdir -p "$WORK/pgbin" + for t in pg_dump pg_restore psql; do + printf '#!/bin/sh\nexec %s run --rm -i --network=host -v /tmp:/tmp docker.io/library/postgres:18 %s "$@"\n' "$RT" "$t" > "$WORK/pgbin/$t" + chmod +x "$WORK/pgbin/$t" + done + export PATH="$WORK/pgbin:$PATH" + fi +fi + +run_suite() { + local name=$1; shift + say "$name" + if "$@"; then record "PASS $name"; else record "FAIL $name"; fi +} + +if wants unit; then + run_suite "unit" npx vitest run + run_suite "lint" npm run lint --silent +fi + +if wants integration || wants e2e; then + trap '$RT rm -f sf-rc-source sf-rc-target >/dev/null 2>&1 || true' EXIT + for p in 15432:source 15433:target; do + $RT rm -f "sf-rc-${p##*:}" >/dev/null 2>&1 || true + $RT run -d --rm --name "sf-rc-${p##*:}" --network=host -e POSTGRES_PASSWORD="${p##*:}-test-pass" \ + docker.io/library/postgres:15 -c listen_addresses=127.0.0.1 -c port="${p%%:*}" >/dev/null + done + until $RT exec sf-rc-source pg_isready -p 15432 -q 2>/dev/null && $RT exec sf-rc-target pg_isready -p 15433 -q 2>/dev/null; do sleep 1; done + export SUPAFORGE_TEST_SOURCE_URL=postgresql://postgres:source-test-pass@127.0.0.1:15432/postgres + export SUPAFORGE_TEST_TARGET_URL=postgresql://postgres:target-test-pass@127.0.0.1:15433/postgres + seed() { + $RT exec -i -e PGPASSWORD=source-test-pass sf-rc-source psql -h 127.0.0.1 -p 15432 -U postgres -q -v ON_ERROR_STOP=1 < tests/fixtures/seed-source.sql >/dev/null 2>&1 + $RT exec -i -e PGPASSWORD=target-test-pass sf-rc-target psql -h 127.0.0.1 -p 15433 -U postgres -q -v ON_ERROR_STOP=1 < tests/fixtures/seed-target.sql >/dev/null 2>&1 + } + if wants integration; then seed; run_suite "integration" npx vitest run --config vitest.integration.config.ts; fi + if wants e2e; then seed; run_suite "database e2e" npx vitest run test/e2e; fi +fi + +if wants scenarios; then + run_suite "scenarios (twin servers)" env SCENARIO_REQUIRE_PROOF=1 npx vitest run -c vitest.scenarios.config.ts +fi +if wants cross-version; then + run_suite "scenarios (PostgreSQL 17 → 15)" env SCENARIO_REQUIRE_PROOF=1 \ + SCENARIO_SOURCE_IMAGE=postgres:17 SCENARIO_TARGET_IMAGE=postgres:15 npx vitest run -c vitest.scenarios.config.ts +fi + +say "Release candidates: SupaForge $(git -C "$REPO_DIR" rev-parse --short HEAD), DBDiff $DBDIFF_SHA${PGC_SRC:+, pg-conformance $(git -C "$PGC_SRC" rev-parse --short HEAD)}" +printf '%s\n' "${RESULTS[@]}" +printf '\nArtifacts in %s\n' "$WORK" +! printf '%s\n' "${RESULTS[@]}" | grep -q '^FAIL' diff --git a/packages/cli/test/harness/PgHarness.ts b/packages/cli/test/harness/PgHarness.ts index 5d03837..a65b224 100644 --- a/packages/cli/test/harness/PgHarness.ts +++ b/packages/cli/test/harness/PgHarness.ts @@ -98,7 +98,10 @@ export class PgHarness { this.keep = opts.keep ?? false; this.readyTimeoutSec = opts.readyTimeoutSec ?? 60; this.verbose = opts.verbose ?? false; - this.cliEntryOverride = opts.cliEntry; + // SUPAFORGE_E2E_CLI points the e2e suites at an installed package's + // bin/run.js instead of this checkout's — how scripts/test-release-candidates.sh + // tests the packed artifact a release would publish. + this.cliEntryOverride = opts.cliEntry ?? (process.env.SUPAFORGE_E2E_CLI || undefined); this.runId = `sf-${process.pid}-${Math.random().toString(36).slice(2, 7)}`; } From 1513ca4d518498ab3dfd64cc2a54f4ed61165d17 Mon Sep 17 00:00:00 2001 From: Jasdeep Date: Thu, 8 Oct 2026 08:12:21 +0000 Subject: [PATCH 11/15] fix: quote RLS and extension names, and no TypeScript warning for users Found by running the scenario suites against packed release candidates: - The RLS check's ENABLE/DISABLE ROW LEVEL SECURITY and the extensions check's CREATE, DROP and ALTER EXTENSION wrote names between plain quotes, and an extension version unescaped. - Under NODE_ENV=development or test, as in many projects running npx supaforge, the published package warned on every command that it could not find TypeScript. The compiled bin turns oclif's TypeScript lookup off. Co-Authored-By: Claude Opus 5.5 --- packages/cli/bin/run.js | 7 ++++++- packages/cli/src/checks/extensions.ts | 9 +++++---- packages/cli/src/checks/rls.ts | 9 +++++---- packages/cli/test/quoted-names.test.ts | 22 +++++++++++++++++++++- 4 files changed, 37 insertions(+), 10 deletions(-) diff --git a/packages/cli/bin/run.js b/packages/cli/bin/run.js index 76bb0bc..37a9483 100755 --- a/packages/cli/bin/run.js +++ b/packages/cli/bin/run.js @@ -1,5 +1,10 @@ #!/usr/bin/env node -import {execute} from '@oclif/core' +import {execute, settings} from '@oclif/core' + +// The published package is compiled. Left on, oclif looks for TypeScript +// whenever NODE_ENV is development or test — as in many projects running +// npx supaforge — and warned on every command that it could not find it. +settings.enableAutoTranspile = false await execute({dir: import.meta.url}).catch((err) => { // oclif handles CLIError internally — this catches truly unexpected errors. diff --git a/packages/cli/src/checks/extensions.ts b/packages/cli/src/checks/extensions.ts index 897068d..055bab9 100644 --- a/packages/cli/src/checks/extensions.ts +++ b/packages/cli/src/checks/extensions.ts @@ -2,6 +2,7 @@ import type { QueryFn } from '../db' import { pgQuery } from '../db' import type { DriftIssue } from '../types/drift' import { Check, type CheckContext } from './base' +import { quoteLiteral, quoteName } from '../utils/sql.js' interface PgExtension { name: string @@ -54,8 +55,8 @@ export function diffExtensions(source: PgExtension[], target: PgExtension[]): Dr description: `Extension "${name}" (v${ext.version}) is enabled in source (schema: ${ext.schema}) but not in target.`, sourceValue: ext, sql: { - up: `CREATE EXTENSION IF NOT EXISTS "${name}"${ext.schema !== 'public' ? ` SCHEMA "${ext.schema}"` : ''};`, - down: `DROP EXTENSION IF EXISTS "${name}";`, + up: `CREATE EXTENSION IF NOT EXISTS ${quoteName(name)}${ext.schema !== 'public' ? ` SCHEMA ${quoteName(ext.schema)}` : ''};`, + down: `DROP EXTENSION IF EXISTS ${quoteName(name)};`, }, }) } @@ -90,8 +91,8 @@ export function diffExtensions(source: PgExtension[], target: PgExtension[]): Dr sourceValue: srcExt, targetValue: tgtExt, sql: { - up: `ALTER EXTENSION "${name}" UPDATE TO '${srcExt.version}';`, - down: `ALTER EXTENSION "${name}" UPDATE TO '${tgtExt.version}';`, + up: `ALTER EXTENSION ${quoteName(name)} UPDATE TO ${quoteLiteral(srcExt.version)};`, + down: `ALTER EXTENSION ${quoteName(name)} UPDATE TO ${quoteLiteral(tgtExt.version)};`, }, }) } diff --git a/packages/cli/src/checks/rls.ts b/packages/cli/src/checks/rls.ts index f635c21..31347c9 100644 --- a/packages/cli/src/checks/rls.ts +++ b/packages/cli/src/checks/rls.ts @@ -1,4 +1,5 @@ import { createPolicySql, dropPolicySql } from '../utils/schema-policies.js' +import { quoteName } from '../utils/sql.js' import { dropEquivalentPolicyChanges, defaultCanonicalizer } from '../utils/policy-equivalence.js' import type { QueryFn } from '../db' import { pgQuery } from '../db' @@ -205,8 +206,8 @@ export function diffRlsStatus( sourceValue: src, targetValue: tgt, sql: { - up: `ALTER TABLE "${src.schemaname}"."${src.tablename}" ENABLE ROW LEVEL SECURITY;`, - down: `ALTER TABLE "${src.schemaname}"."${src.tablename}" DISABLE ROW LEVEL SECURITY;`, + up: `ALTER TABLE ${quoteName(src.schemaname)}.${quoteName(src.tablename)} ENABLE ROW LEVEL SECURITY;`, + down: `ALTER TABLE ${quoteName(src.schemaname)}.${quoteName(src.tablename)} DISABLE ROW LEVEL SECURITY;`, }, }) } else if (!src.rls_enabled && tgt.rls_enabled) { @@ -219,8 +220,8 @@ export function diffRlsStatus( sourceValue: src, targetValue: tgt, sql: { - up: `ALTER TABLE "${src.schemaname}"."${src.tablename}" DISABLE ROW LEVEL SECURITY;`, - down: `ALTER TABLE "${src.schemaname}"."${src.tablename}" ENABLE ROW LEVEL SECURITY;`, + up: `ALTER TABLE ${quoteName(src.schemaname)}.${quoteName(src.tablename)} DISABLE ROW LEVEL SECURITY;`, + down: `ALTER TABLE ${quoteName(src.schemaname)}.${quoteName(src.tablename)} ENABLE ROW LEVEL SECURITY;`, }, }) } diff --git a/packages/cli/test/quoted-names.test.ts b/packages/cli/test/quoted-names.test.ts index 536093e..0c91c15 100644 --- a/packages/cli/test/quoted-names.test.ts +++ b/packages/cli/test/quoted-names.test.ts @@ -13,7 +13,8 @@ import { mkdtemp, readFile, rm } from 'node:fs/promises' import { join } from 'node:path' import { tmpdir } from 'node:os' import { parseStatements } from '../src/sql-ast.js' -import { diffPolicies } from '../src/checks/rls.js' +import { diffPolicies, diffRlsStatus } from '../src/checks/rls.js' +import { diffExtensions } from '../src/checks/extensions.js' import { createPolicySql, dropPolicySql, commentPolicySql, diffSchemaPolicies } from '../src/utils/schema-policies.js' import { captureSnapshot } from '../src/snapshot.js' import type { QueryFn } from '../src/db.js' @@ -87,3 +88,22 @@ describe('snapshot files keep names intact', () => { } }) }) + +describe('other SQL keeps names intact', () => { + it('turning row level security on and off', () => { + const t = (rls_enabled: boolean) => ({ schemaname: 'public', tablename: TABLE, rls_enabled }) as never + for (const [src, tgt] of [[true, false], [false, true]]) { + const [issue] = diffRlsStatus([t(src)], [t(tgt)]) + expect(parseStatements(issue.sql!.up), issue.sql!.up).toBeDefined() + expect(parseStatements(issue.sql!.up)![0].node.relation.relname).toBe(TABLE) + } + }) + + it('creating and updating an extension', () => { + const ext = (version: string) => ({ name: 'odd "ext"', version, schema: 'ext "s"' }) as never + for (const issue of [...diffExtensions([ext("1.0")], []), ...diffExtensions([ext("2.0")], [ext("1'0")])]) { + expect(parseStatements(issue.sql!.up), issue.sql!.up).toBeDefined() + expect(parseStatements(issue.sql!.down!), issue.sql!.down).toBeDefined() + } + }) +}) From 7151367776fc2333531a875c48568ec8f70287cc Mon Sep 17 00:00:00 2001 From: Jasdeep Date: Thu, 8 Oct 2026 08:12:21 +0000 Subject: [PATCH 12/15] ci: install the packed package and run it as a user would Co-Authored-By: Claude Opus 5.5 --- .github/workflows/release-dry-run.yml | 16 ++++++++++++++++ packages/cli/scripts/test-release-candidates.sh | 5 +++++ 2 files changed, 21 insertions(+) diff --git a/.github/workflows/release-dry-run.yml b/.github/workflows/release-dry-run.yml index 0839abe..df45a70 100644 --- a/.github/workflows/release-dry-run.yml +++ b/.github/workflows/release-dry-run.yml @@ -68,3 +68,19 @@ jobs: - name: npm publish --dry-run working-directory: packages/cli run: npm publish --dry-run --tag next + + # The package as a user gets it: packed, installed elsewhere, run. A + # warning here reaches every user — oclif warned on every command, under + # NODE_ENV=development or test, that it could not find TypeScript. + - name: Install the packed package and run it + working-directory: packages/cli + run: | + npm pack --pack-destination "$RUNNER_TEMP" + mkdir "$RUNNER_TEMP/user" && cd "$RUNNER_TEMP/user" + npm init -y >/dev/null + npm install --no-audit --no-fund "$RUNNER_TEMP"/akalforge-supaforge-*.tgz + for env in production development test; do + NODE_ENV=$env npx supaforge --version 2> stderr.txt + NODE_ENV=$env npx supaforge diff --help > /dev/null 2>> stderr.txt + if [ -s stderr.txt ]; then echo "::error::NODE_ENV=$env wrote to stderr:"; cat stderr.txt; exit 1; fi + done diff --git a/packages/cli/scripts/test-release-candidates.sh b/packages/cli/scripts/test-release-candidates.sh index 8c24c79..e071794 100755 --- a/packages/cli/scripts/test-release-candidates.sh +++ b/packages/cli/scripts/test-release-candidates.sh @@ -129,6 +129,11 @@ rm -rf "$WORK/prefix" && mkdir -p "$WORK/prefix" (cd "$WORK/prefix" && npm init -y >/dev/null && npm install --silent --no-audit --no-fund "$WORK"/tgz/akalforge-supaforge-*.tgz "${DEPS[@]}") INSTALLED="$WORK/prefix/node_modules/@akalforge/supaforge" say "Installed: $(node "$INSTALLED/bin/run.js" --version) with $("$WORK/prefix/node_modules/.bin/dbdiff" --version)" +for env in production development test; do + if NODE_ENV=$env node "$INSTALLED/bin/run.js" --version 2>&1 >/dev/null | grep -q .; then + record "FAIL the installed CLI writes to stderr under NODE_ENV=$env" + fi +done export SUPAFORGE_E2E_CLI="$INSTALLED/bin/run.js" cd "$SF" From b2cb78ef4fa9d09ad45c8218b5686e39cf403be2 Mon Sep 17 00:00:00 2001 From: Jasdeep Date: Thu, 8 Oct 2026 08:12:44 +0000 Subject: [PATCH 13/15] docs: the release-candidates script Co-Authored-By: Claude Opus 5.5 --- packages/cli/README.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/cli/README.md b/packages/cli/README.md index c940030..6e452c7 100644 --- a/packages/cli/README.md +++ b/packages/cli/README.md @@ -332,6 +332,13 @@ npm run lint # type-check | Database e2e | `npm run test:e2e` | Docker or Podman | | Scenarios | `npx vitest run -c vitest.scenarios.config.ts` | Docker or Podman, `pg_dump` | | Supabase e2e | `npm run test:e2e:supabase` | Supabase CLI | +| Release candidates | `scripts/test-release-candidates.sh --dbdiff ../DBDiff --pg-conformance ../pg-conformance` | Podman or Docker | + +The release-candidates script builds DBDiff's binary and the npm packages +from local checkouts the way a release does, installs SupaForge against them +as a user would, and runs every suite above against that install. It tests +changes that span the three projects together, before any of them is +released. The scenario suite migrates every case in the [`@akalforge/pg-conformance`](https://www.npmjs.com/package/@akalforge/pg-conformance) corpus in both From 2c398a13796b9021990a678968333793996cd48c Mon Sep 17 00:00:00 2001 From: Jasdeep Date: Thu, 8 Oct 2026 08:36:08 +0000 Subject: [PATCH 14/15] fix(snapshot): keep MAINTAIN in a GRANT of its own MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Grouping a role's privileges into one GRANT put PostgreSQL 17's MAINTAIN alongside the rest, so restoring a 17 snapshot into 15 failed on the whole statement and rolled back, where before only the MAINTAIN line was skipped. Found by the release smoke against the packed candidate. A new scenario restores a snapshot across every pair of servers the scenario suites run on, 17 → 15 included. Co-Authored-By: Claude Opus 5.5 --- packages/cli/src/snapshot.ts | 7 +- packages/cli/test/scenarios/restore.test.ts | 76 +++++++++++++++++++++ packages/cli/test/snapshot.test.ts | 22 ++++++ 3 files changed, 103 insertions(+), 2 deletions(-) create mode 100644 packages/cli/test/scenarios/restore.test.ts diff --git a/packages/cli/src/snapshot.ts b/packages/cli/src/snapshot.ts index 22790d2..d2744e6 100644 --- a/packages/cli/src/snapshot.ts +++ b/packages/cli/src/snapshot.ts @@ -685,10 +685,13 @@ async function captureRoleGrants( // One statement per role, object and grant option, listing the // privileges: one per privilege made a restore that skipped a relation - // list it once for every privilege of every role. + // list it once for every privilege of every role. MAINTAIN, a PostgreSQL + // 17 privilege, stays on its own, so an older server skips only it rather + // than rejecting every privilege granted alongside. const grouped = new Map() for (const row of rows) { - const key = JSON.stringify([row.grantee, row.table_schema, row.table_name, row.column_name, Boolean(row.is_grantable)]) + const key = JSON.stringify([row.grantee, row.table_schema, row.table_name, row.column_name, Boolean(row.is_grantable), + row.privilege_type === 'MAINTAIN']) grouped.set(key, [...(grouped.get(key) ?? []), row]) } const statements = [...grouped.values()].map(([row, ...rest]) => diff --git a/packages/cli/test/scenarios/restore.test.ts b/packages/cli/test/scenarios/restore.test.ts new file mode 100644 index 0000000..e48d22c --- /dev/null +++ b/packages/cli/test/scenarios/restore.test.ts @@ -0,0 +1,76 @@ +/** + * A snapshot of the source restores into the target's server, whatever the + * pair of versions. + * + * Run on every topology the scenario suites run on, so a snapshot of + * PostgreSQL 17 is restored into 15 here. The grants in it include MAINTAIN, + * a 17 privilege: grouped into one GRANT with the others, the 15 target + * rejected the whole statement and the restore rolled back. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest' +import { readdir } from 'node:fs/promises' +import { join } from 'node:path' +import type { PgHarness } from '../harness/PgHarness.js' +import { describeWithContainers } from '../harness/containers.js' +import { scenarioHarness } from './topology.js' + +const describeE2E = describeWithContainers() + +const ROLES = ['anon', 'authenticated', 'service_role'] + .map(r => `DO $$ BEGIN CREATE ROLE ${r} NOLOGIN; EXCEPTION WHEN duplicate_object THEN NULL; END $$;`).join('\n') + +const SOURCE = ` + CREATE TABLE public.items (id int PRIMARY KEY, owner text, note text); + GRANT ALL ON public.items TO anon, authenticated, service_role; + REVOKE INSERT, UPDATE, DELETE, TRUNCATE ON public.items FROM anon; + ALTER TABLE public.items ENABLE ROW LEVEL SECURITY; + CREATE POLICY own ON public.items FOR SELECT TO authenticated USING (owner = current_user); + COMMENT ON TABLE public.items IS 'items'; +` + +/** Grants to the API roles, without MAINTAIN, which a pre-17 server cannot hold. */ +const GRANTS = ` + SELECT coalesce(string_agg(format('%s %s', a.grantee::regrole, a.privilege_type), ', ' + ORDER BY a.grantee::regrole::text, a.privilege_type), '') + FROM pg_class c CROSS JOIN LATERAL aclexplode(c.relacl) a + WHERE c.oid = 'public.items'::regclass AND a.grantee <> c.relowner AND a.privilege_type <> 'MAINTAIN' +` + +describeE2E('scenarios: a snapshot restored across the pair of servers', () => { + let h: PgHarness + let ws: string + + beforeAll(async () => { + h = scenarioHarness() + await h.up() + await h.applySql('source', ROLES + SOURCE) + await h.applySql('target', ROLES) + await h.createDatabase('target', 'sc_restored') + ws = await h.workspace({ + environments: { + source: { dbUrl: h.connectionString('source') }, + restored: { dbUrl: h.urlFor('target', 'sc_restored') }, + }, + }) + }, 300_000) + + afterAll(async () => { await h?.down() }, 120_000) + + it('restores the table, its policy, comment and grants', async () => { + const snap = await h.cli(['snapshot', '-e', 'source', '--apply'], { cwd: ws }) + expect(snap.code, snap.stdout + snap.stderr).toBe(0) + const [snapshot] = await readdir(join(ws, '.supaforge', 'snapshots')) + + // A 15 target skips MAINTAIN and so reports the restore incomplete (exit + // 1); nothing may fail or roll back. + const r = await h.cli(['restore', '-e', 'restored', '--from-snapshot', snapshot, '--apply', '--json'], { cwd: ws }) + const result = JSON.parse(r.stdout) as { errors: unknown[]; rolledBack?: unknown[] } + expect(result.errors, r.stdout).toEqual([]) + expect(result.rolledBack ?? []).toEqual([]) + + const facts = `SELECT obj_description('public.items'::regclass) || ' / ' || + (SELECT count(*) FROM pg_policy WHERE polrelid = 'public.items'::regclass)` + expect(await h.sqlIn('target', 'sc_restored', facts)).toBe('items / 1') + expect(await h.sqlIn('target', 'sc_restored', GRANTS)).toBe(await h.sql('source', GRANTS)) + }, 600_000) +}) diff --git a/packages/cli/test/snapshot.test.ts b/packages/cli/test/snapshot.test.ts index ec23ac3..ba7b825 100644 --- a/packages/cli/test/snapshot.test.ts +++ b/packages/cli/test/snapshot.test.ts @@ -516,4 +516,26 @@ describe('captureSnapshot: grants are captured one statement per role and object 'GRANT UPDATE ("name") ON "public"."items" TO "anon";', ]) }) + + // MAINTAIN is a PostgreSQL 17 privilege. Grouped with the others, a 15 + // target rejected the whole GRANT and the restore rolled back; on its own, + // only it is skipped there. + it('keeps MAINTAIN in a GRANT of its own', async () => { + const rows = ['DELETE', 'MAINTAIN', 'SELECT'].map(privilege_type => + ({ grantee: 'anon', table_schema: 'public', table_name: 'items', column_name: null, privilege_type, is_grantable: false })) + const queryFn = (async (_url: string, sql: string) => sql.includes('aclexplode') ? rows : []) as unknown as QueryFn + const result = await captureSnapshot({ + envName: 'prod', + env: { dbUrl: 'postgres://example' }, + config: { environments: { prod: { dbUrl: 'postgres://example' } } } as never, + outputDir: dir, + queryFn, + fetchFn: (async () => new Response('[]', { status: 200 })) as never, + }) + const grants = (await readFile(join(result.dir, 'roles.sql'), 'utf8')).split('\n').filter(l => l.startsWith('GRANT')) + expect(grants.sort()).toEqual([ + 'GRANT DELETE, SELECT ON "public"."items" TO "anon";', + 'GRANT MAINTAIN ON "public"."items" TO "anon";', + ]) + }) }) From fdc3d4861e0f3a38451bbca5b004555087012e1c Mon Sep 17 00:00:00 2001 From: Jasdeep Date: Thu, 8 Oct 2026 08:57:28 +0000 Subject: [PATCH 15/15] fix(bin): turn oclif's TypeScript lookup off in the published package only Off everywhere, a checkout could no longer run its e2e suites before building dist, which CI does. The published package ships no src/, so that is what the bin checks. Co-Authored-By: Claude Opus 5.5 --- packages/cli/bin/run.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/cli/bin/run.js b/packages/cli/bin/run.js index 37a9483..77d40d1 100755 --- a/packages/cli/bin/run.js +++ b/packages/cli/bin/run.js @@ -1,10 +1,12 @@ #!/usr/bin/env node +import {existsSync} from 'node:fs' import {execute, settings} from '@oclif/core' -// The published package is compiled. Left on, oclif looks for TypeScript -// whenever NODE_ENV is development or test — as in many projects running -// npx supaforge — and warned on every command that it could not find it. -settings.enableAutoTranspile = false +// The published package is compiled and ships no src/. Left on, oclif looks +// for TypeScript whenever NODE_ENV is development or test — as in many +// projects running npx supaforge — and warned on every command that it could +// not find it. A checkout keeps the lookup, which its tests rely on. +if (!existsSync(new URL('../src', import.meta.url))) settings.enableAutoTranspile = false await execute({dir: import.meta.url}).catch((err) => { // oclif handles CLIError internally — this catches truly unexpected errors.