feat: add ability to apply migrations in throwaway db to test if they apply successfully#676
feat: add ability to apply migrations in throwaway db to test if they apply successfully#676pieh wants to merge 7 commits into
Conversation
… apply successfully
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR introduces a comprehensive typed database migration system with enhanced error handling and validation. Key changes include refactoring migrations.ts with stricter name/version pattern validation, new error classes (MissingMigrationDirectoryError, DuplicateMigrationVersionsError, MigrationsApplyError, MigrationFileError), and a MigrationIssue union type for detailed error reporting. The Migration interface now includes a version field extracted from naming conventions. New applyMigrationsWithDetails function provides detailed migration results with transaction handling. A new testMigrationsInEphemeralDatabase utility enables end-to-end testing against ephemeral databases. Test files are updated with new validation scenarios and error message expectations. Public APIs across multiple packages are expanded to export new types and utilities. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Detailed review notesmigrations.ts (primary complexity)
Test coverage additions
API surface changes
Cross-file dependencies
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
| } | ||
|
|
||
| export async function applyMigrations( | ||
| export class MissingMigrationDirectoryError extends Error { |
There was a problem hiding this comment.
Adding few specific error classes to help with programmatic handling of some specific errors that can be thrown during applyMigrations runs.
For the most part the user-facing output will change slightly, but should contain pretty much same information as before and those specific errors just have more structured data for programmatic handling
| export async function applyMigrations(...args: Parameters<typeof applyMigrationsWithDetails>): Promise<string[]> { | ||
| const migrations = await applyMigrationsWithDetails(...args) | ||
| return migrations.map((m) => m.name) | ||
| } |
There was a problem hiding this comment.
I did make "inner" applyMigrations return more details about applied migrations and not just name and to preserve signature for existing consumers adding this small adapter.
…e problems ahead of time
…c with netlify/build migration name matching
![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
🧹 Nitpick comments (1)
packages/database/dev/src/lib/migrations.ts (1)
254-263: ⚡ Quick win
MigrationsApplyError.causeoverrides thesuper()cause and drops Postgres context.The constructor sets
causetwice:super(..., { cause: options.issue.pgError })attaches the richPgErrorDetails, thenthis.cause = new Error(options.issue.pgError.message)replaces it with a bareErrorwhose only data is the message —code,position,hint, etc. are lost from the cause chain (they're still reachable viaerror.issue.pgError, buterror.causeis a strictly worse object). The explicitpublic readonly cause: Errorfield also shadows the inheritedError.cause: unknown, which is what makes the second assignment necessary in the first place.Either drop the field declaration and rely on the
super()cause, or drop thesuper()cause arg and assign the original error directly — the current double-assignment doesn't add value.♻️ Suggested simplification
export class MigrationsApplyError extends Error { public readonly issue: MigrationIssue & { kind: 'apply-failure' } - public readonly cause: Error constructor(options: { issue: MigrationIssue & { kind: 'apply-failure' } }) { super(`${options.issue.summary}\n${options.issue.remediation}`, { cause: options.issue.pgError }) this.name = 'MigrationsApplyError' this.issue = options.issue - this.cause = new Error(options.issue.pgError.message) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/database/dev/src/lib/migrations.ts` around lines 254 - 263, The constructor is currently attaching the rich PgErrorDetails via super(..., { cause: options.issue.pgError }) but then overrides it with this.cause = new Error(...), losing Postgres context; remove the explicit public readonly cause: Error property and the subsequent this.cause = new Error(...) assignment in MigrationsApplyError so the inherited Error.cause (set by super) retains the original options.issue.pgError; keep the super(...) cause argument and assign only this.issue and this.name in the constructor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/database/dev/src/lib/migrations.ts`:
- Around line 310-317: The catch around readdir is too broad; only convert a
missing-directory error into MissingMigrationDirectoryError. Change the catch in
the block that calls readdir(...)/direntToMigration to inspect the caught error
(as NodeJS.ErrnoException) and if error.code === 'ENOENT' (and optionally
'ENOTDIR' if you want to treat non-directory as missing) throw new
MissingMigrationDirectoryError({ migrationsDirectory, cause: error }); otherwise
rethrow the original error so errorToIssues/testMigrationsInEphemeralDatabase
can surface it as unknown/unreadable instead of mapping it to an empty success.
- Around line 414-421: The failure builds the "remaining" list from
migrationsToConsider which still contains migrations that were skipped as
already applied; instead compute the remaining migrations from the filtered list
that actually got considered for application (or compute the failing migration's
index within that filtered list) and pass filtered.slice(failingIndex + 1) into
buildApplyFailureIssue so the failing migration isn't duplicated in "remaining"
(refer to MigrationsApplyError, buildApplyFailureIssue, migration, applied,
migrationsToConsider); also add a test that pre-applies some migrations and then
calls applyMigrations a second time with a tail migration that fails to assert
the remaining list is correct.
In `@packages/database/dev/src/main.ts`:
- Around line 246-249: The current branch treats any empty array from
errorToIssues as a successful "no migrations" case which masks real upstream
I/O/permission errors; change the logic so that errorToIssues emits a specific
sentinel (or throws a specific MissingMigrationsDirectory error) for the
missing-directory case and only map that sentinel to { status: 'success',
applied: [] } here in applyMigrationsWithDetails; otherwise rethrow or propagate
the original error (or return a failure result) so genuine ENOENT/EACCES/IO
errors are not misclassified. Ensure you modify or check for the unique
identifier returned by errorToIssues (e.g., an issue with type
'missing-directory' or a MissingMigrationsDirectory class) rather than using
issues.length === 0, and update callers/tests accordingly.
---
Nitpick comments:
In `@packages/database/dev/src/lib/migrations.ts`:
- Around line 254-263: The constructor is currently attaching the rich
PgErrorDetails via super(..., { cause: options.issue.pgError }) but then
overrides it with this.cause = new Error(...), losing Postgres context; remove
the explicit public readonly cause: Error property and the subsequent this.cause
= new Error(...) assignment in MigrationsApplyError so the inherited Error.cause
(set by super) retains the original options.issue.pgError; keep the super(...)
cause argument and assign only this.issue and this.name in the constructor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fad44527-c168-43cd-8ccb-4430ecc65934
📒 Files selected for processing (5)
packages/database/dev/src/lib/migrations.test.tspackages/database/dev/src/lib/migrations.tspackages/database/dev/src/lib/testMigrationsInEphemeralDatabase.test.tspackages/database/dev/src/main.tspackages/dev/src/main.ts
| try { | ||
| const dirents = await readdir(migrationsDirectory, { withFileTypes: true }) | ||
| migrations = dirents | ||
| .map((entry) => toMigration(entry, migrationsDirectory)) | ||
| .map((entry) => direntToMigration(entry, migrationsDirectory)) | ||
| .filter((m): m is Migration => m !== null) | ||
| } catch { | ||
| throw new Error(`Migration directory not found: ${migrationsDirectory}`) | ||
| } catch (error) { | ||
| throw new MissingMigrationDirectoryError({ migrationsDirectory, cause: error }) | ||
| } |
There was a problem hiding this comment.
Catch is too broad — non-ENOENT errors get reported as “success: applied=[]”.
readdir can fail with EACCES, ENOTDIR, EMFILE, etc. All of those are currently translated to MissingMigrationDirectoryError, which errorToIssues (Lines 275-277) maps to an empty issues array, which testMigrationsInEphemeralDatabase then turns into { status: 'success', applied: [] }. A user with a permission-denied or otherwise-unreadable migrations dir will be told everything is fine.
Narrow the wrapping to the missing-directory case and let the rest surface as unknown issues (or as a distinct 'unreadable' case).
🛡️ Suggested guard
try {
const dirents = await readdir(migrationsDirectory, { withFileTypes: true })
migrations = dirents
.map((entry) => direntToMigration(entry, migrationsDirectory))
.filter((m): m is Migration => m !== null)
} catch (error) {
- throw new MissingMigrationDirectoryError({ migrationsDirectory, cause: error })
+ if ((error as NodeJS.ErrnoException)?.code === 'ENOENT') {
+ throw new MissingMigrationDirectoryError({ migrationsDirectory, cause: error })
+ }
+ throw error
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const dirents = await readdir(migrationsDirectory, { withFileTypes: true }) | |
| migrations = dirents | |
| .map((entry) => toMigration(entry, migrationsDirectory)) | |
| .map((entry) => direntToMigration(entry, migrationsDirectory)) | |
| .filter((m): m is Migration => m !== null) | |
| } catch { | |
| throw new Error(`Migration directory not found: ${migrationsDirectory}`) | |
| } catch (error) { | |
| throw new MissingMigrationDirectoryError({ migrationsDirectory, cause: error }) | |
| } | |
| try { | |
| const dirents = await readdir(migrationsDirectory, { withFileTypes: true }) | |
| migrations = dirents | |
| .map((entry) => direntToMigration(entry, migrationsDirectory)) | |
| .filter((m): m is Migration => m !== null) | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException)?.code === 'ENOENT') { | |
| throw new MissingMigrationDirectoryError({ migrationsDirectory, cause: error }) | |
| } | |
| throw error | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/database/dev/src/lib/migrations.ts` around lines 310 - 317, The
catch around readdir is too broad; only convert a missing-directory error into
MissingMigrationDirectoryError. Change the catch in the block that calls
readdir(...)/direntToMigration to inspect the caught error (as
NodeJS.ErrnoException) and if error.code === 'ENOENT' (and optionally 'ENOTDIR'
if you want to treat non-directory as missing) throw new
MissingMigrationDirectoryError({ migrationsDirectory, cause: error }); otherwise
rethrow the original error so errorToIssues/testMigrationsInEphemeralDatabase
can surface it as unknown/unreadable instead of mapping it to an empty success.
| throw new MigrationsApplyError({ | ||
| issue: buildApplyFailureIssue({ | ||
| migration, | ||
| appliedBefore: applied, | ||
| remaining: migrationsToConsider.slice(applied.length + 1), | ||
| cause: error, | ||
| }), | ||
| }) |
There was a problem hiding this comment.
remaining is computed against the wrong list when some migrations were already applied.
applied only counts migrations applied in this invocation, but migrationsToConsider also contains migrations that had already been applied in a previous run and were skipped at Lines 385-388. migrationsToConsider.slice(applied.length + 1) therefore over-includes (and can include the failing migration itself) whenever the tracking table already has rows.
Example: migrationsToConsider = [m1 (already applied), m2, m3], m2 fails → applied = [], so remaining = migrationsToConsider.slice(1) = [m2, m3], but the intent is [m3].
Use the filtered list (or the failing migration's index) instead.
🐛 Suggested fix
- const applied: Migration[] = []
- for (const { migration, sql } of migrationsToApplyWithContent) {
+ const applied: Migration[] = []
+ for (let i = 0; i < migrationsToApplyWithContent.length; i++) {
+ const { migration, sql } = migrationsToApplyWithContent[i]
try {
await db.transaction(async (tx) => {
await tx.exec(sql)
await tx.query(`INSERT INTO ${TRACKING_TABLE} (name) VALUES ($1)`, [migration.name])
})
} catch (error) {
throw new MigrationsApplyError({
issue: buildApplyFailureIssue({
migration,
appliedBefore: applied,
- remaining: migrationsToConsider.slice(applied.length + 1),
+ remaining: migrationsToApplyWithContent.slice(i + 1).map((entry) => entry.migration),
cause: error,
}),
})
}The existing apply-failure tests don't pre-apply any migrations, so this case isn't covered — adding one that calls applyMigrations twice (with a failing tail migration on the second call) would lock this in.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/database/dev/src/lib/migrations.ts` around lines 414 - 421, The
failure builds the "remaining" list from migrationsToConsider which still
contains migrations that were skipped as already applied; instead compute the
remaining migrations from the filtered list that actually got considered for
application (or compute the failing migration's index within that filtered list)
and pass filtered.slice(failingIndex + 1) into buildApplyFailureIssue so the
failing migration isn't duplicated in "remaining" (refer to
MigrationsApplyError, buildApplyFailureIssue, migration, applied,
migrationsToConsider); also add a test that pre-applies some migrations and then
calls applyMigrations a second time with a tail migration that fails to assert
the remaining list is correct.
| const issues = errorToIssues(error) | ||
| if (issues.length === 0) { | ||
| return { status: 'success', applied: [] } | ||
| } |
There was a problem hiding this comment.
Empty-issues fallback masks upstream classification bugs.
Mapping zero issues to { status: 'success', applied: [] } is fine for the missing-directory case, but it relies on errorToIssues returning [] only for that specific case. Combined with the over-broad catch in applyMigrationsWithDetails (see comment on migrations.ts Lines 310-317), permission-denied / I/O failures on the migrations directory currently surface here as success. Once the upstream catch is narrowed, this branch is correct as-is.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/database/dev/src/main.ts` around lines 246 - 249, The current branch
treats any empty array from errorToIssues as a successful "no migrations" case
which masks real upstream I/O/permission errors; change the logic so that
errorToIssues emits a specific sentinel (or throws a specific
MissingMigrationsDirectory error) for the missing-directory case and only map
that sentinel to { status: 'success', applied: [] } here in
applyMigrationsWithDetails; otherwise rethrow or propagate the original error
(or return a failure result) so genuine ENOENT/EACCES/IO errors are not
misclassified. Ensure you modify or check for the unique identifier returned by
errorToIssues (e.g., an issue with type 'missing-directory' or a
MissingMigrationsDirectory class) rather than using issues.length === 0, and
update callers/tests accordingly.
2 participants
TODO: