refactor(db): use new store interface#831
Open
scottmckendry wants to merge 3 commits intomainfrom
Open
Conversation
dosubot
Bot
added
the
size:L
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:
📝 WalkthroughWalkthroughThis PR introduces a repository.Store abstraction, moves sqlc-generated SQLite artifacts into ChangesStore & SQLite bootstrap
Sequence Diagram(s)sequenceDiagram
participant Dev
participant Bootstrap as BootstrapApp
participant Migrator as Migrations (embed.FS)
participant StoreImpl as sqlite.Store / memory.Store
participant Service as Auth/OIDC Service
Dev->>Bootstrap: SetupStore()
Bootstrap->>Migrator: load migrations from "migrations/sqlite"
Migrator-->>Bootstrap: migrations applied
Bootstrap->>StoreImpl: open DB & wrap / create memory store
Bootstrap-->>Dev: return repository.Store
Dev->>Service: NewOIDCService(config, repository.Store)
Service->>StoreImpl: perform CRUD via Store interface
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/bootstrap/db_bootstrap.go (1)
19-69:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve DB cleanup when hiding the concrete SQLite store.
SetupStore/NewSQLiteStorenow return onlyrepository.Store, so callers can no longer close the underlying*sql.DB. The controller tests used todb.Close(), but after this refactor those handles stay open until process exit, which can leak file descriptors and leave temp SQLite files locked on some platforms.setupSQLitealso leaks the opened DB on migration/setup error paths. Please return a closer alongside the store, or wrap the store in a type that also exposesClose()and use it in the tests/startup cleanup path.
🧹 Nitpick comments (1)
internal/repository/models.go (1)
8-19: 🏗️ Heavy liftDefine canonical repository models instead of aliasing SQLite types.
Aliasing
repository.*directly tosqlite.*keeps the abstraction coupled to one backend. If you want multi-driver support to stay clean, define canonical types inrepositoryand map in each driver implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repository/models.go` around lines 8 - 19, The current file defines repository models as direct aliases to sqlite types (e.g., Session, OidcCode, OidcToken, OidcUserinfo and param types like CreateSessionParams, UpdateSessionParams, CreateOidcCodeParams, etc.), which couples the repository API to one driver; replace these type aliases with canonical repository types (structs/interfaces) in internal/repository/models.go and remove the "type X = sqlite.X" lines, then implement conversion helpers in the sqlite driver (e.g., functions to convert between repository.Session <-> sqlite.Session and repository.CreateSessionParams <-> sqlite.CreateSessionParams) so other drivers can map their native types to the repository canonical types without depending on sqlite symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/proxy_controller_test.go`:
- Around line 395-396: The Store returned by NewSQLiteStore leaks the underlying
*sql.DB because setupSQLite opens the DB but neither Store nor sqlite.Queries
expose a Close; update the API so tests can close the DB: either add Close()
error to the Store interface and implement Close on sqlite.Queries to call the
underlying *sql.DB.Close(), or change setupSQLite/NewSQLiteStore to return the
*sql.DB alongside the Store so callers (tests like proxy_controller_test,
oidc_controller_test, user_controller_test, well_known_controller_test) can
explicitly defer db.Close(); update all call sites and implementations (Store
interface, sqlite.Queries, setupSQLite, NewSQLiteStore) consistently.
---
Nitpick comments:
In `@internal/repository/models.go`:
- Around line 8-19: The current file defines repository models as direct aliases
to sqlite types (e.g., Session, OidcCode, OidcToken, OidcUserinfo and param
types like CreateSessionParams, UpdateSessionParams, CreateOidcCodeParams,
etc.), which couples the repository API to one driver; replace these type
aliases with canonical repository types (structs/interfaces) in
internal/repository/models.go and remove the "type X = sqlite.X" lines, then
implement conversion helpers in the sqlite driver (e.g., functions to convert
between repository.Session <-> sqlite.Session and repository.CreateSessionParams
<-> sqlite.CreateSessionParams) so other drivers can map their native types to
the repository canonical types without depending on sqlite symbols.
🪄 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: c386e1b5-70c0-4440-93d8-eec72b28f642
📒 Files selected for processing (40)
internal/assets/assets.gointernal/assets/migrations/sqlite/000001_init_sqlite.down.sqlinternal/assets/migrations/sqlite/000001_init_sqlite.up.sqlinternal/assets/migrations/sqlite/000002_oauth_name.down.sqlinternal/assets/migrations/sqlite/000002_oauth_name.up.sqlinternal/assets/migrations/sqlite/000003_oauth_sub.down.sqlinternal/assets/migrations/sqlite/000003_oauth_sub.up.sqlinternal/assets/migrations/sqlite/000004_created_at.down.sqlinternal/assets/migrations/sqlite/000004_created_at.up.sqlinternal/assets/migrations/sqlite/000005_oidc_session.down.sqlinternal/assets/migrations/sqlite/000005_oidc_session.up.sqlinternal/assets/migrations/sqlite/000006_oidc_nonce.down.sqlinternal/assets/migrations/sqlite/000006_oidc_nonce.up.sqlinternal/assets/migrations/sqlite/000007_oidc_pkce.down.sqlinternal/assets/migrations/sqlite/000007_oidc_pkce.up.sqlinternal/assets/migrations/sqlite/000008_oidc_code_reuse.down.sqlinternal/assets/migrations/sqlite/000008_oidc_code_reuse.up.sqlinternal/assets/migrations/sqlite/000009_oidc_userinfo_profile.down.sqlinternal/assets/migrations/sqlite/000009_oidc_userinfo_profile.up.sqlinternal/bootstrap/app_bootstrap.gointernal/bootstrap/db_bootstrap.gointernal/bootstrap/service_bootstrap.gointernal/config/config.gointernal/controller/oidc_controller_test.gointernal/controller/proxy_controller_test.gointernal/controller/user_controller_test.gointernal/controller/well_known_controller_test.gointernal/repository/models.gointernal/repository/sqlite/db.gointernal/repository/sqlite/models.gointernal/repository/sqlite/oidc_queries.sql.gointernal/repository/sqlite/session_queries.sql.gointernal/repository/store.gointernal/service/auth_service.gointernal/service/oidc_service.gosql/sqlite/oidc_queries.sqlsql/sqlite/oidc_schemas.sqlsql/sqlite/session_queries.sqlsql/sqlite/session_schemas.sqlsqlc.yml
dosubot
Bot
added
size:XL
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/bootstrap/db_bootstrap.go (1)
37-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose the SQLite handle on every failure after
sql.Open.If
iofs.New,sqlite3.WithInstance, ormigrate.NewWithInstancefails, the opened*sql.DBis never closed. That leaks a live connection/file handle on startup failures and in test helpers.♻️ Proposed fix
func (app *BootstrapApp) setupSQLite(databasePath string) (repository.Store, error) { dir := filepath.Dir(databasePath) @@ db, err := sql.Open("sqlite", databasePath) if err != nil { return nil, fmt.Errorf("failed to open database: %w", err) } + closeOnError := true + defer func() { + if closeOnError { + _ = db.Close() + } + }() @@ if err != nil { return nil, fmt.Errorf("failed to migrate database: %w", err) } + closeOnError = false return sqlite.NewStore(sqlite.New(db)), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/bootstrap/db_bootstrap.go` around lines 37 - 69, After sql.Open succeeds, ensure the opened *sql.DB is closed on any subsequent failure to avoid leaking handles: either add a deferred close that only runs when the function will return an error (e.g., capture the returned err via a named return or a closure checking a local err variable) or explicitly call db.Close() before every early return after calls to iofs.New, sqlite3.WithInstance, migrate.NewWithInstance, and migrator.Up; update the error return paths around iofs.New, sqlite3.WithInstance, migrate.NewWithInstance, and migrator.Up so they close db before returning, while leaving the final successful return (sqlite.NewStore/sqlite.New) untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 29-32: The CI step "Check store codegen is up to date" currently
runs "go generate ./internal/repository/..." then "git diff --exit-code --
internal/repository/" which misses brand-new untracked files; after running go
generate, run "git add -N internal/repository || true" (or "git add -N
./internal/repository || true") to stage intent for new files so they show up in
diffs, then run "git diff --exit-code -- internal/repository/" as before; update
the step commands in the "Check store codegen is up to date" job to include that
git add -N command between the generate and diff commands.
---
Outside diff comments:
In `@internal/bootstrap/db_bootstrap.go`:
- Around line 37-69: After sql.Open succeeds, ensure the opened *sql.DB is
closed on any subsequent failure to avoid leaking handles: either add a deferred
close that only runs when the function will return an error (e.g., capture the
returned err via a named return or a closure checking a local err variable) or
explicitly call db.Close() before every early return after calls to iofs.New,
sqlite3.WithInstance, migrate.NewWithInstance, and migrator.Up; update the error
return paths around iofs.New, sqlite3.WithInstance, migrate.NewWithInstance, and
migrator.Up so they close db before returning, while leaving the final
successful return (sqlite.NewStore/sqlite.New) untouched.
🪄 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: 2a1c97b5-1768-4f54-8215-b27651cd4eb8
⛔ Files ignored due to path filters (1)
cmd/gen/sqlc-wrapper/main.gois excluded by!**/gen/**
📒 Files selected for processing (6)
.github/workflows/ci.ymlgo.modinternal/bootstrap/db_bootstrap.gointernal/repository/models.gointernal/repository/sqlite/generate.gointernal/repository/sqlite/store.go
✅ Files skipped from review due to trivial changes (2)
- internal/repository/sqlite/generate.go
- internal/repository/sqlite/store.go
Member
Author
|
The code gen added in the most recent commit was largely written by an LLM. I have vetted to the best of my ability, but this is a new concept to me - hence this disclaimer. that out of the way, the generate code satisfies the store interface and correctly wraps all of the sqlc-generated code. |
scottmckendry
force-pushed
the
store-refactor
branch
from
14c1027 to
75a00a1
Compare
scottmckendry
force-pushed
the
store-refactor
branch
from
75a00a1 to
0244f39
Compare
removes the sqlite dependency for tests, also brings back the option for users to run zero persistence instances of tinyauth. adds new mapErr fn for sqlc wrapper gen to prevent sql errors from leaking out of the store implementation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/bootstrap/db_bootstrap.go (1)
38-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose the opened DB on sqlite bootstrap failures.
After Line 38 opens the DB, failures on Lines 48–67 return early without
db.Close(). Repeated failed startups can leak file descriptors/locks.🔧 Suggested patch
func (app *BootstrapApp) setupSQLite(databasePath string) (repository.Store, error) { @@ db, err := sql.Open("sqlite", databasePath) if err != nil { return nil, fmt.Errorf("failed to open database: %w", err) } + closeOnErr := func(err error) (repository.Store, error) { + _ = db.Close() + return nil, err + } @@ migrations, err := iofs.New(assets.Migrations, "migrations/sqlite") if err != nil { - return nil, fmt.Errorf("failed to create migrations: %w", err) + return closeOnErr(fmt.Errorf("failed to create migrations: %w", err)) } @@ target, err := sqlite3.WithInstance(db, &sqlite3.Config{}) if err != nil { - return nil, fmt.Errorf("failed to create sqlite3 instance: %w", err) + return closeOnErr(fmt.Errorf("failed to create sqlite3 instance: %w", err)) } @@ migrator, err := migrate.NewWithInstance("iofs", migrations, "sqlite3", target) if err != nil { - return nil, fmt.Errorf("failed to create migrator: %w", err) + return closeOnErr(fmt.Errorf("failed to create migrator: %w", err)) } @@ if err := migrator.Up(); err != nil && err != migrate.ErrNoChange { - return nil, fmt.Errorf("failed to migrate database: %w", err) + return closeOnErr(fmt.Errorf("failed to migrate database: %w", err)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/bootstrap/db_bootstrap.go` around lines 38 - 68, The DB opened by sql.Open is never closed on subsequent error paths (iofs.New, sqlite3.WithInstance, migrate.NewWithInstance, migrator.Up), leaking file descriptors; ensure the DB is closed before every early return after opening (either call db.Close() immediately before each error return or add a defer that closes db only on error using a named return or an "opened" flag and clear it on success), referencing the db variable and the failing expressions iofs.New, sqlite3.WithInstance, migrate.NewWithInstance, and migrator.Up so all those error branches are covered.
🧹 Nitpick comments (1)
internal/repository/memory/oidc_queries.go (1)
13-17: 🏗️ Heavy liftNormalize unique-conflict errors via a shared repository sentinel.
Returning hardcoded SQL-style strings here couples service behavior to backend-specific text. Consider adding a shared
repository.ErrConflict(or equivalent) and returning wrapped sentinel errors from all drivers.♻️ Directional patch (memory side)
- return repository.OidcCode{}, fmt.Errorf("UNIQUE constraint failed: oidc_codes.sub") + return repository.OidcCode{}, fmt.Errorf("%w: oidc_codes.sub", repository.ErrConflict) @@ - return repository.OidcToken{}, fmt.Errorf("UNIQUE constraint failed: oidc_tokens.sub") + return repository.OidcToken{}, fmt.Errorf("%w: oidc_tokens.sub", repository.ErrConflict)And define/map the same sentinel in
internal/repository/store.go+ sqlite wrapper so behavior stays backend-agnostic.Also applies to: 106-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repository/memory/oidc_queries.go` around lines 13 - 17, Replace the hardcoded SQL-style UNIQUE error string returned from the in-memory OIDC code check (the loop over s.oidcCodes in oidc_queries.go that currently returns fmt.Errorf("UNIQUE constraint failed: oidc_codes.sub")) with the shared repository sentinel error (e.g., repository.ErrConflict) wrapped or returned directly; update the Create/Insert OIDC methods that reference s.oidcCodes / repository.OidcCode to use this sentinel for uniqueness conflicts. Also add or map the same repository.ErrConflict sentinel in the store abstraction (internal/repository/store.go) and ensure the sqlite driver wrapper maps its DB-specific UNIQUE errors to repository.ErrConflict so all backends return the same sentinel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/oidc_controller_test.go`:
- Around line 850-853: The test currently creates a shared store via
memory.New() and a shared OIDC service via service.NewOIDCService(...) with
oidcService.Init() before running multiple t.Run subtests, which can cause state
bleed; move the creation and initialization of the memory store and oidcService
into each subtest (or a per-subtest setup helper) so each t.Run gets its own
memory.New() and service.NewOIDCService(...)+oidcService.Init() instance,
ensuring codes/tokens/sessions are isolated and tests are order-independent.
---
Outside diff comments:
In `@internal/bootstrap/db_bootstrap.go`:
- Around line 38-68: The DB opened by sql.Open is never closed on subsequent
error paths (iofs.New, sqlite3.WithInstance, migrate.NewWithInstance,
migrator.Up), leaking file descriptors; ensure the DB is closed before every
early return after opening (either call db.Close() immediately before each error
return or add a defer that closes db only on error using a named return or an
"opened" flag and clear it on success), referencing the db variable and the
failing expressions iofs.New, sqlite3.WithInstance, migrate.NewWithInstance, and
migrator.Up so all those error branches are covered.
---
Nitpick comments:
In `@internal/repository/memory/oidc_queries.go`:
- Around line 13-17: Replace the hardcoded SQL-style UNIQUE error string
returned from the in-memory OIDC code check (the loop over s.oidcCodes in
oidc_queries.go that currently returns fmt.Errorf("UNIQUE constraint failed:
oidc_codes.sub")) with the shared repository sentinel error (e.g.,
repository.ErrConflict) wrapped or returned directly; update the Create/Insert
OIDC methods that reference s.oidcCodes / repository.OidcCode to use this
sentinel for uniqueness conflicts. Also add or map the same
repository.ErrConflict sentinel in the store abstraction
(internal/repository/store.go) and ensure the sqlite driver wrapper maps its
DB-specific UNIQUE errors to repository.ErrConflict so all backends return the
same sentinel.
🪄 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: 421b1e3a-4a34-4570-b12f-b189f5afaa46
⛔ Files ignored due to path filters (1)
cmd/gen/sqlc-wrapper/main.gois excluded by!**/gen/**
📒 Files selected for processing (13)
internal/bootstrap/db_bootstrap.gointernal/config/config.gointernal/controller/oidc_controller_test.gointernal/controller/proxy_controller_test.gointernal/controller/user_controller_test.gointernal/controller/well_known_controller_test.gointernal/repository/memory/oidc_queries.gointernal/repository/memory/session_queries.gointernal/repository/memory/store.gointernal/repository/sqlite/store.gointernal/repository/store.gointernal/service/auth_service.gointernal/service/oidc_service.go
✅ Files skipped from review due to trivial changes (1)
- internal/config/config.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/service/auth_service.go
- internal/repository/sqlite/store.go
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
1 participant
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.
lays the groundwork for adding more storage drivers in the future (postgres, redis) with slightly lower effort and easier maintainability in the future.
moves sqlite specific queries, migrations and generated go code into subdirs.
Summary by CodeRabbit