From 6a94bdaa0c2266090ee09c82f4f6ecaa3879d4cb Mon Sep 17 00:00:00 2001 From: Yasiru Geevinda Date: Wed, 23 Sep 2026 16:06:22 +0000 Subject: [PATCH] fix(db): make deploy's dry-run clone schema-only by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `db deploy` cloned the full target database — schema and every row — into a throwaway local container before running its DDL dry-run, then dropped it. On any database with real data this dominated the command's runtime, billed egress on every deploy, and put production rows on developer machines. The dry-run verifies that committed DDL applies cleanly, which only needs the target's structure. Default to `pg_dump --schema-only` and add `--with-data` for migrations whose risk lives in the data (adding NOT NULL or UNIQUE to a populated column, type narrowing, backfills). `schemaOnly` is now a required parameter on `cloneDatabase` and `cloneDatabaseViaContainer`. The silent `= false` default is how this happened: `db start` remembered to pass `true`, `db deploy` did not. Requiring it means no future caller can get row data by omission. Also warn when `--with-data` is used that seeds run against a clone already holding the target's rows and may conflict on existing keys. --- CLAUDE.md | 2 +- cli/docs/db.md | 9 ++-- cli/src/modules/db/commands/deploy.ts | 41 ++++++++++++++++--- cli/src/modules/db/index.ts | 1 + cli/src/modules/db/services/container.ts | 7 +++- cli/src/modules/db/services/database.ts | 8 +++- cli/test/e2e/smoke/basic-commands.test.ts | 8 ++++ .../modules/db/services/container.test.ts | 12 +++--- cli/test/modules/db/services/database.test.ts | 12 +++--- 9 files changed, 77 insertions(+), 23 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index aa22876..fd486a9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -296,7 +296,7 @@ Remotes are managed via utilities in `modules/db/utils/remotes.ts`: | `postkit db plan` | Generate schema diff with pgschema | | `postkit db apply` | Apply migration to local DB (creates dbmate migration) | | `postkit db commit` | Commit session migrations for deployment | -| `postkit db deploy [--remote ]` | Deploy committed migrations (with dry-run verification) | +| `postkit db deploy [--remote ] [--with-data]` | Deploy committed migrations (with schema-only dry-run verification; `--with-data` clones target rows too) | | `postkit db status` | Show session state | | `postkit db abort` | Cancel session, cleanup local resources | | `postkit db migration []` | Create a manual SQL migration | diff --git a/cli/docs/db.md b/cli/docs/db.md index cd77a7e..757a6a8 100644 --- a/cli/docs/db.md +++ b/cli/docs/db.md @@ -260,7 +260,7 @@ postkit db start --remote staging # Use specific remote **Why infra is applied before cloning:** the remote's dump includes `CREATE POLICY ... TO `, `GRANT ... TO `, and `ALTER DEFAULT PRIVILEGES ... TO ` statements for every custom role referenced by your RLS policies and grants. Those statements fail with `role "" does not exist` if replayed before the role exists — which is exactly what happens on a brand-new local DB/container. Applying `db/infra/` first (same roles/schemas your project already declares) means those statements succeed, so RLS policies and grants clone correctly. The clone's `psql` runs with `-v ON_ERROR_STOP=1`, so if a referenced role genuinely doesn't exist anywhere in `db/infra/`, the clone now fails loudly with a clear error instead of silently dropping that policy/grant. -**Why the clone is schema-only:** copying full production row data (customer records, emails, tokens, etc.) into every disposable local dev container on every `db start` is unnecessary and a privacy/compliance risk. `db start` only needs to reproduce *structure* — tables, RLS policies, grants, indexes, triggers, functions — so it doesn't copy any rows. For synthetic local test data, use `db/schema//seeds/`, applied separately during `db plan`/`db apply`. (`postkit db deploy`'s own dry-run clone is unaffected by this — it still clones full data, since it's specifically verifying real migrations against realistic data before they touch production.) +**Why the clone is schema-only:** copying full production row data (customer records, emails, tokens, etc.) into every disposable local dev container on every `db start` is unnecessary and a privacy/compliance risk. `db start` only needs to reproduce *structure* — tables, RLS policies, grants, indexes, triggers, functions — so it doesn't copy any rows. For synthetic local test data, use `db/schema//seeds/`, applied separately during `db plan`/`db apply`. (`postkit db deploy`'s dry-run clone is schema-only for the same reasons; pass `--with-data` if a migration's risk lies in the data itself.) --- @@ -320,7 +320,7 @@ postkit db commit -f # Skip confirmation --- -### `postkit db deploy [--remote ] [--url ]` +### `postkit db deploy [--remote ] [--url ] [--with-data]` Deploy committed migrations to a remote database. Performs a full dry-run verification on a local clone before touching the target. @@ -330,6 +330,7 @@ postkit db deploy --remote staging # Use specific remote postkit db deploy --url=postgres://... # Direct URL override postkit db deploy --remote production -f # Skip confirmations postkit db deploy --dry-run # Verify only, don't touch target +postkit db deploy --with-data # Clone target row data into the dry-run clone ``` **What it does:** @@ -339,7 +340,7 @@ postkit db deploy --dry-run # Verify only, don't touch target 4. Tests the target database connection and detects its PostgreSQL major version 5. **If `localDbUrl` is empty**: Starts a temporary `postgres:{version}-alpine` container (version-matched to the target) for the dry-run 6. **Applies `db/infra/` (roles, schemas, extensions) to the local/temp database** — before cloning, for the same reason `db start` does (RLS policies and grants in the target's dump reference roles that must already exist) -7. Clones the target database (full data — this dry-run intentionally tests against realistic data) to the local URL. When using a temp container, cloning runs via `docker exec` inside the container +7. Clones the target database's **structure only** (`pg_dump --schema-only`, no row data) to the local URL. When using a temp container, cloning runs via `docker exec` inside the container. Pass `--with-data` to clone row data as well 8. Runs a full dry-run on the local clone: infra (reapplied, idempotently), dbmate migrate, seeds 9. If `--dry-run` is set, stops here and reports results without touching the target 10. Reports dry-run results and confirms deployment (unless `-f`) @@ -348,6 +349,8 @@ postkit db deploy --dry-run # Verify only, don't touch target If the dry run fails, deployment is aborted and no changes are made to the target database. +**`--with-data`:** the dry-run verifies that committed DDL applies cleanly, which only needs the target's structure. Cloning row data as well means every deploy pulls the entire target dataset across the network into a clone that is dropped minutes later — slow, costly in egress, and it puts production rows on developer machines. Reach for `--with-data` when the migration's risk lives in the data rather than the schema: adding `NOT NULL` or a `UNIQUE` constraint to a populated column, a type narrowing that existing rows might not satisfy, or a data backfill. Note that seeds run as part of the dry-run, so against a clone that already holds the target's rows seed inserts may conflict on existing keys. + --- ### `postkit db remote` diff --git a/cli/src/modules/db/commands/deploy.ts b/cli/src/modules/db/commands/deploy.ts index 518610b..9baf4f0 100644 --- a/cli/src/modules/db/commands/deploy.ts +++ b/cli/src/modules/db/commands/deploy.ts @@ -24,6 +24,7 @@ import {PostkitError} from "../../../common/errors"; interface DeployOptions extends CommandOptions { remote?: string; url?: string; + withData?: boolean; } function resolveTargetUrl(options: DeployOptions): {url: string; label: string} { @@ -224,15 +225,45 @@ export async function deployCommand(options: DeployOptions): Promise { logger.step(3, totalSteps, "Applying infrastructure to local database..."); await applyInfraStep(spinner, localDbUrl, "local clone"); - logger.step(4, totalSteps, "Cloning target database to local..."); - spinner.start("Cloning target database to local for dry-run verification..."); + // The dry-run verifies that committed DDL applies cleanly, which only needs + // the target's *structure*. Cloning row data too means every deploy drags the + // entire production dataset across the network into a container we throw away + // minutes later. --with-data opts back in for migrations whose risk is in the + // data itself (adding NOT NULL or a UNIQUE constraint to a populated column). + const schemaOnly = !options.withData; + logger.step( + 4, + totalSteps, + schemaOnly + ? "Cloning target database structure to local..." + : "Cloning target database (with data) to local...", + ); + spinner.start( + schemaOnly + ? "Cloning database structure for dry-run verification..." + : "Cloning database with row data for dry-run verification (this may take a while)...", + ); if (tempContainerID) { - await cloneDatabaseViaContainer(tempContainerID, targetUrl, localDbUrl); + await cloneDatabaseViaContainer(tempContainerID, targetUrl, localDbUrl, schemaOnly); } else { - await cloneDatabase(targetUrl, localDbUrl); + await cloneDatabase(targetUrl, localDbUrl, schemaOnly); } const localTableCount = await getTableCount(localDbUrl); - spinner.succeed(`Target cloned to local (${localTableCount} tables)`); + spinner.succeed( + schemaOnly + ? `Target structure cloned to local (${localTableCount} tables, no row data)` + : `Target cloned to local (${localTableCount} tables, with row data)`, + ); + + if (options.withData) { + // Seeds are written to populate an empty database. Run against a clone that + // already holds the target's rows, they can collide on primary/unique keys + // and fail the dry-run for reasons unrelated to the migration being deployed. + logger.warn( + "--with-data: seeds run against a clone that already contains the target's rows; " + + "seed inserts may conflict on existing keys.", + ); + } // Steps 5-7: Dry run on local clone logger.blank(); diff --git a/cli/src/modules/db/index.ts b/cli/src/modules/db/index.ts index b916898..60d2f0a 100644 --- a/cli/src/modules/db/index.ts +++ b/cli/src/modules/db/index.ts @@ -132,6 +132,7 @@ export function registerDbModule(program: Command): void { .description("Deploy committed migrations (defaults to remote DB)") .option("--remote ", "Target remote name") .option("--url ", "Direct database URL to deploy to") + .option("--with-data", "Clone the target's row data into the dry-run clone (slow; off by default)") .option("-f, --force", "Skip confirmation prompts") .action(async (cmdOptions) => { await withInitCheck(async () => { diff --git a/cli/src/modules/db/services/container.ts b/cli/src/modules/db/services/container.ts index d9c9a30..1fa265c 100644 --- a/cli/src/modules/db/services/container.ts +++ b/cli/src/modules/db/services/container.ts @@ -119,11 +119,16 @@ function toDockerHost(host: string): string { return host; } +/** + * `schemaOnly` is required here for the same reason as in `cloneDatabase` — the + * cost gap between structure and full data is large enough that no caller + * should get one by accident. + */ export async function cloneDatabaseViaContainer( containerID: string, sourceUrl: string, targetUrl: string, - schemaOnly = false, + schemaOnly: boolean, ): Promise { const src = parseConnectionUrl(sourceUrl); const dst = parseConnectionUrl(targetUrl); diff --git a/cli/src/modules/db/services/database.ts b/cli/src/modules/db/services/database.ts index 28a7016..df2db87 100644 --- a/cli/src/modules/db/services/database.ts +++ b/cli/src/modules/db/services/database.ts @@ -113,10 +113,16 @@ export function sanitizeCloneLine(line: string): string { return makeSchemaCreationIdempotent(neutralizeUnsupportedPreambleSettings(line)); } +/** + * `schemaOnly` is intentionally required, with no default. Cloning row data is + * enormously more expensive than cloning structure — a silent default here is + * how `db deploy` ended up pulling entire production datasets across the + * network for a DDL dry-run. Every caller must state which one it wants. + */ export async function cloneDatabase( sourceUrl: string, targetUrl: string, - schemaOnly = false, + schemaOnly: boolean, ): Promise { const src = parseConnectionUrl(sourceUrl); const dst = parseConnectionUrl(targetUrl); diff --git a/cli/test/e2e/smoke/basic-commands.test.ts b/cli/test/e2e/smoke/basic-commands.test.ts index 926025a..3950ba1 100644 --- a/cli/test/e2e/smoke/basic-commands.test.ts +++ b/cli/test/e2e/smoke/basic-commands.test.ts @@ -30,6 +30,14 @@ describe("Smoke tests — basic CLI commands (no Docker)", () => { expect(result.stdout).toContain("abort"); }); + it("db deploy help documents --with-data as opt-in", async () => { + const result = await runCli(["db", "deploy", "--help"]); + expect(result.exitCode).toBe(0); + expect(result.stdout).toContain("--with-data"); + // Row data is opt-in: the dry-run clone is schema-only unless asked otherwise. + expect(result.stdout).toContain("off by default"); + }); + it("db status fails without config file", async () => { const tmpDir = await createEmptyDir(); try { diff --git a/cli/test/modules/db/services/container.test.ts b/cli/test/modules/db/services/container.test.ts index 443ea5e..b0d9cae 100644 --- a/cli/test/modules/db/services/container.test.ts +++ b/cli/test/modules/db/services/container.test.ts @@ -187,7 +187,7 @@ describe("container", () => { it("runs pg_dump and psql inside the container via docker exec", async () => { vi.mocked(runPipedCommands).mockResolvedValue({stdout: "", stderr: "", exitCode: 0}); - await cloneDatabaseViaContainer(containerID, sourceUrl, targetUrl); + await cloneDatabaseViaContainer(containerID, sourceUrl, targetUrl, false); expect(runPipedCommands).toHaveBeenCalledTimes(1); const [producer, consumer] = vi.mocked(runPipedCommands).mock.calls[0]!; @@ -210,7 +210,7 @@ describe("container", () => { it("psql connects to container-internal localhost:5432, not the mapped port", async () => { vi.mocked(runPipedCommands).mockResolvedValue({stdout: "", stderr: "", exitCode: 0}); - await cloneDatabaseViaContainer(containerID, sourceUrl, targetUrl); + await cloneDatabaseViaContainer(containerID, sourceUrl, targetUrl, false); const [, consumer] = vi.mocked(runPipedCommands).mock.calls[0]!; expect(consumer.args).toContain("localhost"); @@ -219,9 +219,9 @@ describe("container", () => { expect(consumer.args).not.toContain("15432"); }); - it("does not pass --schema-only by default", async () => { + it("omits --schema-only when schemaOnly is false", async () => { vi.mocked(runPipedCommands).mockResolvedValue({stdout: "", stderr: "", exitCode: 0}); - await cloneDatabaseViaContainer(containerID, sourceUrl, targetUrl); + await cloneDatabaseViaContainer(containerID, sourceUrl, targetUrl, false); const [producer] = vi.mocked(runPipedCommands).mock.calls[0]!; expect(producer.args).not.toContain("--schema-only"); }); @@ -236,7 +236,7 @@ describe("container", () => { it("passes PGPASSWORD for source via -e flag in docker exec args", async () => { vi.mocked(runPipedCommands).mockResolvedValue({stdout: "", stderr: "", exitCode: 0}); - await cloneDatabaseViaContainer(containerID, sourceUrl, targetUrl); + await cloneDatabaseViaContainer(containerID, sourceUrl, targetUrl, false); const [producer] = vi.mocked(runPipedCommands).mock.calls[0]!; const envFlag = producer.args.findIndex((a) => a === "-e"); @@ -252,7 +252,7 @@ describe("container", () => { }); await expect( - cloneDatabaseViaContainer(containerID, sourceUrl, targetUrl), + cloneDatabaseViaContainer(containerID, sourceUrl, targetUrl, false), ).rejects.toThrow("Failed to clone database via container"); }); }); diff --git a/cli/test/modules/db/services/database.test.ts b/cli/test/modules/db/services/database.test.ts index e0d16e4..7e217a1 100644 --- a/cli/test/modules/db/services/database.test.ts +++ b/cli/test/modules/db/services/database.test.ts @@ -133,7 +133,7 @@ describe("database", () => { describe("cloneDatabase()", () => { it("calls runPipedCommands with pg_dump and psql", async () => { vi.mocked(runPipedCommands).mockResolvedValue({stdout: "", stderr: "", exitCode: 0}); - await cloneDatabase("postgres://src:pass@src-host:5432/srcdb", "postgres://dst:pass@dst-host:5432/dstdb"); + await cloneDatabase("postgres://src:pass@src-host:5432/srcdb", "postgres://dst:pass@dst-host:5432/dstdb", false); expect(runPipedCommands).toHaveBeenCalledTimes(1); const [producer, consumer] = vi.mocked(runPipedCommands).mock.calls[0]!; expect(producer.args[0]).toBe("pg_dump"); @@ -144,13 +144,13 @@ describe("database", () => { it("throws on failure", async () => { vi.mocked(runPipedCommands).mockResolvedValue({stdout: "", stderr: "error", exitCode: 1}); await expect( - cloneDatabase("postgres://src:pass@host:5432/src", "postgres://dst:pass@host:5432/dst"), + cloneDatabase("postgres://src:pass@host:5432/src", "postgres://dst:pass@host:5432/dst", false), ).rejects.toThrow("Failed to clone"); }); it("runs psql with -v ON_ERROR_STOP=1 so restore failures surface instead of being silently swallowed", async () => { vi.mocked(runPipedCommands).mockResolvedValue({stdout: "", stderr: "", exitCode: 0}); - await cloneDatabase("postgres://src:pass@src-host:5432/srcdb", "postgres://dst:pass@dst-host:5432/dstdb"); + await cloneDatabase("postgres://src:pass@src-host:5432/srcdb", "postgres://dst:pass@dst-host:5432/dstdb", false); const [, consumer] = vi.mocked(runPipedCommands).mock.calls[0]!; expect(consumer.args).toContain("-v"); expect(consumer.args).toContain("ON_ERROR_STOP=1"); @@ -158,14 +158,14 @@ describe("database", () => { it("passes sanitizeCloneLine as the transformLine argument", async () => { vi.mocked(runPipedCommands).mockResolvedValue({stdout: "", stderr: "", exitCode: 0}); - await cloneDatabase("postgres://src:pass@src-host:5432/srcdb", "postgres://dst:pass@dst-host:5432/dstdb"); + await cloneDatabase("postgres://src:pass@src-host:5432/srcdb", "postgres://dst:pass@dst-host:5432/dstdb", false); const call = vi.mocked(runPipedCommands).mock.calls[0]!; expect(call[2]).toBe(sanitizeCloneLine); }); - it("does not pass --schema-only by default", async () => { + it("omits --schema-only when schemaOnly is false", async () => { vi.mocked(runPipedCommands).mockResolvedValue({stdout: "", stderr: "", exitCode: 0}); - await cloneDatabase("postgres://src:pass@src-host:5432/srcdb", "postgres://dst:pass@dst-host:5432/dstdb"); + await cloneDatabase("postgres://src:pass@src-host:5432/srcdb", "postgres://dst:pass@dst-host:5432/dstdb", false); const [producer] = vi.mocked(runPipedCommands).mock.calls[0]!; expect(producer.args).not.toContain("--schema-only"); });