fix(accounts): keep a service override's other fields when one field is edited

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)
 }
 

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,
@@ -1171,112 +1181,119 @@ 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)
 	if err != nil {
 		return nil, NewClientError(400, "invalid request body")
 	}
 
-	now := time.Now()
-	existing, err := h.config.GetAccountServiceOverride(ctx, accountID, provider, service)
+	present, err := presentKeys(httpReq.Body, serviceOverrideOverlayKeys)
 	if err != nil {
-		return nil, fmt.Errorf("accounts: get existing override: %w", err)
+		return nil, err
 	}
 
-	override := buildServiceOverride(accountID, provider, service, req, existing, now)
+	// 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: %w", err)
+	}
+
+	return override, nil
+}
 
-	// Defense-in-depth: reject invalid (term, payment) combos 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),
 		Payment:  derefString(override.Payment),
 	}); 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
 }
 
-// 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
 	}
 }

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
 				},
 			},
 		}

internal/api/handler_config.go

@@ -382,7 +382,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
@@ -391,7 +391,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
 		}
 	}