From 894de652d9d52d2ddaa7f24d2682f376dc6ba4f9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 15:12:58 +0100 Subject: [PATCH 1/4] refactor(api): match overlay body keys case-insensitively presentKeys now reuses bodyHasKey, so a key spelled "Term" counts as sent, the same way encoding/json binds it. Prepares the shared overlay for the account service override PUT. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_config.go | 4 ++-- internal/api/handler_present_keys_test.go | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 internal/api/handler_present_keys_test.go diff --git a/internal/api/handler_config.go b/internal/api/handler_config.go index 19755f11c..52bc6a151 100644 --- a/internal/api/handler_config.go +++ b/internal/api/handler_config.go @@ -364,7 +364,7 @@ func mergeServiceConfig(existing *config.ServiceConfig, cfg config.ServiceConfig } // presentKeys returns the subset of keys that appear as top-level fields in the -// JSON body. A malformed body is reported as a client error so the caller +// JSON body, matched case-insensitively like encoding/json binds them. A malformed body is reported as a client error so the caller // returns 400 rather than silently overlaying nothing. func presentKeys(body string, keys []string) (map[string]bool, error) { var raw map[string]json.RawMessage @@ -373,7 +373,7 @@ func presentKeys(body string, keys []string) (map[string]bool, error) { } present := make(map[string]bool, len(keys)) for _, k := range keys { - if _, ok := raw[k]; ok { + if bodyHasKey(raw, k) { present[k] = true } } diff --git a/internal/api/handler_present_keys_test.go b/internal/api/handler_present_keys_test.go new file mode 100644 index 000000000..1c1ed0ff0 --- /dev/null +++ b/internal/api/handler_present_keys_test.go @@ -0,0 +1,16 @@ +package api + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// encoding/json binds "Term" to the term field, so presentKeys must see it as +// present too, otherwise the overlay silently drops a value that was decoded. +func TestPresentKeys_CaseInsensitive(t *testing.T) { + got, err := presentKeys(`{"Term":3,"PAYMENT":null,"coverage":80}`, []string{"term", "payment", "coverage", "enabled"}) + require.NoError(t, err) + assert.Equal(t, map[string]bool{"term": true, "payment": true, "coverage": true}, got) +} From 8068716d50b26c3a94a635b416755e0f19e1c4dc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 16:39:02 +0100 Subject: [PATCH 2/4] fix(api): merge service override PUT into the stored row The Settings UI sends one field per PUT, but the handler built a fresh row and the store upsert overwrote every column, so editing one field cleared the rest. Overlay only the keys present in the body onto the existing row (null clears, absent keeps), check the term/payment combo on the merged result, and describe the semantics in the OpenAPI spec. Closes #1775 Co-Authored-By: Claude Opus 5.5 --- internal/api/coverage_extras_test.go | 15 +- internal/api/handler_accounts.go | 74 +++---- internal/api/handler_override_merge_test.go | 205 ++++++++++++++++++++ internal/api/openapi.yaml | 13 +- internal/api/override_transfer.go | 5 +- 5 files changed, 267 insertions(+), 45 deletions(-) create mode 100644 internal/api/handler_override_merge_test.go diff --git a/internal/api/coverage_extras_test.go b/internal/api/coverage_extras_test.go index 29fd978dc..98243869c 100644 --- a/internal/api/coverage_extras_test.go +++ b/internal/api/coverage_extras_test.go @@ -221,10 +221,10 @@ func TestHandler_forgotPassword_RateLimitExceeded(t *testing.T) { } // --------------------------------------------------------------------------- -// applyOverrideSlices — exercise all branches +// overlayOverrideSlices — exercise all branches // --------------------------------------------------------------------------- -func TestApplyOverrideSlices(t *testing.T) { +func TestOverlayOverrideSlices(t *testing.T) { override := &config.AccountServiceOverride{} req := AccountServiceOverrideRequest{ IncludeEngines: []string{"mysql"}, @@ -235,7 +235,10 @@ func TestApplyOverrideSlices(t *testing.T) { ExcludeTypes: []string{"db.r5.large"}, } - applyOverrideSlices(override, req) + overlayOverrideSlices(override, req, map[string]bool{ + "include_engines": true, "exclude_engines": true, "include_regions": true, + "exclude_regions": true, "include_types": true, "exclude_types": true, + }) assert.Equal(t, []string{"mysql"}, override.IncludeEngines) assert.Equal(t, []string{"postgres"}, override.ExcludeEngines) @@ -245,12 +248,12 @@ func TestApplyOverrideSlices(t *testing.T) { assert.Equal(t, []string{"db.r5.large"}, override.ExcludeTypes) } -func TestApplyOverrideSlices_NilFields(t *testing.T) { +func TestOverlayOverrideSlices_AbsentKeysKept(t *testing.T) { override := &config.AccountServiceOverride{ IncludeEngines: []string{"existing"}, } - // Nil fields should not overwrite existing values - applyOverrideSlices(override, AccountServiceOverrideRequest{}) + // Keys absent from the body must not overwrite existing values + overlayOverrideSlices(override, AccountServiceOverrideRequest{}, map[string]bool{}) assert.Equal(t, []string{"existing"}, override.IncludeEngines) } diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index b3dbddb06..e479940bd 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -79,6 +79,16 @@ type AccountServiceOverrideRequest struct { ExcludeTypes []string `json:"exclude_types,omitempty"` } +// serviceOverrideOverlayKeys are the JSON keys of AccountServiceOverrideRequest. +// A key present in the PUT body replaces the stored value (null clears it); an +// absent key keeps it. +var serviceOverrideOverlayKeys = []string{ + "enabled", "term", "payment", "coverage", "ramp_schedule", + "include_engines", "exclude_engines", + "include_regions", "exclude_regions", + "include_types", "exclude_types", +} + // validCredentialTypes is the set of accepted credential type names. var validCredentialTypes = map[string]bool{ "aws_access_keys": true, @@ -1182,15 +1192,20 @@ func (h *Handler) saveAccountServiceOverride(ctx context.Context, httpReq *event return nil, NewClientError(400, "invalid request body") } + present, err := presentKeys(httpReq.Body, serviceOverrideOverlayKeys) + if err != nil { + return nil, err + } + now := time.Now() existing, err := h.config.GetAccountServiceOverride(ctx, accountID, provider, service) if err != nil { return nil, fmt.Errorf("accounts: get existing override: %w", err) } - override := buildServiceOverride(accountID, provider, service, req, existing, now) + override := buildServiceOverride(accountID, provider, service, req, existing, present, now) - // Defense-in-depth: reject invalid (term, payment) combos before persisting. + // Defense-in-depth: reject invalid (term, payment) combos on the merged row before persisting. // checkCommitmentOptionCombo is permissive when commitmentOpts is nil or // probe data is absent (ErrNoData) — the frontend's hardcoded rules are the // primary gate in those cases. @@ -1214,69 +1229,62 @@ func (h *Handler) saveAccountServiceOverride(ctx context.Context, httpReq *event return override, nil } -// buildServiceOverride constructs an AccountServiceOverride from request and existing data. -func buildServiceOverride(accountID, provider, service string, req AccountServiceOverrideRequest, existing *config.AccountServiceOverride, now time.Time) *config.AccountServiceOverride { - override := &config.AccountServiceOverride{ - AccountID: accountID, - Provider: provider, - Service: service, - UpdatedAt: now, - } - +// buildServiceOverride constructs an AccountServiceOverride by overlaying the +// request onto the stored row (or an empty one). present names the request +// keys the body carried: only those replace the stored value, so a one-field +// PUT keeps the rest (#1775). A present key with a null value clears it. +func buildServiceOverride(accountID, provider, service string, req AccountServiceOverrideRequest, existing *config.AccountServiceOverride, present map[string]bool, now time.Time) *config.AccountServiceOverride { + override := &config.AccountServiceOverride{} if existing != nil { - override.ID = existing.ID - override.CreatedAt = existing.CreatedAt + *override = *existing } else { override.ID = uuid.New().String() override.CreatedAt = now } + override.AccountID, override.Provider, override.Service = accountID, provider, service + override.UpdatedAt = now - applyServiceOverrideFields(override, req) + overlayOverrideScalars(override, req, present) + overlayOverrideSlices(override, req, present) return override } -// applyServiceOverrideFields copies sparse request fields onto an override. -func applyServiceOverrideFields(o *config.AccountServiceOverride, req AccountServiceOverrideRequest) { - applyOverrideScalars(o, req) - applyOverrideSlices(o, req) -} - -func applyOverrideScalars(o *config.AccountServiceOverride, req AccountServiceOverrideRequest) { - if req.Enabled != nil { +func overlayOverrideScalars(o *config.AccountServiceOverride, req AccountServiceOverrideRequest, present map[string]bool) { + if present["enabled"] { o.Enabled = req.Enabled } - if req.Term != nil { + if present["term"] { o.Term = req.Term } - if req.Payment != nil { + if present["payment"] { o.Payment = req.Payment } - if req.Coverage != nil { + if present["coverage"] { o.Coverage = req.Coverage } - if req.RampSchedule != nil { + if present["ramp_schedule"] { o.RampSchedule = req.RampSchedule } } -func applyOverrideSlices(o *config.AccountServiceOverride, req AccountServiceOverrideRequest) { - if req.IncludeEngines != nil { +func overlayOverrideSlices(o *config.AccountServiceOverride, req AccountServiceOverrideRequest, present map[string]bool) { + if present["include_engines"] { o.IncludeEngines = req.IncludeEngines } - if req.ExcludeEngines != nil { + if present["exclude_engines"] { o.ExcludeEngines = req.ExcludeEngines } - if req.IncludeRegions != nil { + if present["include_regions"] { o.IncludeRegions = req.IncludeRegions } - if req.ExcludeRegions != nil { + if present["exclude_regions"] { o.ExcludeRegions = req.ExcludeRegions } - if req.IncludeTypes != nil { + if present["include_types"] { o.IncludeTypes = req.IncludeTypes } - if req.ExcludeTypes != nil { + if present["exclude_types"] { o.ExcludeTypes = req.ExcludeTypes } } diff --git a/internal/api/handler_override_merge_test.go b/internal/api/handler_override_merge_test.go new file mode 100644 index 000000000..1c575f020 --- /dev/null +++ b/internal/api/handler_override_merge_test.go @@ -0,0 +1,205 @@ +package api + +import ( + "context" + "encoding/json" + "reflect" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +const overrideMergeAccount = "11111111-1111-1111-1111-111111111111" + +func ovPtr[T any](v T) *T { return &v } + +// fullStoredOverride is a stored override with every overlay field set. +func fullStoredOverride() *config.AccountServiceOverride { + return &config.AccountServiceOverride{ + ID: "ov-1", AccountID: overrideMergeAccount, Provider: "aws", Service: "rds", + Enabled: ovPtr(true), Term: ovPtr(1), Payment: ovPtr("all-upfront"), + Coverage: ovPtr(80.0), RampSchedule: ovPtr("weekly-25pct"), + IncludeEngines: []string{"postgres"}, ExcludeEngines: []string{"mysql"}, + IncludeRegions: []string{"us-east-1"}, ExcludeRegions: []string{"eu-west-1"}, + IncludeTypes: []string{"db.m5.large"}, ExcludeTypes: []string{"db.t3.micro"}, + CreatedAt: time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC), + } +} + +// putOverride runs the PUT handler against a store holding existing and +// returns the row handed to SaveAccountServiceOverrideAuditedTx (nil if none). +func putOverride(t *testing.T, existing *config.AccountServiceOverride, body string, opts CommitmentOptsInterface) (*config.AccountServiceOverride, error) { + t.Helper() + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + setupAdminAuth(ctx, mockAuth) + store := new(MockConfigStore) + store.On("GetAccountServiceOverride", ctx, overrideMergeAccount, "aws", "rds").Return(existing, nil) + var saved *config.AccountServiceOverride + store.On("SaveAccountServiceOverrideAuditedTx", ctx, mock.Anything, mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { saved = args.Get(2).(*config.AccountServiceOverride) }).Return(nil) + h := &Handler{auth: mockAuth, config: store} + if opts != nil { + h.commitmentOpts = opts + } + _, err := h.saveAccountServiceOverride(ctx, adminRequest(body), overrideMergeAccount+"/service-overrides/aws/rds") + return saved, err +} + +// overrideMergeCases maps each overlay key to a body that changes it and the +// edit that produces the expected row from the stored one. +var overrideMergeCases = map[string]struct { + body string + apply func(*config.AccountServiceOverride) +}{ + "enabled": {`{"enabled":false}`, func(o *config.AccountServiceOverride) { o.Enabled = ovPtr(false) }}, + "term": {`{"term":3}`, func(o *config.AccountServiceOverride) { o.Term = ovPtr(3) }}, + "payment": {`{"payment":"no-upfront"}`, func(o *config.AccountServiceOverride) { o.Payment = ovPtr("no-upfront") }}, + "coverage": {`{"coverage":0}`, func(o *config.AccountServiceOverride) { o.Coverage = ovPtr(0.0) }}, + "ramp_schedule": {`{"ramp_schedule":"immediate"}`, func(o *config.AccountServiceOverride) { o.RampSchedule = ovPtr("immediate") }}, + "include_engines": {`{"include_engines":["aurora"]}`, func(o *config.AccountServiceOverride) { o.IncludeEngines = []string{"aurora"} }}, + "exclude_engines": {`{"exclude_engines":["oracle"]}`, func(o *config.AccountServiceOverride) { o.ExcludeEngines = []string{"oracle"} }}, + "include_regions": {`{"include_regions":["us-west-2"]}`, func(o *config.AccountServiceOverride) { o.IncludeRegions = []string{"us-west-2"} }}, + "exclude_regions": {`{"exclude_regions":["ap-south-1"]}`, func(o *config.AccountServiceOverride) { o.ExcludeRegions = []string{"ap-south-1"} }}, + "include_types": {`{"include_types":["db.r5.large"]}`, func(o *config.AccountServiceOverride) { o.IncludeTypes = []string{"db.r5.large"} }}, + "exclude_types": {`{"exclude_types":["db.t3.small"]}`, func(o *config.AccountServiceOverride) { o.ExcludeTypes = []string{"db.t3.small"} }}, +} + +// A single-key PUT (what the Settings UI sends) changes that key and keeps +// every other stored field (#1775). +func TestSaveAccountServiceOverride_SingleKeyKeepsOtherFields(t *testing.T) { + for key, tc := range overrideMergeCases { + t.Run(key, func(t *testing.T) { + want := fullStoredOverride() + tc.apply(want) + got, err := putOverride(t, fullStoredOverride(), tc.body, nil) + require.NoError(t, err) + require.NotNil(t, got) + assert.WithinDuration(t, time.Now(), got.UpdatedAt, time.Minute) + got.UpdatedAt = time.Time{} + assert.Equal(t, want, got) + }) + } +} + +// An explicit JSON null clears only that key back to inherit. +func TestSaveAccountServiceOverride_ExplicitNullClearsOnlyThatKey(t *testing.T) { + for key := range overrideMergeCases { + t.Run(key, func(t *testing.T) { + want := fullStoredOverride() + clearOverrideKey(want, key) + got, err := putOverride(t, fullStoredOverride(), `{"`+key+`":null}`, nil) + require.NoError(t, err) + got.UpdatedAt = time.Time{} + assert.Equal(t, want, got) + }) + } +} + +func clearOverrideKey(o *config.AccountServiceOverride, key string) { + switch key { + case "enabled": + o.Enabled = nil + case "term": + o.Term = nil + case "payment": + o.Payment = nil + case "coverage": + o.Coverage = nil + case "ramp_schedule": + o.RampSchedule = nil + case "include_engines": + o.IncludeEngines = nil + case "exclude_engines": + o.ExcludeEngines = nil + case "include_regions": + o.IncludeRegions = nil + case "exclude_regions": + o.ExcludeRegions = nil + case "include_types": + o.IncludeTypes = nil + case "exclude_types": + o.ExcludeTypes = nil + } +} + +// Key matching follows encoding/json: a differently-cased key still counts as sent. +func TestSaveAccountServiceOverride_KeyMatchIsCaseInsensitive(t *testing.T) { + want := fullStoredOverride() + want.Term = ovPtr(3) + got, err := putOverride(t, fullStoredOverride(), `{"Term":3}`, nil) + require.NoError(t, err) + got.UpdatedAt = time.Time{} + assert.Equal(t, want, got) +} + +// With no stored row the result carries only the sent fields. +func TestSaveAccountServiceOverride_NoExistingRowKeepsOnlySentFields(t *testing.T) { + got, err := putOverride(t, nil, `{"term":1,"include_regions":["us-east-1"]}`, nil) + require.NoError(t, err) + assert.NotEmpty(t, got.ID) + assert.Equal(t, ovPtr(1), got.Term) + assert.Equal(t, []string{"us-east-1"}, got.IncludeRegions) + assert.Nil(t, got.Enabled) + assert.Nil(t, got.Payment) + assert.Nil(t, got.Coverage) + assert.Nil(t, got.RampSchedule) + assert.Nil(t, got.IncludeEngines) + assert.Nil(t, got.ExcludeTypes) +} + +// The (term, payment) check sees the merged pair, not just the sent key. +func TestSaveAccountServiceOverride_ComboCheckedOnMergedRow(t *testing.T) { + existing := &config.AccountServiceOverride{ + ID: "ov-1", AccountID: overrideMergeAccount, Provider: "aws", Service: "rds", + Payment: ovPtr("no-upfront"), + } + var checkedTerm int + var checkedPayment string + opts := &stubCommitmentOpts{ + validateFn: func(_ context.Context, _, _ string, term int, payment string) (bool, error) { + checkedTerm, checkedPayment = term, payment + return term != 3 || payment != "no-upfront", nil + }, + } + got, err := putOverride(t, existing, `{"term":3}`, opts) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + assert.Equal(t, 3, checkedTerm) + assert.Equal(t, "no-upfront", checkedPayment) + assert.Nil(t, got, "a refused combo must not be saved") +} + +// Every json field of the request is merged: a new field must be added to the +// overlay allowlist (and to overrideMergeCases) or this fails. +func TestAccountServiceOverrideRequest_EveryFieldIsMerged(t *testing.T) { + var tags []string + rt := reflect.TypeOf(AccountServiceOverrideRequest{}) + for i := 0; i < rt.NumField(); i++ { + tags = append(tags, strings.Split(rt.Field(i).Tag.Get("json"), ",")[0]) + } + caseKeys := make([]string, 0, len(overrideMergeCases)) + for k := range overrideMergeCases { + caseKeys = append(caseKeys, k) + } + assert.ElementsMatch(t, tags, serviceOverrideOverlayKeys) + assert.ElementsMatch(t, tags, caseKeys) + + // The same set holds on the stored side: no request key is unknown to the row. + var row map[string]json.RawMessage + b, err := json.Marshal(fullStoredOverride()) + require.NoError(t, err) + require.NoError(t, json.Unmarshal(b, &row)) + for _, k := range tags { + assert.Contains(t, row, k) + } +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 8b7539eb8..b8bd4e72a 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -4403,9 +4403,12 @@ paths: tags: [Accounts] summary: Create or replace an account's override for one service (admin only) description: > - Upserts the override for this account, provider and service. The - override is replaced as a whole: fields omitted from the body are - cleared and inherit the global service configuration. + Upserts the override for this account, provider and service. The body + is merged into the stored override: a field omitted from the body keeps + its stored value, a field sent as null is cleared and inherits the + global service configuration, and any other value replaces it. With no + stored override, only the fields sent are set. The AWS term and payment + combination is checked on the merged result. parameters: - $ref: '#/components/parameters/CSRFToken' requestBody: @@ -8148,7 +8151,9 @@ components: AccountServiceOverrideRequest: type: object - description: Fields to override. Omitted fields inherit the global value. + description: > + Fields to override. Omitted fields keep their stored value (or inherit + the global value when nothing is stored); null clears a field. properties: enabled: type: boolean diff --git a/internal/api/override_transfer.go b/internal/api/override_transfer.go index a08881c6b..45e620a9c 100644 --- a/internal/api/override_transfer.go +++ b/internal/api/override_transfer.go @@ -15,8 +15,9 @@ import ( // UpdatedAt are informational: import ignores them. // // An import row is the desired state of the override: an unset field clears -// that field, unlike the sparse PUT /api/accounts/:id/service-overrides/... -// merge. Overrides missing from the file are left alone. +// that field, unlike the PUT /api/accounts/:id/service-overrides/... merge, +// which keeps the stored value of any key the body omits. Overrides missing +// from the file are left alone. type OverrideTransferRow struct { AccountID string `json:"account_id,omitempty"` AccountName string `json:"account_name,omitempty"` From 7480d451d5451056d05bfd5b10a4cdc085dab3c6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 18:38:18 +0100 Subject: [PATCH 3/4] fix(api): merge a service override PUT inside the locked write The PUT read the stored override on the pool, merged and checked the combo on that copy, and only then took the advisory lock and FOR UPDATE to write every column. Two single-field PUTs merged onto the same base and the later commit reverted the other change (a lost exclusion widens what can be bought). Add UpdateAccountServiceOverrideAtomic: lock, read, merge and audited write in one transaction, sharing the lock/read and write helpers with SaveAccountServiceOverrideAuditedTx. The handler prefetches the commitment options before the transaction and merges and checks the combo inside it. Refs #1775 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_accounts.go | 42 ++++--- internal/api/handler_accounts_test.go | 16 +-- ...handler_override_merge_integration_test.go | 103 ++++++++++++++++++ internal/api/handler_override_merge_test.go | 23 ++-- internal/config/interfaces.go | 6 + .../config/store_postgres_config_audit.go | 59 +++++++++- ..._postgres_config_audit_integration_test.go | 55 ++++++++++ internal/mocks/stores.go | 34 ++++++ 8 files changed, 299 insertions(+), 39 deletions(-) create mode 100644 internal/api/handler_override_merge_integration_test.go diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index e479940bd..dae8bb72d 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -1197,19 +1197,34 @@ func (h *Handler) saveAccountServiceOverride(ctx context.Context, httpReq *event return nil, err } - now := time.Now() - existing, err := h.config.GetAccountServiceOverride(ctx, accountID, provider, service) + // Fetch the commitment options before the transaction opens: the merge + // callback runs while it holds a pooled connection, so it must not take + // another (see commitmentOptsSnapshot). + combos := h.prefetchCommitmentOptions(ctx, provider) + + // The read, merge and write run in one locked transaction (issue #1775), + // so two single-field PUTs cannot merge onto the same stale row and lose + // one change. The audit event commits with the write (issue #1493). + audit := config.AuditContext{Actor: configAuditActor(session)} + override, err := h.config.UpdateAccountServiceOverrideAtomic(ctx, accountID, provider, service, audit, + func(existing *config.AccountServiceOverride) (*config.AccountServiceOverride, error) { + return mergeAndValidateServiceOverride(accountID, provider, service, req, existing, present, combos) + }) if err != nil { - return nil, fmt.Errorf("accounts: get existing override: %w", err) + return nil, fmt.Errorf("accounts: %w", err) } - override := buildServiceOverride(accountID, provider, service, req, existing, present, now) + return override, nil +} - // Defense-in-depth: reject invalid (term, payment) combos on the merged row before persisting. - // checkCommitmentOptionCombo is permissive when commitmentOpts is nil or - // probe data is absent (ErrNoData) — the frontend's hardcoded rules are the - // primary gate in those cases. - if err := h.checkCommitmentOptionCombo(ctx, config.ServiceConfig{ +// mergeAndValidateServiceOverride is the read-merge step of an override PUT, +// run inside the store's locked transaction: it merges the request onto the +// stored row, then rejects an invalid (term, payment) combo on the merged +// result. The check is permissive when no probe data exists, as for the +// service-config PUT. It does no I/O. +func mergeAndValidateServiceOverride(accountID, provider, service string, req AccountServiceOverrideRequest, existing *config.AccountServiceOverride, present map[string]bool, combos commitmentOptsSnapshot) (*config.AccountServiceOverride, error) { + override := buildServiceOverride(accountID, provider, service, req, existing, present, time.Now()) + if err := combos.check(config.ServiceConfig{ Provider: override.Provider, Service: override.Service, Term: derefInt(override.Term), @@ -1217,15 +1232,6 @@ func (h *Handler) saveAccountServiceOverride(ctx context.Context, httpReq *event }); err != nil { return nil, err } - - // The write and its config_audit_events row commit together (issue #1493). - audit := config.AuditContext{Actor: configAuditActor(session)} - if err := h.config.WithTx(ctx, func(tx pgx.Tx) error { - return h.config.SaveAccountServiceOverrideAuditedTx(ctx, tx, override, audit) - }); err != nil { - return nil, fmt.Errorf("accounts: %w", err) - } - return override, nil } diff --git a/internal/api/handler_accounts_test.go b/internal/api/handler_accounts_test.go index beeb0362e..e3a0b6db4 100644 --- a/internal/api/handler_accounts_test.go +++ b/internal/api/handler_accounts_test.go @@ -1080,12 +1080,8 @@ func TestSaveAccountServiceOverride_InvalidCombo_Returns400(t *testing.T) { auth: mockAuth, config: store, commitmentOpts: &stubCommitmentOpts{ - validateFn: func(_ context.Context, provider, service string, term int, payment string) (bool, error) { - assert.Equal(t, "aws", provider) - assert.Equal(t, "rds", service) - assert.Equal(t, 3, term) - assert.Equal(t, "no-upfront", payment) - return false, nil + getFn: func(context.Context) (commitmentopts.Options, error) { + return commitmentopts.Options{"aws": {"rds": {{Provider: "aws", Service: "rds", TermYears: 1, Payment: "all-upfront"}}}}, nil }, }, } @@ -1115,8 +1111,8 @@ func TestSaveAccountServiceOverride_ValidCombo_Saves(t *testing.T) { auth: mockAuth, config: store, commitmentOpts: &stubCommitmentOpts{ - validateFn: func(context.Context, string, string, int, string) (bool, error) { - return true, nil + getFn: func(context.Context) (commitmentopts.Options, error) { + return commitmentopts.Options{"aws": {"rds": {{Provider: "aws", Service: "rds", TermYears: 1, Payment: "all-upfront"}}}}, nil }, }, } @@ -1163,8 +1159,8 @@ func TestSaveAccountServiceOverride_NoProbeData_Permissive(t *testing.T) { auth: mockAuth, config: store, commitmentOpts: &stubCommitmentOpts{ - validateFn: func(context.Context, string, string, int, string) (bool, error) { - return false, commitmentopts.ErrNoData + getFn: func(context.Context) (commitmentopts.Options, error) { + return nil, commitmentopts.ErrNoData }, }, } diff --git a/internal/api/handler_override_merge_integration_test.go b/internal/api/handler_override_merge_integration_test.go new file mode 100644 index 000000000..7ec3f1f84 --- /dev/null +++ b/internal/api/handler_override_merge_integration_test.go @@ -0,0 +1,103 @@ +//go:build integration + +package api + +// Real-Postgres tests for the account service override PUT merge (issue +// #1775): a single-field PUT keeps every other stored field, and two +// concurrent single-field PUTs both persist. A mock cannot hold the row and +// advisory locks the race depends on. + +import ( + "context" + "fmt" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// overrideMergeRaceRounds is how many concurrent PUT pairs the race test fires. +const overrideMergeRaceRounds = 40 + +func fullOverrideFor(accountID string) *config.AccountServiceOverride { + o := fullStoredOverride() + o.ID, o.AccountID, o.CreatedAt = "", accountID, o.CreatedAt.UTC() + return o +} + +// A single-field PUT over a fully populated row, through the real store, +// changes that field only. +func TestOverrideMerge_SingleKeyPutKeepsOtherFieldsInStore(t *testing.T) { + ctx := context.Background() + f := newOverrideAuditFixture(ctx, t) + seed := fullOverrideFor(f.acct.ID) + require.NoError(t, f.store.SaveAccountServiceOverride(ctx, seed)) + + f.put(ctx, t, "rds", `{"term":3}`) + + got, err := f.store.GetAccountServiceOverride(ctx, f.acct.ID, "aws", "rds") + require.NoError(t, err) + require.NotNil(t, got) + want := fullOverrideFor(f.acct.ID) + want.Term = ovPtr(3) + assert.Equal(t, want.Enabled, got.Enabled) + assert.Equal(t, want.Term, got.Term) + assert.Equal(t, want.Payment, got.Payment) + assert.Equal(t, want.Coverage, got.Coverage) + assert.Equal(t, want.RampSchedule, got.RampSchedule) + assert.Equal(t, want.IncludeEngines, got.IncludeEngines) + assert.Equal(t, want.ExcludeEngines, got.ExcludeEngines) + assert.Equal(t, want.IncludeRegions, got.IncludeRegions) + assert.Equal(t, want.ExcludeRegions, got.ExcludeRegions) + assert.Equal(t, want.IncludeTypes, got.IncludeTypes) + assert.Equal(t, want.ExcludeTypes, got.ExcludeTypes) +} + +// The reviewer's repro for #1775 (PR #1788): reset the row to term 1, +// all-upfront and exclude_regions [eu-west-1], then PUT {"exclude_regions": +// ["r-N"]} and {"term":3} at once. Both return 200; unless the read-merge-write +// is one locked transaction, one change is lost every round (an exclusion +// silently reverted widens what can be bought). +func TestOverrideMerge_ConcurrentSingleKeyPutsKeepBothKeys(t *testing.T) { + ctx := context.Background() + f := newOverrideAuditFixture(ctx, t) + + lost := 0 + for round := 1; round <= overrideMergeRaceRounds; round++ { + require.NoError(t, f.store.SaveAccountServiceOverride(ctx, &config.AccountServiceOverride{ + AccountID: f.acct.ID, Provider: "aws", Service: "rds", + Term: ovPtr(1), Payment: ovPtr("all-upfront"), ExcludeRegions: []string{"eu-west-1"}, + })) + region := fmt.Sprintf("r-%d", round) + + bodies := []string{fmt.Sprintf(`{"exclude_regions":[%q]}`, region), `{"term":3}`} + start := make(chan struct{}) + errs := make([]error, len(bodies)) + var wg sync.WaitGroup + for i, body := range bodies { + wg.Add(1) + go func(i int, body string) { + defer wg.Done() + <-start + _, errs[i] = f.h.saveAccountServiceOverride(ctx, adminRequest(body), f.acct.ID+"/service-overrides/aws/rds") + }(i, body) + } + close(start) + wg.Wait() + for i, err := range errs { + require.NoError(t, err, "PUT %d (%s)", i, bodies[i]) + } + + got, err := f.store.GetAccountServiceOverride(ctx, f.acct.ID, "aws", "rds") + require.NoError(t, err) + if got.Term == nil || *got.Term != 3 || len(got.ExcludeRegions) != 1 || got.ExcludeRegions[0] != region { + lost++ + t.Logf("round %d: term=%v (want 3) exclude_regions=%v (want [%s])", round, got.Term, got.ExcludeRegions, region) + } + require.Equal(t, ovPtr("all-upfront"), got.Payment, "round %d: untouched field must survive", round) + } + require.Zero(t, lost, "%d of %d rounds lost one of two concurrent PUTs", lost, overrideMergeRaceRounds) +} diff --git a/internal/api/handler_override_merge_test.go b/internal/api/handler_override_merge_test.go index 1c575f020..8e32bd065 100644 --- a/internal/api/handler_override_merge_test.go +++ b/internal/api/handler_override_merge_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "github.com/LeanerCloud/cloud-commitments-platform/internal/commitmentopts" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" ) @@ -140,6 +141,16 @@ func TestSaveAccountServiceOverride_KeyMatchIsCaseInsensitive(t *testing.T) { assert.Equal(t, want, got) } +// [] sets an empty list, which is not the same as null clearing the key. +func TestSaveAccountServiceOverride_EmptyListSetsEmptyNotNil(t *testing.T) { + want := fullStoredOverride() + want.IncludeRegions = []string{} + got, err := putOverride(t, fullStoredOverride(), `{"include_regions":[]}`, nil) + require.NoError(t, err) + got.UpdatedAt = time.Time{} + assert.Equal(t, want, got) +} + // With no stored row the result carries only the sent fields. func TestSaveAccountServiceOverride_NoExistingRowKeepsOnlySentFields(t *testing.T) { got, err := putOverride(t, nil, `{"term":1,"include_regions":["us-east-1"]}`, nil) @@ -161,12 +172,11 @@ func TestSaveAccountServiceOverride_ComboCheckedOnMergedRow(t *testing.T) { ID: "ov-1", AccountID: overrideMergeAccount, Provider: "aws", Service: "rds", Payment: ovPtr("no-upfront"), } - var checkedTerm int - var checkedPayment string + // The options offer only 1yr no-upfront: the merged 3yr no-upfront is + // refused, though the sent term alone (no payment) would pass. opts := &stubCommitmentOpts{ - validateFn: func(_ context.Context, _, _ string, term int, payment string) (bool, error) { - checkedTerm, checkedPayment = term, payment - return term != 3 || payment != "no-upfront", nil + getFn: func(context.Context) (commitmentopts.Options, error) { + return commitmentopts.Options{"aws": {"rds": {{Provider: "aws", Service: "rds", TermYears: 1, Payment: "no-upfront"}}}}, nil }, } got, err := putOverride(t, existing, `{"term":3}`, opts) @@ -174,8 +184,7 @@ func TestSaveAccountServiceOverride_ComboCheckedOnMergedRow(t *testing.T) { ce, ok := IsClientError(err) require.True(t, ok) assert.Equal(t, 400, ce.code) - assert.Equal(t, 3, checkedTerm) - assert.Equal(t, "no-upfront", checkedPayment) + assert.Contains(t, ce.message, "3yr no-upfront") assert.Nil(t, got, "a refused combo must not be saved") } diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index ec2d93781..d7909ba3d 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -666,6 +666,12 @@ type StoreInterface interface { // write them in a consistent key order. A write that changes no setting // records nothing. override is updated to the stored row. SaveAccountServiceOverrideAuditedTx(ctx context.Context, tx pgx.Tx, override *AccountServiceOverride, audit AuditContext) error + // UpdateAccountServiceOverrideAtomic serializes a read-merge-write of one + // override under the same lock as SaveAccountServiceOverrideAuditedTx and + // audits the write. merge receives the stored row (nil when none) and + // returns the full row to write; its error aborts the write and is + // propagated unchanged. Returns the saved row. + UpdateAccountServiceOverrideAtomic(ctx context.Context, accountID, provider, service string, audit AuditContext, merge func(existing *AccountServiceOverride) (*AccountServiceOverride, error)) (*AccountServiceOverride, error) // DeleteAccountServiceOverrideAuditedTx deletes the override inside the // caller's transaction and records the deleted row in config_audit_events. // Deleting a missing override records nothing. diff --git a/internal/config/store_postgres_config_audit.go b/internal/config/store_postgres_config_audit.go index 792159f60..73b86124f 100644 --- a/internal/config/store_postgres_config_audit.go +++ b/internal/config/store_postgres_config_audit.go @@ -137,17 +137,68 @@ func scanAccountServiceOverride(row pgx.Row) (*AccountServiceOverride, error) { // Callers writing several overrides in one tx must call this in a consistent // key order, or two such transactions could deadlock on each other's locks. func (s *PostgresStore) SaveAccountServiceOverrideAuditedTx(ctx context.Context, tx pgx.Tx, o *AccountServiceOverride, audit AuditContext) error { - key := accountServiceOverrideLockPrefix + o.AccountID + "/" + o.Provider + "/" + o.Service + before, err := lockAndReadOverride(ctx, tx, o.AccountID, o.Provider, o.Service) + if err != nil { + return err + } + return writeOverrideAudited(ctx, tx, o, before, audit) +} + +// UpdateAccountServiceOverrideAtomic is a serialized read-merge-write of one +// override (issue #1775). In one transaction it takes the same advisory lock +// and FOR UPDATE read as SaveAccountServiceOverrideAuditedTx, hands the stored +// row (nil when none) to merge, and writes and audits the row merge returns. +// Two partial PUTs of one override therefore cannot merge onto the same stale +// base and lose a field. merge must not modify existing, and must not touch +// the database: the transaction holds a pooled connection. An error from +// merge aborts the transaction and is returned unchanged. The saved row is +// returned. +func (s *PostgresStore) UpdateAccountServiceOverrideAtomic(ctx context.Context, accountID, provider, service string, audit AuditContext, merge func(existing *AccountServiceOverride) (*AccountServiceOverride, error)) (*AccountServiceOverride, error) { + var saved *AccountServiceOverride + err := s.WithTx(ctx, func(tx pgx.Tx) error { + before, err := lockAndReadOverride(ctx, tx, accountID, provider, service) + if err != nil { + return err + } + next, err := merge(before) + if err != nil { + return err + } + if next == nil || next.AccountID != accountID || next.Provider != provider || next.Service != service { + return fmt.Errorf("service override update for %s/%s/%s must return a row with the same key", accountID, provider, service) + } + if err := writeOverrideAudited(ctx, tx, next, before, audit); err != nil { + return err + } + saved = next + return nil + }) + if err != nil { + return nil, err + } + return saved, nil +} + +// lockAndReadOverride takes the override's transaction-scoped advisory lock, +// then reads the stored row FOR UPDATE (nil when none). +func lockAndReadOverride(ctx context.Context, tx pgx.Tx, accountID, provider, service string) (*AccountServiceOverride, error) { + key := accountServiceOverrideLockPrefix + accountID + "/" + provider + "/" + service if _, err := tx.Exec(ctx, "SELECT pg_advisory_xact_lock(hashtextextended($1, 0))", key); err != nil { - return fmt.Errorf("failed to lock service override: %w", err) + return nil, fmt.Errorf("failed to lock service override: %w", err) } before, err := scanAccountServiceOverride(tx.QueryRow(ctx, `SELECT `+accountServiceOverrideColumns+` FROM account_service_overrides WHERE account_id = $1 AND provider = $2 AND service = $3 FOR UPDATE`, - o.AccountID, o.Provider, o.Service)) + accountID, provider, service)) if err != nil { - return fmt.Errorf("failed to read service override: %w", err) + return nil, fmt.Errorf("failed to read service override: %w", err) } + return before, nil +} + +// writeOverrideAudited upserts o, updates o to the stored row and records the +// change against before. +func writeOverrideAudited(ctx context.Context, tx pgx.Tx, o, before *AccountServiceOverride, audit AuditContext) error { after, err := upsertAccountServiceOverrideReturning(ctx, tx, o) if err != nil { return err diff --git a/internal/config/store_postgres_config_audit_integration_test.go b/internal/config/store_postgres_config_audit_integration_test.go index 1d6b747f4..1503cb57a 100644 --- a/internal/config/store_postgres_config_audit_integration_test.go +++ b/internal/config/store_postgres_config_audit_integration_test.go @@ -133,3 +133,58 @@ func TestOverrideAudit_AfterIsTheStoredRow(t *testing.T) { assert.Equal(t, 33.33, after["coverage"]) assert.Equal(t, 33.33, *o.Coverage, "the caller's override is updated to the stored row") } + +// TestOverrideAtomic_MergeErrorAbortsUnchanged: an error from merge rolls the +// transaction back (no row, no event) and reaches the caller as the same +// value, so a ClientError such as the combo 400 is not rewrapped. +func TestOverrideAtomic_MergeErrorAbortsUnchanged(t *testing.T) { + ctx := context.Background() + f := newOverrideAuditStoreFixture(ctx, t) + sentinel := errors.New("combo refused") + + got, err := f.store.UpdateAccountServiceOverrideAtomic(ctx, f.account, "aws", "rds", AuditContext{}, + func(existing *AccountServiceOverride) (*AccountServiceOverride, error) { + assert.Nil(t, existing) + return nil, sentinel + }) + require.Nil(t, got) + require.Same(t, sentinel, err, "merge error must be returned unchanged") + stored, gerr := f.store.GetAccountServiceOverride(ctx, f.account, "aws", "rds") + require.NoError(t, gerr) + assert.Nil(t, stored) +} + +// TestOverrideAtomic_QueuesBehindCompetingWriter: the merge sees the row a +// concurrent writer committed, not the one that existed when the call began. +func TestOverrideAtomic_QueuesBehindCompetingWriter(t *testing.T) { + ctx := context.Background() + f := newOverrideAuditStoreFixture(ctx, t) + require.NoError(t, saveAccountServiceOverride(ctx, f.store.db, &AccountServiceOverride{AccountID: f.account, Provider: "aws", Service: "rds", Term: intPtrAudit(1)})) + + writer := f.hold(ctx, t) + _, err := writer.Exec(ctx, "SELECT pg_advisory_xact_lock(hashtextextended($1, 0))", + accountServiceOverrideLockPrefix+f.account+"/aws/rds") + require.NoError(t, err) + _, err = writer.Exec(ctx, `UPDATE account_service_overrides SET term = 3 WHERE account_id = $1`, f.account) + require.NoError(t, err) + + var seen *int + done := make(chan error, 1) + go func() { + _, e := f.store.UpdateAccountServiceOverrideAtomic(ctx, f.account, "aws", "rds", AuditContext{Actor: AuditActor{Identity: "admin@example.com"}}, + func(existing *AccountServiceOverride) (*AccountServiceOverride, error) { + seen = existing.Term + next := *existing + next.Coverage = floatPtrAudit(50) + return &next, nil + }) + done <- e + }() + testhelpers.WaitForLockWait(ctx, t, f.store.db, "") + require.NoError(t, writer.Commit(ctx)) + require.NoError(t, testhelpers.Await(t, done)) + require.NotNil(t, seen) + assert.Equal(t, 3, *seen, "merge must read the committed row") +} + +func floatPtrAudit(v float64) *float64 { return &v } diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 5937f85e8..331f10bcb 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -1772,6 +1772,40 @@ func (m *MockConfigStore) SaveAccountServiceOverrideAuditedTx(ctx context.Contex return args.Error(0) } +// UpdateAccountServiceOverrideAtomic mocks the locked read-merge-write of one +// override. With an expectation it dispatches through m.Called; otherwise it +// emulates the store via the mocked WithTx, GetAccountServiceOverride, merge +// and SaveAccountServiceOverrideAuditedTx, so tests that seed those keep working. +func (m *MockConfigStore) UpdateAccountServiceOverrideAtomic(ctx context.Context, accountID, provider, service string, audit config.AuditContext, merge func(*config.AccountServiceOverride) (*config.AccountServiceOverride, error)) (*config.AccountServiceOverride, error) { + m.record("UpdateAccountServiceOverrideAtomic", ctx, accountID, provider, service, audit, merge) + if isExpected(&m.Mock, "UpdateAccountServiceOverrideAtomic") { + args := m.Called(ctx, accountID, provider, service, audit, merge) + if args.Get(0) == nil { + return nil, args.Error(1) + } + v, ok := args.Get(0).(*config.AccountServiceOverride) + if !ok { + m.reporter.wrongReturnType("UpdateAccountServiceOverrideAtomic", "*config.AccountServiceOverride", args.Get(0)) + } + return v, args.Error(1) + } + var next *config.AccountServiceOverride + err := m.WithTx(ctx, func(tx pgx.Tx) error { + existing, err := m.GetAccountServiceOverride(ctx, accountID, provider, service) + if err != nil { + return err + } + if next, err = merge(existing); err != nil { + return err + } + return m.SaveAccountServiceOverrideAuditedTx(ctx, tx, next, audit) + }) + if err != nil { + return nil, err + } + return next, nil +} + // DeleteAccountServiceOverrideAuditedTx falls back to // DeleteAccountServiceOverride when no expectation is registered. func (m *MockConfigStore) DeleteAccountServiceOverrideAuditedTx(ctx context.Context, tx pgx.Tx, accountID, provider, service string, audit config.AuditContext) error { From bbc9d188fa9f2edad8d513635243137f6f9c8e50 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 20:17:44 +0100 Subject: [PATCH 4/4] fix(api): lock a service override on the canonical account id The PUT used the path spelling of the account id as the lock and merge key, so an upper-case and a lower-case first PUT could race and drop keys. It now uses the stored account id. The spec summary and nullable fields are corrected. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_accounts.go | 5 ++- ...handler_override_merge_integration_test.go | 43 +++++++++++++++++++ internal/api/handler_override_merge_test.go | 23 ++++++++++ internal/api/openapi.yaml | 13 +++++- 4 files changed, 82 insertions(+), 2 deletions(-) diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index dae8bb72d..a9fff55cd 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -1181,10 +1181,13 @@ func (h *Handler) saveAccountServiceOverride(ctx context.Context, httpReq *event return nil, err } - _, err = h.requireAccountAccess(ctx, session, accountID) + account, err := h.requireAccountAccess(ctx, session, accountID) if err != nil { return nil, err } + // The store's lock key is built from the id, so every spelling of one + // account must reach it as the stored (canonical) one. + accountID = account.ID var req AccountServiceOverrideRequest err = json.Unmarshal([]byte(httpReq.Body), &req) diff --git a/internal/api/handler_override_merge_integration_test.go b/internal/api/handler_override_merge_integration_test.go index 7ec3f1f84..ac95fedf0 100644 --- a/internal/api/handler_override_merge_integration_test.go +++ b/internal/api/handler_override_merge_integration_test.go @@ -10,6 +10,7 @@ package api import ( "context" "fmt" + "strings" "sync" "testing" @@ -101,3 +102,45 @@ func TestOverrideMerge_ConcurrentSingleKeyPutsKeepBothKeys(t *testing.T) { } require.Zero(t, lost, "%d of %d rounds lost one of two concurrent PUTs", lost, overrideMergeRaceRounds) } + +// Two spellings of one account id must take the same lock: with the raw path id +// in the lock key, a first-time override raced by a lower- and an upper-case PUT +// merged both onto an empty row and the second write NULLed the first's field. +func TestOverrideMerge_ConcurrentFirstPutsAcrossIDSpellingsKeepBothKeys(t *testing.T) { + ctx := context.Background() + f := newOverrideAuditFixture(ctx, t) + + lost := 0 + for round := 1; round <= overrideMergeRaceRounds; round++ { + _, err := f.pg.DB.Pool().Exec(ctx, `DELETE FROM account_service_overrides WHERE account_id = $1`, f.acct.ID) + require.NoError(t, err) + region := fmt.Sprintf("r-%d", round) + paths := []string{f.acct.ID, strings.ToUpper(f.acct.ID)} + bodies := []string{fmt.Sprintf(`{"exclude_regions":[%q]}`, region), `{"term":3}`} + + start := make(chan struct{}) + errs := make([]error, len(bodies)) + var wg sync.WaitGroup + for i := range bodies { + wg.Add(1) + go func(i int) { + defer wg.Done() + <-start + _, errs[i] = f.h.saveAccountServiceOverride(ctx, adminRequest(bodies[i]), paths[i]+"/service-overrides/aws/rds") + }(i) + } + close(start) + wg.Wait() + for i, e := range errs { + require.NoError(t, e, "PUT %d (%s)", i, bodies[i]) + } + + got, err := f.store.GetAccountServiceOverride(ctx, f.acct.ID, "aws", "rds") + require.NoError(t, err) + if got.Term == nil || *got.Term != 3 || len(got.ExcludeRegions) != 1 || got.ExcludeRegions[0] != region { + lost++ + t.Logf("round %d: term=%v exclude_regions=%v (want 3, [%s])", round, got.Term, got.ExcludeRegions, region) + } + } + require.Zero(t, lost, "%d of %d rounds lost one of two concurrent first PUTs", lost, overrideMergeRaceRounds) +} diff --git a/internal/api/handler_override_merge_test.go b/internal/api/handler_override_merge_test.go index 8e32bd065..2404e99c4 100644 --- a/internal/api/handler_override_merge_test.go +++ b/internal/api/handler_override_merge_test.go @@ -212,3 +212,26 @@ func TestAccountServiceOverrideRequest_EveryFieldIsMerged(t *testing.T) { assert.Contains(t, row, k) } } + +// The store locks on the id it is given, so a mixed-case path id must reach it +// as the stored account's canonical id. +func TestSaveAccountServiceOverride_UsesCanonicalAccountID(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + setupAdminAuth(ctx, mockAuth) + store := new(MockConfigStore) + const canonical = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + store.GetCloudAccountFn = func(_ context.Context, _ string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: canonical, Provider: "aws"}, nil + } + var gotID string + store.On("UpdateAccountServiceOverrideAtomic", ctx, mock.Anything, "aws", "rds", mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { gotID = args.String(1) }). + Return(&config.AccountServiceOverride{}, nil) + h := &Handler{auth: mockAuth, config: store} + + _, err := h.saveAccountServiceOverride(ctx, adminRequest(`{"term":1}`), strings.ToUpper(canonical)+"/service-overrides/aws/rds") + require.NoError(t, err) + assert.Equal(t, canonical, gotID) +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 897d3249d..f581b162f 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -4498,7 +4498,7 @@ paths: put: operationId: saveAccountServiceOverride tags: [Accounts] - summary: Create or replace an account's override for one service (admin only) + summary: Create or update an account's override for one service (admin only) description: > Upserts the override for this account, provider and service. The body is merged into the stored override: a field omitted from the body keeps @@ -8258,37 +8258,48 @@ components: the global value when nothing is stored); null clears a field. properties: enabled: + nullable: true type: boolean term: + nullable: true type: integer payment: + nullable: true type: string coverage: + nullable: true type: number format: double ramp_schedule: + nullable: true type: string include_engines: + nullable: true type: array items: type: string exclude_engines: + nullable: true type: array items: type: string include_regions: + nullable: true type: array items: type: string exclude_regions: + nullable: true type: array items: type: string include_types: + nullable: true type: array items: type: string exclude_types: + nullable: true type: array items: type: string