feat(db): add postgresql support#892
Merged
Merged
Conversation
dosubot
Bot
added
the
size:XXL
Contributor
|
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:
📝 WalkthroughWalkthroughAdds Postgres backend: go.mod indirect deps, expanded embedded migrations, Postgres migration DDL and SQL named queries, sqlc config and generated Go query/models, a Postgres Store adapter, and bootstrap wiring to open pgx and apply migrations. ChangesPostgreSQL Database Support
Sequence DiagramsequenceDiagram
participant App
participant Store
participant Queries
participant PgDB
App->>Store: CreateSession(ctx, params)
Store->>Queries: CreateSession(ctx, params)
Queries->>PgDB: QueryRowContext(INSERT ... RETURNING)
PgDB-->>Queries: row/result
Queries-->>Store: scanned Session or error
Store->>App: repository.Session or mapped error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)✅ Unit Test PR creation complete.
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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@internal/assets/migrations/postgres/000004_created_at.up.sql`:
- Line 1: The migration adds created_at with default 0 which causes existing
sessions to bypass SessionMaxLifetime in GetSession and allows
RefreshSession/UpdateSession to extend sessions indefinitely; update the
migration to backfill created_at for existing rows (instead of 0) — set
created_at to the current timestamp in the same units used by the code (e.g.,
UNIX epoch ms) for all existing sessions and make the column default to the
current time for new rows, so GetSession, RefreshSession, and UpdateSession (and
the SessionMaxLifetime check) behave correctly for pre-migration sessions.
In `@internal/assets/migrations/postgres/000006_oidc_nonce.up.sql`:
- Around line 1-2: Add statements to drop the migration-time defaults for the
new nonce columns after the columns are created and any necessary backfill;
specifically, after adding "nonce" to the oidc_codes and oidc_tokens tables, run
ALTER TABLE ... ALTER COLUMN "nonce" DROP DEFAULT for both the oidc_codes and
oidc_tokens tables so the empty-string default is not permanent and future
inserts must provide a nonce.
In `@internal/bootstrap/db_bootstrap.go`:
- Around line 95-120: The deferred db.Close() is not triggered when
migrator.Up() fails because the code shadows err with "if err := migrator.Up();
...", leaving the outer err nil; change the shadowing so the outer err is set
(e.g., use "if err = migrator.Up(); err != nil && err != migrate.ErrNoChange {
... }") or explicitly call db.Close() before returning on migration failure so
the pgx handle is always closed; update the error handling around migrator.Up()
(referencing migrator.Up, the deferred func that checks err, and db) to ensure
the outer scoped err reflects migration failures.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2261a937-3dea-4b1f-8ad3-0c1a51c25ab1
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
go.modinternal/assets/assets.gointernal/assets/migrations/postgres/000001_init_postgres.down.sqlinternal/assets/migrations/postgres/000001_init_postgres.up.sqlinternal/assets/migrations/postgres/000002_oauth_name.down.sqlinternal/assets/migrations/postgres/000002_oauth_name.up.sqlinternal/assets/migrations/postgres/000003_oauth_sub.down.sqlinternal/assets/migrations/postgres/000003_oauth_sub.up.sqlinternal/assets/migrations/postgres/000004_created_at.down.sqlinternal/assets/migrations/postgres/000004_created_at.up.sqlinternal/assets/migrations/postgres/000005_oidc_session.down.sqlinternal/assets/migrations/postgres/000005_oidc_session.up.sqlinternal/assets/migrations/postgres/000006_oidc_nonce.down.sqlinternal/assets/migrations/postgres/000006_oidc_nonce.up.sqlinternal/assets/migrations/postgres/000007_oidc_pkce.down.sqlinternal/assets/migrations/postgres/000007_oidc_pkce.up.sqlinternal/assets/migrations/postgres/000008_oidc_code_reuse.down.sqlinternal/assets/migrations/postgres/000008_oidc_code_reuse.up.sqlinternal/assets/migrations/postgres/000009_oidc_userinfo_profile.down.sqlinternal/assets/migrations/postgres/000009_oidc_userinfo_profile.up.sqlinternal/bootstrap/db_bootstrap.gointernal/model/config.gointernal/repository/postgres/db.gointernal/repository/postgres/generate.gointernal/repository/postgres/models.gointernal/repository/postgres/oidc_queries.sql.gointernal/repository/postgres/session_queries.sql.gointernal/repository/postgres/store.gosql/postgres/oidc_queries.sqlsql/postgres/oidc_schemas.sqlsql/postgres/session_queries.sqlsql/postgres/session_schemas.sqlsqlc.yml
scottmckendry
force-pushed
the
feat/pgsql-driver
branch
from
f642298 to
2566199
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/assets/migrations/postgres/000001_init_postgres.up.sql (1)
1-1: ⚡ Quick winAvoid
IF NOT EXISTSin versioned migrations.Using
IF NOT EXISTShere can hide schema drift instead of failing fast when pre-existing tables don't match expected structure.Suggested change
-CREATE TABLE IF NOT EXISTS "sessions" ( +CREATE TABLE "sessions" ( @@ -CREATE TABLE IF NOT EXISTS "oidc_codes" ( +CREATE TABLE "oidc_codes" ( @@ -CREATE TABLE IF NOT EXISTS "oidc_tokens" ( +CREATE TABLE "oidc_tokens" ( @@ -CREATE TABLE IF NOT EXISTS "oidc_userinfo" ( +CREATE TABLE "oidc_userinfo" (Also applies to: 15-15, 26-26, 38-38
🤖 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 `@internal/assets/migrations/postgres/000001_init_postgres.up.sql` at line 1, Remove the usage of IF NOT EXISTS from the CREATE TABLE statements so the versioned migration fails fast on unexpected pre-existing objects; specifically update the CREATE TABLE "sessions" statement (and the other CREATE TABLE statements referenced around lines 15, 26, 38) to omit IF NOT EXISTS, leaving a plain CREATE TABLE "sessions" (...) and equivalent plain creates for the other tables so the migration will error if the schema already exists or mismatches.
🤖 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.
Nitpick comments:
In `@internal/assets/migrations/postgres/000001_init_postgres.up.sql`:
- Line 1: Remove the usage of IF NOT EXISTS from the CREATE TABLE statements so
the versioned migration fails fast on unexpected pre-existing objects;
specifically update the CREATE TABLE "sessions" statement (and the other CREATE
TABLE statements referenced around lines 15, 26, 38) to omit IF NOT EXISTS,
leaving a plain CREATE TABLE "sessions" (...) and equivalent plain creates for
the other tables so the migration will error if the schema already exists or
mismatches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 27bcc77d-5ad5-461c-a68f-63870633e0a2
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
go.modinternal/assets/assets.gointernal/assets/migrations/postgres/000001_init_postgres.down.sqlinternal/assets/migrations/postgres/000001_init_postgres.up.sqlinternal/bootstrap/db_bootstrap.gointernal/model/config.gointernal/repository/postgres/db.gointernal/repository/postgres/generate.gointernal/repository/postgres/models.gointernal/repository/postgres/oidc_queries.sql.gointernal/repository/postgres/session_queries.sql.gointernal/repository/postgres/store.gosql/postgres/oidc_queries.sqlsql/postgres/oidc_schemas.sqlsql/postgres/session_queries.sqlsql/postgres/session_schemas.sqlsqlc.yml
✅ Files skipped from review due to trivial changes (5)
- internal/repository/postgres/models.go
- internal/repository/postgres/db.go
- internal/model/config.go
- internal/repository/postgres/store.go
- internal/repository/postgres/oidc_queries.sql.go
steveiliop56
previously approved these changes
May 24, 2026
Member
steveiliop56
left a comment
steveiliop56
left a comment
There was a problem hiding this comment.
@scottmckendry LGTM. Nothing stopping us from merging right? Really happy that the generator worked flawlessly.
scottmckendry
force-pushed
the
feat/pgsql-driver
branch
from
2566199 to
dce5c2c
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/assets/migrations/postgres/000001_init_postgres.up.sql (1)
1-13: ⚡ Quick winAdd an index for session expiry cleanup.
Line 9 introduces
"expiry"and cleanup queries filter on this column; without an index, expired-session deletions will trend toward full table scans.♻️ Proposed change
CREATE TABLE "sessions" ( "uuid" TEXT NOT NULL PRIMARY KEY, "username" TEXT NOT NULL, "email" TEXT NOT NULL, "name" TEXT NOT NULL, "provider" TEXT NOT NULL, "totp_pending" BOOLEAN NOT NULL, "oauth_groups" TEXT NOT NULL DEFAULT '', "expiry" BIGINT NOT NULL, "created_at" BIGINT NOT NULL, "oauth_name" TEXT NOT NULL DEFAULT '', "oauth_sub" TEXT NOT NULL DEFAULT '' ); + +CREATE INDEX "idx_sessions_expiry" ON "sessions" ("expiry");🤖 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 `@internal/assets/migrations/postgres/000001_init_postgres.up.sql` around lines 1 - 13, Add a btree index on the sessions.expiry column to avoid full-table scans during expired-session cleanup: in the migration that creates the "sessions" table, add an index statement (e.g., CREATE INDEX ON "sessions"("expiry")) targeting the "expiry" column so cleanup queries filtering on expiry run efficiently; ensure the index name is unique (e.g., idx_sessions_expiry) and included in the same migration or a follow-up migration.sql/postgres/oidc_queries.sql (1)
130-133: ⚡ Quick winUse a single cutoff parameter for token cleanup.
Using two params for the same cleanup point makes accidental divergence easy. Reusing one cutoff simplifies the contract and reduces misuse risk.
Proposed diff
-- name: DeleteExpiredOidcTokens :many DELETE FROM "oidc_tokens" -WHERE "token_expires_at" < $1 AND "refresh_token_expires_at" < $2 +WHERE "token_expires_at" < $1 AND "refresh_token_expires_at" < $1 RETURNING *;🤖 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 `@sql/postgres/oidc_queries.sql` around lines 130 - 133, The DeleteExpiredOidcTokens query uses two cutoff parameters which can diverge; change the WHERE clause in the oidc_tokens DELETE query to use a single cutoff parameter (reuse the same $1) for both "token_expires_at" and "refresh_token_expires_at" comparisons so both use the same cutoff time (i.e., WHERE "token_expires_at" < $1 AND "refresh_token_expires_at" < $1), leaving the RETURNING *; intact.
🤖 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.
Nitpick comments:
In `@internal/assets/migrations/postgres/000001_init_postgres.up.sql`:
- Around line 1-13: Add a btree index on the sessions.expiry column to avoid
full-table scans during expired-session cleanup: in the migration that creates
the "sessions" table, add an index statement (e.g., CREATE INDEX ON
"sessions"("expiry")) targeting the "expiry" column so cleanup queries filtering
on expiry run efficiently; ensure the index name is unique (e.g.,
idx_sessions_expiry) and included in the same migration or a follow-up
migration.
In `@sql/postgres/oidc_queries.sql`:
- Around line 130-133: The DeleteExpiredOidcTokens query uses two cutoff
parameters which can diverge; change the WHERE clause in the oidc_tokens DELETE
query to use a single cutoff parameter (reuse the same $1) for both
"token_expires_at" and "refresh_token_expires_at" comparisons so both use the
same cutoff time (i.e., WHERE "token_expires_at" < $1 AND
"refresh_token_expires_at" < $1), leaving the RETURNING *; intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 904eb1db-6b47-403e-a0b5-3d8651a7896b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
go.modinternal/assets/assets.gointernal/assets/migrations/postgres/000001_init_postgres.down.sqlinternal/assets/migrations/postgres/000001_init_postgres.up.sqlinternal/bootstrap/db_bootstrap.gointernal/model/config.gointernal/repository/postgres/db.gointernal/repository/postgres/generate.gointernal/repository/postgres/models.gointernal/repository/postgres/oidc_queries.sql.gointernal/repository/postgres/session_queries.sql.gointernal/repository/postgres/store.gosql/postgres/oidc_queries.sqlsql/postgres/oidc_schemas.sqlsql/postgres/session_queries.sqlsql/postgres/session_schemas.sqlsqlc.yml
✅ Files skipped from review due to trivial changes (7)
- internal/assets/migrations/postgres/000001_init_postgres.down.sql
- internal/repository/postgres/generate.go
- internal/repository/postgres/models.go
- internal/repository/postgres/session_queries.sql.go
- internal/repository/postgres/store.go
- internal/repository/postgres/db.go
- internal/repository/postgres/oidc_queries.sql.go
scottmckendry
force-pushed
the
feat/pgsql-driver
branch
from
dce5c2c to
bb2bf06
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/bootstrap/db_bootstrap.go (1)
94-99:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winImplement cleanup flag as previously discussed.
The shadowing issue from the prior review was fixed by using
err =instead oferr :=. However, the cleanup flag approach wasn't implemented, and the current code still has a bug: whenmigrator.Up()returnsErrNoChange,erris non-nil, so the defer closesdbeven on the success path.This affects every startup after initial migration, returning a store with a closed database handle.
🔧 Proposed fix (cleanup flag pattern)
func (app *BootstrapApp) setupPostgres(databaseURL string) (repository.Store, error) { db, err := sql.Open("pgx", databaseURL) if err != nil { return nil, fmt.Errorf("failed to open database: %w", err) } - // Close the database if there is an error during migration - defer func() { - if err != nil { - db.Close() - } - }() + cleanup := true + defer func() { + if cleanup { + _ = db.Close() + } + }() migrations, err := iofs.New(assets.Migrations, "migrations/postgres") // ... rest unchanged ... if err = migrator.Up(); err != nil && err != migrate.ErrNoChange { return nil, fmt.Errorf("failed to migrate database: %w", err) } + cleanup = false app.db = db return postgres.NewStore(postgres.New(db)), nil }Also applies to: 119-121
🤖 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 `@internal/bootstrap/db_bootstrap.go` around lines 94 - 99, The defer that closes db today checks only if err != nil and thus closes the DB on migrate.Up() returning migrate.ErrNoChange; change to the cleanup-flag pattern: introduce a boolean (e.g., cleanup := true) before opening/using the DB, defer a func that only closes db when cleanup is true, and after calling migrator.Up() set cleanup = false when the outcome is considered successful (err == nil or errors.Is(err, migrate.ErrNoChange)); apply the same cleanup-flag adjustment to the similar block around the code referenced at the 119-121 region so the store is not returned with a closed DB handle.
🤖 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 `@internal/bootstrap/db_bootstrap.go`:
- Around line 78-80: The defer that closes the DB when err != nil is firing on
the benign migrate.ErrNoChange path because err is left non-nil; update both the
function that calls migrator.Up() and setupPostgres to use the cleanup-flag
pattern: introduce a local bool (e.g., cleanup := true) and a defer that closes
the DB only if cleanup is true, set cleanup = false just before the normal
successful return, and treat migrate.ErrNoChange as a non-error by not letting
it trigger cleanup (either by clearing err or returning after setting
cleanup=false) so the returned store handle is not closed.
---
Duplicate comments:
In `@internal/bootstrap/db_bootstrap.go`:
- Around line 94-99: The defer that closes db today checks only if err != nil
and thus closes the DB on migrate.Up() returning migrate.ErrNoChange; change to
the cleanup-flag pattern: introduce a boolean (e.g., cleanup := true) before
opening/using the DB, defer a func that only closes db when cleanup is true, and
after calling migrator.Up() set cleanup = false when the outcome is considered
successful (err == nil or errors.Is(err, migrate.ErrNoChange)); apply the same
cleanup-flag adjustment to the similar block around the code referenced at the
119-121 region so the store is not returned with a closed DB handle.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c4108d20-19f5-41a6-a05e-3a8f50ae664c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
go.modinternal/assets/assets.gointernal/assets/migrations/postgres/000001_init_postgres.down.sqlinternal/assets/migrations/postgres/000001_init_postgres.up.sqlinternal/bootstrap/db_bootstrap.gointernal/model/config.gointernal/repository/postgres/db.gointernal/repository/postgres/generate.gointernal/repository/postgres/models.gointernal/repository/postgres/oidc_queries.sql.gointernal/repository/postgres/session_queries.sql.gointernal/repository/postgres/store.gosql/postgres/oidc_queries.sqlsql/postgres/oidc_schemas.sqlsql/postgres/session_queries.sqlsql/postgres/session_schemas.sqlsqlc.yml
✅ Files skipped from review due to trivial changes (4)
- internal/model/config.go
- internal/repository/postgres/store.go
- internal/repository/postgres/session_queries.sql.go
- internal/repository/postgres/oidc_queries.sql.go
scottmckendry
force-pushed
the
feat/pgsql-driver
branch
from
bb2bf06 to
40540ce
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@sql/postgres/session_schemas.sql`:
- Around line 1-13: The Postgres sessions table is missing the ldap_groups
column which causes model/behavior drift vs SQLite; update the "sessions" CREATE
TABLE to add an "ldap_groups" TEXT NOT NULL DEFAULT '' column (matching the
sqlite contract), and then update any related INSERT/UPDATE SQL statements (the
session insert/update queries used to generate models) so they include
ldap_groups; ensure the column name "ldap_groups" appears in the sessions schema
and in the parameter list of the functions that insert/update sessions so
sqlc-generated models remain aligned.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e4310b9e-40fd-42d5-9eac-3290b27a12e6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
go.modinternal/assets/assets.gointernal/assets/migrations/postgres/000001_init_postgres.down.sqlinternal/assets/migrations/postgres/000001_init_postgres.up.sqlinternal/bootstrap/db_bootstrap.gointernal/model/config.gointernal/repository/postgres/db.gointernal/repository/postgres/generate.gointernal/repository/postgres/models.gointernal/repository/postgres/oidc_queries.sql.gointernal/repository/postgres/session_queries.sql.gointernal/repository/postgres/store.gosql/postgres/oidc_queries.sqlsql/postgres/oidc_schemas.sqlsql/postgres/session_queries.sqlsql/postgres/session_schemas.sqlsqlc.yml
✅ Files skipped from review due to trivial changes (7)
- internal/model/config.go
- go.mod
- internal/repository/postgres/models.go
- internal/repository/postgres/db.go
- internal/repository/postgres/store.go
- internal/repository/postgres/session_queries.sql.go
- internal/repository/postgres/oidc_queries.sql.go
Contributor
|
Note Docstrings generation - SKIPPED |
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
coderabbitai Bot
added a commit
that referenced
this pull request
May 25, 2026
Docstrings generation was requested by @scottmckendry. * #892 (comment) The following files were modified: * `internal/repository/postgres/db.go` * `internal/repository/postgres/store.go`
Contributor
|
Warning Docstrings generation is disabled for your repository or organization. |
Contributor
|
✅ Created PR with unit tests: #906 |
steveiliop56
approved these changes
May 25, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
previous refactor made this really straightforward. code gen for the sqlc wrapper worked perfectly!
Summary by CodeRabbit
New Features
Chores