refactor: extend blocking everywhere where we serialize rate cards

Author: rolosp

e2e/v1readsurface_unitconfig_test.go

@@ -9,7 +9,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	api "github.com/openmeterio/openmeter/api/client/go"
-	apiv3 "github.com/openmeterio/openmeter/api/v3"
+	v3sdk "github.com/openmeterio/openmeter/api/v3/client"
 )
 
 const unitConfigNotRepresentableCode = "unit_config_not_representable"
@@ -33,88 +33,86 @@ func TestV1ReadSurfaceExcludesUnitConfig(t *testing.T) {
 	ucPlanKey := "ucread_uc_plan_" + uniq
 	plainPlanKey := "ucread_plain_plan_" + uniq
 
-	var ucPlan *apiv3.BillingPlan
-	var plainPlan *apiv3.BillingPlan
+	var ucPlan *v3sdk.Plan
+	var plainPlan *v3sdk.Plan
 
 	// given:
 	// - a unit_config plan (usage-based rate card, divide-by-1000 ceiling) and a plain plan, both
 	//   authored and published via v3 (v1 cannot author unit_config).
 	runRequired(t, "creates a unit_config plan and a plain plan", func(t *testing.T) {
-		status, meter, problem := c.CreateMeter(apiv3.CreateMeterRequest{
+		meter, err := c.Meters.Create(t.Context(), v3sdk.CreateMeterRequest{
 			Key:           meterKey,
 			Name:          "UC Read Meter " + uniq,
-			Aggregation:   apiv3.MeterAggregationSum,
+			Aggregation:   v3sdk.MeterAggregationSum,
 			EventType:     eventType,
 			ValueProperty: lo.ToPtr("$.value"),
 		})
-		require.Equal(t, http.StatusCreated, status, "problem: %+v", problem)
+		c.requireStatus(http.StatusCreated, err)
 		require.NotNil(t, meter)
 
-		status, feature, problem := c.CreateFeature(apiv3.CreateFeatureRequest{
+		feature, err := c.Features.Create(t.Context(), v3sdk.CreateFeatureRequest{
 			Key:   featureKey,
 			Name:  "UC Read Feature " + uniq,
-			Meter: &apiv3.FeatureMeterReference{Id: meter.Id},
+			Meter: &v3sdk.FeatureMeterReferenceInput{ID: meter.ID},
 		})
-		require.Equal(t, http.StatusCreated, status, "problem: %+v", problem)
+		c.requireStatus(http.StatusCreated, err)
 		require.NotNil(t, feature)
 
-		cadence := apiv3.ISO8601Duration("P1M")
-		term := apiv3.BillingPricePaymentTermInArrears
-		price := apiv3.BillingPrice{}
-		require.NoError(t, price.FromBillingPriceUnit(apiv3.BillingPriceUnit{
-			Type:   apiv3.BillingPriceUnitTypeUnit,
+		cadence := "P1M"
+		term := v3sdk.PricePaymentTermInArrears
+		price := lo.Must(v3sdk.PriceFromPriceUnit(v3sdk.PriceUnit{
 			Amount: "0.10",
 		}))
-		ucRateCard := apiv3.BillingRateCard{
+		ucRateCard := v3sdk.RateCardInput{
 			Key:            feature.Key,
 			Name:           "UC Read Rate Card " + uniq,
 			Price:          price,
 			BillingCadence: &cadence,
 			PaymentTerm:    &term,
-			Feature:        &apiv3.FeatureReference{Id: feature.Id},
-			UnitConfig: &apiv3.BillingUnitConfig{
-				Operation:        apiv3.BillingUnitConfigOperationDivide,
+			Feature:        &v3sdk.FeatureReference{ID: feature.ID},
+			UnitConfig: &v3sdk.UnitConfig{
+				Operation:        v3sdk.UnitConfigOperationDivide,
 				ConversionFactor: "1000",
-				Rounding:         lo.ToPtr(apiv3.BillingUnitConfigRoundingModeCeiling),
-				Precision:        lo.ToPtr(0),
+				Rounding:         lo.ToPtr(v3sdk.UnitConfigRoundingModeCeiling),
+				Precision:        lo.ToPtr(int64(0)),
 			},
 		}
 
-		status, createdUC, problem := c.CreatePlan(apiv3.CreatePlanRequest{
+		createdUC, err := c.Plans.Create(t.Context(), v3sdk.CreatePlanRequest{
 			Key:            ucPlanKey,
 			Name:           "UC Read Plan " + uniq,
 			Currency:       "USD",
-			BillingCadence: apiv3.ISO8601Duration("P1M"),
-			Phases: []apiv3.BillingPlanPhase{{
+			BillingCadence: "P1M",
+			Phases: []v3sdk.PlanPhaseInput{{
 				Key:       "phase_1",
 				Name:      "UC Phase",
-				RateCards: []apiv3.BillingRateCard{ucRateCard},
+				RateCards: []v3sdk.RateCardInput{ucRateCard},
 			}},
 		})
-		require.Equal(t, http.StatusCreated, status, "problem: %+v", problem)
+		c.requireStatus(http.StatusCreated, err)
 		require.NotNil(t, createdUC)
-		status, ucPlan, problem = c.PublishPlan(createdUC.Id)
-		require.Equal(t, http.StatusOK, status, "problem: %+v", problem)
+		ucPlan, err = c.Plans.Publish(t.Context(), createdUC.ID)
+		c.requireStatus(http.StatusOK, err)
 		require.NotNil(t, ucPlan)
 
-		status, createdPlain, problem := c.CreatePlan(apiv3.CreatePlanRequest{
+		createdPlain, err := c.Plans.Create(t.Context(), v3sdk.CreatePlanRequest{
 			Key:            plainPlanKey,
 			Name:           "Plain Read Plan " + uniq,
 			Currency:       "USD",
-			BillingCadence: apiv3.ISO8601Duration("P1M"),
-			Phases:         []apiv3.BillingPlanPhase{validPlanPhase("plain_phase", true /* isLast */)},
+			BillingCadence: "P1M",
+			Phases:         []v3sdk.PlanPhaseInput{validPlanPhase("plain_phase", true /* isLast */)},
 		})
-		require.Equal(t, http.StatusCreated, status, "problem: %+v", problem)
+		c.requireStatus(http.StatusCreated, err)
 		require.NotNil(t, createdPlain)
-		status, plainPlan, problem = c.PublishPlan(createdPlain.Id)
-		require.Equal(t, http.StatusOK, status, "problem: %+v", problem)
+		plainPlan, err = c.Plans.Publish(t.Context(), createdPlain.ID)
+		c.requireStatus(http.StatusOK, err)
 		require.NotNil(t, plainPlan)
 	})
 
 	// then:
 	// - v1 GET on the unit_config plan is rejected with the typed code (not a silently-stripped 200).
 	runRequired(t, "v1 plan GET rejects the unit_config plan", func(t *testing.T) {
-		resp, err := v1.GetPlanWithResponse(t.Context(), ucPlan.Id, nil)
+		resp, err := v1.GetPlanWithResponse(t.Context(), ucPlan.ID, nil)
 		require.NoError(t, err)
 		require.Equal(t, http.StatusBadRequest, resp.StatusCode(), "body: %s", string(resp.Body))
 		require.NotNil(t, resp.ApplicationproblemJSON400)
@@ -124,7 +122,7 @@ func TestV1ReadSurfaceExcludesUnitConfig(t *testing.T) {
 	// and:
 	// - the plain plan is still gettable via v1 (the exclusion is content-derived, not a blanket block).
 	runRequired(t, "v1 plan GET returns the plain plan", func(t *testing.T) {
-		resp, err := v1.GetPlanWithResponse(t.Context(), plainPlan.Id, nil)
+		resp, err := v1.GetPlanWithResponse(t.Context(), plainPlan.ID, nil)
 		require.NoError(t, err)
 		require.Equal(t, http.StatusOK, resp.StatusCode(), "body: %s", string(resp.Body))
 		require.NotNil(t, resp.JSON200)

openmeter/productcatalog/addon/adapter/adapter_test.go

@@ -450,6 +450,78 @@ func TestPostgresAdapter(t *testing.T) {
 	})
 }
 
+func TestListAddonsExcludeUnitConfig(t *testing.T) {
+	ctx := context.Background()
+
+	env := pctestutils.NewTestEnv(t)
+	t.Cleanup(func() { env.Close(t) })
+	env.DBSchemaMigrate(t)
+
+	namespace := pctestutils.NewTestNamespace(t)
+
+	require.NoError(t, env.Meter.ReplaceMeters(ctx, pctestutils.NewTestMeters(t, namespace)),
+		"replacing meters must not fail")
+	meters, err := env.Meter.ListMeters(ctx, meter.ListMetersParams{
+		Page:      pagination.Page{PageSize: 1000, PageNumber: 1},
+		Namespace: namespace,
+	})
+	require.NoError(t, err, "listing meters must not fail")
+	require.NotEmpty(t, meters.Items, "list of meters must not be empty")
+
+	feat, err := env.Feature.CreateFeature(ctx, pctestutils.NewTestFeatureFromMeter(t, &meters.Items[0]))
+	require.NoError(t, err, "creating feature must not fail")
+
+	// Plain add-on: flat rate card, no unit_config → v1-representable.
+	plainInput := pctestutils.NewTestAddon(t, namespace, &productcatalog.FlatFeeRateCard{
+		RateCardMeta: productcatalog.RateCardMeta{Key: "flat", Name: "Flat"},
+	})
+	plainInput.Addon.Key = "plain"
+	_, err = env.AddonRepository.CreateAddon(ctx, plainInput)
+	require.NoError(t, err, "creating plain add-on must not fail")
+
+	// unit_config add-on: usage-based rate card carrying a unit_config → not v1-representable.
+	ucInput := pctestutils.NewTestAddon(t, namespace, &productcatalog.UsageBasedRateCard{
+		RateCardMeta: productcatalog.RateCardMeta{
+			Key:        feat.Key,
+			Name:       "UC RateCard",
+			FeatureKey: lo.ToPtr(feat.Key),
+			FeatureID:  lo.ToPtr(feat.ID),
+			Price:      productcatalog.NewPriceFrom(productcatalog.UnitPrice{Amount: decimal.NewFromInt(1)}),
+			UnitConfig: &productcatalog.UnitConfig{
+				Operation:        productcatalog.UnitConfigOperationDivide,
+				ConversionFactor: decimal.NewFromInt(1000),
+			},
+		},
+		BillingCadence: MonthPeriod,
+	})
+	ucInput.Addon.Key = "with-uc"
+	_, err = env.AddonRepository.CreateAddon(ctx, ucInput)
+	require.NoError(t, err, "creating unit_config add-on must not fail")
+
+	t.Run("included when ExcludeUnitConfig is false", func(t *testing.T) {
+		list, err := env.AddonRepository.ListAddons(ctx, addon.ListAddonsInput{
+			Namespaces: []string{namespace},
+		})
+		require.NoError(t, err, "listing add-ons must not fail")
+
+		keys := lo.Map(list.Items, func(a addon.Addon, _ int) string { return a.Key })
+		require.ElementsMatch(t, []string{"plain", "with-uc"}, keys)
+		require.Equal(t, 2, list.TotalCount, "TotalCount must count both add-ons")
+	})
+
+	t.Run("excluded when ExcludeUnitConfig is true, TotalCount stays consistent", func(t *testing.T) {
+		list, err := env.AddonRepository.ListAddons(ctx, addon.ListAddonsInput{
+			Namespaces:        []string{namespace},
+			ExcludeUnitConfig: true,
+		})
+		require.NoError(t, err, "listing add-ons must not fail")
+
+		keys := lo.Map(list.Items, func(a addon.Addon, _ int) string { return a.Key })
+		require.ElementsMatch(t, []string{"plain"}, keys)
+		require.Equal(t, 1, list.TotalCount, "TotalCount must exclude the unit_config add-on, not just the page slice")
+	})
+}
+
 // TestFromPlanRateCardRowMapsUnitConfig guards the cross-package mapper used when an
 // add-on is loaded with expanded linked plans
 // (FromAddonRow → FromPlanAddonRow → FromPlanRow → FromPlanPhaseRow → FromPlanRateCardRow).

openmeter/productcatalog/addon/adapter/addon.go

@@ -50,6 +50,13 @@ func (a *adapter) ListAddons(ctx context.Context, params addon.ListAddonsInput)
 		query = filter.ApplyToQuery(query, params.Name, addondb.FieldName)
 		query = filter.ApplyToQuery(query, params.Currency, addondb.FieldCurrency)
 
+		if params.ExcludeUnitConfig {
+			query = query.Where(addondb.Not(addondb.HasRatecardsWith(
+				addonratecarddb.UnitConfigNotNil(),
+				addonratecarddb.DeletedAtIsNil(),
+			)))
+		}
+
 		if !params.IncludeDeleted {
 			query = query.Where(addondb.DeletedAtIsNil())
 		}

openmeter/productcatalog/addon/httpdriver/addon.go

@@ -56,6 +56,8 @@ func (h *handler) ListAddons() ListAddonsHandler {
 				KeyVersions:    lo.FromPtr(params.KeyVersion),
 				IncludeDeleted: lo.FromPtr(params.IncludeDeleted),
 				Status:         statusFilter,
+				// The v1 API cannot represent unit_config; exclude such add-ons from the v1 list at the query layer.
+				ExcludeUnitConfig: true,
 			}
 
 			if params.Id != nil {

openmeter/productcatalog/addon/service.go

@@ -95,6 +95,9 @@ type ListAddonsInput struct {
 	Key      *filter.FilterString
 	Name     *filter.FilterString
 	Currency *filter.FilterString
+
+	// ExcludeUnitConfig omits add-ons carrying a unit_config conversion on any of their rate cards.
+	ExcludeUnitConfig bool
 }
 
 func (i ListAddonsInput) Validate() error {

openmeter/productcatalog/http/mapping.go

@@ -22,6 +22,15 @@ 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
@@ -923,7 +932,7 @@ func FromAnnotations(annotations models.Annotations) *api.Annotations {
 		return nil
 	}
 
-	return lo.ToPtr((api.Annotations)(annotations))
+	return lo.ToPtr(api.Annotations(annotations))
 }
 
 func FromValidationErrors(issues models.ValidationIssues) *[]api.ValidationError {

openmeter/productcatalog/http/mapping_test.go

@@ -0,0 +1,59 @@
+package http
+
+import (
+	"testing"
+
+	decimal "github.com/alpacahq/alpacadecimal"
+	"github.com/samber/lo"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/openmeterio/openmeter/openmeter/productcatalog"
+	"github.com/openmeterio/openmeter/pkg/datetime"
+	"github.com/openmeterio/openmeter/pkg/models"
+)
+
+// FromRateCard is the single v1 serialization choke point for rate cards. It must fail closed on a
+// unit_config-bearing card (the v1 types cannot represent it) so no v1 surface silently strips the
+// conversion, while a plain card still serializes normally.
+func TestFromRateCardRejectsUnitConfig(t *testing.T) {
+	cadence := datetime.NewISODuration(0, 1, 0, 0, 0, 0, 0)
+
+	t.Run("unit_config card is rejected with the typed code", func(t *testing.T) {
+		rc := &productcatalog.UsageBasedRateCard{
+			RateCardMeta: productcatalog.RateCardMeta{
+				Key:        "feat-1",
+				Name:       "Feature 1",
+				FeatureKey: lo.ToPtr("feat-1"),
+				Price:      productcatalog.NewPriceFrom(productcatalog.UnitPrice{Amount: decimal.NewFromInt(1)}),
+				UnitConfig: &productcatalog.UnitConfig{
+					Operation:        productcatalog.UnitConfigOperationDivide,
+					ConversionFactor: decimal.NewFromInt(1000),
+				},
+			},
+			BillingCadence: cadence,
+		}
+
+		_, err := FromRateCard(rc)
+		require.Error(t, err)
+
+		issues, convErr := models.AsValidationIssues(err)
+		require.NoError(t, convErr)
+		require.Len(t, issues, 1)
+		assert.Equal(t, productcatalog.ErrCodeUnitConfigNotRepresentable, issues[0].Code())
+	})
+
+	t.Run("plain card serializes without error", func(t *testing.T) {
+		rc := &productcatalog.FlatFeeRateCard{
+			RateCardMeta: productcatalog.RateCardMeta{
+				Key:   "flat-1",
+				Name:  "Flat 1",
+				Price: productcatalog.NewPriceFrom(productcatalog.FlatPrice{Amount: decimal.NewFromInt(10), PaymentTerm: productcatalog.InArrearsPaymentTerm}),
+			},
+			BillingCadence: &cadence,
+		}
+
+		_, err := FromRateCard(rc)
+		require.NoError(t, err)
+	})
+}