refactor: reject unit config in v1 handler/service instead of the response mapper

Author: rolosp

e2e/v1readsurface_unitconfig_test.go

@@ -147,6 +147,37 @@ func TestV1ReadSurfaceExcludesUnitConfig(t *testing.T) {
 		assert.Equal(t, 1, resp.JSON200.TotalCount, "TotalCount must reflect the filtered set, not the raw count")
 	})
 
+	// and:
+	// - v1 mutations on the unit_config plan are rejected BEFORE any side effect. The guard runs
+	//   pre-commit in the service (not in the response mapper), so a 400 means nothing committed —
+	//   the plan is left exactly as it was. This is the core OM-409 follow-up: no commit-then-400.
+	runRequired(t, "v1 plan mutations reject the unit_config plan with no side effect", func(t *testing.T) {
+		// archive: the guard runs before the active-status check, so an active plan is still rejected.
+		archiveResp, err := v1.ArchivePlanWithResponse(t.Context(), ucPlan.ID)
+		require.NoError(t, err)
+		require.Equal(t, http.StatusBadRequest, archiveResp.StatusCode(), "body: %s", string(archiveResp.Body))
+		assert.Contains(t, string(archiveResp.Body), unitConfigNotRepresentableCode, "archive 400 must carry the typed unit_config code")
+
+		// publish: the guard runs before the publishable-status check.
+		publishResp, err := v1.PublishPlanWithResponse(t.Context(), ucPlan.ID)
+		require.NoError(t, err)
+		require.Equal(t, http.StatusBadRequest, publishResp.StatusCode(), "body: %s", string(publishResp.Body))
+		assert.Contains(t, string(publishResp.Body), unitConfigNotRepresentableCode, "publish 400 must carry the typed unit_config code")
+
+		// next: the guard runs before the next draft version is created.
+		nextResp, err := v1.NextPlanWithResponse(t.Context(), ucPlan.ID)
+		require.NoError(t, err)
+		require.Equal(t, http.StatusBadRequest, nextResp.StatusCode(), "body: %s", string(nextResp.Body))
+		assert.Contains(t, string(nextResp.Body), unitConfigNotRepresentableCode, "next 400 must carry the typed unit_config code")
+
+		// no side effect: after three rejected mutations the plan is unchanged — still the single
+		// active version it was published as (a commit-then-400 mapper would have left it archived).
+		after, err := c.Plans.Get(t.Context(), ucPlan.ID)
+		c.requireStatus(http.StatusOK, err)
+		require.NotNil(t, after)
+		assert.Equal(t, v3sdk.PlanStatusActive, after.Status, "the plan must remain active — no mutation may have committed")
+	})
+
 	// and:
 	// - a v1 subscription created from the unit_config plan BY KEY still succeeds (read ≠ subscribe; the
 	//   server rates it correctly per OM-399, only the read surfaces are restricted).
@@ -185,4 +216,16 @@ func TestV1ReadSurfaceExcludesUnitConfig(t *testing.T) {
 		require.NotNil(t, resp.ApplicationproblemJSON400)
 		assert.Contains(t, string(resp.Body), unitConfigNotRepresentableCode, "the 400 must carry the typed unit_config code")
 	})
+
+	// and:
+	// - v1 add-on read surfaces of that subscription are rejected too (the sub-addon list serializes
+	//   the subscription's items, so it must not silently strip). Guarded on the subscription's spec,
+	//   so it rejects even with no add-ons attached.
+	runRequired(t, "v1 subscription add-on LIST rejects the unit_config subscription", func(t *testing.T) {
+		require.NotEmpty(t, subscriptionID)
+		resp, err := v1.ListSubscriptionAddonsWithResponse(t.Context(), subscriptionID)
+		require.NoError(t, err)
+		require.Equal(t, http.StatusBadRequest, resp.StatusCode(), "body: %s", string(resp.Body))
+		assert.Contains(t, string(resp.Body), unitConfigNotRepresentableCode, "the 400 must carry the typed unit_config code")
+	})
 }

openmeter/productcatalog/addon.go

@@ -184,6 +184,10 @@ func (a Addon) ValidateWith(validators ...models.ValidatorFunc[Addon]) error {
 	return models.Validate(a, validators...)
 }
 
+func (a Addon) HasUnitConfig() bool {
+	return a.RateCards.HasUnitConfig()
+}
+
 // ValidationErrors returns a list of possible validation errors for the add-on.
 // It returns nil if the add-on has no validation issues.
 func (a Addon) ValidationErrors() (models.ValidationIssues, error) {

openmeter/productcatalog/addon/httpdriver/addon.go

@@ -187,6 +187,8 @@ func (h *handler) UpdateAddon() UpdateAddonHandler {
 
 			req.IgnoreNonCriticalIssues = true
 
+			req.RejectUnitConfig = true
+
 			return req, nil
 		},
 		func(ctx context.Context, request UpdateAddonRequest) (UpdateAddonResponse, error) {
@@ -289,6 +291,10 @@ func (h *handler) GetAddon() GetAddonHandler {
 				return GetAddonResponse{}, fmt.Errorf("failed to get add-on [namespace=%s key=%s id=%s]: %w", request.Namespace, request.Key, request.ID, err)
 			}
 
+			if a.AsProductCatalogAddon().HasUnitConfig() {
+				return GetAddonResponse{}, productcatalog.ErrUnitConfigNotRepresentable
+			}
+
 			return FromAddon(*a)
 		},
 		commonhttp.JSONResponseEncoderWithStatus[GetAddonResponse](http.StatusOK),
@@ -324,6 +330,7 @@ func (h *handler) PublishAddon() PublishAddonHandler {
 				EffectivePeriod: productcatalog.EffectivePeriod{
 					EffectiveFrom: lo.ToPtr(clock.Now()),
 				},
+				RejectUnitConfig: true,
 			}
 
 			return req, nil
@@ -367,6 +374,8 @@ func (h *handler) ArchiveAddon() ArchiveAddonHandler {
 					ID:        addonID,
 				},
 				EffectiveTo: clock.Now(),
+				// The v1 API cannot represent unit_config; reject archiving such an add-on.
+				RejectUnitConfig: true,
 			}
 
 			return req, nil

openmeter/productcatalog/addon/service.go

@@ -148,6 +148,12 @@ type inputOptions struct {
 	// ignoreNonCriticalIssues makes Validate() return errors with critical severity or higher.
 	// This allows creating resource with expected validation issues.
 	IgnoreNonCriticalIssues bool
+
+	// RejectUnitConfig makes mutation validation reject an add-on that carries a unit_config
+	// conversion on any rate card. The v1 API cannot represent unit_config, and v1 update
+	// rewrites rate cards from a body that has no such field, so proceeding would silently
+	// drop the conversion. v1 handlers set this; v3 leaves it false.
+	RejectUnitConfig bool
 }
 
 var _ models.Validator = (*CreateAddonInput)(nil)
@@ -353,6 +359,10 @@ type PublishAddonInput struct {
 
 	// AddonEffectivePeriod
 	productcatalog.EffectivePeriod
+
+	// RejectUnitConfig rejects the operation when the target add-on carries a unit_config
+	// conversion. The v1 API cannot represent it, so v1 handlers set this; v3 leaves it false.
+	RejectUnitConfig bool
 }
 
 func (i PublishAddonInput) Validate() error {
@@ -401,6 +411,10 @@ type ArchiveAddonInput struct {
 
 	// EffectiveFrom defines the time from the Addon is going to be unpublished.
 	EffectiveTo time.Time `json:"effectiveTo,omitempty"`
+
+	// RejectUnitConfig rejects the operation when the target add-on carries a unit_config
+	// conversion. The v1 API cannot represent it, so v1 handlers set this; v3 leaves it false.
+	RejectUnitConfig bool
 }
 
 func (i ArchiveAddonInput) Validate() error {

openmeter/productcatalog/addon/service/addon.go

@@ -342,6 +342,10 @@ func (s service) UpdateAddon(ctx context.Context, params addon.UpdateAddonInput)
 			return nil, fmt.Errorf("failed to get add-on: %w", err)
 		}
 
+		if params.RejectUnitConfig && add.AsProductCatalogAddon().HasUnitConfig() {
+			return nil, productcatalog.ErrUnitConfigNotRepresentable
+		}
+
 		// Run validations prior updating add-on.
 		if err = add.AsProductCatalogAddon().ValidateWith(
 			productcatalog.ValidateAddonWithStatus(productcatalog.AddonStatusDraft),
@@ -404,6 +408,10 @@ func (s service) PublishAddon(ctx context.Context, params addon.PublishAddonInpu
 			)
 		}
 
+		if params.RejectUnitConfig && add.AsProductCatalogAddon().HasUnitConfig() {
+			return nil, productcatalog.ErrUnitConfigNotRepresentable
+		}
+
 		pa := add.AsProductCatalogAddon()
 
 		// Run validations prior publishing add-on.
@@ -523,6 +531,10 @@ func (s service) ArchiveAddon(ctx context.Context, params addon.ArchiveAddonInpu
 			)
 		}
 
+		if params.RejectUnitConfig && add.AsProductCatalogAddon().HasUnitConfig() {
+			return nil, productcatalog.ErrUnitConfigNotRepresentable
+		}
+
 		// Run validations prior archiving add-on.
 		if err = add.AsProductCatalogAddon().ValidateWith(
 			productcatalog.ValidateAddonWithStatus(productcatalog.AddonStatusActive),

openmeter/productcatalog/http/mapping.go

@@ -22,15 +22,6 @@ func FromRateCard(r productcatalog.RateCard) (api.RateCard, error) {
 
 	meta := r.AsMeta()
 
-	// The v1 rate card types have no unit_config field. This mapper is the single serialization choke
-	// point for every v1 surface that emits a rate card, so it fails closed here rather than silently
-	// dropping the conversion and misrepresenting billing: reject the object, available via the v3 API.
-	// List surfaces exclude unit_config objects at the query layer so they never reach this mapper;
-	// subscription items are serialized without this mapper, so subscription GET guards them via Spec.HasUnitConfig.
-	if meta.UnitConfig != nil {
-		return resp, productcatalog.ErrUnitConfigNotRepresentable
-	}
-
 	var featureKey *string
 	if meta.FeatureKey != nil {
 		featureKey = meta.FeatureKey

openmeter/productcatalog/http/mapping_test.go

@@ -63,6 +63,15 @@ func (p Plan) ValidateWith(validators ...models.ValidatorFunc[Plan]) error {
 	return models.Validate(p, validators...)
 }
 
+// HasUnitConfig reports whether any rate card in any phase carries a unit_config
+// conversion. The v1 API cannot represent unit_config, so v1 read and mutation
+// surfaces use this to reject such plans instead of silently stripping the field.
+func (p Plan) HasUnitConfig() bool {
+	return lo.SomeBy(p.Phases, func(ph Phase) bool {
+		return ph.RateCards.HasUnitConfig()
+	})
+}
+
 func ValidatePlanMeta() models.ValidatorFunc[Plan] {
 	return func(p Plan) error {
 		return p.PlanMeta.Validate()

openmeter/productcatalog/plan.go

@@ -183,6 +183,7 @@ func (h *handler) UpdatePlan() UpdatePlanHandler {
 			}
 
 			req.IgnoreNonCriticalIssues = true
+			req.RejectUnitConfig = true
 
 			return req, nil
 		},
@@ -331,6 +332,7 @@ func (h *handler) PublishPlan() PublishPlanHandler {
 				EffectivePeriod: productcatalog.EffectivePeriod{
 					EffectiveFrom: lo.ToPtr(clock.Now()),
 				},
+				RejectUnitConfig: true,
 			}
 
 			return req, nil
@@ -373,7 +375,8 @@ func (h *handler) ArchivePlan() ArchivePlanHandler {
 					Namespace: ns,
 					ID:        planID,
 				},
-				EffectiveTo: clock.Now(),
+				EffectiveTo:      clock.Now(),
+				RejectUnitConfig: true,
 			}
 
 			return req, nil
@@ -419,8 +422,9 @@ func (h *handler) NextPlan() NextPlanHandler {
 					Namespace: ns,
 					ID:        idOrKey.ID,
 				},
-				Key:     idOrKey.Key,
-				Version: 0,
+				Key:              idOrKey.Key,
+				Version:          0,
+				RejectUnitConfig: true,
 			}
 
 			return req, nil

openmeter/productcatalog/plan/httpdriver/plan.go

@@ -151,6 +151,9 @@ type inputOptions struct {
 	// ignoreNonCriticalIssues makes Validate() return errors with critical severity or higher.
 	// This allows creating resource with expected validation issues.
 	IgnoreNonCriticalIssues bool
+
+	// RejectUnitConfig makes mutation validation reject a plan that carries a unit_config conversion on any rate card.
+	RejectUnitConfig bool
 }
 
 var _ models.Validator = (*CreatePlanInput)(nil)
@@ -322,6 +325,10 @@ func (i UpdatePlanInput) Validate() error {
 }
 
 func (i UpdatePlanInput) ValidateWithPlan(p productcatalog.Plan) error {
+	if i.RejectUnitConfig && p.HasUnitConfig() {
+		return productcatalog.ErrUnitConfigNotRepresentable
+	}
+
 	var errs []error
 
 	if i.Name != nil {
@@ -423,6 +430,10 @@ type PublishPlanInput struct {
 
 	// EffectivePeriod
 	productcatalog.EffectivePeriod
+
+	// RejectUnitConfig rejects the operation when the target plan carries a unit_config
+	// conversion. The v1 API cannot represent it, so v1 handlers set this; v3 leaves it false.
+	RejectUnitConfig bool
 }
 
 func (i PublishPlanInput) Validate() error {
@@ -471,6 +482,10 @@ type ArchivePlanInput struct {
 
 	// EffectiveFrom defines the time from the Plan is going to be unpublished.
 	EffectiveTo time.Time `json:"effectiveTo,omitempty"`
+
+	// RejectUnitConfig rejects the operation when the target plan carries a unit_config
+	// conversion. The v1 API cannot represent it, so v1 handlers set this; v3 leaves it false.
+	RejectUnitConfig bool
 }
 
 func (i ArchivePlanInput) Validate() error {
@@ -506,6 +521,10 @@ type NextPlanInput struct {
 	// Version is the version of the Plan.
 	// If not set the latest version is assumed.
 	Version int `json:"version,omitempty"`
+
+	// RejectUnitConfig rejects the operation when the target plan carries a unit_config
+	// conversion. The v1 API cannot represent it, so v1 handlers set this; v3 leaves it false.
+	RejectUnitConfig bool
 }
 
 func (i NextPlanInput) Validate() error {