Fix database migration with Drizzle Kit #3
Conversation
## Changes 1. **Fix migration path resolution** - Changed from relative path `./server/db/migrations` to absolute path - Uses `fileURLToPath` and `dirname` for reliable path resolution - Ensures migrations work in production environments (Railway) 2. **Remove duplicate migration files** - Deleted `0030_fix_uuid_defaults.sql` (duplicate of 0030_far_silver_fox) - Deleted `0031_add_row_level_security.sql` (duplicate of 0031_stormy_black_panther) - Cleans up migration journal to prevent conflicts 3. **Improve migration logging** - Added detailed startup logging with migrations folder path - Added database URL logging (with password redacted) - Enhanced error logging with migration folder info for debugging ## Why This Fixes Railway Deployment The original script used a relative path that failed in production because: - Working directory may differ in containerized environments - Relative paths are unreliable during Railway builds - Absolute paths ensure the script finds migrations regardless of cwd ## Testing Verified migration script works correctly with proper error handling: - ✅ Modules load successfully (drizzle-orm, postgres) - ✅ Migrations folder resolves to absolute path - ✅ Database connection attempts correctly - ✅ Error logging provides debugging information
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR refactors the migration runner (path resolution, env validation, logging, suppressing PG notices, richer error handling) and deletes two SQL migration files that previously set UUID defaults and added row-level security policies. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as migrate.ts
participant FS as Filesystem
participant PG as Postgres (migrate client)
Runner->>FS: resolve __filename/__dirname -> migrationsFolder
Runner->>Runner: validate DATABASE_URL (exit on missing)
Runner->>Runner: mask DATABASE_URL, logger.info(startup)
Runner->>PG: create client with onnotice:() => {}
Runner->>PG: run migrate(migrationsFolder)
alt success
PG-->>Runner: migration result
Runner->>PG: client.end()
Runner->>Runner: process.exit(0)
else failure
PG-->>Runner: error (with cause)
Runner->>Runner: derive errorCode/message, detect "already exists"
Runner->>PG: client.end()
Runner->>Runner: logger.error(...), process.exit(1)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
Summary of ChangesHello @Dexploarer, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on improving the robustness and reliability of database migrations, particularly for production deployments. It addresses issues with migration path resolution, streamlines the migration history by removing duplicates, and enhances logging to provide clearer insights into the migration process and any potential failures. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| ); | ||
| process.exit(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
Resource Management: Potential Unclosed Database Connection
If an error occurs before migrationClient.end() is called (for example, if an exception is thrown before or during the migration process), the database connection may remain open. To ensure proper resource cleanup, consider using a finally block to always close the connection, regardless of whether an error occurs:
try {
await migrate(db, { migrationsFolder });
logger.info({}, "[Migrations] ✓ Migrations completed successfully");
} catch (error: any) {
// ... error handling ...
} finally {
await migrationClient.end();
}This guarantees that the connection is closed even if an error is thrown.
PR Compliance Guide 🔍(Compliance updated until commit c33dc2c)Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label Previous compliance checksCompliance check up to commit 5a4252d
|
||||||||||||||||||||||||||||||||||||||||||||||
Greptile OverviewGreptile SummaryThis PR fixes critical database migration issues that were preventing deployments on Railway. The main change switches from relative path resolution ( Important Files Changed
Confidence score: 5/5
Sequence DiagramsequenceDiagram
participant "Railway Deploy" as Railway
participant "Migration Script" as Script
participant "File System" as FS
participant "Database" as DB
participant "Logger" as Log
Railway->>Script: "Execute migrate.ts"
Script->>Script: "Get absolute path using fileURLToPath + dirname"
Script->>FS: "Check migrations folder exists"
FS-->>Script: "Path resolved successfully"
Script->>Log: "Log migration info with redacted DB URL"
Script->>DB: "Connect with postgres client"
DB-->>Script: "Connection established"
Script->>DB: "Run migrate() with absolute migrationsFolder"
alt Migration successful
DB-->>Script: "Migrations completed"
Script->>Log: "Log success message"
else Tables already exist (42P07 error)
DB-->>Script: "PostgreSQL error 42P07"
Script->>Log: "Log warning about existing tables"
Script->>Log: "Log schema up to date"
else Other migration error
DB-->>Script: "Other error"
Script->>Log: "Log error with details"
Script->>Railway: "Exit with code 1"
end
Script->>DB: "Close connection"
Script->>Railway: "Exit with code 0"
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
PR Code Suggestions ✨
Explore these optional code suggestions:
|
|||||||||
CI Feedback 🧐(Feedback updated until commit 5a4252d)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes the database migration script by using an absolute path for the migrations folder, which is crucial for containerized environments. It also cleans up duplicate migration files and improves logging. My main feedback is to improve the password redaction logic for the database URL in the logs. The current regex-based approach can fail if the password contains special characters like '@', potentially leaking parts of the password. Using the built-in URL constructor is a more robust and secure method for this.
![codiumai-pr-agent-free[bot]](https://avatars.githubusercontent.com/in/354216?s=60&v=4){kind=small}
| migrationsFolder, | ||
| databaseUrl: process.env.DATABASE_URL?.replace(/:[^:@]+@/, ":****@"), // Hide password | ||
| }, |
There was a problem hiding this comment.
Suggestion: Remove redundant optional chaining operator
| migrationsFolder, | |
| databaseUrl: process.env.DATABASE_URL?.replace(/:[^:@]+@/, ":****@"), // Hide password | |
| }, | |
| migrationsFolder, | |
| databaseUrl: process.env.DATABASE_URL.replace(/:[^:@]+@/, ":****@"), // Hide password | |
| }, |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/core/server/db/migrate.ts (2)
32-39: Startup logging is helpful; URL masking is good but could be made more robustLogging
migrationsFolderplus a password-redactedDATABASE_URLis very useful operationally, and the current regex works for standardpostgres://user:pass@host/...URLs. If you anticipate supporting less conventional DSN formats (e.g., credentials in query params), consider switching tonew URL()and explicitly blankingurl.password/ removing anypasswordquery param before logging as a future hardening step.
68-76: Structured error logging is strong; consider narrowing the “already exists” heuristicIncluding
err,code,message, andmigrationsFolderin the error log gives you the right context to debug failed migrations. One minor concern is that the earlierisAlreadyExistsErrorcheck also relies onerrorMessage.includes("already exists"), which might, in rare cases, classify non-42P07 errors as ignorable. If you later run into masked failures, consider tightening that logic (e.g., preferringerrorCodewhen available and being more specific with the message match) while keeping this structured logging as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/core/server/db/migrate.ts(4 hunks)apps/core/server/db/migrations/0030_fix_uuid_defaults.sql(0 hunks)apps/core/server/db/migrations/0031_add_row_level_security.sql(0 hunks)
💤 Files with no reviewable changes (2)
- apps/core/server/db/migrations/0031_add_row_level_security.sql
- apps/core/server/db/migrations/0030_fix_uuid_defaults.sql
🧰 Additional context used
🧬 Code graph analysis (1)
apps/core/server/db/migrate.ts (1)
apps/core/server/db/db.ts (1)
db(95-98)
🔇 Additional comments (3)
apps/core/server/db/migrate.ts (3)
4-4: Header comment accurately documents Bun behaviorThe clarification that Bun auto-loads
.envfiles matches the current setup and avoids unnecessarydotenvusage. No changes needed here.
10-18: AbsolutemigrationsFolderresolution is a solid improvement; be mindful of build layoutUsing
import.meta.url+fileURLToPath+dirnameto compute an absolutemigrationsFolderremoves CWD sensitivity and should behave reliably in environments like Railway. This does assume that themigrationsdirectory is deployed as a sibling of this file (e.g., if you ever move to a compiled/bundleddistlayout, the SQL files will also need to live alongside the compiled script).
53-53: Using the computedmigrationsFolderinmigrate()is correctPassing the
migrationsFolderconstant intomigrate(db, { migrationsFolder })ties the runtime configuration to the resolved absolute path and eliminates the previous hard-coded relative path issue.
User description
Changes
Fix migration path resolution
./server/db/migrationsto absolute pathfileURLToPathanddirnamefor reliable path resolutionRemove duplicate migration files
0030_fix_uuid_defaults.sql(duplicate of 0030_far_silver_fox)0031_add_row_level_security.sql(duplicate of 0031_stormy_black_panther)Improve migration logging
Why This Fixes Railway Deployment
The original script used a relative path that failed in production because:
Testing
Verified migration script works correctly with proper error handling:
PR Type
Bug fix
Description
Fix migration path resolution using absolute paths for Railway deployment
Remove duplicate migration files (0030 and 0031) to prevent conflicts
Improve migration logging with folder path and redacted database URL
Remove unnecessary dotenv import as Bun auto-loads .env files
Diagram Walkthrough
File Walkthrough
migrate.ts
Fix migration path resolution and improve loggingapps/core/server/db/migrate.ts
fileURLToPathand
dirnamedotenv/configimport as Bun auto-loads environment filesdatabase URL
for better debugging
0030_fix_uuid_defaults.sql
Remove duplicate UUID defaults migrationapps/core/server/db/migrations/0030_fix_uuid_defaults.sql
0031_add_row_level_security.sql
Remove duplicate row-level security migrationapps/core/server/db/migrations/0031_add_row_level_security.sql
policies
creation
Summary by CodeRabbit
Revert
Chores
✏️ Tip: You can customize this high-level summary in your review settings.