From 8e3c9502122a075accbcbfef0e01baab67d0933b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 14 Oct 2026 20:46:50 +0200 Subject: [PATCH 1/3] fix(api): distinguish omitted and cleared plan caps --- internal/api/handler_plans.go | 15 ++- ...dler_plans_concurrency_integration_test.go | 103 ++++++++++++++++++ internal/api/handler_plans_step_cap_test.go | 49 ++++++++- internal/api/openapi.yaml | 4 +- 4 files changed, 164 insertions(+), 7 deletions(-) diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index f44a1e85..2e6ca4a6 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -294,6 +294,12 @@ func (h *Handler) updatePlan(ctx context.Context, httpReq *events.LambdaFunction if err != nil { return nil, NewClientError(400, "invalid request body") } + var capField struct { + MaxStepCommitment json.RawMessage `json:"max_step_commitment"` + } + if err = json.Unmarshal([]byte(httpReq.Body), &capField); err != nil { + return nil, NewClientError(400, "invalid request body") + } // Fetch existing plan to preserve data not sent in request. Do NOT wrap // err with the raw plan UUID: a non-ClientError propagates to the router's @@ -306,7 +312,7 @@ func (h *Handler) updatePlan(ctx context.Context, httpReq *events.LambdaFunction "updatePlan: GetPurchasePlan failed") } - plan, err := mergeUpdatedPlan(existingPlan, &req, planID) + plan, err := mergeUpdatedPlan(existingPlan, &req, planID, capField.MaxStepCommitment != nil) if err != nil { return nil, err } @@ -325,7 +331,7 @@ func (h *Handler) updatePlan(ctx context.Context, httpReq *events.LambdaFunction // mergeUpdatedPlan builds the plan an update request describes, carrying over // the state the request cannot express (timestamps, ramp progress, services) // from the stored plan, and validates it. -func mergeUpdatedPlan(existingPlan *config.PurchasePlan, req *PlanRequest, planID string) (*config.PurchasePlan, error) { +func mergeUpdatedPlan(existingPlan *config.PurchasePlan, req *PlanRequest, planID string, capPresent bool) (*config.PurchasePlan, error) { // Create new plan from request plan := req.toPurchasePlan() plan.ID = planID @@ -343,9 +349,12 @@ func mergeUpdatedPlan(existingPlan *config.PurchasePlan, req *PlanRequest, planI return nil, err } - if req.MaxStepCommitment == nil { + if req.MaxStepCommitment == nil && !capPresent { plan.MaxStepCommitment = existingPlan.MaxStepCommitment } + if capPresent && req.MaxStepCommitment == nil && plan.AutoPurchase { + return nil, NewClientError(400, "max_step_commitment cannot be cleared while auto_purchase is enabled") + } // If no services were created from request, preserve existing services if len(plan.Services) == 0 && len(existingPlan.Services) > 0 { diff --git a/internal/api/handler_plans_concurrency_integration_test.go b/internal/api/handler_plans_concurrency_integration_test.go index 879e6bf1..6107b2ca 100644 --- a/internal/api/handler_plans_concurrency_integration_test.go +++ b/internal/api/handler_plans_concurrency_integration_test.go @@ -15,6 +15,8 @@ package api import ( "context" + "encoding/json" + "fmt" "sync" "testing" "time" @@ -22,6 +24,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/aws/aws-lambda-go/events" "github.com/google/uuid" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgxpool" @@ -262,3 +265,103 @@ func TestCreatePlannedPurchases_CanceledStepIsReschedulable(t *testing.T) { require.NoError(t, err, "a step whose only rows were canceled must be reschedulable") assert.Equal(t, 1, resp.Created) } + +func TestUpdatePlan_CapPresencePostgres(t *testing.T) { + cap100, cap5 := 100.0, 5.0 + cases := []struct { + name string + field string + auto bool + status int + want *float64 + }{ + {"manual null clears", `,"max_step_commitment":null`, false, 200, nil}, + {"omitted preserves", "", false, 200, &cap100}, + {"numeric replaces", `,"max_step_commitment":5`, false, 200, &cap5}, + {"automatic omitted preserves", "", true, 200, &cap100}, + {"automatic numeric decreases", `,"max_step_commitment":5`, true, 200, &cap5}, + {"automatic null refuses", `,"max_step_commitment":null`, true, 400, &cap100}, + {"string refuses", `,"max_step_commitment":"5"`, false, 400, &cap100}, + {"zero refuses", `,"max_step_commitment":0`, false, 400, &cap100}, + {"negative refuses", `,"max_step_commitment":-1`, false, 400, &cap100}, + {"precision refuses", `,"max_step_commitment":1.005`, false, 400, &cap100}, + {"unknown remains ignored", `,"max_step_commitment":null,"future_field":false`, false, 200, nil}, + {"case-insensitive null", `,"MAX_STEP_COMMITMENT":null`, false, 200, nil}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + f := newCreateConcurrencyFixture(ctx, t) + plan, err := f.store.GetPurchasePlan(ctx, f.planID) + require.NoError(t, err) + plan.Enabled, plan.AutoPurchase = true, tc.auto + plan.NotificationDaysBefore = 2 + plan.MaxStepCommitment = &cap100 + plan.Services = map[string]config.ServiceConfig{"aws:ec2": {Provider: "aws", Service: "ec2", Enabled: true, Term: 1, Payment: "no-upfront", Coverage: 80}} + next := time.Now().AddDate(0, 0, 7) + plan.NextExecutionDate = &next + require.NoError(t, f.store.UpdatePurchasePlan(ctx, plan)) + before, err := f.store.GetPurchasePlan(ctx, f.planID) + require.NoError(t, err) + auth := f.handler.auth.(*MockAuthService) + auth.On("ValidateCSRFToken", mock.Anything, mock.Anything, mock.Anything).Return(nil) + body := fmt.Sprintf(`{"name":"Concurrent Create Plan","provider":"aws","service":"ec2","term":1,"payment":"no-upfront","target_coverage":80,"ramp_schedule":"weekly-25pct","enabled":true,"auto_purchase":%t,"notification_days_before":2%s}`, tc.auto, tc.field) + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token", "Content-Type": "application/json", "X-CSRF-Token": "test-csrf"}, Body: body, + RequestContext: events.LambdaFunctionURLRequestContext{HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "PUT", Path: "/api/plans/" + f.planID}}, + } + response, err := f.handler.HandleRequest(ctx, req) + require.NoError(t, err) + require.Equal(t, tc.status, response.StatusCode, response.Body) + stored, err := f.store.GetPurchasePlan(ctx, f.planID) + require.NoError(t, err) + assert.Equal(t, tc.want, stored.MaxStepCommitment) + assert.Equal(t, tc.auto, stored.AutoPurchase) + assert.Equal(t, before.RampSchedule.CurrentStep, stored.RampSchedule.CurrentStep) + assert.True(t, before.RampSchedule.StartDate.Equal(stored.RampSchedule.StartDate)) + assert.True(t, before.NextExecutionDate.Equal(*stored.NextExecutionDate)) + req.RequestContext.HTTP.Method, req.Body = "GET", "" + response, err = f.handler.HandleRequest(ctx, req) + require.NoError(t, err) + require.Equal(t, 200, response.StatusCode, response.Body) + var returned config.PurchasePlan + require.NoError(t, json.Unmarshal([]byte(response.Body), &returned)) + assert.Equal(t, tc.want, returned.MaxStepCommitment) + }) + } +} + +type concurrentCapStore struct{ config.StoreInterface } + +func (s concurrentCapStore) UpdatePurchasePlan(ctx context.Context, plan *config.PurchasePlan) error { + winner, err := s.GetPurchasePlan(ctx, plan.ID) + if err != nil { + return err + } + cap80 := 80.0 + winner.MaxStepCommitment = &cap80 + if err = s.StoreInterface.UpdatePurchasePlan(ctx, winner); err != nil { + return err + } + return s.StoreInterface.UpdatePurchasePlan(ctx, plan) +} + +func TestUpdatePlan_CapNullPreservesConcurrentWinner(t *testing.T) { + ctx := context.Background() + f := newCreateConcurrencyFixture(ctx, t) + plan, err := f.store.GetPurchasePlan(ctx, f.planID) + require.NoError(t, err) + cap100 := 100.0 + plan.MaxStepCommitment = &cap100 + require.NoError(t, f.store.UpdatePurchasePlan(ctx, plan)) + f.handler.auth.(*MockAuthService).On("ValidateCSRFToken", mock.Anything, mock.Anything, mock.Anything).Return(nil) + f.handler.config = concurrentCapStore{StoreInterface: f.store} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-token", "Content-Type": "application/json", "X-CSRF-Token": "test-csrf"}, Body: `{"name":"Concurrent Create Plan","ramp_schedule":"weekly-25pct","enabled":false,"auto_purchase":false,"max_step_commitment":null}`, RequestContext: events.LambdaFunctionURLRequestContext{HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "PUT", Path: "/api/plans/" + f.planID}}} + response, err := f.handler.HandleRequest(ctx, req) + require.NoError(t, err) + require.Equal(t, 409, response.StatusCode, response.Body) + stored, err := f.store.GetPurchasePlan(ctx, f.planID) + require.NoError(t, err) + require.NotNil(t, stored.MaxStepCommitment) + assert.Equal(t, 80.0, *stored.MaxStepCommitment) +} diff --git a/internal/api/handler_plans_step_cap_test.go b/internal/api/handler_plans_step_cap_test.go index a5b5a1d0..123ea146 100644 --- a/internal/api/handler_plans_step_cap_test.go +++ b/internal/api/handler_plans_step_cap_test.go @@ -1,11 +1,16 @@ package api import ( + "context" + "fmt" "math" "testing" + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -15,22 +20,60 @@ func TestMergeUpdatedPlan_MaxStepCommitment(t *testing.T) { RampSchedule: config.RampSchedule{Type: "immediate", PercentPerStep: 100, TotalSteps: 1}} t.Run("omitted on update keeps the current cap", func(t *testing.T) { - plan, err := mergeUpdatedPlan(existing, &PlanRequest{Name: "p", RampSchedule: "immediate"}, "id") + plan, err := mergeUpdatedPlan(existing, &PlanRequest{Name: "p", RampSchedule: "immediate"}, "id", false) require.NoError(t, err) require.NotNil(t, plan.MaxStepCommitment) assert.Equal(t, 100.0, *plan.MaxStepCommitment) }) t.Run("provided on update replaces it", func(t *testing.T) { - plan, err := mergeUpdatedPlan(existing, &PlanRequest{Name: "p", RampSchedule: "immediate", MaxStepCommitment: &cap5}, "id") + plan, err := mergeUpdatedPlan(existing, &PlanRequest{Name: "p", RampSchedule: "immediate", MaxStepCommitment: &cap5}, "id", true) require.NoError(t, err) assert.Equal(t, 5.0, *plan.MaxStepCommitment) }) + t.Run("explicit null clears a manual cap", func(t *testing.T) { + plan, err := mergeUpdatedPlan(existing, &PlanRequest{Name: "p", RampSchedule: "immediate"}, "id", true) + require.NoError(t, err) + assert.Nil(t, plan.MaxStepCommitment) + assert.Equal(t, 100.0, *existing.MaxStepCommitment) + }) + t.Run("explicit null refuses automatic mode", func(t *testing.T) { + plan, err := mergeUpdatedPlan(existing, &PlanRequest{Name: "p", RampSchedule: "immediate", AutoPurchase: true}, "id", true) + require.ErrorContains(t, err, "cannot be cleared") + assert.Nil(t, plan) + }) for name, v := range map[string]float64{"zero": 0, "negative": -1, "NaN": math.NaN(), "Inf": math.Inf(1)} { t.Run("rejects "+name, func(t *testing.T) { bad := v - _, err := mergeUpdatedPlan(existing, &PlanRequest{Name: "p", RampSchedule: "immediate", MaxStepCommitment: &bad}, "id") + _, err := mergeUpdatedPlan(existing, &PlanRequest{Name: "p", RampSchedule: "immediate", MaxStepCommitment: &bad}, "id", true) require.Error(t, err) assert.Contains(t, err.Error(), "max_step_commitment") }) } } + +func TestUpdatePlan_CapNullKeepsAuthorization(t *testing.T) { + for _, status := range []int{403, 404} { + t.Run(fmt.Sprint(status), func(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "cap-token").Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"}, nil) + mockAuth.On("ValidateCSRFToken", mock.Anything, mock.Anything, mock.Anything).Return(nil) + if status == 403 { + mockAuth.grantPermissions([]auth.Permission{{Action: auth.ActionView, Resource: auth.ResourcePlans}}) + } else { + mockAuth.grantScoped("allowed-account") + store.GetPlanAccountsFn = func(context.Context, string) ([]config.CloudAccount, error) { + return []config.CloudAccount{{Name: "other-account"}}, nil + } + } + h := &Handler{config: store, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer cap-token", "Content-Type": "application/json", "X-CSRF-Token": "test-csrf"}, Body: `{"name":"p","max_step_commitment":null}`, RequestContext: events.LambdaFunctionURLRequestContext{HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "PUT", Path: "/api/plans/11111111-1111-1111-1111-111111111111"}}} + response, err := h.HandleRequest(ctx, req) + require.NoError(t, err) + assert.Equal(t, status, response.StatusCode, response.Body) + store.AssertNotCalled(t, "GetPurchasePlan") + store.AssertNotCalled(t, "UpdatePurchasePlan") + }) + } +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 6a2ee39d..f5667a69 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -3172,12 +3172,14 @@ components: type: integer max_step_commitment: type: number + nullable: true exclusiveMinimum: 0 description: >- Ceiling on the total commitment (upfront + monthly * term) one ramp step may auto-purchase. Omit on create for no cap (auto-purchase of steps with recommendations is then refused); omit on update to keep - the current cap. + the current cap. Explicit null clears a manual plan's cap and is + refused while auto_purchase is true. provider: type: string service: From eab418141a743d57f9826b8196bf28d960e942f6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 14 Oct 2026 21:02:10 +0200 Subject: [PATCH 2/3] docs(api): qualify plan cap clearing on update --- internal/api/openapi.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index f5667a69..346144b8 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -3178,8 +3178,8 @@ components: Ceiling on the total commitment (upfront + monthly * term) one ramp step may auto-purchase. Omit on create for no cap (auto-purchase of steps with recommendations is then refused); omit on update to keep - the current cap. Explicit null clears a manual plan's cap and is - refused while auto_purchase is true. + the current cap. On update, explicit null clears a manual plan's + cap and is refused while auto_purchase is true. provider: type: string service: From f8b359d43e68ce35b96259261a4f32b347bb7102 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 15 Oct 2026 00:15:54 +0200 Subject: [PATCH 3/3] test(api): preserve Savings Plans guard with cap presence --- internal/api/handler_plans_sp_multistep_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/api/handler_plans_sp_multistep_test.go b/internal/api/handler_plans_sp_multistep_test.go index b8d81661..3e72a9cb 100644 --- a/internal/api/handler_plans_sp_multistep_test.go +++ b/internal/api/handler_plans_sp_multistep_test.go @@ -49,7 +49,7 @@ func TestCreatePlan_RejectsArmedMultiStepSavingsPlan(t *testing.T) { func TestMergeUpdatedPlan_RejectsArmedMultiStepSavingsPlan(t *testing.T) { existing := &config.PurchasePlan{Name: "p", RampSchedule: config.RampSchedule{Type: "immediate", PercentPerStep: 100, TotalSteps: 1}} _, err := mergeUpdatedPlan(existing, &PlanRequest{Name: "p", Provider: "aws", Service: "savingsplans", - RampSchedule: "weekly-25pct", Enabled: true, AutoPurchase: true}, "id") + RampSchedule: "weekly-25pct", Enabled: true, AutoPurchase: true}, "id", false) requireSP400(t, err) }