-
Notifications
You must be signed in to change notification settings - Fork 3.8k
fix(db): make development schema pushes noninteractive #7906
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,9 @@ | ||
| /** Matches the main migrator: an empty optional direct DSN falls back to DATABASE_URL. */ | ||
| export function resolveMigrationDatabaseUrl( | ||
| env: { MIGRATION_DATABASE_URL?: string; DATABASE_URL?: string } = process.env | ||
| env: { MIGRATION_DATABASE_URL?: string; DATABASE_URL?: string } = { | ||
| MIGRATION_DATABASE_URL: process.env.MIGRATION_DATABASE_URL, | ||
| DATABASE_URL: process.env.DATABASE_URL, | ||
| } | ||
| ): string | undefined { | ||
| return env.MIGRATION_DATABASE_URL || env.DATABASE_URL | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,187 @@ | ||
| import { spawnSync } from 'node:child_process' | ||
| import { mkdtemp, rm, writeFile } from 'node:fs/promises' | ||
| import { tmpdir } from 'node:os' | ||
| import { join } from 'node:path' | ||
| import { generateId } from '@sim/utils/id' | ||
| import postgres, { type Sql } from 'postgres' | ||
| import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest' | ||
|
|
||
| const databaseUrl = process.env.DB_PUSH_TEST_DATABASE_URL | ||
|
|
||
| describe.skipIf(!databaseUrl)('patched Drizzle push against PostgreSQL', () => { | ||
| const databaseName = `push_policy_${generateId().replaceAll('-', '')}` | ||
| let admin: Sql | ||
| let sql: Sql | ||
| let fixtureUrl: string | ||
| let directory: string | ||
|
|
||
| beforeAll(async () => { | ||
| admin = postgres(databaseUrl!, { max: 1, onnotice: () => {} }) | ||
| await admin`CREATE DATABASE ${admin(databaseName)}` | ||
| const url = new URL(databaseUrl!) | ||
| url.pathname = `/${databaseName}` | ||
| fixtureUrl = url.toString() | ||
| sql = postgres(fixtureUrl, { max: 1, onnotice: () => {} }) | ||
| directory = await mkdtemp(join(tmpdir(), 'push-policy-')) | ||
| await writeFile( | ||
| join(directory, 'drizzle.config.ts'), | ||
| `export default { | ||
| dialect: 'postgresql', | ||
| schema: ${JSON.stringify(join(directory, 'schema.ts'))}, | ||
| schemaFilter: ['public', 'old_scope', 'new_scope'], | ||
| tablesFilter: ['!script_migrations'], | ||
| dbCredentials: { url: process.env.DATABASE_URL }, | ||
| }` | ||
| ) | ||
| }) | ||
|
|
||
| beforeEach(async () => { | ||
| await sql`DROP SCHEMA IF EXISTS old_scope CASCADE` | ||
| await sql`DROP SCHEMA IF EXISTS new_scope CASCADE` | ||
| await sql`DROP SCHEMA public CASCADE` | ||
| await sql`CREATE SCHEMA public` | ||
| }) | ||
|
|
||
| afterAll(async () => { | ||
| await sql?.end() | ||
| if (admin) { | ||
| await admin`DROP DATABASE IF EXISTS ${admin(databaseName)}` | ||
| await admin.end() | ||
| } | ||
| if (directory) await rm(directory, { recursive: true, force: true }) | ||
| }) | ||
|
|
||
| async function schema(source: string) { | ||
| await writeFile( | ||
| join(directory, 'schema.ts'), | ||
| `import { pgTable, pgSchema, pgEnum, text, integer, boolean, check } from ${JSON.stringify(import.meta.resolve('drizzle-orm/pg-core'))} | ||
| import { sql } from ${JSON.stringify(import.meta.resolve('drizzle-orm'))} | ||
| ${source}` | ||
| ) | ||
| } | ||
|
|
||
| /** Exercise the patched CLI with pipes, never a terminal or canned prompt answers. */ | ||
| function push(args = ['--force'], renameMode: string | undefined = 'create') { | ||
| return spawnSync( | ||
| 'bunx', | ||
| [ | ||
| '--no-install', | ||
| 'drizzle-kit', | ||
| 'push', | ||
| '--config', | ||
| join(directory, 'drizzle.config.ts'), | ||
| ...args, | ||
| ], | ||
| { | ||
| env: { ...process.env, DATABASE_URL: fixtureUrl, SIM_DB_PUSH_RENAME_MODE: renameMode }, | ||
| encoding: 'utf8', | ||
| timeout: 30_000, | ||
| } | ||
| ) | ||
| } | ||
|
|
||
| async function legacyColumns() { | ||
| await sql`CREATE TABLE records (id text PRIMARY KEY, old_label text, old_enabled boolean)` | ||
| await sql`INSERT INTO records VALUES ('existing', 'original value', true)` | ||
| await schema(`export const records = pgTable('records', { | ||
| id: text('id').primaryKey(), | ||
| newLabel: text('new_label').default('new default'), | ||
| newEnabled: boolean('new_enabled').notNull().default(false), | ||
| })`) | ||
| } | ||
|
|
||
| it('initializes a fresh database', async () => { | ||
| await schema(`export const records = pgTable('records', { | ||
| id: text('id').primaryKey(), enabled: boolean('enabled').notNull().default(false), | ||
| })`) | ||
| const result = push() | ||
| expect(result.error).toBeUndefined() | ||
| expect(result.status).toBe(0) | ||
| await sql`INSERT INTO records (id) VALUES ('new-row')` | ||
| expect(await sql`SELECT * FROM records`).toEqual([{ id: 'new-row', enabled: false }]) | ||
| }, 30_000) | ||
|
|
||
| it('creates independent columns across ambiguous pairs and can be rerun', async () => { | ||
| await legacyColumns() | ||
| const result = push() | ||
| expect(result.error).toBeUndefined() | ||
| expect(result.status).toBe(0) | ||
| expect(result.stdout + result.stderr).not.toContain( | ||
| 'Interactive prompts require a TTY terminal' | ||
| ) | ||
| expect(await sql`SELECT * FROM records`).toEqual([ | ||
| { id: 'existing', new_label: 'new default', new_enabled: false }, | ||
| ]) | ||
| const repeated = push() | ||
| expect(repeated.status).toBe(0) | ||
| expect(repeated.stdout).toContain('No changes detected') | ||
| }, 60_000) | ||
|
|
||
| it('creates independent tables and enums while preserving the excluded script ledger', async () => { | ||
| await sql`CREATE TYPE old_status AS ENUM ('active')` | ||
| await sql`CREATE TABLE old_records (id text PRIMARY KEY, status old_status)` | ||
| await sql`INSERT INTO old_records VALUES ('old-row', 'active')` | ||
| await sql`CREATE TABLE script_migrations (name text PRIMARY KEY)` | ||
| await sql`INSERT INTO script_migrations VALUES ('completed-fixture-migration')` | ||
| await schema(`export const status = pgEnum('new_status', ['active']) | ||
| export const records = pgTable('new_records', { id: text('id').primaryKey(), status: status('status') })`) | ||
| const result = push() | ||
| expect(result.error).toBeUndefined() | ||
| expect(result.status).toBe(0) | ||
| expect(await sql`SELECT * FROM new_records`).toEqual([]) | ||
| expect( | ||
| await sql`SELECT to_regclass('old_records') AS old_table, to_regtype('old_status') AS old_type` | ||
| ).toEqual([{ old_table: null, old_type: null }]) | ||
| expect(await sql`SELECT * FROM script_migrations`).toEqual([ | ||
| { name: 'completed-fixture-migration' }, | ||
| ]) | ||
| }, 30_000) | ||
|
|
||
| it('creates a new schema instead of moving a removed schema', async () => { | ||
| await sql`CREATE SCHEMA old_scope` | ||
| await sql`CREATE TABLE old_scope.records (id text PRIMARY KEY)` | ||
| await sql`INSERT INTO old_scope.records VALUES ('old-row')` | ||
| await schema(`export const scope = pgSchema('new_scope') | ||
| export const records = scope.table('records', { id: text('id').primaryKey() })`) | ||
| const result = push() | ||
| expect(result.error).toBeUndefined() | ||
| expect(result.status, result.stdout + result.stderr).toBe(0) | ||
| expect(await sql`SELECT * FROM new_scope.records`).toEqual([]) | ||
| expect(await sql`SELECT to_regnamespace('old_scope') AS old_schema`).toEqual([ | ||
| { old_schema: null }, | ||
| ]) | ||
| }, 30_000) | ||
|
|
||
| it('keeps the data-loss approval independent of rename resolution', async () => { | ||
| await legacyColumns() | ||
| const result = push([]) | ||
| expect(result.error).toBeUndefined() | ||
| expect(result.status).toBe(1) | ||
| expect(result.stdout).toContain('Found data-loss statements') | ||
| expect(await sql`SELECT * FROM records`).toEqual([ | ||
| { id: 'existing', old_label: 'original value', old_enabled: true }, | ||
| ]) | ||
| }, 30_000) | ||
|
|
||
| it('retains native rename prompts when the policy is not enabled', async () => { | ||
| await legacyColumns() | ||
| const result = push(['--force'], 'prompt') | ||
| expect(result.error).toBeUndefined() | ||
| expect(result.status).toBe(1) | ||
| expect(result.stdout + result.stderr).toContain('Interactive prompts require a TTY terminal') | ||
| expect(await sql`SELECT old_label FROM records`).toEqual([{ old_label: 'original value' }]) | ||
| }, 30_000) | ||
|
|
||
| it('propagates a database DDL error instead of reporting success', async () => { | ||
| await sql`CREATE TABLE records (id text PRIMARY KEY, value integer)` | ||
| await sql`INSERT INTO records VALUES ('invalid-row', -1)` | ||
| await schema(`export const records = pgTable('records', { | ||
| id: text('id').primaryKey(), value: integer('value'), | ||
| }, (table) => [check('nonnegative_value', sql\`\${table.value} >= 0\`)])`) | ||
| const result = push() | ||
| expect(result.error).toBeUndefined() | ||
| expect(result.status).toBe(1) | ||
| expect(result.stderr).toContain('23514') | ||
| expect(await sql`SELECT value FROM records`).toEqual([{ value: -1 }]) | ||
| }, 30_000) | ||
| }) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| import { runPush } from '@sim/db/scripts/push' | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' | ||
|
|
||
| vi.mock('@sim/logger', () => ({ createLogger: () => ({ info: vi.fn(), error: vi.fn() }) })) | ||
|
|
||
| interface SpawnOptions { | ||
| env?: NodeJS.ProcessEnv | ||
| stdin: string | ||
| stdout: string | ||
| stderr: string | ||
| } | ||
|
|
||
| const spawn = vi.fn<(command: string[], options: SpawnOptions) => { exited: Promise<number> }>() | ||
| const stdinTty = Object.getOwnPropertyDescriptor(process.stdin, 'isTTY') | ||
| const stdoutTty = Object.getOwnPropertyDescriptor(process.stdout, 'isTTY') | ||
|
|
||
| function setTerminal(enabled: boolean) { | ||
| Object.defineProperty(process.stdin, 'isTTY', { configurable: true, value: enabled }) | ||
| Object.defineProperty(process.stdout, 'isTTY', { configurable: true, value: enabled }) | ||
| } | ||
|
|
||
| beforeEach(() => { | ||
| spawn.mockReset().mockImplementation(() => ({ exited: Promise.resolve(0) })) | ||
| vi.stubGlobal('Bun', { spawn }) | ||
| setTerminal(false) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| vi.unstubAllGlobals() | ||
| vi.unstubAllEnvs() | ||
| if (stdinTty) Object.defineProperty(process.stdin, 'isTTY', stdinTty) | ||
| else Reflect.deleteProperty(process.stdin, 'isTTY') | ||
| if (stdoutTty) Object.defineProperty(process.stdout, 'isTTY', stdoutTty) | ||
| else Reflect.deleteProperty(process.stdout, 'isTTY') | ||
| }) | ||
|
|
||
| describe('db:push policy and process boundaries', () => { | ||
| it('sets create/drop only on the Drizzle child and forwards force independently', async () => { | ||
| vi.stubEnv('SIM_DB_PUSH_RENAME_MODE', undefined) | ||
| expect(await runPush(['--force'])).toBe(0) | ||
| expect(spawn).toHaveBeenCalledTimes(4) | ||
| expect(spawn.mock.calls[0][0]).toEqual([ | ||
| 'bunx', | ||
| '--no-install', | ||
| 'drizzle-kit', | ||
| 'push', | ||
| '--config=./drizzle.config.ts', | ||
| '--force', | ||
| ]) | ||
| expect(spawn.mock.calls[0][1].env?.SIM_DB_PUSH_RENAME_MODE).toBe('create') | ||
| expect(process.env.SIM_DB_PUSH_RENAME_MODE).toBeUndefined() | ||
| for (const [, options] of spawn.mock.calls.slice(1)) expect(options.env).toBeUndefined() | ||
| }) | ||
|
|
||
| it('does not implicitly approve data loss', async () => { | ||
| expect(await runPush([])).toBe(0) | ||
| expect(spawn.mock.calls[0][0]).not.toContain('--force') | ||
| }) | ||
|
|
||
| it('stops reconciliation when Drizzle fails', async () => { | ||
| spawn.mockReturnValueOnce({ exited: Promise.resolve(42) }) | ||
| expect(await runPush(['--force'])).toBe(42) | ||
| expect(spawn).toHaveBeenCalledTimes(1) | ||
| }) | ||
|
|
||
| it('stops after the first failed reconciliation', async () => { | ||
| spawn.mockReturnValueOnce({ exited: Promise.resolve(0) }) | ||
| spawn.mockReturnValueOnce({ exited: Promise.resolve(43) }) | ||
| expect(await runPush([])).toBe(43) | ||
| expect(spawn).toHaveBeenCalledTimes(2) | ||
| }) | ||
|
|
||
| it('passes intentional renames to the native chooser in a terminal', async () => { | ||
| setTerminal(true) | ||
| vi.stubEnv('SIM_DB_PUSH_RENAME_MODE', 'create') | ||
| expect(await runPush(['--interactive-renames', '--verbose'])).toBe(0) | ||
| expect(spawn.mock.calls[0][0]).not.toContain('--interactive-renames') | ||
| expect(spawn.mock.calls[0][0]).toContain('--verbose') | ||
| expect(spawn.mock.calls[0][1].env?.SIM_DB_PUSH_RENAME_MODE).toBe('prompt') | ||
| }) | ||
|
|
||
| it('rejects interactive renames without a terminal before any database commands', async () => { | ||
| expect(await runPush(['--interactive-renames', '--force'])).toBe(1) | ||
| expect(spawn).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('does not reconcile the database when requesting help', async () => { | ||
| expect(await runPush(['--help'])).toBe(0) | ||
| expect(spawn).toHaveBeenCalledTimes(1) | ||
| }) | ||
| }) |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.