diff --git a/packages/cli/src/branch.ts b/packages/cli/src/branch.ts index 8072b3a..08f6bd1 100644 --- a/packages/cli/src/branch.ts +++ b/packages/cli/src/branch.ts @@ -76,6 +76,32 @@ export function replaceDbName(sourceUrl: string, dbName: string): string { return u.toString() } +/** The database a connection URL names, or '' when it names none. */ +export function dbNameOf(url: string): string { + return decodeURIComponent(new URL(url).pathname.replace(/^\//, '')) +} + +/** + * Where to create or drop `dbName` from. A URL naming that very database + * cannot drop it — PostgreSQL refuses to drop the database a session is + * connected to — so the server's `postgres` database is used instead. + */ +export function maintenanceUrl(baseUrl: string, dbName: string): string { + return dbNameOf(baseUrl) === dbName ? replaceDbName(baseUrl, 'postgres') : baseUrl +} + +/** + * The database `clone` writes to when --local-db is not given: the one the + * local URL names, or `supaforge_local` when it names none or only the + * `postgres` maintenance database. A URL naming a database used to be read + * only for its server, and the clone went to `supaforge_local` regardless. + */ +export function cloneTargetName(localUrl: string, explicit?: string): string { + if (explicit) return explicit + const named = dbNameOf(localUrl) + return named && named !== 'postgres' ? named : 'supaforge_local' +} + /** Parse connection URL to extract components. */ function parseUrl(url: string) { const u = new URL(url) @@ -459,7 +485,7 @@ export async function cloneRemoteToLocal(opts: CloneRemoteOptions): Promise { try { - const client = newPgClient(localBaseUrl) + const client = newPgClient(maintenanceUrl(localBaseUrl, localDbName)) await client.connect() const { rows } = await client.query( 'SELECT 1 FROM pg_database WHERE datname = $1', diff --git a/packages/cli/src/pg-errors.ts b/packages/cli/src/pg-errors.ts index ec30d7e..1925e9d 100644 --- a/packages/cli/src/pg-errors.ts +++ b/packages/cli/src/pg-errors.ts @@ -4,6 +4,12 @@ */ export const ABSENT_ON_TARGET: ReadonlySet = new Set(['42P01', '42883', '3F000', '42704']) +/** + * An object others still depend on: for DROP ROLE, a role that owns objects or + * holds privileges somewhere on the server, in any of its databases. + */ +export const DEPENDENT_OBJECTS_STILL_EXIST = '2BP01' + /** A column the statement names is missing. */ export const UNDEFINED_COLUMN = '42703' diff --git a/packages/cli/src/promote.ts b/packages/cli/src/promote.ts index d3c10d8..2e3d95e 100644 --- a/packages/cli/src/promote.ts +++ b/packages/cli/src/promote.ts @@ -2,7 +2,7 @@ import pg from 'pg' import { newPgClient } from './db.js' import type { ScanResult, SyncAction } from './types/drift' import { errMsg } from './utils/error' -import { ABSENT_ON_TARGET, EXTENSION_UNAVAILABLE, OLDER_SERVER, UNDEFINED_COLUMN, sqlState } from './pg-errors.js' +import { ABSENT_ON_TARGET, DEPENDENT_OBJECTS_STILL_EXIST, EXTENSION_UNAVAILABLE, OLDER_SERVER, UNDEFINED_COLUMN, sqlState } from './pg-errors.js' import { destructiveReason } from './dbdiff' import { parseStatements } from './sql-ast' import { estimateLocks, lockBudgetWarning, readLockBudget } from './lock-budget.js' @@ -599,7 +599,10 @@ async function runInTransaction( * is something else. * * Only "does not exist" failures qualify, and for `CREATE EXTENSION`, an - * extension the server does not ship. + * extension the server does not ship — and a `DROP ROLE` for a role that still + * owns or is granted something. Roles are server-wide, so a role this database + * does not use can still own objects in another one on the same server; that + * one refusal used to roll back every other fix in the apply. */ export function unmetDependency(sql: string, err: unknown, olderTarget = false): string | undefined { const code = sqlState(err) @@ -614,6 +617,9 @@ export function unmetDependency(sql: string, err: unknown, olderTarget = false): if (ABSENT_ON_TARGET.has(code) || code === UNDEFINED_COLUMN) { return 'it needs something the target does not have' } + if (code === DEPENDENT_OBJECTS_STILL_EXIST && /^\s*DROP\s+ROLE\b/i.test(sqlSkeleton(sql))) { + return 'the role still owns or is granted objects on this server, in this or another database; reassign or drop those (REASSIGN OWNED / DROP OWNED in each database) before dropping it' + } return undefined } diff --git a/packages/cli/test/branch.test.ts b/packages/cli/test/branch.test.ts index bd7d893..d9f5591 100644 --- a/packages/cli/test/branch.test.ts +++ b/packages/cli/test/branch.test.ts @@ -203,3 +203,19 @@ describe('isCloneDatabase', () => { expect(await isCloneDatabase('postgresql://postgres@localhost:5432/', tempDir)).toBe(false) }) }) + +describe('clone target from --local-url', () => { + it('clones into the database the URL names when --local-db is not given', async () => { + const { cloneTargetName } = await import('../src/branch.js') + expect(cloneTargetName('postgres://u:p@h:5432/mydb')).toBe('mydb') + expect(cloneTargetName('postgres://u:p@h:5432/postgres')).toBe('supaforge_local') + expect(cloneTargetName('postgres://u:p@h:5432')).toBe('supaforge_local') + expect(cloneTargetName('postgres://u:p@h:5432/mydb', 'other')).toBe('other') + }) + + it('creates and drops a database from postgres, never from inside it', async () => { + const { maintenanceUrl } = await import('../src/branch.js') + expect(maintenanceUrl('postgres://u:p@h:5432/mydb', 'mydb')).toBe('postgres://u:p@h:5432/postgres') + expect(maintenanceUrl('postgres://u:p@h:5432/admin', 'mydb')).toBe('postgres://u:p@h:5432/admin') + }) +}) diff --git a/packages/cli/test/promote-savepoints.test.ts b/packages/cli/test/promote-savepoints.test.ts index ce3e6a6..7fcbe87 100644 --- a/packages/cli/test/promote-savepoints.test.ts +++ b/packages/cli/test/promote-savepoints.test.ts @@ -104,6 +104,27 @@ describe('promote: one fix the target cannot take', () => { // Ordering is meant to make this impossible, but a missing dependency no // longer stops the apply, so an ordering miss must not quietly leave a fix // out that would have applied a moment later. + // Roles are server-wide: one the source lacks can own objects in another + // database on the target's server, and DROP ROLE refuses. That used to roll + // back every schema fix in the apply with it. + it('leaves out a role drop the server refuses and commits the rest', async () => { + const result = scan([['a', 'provides:a'], ['b', 'needs:a provides:b']]) + result.checks.push({ + check: 'roles', status: 'drifted', durationMs: 1, + issues: [{ + id: 'roles-extra-reporting', check: 'roles', severity: 'warning', title: 'Extra role: reporting', + description: 'test', sql: { up: 'DROP ROLE IF EXISTS "reporting"; -- code:2BP01', down: '' }, + }], + }) + const applied = await promote({ dbUrl: 'postgres://t', scanResult: result, allowDestructive: true }) + + expect(applied.rolledBack).toBeUndefined() + expect(applied.applied.map(f => f.issueId)).toEqual(['a', 'b']) + expect(applied.errors.map(e => e.issueId)).toEqual(['roles-extra-reporting']) + expect(applied.errors[0].error).toContain('still owns or is granted') + expect(committed).toEqual(new Set(['a', 'b'])) + }) + it('retries a fix whose dependency comes later in the batch', async () => { const result = await promote({ dbUrl: 'postgres://t', @@ -160,6 +181,11 @@ describe('unmetDependency', () => { } }) + it('accepts a role the server still needs, only for DROP ROLE', () => { + expect(unmetDependency('DROP ROLE IF EXISTS "reporting";', err('2BP01'))).toContain('still owns or is granted') + expect(unmetDependency('DROP TABLE t;', err('2BP01'))).toBeUndefined() + }) + it('rejects every other failure', () => { expect(unmetDependency('CREATE TABLE t (id int)', err('42P07'))).toBeUndefined() expect(unmetDependency('ALTER TABLE t ADD CONSTRAINT u UNIQUE (id)', err('23505'))).toBeUndefined()