Add fallback migrations when schema creation is blocked#38
Add fallback migrations when schema creation is blocked#38ponderingdemocritus merged 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughAdded robust database migration handling with a fallback mechanism using a persistent journal to track migrations via hash-based tracking. Extended Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant RunMigrations
participant Migrator
participant FallbackHandler
participant Pool as Database Pool
participant Journal as Journal Table
participant FSys as File System
Client->>RunMigrations: runMigrations(pool, migrateFn?)
RunMigrations->>Migrator: invoke primary migrator
alt Schema Creation Error
Migrator-->>RunMigrations: throws schema error
RunMigrations->>FallbackHandler: invoke runMigrationsFallback
FallbackHandler->>Journal: read migration journal
Journal-->>FallbackHandler: pending migrations list
FallbackHandler->>FSys: read SQL migration files
FSys-->>FallbackHandler: SQL file contents
FallbackHandler->>FallbackHandler: parse & split statements
FallbackHandler->>Pool: begin transaction
loop For each statement
FallbackHandler->>Pool: execute statement
Pool-->>FallbackHandler: success
end
FallbackHandler->>Pool: commit transaction
Pool-->>FallbackHandler: transaction committed
FallbackHandler->>Journal: record migration hash + timestamp
Journal-->>FallbackHandler: confirmed
FallbackHandler-->>RunMigrations: fallback complete
else No Error
Migrator-->>RunMigrations: migrations complete
end
RunMigrations-->>Client: success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@examples/facilitator-server/src/db-migrate.ts`:
- Around line 62-72: The current filter uses strict greater-than (entry.when >
lastApplied) which skips journal entries that share the exact timestamp as the
last applied migration; update the filter logic in the pendingEntries
computation so entries with when equal to lastApplied are not unintentionally
dropped — for example change the predicate to include equals (entry.when >=
lastApplied) or adopt a more robust tie-breaker (e.g., sort by when then by
entry name and include entries where when === lastApplied) when constructing
pendingEntries; update references to migrationState, lastApplied, journalPath,
journal, pendingEntries and DRIZZLE_MIGRATIONS_TABLE accordingly so
same-timestamp journal entries are handled deterministically.
- Around line 50-60: runMigrationsFallback currently creates
${DRIZZLE_MIGRATIONS_TABLE} with only id/hash/created_at which is incompatible
with Drizzle ORM 0.44.0; update the CREATE TABLE in runMigrationsFallback to
define id as "int generated always as identity primary key", add a fourth column
"status varchar" (or "varchar NOT NULL DEFAULT 'applied'" if you want a
default), and then update the migration INSERT (the statement that inserts into
DRIZZLE_MIGRATIONS_TABLE) to include the status column or rely on the default so
every inserted row satisfies Drizzle's expected columns.
In `@examples/facilitator-server/tests/db-migrate.test.ts`:
- Around line 97-122: The test relies on runMigrations' hard-coded
migrationsFolder resolution (resolve(__dirname, "../drizzle")), causing an
implicit dependency on real files; update runMigrations to accept an optional
migrationsFolder path or options object (e.g., add a parameter like
migrationsFolder?: string) and use that instead of resolve(...) inside
runMigrations and any helper like runMigrationsFallback, then update the test to
pass a controlled temporary directory containing deterministic _journal.json and
SQL files so the test is self-contained and does not depend on the repo's
drizzle/ folder.
🧹 Nitpick comments (3)
examples/facilitator-server/src/db-migrate.ts (2)
32-35: Detection is case-insensitive but relies on a specific substring — may miss variant messages.The check uppercases the message and looks for
"CREATE SCHEMA IF NOT EXISTS". This works for the known Drizzle error format, but if Drizzle ever changes its error wording (e.g., omittingIF NOT EXISTS, or localizing the message), this would silently fail and rethrow. Consider adding a broader pattern or a secondary check on the PostgreSQL error code (e.g.,42501for insufficient privilege) if available on the error object.
91-96: ROLLBACK could throw and mask the original migration error.If the connection is broken (e.g., network failure),
client.query("ROLLBACK")will throw, replacing the originalerroron line 93. Wrapping the ROLLBACK in a try/catch is a common defensive practice.🛡️ Proposed defensive ROLLBACK
} catch (error) { - await client.query("ROLLBACK"); + try { + await client.query("ROLLBACK"); + } catch { + // ROLLBACK failed (e.g., broken connection); original error is more useful + } throw error; } finally {examples/facilitator-server/tests/db-migrate.test.ts (1)
21-35: Good coverage for both positive and negative cases.Consider adding a test for non-Error inputs (e.g., a plain string or
null) to exercise thetoErrorMessagefallback path, sinceisSchemaCreateFailureacceptsunknown.
| export async function runMigrationsFallback( | ||
| pool: Pool, | ||
| migrationsFolder: string | ||
| ): Promise<void> { | ||
| await pool.query(` | ||
| CREATE TABLE IF NOT EXISTS ${DRIZZLE_MIGRATIONS_TABLE} ( | ||
| id SERIAL PRIMARY KEY, | ||
| hash text NOT NULL, | ||
| created_at bigint | ||
| ) | ||
| `); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the db-migrate.ts file to understand the context
find . -name "db-migrate.ts" -type f | head -20Repository: daydreamsai/facilitator
Length of output: 114
🌐 Web query:
Drizzle ORM 0.44.0 __drizzle_migrations table schema columns
💡 Result:
In Drizzle ORM 0.44.0, the __drizzle_migrations table has these columns:
idhashcreated_atstatus(tracks e.g.failed/applied/rollbacked) [1]
Dialect-specific schema (as defined by Drizzle’s migration process update):
PostgreSQL
CREATE TABLE __drizzle_migrations (
id int generated always as identity,
hash text not null,
created_at bigint,
status varchar
);[1]
MySQL
CREATE TABLE __drizzle_migrations (
id serial,
hash text not null,
created_at bigint,
status varchar
);[1]
SQLite
CREATE TABLE __drizzle_migrations (
id integer primary key,
hash text not null,
created_at numeric,
status text
);[1]
By default, executed migrations are stored in the __drizzle_migrations table (and on PostgreSQL, by default under the drizzle schema unless configured otherwise). [2]
Sources: [1] [2]
🏁 Script executed:
cat -n ./examples/facilitator-server/src/db-migrate.tsRepository: daydreamsai/facilitator
Length of output: 4398
Fix __drizzle_migrations table schema—missing status column required by Drizzle ORM 0.44.0.
The fallback creates a 3-column table but Drizzle ORM 0.44.0 expects 4 columns: id, hash, created_at, and status. The status column (type varchar) tracks migration state (applied, failed, rollbacked). Without it, the schema is incompatible with Drizzle's expectations. Additionally, for PostgreSQL, id should be int generated always as identity instead of SERIAL PRIMARY KEY. The INSERT on line 87 also needs to populate the status column (or specify a default). The hash algorithm (SHA-256 on line 78) is correct.
🤖 Prompt for AI Agents
In `@examples/facilitator-server/src/db-migrate.ts` around lines 50 - 60,
runMigrationsFallback currently creates ${DRIZZLE_MIGRATIONS_TABLE} with only
id/hash/created_at which is incompatible with Drizzle ORM 0.44.0; update the
CREATE TABLE in runMigrationsFallback to define id as "int generated always as
identity primary key", add a fourth column "status varchar" (or "varchar NOT
NULL DEFAULT 'applied'" if you want a default), and then update the migration
INSERT (the statement that inserts into DRIZZLE_MIGRATIONS_TABLE) to include the
status column or rely on the default so every inserted row satisfies Drizzle's
expected columns.
| const migrationState = await pool.query<{ created_at: string | number }>( | ||
| `SELECT created_at FROM ${DRIZZLE_MIGRATIONS_TABLE} ORDER BY created_at DESC LIMIT 1` | ||
| ); | ||
| const lastApplied = Number(migrationState.rows[0]?.created_at ?? 0); | ||
|
|
||
| const journalPath = resolve(migrationsFolder, "meta", "_journal.json"); | ||
| const journalContent = await readFile(journalPath, "utf8"); | ||
| const journal = JSON.parse(journalContent) as MigrationJournal; | ||
| const pendingEntries = [...journal.entries] | ||
| .sort((a, b) => a.when - b.when) | ||
| .filter((entry) => entry.when > lastApplied); |
There was a problem hiding this comment.
Filtering by strict > on when assumes each journal entry has a unique timestamp.
If two journal entries share the same when value, only the first will be applied — the second would be filtered out on subsequent runs because lastApplied would equal its when. Drizzle journals typically have unique timestamps, so this is likely fine in practice, but worth noting as a latent edge case.
🤖 Prompt for AI Agents
In `@examples/facilitator-server/src/db-migrate.ts` around lines 62 - 72, The
current filter uses strict greater-than (entry.when > lastApplied) which skips
journal entries that share the exact timestamp as the last applied migration;
update the filter logic in the pendingEntries computation so entries with when
equal to lastApplied are not unintentionally dropped — for example change the
predicate to include equals (entry.when >= lastApplied) or adopt a more robust
tie-breaker (e.g., sort by when then by entry name and include entries where
when === lastApplied) when constructing pendingEntries; update references to
migrationState, lastApplied, journalPath, journal, pendingEntries and
DRIZZLE_MIGRATIONS_TABLE accordingly so same-timestamp journal entries are
handled deterministically.
| describe("runMigrations", () => { | ||
| it("falls back to SQL file execution when Drizzle schema bootstrap fails", async () => { | ||
| const clientQuery = mock(async () => ({ rows: [] as unknown[] })); | ||
| const pool = { | ||
| query: mock(async (sql: string) => { | ||
| if (sql.includes("SELECT created_at")) { | ||
| return { rows: [] as unknown[] }; | ||
| } | ||
| return { rows: [] as unknown[] }; | ||
| }), | ||
| connect: mock(async () => ({ | ||
| query: clientQuery, | ||
| release: () => {}, | ||
| })), | ||
| }; | ||
|
|
||
| await runMigrations( | ||
| pool as any, | ||
| mock(async () => { | ||
| throw new Error('Failed query: CREATE SCHEMA IF NOT EXISTS "public"'); | ||
| }) as any | ||
| ); | ||
|
|
||
| expect(pool.connect).toHaveBeenCalledTimes(1); | ||
| expect(clientQuery).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
This test has an implicit dependency on real migration files in the drizzle/ folder.
runMigrations resolves migrationsFolder to resolve(__dirname, "../drizzle") internally. When the fallback is triggered, it reads the journal and SQL files from that path. This means the test will break if:
- The
drizzle/directory doesn't exist or is empty - The migration files are renamed or reorganized
- The journal's
whenvalues change relative to what's already "applied" (empty mock returns{ rows: [] })
Consider extracting the migrationsFolder resolution or making it configurable so the test can use a controlled temp directory (similar to the runMigrationsFallback test above), making it fully self-contained.
#!/bin/bash
# Check if the drizzle folder and journal exist in the expected location
fd -t d "drizzle" --full-path "examples/facilitator-server"
fd "_journal.json" --full-path "examples/facilitator-server"
# Also check for SQL migration files
fd -e sql --full-path "examples/facilitator-server/drizzle"🤖 Prompt for AI Agents
In `@examples/facilitator-server/tests/db-migrate.test.ts` around lines 97 - 122,
The test relies on runMigrations' hard-coded migrationsFolder resolution
(resolve(__dirname, "../drizzle")), causing an implicit dependency on real
files; update runMigrations to accept an optional migrationsFolder path or
options object (e.g., add a parameter like migrationsFolder?: string) and use
that instead of resolve(...) inside runMigrations and any helper like
runMigrationsFallback, then update the test to pass a controlled temporary
directory containing deterministic _journal.json and SQL files so the test is
self-contained and does not depend on the repo's drizzle/ folder.
1 participant
This adds a migration fallback path for environments where Drizzle cannot run CREATE SCHEMA. On schema-bootstrap failure, the server now applies pending SQL migration files directly and records progress in public.__drizzle_migrations. It preserves existing behavior by rethrowing non-schema migration errors. Tests were expanded to cover schema failure detection, fallback execution, and non-schema error propagation.
Summary by CodeRabbit
New Features
Tests