fix(api/plans): differentiate ErrNotFound->404 + add structured slog on createPlan errors (closes #944) - #946
Merged
cristim merged 2 commits intoJun 5, 2026
Conversation
cristim
added
triaged
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR standardizes HTTP error classification in the ChangesError handling standardization in createPlan
Sequence DiagramsequenceDiagram
participant Client
participant createPlan
participant CreatePurchasePlan
participant SetPlanAccounts
participant mapCreatePlanStorageError
participant Logger
Client->>createPlan: POST /api/plans
createPlan->>CreatePurchasePlan: config.CreatePurchasePlan(ctx, plan)
CreatePurchasePlan-->>createPlan: error (DB failure or ErrNotFound)
createPlan->>mapCreatePlanStorageError: err
alt ErrNotFound
mapCreatePlanStorageError-->>createPlan: 404 ClientError
createPlan-->>Client: 404 response
else Other error
mapCreatePlanStorageError->>Logger: ERROR with context
mapCreatePlanStorageError-->>createPlan: 500 ClientError
createPlan-->>Client: 500 response
end
Client->>createPlan: POST /api/plans (valid)
createPlan->>SetPlanAccounts: h.config.SetPlanAccounts(ctx, plan.ID, accounts)
SetPlanAccounts-->>createPlan: error (account not found or DB)
createPlan->>mapCreatePlanStorageError: err
mapCreatePlanStorageError-->>createPlan: 404 or 500 ClientError
createPlan-->>Client: 404 or 500 response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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 |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
…n createPlan errors (closes #944) Three call sites in createPlan returned raw/wrapped plain errors that the router defaulted to HTTP 500: 1. CreatePurchasePlan: now maps ErrNotFound->404; any other DB error logs provider/service/account-count context via logging.Errorf and returns a well-formed 500 ClientError instead of leaking internal details. 2. validatePlanAccountProviders (inner GetCloudAccount path): already returns ClientError(404) for nil accounts; the function is now called from createPlan with a rollback guard so partial plan rows are cleaned up on validation failure. 3. SetPlanAccounts: maps ErrNotFound->404; transient DB errors log plan-ID and account-count then return 500 ClientError. Also adds: - validateTargetAccounts: requires non-empty target_accounts with valid UUIDs (rejects "universal plans" that have no plan_accounts rows) - TargetAccounts field on PlanRequest - rollbackPlan closure: deletes the partial purchase_plans row if validatePlanAccountProviders or SetPlanAccounts fail - Three regression tests: RejectsEmptyTargetAccounts, UnknownAccountReturns404, DBErrorOnCreateReturns500WithLog
cristim
force-pushed
the
fix/create-plan-500-observability
branch
from
June 4, 2026 13:47
0fce515 to
94f8805
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jun 5, 2026
…unt validation log
The GetCloudAccount DB-error path inside validatePlanAccountProviders
returned fmt.Errorf("accounts: failed to get account %s: %w", aid, getErr),
a non-ClientError that propagated to the router's
logging.Errorf("API error: %v"), leaking the raw account UUID and the
raw DB error string into logs (gap left by #946 which closed #944).
Wrap the error with the existing mapCreatePlanStorageError helper so
that: (a) a DB error emits a structured logging.Errorf carrying only
the derived provider list and account count (never the UUID), and
(b) returns a generic 500 ClientError ("failed to validate plan
accounts") with no raw UUID or DB string. The ErrNotFound sentinel
still maps to 404; the explicit (nil, nil) not-found path is unchanged.
Add logging.SetOutput as a test seam on the default logger and two
regression tests:
- TestValidatePlanAccountProviders_GetAccountDBError_NoPIILeak: asserts
that a DB error yields a 500 ClientError whose message and emitted
log contain neither the account UUID nor the raw DB error string.
- TestValidatePlanAccountProviders_AccountNotFound_Still404: guards that
the not-found path still returns a 404 referencing the missing ID.
Also fixes the pre-existing build failure on the base branch
(Session.Role field removed in #940 but the test was not updated) and
gofmt drift in handler_purchases_guards_test.go.
closes #965
cristim
added a commit
that referenced
this pull request
Jun 7, 2026
…unt validation log
The GetCloudAccount DB-error path inside validatePlanAccountProviders
returned fmt.Errorf("accounts: failed to get account %s: %w", aid, getErr),
a non-ClientError that propagated to the router's
logging.Errorf("API error: %v"), leaking the raw account UUID and the
raw DB error string into logs (gap left by #946 which closed #944).
Wrap the error with the existing mapCreatePlanStorageError helper so
that: (a) a DB error emits a structured logging.Errorf carrying only
the derived provider list and account count (never the UUID), and
(b) returns a generic 500 ClientError ("failed to validate plan
accounts") with no raw UUID or DB string. The ErrNotFound sentinel
still maps to 404; the explicit (nil, nil) not-found path is unchanged.
Add logging.SetOutput as a test seam on the default logger and two
regression tests:
- TestValidatePlanAccountProviders_GetAccountDBError_NoPIILeak: asserts
that a DB error yields a 500 ClientError whose message and emitted
log contain neither the account UUID nor the raw DB error string.
- TestValidatePlanAccountProviders_AccountNotFound_Still404: guards that
the not-found path still returns a 404 referencing the missing ID.
Also fixes the pre-existing build failure on the base branch
(Session.Role field removed in #940 but the test was not updated) and
gofmt drift in handler_purchases_guards_test.go.
closes #965
cristim
added a commit
that referenced
this pull request
Jun 8, 2026
…unt validation log
The GetCloudAccount DB-error path inside validatePlanAccountProviders
returned fmt.Errorf("accounts: failed to get account %s: %w", aid, getErr),
a non-ClientError that propagated to the router's
logging.Errorf("API error: %v"), leaking the raw account UUID and the
raw DB error string into logs (gap left by #946 which closed #944).
Wrap the error with the existing mapCreatePlanStorageError helper so
that: (a) a DB error emits a structured logging.Errorf carrying only
the derived provider list and account count (never the UUID), and
(b) returns a generic 500 ClientError ("failed to validate plan
accounts") with no raw UUID or DB string. The ErrNotFound sentinel
still maps to 404; the explicit (nil, nil) not-found path is unchanged.
Add logging.SetOutput as a test seam on the default logger and two
regression tests:
- TestValidatePlanAccountProviders_GetAccountDBError_NoPIILeak: asserts
that a DB error yields a 500 ClientError whose message and emitted
log contain neither the account UUID nor the raw DB error string.
- TestValidatePlanAccountProviders_AccountNotFound_Still404: guards that
the not-found path still returns a 404 referencing the missing ID.
Also fixes the pre-existing build failure on the base branch
(Session.Role field removed in #940 but the test was not updated) and
gofmt drift in handler_purchases_guards_test.go.
closes #965
4 tasks
cristim
added a commit
that referenced
this pull request
Jun 8, 2026
…unt validation log (#969) * sec(api/plans): stop leaking raw account UUID + DB error in plan-account validation log The GetCloudAccount DB-error path inside validatePlanAccountProviders returned fmt.Errorf("accounts: failed to get account %s: %w", aid, getErr), a non-ClientError that propagated to the router's logging.Errorf("API error: %v"), leaking the raw account UUID and the raw DB error string into logs (gap left by #946 which closed #944). Wrap the error with the existing mapCreatePlanStorageError helper so that: (a) a DB error emits a structured logging.Errorf carrying only the derived provider list and account count (never the UUID), and (b) returns a generic 500 ClientError ("failed to validate plan accounts") with no raw UUID or DB string. The ErrNotFound sentinel still maps to 404; the explicit (nil, nil) not-found path is unchanged. Add logging.SetOutput as a test seam on the default logger and two regression tests: - TestValidatePlanAccountProviders_GetAccountDBError_NoPIILeak: asserts that a DB error yields a 500 ClientError whose message and emitted log contain neither the account UUID nor the raw DB error string. - TestValidatePlanAccountProviders_AccountNotFound_Still404: guards that the not-found path still returns a 404 referencing the missing ID. Also fixes the pre-existing build failure on the base branch (Session.Role field removed in #940 but the test was not updated) and gofmt drift in handler_purchases_guards_test.go. closes #965 * fix(api/plans): stop leaking raw DB error in plan-fetch validation branch (refs #965) getPlanForAccountProviderValidation returned fmt.Errorf("accounts: failed to get plan: %w", err) on a non-ErrNotFound storage error. Because this is not a ClientError, the router logged it via logging.Errorf("API error: %v"), emitting the raw DB error string on every POST /plans and PUT /plans/:id/accounts call that hits a DB failure (same endpoints as the sibling GetCloudAccount leak closed by PR #969). Fix: delegate to the existing mapCreatePlanStorageError helper (introduced by PR #969 for the GetCloudAccount branch) so the response is a generic 500 ClientError and the log line contains only a static format string -- no raw DB error, no plan UUID. Adds two regression tests: - TestGetPlanForAccountProviderValidation_GetPlanDBError_NoPIILeak: asserts the handler returns a 500 ClientError and that neither the client response nor the captured log buffer contain the raw DB error string; FAILS pre-fix. - TestGetPlanForAccountProviderValidation_PlanNotFound_Still404: guards that ErrNotFound still maps to 404 after the refactor. * sec(api): fix sibling PII leaks in plan/purchase handler paths (followup #965) PR #969 closed the raw-UUID + raw-DB-error leak in validatePlanAccountProviders and getPlanForAccountProviderValidation. Adversarial review surfaced five sibling leak sites of the exact same shape -- a non-ClientError wrapped with a raw account/plan UUID and the underlying DB error -- that all propagate through the router's `logging.Errorf("API error: %v")` path and leak both the UUID and the raw DB error string. Fixed sites (all use `mapCreatePlanStorageError` so a DB error becomes a generic 500 ClientError, and a structured log line is emitted without PII): - handler_plans.go:209 updatePlan -- existingPlan fetch - handler_plans.go:333 getPlanForPurchaseCreation -- plan fetch for createPlannedPurchases - handler_plans.go:447 patchPlan -- plan fetch for PATCH - handler_purchases.go:323/328 disablePlan -- GetPurchasePlan and UpdatePurchasePlan failure paths - handler_purchases.go:2006 lookupContactEmail -- the Warnf used to log the raw account UUID and the raw err; now logs only the failure point - handler_purchases.go:1955 gatherAccountContactEmails -- the wrap site used to re-leak the same account UUID through `%s: %w`; now returns a generic 500 ClientError (lookupContactEmail has already logged without PII) Test changes: - New: TestHandler_patchPlan_GetPlanDBError_NoPIILeak, TestHandler_updatePlan_GetPlanDBError_NoPIILeak, TestDisablePlan_GetPlanDBError_NoPIILeak, TestDisablePlan_UpdatePlanDBError_NoPIILeak, TestLookupContactEmail_GetAccountDBError_NoPIILeak, TestGatherAccountContactEmails_DBError_NoPIILeak -- each asserts the ClientError is 500 (server-side fault), the returned message contains neither the raw UUID nor the raw DB error string, and (where the path logs via the default logger) the captured log buffer contains neither either. Pattern mirrors the existing TestValidatePlanAccountProviders_GetAccountDBError_NoPIILeak. - Updated: TestHandler_patchPlan_NotFound, TestHandler_createPlannedPurchases_PlanNotFound -- assertions moved off the legacy "failed to get plan" wrapper string to the new contract: 500 ClientError without the raw plan UUID. Both tests return a plain `errors.New("not found")` from the mock (not config.ErrNotFound) so they exercise the 500 branch, not the 404 branch. - Updated: TestHandler_resolveApprovalRecipients_LookupErrorPropagates -- the test's stated intent (no silent globalNotify fallback) is retained via the require.Error guard; the ErrorIs(transient) assertion was guarding the implementation detail that broke the no-PII-leak contract. Replaced with 500-ClientError + no-UUID + no-raw-DB-error assertions consistent with the rest of the issue #965 family. Full internal/api suite: 1474 passed; full short suite: only two pre-existing internal/auth failures unrelated to this PR. refs #965 * sec(api/purchases): keep lookupContactEmail failure at error level The GetCloudAccount failure in lookupContactEmail was logged at Warnf, but its caller gatherAccountContactEmails maps the error into a generic 500 ClientError. The router returns ClientErrors via the branch that skips logging.Errorf("API error: %v"), so this log line is the only error-level breadcrumb for the resulting user-visible 500. Downgrading it to Warnf weakened error-level alerting for exactly the outage this change set is preserving visibility for. Restore Errorf while keeping the message generic (no raw account UUID, no raw DB error string) so the PII-leak fix is unaffected. Extend the regression test to assert the [ERROR] severity and the generic message; it fails pre-fix (emits [WARN]) and passes after. Addresses CodeRabbit review on PR #969.
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
ErrNotFound->404mapping +logging.Errorfcontext at all three DB call sites increatePlan(was returning opaque 500 on every error)validateTargetAccounts+rollbackPlan+SetPlanAccountswiring socreatePlanis on parity with thesetPlanAccountsendpoint (which already maps ErrNotFound->404)TargetAccountsfield toPlanRequestand enforces non-empty with UUID validation, preventing "universal plan" rowsFixes #944.
Test plan
TestHandler_createPlan-- updated to supplytarget_accountsand assertSetPlanAccountswas calledTestHandler_createPlan_RejectsEmptyTargetAccounts-- missing/emptytarget_accountsreturns 400,CreatePurchasePlannever calledTestHandler_createPlan_UnknownAccountReturns404-- account UUID not in store returns 404 (not 500); regression guard for the inconsistencyTestHandler_createPlan_DBErrorOnCreateReturns500WithLog-- DB error onCreatePurchasePlanreturns structured 500ClientError; raw error detail does not leak to callerTestHandler_HandleRequest_CreatePlan-- updated to passtarget_accountsthrough the full router pathgo test ./... -timeout 120sSummary by CodeRabbit
Bug Fixes
Tests