feat(api): postgres schema migrations via Flyway - #40
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds a versioned ChangesDatabase foundation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant postgres
participant flyway
participant api
postgres->>flyway: report healthy
flyway->>postgres: execute V1__profiles.sql
flyway-->>api: signal successful completion
api->>postgres: start after migration completion
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
markus-azer
changed the title
There was a problem hiding this comment.
Pull request overview
Adds first-class Postgres schema migrations to the API stack by introducing Flyway as the migration runner and landing an initial profiles table, alongside a small DB helper to support upcoming transactional work.
Changes:
- Adds an architecture decision log entry documenting the choice of Flyway and the intended migration workflow.
- Updates
docker-compose.ymlto run Flyway migrations fromapps/api/sql/before starting the API container. - Introduces a
Postgres.transaction()helper and a first migrationV1__profiles.sqlto create theprofilestable.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| docs/decision-log/architecture/db-migrations.md | Documents the migration approach (Flyway + versioned SQL under apps/api/sql/). |
| docker-compose.yml | Adds a Flyway service and gates API startup on successful migrations. |
| apps/api/src/infrastructure/db/postgres.ts | Adds transaction() helper for running work on a single client in a DB transaction. |
| apps/api/sql/V1__profiles.sql | Adds initial schema migration to create the profiles table. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
apps/api/src/infrastructure/db/postgres.ts:45
ROLLBACKerrors are currently swallowed silently (catch(() => {})). This makes connection/transaction failures harder to diagnose in production. Elsewhere, swallowed errors are still logged (e.g. apps/api/src/lifecycle/manager.ts:64-69). Consider logging rollback failures while still rethrowing the original error.
} catch (err) {
await client.query("ROLLBACK").catch(() => {});
throw err;
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/src/infrastructure/db/postgres.ts (1)
36-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd transaction lifecycle tests before the first caller.
Cover successful commit, callback failure with original-error preservation, rollback failure causing client destruction, and client release.
🤖 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 `@apps/api/src/infrastructure/db/postgres.ts` around lines 36 - 49, Add transaction lifecycle tests targeting the transaction method in the Postgres database class before introducing its first caller. Verify successful callbacks commit and release the client, callback failures preserve the original error while rolling back, and rollback failures destroy the client rather than silently reusing it.
🤖 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 `@apps/api/src/infrastructure/db/postgres.ts`:
- Around line 43-47: Update the transaction error-handling block around the
catch/finally logic to track whether ROLLBACK fails and release the client with
destruction enabled in that case. Preserve normal reuse for successful rollback,
while ensuring the finally block invokes client.release(true) when the
connection’s transaction state is unknown.
---
Nitpick comments:
In `@apps/api/src/infrastructure/db/postgres.ts`:
- Around line 36-49: Add transaction lifecycle tests targeting the transaction
method in the Postgres database class before introducing its first caller.
Verify successful callbacks commit and release the client, callback failures
preserve the original error while rolling back, and rollback failures destroy
the client rather than silently reusing it.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a9fca19-e64a-46dc-b55f-955645df3cdc
📒 Files selected for processing (4)
apps/api/sql/V1__profiles.sqlapps/api/src/infrastructure/db/postgres.tsdocker-compose.ymldocs/decision-log/architecture/db-migrations.md
2be53d1 to
dc150a2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
apps/api/sql/V1__profiles.sql:8
updated_atisNOT NULLbut has noDEFAULT, so everyINSERTmust supply it explicitly. Also, without aBEFORE UPDATEtrigger,updated_atwill become stale unless everyUPDATEremembers to set it.
Consider giving it DEFAULT now() and adding a trigger to automatically refresh it on updates.
socials jsonb NOT NULL,
body_md text NOT NULL,
updated_at timestamptz NOT NULL
2634d2b to
bc7bcd4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
docs/decision-log/architecture/db-migrations.md:14
- Decision-log rule requires the final
Why: ...line to name each rejected alternative and why it was rejected (docs/decision-log/README.md:13). This entry mentions node-pg-migrate and Drizzle, but only explains the rejection reason for Drizzle. Update theWhy:line to include why node-pg-migrate was not selected, and keep the language formal (e.g., “Docker”, “runs on the JVM”).
Trade-off: Flyway is JVM, run via docker not node. Integration tests reuse the same `V*.sql` against `thoth_test` to skip the JVM.
Why: raw `pg`, no ORM, so plain SQL keeps schema infra-owned with checksums for free. node-pg-migrate was the node-native runner-up. Drizzle was rejected — it adds an ORM the codebase avoids.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
docs/decision-log/architecture/db-migrations.md:14
- Decision-log rule requires naming each rejected alternative and why (docs/decision-log/README.md:13). This entry names node-pg-migrate as the runner-up, but doesn’t state why it was rejected, so the rationale is incomplete.
Trade-off: Flyway is JVM, run via docker not node. Integration tests reuse the same `V*.sql` against `thoth_test` to skip the JVM.
Why: raw `pg`, no ORM, so plain SQL keeps schema infra-owned with checksums for free. node-pg-migrate was the node-native runner-up. Drizzle was rejected — it adds an ORM the codebase avoids.
ade0ac9 to
2cca4e9
Compare
Summary
Introduces Postgres schema migrations with Flyway and a first
profilestable.Changes
apps/api/sql/as a docker-compose service. The api waits on it viaservice_completed_successfully.V1__profiles.sql—profilestable:id(uuid),name,tagline,avatar_url,socials(jsonb),body_md,updated_at.Postgres.transaction()— runs a function in one transaction, commits on success, rolls back on throw.Notes
transaction()has no caller yet — it lands ahead of the content-sync work.pnpm compose:up.🤖 Generated with Claude Code
Greptile Summary
This PR wires Flyway into docker-compose to run versioned SQL migrations from
apps/api/sql/, adds the first migration (V1__profiles.sql) creating aprofilestable, and introduces aPostgres.transaction()helper for running work inside a single database transaction.service_completed_successfully, so a failed migration blocks startup rather than letting the app start against a stale schema.profilestable withid uuid PRIMARY KEY, typed columns, andsocials jsonb;updated_atis declaredNOT NULLbut has noDEFAULTand no auto-update trigger, so callers must supply and maintain it entirely by hand.transaction()follows a standard begin/rollback/commit pattern but theROLLBACKcall sits outside a nested try/catch, meaning a connection failure during rollback discards the original error.Confidence Score: 3/5
Safe to merge for local dev; the transaction helper has an error-masking bug that should be fixed before it gets a real caller.
The docker-compose wiring and migration file are straightforward and correct. The transaction() method has a real defect: if the connection drops while ROLLBACK is executing, the rollback exception replaces the original error, making failures harder to diagnose. The method has no callers yet, so nothing breaks today, but it will cause confusing error propagation the moment content-sync work lands.
apps/api/src/infrastructure/db/postgres.ts — the rollback error-swallowing issue should be addressed before transaction() gets its first caller
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant DC as docker-compose participant PG as postgres participant FW as flyway participant API as api DC->>PG: start PG-->>DC: healthy (pg_isready) DC->>FW: start (depends_on postgres healthy) FW->>PG: connect + run V1__profiles.sql PG-->>FW: migration applied FW-->>DC: exited 0 (service_completed_successfully) DC->>API: start (depends_on flyway completed) API->>PG: SELECT 1 (start/ping) PG-->>API: ok%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant DC as docker-compose participant PG as postgres participant FW as flyway participant API as api DC->>PG: start PG-->>DC: healthy (pg_isready) DC->>FW: start (depends_on postgres healthy) FW->>PG: connect + run V1__profiles.sql PG-->>FW: migration applied FW-->>DC: exited 0 (service_completed_successfully) DC->>API: start (depends_on flyway completed) API->>PG: SELECT 1 (start/ping) PG-->>API: okReviews (1): Last reviewed commit: "feat(api): add postgres transaction help..." | Re-trigger Greptile
Context used:
Summary by CodeRabbit