refactor(db): use new store interface#831
Open
scottmckendry wants to merge 10 commits into
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a repository.Store abstraction and ErrNotFound, moves sqlc-generated sqlite code to internal/repository/sqlite with a sqlite Store adapter, adds an in-memory Store and tests, refactors services/bootstrap to use Store, narrows embedded migrations to migrations/sqlite, and adds codegen/CI tooling. ChangesPersistence Abstraction & Full Integration
🎯 4 (Complex) | ⏱️ ~60 minutes Sequence DiagramsequenceDiagram
participant BootstrapApp
participant Service
participant Store
participant SQLiteQueries
BootstrapApp->>Store: SetupStore() selects memory or sqlite
alt sqlite
BootstrapApp->>SQLiteQueries: open DB, run migrations (migrations/sqlite/*.sql)
SQLiteQueries-->>Store: return *Queries wrapped by sqlite.NewStore
end
BootstrapApp->>Service: inject repository.Store
Service->>Store: CRUD calls (GetSession/CreateOidcToken/DeleteExpired*)
Store-->>Service: results / repository.ErrNotFound
Service->>Service: map ErrNotFound -> service errors or ignore for deletions
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)
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 |
![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
2 times, most recently
from
75a00a1 to
0244f39
Compare
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
scottmckendry
force-pushed
the
store-refactor
branch
from
592c26c to
3eeabd3
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
scottmckendry
force-pushed
the
store-refactor
branch
from
3eeabd3 to
db911a4
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/middleware/context_middleware_test.go (1)
273-313:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid sharing one in-memory store across all subtests.
Using a single
memory.New()instance for the whole suite allows session data to leak across test cases, which can make tests order-dependent over time.Suggested isolation pattern
- store := memory.New() - - ldap := service.NewLdapService(service.LdapServiceConfig{}) - err := ldap.Init() - require.NoError(t, err) - - broker := service.NewOAuthBrokerService(oauthBrokerCfgs) - err = broker.Init() - require.NoError(t, err) - - authService := service.NewAuthService(authServiceCfg, ldap, store, broker) - err = authService.Init() - require.NoError(t, err) - - contextMiddleware := middleware.NewContextMiddleware(middlewareCfg, authService, broker) - err = contextMiddleware.Init() - require.NoError(t, err) - for _, test := range tests { - authService.ClearRateLimitsTestingOnly() t.Run(test.description, func(t *testing.T) { + store := memory.New() + ldap := service.NewLdapService(service.LdapServiceConfig{}) + require.NoError(t, ldap.Init()) + broker := service.NewOAuthBrokerService(oauthBrokerCfgs) + require.NoError(t, broker.Init()) + authService := service.NewAuthService(authServiceCfg, ldap, store, broker) + require.NoError(t, authService.Init()) + contextMiddleware := middleware.NewContextMiddleware(middlewareCfg, authService, broker) + require.NoError(t, contextMiddleware.Init()) + authService.ClearRateLimitsTestingOnly() + gin.SetMode(gin.TestMode) ... test.run(t, runArgs{do: do, queries: store}) }) }🤖 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/middleware/context_middleware_test.go` around lines 273 - 313, Tests currently share a single in-memory store (memory.New()) across all subtests which allows session data to leak; to fix, create a fresh store per subtest and re-initialize the services that depend on it: move the memory.New() call into the t.Run loop and recreate the auth service and context middleware there (use service.NewAuthService(...) and middleware.NewContextMiddleware(...), then call Init() and require.NoError for each), while keeping reusable services like ldap/broker outside if desired; ensure authService.ClearRateLimitsTestingOnly() still operates on the per-test instance.
🧹 Nitpick comments (1)
internal/service/auth_service.go (1)
423-425: ⚡ Quick winPreserve the not-found sentinel when remapping the error.
Returning
errors.New("session not found")drops type information and prevents upstreamerrors.Is(..., repository.ErrNotFound)checks.Proposed adjustment
if err != nil { if errors.Is(err, repository.ErrNotFound) { - return nil, errors.New("session not found") + return nil, fmt.Errorf("session not found: %w", repository.ErrNotFound) } return nil, err }🤖 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/service/auth_service.go` around lines 423 - 425, The current remapping returns a new error via errors.New("session not found") which drops the sentinel; instead wrap the original not-found error so upstream errors.Is(..., repository.ErrNotFound) still matches. Replace the return in the errors.Is(err, repository.ErrNotFound) branch to return a wrapped error that includes context and the original error (use fmt.Errorf with %w or errors.Wrap-style wrapping) so repository.ErrNotFound is preserved when checking later.
🤖 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.
Outside diff comments:
In `@internal/middleware/context_middleware_test.go`:
- Around line 273-313: Tests currently share a single in-memory store
(memory.New()) across all subtests which allows session data to leak; to fix,
create a fresh store per subtest and re-initialize the services that depend on
it: move the memory.New() call into the t.Run loop and recreate the auth service
and context middleware there (use service.NewAuthService(...) and
middleware.NewContextMiddleware(...), then call Init() and require.NoError for
each), while keeping reusable services like ldap/broker outside if desired;
ensure authService.ClearRateLimitsTestingOnly() still operates on the per-test
instance.
---
Nitpick comments:
In `@internal/service/auth_service.go`:
- Around line 423-425: The current remapping returns a new error via
errors.New("session not found") which drops the sentinel; instead wrap the
original not-found error so upstream errors.Is(..., repository.ErrNotFound)
still matches. Replace the return in the errors.Is(err, repository.ErrNotFound)
branch to return a wrapped error that includes context and the original error
(use fmt.Errorf with %w or errors.Wrap-style wrapping) so repository.ErrNotFound
is preserved when checking later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d40c9d4-416c-4970-aef0-32255fea7eaa
⛔ Files ignored due to path filters (2)
cmd/gen/sqlc-wrapper/sqlc_wrapper.gois excluded by!**/gen/**cmd/gen/sqlc-wrapper/store.tmplis excluded by!**/gen/**
📒 Files selected for processing (49)
.github/workflows/ci.ymlMakefilego.modinternal/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/controller/oidc_controller_test.gointernal/controller/proxy_controller_test.gointernal/controller/user_controller_test.gointernal/controller/well_known_controller_test.gointernal/middleware/context_middleware_test.gointernal/model/config.gointernal/repository/memory/oidc_queries.gointernal/repository/memory/session_queries.gointernal/repository/memory/store.gointernal/repository/models.gointernal/repository/sqlite/db.gointernal/repository/sqlite/generate.gointernal/repository/sqlite/models.gointernal/repository/sqlite/oidc_queries.sql.gointernal/repository/sqlite/session_queries.sql.gointernal/repository/sqlite/store.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
✅ Files skipped from review due to trivial changes (3)
- internal/repository/sqlite/generate.go
- sqlc.yml
- internal/repository/sqlite/models.go
🚧 Files skipped from review as they are similar to previous changes (20)
- internal/repository/sqlite/db.go
- internal/repository/sqlite/oidc_queries.sql.go
- internal/assets/assets.go
- internal/repository/sqlite/session_queries.sql.go
- internal/bootstrap/db_bootstrap.go
- go.mod
- .github/workflows/ci.yml
- internal/repository/memory/session_queries.go
- internal/repository/memory/store.go
- internal/bootstrap/service_bootstrap.go
- internal/controller/well_known_controller_test.go
- internal/repository/store.go
- internal/service/oidc_service.go
- internal/repository/sqlite/store.go
- internal/controller/proxy_controller_test.go
- internal/controller/user_controller_test.go
- internal/bootstrap/app_bootstrap.go
- internal/controller/oidc_controller_test.go
- internal/repository/memory/oidc_queries.go
- internal/repository/models.go
scottmckendry
force-pushed
the
store-refactor
branch
from
374b879 to
e2fd605
Compare
steveiliop56
requested changes
May 11, 2026
scottmckendry
force-pushed
the
store-refactor
branch
2 times, most recently
from
12c2ba3 to
5f5b188
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/bootstrap/app_bootstrap.go (1)
173-177:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winNil pointer dereference when using in-memory store.
The defer unconditionally calls
app.db.Close()on line 176, butapp.dbwill be nil when the in-memory driver is selected viaSetupStore(). This will cause a runtime panic during cleanup.🛡️ Proposed fix to guard the Close call
defer func() { app.cancel() app.wg.Wait() - app.db.Close() + if app.db != nil { + app.db.Close() + } }()🤖 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/app_bootstrap.go` around lines 173 - 177, The deferred cleanup unconditionally calls app.db.Close(), which panics when SetupStore selected the in-memory driver and app.db is nil; change the defer to guard the Close call (check that app.db != nil before calling app.db.Close()) so the cleanup sequence via app.cancel(), app.wg.Wait(), and conditional app.db.Close() runs safely when the in-memory store is used.
🤖 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.
Outside diff comments:
In `@internal/bootstrap/app_bootstrap.go`:
- Around line 173-177: The deferred cleanup unconditionally calls
app.db.Close(), which panics when SetupStore selected the in-memory driver and
app.db is nil; change the defer to guard the Close call (check that app.db !=
nil before calling app.db.Close()) so the cleanup sequence via app.cancel(),
app.wg.Wait(), and conditional app.db.Close() runs safely when the in-memory
store is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ff832682-3b4e-4481-8567-db4250b6c779
⛔ Files ignored due to path filters (2)
gen/sqlc-wrapper/sqlc_wrapper.gois excluded by!**/gen/**gen/sqlc-wrapper/store.tmplis excluded by!**/gen/**
📒 Files selected for processing (50)
.github/workflows/ci.ymlMakefiledocker-compose.dev.ymlgo.modinternal/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/controller/oidc_controller_test.gointernal/controller/proxy_controller_test.gointernal/controller/user_controller_test.gointernal/controller/well_known_controller_test.gointernal/middleware/context_middleware_test.gointernal/model/config.gointernal/repository/memory/memory_test.gointernal/repository/memory/oidc_queries.gointernal/repository/memory/session_queries.gointernal/repository/memory/store.gointernal/repository/models.gointernal/repository/sqlite/db.gointernal/repository/sqlite/generate.gointernal/repository/sqlite/models.gointernal/repository/sqlite/oidc_queries.sql.gointernal/repository/sqlite/session_queries.sql.gointernal/repository/sqlite/store.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
✅ Files skipped from review due to trivial changes (4)
- docker-compose.dev.yml
- internal/repository/sqlite/generate.go
- internal/repository/sqlite/models.go
- internal/repository/sqlite/store.go
🚧 Files skipped from review as they are similar to previous changes (19)
- .github/workflows/ci.yml
- internal/repository/sqlite/oidc_queries.sql.go
- go.mod
- internal/repository/memory/store.go
- internal/assets/assets.go
- internal/middleware/context_middleware_test.go
- Makefile
- sqlc.yml
- internal/bootstrap/db_bootstrap.go
- internal/controller/oidc_controller_test.go
- internal/repository/memory/session_queries.go
- internal/model/config.go
- internal/repository/sqlite/session_queries.sql.go
- internal/repository/models.go
- internal/controller/user_controller_test.go
- internal/repository/store.go
- internal/service/auth_service.go
- internal/service/oidc_service.go
- internal/repository/memory/oidc_queries.go
Comment on lines
+177
to
+179
| if app.db != nil { | ||
| app.db.Close() | ||
| } |
Member
There was a problem hiding this comment.
Why would the db be nil here? This defer is defined after the db is initialized.
Member
Author
There was a problem hiding this comment.
it's nil if it's a memory driver - this may panic without the check. Something coderabbit picked up in one of it's earlier comments
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.
scottmckendry
force-pushed
the
store-refactor
branch
from
d29bff1 to
ac1ff0a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/repository/memory/memory_test.go (1)
175-182: 🏗️ Heavy liftAvoid asserting SQLite-specific unique-constraint message text in Store contract tests.
These assertions couple the memory Store behavior to SQLite error phrasing, which will make Postgres/Redis backends harder to support consistently. Prefer asserting a driver-agnostic sentinel/type (or a
repository-level conflict error) instead of message text.Also applies to: 243-250
🤖 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/repository/memory/memory_test.go` around lines 175 - 182, The test TestCreateOidcCode_UniqueSubConstraint couples behavior to SQLite error text; update it to assert a driver-agnostic conflict sentinel instead of message matching: after calling memory.New() and s.CreateOidcCode(ctx, repository.CreateOidcCodeParams{...}) assert that the returned error from the second CreateOidcCode is errors.Is(err, repository.ErrConflict) (or compare against the repository-level conflict error/type your store contract defines) rather than matching the SQLite string; apply the same change to the other similar test block referenced (the one around the second occurrence).
🤖 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/repository/memory/memory_test.go`:
- Around line 365-392: The test TestDeleteExpiredOidcTokens expects tokens to be
removed when either expiry field is stale, but the SQLite implementation of
DeleteExpiredOidcTokens currently requires both token_expires_at AND
refresh_token_expires_at to be stale; update the DELETE query (and any
corresponding SELECT/RETURNING logic) in the DeleteExpiredOidcTokens function of
the SQLite repository implementation to use OR between conditions
(token_expires_at <= ? OR refresh_token_expires_at <= ?) so it deletes and
returns tokens when either expiry is past the provided thresholds; ensure
parameter order matches repository.DeleteExpiredOidcTokensParams and that
returned deleted entries still map to the
repository.CreateOidcTokenParams/returned token shape.
---
Nitpick comments:
In `@internal/repository/memory/memory_test.go`:
- Around line 175-182: The test TestCreateOidcCode_UniqueSubConstraint couples
behavior to SQLite error text; update it to assert a driver-agnostic conflict
sentinel instead of message matching: after calling memory.New() and
s.CreateOidcCode(ctx, repository.CreateOidcCodeParams{...}) assert that the
returned error from the second CreateOidcCode is errors.Is(err,
repository.ErrConflict) (or compare against the repository-level conflict
error/type your store contract defines) rather than matching the SQLite string;
apply the same change to the other similar test block referenced (the one around
the second occurrence).
🪄 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: fab5be32-95ca-42fc-8cc9-60baba0a775e
⛔ Files ignored due to path filters (2)
gen/sqlc-wrapper/sqlc_wrapper.gois excluded by!**/gen/**gen/sqlc-wrapper/store.tmplis excluded by!**/gen/**
📒 Files selected for processing (50)
.github/workflows/ci.ymlMakefiledocker-compose.dev.ymlgo.modinternal/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/controller/oidc_controller_test.gointernal/controller/proxy_controller_test.gointernal/controller/user_controller_test.gointernal/controller/well_known_controller_test.gointernal/middleware/context_middleware_test.gointernal/model/config.gointernal/repository/memory/memory_test.gointernal/repository/memory/oidc_queries.gointernal/repository/memory/session_queries.gointernal/repository/memory/store.gointernal/repository/models.gointernal/repository/sqlite/db.gointernal/repository/sqlite/generate.gointernal/repository/sqlite/models.gointernal/repository/sqlite/oidc_queries.sql.gointernal/repository/sqlite/session_queries.sql.gointernal/repository/sqlite/store.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
✅ Files skipped from review due to trivial changes (3)
- internal/repository/sqlite/db.go
- internal/repository/sqlite/generate.go
- internal/repository/sqlite/models.go
🚧 Files skipped from review as they are similar to previous changes (21)
- internal/repository/memory/store.go
- internal/repository/sqlite/session_queries.sql.go
- internal/assets/assets.go
- docker-compose.dev.yml
- go.mod
- internal/model/config.go
- sqlc.yml
- .github/workflows/ci.yml
- internal/service/auth_service.go
- internal/repository/memory/session_queries.go
- Makefile
- internal/bootstrap/app_bootstrap.go
- internal/repository/store.go
- internal/controller/oidc_controller_test.go
- internal/controller/user_controller_test.go
- internal/bootstrap/db_bootstrap.go
- internal/repository/models.go
- internal/service/oidc_service.go
- internal/middleware/context_middleware_test.go
- internal/repository/sqlite/store.go
- internal/repository/memory/oidc_queries.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
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.
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
New Features
Chores
Tests