From fcc397929f6866848966ca551bd4ef48fe20ee79 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Mon, 17 Aug 2026 11:14:29 +0530 Subject: [PATCH 1/2] fix(reconcile): honor a product's stated behavior on create and match feature-name case on apply --- billing/product/service.go | 6 +- billing/product/service_test.go | 103 ++++++++++++++++++ internal/reconcile/billingproduct.go | 26 +++-- .../reconcile/billingproduct_reconciler.go | 4 +- internal/reconcile/billingproduct_test.go | 25 ++++- 5 files changed, 151 insertions(+), 13 deletions(-) diff --git a/billing/product/service.go b/billing/product/service.go index cd1d4ba8c6..f88b93137e 100644 --- a/billing/product/service.go +++ b/billing/product/service.go @@ -64,8 +64,12 @@ func (s *Service) Create(ctx context.Context, product Product) (Product, error) product.ID = uuid.New().String() product.ProviderID = product.ID } + // Capture whether the caller stated a behavior before SetDefaults fills the + // "basic" default, so an explicit behavior is honored and only an omitted one + // on a credit product falls back to "credits". + statedBehavior := product.Behavior defaults.SetDefaults(&product) - if product.Config.CreditAmount > 0 { + if statedBehavior == "" && product.Config.CreditAmount > 0 { product.Behavior = CreditBehavior } product.Name = strings.ToLower(product.Name) diff --git a/billing/product/service_test.go b/billing/product/service_test.go index b1ca1eb785..00fbcfa19e 100644 --- a/billing/product/service_test.go +++ b/billing/product/service_test.go @@ -85,6 +85,109 @@ func TestService_Create(t *testing.T) { return product.NewService(stripeClient, mockProductRepo, mockPriceRepo, mockFeatureRepo) }, }, + { + name: "honors an explicit behavior on a credit product instead of forcing credits", + args: args{ + product: product.Product{ + ID: "creditprod", + Name: "creditprod", + Description: "credit product", + Behavior: product.PerSeatBehavior, + Config: product.BehaviorConfig{CreditAmount: 5}, + }, + }, + want: product.Product{ + ID: "creditprod", + Name: "creditprod", + Description: "credit product", + Behavior: product.PerSeatBehavior, + Config: product.BehaviorConfig{CreditAmount: 5}, + }, + wantErr: false, + setup: func() *product.Service { + stripeClient, mockStripeBackend, mockProductRepo, mockPriceRepo, mockFeatureRepo := mockService(t) + mockProductRepo.EXPECT().Create(ctx, product.Product{ + ID: "creditprod", + Name: "creditprod", + Description: "credit product", + Behavior: product.PerSeatBehavior, + Config: product.BehaviorConfig{CreditAmount: 5}, + }).Return(product.Product{ + ID: "creditprod", + Name: "creditprod", + Description: "credit product", + Behavior: product.PerSeatBehavior, + Config: product.BehaviorConfig{CreditAmount: 5}, + }, nil) + mockStripeBackend.EXPECT().Call("POST", "/v1/products", "key_123", &stripe.ProductParams{ + Params: stripe.Params{ + Context: ctx, + }, + ID: new(""), + Name: new(""), + Description: new("credit product"), + Metadata: map[string]string{ + "behavior": "per_seat", + "credit_amount": "5", + "managed_by": "frontier", + "name": "creditprod", + "product_id": "creditprod", + }, + }, &stripe.Product{}).Return(nil) + return product.NewService(stripeClient, mockProductRepo, mockPriceRepo, mockFeatureRepo) + }, + }, + { + name: "defaults an omitted behavior to credits on a credit product", + args: args{ + product: product.Product{ + ID: "creditprod2", + Name: "creditprod2", + Description: "credit product", + Config: product.BehaviorConfig{CreditAmount: 5}, + }, + }, + want: product.Product{ + ID: "creditprod2", + Name: "creditprod2", + Description: "credit product", + Behavior: product.CreditBehavior, + Config: product.BehaviorConfig{CreditAmount: 5}, + }, + wantErr: false, + setup: func() *product.Service { + stripeClient, mockStripeBackend, mockProductRepo, mockPriceRepo, mockFeatureRepo := mockService(t) + mockProductRepo.EXPECT().Create(ctx, product.Product{ + ID: "creditprod2", + Name: "creditprod2", + Description: "credit product", + Behavior: product.CreditBehavior, + Config: product.BehaviorConfig{CreditAmount: 5}, + }).Return(product.Product{ + ID: "creditprod2", + Name: "creditprod2", + Description: "credit product", + Behavior: product.CreditBehavior, + Config: product.BehaviorConfig{CreditAmount: 5}, + }, nil) + mockStripeBackend.EXPECT().Call("POST", "/v1/products", "key_123", &stripe.ProductParams{ + Params: stripe.Params{ + Context: ctx, + }, + ID: new(""), + Name: new(""), + Description: new("credit product"), + Metadata: map[string]string{ + "behavior": "credits", + "credit_amount": "5", + "managed_by": "frontier", + "name": "creditprod2", + "product_id": "creditprod2", + }, + }, &stripe.Product{}).Return(nil) + return product.NewService(stripeClient, mockProductRepo, mockPriceRepo, mockFeatureRepo) + }, + }, { name: "should create product in repo and billing provider with price and features", args: args{ diff --git a/internal/reconcile/billingproduct.go b/internal/reconcile/billingproduct.go index 0ad7759414..70e30c035b 100644 --- a/internal/reconcile/billingproduct.go +++ b/internal/reconcile/billingproduct.go @@ -259,14 +259,14 @@ func billingProductChanges(s BillingProductSpec, cur currentBillingProduct) ([]s if s.Description != cur.Description { changes = append(changes, "description") } - // behavior is create-only: the server sets it at create (rewriting it to - // "credits" when credit_amount > 0) and never changes it on update. A file - // that asks to change it to something the product is not cannot apply, so fail - // the plan rather than skip it in silence. A credit product's behavior is - // always "credits", so fold that in to avoid a false failure when the file - // still names a behavior alongside a credit amount. + // behavior is create-only: the server sets it at create and never changes it + // on update. The file's behavior is honored as written, so a file that names a + // behavior the product was not created with fails the plan rather than being + // silently overridden. Only an omitted behavior on a credit product falls back + // to "credits", matching the server's create-time default, so an omitted + // behavior does not read as a change against a credit product. expectedBehavior := s.Behavior - if s.Config.CreditAmount > 0 { + if expectedBehavior == "" && s.Config.CreditAmount > 0 { expectedBehavior = "credits" } if expectedBehavior != "" && expectedBehavior != cur.Behavior { @@ -298,14 +298,22 @@ func billingConfigChanged(desired, cur BillingProductConfig) bool { desired.MaxQuantity != cur.MaxQuantity } +// normalizeFeatureName is the canonical form of a feature name, used by both the +// diff and the apply so a name is compared and written the same way. The server +// looks features up by name, so a case or whitespace difference between the two +// would fork a duplicate feature the diff had reported as unchanged. +func normalizeFeatureName(name string) string { + return strings.ToLower(strings.TrimSpace(name)) +} + func billingFeatureSetsEqual(desired []BillingFeatureRef, current []string) bool { d := make([]string, 0, len(desired)) for _, f := range desired { - d = append(d, strings.ToLower(strings.TrimSpace(f.Name))) + d = append(d, normalizeFeatureName(f.Name)) } c := make([]string, 0, len(current)) for _, name := range current { - c = append(c, strings.ToLower(strings.TrimSpace(name))) + c = append(c, normalizeFeatureName(name)) } return stringSetsEqual(uniqueSorted(d), uniqueSorted(c)) } diff --git a/internal/reconcile/billingproduct_reconciler.go b/internal/reconcile/billingproduct_reconciler.go index 4aa255f498..d40ede7ee7 100644 --- a/internal/reconcile/billingproduct_reconciler.go +++ b/internal/reconcile/billingproduct_reconciler.go @@ -288,7 +288,9 @@ func billingProductBody(s BillingProductSpec) *frontierv1beta1.ProductRequestBod } features := make([]*frontierv1beta1.Feature, 0, len(s.Features)) for _, f := range s.Features { - features = append(features, &frontierv1beta1.Feature{Name: f.Name}) + // Send the name in the same normalized form the diff compares, so the + // server matches an existing feature instead of forking a duplicate. + features = append(features, &frontierv1beta1.Feature{Name: normalizeFeatureName(f.Name)}) } return &frontierv1beta1.ProductRequestBody{ Name: s.Name, diff --git a/internal/reconcile/billingproduct_test.go b/internal/reconcile/billingproduct_test.go index a309b969bd..6a4a217d4e 100644 --- a/internal/reconcile/billingproduct_test.go +++ b/internal/reconcile/billingproduct_test.go @@ -149,14 +149,21 @@ func TestDiffBillingProducts(t *testing.T) { } }) - t.Run("does not fail on a behavior a credit amount forces", func(t *testing.T) { + t.Run("defaults an omitted behavior to credits for a credit product", func(t *testing.T) { s := newBillingProduct() - s.Behavior = "basic" // the file names basic, but credit_amount > 0 forces "credits" + s.Behavior = "" // omitted; credit_amount > 0 defaults it to the server's "credits" ops, err := diffBillingProducts([]BillingProductSpec{s}, []currentBillingProduct{curToken()}) assert.NoError(t, err) assert.Empty(t, ops) }) + t.Run("fails when the file names a behavior the credit product was not created with", func(t *testing.T) { + s := newBillingProduct() + s.Behavior = "basic" // the file states basic, but the product was created as credits + _, err := diffBillingProducts([]BillingProductSpec{s}, []currentBillingProduct{curToken()}) + assert.ErrorContains(t, err, "behavior cannot change") + }) + t.Run("fails the plan on a behavior change the server cannot apply", func(t *testing.T) { cur := curToken() cur.Behavior = "basic" @@ -254,3 +261,17 @@ func TestDiffBillingProducts(t *testing.T) { assert.Empty(t, ops) }) } + +func TestBillingProductBody_NormalizesFeatureNames(t *testing.T) { + // The diff compares feature names case-insensitively and trimmed, so the apply + // body must send them the same way, or the server (which looks features up by + // name) forks a duplicate feature the plan reported as unchanged. + s := newBillingProduct() + s.Features = []BillingFeatureRef{{Name: " Foo "}, {Name: "BAR"}} + body := billingProductBody(s) + var got []string + for _, f := range body.GetFeatures() { + got = append(got, f.GetName()) + } + assert.Equal(t, []string{"foo", "bar"}, got) +} From 10ce27632533c5c97b397b03111dc63e0592990f Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Wed, 19 Aug 2026 12:59:27 +0530 Subject: [PATCH 2/2] fix(billing): reject a credit amount on a non-credit behavior and send feature names verbatim --- billing/product/service.go | 7 ++++ billing/product/service_test.go | 32 +++++++++++++++---- internal/reconcile/billingproduct.go | 20 +++++++++--- .../reconcile/billingproduct_reconciler.go | 9 ++++-- internal/reconcile/billingproduct_test.go | 20 ++++++++---- 5 files changed, 69 insertions(+), 19 deletions(-) diff --git a/billing/product/service.go b/billing/product/service.go index f88b93137e..2976e17e55 100644 --- a/billing/product/service.go +++ b/billing/product/service.go @@ -72,6 +72,13 @@ func (s *Service) Create(ctx context.Context, product Product) (Product, error) if statedBehavior == "" && product.Config.CreditAmount > 0 { product.Behavior = CreditBehavior } + // Credits are only granted for the credits behavior, so a positive credit + // amount on any other behavior would store credits that no path ever awards. + // Reject the contradiction instead of silently keeping a product whose credits + // are dead. + if product.Config.CreditAmount > 0 && product.Behavior != CreditBehavior { + return Product{}, fmt.Errorf("%w: credit_amount %d needs behavior %q, got %q", ErrInvalidDetail, product.Config.CreditAmount, CreditBehavior, product.Behavior) + } product.Name = strings.ToLower(product.Name) _, err := s.stripeClient.Products.New(&stripe.ProductParams{ diff --git a/billing/product/service_test.go b/billing/product/service_test.go index 00fbcfa19e..3a34b454de 100644 --- a/billing/product/service_test.go +++ b/billing/product/service_test.go @@ -86,13 +86,13 @@ func TestService_Create(t *testing.T) { }, }, { - name: "honors an explicit behavior on a credit product instead of forcing credits", + name: "honors an explicit credits behavior on a credit product", args: args{ product: product.Product{ ID: "creditprod", Name: "creditprod", Description: "credit product", - Behavior: product.PerSeatBehavior, + Behavior: product.CreditBehavior, Config: product.BehaviorConfig{CreditAmount: 5}, }, }, @@ -100,7 +100,7 @@ func TestService_Create(t *testing.T) { ID: "creditprod", Name: "creditprod", Description: "credit product", - Behavior: product.PerSeatBehavior, + Behavior: product.CreditBehavior, Config: product.BehaviorConfig{CreditAmount: 5}, }, wantErr: false, @@ -110,13 +110,13 @@ func TestService_Create(t *testing.T) { ID: "creditprod", Name: "creditprod", Description: "credit product", - Behavior: product.PerSeatBehavior, + Behavior: product.CreditBehavior, Config: product.BehaviorConfig{CreditAmount: 5}, }).Return(product.Product{ ID: "creditprod", Name: "creditprod", Description: "credit product", - Behavior: product.PerSeatBehavior, + Behavior: product.CreditBehavior, Config: product.BehaviorConfig{CreditAmount: 5}, }, nil) mockStripeBackend.EXPECT().Call("POST", "/v1/products", "key_123", &stripe.ProductParams{ @@ -127,7 +127,7 @@ func TestService_Create(t *testing.T) { Name: new(""), Description: new("credit product"), Metadata: map[string]string{ - "behavior": "per_seat", + "behavior": "credits", "credit_amount": "5", "managed_by": "frontier", "name": "creditprod", @@ -137,6 +137,26 @@ func TestService_Create(t *testing.T) { return product.NewService(stripeClient, mockProductRepo, mockPriceRepo, mockFeatureRepo) }, }, + { + name: "rejects a credit amount paired with a non-credit behavior", + args: args{ + product: product.Product{ + ID: "creditprod", + Name: "creditprod", + Description: "credit product", + Behavior: product.PerSeatBehavior, + Config: product.BehaviorConfig{CreditAmount: 5}, + }, + }, + want: product.Product{}, + wantErr: true, + setup: func() *product.Service { + // The contradiction is rejected before any provider or repo call, so + // no expectations are set. + stripeClient, _, mockProductRepo, mockPriceRepo, mockFeatureRepo := mockService(t) + return product.NewService(stripeClient, mockProductRepo, mockPriceRepo, mockFeatureRepo) + }, + }, { name: "defaults an omitted behavior to credits on a credit product", args: args{ diff --git a/internal/reconcile/billingproduct.go b/internal/reconcile/billingproduct.go index 70e30c035b..9fdc540fac 100644 --- a/internal/reconcile/billingproduct.go +++ b/internal/reconcile/billingproduct.go @@ -132,6 +132,16 @@ func validateBillingProductSpec(s BillingProductSpec) error { return fmt.Errorf("product %q cannot be deleted: there is no product delete or deactivate API; remove the entry and archive the product by hand", s.Name) } + // Credits are only granted for the credits behavior. A positive credit amount + // with any other stated behavior stores credits the server never awards, so + // reject it at plan time rather than apply a product whose credits are dead. An + // omitted behavior is fine: the server coerces it to credits when there is a + // credit amount. + behavior := strings.ToLower(strings.TrimSpace(s.Behavior)) + if s.Config.CreditAmount > 0 && behavior != "" && behavior != string(product.CreditBehavior) { + return fmt.Errorf("product %q sets credit_amount %d with behavior %q: credits are only granted for the %q behavior", s.Name, s.Config.CreditAmount, behavior, product.CreditBehavior) + } + seenPrice := map[string]struct{}{} for _, p := range s.Prices { priceName := strings.ToLower(strings.TrimSpace(p.Name)) @@ -298,10 +308,12 @@ func billingConfigChanged(desired, cur BillingProductConfig) bool { desired.MaxQuantity != cur.MaxQuantity } -// normalizeFeatureName is the canonical form of a feature name, used by both the -// diff and the apply so a name is compared and written the same way. The server -// looks features up by name, so a case or whitespace difference between the two -// would fork a duplicate feature the diff had reported as unchanged. +// normalizeFeatureName is the canonical form of a feature name, used only to +// compare the desired and current feature sets so a case or whitespace difference +// does not plan a spurious update. The apply does not use it: the server matches +// and stores feature names verbatim, so sending a normalized name would miss a +// differently cased stored feature and fork a duplicate. Making the two fully +// agree needs the server to canonicalize feature names, which is a separate change. func normalizeFeatureName(name string) string { return strings.ToLower(strings.TrimSpace(name)) } diff --git a/internal/reconcile/billingproduct_reconciler.go b/internal/reconcile/billingproduct_reconciler.go index d40ede7ee7..8cfe964bb5 100644 --- a/internal/reconcile/billingproduct_reconciler.go +++ b/internal/reconcile/billingproduct_reconciler.go @@ -288,9 +288,12 @@ func billingProductBody(s BillingProductSpec) *frontierv1beta1.ProductRequestBod } features := make([]*frontierv1beta1.Feature, 0, len(s.Features)) for _, f := range s.Features { - // Send the name in the same normalized form the diff compares, so the - // server matches an existing feature instead of forking a duplicate. - features = append(features, &frontierv1beta1.Feature{Name: normalizeFeatureName(f.Name)}) + // Send the name verbatim. The server matches and stores feature names + // case-sensitively, so a normalized name here would miss a differently + // cased stored feature and fork a duplicate. The diff still compares + // normalized names so a case-only difference does not plan a spurious + // update. + features = append(features, &frontierv1beta1.Feature{Name: f.Name}) } return &frontierv1beta1.ProductRequestBody{ Name: s.Name, diff --git a/internal/reconcile/billingproduct_test.go b/internal/reconcile/billingproduct_test.go index 6a4a217d4e..62dd73a54a 100644 --- a/internal/reconcile/billingproduct_test.go +++ b/internal/reconcile/billingproduct_test.go @@ -53,6 +53,12 @@ func TestValidateBillingProductSpec(t *testing.T) { {"duplicate feature", func(s *BillingProductSpec) { s.Features = []BillingFeatureRef{{Name: "f"}, {Name: "f"}} }, "more than once"}, + {"credit amount with a non-credit behavior rejected", func(s *BillingProductSpec) { + s.Behavior = "per_seat" // newBillingProduct already sets CreditAmount 1 + }, "credits are only granted"}, + {"credit amount with an omitted behavior is allowed", func(s *BillingProductSpec) { + s.Behavior = "" // the server coerces an omitted behavior to credits + }, ""}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -159,7 +165,8 @@ func TestDiffBillingProducts(t *testing.T) { t.Run("fails when the file names a behavior the credit product was not created with", func(t *testing.T) { s := newBillingProduct() - s.Behavior = "basic" // the file states basic, but the product was created as credits + s.Behavior = "basic" // the file states basic, but the product was created as credits + s.Config.CreditAmount = 0 // basic carries no credit amount, so the spec is valid on its own _, err := diffBillingProducts([]BillingProductSpec{s}, []currentBillingProduct{curToken()}) assert.ErrorContains(t, err, "behavior cannot change") }) @@ -262,10 +269,11 @@ func TestDiffBillingProducts(t *testing.T) { }) } -func TestBillingProductBody_NormalizesFeatureNames(t *testing.T) { - // The diff compares feature names case-insensitively and trimmed, so the apply - // body must send them the same way, or the server (which looks features up by - // name) forks a duplicate feature the plan reported as unchanged. +func TestBillingProductBody_SendsFeatureNamesVerbatim(t *testing.T) { + // The server matches and stores feature names case-sensitively, so the apply + // body must send them exactly as written. Normalizing here would miss a + // differently cased stored feature and fork a duplicate. The diff still + // compares normalized names, so a case-only difference does not plan a change. s := newBillingProduct() s.Features = []BillingFeatureRef{{Name: " Foo "}, {Name: "BAR"}} body := billingProductBody(s) @@ -273,5 +281,5 @@ func TestBillingProductBody_NormalizesFeatureNames(t *testing.T) { for _, f := range body.GetFeatures() { got = append(got, f.GetName()) } - assert.Equal(t, []string{"foo", "bar"}, got) + assert.Equal(t, []string{" Foo ", "BAR"}, got) }