refactor(store): wave B.2 — settings + identity_link + logs repos (#247)#248
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe PR introduces three small domain repositories (IdentityLink, Logs, Settings) with models and interfaces, implements them using PostgreSQL and sqlc-generated queries, wires them into the store infrastructure, and migrates their respective API handlers to use the new abstractions instead of calling SQL query methods directly. Cross-domain reads remain on Store.Queries() until their owning domains migrate. ChangesDomain repositories: settings, identity links, and logs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes The changes follow a consistent, repetitive pattern: three independent domain repos with identical checkpoint structure (models → interface → postgres impl → handler migration). No intricate logic, cross-domain atomicity, or complex error handling beyond standard not-found translation. Handoff points are well-defined and testable in isolation. Possibly related PRs
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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/api/compliance_handler.go`:
- Around line 33-41: The DeviceResults error branch currently always returns an
internal error (apiErrorCtx with ErrInternal/connect.CodeInternal) even when the
repo indicates the device is missing; update the error handling for
h.store.Repos().Compliance.DeviceResults(ctx, deviceID) to detect the repo "not
found" case (same normalization used by DeviceSummary) and return a NotFound API
error (use the NotFound error/code variant used elsewhere) instead of
ErrInternal, so missing-device errors map to NotFound while other errors still
return internal errors via apiErrorCtx.
In `@internal/api/logs_handler.go`:
- Around line 101-104: The current error handling around
h.store.Repos().Logs.GetQueryResult treats every error as NotFound; change it to
call store.IsNotFound(err) and only return apiErrorCtx(ctx,
ErrQueryResultNotFound, connect.CodeNotFound, ...) for that case, otherwise
return an internal error via apiErrorCtx with connect.CodeInternal (e.g.,
apiErrorCtx(ctx, err, connect.CodeInternal, "failed to get log query result")).
Update the GetQueryResult error branch to discriminate store.IsNotFound(err) vs
other errors accordingly.
In `@internal/idp/linker.go`:
- Around line 142-143: The current conditional unconditionally calls
store.IsNotFound(err) and can misclassify a nil error from the stale-link
cleanup; change the guard so you only classify not-found when err is non-nil
(e.g. replace the condition with "if err != nil && !store.IsNotFound(err)" or
first check "if err != nil { if !store.IsNotFound(err) return ... }") so that a
nil err falls through into the Step 2/3 linking logic; update the branch
containing store.IsNotFound(err) accordingly.
🪄 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
Run ID: 9b968f12-370c-4a8a-a6f9-b65b36c73f9e
📒 Files selected for processing (54)
cmd/control/main.gocmd/indexer/main.gointernal/api/action_crud.gointernal/api/action_dispatch.gointernal/api/assignment_handler.gointernal/api/auth_handler.gointernal/api/certificate_handler.gointernal/api/compliance_handler.gointernal/api/compliance_policy_handler.gointernal/api/device_group_handler.gointernal/api/device_handler.gointernal/api/helpers.gointernal/api/helpers_test.gointernal/api/identity_link_handler.gointernal/api/idp_handler.gointernal/api/logs_handler.gointernal/api/registration_handler.gointernal/api/role_handler.gointernal/api/search_listener.gointernal/api/search_listener_e2e_test.gointernal/api/settings_handler.gointernal/api/sso_handler.gointernal/api/terminal_handler.gointernal/api/totp_handler.gointernal/api/user_group_handler.gointernal/api/user_handler.gointernal/control/inbox_worker.gointernal/idp/linker.gointernal/idp/linker_test.gointernal/projectors/assignment_listener.gointernal/projectors/assignment_test.gointernal/projectors/device_group_listener.gointernal/projectors/user_group_listener.gointernal/scim/auth.gointernal/scim/groups.gointernal/scim/groups_create.gointernal/scim/groups_helpers.gointernal/scim/groups_mutate.gointernal/scim/users.gointernal/scim/users_create.gointernal/scim/users_mutate.gointernal/store/compliance.gointernal/store/identity_link.gointernal/store/logs.gointernal/store/notfound.gointernal/store/postgres/compliance.gointernal/store/postgres/identity_link.gointernal/store/postgres/logs.gointernal/store/postgres/settings.gointernal/store/postgres/wire.gointernal/store/repos.gointernal/store/settings.gointernal/store/store.gointernal/testutil/postgres.go
Comment on lines
+33
to
41
| results, err := h.store.Repos().Compliance.DeviceResults(ctx, deviceID) | ||
| if err != nil { | ||
| return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to query compliance results") | ||
| } | ||
|
|
||
| summary, err := h.store.Queries().GetDeviceComplianceSummary(ctx, deviceID) | ||
| summary, err := h.store.Repos().Compliance.DeviceSummary(ctx, deviceID) | ||
| if err != nil { | ||
| return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to query compliance summary") | ||
| } |
There was a problem hiding this comment.
Map missing-device compliance to NotFound instead of Internal.
The summary lookup now goes through repo not-found normalization, but this branch still always returns Internal. This can return the wrong API code for a missing device.
Suggested fix
- results, err := h.store.Repos().Compliance.DeviceResults(ctx, deviceID)
- if err != nil {
- return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to query compliance results")
- }
-
summary, err := h.store.Repos().Compliance.DeviceSummary(ctx, deviceID)
if err != nil {
- return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to query compliance summary")
+ return nil, handleGetError(ctx, err, ErrDeviceNotFound, "device not found")
}
+
+ results, err := h.store.Repos().Compliance.DeviceResults(ctx, deviceID)
+ if err != nil {
+ return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to query compliance results")
+ }🤖 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/api/compliance_handler.go` around lines 33 - 41, The DeviceResults
error branch currently always returns an internal error (apiErrorCtx with
ErrInternal/connect.CodeInternal) even when the repo indicates the device is
missing; update the error handling for
h.store.Repos().Compliance.DeviceResults(ctx, deviceID) to detect the repo "not
found" case (same normalization used by DeviceSummary) and return a NotFound API
error (use the NotFound error/code variant used elsewhere) instead of
ErrInternal, so missing-device errors map to NotFound while other errors still
return internal errors via apiErrorCtx.
Comment on lines
+101
to
104
| result, err := h.store.Repos().Logs.GetQueryResult(ctx, req.Msg.QueryId) | ||
| if err != nil { | ||
| return nil, apiErrorCtx(ctx, ErrQueryResultNotFound, connect.CodeNotFound, "log query result not found") | ||
| } |
There was a problem hiding this comment.
Differentiate not-found from backend failures in result lookup.
Line 102 currently maps every GetQueryResult failure to connect.CodeNotFound. That hides real backend failures as 404s; only store.IsNotFound(err) should return not-found, otherwise return internal.
As per coding guidelines, "Errors must use apiErrorCtx with proper Connect error codes."
💡 Suggested fix
func (h *LogsHandler) GetDeviceLogResult(ctx context.Context, req *connect.Request[pm.GetDeviceLogResultRequest]) (*connect.Response[pm.GetDeviceLogResultResponse], error) {
result, err := h.store.Repos().Logs.GetQueryResult(ctx, req.Msg.QueryId)
if err != nil {
- return nil, apiErrorCtx(ctx, ErrQueryResultNotFound, connect.CodeNotFound, "log query result not found")
+ if store.IsNotFound(err) {
+ return nil, apiErrorCtx(ctx, ErrQueryResultNotFound, connect.CodeNotFound, "log query result not found")
+ }
+ h.logger.Error("failed to get log query result", "query_id", req.Msg.QueryId, "error", err)
+ return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to get log query result")
}🤖 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/api/logs_handler.go` around lines 101 - 104, The current error
handling around h.store.Repos().Logs.GetQueryResult treats every error as
NotFound; change it to call store.IsNotFound(err) and only return
apiErrorCtx(ctx, ErrQueryResultNotFound, connect.CodeNotFound, ...) for that
case, otherwise return an internal error via apiErrorCtx with
connect.CodeInternal (e.g., apiErrorCtx(ctx, err, connect.CodeInternal, "failed
to get log query result")). Update the GetQueryResult error branch to
discriminate store.IsNotFound(err) vs other errors accordingly.
Comment on lines
+142
to
143
| if !store.IsNotFound(err) { | ||
| return nil, fmt.Errorf("lookup identity link: %w", err) |
There was a problem hiding this comment.
Guard the fallback branch with err != nil before classifying not-found.
After stale-link cleanup (Line 102), the code intentionally falls through with a nil err; Line 142 can then return early and skip Step 2/3 linking logic.
Proposed fix
- if !store.IsNotFound(err) {
+ if err != nil && !store.IsNotFound(err) {
return nil, fmt.Errorf("lookup identity link: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if !store.IsNotFound(err) { | |
| return nil, fmt.Errorf("lookup identity link: %w", err) | |
| if err != nil && !store.IsNotFound(err) { | |
| return nil, fmt.Errorf("lookup identity link: %w", 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/idp/linker.go` around lines 142 - 143, The current conditional
unconditionally calls store.IsNotFound(err) and can misclassify a nil error from
the stale-link cleanup; change the guard so you only classify not-found when err
is non-nil (e.g. replace the condition with "if err != nil &&
!store.IsNotFound(err)" or first check "if err != nil { if
!store.IsNotFound(err) return ... }") so that a nil err falls through into the
Step 2/3 linking logic; update the branch containing store.IsNotFound(err)
accordingly.
Continues the per-domain migration started by Wave B.1 (#246). Three small handlers move from Store.Queries() to domain-specific repos under store.Repos(); cross-domain reads (GetUserByID, GetDeviceByID) stay on Queries until those domains migrate in later sub-waves. Domains migrated: - SettingsRepo (GetServer) — settings_handler.go. - IdentityLinkRepo (Get / ListForUser / CountForUser) — identity_link_handler.go + populateUserIdentityLinks in user_handler.go. ListForUser returns the join-shape IdentityLinkWithProvider so handlers can't confuse it with the basic Get shape. - LogsRepo (CreateQueryResult / GetQueryResult / ExpirePendingQueryResult) — logs_handler.go. First write-side migration in Wave B; the :exec signatures stay opaque to handlers, matching the read-side pattern. Each new repo's Postgres impl translates pgx.ErrNoRows to store.ErrNotFound at the boundary, matching the contract established in Wave A (#244) and applied in Wave B.1. Non-goals: - User / device repos — cross-domain reads in these handlers remain on Store.Queries() until those domains migrate. - Compliance-policy queries — separate sub-wave. Closes #247.
PaulDotterer
force-pushed
the
feat/247-settings-idlink-logs-repos
branch
from
e246862 to
fc5711f
Compare
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 `@internal/api/user_handler.go`:
- Around line 802-805: The call to h.store.Repos().IdentityLink.ListForUser is
ignoring errors with a bare return; change this to handle the error explicitly
by logging or returning it to the caller (e.g., wrap with context like "failed
to load identity links for user <user.Id>") and return an appropriate error/HTTP
response instead of a silent return so failures are visible and traceable;
ensure the error path in the surrounding handler function returns the error (or
sends a proper error response) rather than aborting silently.
🪄 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
Run ID: d78125b0-690a-47ad-8aa2-1b859bee0c73
📒 Files selected for processing (12)
internal/api/identity_link_handler.gointernal/api/logs_handler.gointernal/api/settings_handler.gointernal/api/user_handler.gointernal/store/identity_link.gointernal/store/logs.gointernal/store/postgres/identity_link.gointernal/store/postgres/logs.gointernal/store/postgres/settings.gointernal/store/postgres/wire.gointernal/store/repos.gointernal/store/settings.go
✅ Files skipped from review due to trivial changes (1)
- internal/store/identity_link.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/store/settings.go
- internal/api/identity_link_handler.go
- internal/store/postgres/settings.go
- internal/store/logs.go
- internal/store/repos.go
- internal/api/logs_handler.go
- internal/store/postgres/identity_link.go
- internal/store/postgres/logs.go
Comment on lines
+802
to
805
| links, err := h.store.Repos().IdentityLink.ListForUser(ctx, user.Id) | ||
| if err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
Don’t silently ignore identity-link load errors.
At Line 803, the repo error is dropped with a bare return, so failures are invisible and the response may be partially populated without traceability.
Suggested fix
func (h *UserHandler) populateUserIdentityLinks(ctx context.Context, user *pm.User) {
links, err := h.store.Repos().IdentityLink.ListForUser(ctx, user.Id)
if err != nil {
+ h.logger.Warn("failed to load identity links", "user_id", user.Id, "error", err)
return
}
for _, link := range links {
user.IdentityLinks = append(user.IdentityLinks, identityLinkRowToProto(link))
}
}As per coding guidelines: internal/api/*_handler.go: “Never silently ignore errors.”
🤖 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/api/user_handler.go` around lines 802 - 805, The call to
h.store.Repos().IdentityLink.ListForUser is ignoring errors with a bare return;
change this to handle the error explicitly by logging or returning it to the
caller (e.g., wrap with context like "failed to load identity links for user
<user.Id>") and return an appropriate error/HTTP response instead of a silent
return so failures are visible and traceable; ensure the error path in the
surrounding handler function returns the error (or sends a proper error
response) rather than aborting silently.
This was referenced May 15, 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
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.
Summary
Continues the per-domain migration started by Wave B.1 (#246). Three small single-handler domains move from `Store.Queries()` to domain-specific repos under `store.Repos()`; cross-domain reads stay on `Queries()` until those domains migrate in later sub-waves.
Stacked on #246. Diff currently shows Wave A + B.1 + B.2 commits; after #244 / #246 merge, the diff collapses to B.2 only.
Domains migrated
`ListForUser` returns the join-shape `IdentityLinkWithProvider` so handlers can't confuse it with the basic Get shape (which doesn't carry provider display fields).
Each Postgres impl translates `pgx.ErrNoRows` → `store.ErrNotFound` at the boundary, matching Wave A's contract.
Non-goals
Acceptance
Test plan
Part of #242. Closes #247.
Summary by CodeRabbit
Release Notes