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/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 diff --git a/packages/cli/bin/run.js b/packages/cli/bin/run.js index 76bb0bc..77d40d1 100755 --- a/packages/cli/bin/run.js +++ b/packages/cli/bin/run.js @@ -1,5 +1,12 @@ #!/usr/bin/env node -import {execute} from '@oclif/core' +import {existsSync} from 'node:fs' +import {execute, settings} from '@oclif/core' + +// 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. diff --git a/packages/cli/scripts/test-release-candidates.sh b/packages/cli/scripts/test-release-candidates.sh new file mode 100755 index 0000000..e071794 --- /dev/null +++ b/packages/cli/scripts/test-release-candidates.sh @@ -0,0 +1,194 @@ +#!/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)" +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" + +# 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/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/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[] = [] @@ -220,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) { @@ -234,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/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/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/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/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 87b0513..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']) @@ -577,21 +582,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 +605,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', @@ -924,7 +967,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 +978,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 +988,7 @@ const REPLACEABLE_REPORT: Record { 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-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/restore.ts b/packages/cli/src/restore.ts index 38d9d77..8465ad6 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' @@ -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 } @@ -843,7 +853,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 +944,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 +977,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..d2744e6 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' @@ -445,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)}`) @@ -466,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, @@ -539,7 +553,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 +563,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};`) } } @@ -669,12 +683,22 @@ 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 ? ` (${quoteIdent(row.column_name)})` : '') - + ` ON ${quoteIdent(row.table_schema)}.${quoteIdent(row.table_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. 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), + row.privilege_type === 'MAINTAIN']) + 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' : quoteIdent(row.grantee)}` + + ` TO ${row.grantee === 'PUBLIC' ? 'PUBLIC' : quoteName(row.grantee)}` + (row.is_grantable ? ' WITH GRANT OPTION' : '') + ';') @@ -702,7 +726,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 +877,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/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/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/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/dbdiff-units.test.ts b/packages/cli/test/dbdiff-units.test.ts index 89aa4a7..c2d7f46 100644 --- a/packages/cli/test/dbdiff-units.test.ts +++ b/packages/cli/test/dbdiff-units.test.ts @@ -99,6 +99,22 @@ describe('sqlToIssues with units', () => { // 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 1fadb81..f19a33c 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: [ @@ -1499,3 +1507,72 @@ 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) + }) +}) + +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;') + }) +}) + +// 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 new file mode 100644 index 0000000..5027866 --- /dev/null +++ b/packages/cli/test/e2e/grants-on-new-tables.test.ts @@ -0,0 +1,82 @@ +/** + * 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); + -- 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. */ +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', 'private_v') + 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.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) + + 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) +}) 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) +}) 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/e2e/snapshot-restore.test.ts b/packages/cli/test/e2e/snapshot-restore.test.ts index 575138f..3b75af7 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,8 @@ 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') }, + rolledback: { dbUrl: h.urlFor('target', 'sf_rolled_back') }, }, checks: { data: { tables: ['public.features', 'public.plans'] } }, }) @@ -108,6 +114,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 +149,49 @@ 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 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/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)}`; } diff --git a/packages/cli/test/quoted-names.test.ts b/packages/cli/test/quoted-names.test.ts new file mode 100644 index 0000000..0c91c15 --- /dev/null +++ b/packages/cli/test/quoted-names.test.ts @@ -0,0 +1,109 @@ +/** + * 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, 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' + +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 }) + } + }) +}) + +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() + } + }) +}) 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/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/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 ')}`) } diff --git a/packages/cli/test/snapshot.test.ts b/packages/cli/test/snapshot.test.ts index c773994..ba7b825 100644 --- a/packages/cli/test/snapshot.test.ts +++ b/packages/cli/test/snapshot.test.ts @@ -482,3 +482,60 @@ 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";', + ]) + }) + + // 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";', + ]) + }) +}) 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"']) + }) +}) 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']) + }) +})