From 217e4534ab467133d7c289ff370e4bb436558a5a Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 19 Oct 2026 23:11:46 +0100 Subject: [PATCH 1/5] fix(api): return 400 for an unsupported analytics interval or dimension GET /api/history/analytics and /api/history/breakdown passed interval and dimension straight to the Postgres analytics client. Its allowlist rejected unknown values with a plain error, which the handler wrapped and the router answered as a 500. The handlers now check both values against the same allowlist functions (intervalToTruncUnit, dimensionToColumn) before any query runs and answer 400 with the accepted values. Defaults are unchanged: interval hourly, dimension service. Refs #1102 (item 1) Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_analytics.go | 38 +++++-- .../handler_analytics_invalid_params_test.go | 102 ++++++++++++++++++ 2 files changed, 134 insertions(+), 6 deletions(-) create mode 100644 internal/api/handler_analytics_invalid_params_test.go diff --git a/internal/api/handler_analytics.go b/internal/api/handler_analytics.go index 770878b04..e8d2eab10 100644 --- a/internal/api/handler_analytics.go +++ b/internal/api/handler_analytics.go @@ -131,9 +131,9 @@ func (h *Handler) getHistoryAnalytics(ctx context.Context, req *events.LambdaFun // Parse parameters accountID := params["account_id"] - interval := params["interval"] - if interval == "" { - interval = "hourly" + interval, err := analyticsInterval(params["interval"]) + if err != nil { + return nil, err } // Normalise + validate the global-filter provider chip, mirroring @@ -197,9 +197,9 @@ func (h *Handler) getHistoryBreakdown(ctx context.Context, req *events.LambdaFun } accountID := params["account_id"] - dimension := params["dimension"] - if dimension == "" { - dimension = "service" + dimension, err := analyticsDimension(params["dimension"]) + if err != nil { + return nil, err } err = h.validateAnalyticsAccountScope(ctx, session, accountID) @@ -229,6 +229,32 @@ func (h *Handler) getHistoryBreakdown(ctx context.Context, req *events.LambdaFun }, nil } +// analyticsInterval returns the requested bucket interval, defaulting to +// hourly. A value the store cannot bucket by is a 400 here; without this check +// it reached intervalToTruncUnit inside QueryHistory and came back as a 500. +func analyticsInterval(raw string) (string, error) { + if raw == "" { + return "hourly", nil + } + if _, err := intervalToTruncUnit(raw); err != nil { + return "", NewClientError(400, "invalid interval: must be one of hourly, daily, weekly, monthly") + } + return raw, nil +} + +// analyticsDimension returns the requested breakdown dimension, defaulting to +// service. A value the store cannot group by is a 400 here; without this check +// it reached dimensionToColumn inside QueryBreakdown and came back as a 500. +func analyticsDimension(raw string) (string, error) { + if raw == "" { + return "service", nil + } + if _, err := dimensionToColumn(raw); err != nil { + return "", NewClientError(400, "invalid dimension: must be one of service, provider, region, account") + } + return raw, nil +} + // validateAnalyticsAccountScope rejects scoped users who request analytics // without account_id or for an account outside their allowed_accounts list. // Admin/unrestricted sessions pass through (account_id may be empty). diff --git a/internal/api/handler_analytics_invalid_params_test.go b/internal/api/handler_analytics_invalid_params_test.go new file mode 100644 index 000000000..68549faa2 --- /dev/null +++ b/internal/api/handler_analytics_invalid_params_test.go @@ -0,0 +1,102 @@ +package api + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Regression tests for issue #1102 item 1: an unsupported analytics interval +// or dimension used to reach the Postgres client, fail its allowlist and come +// back as a 500. It is a bad request, so the handler now answers 400 before +// any query runs. + +func TestHandler_getHistoryAnalytics_UnsupportedIntervalIs400(t *testing.T) { + for _, interval := range []string{"yearly", "minute", "HOURLY", "hour"} { + t.Run(interval, func(t *testing.T) { + ctx := context.Background() + mockClient := new(MockAnalyticsClient) + // What the Postgres client answers for an interval outside its + // allowlist, so the pre-fix handler fails this test with a 500 + // instead of panicking on an unexpected call. + mockClient.On("QueryHistory", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, interval). + Return(nil, nil, fmt.Errorf("unsupported interval %q", interval)).Maybe() + mockAuth, req := adminAnalyticsReq(ctx) + handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)} + + _, err := handler.getHistoryAnalytics(ctx, req, map[string]string{"interval": interval}) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "want a client error, got %v", err) + assert.Equal(t, 400, ce.code) + assert.Contains(t, ce.message, "invalid interval") + mockClient.AssertNotCalled(t, "QueryHistory", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + }) + } +} + +func TestHandler_getHistoryAnalytics_SupportedIntervalsReachStore(t *testing.T) { + for _, interval := range []string{"hourly", "daily", "weekly", "monthly"} { + t.Run(interval, func(t *testing.T) { + ctx := context.Background() + mockClient := new(MockAnalyticsClient) + mockClient.On("QueryHistory", ctx, []string(nil), map[string][]string(nil), "", mock.Anything, mock.Anything, interval). + Return([]HistoryDataPoint{}, (*HistorySummary)(nil), nil) + mockAuth, req := adminAnalyticsReq(ctx) + handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)} + + result, err := handler.getHistoryAnalytics(ctx, req, map[string]string{"interval": interval}) + require.NoError(t, err) + resp, ok := result.(*AnalyticsResponse) + require.True(t, ok) + assert.Equal(t, interval, resp.Interval) + mockClient.AssertExpectations(t) + }) + } +} + +func TestHandler_getHistoryBreakdown_UnsupportedDimensionIs400(t *testing.T) { + for _, dimension := range []string{"tenant", "account_id", "SERVICE", "service_type"} { + t.Run(dimension, func(t *testing.T) { + ctx := context.Background() + mockClient := new(MockAnalyticsClient) + // The Postgres client's answer for an unknown dimension; see above. + mockClient.On("QueryBreakdown", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything, dimension). + Return(nil, fmt.Errorf("unsupported dimension %q", dimension)).Maybe() + mockAuth, req := adminAnalyticsReq(ctx) + handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)} + + _, err := handler.getHistoryBreakdown(ctx, req, map[string]string{"dimension": dimension}) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "want a client error, got %v", err) + assert.Equal(t, 400, ce.code) + assert.Contains(t, ce.message, "invalid dimension") + mockClient.AssertNotCalled(t, "QueryBreakdown", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + }) + } +} + +func TestHandler_getHistoryBreakdown_SupportedDimensionsReachStore(t *testing.T) { + for _, dimension := range []string{"service", "provider", "region", "account"} { + t.Run(dimension, func(t *testing.T) { + ctx := context.Background() + mockClient := new(MockAnalyticsClient) + mockClient.On("QueryBreakdown", ctx, []string(nil), map[string][]string(nil), mock.Anything, mock.Anything, dimension). + Return(map[string]BreakdownValue{}, nil) + mockAuth, req := adminAnalyticsReq(ctx) + handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)} + + result, err := handler.getHistoryBreakdown(ctx, req, map[string]string{"dimension": dimension}) + require.NoError(t, err) + resp, ok := result.(*BreakdownResponse) + require.True(t, ok) + assert.Equal(t, dimension, resp.Dimension) + mockClient.AssertExpectations(t) + }) + } +} From bbb5ebaeeb72ad15fa0aa88310075adf38adc9c3 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 19 Oct 2026 23:12:47 +0100 Subject: [PATCH 2/5] fix(api): refuse a provider change on PUT /api/accounts/{id} updateAccount validated the request's provider and returned it in the response, but UpdateCloudAccount never writes the provider column, so the change was silently dropped and the caller was told it had been saved. Saving it is not safe either: credentials, service overrides and plan memberships all depend on the provider, and plan membership is only checked against it when an account is added to a plan. So the provider is treated as fixed at creation: a PUT whose provider differs from the stored one now gets a 400 telling the caller to delete and re-add the account, and nothing is saved. The check runs after the account-scope check, so a caller who cannot see the account still gets a 404. The settings UI always sends the account's own provider, so it is unaffected. Refs #1102 (item 3) Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_accounts.go | 9 +++ ...andler_accounts_provider_immutable_test.go | 74 +++++++++++++++++++ 2 files changed, 83 insertions(+) create mode 100644 internal/api/handler_accounts_provider_immutable_test.go diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index a0e51ee1e..658e02043 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -573,6 +573,15 @@ func (h *Handler) updateAccount(ctx context.Context, httpReq *events.LambdaFunct return nil, err } + // The provider is fixed when the account is created: UpdateCloudAccount + // does not write it, and credentials, service overrides and plan + // memberships all depend on it. Refuse a change rather than echo back a + // provider that was never saved. + if req.Provider != existing.Provider { + return nil, NewClientError(400, fmt.Sprintf( + "provider cannot be changed (this account is %s); delete the account and add it again under the new provider", existing.Provider)) + } + account := cloudAccountFromRequest(req) account.ID = id account.CreatedAt = existing.CreatedAt diff --git a/internal/api/handler_accounts_provider_immutable_test.go b/internal/api/handler_accounts_provider_immutable_test.go new file mode 100644 index 000000000..497f6ef97 --- /dev/null +++ b/internal/api/handler_accounts_provider_immutable_test.go @@ -0,0 +1,74 @@ +package api + +import ( + "context" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Regression tests for issue #1102 item 3: PUT /api/accounts/{id} validated +// the request's provider and echoed it back, but UpdateCloudAccount never +// writes the provider column, so the change was silently dropped. A provider +// change is now refused with a 400 before anything is saved. + +func updateAccountRequest(body, id string) *events.LambdaFunctionURLRequest { + req := adminRequest(body) + req.RequestContext.HTTP.Method = "PUT" + req.RequestContext.HTTP.Path = "/api/accounts/" + id + return req +} + +func TestUpdateAccount_ProviderChangeIsRejected(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + + existing := sampleAccount() // provider aws + store := new(MockConfigStore) + store.On("GetCloudAccount", ctx, existing.ID).Return(&existing, nil) + // Registered so the pre-fix handler reaches the save and returns the + // echoed account instead of panicking on an unexpected call. + store.On("UpdateCloudAccount", ctx, mock.Anything).Return(nil).Maybe() + handler := &Handler{auth: mockAuth, config: store} + + body := `{"name":"Test Account","provider":"gcp","external_id":"123456789012","gcp_auth_mode":"application_default"}` + result, err := handler.updateAccount(ctx, updateAccountRequest(body, existing.ID), existing.ID) + + assert.Nil(t, result, "a provider that is never saved must not be echoed back") + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "want a client error, got %v", err) + assert.Equal(t, 400, ce.code) + assert.Contains(t, ce.message, "provider cannot be changed") + assert.Contains(t, ce.message, "aws") + store.AssertNotCalled(t, "UpdateCloudAccount", mock.Anything, mock.Anything) +} + +func TestUpdateAccount_SameProviderIsSaved(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + + existing := sampleAccount() + store := new(MockConfigStore) + store.On("GetCloudAccount", ctx, existing.ID).Return(&existing, nil) + store.On("UpdateCloudAccount", ctx, mock.MatchedBy(func(a *config.CloudAccount) bool { + return a.ID == existing.ID && a.Provider == "aws" && a.Name == "Renamed" + })).Return(nil).Once() + handler := &Handler{auth: mockAuth, config: store} + + body := `{"name":"Renamed","provider":"aws","external_id":"123456789012"}` + result, err := handler.updateAccount(ctx, updateAccountRequest(body, existing.ID), existing.ID) + require.NoError(t, err) + saved, ok := result.(*config.CloudAccount) + require.True(t, ok) + assert.Equal(t, "aws", saved.Provider) + assert.Equal(t, "Renamed", saved.Name) + store.AssertExpectations(t) +} From 106608d46313147e5e35e2ef0230f80828d52278 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 19 Oct 2026 23:16:44 +0100 Subject: [PATCH 3/5] fix(config): treat a non-UUID execution ID as not found POST /api/purchases/{id}/revoke asks GetExecutionByID whether the ID is a scheduled execution before it looks in purchase_history, and falls through on ErrNotFound. An Azure purchase ID is an ARM path, and execution_id is a uuid column, so Postgres refused the cast with SQLSTATE 22P02. GetExecutionByID returned that as a plain error, and every Azure revoke failed with a 500 before reaching the Azure return path. The unit tests mocked the lookup as ErrNotFound for ARM paths, so they never saw it. GetExecutionByID now maps 22P02 to ErrNotFound: no execution can have an ID that is not a UUID. Every other error is returned unchanged, so a real database failure is still a 500. Tests, on real Postgres: - the store returns ErrNotFound for ARM paths, an arbitrary string and "", and still finds a real execution afterwards; - revokePurchase with stubbed Azure SDK clients returns the Azure reservation and records revoked_at, and answers 404 for an unknown ARM ID. Both failed with the 22P02 error before. Refs #1102 (item 5) Co-Authored-By: Claude Opus 5.5 --- ...chases_revoke_azure_id_integration_test.go | 80 +++++++++++++++++++ internal/config/store_postgres.go | 19 +++++ ...res_execution_non_uuid_integration_test.go | 49 ++++++++++++ 3 files changed, 148 insertions(+) create mode 100644 internal/api/handler_purchases_revoke_azure_id_integration_test.go create mode 100644 internal/config/store_postgres_execution_non_uuid_integration_test.go diff --git a/internal/api/handler_purchases_revoke_azure_id_integration_test.go b/internal/api/handler_purchases_revoke_azure_id_integration_test.go new file mode 100644 index 000000000..e6f2b3544 --- /dev/null +++ b/internal/api/handler_purchases_revoke_azure_id_integration_test.go @@ -0,0 +1,80 @@ +//go:build integration + +package api + +import ( + "context" + "testing" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// TestRevokePurchase_AzureARMPurchaseIDOnRealPostgres is the regression test +// for issue #1102 item 5. revokePurchase first asks GetExecutionByID whether +// the ID is a scheduled execution. An Azure purchase ID is an ARM path, which +// Postgres refused to cast to the uuid execution_id column (SQLSTATE 22P02), so +// every Azure revoke failed with a 500 before reaching purchase_history. The +// unit tests mocked that lookup as ErrNotFound and never saw it. +// +// The store and the purchase_history row are real; only the two Azure SDK +// clients are stubs, so no cloud call is made. +func TestRevokePurchase_AzureARMPurchaseIDOnRealPostgres(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 90*time.Second) + t.Cleanup(cancel) + f := newCreateConcurrencyFixture(ctx, t) + + var acctID string + require.NoError(t, f.pool.QueryRow(ctx, + "SELECT id::text FROM cloud_accounts WHERE external_id = 'fixture-subscription-a'").Scan(&acctID)) + + const purchaseID = "/subscriptions/00000000-0000-0000-0000-00000000000a/providers/Microsoft.Capacity/reservationOrders/order-1102/reservations/res-1102" + purchasedAt := time.Now().UTC().Add(-24 * time.Hour) + require.NoError(t, f.store.SavePurchaseHistory(ctx, &config.PurchaseHistoryRecord{ + AccountID: "fixture-subscription-a", CloudAccountID: &acctID, PurchaseID: purchaseID, + Timestamp: purchasedAt, Provider: "azure", Service: "compute", Region: "eastus", + ResourceType: "Standard_D2s_v3", Count: 1, Term: 1, Payment: "monthly", + RevocationWindowClosesAt: config.RevocationWindowClosesAtFor("azure", purchasedAt), + })) + + returnClient := &stubReturnClient{} + f.handler.azureRevokeFactory = &azureRevokeClientFactory{ + newCredential: func() (azcore.TokenCredential, error) { return nil, nil }, + newCalculateRefundClient: func(azcore.TokenCredential) (azureCalculateRefundClient, error) { + return &stubCalcRefundClientWithAmount{amount: 42.5, currency: "USD", sessID: "sess-1102"}, nil + }, + newReturnClient: func(azcore.TokenCredential) (azureReturnClient, error) { return returnClient, nil }, + } + req := func(body string) *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-token"}, Body: body} + } + + t.Run("unknown ARM id is a 404, not a 500", func(t *testing.T) { + _, err := f.handler.revokePurchase(ctx, req(""), "/providers/Microsoft.Capacity/reservationOrders/no-such-order/reservations/none") + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "want a client error, got %v", err) + assert.Equal(t, 404, ce.code) + assert.Zero(t, returnClient.calls) + }) + + t.Run("Azure reservation is returned and recorded", func(t *testing.T) { + result, err := f.handler.revokePurchase(ctx, + req(`{"expected_refund_amount":42.5,"expected_refund_currency":"USD"}`), purchaseID) + require.NoError(t, err) + res, ok := result.(*revokePurchaseResult) + require.True(t, ok, "got %T", result) + assert.Equal(t, "revoked", res.Status) + assert.Equal(t, 1, returnClient.calls) + + var revokedAt *time.Time + require.NoError(t, f.pool.QueryRow(ctx, + "SELECT revoked_at FROM purchase_history WHERE purchase_id = $1", purchaseID).Scan(&revokedAt)) + assert.NotNil(t, revokedAt, "the revoke must be persisted on the purchase_history row") + }) +} diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index c90c71ab5..a7e05ea75 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1889,6 +1889,13 @@ func (s *PostgresStore) GetExecutionByID(ctx context.Context, executionID string ` executions, err := s.queryExecutions(ctx, query, executionID) + if isInvalidTextRepresentation(err) { + // execution_id is a UUID column, and Postgres rejects an ID it cannot + // cast instead of matching no row. No execution can have such an ID, + // so it is not found. Callers such as the purchase revoke route look up + // IDs that may be an Azure ARM path and fall through on ErrNotFound. + return nil, fmt.Errorf("%w: execution %s", ErrNotFound, executionID) + } if err != nil { return nil, err } @@ -1900,6 +1907,18 @@ func (s *PostgresStore) GetExecutionByID(ctx context.Context, executionID string return &executions[0], nil } +// pgInvalidTextRepresentation is the SQLSTATE Postgres raises when a text +// value cannot be cast to the column type, for example a non-UUID string +// compared with a uuid column. +const pgInvalidTextRepresentation = "22P02" + +// isInvalidTextRepresentation reports whether err is Postgres refusing a text value +// for a typed column (SQLSTATE 22P02). +func isInvalidTextRepresentation(err error) bool { + var pgErr *pgconn.PgError + return errors.As(err, &pgErr) && pgErr.Code == pgInvalidTextRepresentation +} + // GetExecutionByPlanAndDate retrieves execution for a specific plan and date. func (s *PostgresStore) GetExecutionByPlanAndDate(ctx context.Context, planID string, scheduledDate time.Time) (*PurchaseExecution, error) { query := ` diff --git a/internal/config/store_postgres_execution_non_uuid_integration_test.go b/internal/config/store_postgres_execution_non_uuid_integration_test.go new file mode 100644 index 000000000..60e2657af --- /dev/null +++ b/internal/config/store_postgres_execution_non_uuid_integration_test.go @@ -0,0 +1,49 @@ +//go:build integration + +package config + +// Real-PostgreSQL test for issue #1102 item 5. purchase_executions.execution_id +// is a UUID column, and POST /api/purchases/{id}/revoke asks GetExecutionByID +// about every ID before falling back to purchase_history. An Azure purchase ID +// is an ARM path, which Postgres refuses to cast to uuid (SQLSTATE 22P02), so +// the lookup failed with an error the handler answered as a 500 instead of the +// ErrNotFound that sends the revoke on to the purchase_history path. + +import ( + "context" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +func TestGetExecutionByID_NonUUIDIsNotFoundPostgres(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 90*time.Second) + defer cancel() + store := setupLadderStore(ctx, t) + + for _, id := range []string{ + "/subscriptions/00000000-0000-0000-0000-000000000000/providers/Microsoft.Capacity/reservationOrders/order-1/reservations/res-1", + "/providers/Microsoft.Capacity/reservationOrders/order-1", + "pid-1", + "", + } { + exec, err := store.GetExecutionByID(ctx, id) + require.ErrorIs(t, err, ErrNotFound, "id %q", id) + require.Nil(t, exec) + } + + // The connection is still usable afterwards, and a well-formed UUID behaves + // as before: missing is ErrNotFound, present is returned. + _, err := store.GetExecutionByID(ctx, uuid.NewString()) + require.ErrorIs(t, err, ErrNotFound) + + execID := uuid.NewString() + require.NoError(t, store.SavePurchaseExecution(ctx, &PurchaseExecution{ + ExecutionID: execID, Status: "scheduled", ScheduledDate: time.Now(), + })) + got, err := store.GetExecutionByID(ctx, execID) + require.NoError(t, err) + require.Equal(t, execID, got.ExecutionID) +} From 340cf181925e59e9a167427374eecfe7492d49b3 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 19 Oct 2026 23:23:59 +0100 Subject: [PATCH 4/5] fix(api): approve a registration and create its account atomically approveRegistration flipped the registration to approved, then created the cloud account and linked it in separate statements. When the create failed (the provider + external_id already registered, or any database error), the registration stayed approved with no account. Approving it again was refused with a 409 and the UI offered no way back. A failed link was only logged. A new store method, ApproveAccountRegistration, runs the conditional status change (WHERE status = 'pending'), the account insert and the link in one transaction. The status change goes first, so its row lock queues any concurrent approve or reject of the same registration behind the transaction. After a commit the other request matches no row and gets 409; after a rollback it proceeds as if the approval never ran. The handler maps a conflict or a duplicate account to 409 and anything else to 500. Credentials are still stored after the commit, best effort, as before. The in-memory reset of reg.RegCredentialPayload is removed: neither registration UPDATE writes that column, so it never cleared anything. That gap is pre-existing and left for a separate fix. Tests: - unit: the handler makes the single store call and none of the old piecemeal ones, and maps conflict/duplicate/other errors; - real Postgres: a duplicate account leaves the registration pending and unreviewed (failed before: approved, no account); 8 concurrent approvals yield one success, seven 409s, one account, and a link to it; a reject issued while an approval is inside its account insert waits on the row lock and then gets 409 (approval committed) or succeeds with no account left (approval failed), with the insert slowed or failed by a test-only trigger. Refs #1102 (item 2) Co-Authored-By: Claude Opus 5.5 --- internal/analytics/collector_test.go | 3 + internal/api/handler_registrations.go | 39 +-- ..._registrations_approve_integration_test.go | 234 ++++++++++++++++++ .../api/handler_registrations_approve_test.go | 114 +++++++++ internal/config/interfaces.go | 4 + .../config/store_postgres_registrations.go | 29 +++ internal/mocks/stores.go | 6 + internal/server/test_helpers_test.go | 3 + 8 files changed, 413 insertions(+), 19 deletions(-) create mode 100644 internal/api/handler_registrations_approve_integration_test.go create mode 100644 internal/api/handler_registrations_approve_test.go diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index f5422c072..30063ef15 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -469,6 +469,9 @@ func (m *mockConfigStore) UpdateAccountRegistration(_ context.Context, _ *config func (m *mockConfigStore) TransitionRegistrationStatus(_ context.Context, _ *config.AccountRegistration, _ string, _ *string) error { return nil } +func (m *mockConfigStore) ApproveAccountRegistration(_ context.Context, _ *config.AccountRegistration, _ *config.CloudAccount, _ *string) error { + return nil +} func (m *mockConfigStore) DeleteAccountRegistration(_ context.Context, _ string) error { return nil } diff --git a/internal/api/handler_registrations.go b/internal/api/handler_registrations.go index 344da4ffc..a6faa08be 100644 --- a/internal/api/handler_registrations.go +++ b/internal/api/handler_registrations.go @@ -286,18 +286,8 @@ func (h *Handler) approveRegistration(ctx context.Context, httpReq *events.Lambd return nil, err } - // Atomically transition to "approved" first — prevents double-approval. - reg.Status = "approved" h.setReviewMetadata(ctx, reg, httpReq) - // reviewed_by may be "admin-api-key" (not a UUID FK); validate before passing as actor. - if err := h.config.TransitionRegistrationStatus(ctx, reg, "pending", validUUIDPtrOrNil(reg.ReviewedBy)); err != nil { - if errors.Is(err, config.ErrRegistrationConflict) { - return nil, NewClientError(409, "registration was already processed by another request") - } - return nil, fmt.Errorf("registrations: transition: %w", err) - } - // Create the cloud account (only one request reaches here). now := time.Now() account := cloudAccountFromRequest(acctReq) account.ID = uuid.New().String() @@ -312,20 +302,18 @@ func (h *Handler) approveRegistration(ctx context.Context, httpReq *events.Lambd account.CreatedAt = now account.UpdatedAt = now - if err := h.config.CreateCloudAccount(ctx, account); err != nil { - return nil, classifyStoreError(err, fmt.Sprintf("an account with external ID %q already exists for %s", acctReq.ExternalID, acctReq.Provider)) + // Approve, create the account and link them in one transaction, so a + // failed account insert leaves the registration pending rather than + // approved with no account (issue #1102). The conditional status change + // inside it also stops a double approval. reviewed_by may be + // "admin-api-key" (not a UUID FK); validate before passing as actor. + if err := h.config.ApproveAccountRegistration(ctx, reg, account, validUUIDPtrOrNil(reg.ReviewedBy)); err != nil { + return nil, classifyApproveRegistrationError(err, acctReq) } // Auto-store credentials if the registration included them. h.storeRegistrationCredentials(ctx, reg, account.ID) - // Link the cloud account and wipe the credential payload from the registration record. - reg.CloudAccountID = &account.ID - reg.RegCredentialPayload = "" // Don't keep encrypted credentials longer than needed. - if err := h.config.UpdateAccountRegistration(ctx, reg); err != nil { - logging.Warnf("registration %s approved but failed to link cloud_account_id: %v", reg.ID, err) - } - h.notifyRegistrant(reg, email.RegistrationDecisionData{ AccountName: reg.AccountName, Provider: reg.Provider, ExternalID: reg.ExternalID, Decision: "approved", @@ -334,6 +322,19 @@ func (h *Handler) approveRegistration(ctx context.Context, httpReq *events.Lambd return account, nil } +// classifyApproveRegistrationError maps an ApproveAccountRegistration failure +// to the response: 409 when another request already decided the registration +// or the account is already registered, 500 otherwise. +func classifyApproveRegistrationError(err error, acctReq CloudAccountRequest) error { + if errors.Is(err, config.ErrRegistrationConflict) { + return NewClientError(409, "registration was already processed by another request") + } + if isDuplicateKeyError(err) { + return NewClientError(409, fmt.Sprintf("an account with external ID %q already exists for %s", acctReq.ExternalID, acctReq.Provider)) + } + return fmt.Errorf("registrations: approve: %w", err) +} + // accountHasCredentialFreePath returns true when the account's auth // mode resolves a credential without any stored secret — either via // ambient instance credentials (role assumption, managed identity, diff --git a/internal/api/handler_registrations_approve_integration_test.go b/internal/api/handler_registrations_approve_integration_test.go new file mode 100644 index 000000000..2ae68dfb0 --- /dev/null +++ b/internal/api/handler_registrations_approve_integration_test.go @@ -0,0 +1,234 @@ +//go:build integration + +package api + +import ( + "context" + "encoding/json" + "fmt" + "sync" + "testing" + "time" + + "github.com/aws/aws-lambda-go/events" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Real-PostgreSQL tests for issue #1102 item 2: approving a registration +// flipped it to approved and only then created the cloud account, in separate +// statements, so a failed create left it approved with no account. Approval, +// account creation and the link now share one transaction. + +func newRegistrationForApproval(ctx context.Context, t *testing.T, f *createConcurrencyFixture, externalID string) string { + t.Helper() + reg := &config.AccountRegistration{ + ReferenceToken: "tok-" + externalID, Status: "pending", Provider: "azure", + ExternalID: externalID, AccountName: "Fixture " + externalID, ContactEmail: "owner@example.com", + } + require.NoError(t, f.store.CreateAccountRegistration(ctx, reg)) + return reg.ID +} + +func approveRequest(t *testing.T, externalID string) *events.LambdaFunctionURLRequest { + t.Helper() + body, err := json.Marshal(CloudAccountRequest{ + Name: "Fixture " + externalID, Provider: "azure", ExternalID: externalID, AzureAuthMode: "managed_identity", + }) + require.NoError(t, err) + return &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-token"}, Body: string(body)} +} + +type registrationRow struct { + status string + cloudAccountID *string + reviewedAt *time.Time +} + +func readRegistration(ctx context.Context, t *testing.T, pool *pgxpool.Pool, id string) registrationRow { + t.Helper() + var r registrationRow + require.NoError(t, pool.QueryRow(ctx, + `SELECT status, cloud_account_id::text, reviewed_at FROM account_registrations WHERE id = $1`, id, + ).Scan(&r.status, &r.cloudAccountID, &r.reviewedAt)) + return r +} + +func countAccounts(ctx context.Context, t *testing.T, pool *pgxpool.Pool, externalID string) int { + t.Helper() + var n int + require.NoError(t, pool.QueryRow(ctx, + `SELECT count(*) FROM cloud_accounts WHERE provider = 'azure' AND external_id = $1`, externalID).Scan(&n)) + return n +} + +// A failed account insert must leave the registration exactly as it was. +func TestApproveRegistration_FailedAccountCreateLeavesRegistrationPending(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 90*time.Second) + t.Cleanup(cancel) + f := newCreateConcurrencyFixture(ctx, t) + + const ext = "fixture-subscription-dup" + require.NoError(t, f.store.CreateCloudAccount(ctx, &config.CloudAccount{ + Name: "Already registered", Provider: "azure", ExternalID: ext, Enabled: true, + })) + regID := newRegistrationForApproval(ctx, t, f, ext) + + _, err := f.handler.approveRegistration(ctx, approveRequest(t, ext), regID) + requireClientCode(t, err, 409) + + row := readRegistration(ctx, t, f.pool, regID) + assert.Equal(t, "pending", row.status, "a failed approval must not leave the registration approved") + assert.Nil(t, row.cloudAccountID) + assert.Nil(t, row.reviewedAt) + assert.Equal(t, 1, countAccounts(ctx, t, f.pool, ext)) + + // Still pending, so the admin can act on it again. + _, err = f.handler.rejectRegistration(ctx, approveRequest(t, ext), regID) + require.NoError(t, err) +} + +// Concurrent approvals of one registration: exactly one wins, one account is +// created, and the registration is linked to it. +func TestApproveRegistration_ConcurrentApprovalsCreateOneAccount(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 90*time.Second) + t.Cleanup(cancel) + f := newCreateConcurrencyFixture(ctx, t) + + const ext = "fixture-subscription-race" + regID := newRegistrationForApproval(ctx, t, f, ext) + + const workers = 8 + var wg sync.WaitGroup + start := make(chan struct{}) + results := make([]any, workers) + errs := make([]error, workers) + for i := range workers { + wg.Add(1) + go func() { + defer wg.Done() + <-start + results[i], errs[i] = f.handler.approveRegistration(ctx, approveRequest(t, ext), regID) + }() + } + close(start) + wg.Wait() + + var winner *config.CloudAccount + for i := range workers { + if errs[i] == nil { + require.Nil(t, winner, "more than one approval succeeded") + winner = results[i].(*config.CloudAccount) + continue + } + requireClientCode(t, errs[i], 409) + } + require.NotNil(t, winner, "no approval succeeded") + + row := readRegistration(ctx, t, f.pool, regID) + assert.Equal(t, "approved", row.status) + require.NotNil(t, row.cloudAccountID) + assert.Equal(t, winner.ID, *row.cloudAccountID) + assert.Equal(t, 1, countAccounts(ctx, t, f.pool, ext)) +} + +// installSlowAccountInsert makes every cloud_accounts insert sleep, and fail +// when the external_id ends in "-fail", so an approval holds its transaction +// open long enough for a concurrent reject to queue behind it. +func installSlowAccountInsert(ctx context.Context, t *testing.T, pool *pgxpool.Pool) { + t.Helper() + _, err := pool.Exec(ctx, ` + CREATE FUNCTION test_slow_account_insert() RETURNS trigger AS $$ + BEGIN + PERFORM pg_sleep(1); + IF NEW.external_id LIKE '%-fail' THEN + RAISE EXCEPTION 'injected cloud_accounts insert failure'; + END IF; + RETURN NEW; + END $$ LANGUAGE plpgsql; + CREATE TRIGGER test_slow_account_insert BEFORE INSERT ON cloud_accounts + FOR EACH ROW EXECUTE FUNCTION test_slow_account_insert();`) + require.NoError(t, err) +} + +// waitForActivity polls pg_stat_activity until a backend matches the predicate. +func waitForActivity(ctx context.Context, t *testing.T, pool *pgxpool.Pool, predicate string) { + t.Helper() + require.Eventually(t, func() bool { + var n int + err := pool.QueryRow(ctx, `SELECT count(*) FROM pg_stat_activity WHERE `+predicate).Scan(&n) + return err == nil && n > 0 + }, 10*time.Second, 10*time.Millisecond, "no backend matched %s", predicate) +} + +// A reject that arrives while an approval is mid-transaction waits for it. +// When the approval commits, the reject gets 409; when the approval fails and +// rolls back, the reject proceeds and no account or approval is left behind. +func TestApproveRegistration_RejectQueuedBehindApproval(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) + t.Cleanup(cancel) + f := newCreateConcurrencyFixture(ctx, t) + installSlowAccountInsert(ctx, t, f.pool) + + for _, tc := range []struct { + ext string + approveCode int // 0: approval succeeds + wantStatus string + wantRejectCode int // 0: reject succeeds + wantAccounts int + }{ + {ext: "fixture-subscription-commit", wantStatus: "approved", wantRejectCode: 409, wantAccounts: 1}, + {ext: "fixture-subscription-fail", approveCode: 500, wantStatus: "rejected", wantAccounts: 0}, + } { + t.Run(tc.ext, func(t *testing.T) { + regID := newRegistrationForApproval(ctx, t, f, tc.ext) + + approveDone := make(chan error, 1) + go func() { + _, err := f.handler.approveRegistration(ctx, approveRequest(t, tc.ext), regID) + approveDone <- err + }() + // The approval has changed the status and is now inside the + // account insert, holding the registration row lock. + waitForActivity(ctx, t, f.pool, `wait_event = 'PgSleep' AND query LIKE '%INSERT INTO cloud_accounts%'`) + + rejectDone := make(chan error, 1) + go func() { + _, err := f.handler.rejectRegistration(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + }, regID) + rejectDone <- err + }() + // The reject's status change is blocked on that lock, not run after. + waitForActivity(ctx, t, f.pool, `wait_event_type = 'Lock' AND query LIKE '%UPDATE account_registrations%'`) + + approveErr := <-approveDone + rejectErr := <-rejectDone + switch tc.approveCode { + case 0: + require.NoError(t, approveErr) + default: + require.Error(t, approveErr) + _, isClient := IsClientError(approveErr) + assert.False(t, isClient, "an injected database failure is a 500, got %v", approveErr) + } + if tc.wantRejectCode == 0 { + require.NoError(t, rejectErr) + } else { + requireClientCode(t, rejectErr, tc.wantRejectCode) + } + + row := readRegistration(ctx, t, f.pool, regID) + assert.Equal(t, tc.wantStatus, row.status) + assert.Equal(t, tc.wantAccounts, countAccounts(ctx, t, f.pool, tc.ext)) + if tc.wantAccounts == 0 { + assert.Nil(t, row.cloudAccountID, fmt.Sprintf("registration %s", regID)) + } else { + assert.NotNil(t, row.cloudAccountID) + } + }) + } +} diff --git a/internal/api/handler_registrations_approve_test.go b/internal/api/handler_registrations_approve_test.go new file mode 100644 index 000000000..d0d352701 --- /dev/null +++ b/internal/api/handler_registrations_approve_test.go @@ -0,0 +1,114 @@ +package api + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Unit tests for approveRegistration after issue #1102 item 2. Approval used +// to flip the registration to approved, then create the account in a separate +// statement, so a failed create left an approved registration with no account. +// The handler now makes one ApproveAccountRegistration call, which does all of +// it in a transaction; the real-Postgres atomicity and race tests are in +// handler_registrations_approve_integration_test.go. + +const approveRegID = "5a5a5a5a-1102-4102-8102-000000000001" + +const approveRegBody = `{"name":"Fixture Sub","provider":"azure","external_id":"fixture-subscription-1102","azure_auth_mode":"managed_identity"}` + +func pendingRegistrationFixture() *config.AccountRegistration { + return &config.AccountRegistration{ + ID: approveRegID, Status: "pending", Provider: "azure", + ExternalID: "fixture-subscription-1102", AccountName: "Fixture Sub", + } +} + +// newApproveRegistrationHandler wires an admin session and a pending +// registration. The pre-#1102 steps are registered with .Maybe() so a +// regression to them fails the AssertNotCalled checks instead of panicking. +func newApproveRegistrationHandler(ctx context.Context, approveErr error) (*Handler, *MockConfigStore) { + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + store := new(MockConfigStore) + store.On("GetAccountRegistration", ctx, approveRegID).Return(pendingRegistrationFixture(), nil) + store.On("ApproveAccountRegistration", ctx, mock.Anything, mock.Anything, mock.Anything).Return(approveErr) + store.On("TransitionRegistrationStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + store.On("CreateCloudAccount", mock.Anything, mock.Anything).Return(nil).Maybe() + store.On("UpdateAccountRegistration", mock.Anything, mock.Anything).Return(nil).Maybe() + return &Handler{auth: mockAuth, config: store}, store +} + +// assertNoPiecemealApproval checks that none of the separate, non-atomic +// steps the handler used before #1102 ran. +func assertNoPiecemealApproval(t *testing.T, store *MockConfigStore) { + t.Helper() + store.AssertNotCalled(t, "TransitionRegistrationStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + store.AssertNotCalled(t, "CreateCloudAccount", mock.Anything, mock.Anything) + store.AssertNotCalled(t, "UpdateAccountRegistration", mock.Anything, mock.Anything) +} + +func TestApproveRegistration_ApprovesAndCreatesInOneStoreCall(t *testing.T) { + ctx := context.Background() + h, store := newApproveRegistrationHandler(ctx, nil) + + result, err := h.approveRegistration(ctx, adminRequest(approveRegBody), approveRegID) + require.NoError(t, err) + account, ok := result.(*config.CloudAccount) + require.True(t, ok, "got %T", result) + + store.AssertCalled(t, "ApproveAccountRegistration", ctx, + mock.MatchedBy(func(r *config.AccountRegistration) bool { + return r.ID == approveRegID && r.ReviewedAt != nil && r.ReviewedBy != nil && + *r.ReviewedBy == adminAccountSession().UserID + }), + mock.MatchedBy(func(a *config.CloudAccount) bool { + return a.ID == account.ID && a.Provider == "azure" && + a.ExternalID == "fixture-subscription-1102" && a.Enabled + }), + mock.MatchedBy(func(actor *string) bool { return actor != nil && *actor == adminAccountSession().UserID }), + ) + assertNoPiecemealApproval(t, store) +} + +func TestApproveRegistration_StoreErrorsMapToResponses(t *testing.T) { + cases := []struct { + name string + storeErr error + code int // 0: not a client error (500) + message string + }{ + {"already decided", config.ErrRegistrationConflict, 409, "already processed"}, + {"account already registered", fmt.Errorf("failed to create cloud account: %w", + errors.New(`ERROR: duplicate key value violates unique constraint "cloud_accounts_provider_external_id_key" (SQLSTATE 23505)`)), + 409, `external ID "fixture-subscription-1102" already exists for azure`}, + {"database failure", errors.New("connection reset"), 0, "registrations: approve: connection reset"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + h, store := newApproveRegistrationHandler(ctx, tc.storeErr) + + result, err := h.approveRegistration(ctx, adminRequest(approveRegBody), approveRegID) + assert.Nil(t, result) + require.Error(t, err) + ce, ok := IsClientError(err) + if tc.code == 0 { + require.False(t, ok, "a database failure must stay a 500, got client error %v", err) + assert.ErrorContains(t, err, tc.message) + } else { + require.True(t, ok, "want a client error, got %v", err) + assert.Equal(t, tc.code, ce.code) + assert.Contains(t, ce.message, tc.message) + } + assertNoPiecemealApproval(t, store) + }) + } +} diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 6483836dc..0a274195b 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -581,6 +581,10 @@ type StoreInterface interface { // TransitionRegistrationStatus atomically updates a registration's workflow fields. // actor is the UUID of the reviewer (nil for system-initiated transitions). TransitionRegistrationStatus(ctx context.Context, reg *AccountRegistration, fromStatus string, actor *string) error + // ApproveAccountRegistration atomically approves a pending registration, + // creates its cloud account and links them. Returns ErrRegistrationConflict + // when the registration is no longer pending; on any error nothing is saved. + ApproveAccountRegistration(ctx context.Context, reg *AccountRegistration, account *CloudAccount, actor *string) error DeleteAccountRegistration(ctx context.Context, id string) error // Purchase suppressions. Written inside a WithTx block during bulk diff --git a/internal/config/store_postgres_registrations.go b/internal/config/store_postgres_registrations.go index 0cfc22505..f5e4b8b69 100644 --- a/internal/config/store_postgres_registrations.go +++ b/internal/config/store_postgres_registrations.go @@ -9,6 +9,7 @@ import ( "time" "github.com/google/uuid" + "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgconn" ) @@ -245,6 +246,34 @@ func (s *PostgresStore) TransitionRegistrationStatus(ctx context.Context, reg *A return nil } +// ApproveAccountRegistration moves a pending registration to approved, creates +// its cloud account and links the two, all in one transaction. If any step +// fails nothing is written, so a registration is never left approved with no +// account (issue #1102). +// +// The status change runs first and is conditional on status = 'pending'. Its +// row lock makes a concurrent approve or reject of the same registration wait +// for this transaction: after a commit the other request matches no row and +// gets ErrRegistrationConflict; after a rollback it proceeds as if this one +// never ran. A failed account insert (for example the provider + external_id +// already being registered) returns the driver error and leaves the +// registration pending. +func (s *PostgresStore) ApproveAccountRegistration(ctx context.Context, reg *AccountRegistration, account *CloudAccount, actor *string) error { + return s.WithTx(ctx, func(tx pgx.Tx) error { + txStore := &PostgresStore{db: tx} + reg.Status = "approved" + reg.CloudAccountID = nil + if err := txStore.TransitionRegistrationStatus(ctx, reg, "pending", actor); err != nil { + return err + } + if err := txStore.CreateCloudAccount(ctx, account); err != nil { + return err + } + reg.CloudAccountID = &account.ID + return txStore.UpdateAccountRegistration(ctx, reg) + }) +} + // DeleteAccountRegistration removes a registration record by ID. func (s *PostgresStore) DeleteAccountRegistration(ctx context.Context, id string) error { _, err := s.db.Exec(ctx, `DELETE FROM account_registrations WHERE id = $1`, id) diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index a4c9546f7..083c6a8c5 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -1704,6 +1704,12 @@ func (m *MockConfigStore) TransitionRegistrationStatus(ctx context.Context, reg return args.Error(0) } +func (m *MockConfigStore) ApproveAccountRegistration(ctx context.Context, reg *config.AccountRegistration, account *config.CloudAccount, actor *string) error { + m.record("ApproveAccountRegistration", ctx, reg, account, actor) + args := m.Called(ctx, reg, account, actor) + return args.Error(0) +} + func (m *MockConfigStore) DeleteAccountRegistration(ctx context.Context, id string) error { m.record("DeleteAccountRegistration", ctx, id) args := m.Called(ctx, id) diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index bf5d1ee90..e26716153 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -320,6 +320,9 @@ func (m *mockConfigStoreForHealth) UpdateAccountRegistration(_ context.Context, func (m *mockConfigStoreForHealth) TransitionRegistrationStatus(_ context.Context, _ *config.AccountRegistration, _ string, _ *string) error { return nil } +func (m *mockConfigStoreForHealth) ApproveAccountRegistration(_ context.Context, _ *config.AccountRegistration, _ *config.CloudAccount, _ *string) error { + return nil +} func (m *mockConfigStoreForHealth) DeleteAccountRegistration(_ context.Context, _ string) error { return nil } From ffafbd0a1e2757bea49e5ea1432b6694a2f2289d Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Wed, 21 Oct 2026 08:41:35 +0100 Subject: [PATCH 5/5] test(config): pin that only SQLSTATE 22P02 makes GetExecutionByID not found Review of #1141 found that mapping every Postgres error to ErrNotFound passed all tests, so a real DB failure could have become a 404 on the revoke route. Add pgxmock unit tests on the real PostgresStore: 22P02 is ErrNotFound, while 57P01, 53300, 42P01 and 22P03 are returned as they are, with the *pgconn.PgError still reachable. Co-Authored-By: Claude Opus 5.5 --- .../store_postgres_execution_non_uuid_test.go | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 internal/config/store_postgres_execution_non_uuid_test.go diff --git a/internal/config/store_postgres_execution_non_uuid_test.go b/internal/config/store_postgres_execution_non_uuid_test.go new file mode 100644 index 000000000..a8a84cccd --- /dev/null +++ b/internal/config/store_postgres_execution_non_uuid_test.go @@ -0,0 +1,56 @@ +package config + +// Unit tests pinning how narrowly GetExecutionByID turns a driver error into +// ErrNotFound (issue #1102 item 5). Only SQLSTATE 22P02 (a value the uuid +// column cannot hold) means "no such execution"; any other Postgres error is a +// real failure and must not reach callers as a 404. The real-Postgres side of +// 22P02 is covered by TestGetExecutionByID_NonUUIDIsNotFoundPostgres. + +import ( + "context" + "testing" + + "github.com/jackc/pgx/v5/pgconn" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func getExecutionByIDWithDriverError(t *testing.T, dbErr error) error { + t.Helper() + mock := newMock(t) + defer mock.Close() + mock.ExpectQuery("FROM purchase_executions"). + WithArgs("exec-1"). + WillReturnError(dbErr) + + exec, err := storeWith(mock).GetExecutionByID(context.Background(), "exec-1") + require.NoError(t, mock.ExpectationsWereMet()) + require.Error(t, err) + assert.Nil(t, exec) + return err +} + +func TestGetExecutionByID_InvalidTextRepresentationIsNotFound(t *testing.T) { + err := getExecutionByIDWithDriverError(t, &pgconn.PgError{Code: "22P02", + Message: `invalid input syntax for type uuid: "exec-1"`}) + assert.ErrorIs(t, err, ErrNotFound) +} + +func TestGetExecutionByID_OtherPostgresErrorsAreNotNotFound(t *testing.T) { + for _, code := range []string{ + "57P01", // admin_shutdown + "53300", // too_many_connections + "42P01", // undefined_table + "22P03", // invalid_binary_representation, the code next to 22P02 + } { + t.Run(code, func(t *testing.T) { + pgErr := &pgconn.PgError{Code: code, Message: "boom"} + err := getExecutionByIDWithDriverError(t, pgErr) + + assert.NotErrorIs(t, err, ErrNotFound) + var got *pgconn.PgError + require.ErrorAs(t, err, &got, "driver error must stay reachable for callers") + assert.Same(t, pgErr, got) + }) + } +}