Skip to content

Commit 7e28ebf

Browse files
rolosprolosp
committed
fix: review comment fixes
1 parent beab7d4 commit 7e28ebf

12 files changed

Lines changed: 29 additions & 47 deletions

File tree

‎e2e/v1readsurface_unitconfig_test.go‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -216,16 +216,4 @@ func TestV1ReadSurfaceExcludesUnitConfig(t *testing.T) {
216216
require.NotNil(t, resp.ApplicationproblemJSON400)
217217
assert.Contains(t, string(resp.Body), unitConfigNotRepresentableCode, "the 400 must carry the typed unit_config code")
218218
})
219-
220-
// and:
221-
// - v1 add-on read surfaces of that subscription are rejected too (the sub-addon list serializes
222-
// the subscription's items, so it must not silently strip). Guarded on the subscription's spec,
223-
// so it rejects even with no add-ons attached.
224-
runRequired(t, "v1 subscription add-on LIST rejects the unit_config subscription", func(t *testing.T) {
225-
require.NotEmpty(t, subscriptionID)
226-
resp, err := v1.ListSubscriptionAddonsWithResponse(t.Context(), subscriptionID)
227-
require.NoError(t, err)
228-
require.Equal(t, http.StatusBadRequest, resp.StatusCode(), "body: %s", string(resp.Body))
229-
assert.Contains(t, string(resp.Body), unitConfigNotRepresentableCode, "the 400 must carry the typed unit_config code")
230-
})
231219
}

‎openmeter/productcatalog/addon/service.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -148,12 +148,6 @@ type inputOptions struct {
148148
// ignoreNonCriticalIssues makes Validate() return errors with critical severity or higher.
149149
// This allows creating resource with expected validation issues.
150150
IgnoreNonCriticalIssues bool
151-
152-
// RejectUnitConfig makes mutation validation reject an add-on that carries a unit_config
153-
// conversion on any rate card. The v1 API cannot represent unit_config, and v1 update
154-
// rewrites rate cards from a body that has no such field, so proceeding would silently
155-
// drop the conversion. v1 handlers set this; v3 leaves it false.
156-
RejectUnitConfig bool
157151
}
158152

159153
var _ models.Validator = (*CreateAddonInput)(nil)
@@ -217,6 +211,12 @@ type UpdateAddonInput struct {
217211
// RateCards
218212
RateCards *productcatalog.RateCards `json:"rateCards,omitempty"`
219213

214+
// RejectUnitConfig makes mutation validation reject an add-on that carries a unit_config
215+
// conversion on any rate card. The v1 API cannot represent unit_config, and v1 update
216+
// rewrites rate cards from a body that has no such field, so proceeding would silently
217+
// drop the conversion. v1 handlers set this; v3 leaves it false.
218+
RejectUnitConfig bool
219+
220220
inputOptions
221221
}
222222

‎openmeter/productcatalog/addon/service/addon.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -460,7 +460,8 @@ func (s service) PublishAddon(ctx context.Context, params addon.PublishAddonInpu
460460
Namespace: activeAddon.Namespace,
461461
ID: activeAddon.ID,
462462
},
463-
EffectiveTo: lo.FromPtr(params.EffectiveFrom),
463+
EffectiveTo: lo.FromPtr(params.EffectiveFrom),
464+
RejectUnitConfig: params.RejectUnitConfig,
464465
})
465466
if err != nil {
466467
return nil, fmt.Errorf("failed to archive add-on with active status: %w", err)

‎openmeter/productcatalog/plan/service.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -151,9 +151,6 @@ type inputOptions struct {
151151
// ignoreNonCriticalIssues makes Validate() return errors with critical severity or higher.
152152
// This allows creating resource with expected validation issues.
153153
IgnoreNonCriticalIssues bool
154-
155-
// RejectUnitConfig makes mutation validation reject a plan that carries a unit_config conversion on any rate card.
156-
RejectUnitConfig bool
157154
}
158155

159156
var _ models.Validator = (*CreatePlanInput)(nil)
@@ -220,6 +217,9 @@ type UpdatePlanInput struct {
220217
// Phases
221218
Phases *[]productcatalog.Phase `json:"phases"`
222219

220+
// RejectUnitConfig makes mutation validation reject a plan that carries a unit_config conversion on any rate card.
221+
RejectUnitConfig bool
222+
223223
inputOptions
224224
}
225225

‎openmeter/productcatalog/plan/service/plan.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -494,7 +494,8 @@ func (s service) PublishPlan(ctx context.Context, params plan.PublishPlanInput)
494494
Namespace: activePlan.Namespace,
495495
ID: activePlan.ID,
496496
},
497-
EffectiveTo: lo.FromPtr(params.EffectiveFrom),
497+
EffectiveTo: lo.FromPtr(params.EffectiveFrom),
498+
RejectUnitConfig: params.RejectUnitConfig,
498499
})
499500
if err != nil {
500501
return nil, fmt.Errorf("failed to archive plan with active status: %w", err)

‎openmeter/productcatalog/plan/service_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ func TestUpdatePlanInputValidateWithPlanRejectsUnitConfig(t *testing.T) {
5050
t.Run("flag on + stored plan has unit_config is rejected even though the update body carries none", func(t *testing.T) {
5151
// The input intentionally carries no phases (a v1 body cannot express unit_config); the
5252
// rejection must come from the stored plan, proving the check runs before the merge.
53-
in := UpdatePlanInput{inputOptions: inputOptions{RejectUnitConfig: true}}
53+
in := UpdatePlanInput{RejectUnitConfig: true}
5454

5555
err := in.ValidateWithPlan(planWith(card(divide)))
5656
require.Error(t, err)
@@ -64,7 +64,7 @@ func TestUpdatePlanInputValidateWithPlanRejectsUnitConfig(t *testing.T) {
6464
})
6565

6666
t.Run("flag on does not reject a plan without unit_config", func(t *testing.T) {
67-
in := UpdatePlanInput{inputOptions: inputOptions{RejectUnitConfig: true}}
67+
in := UpdatePlanInput{RejectUnitConfig: true}
6868

6969
assert.False(t, rejectedForUnitConfig(in.ValidateWithPlan(planWith(card(nil)))))
7070
})

‎openmeter/server/router/router.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -498,6 +498,7 @@ func NewRouter(config Config) (*Router, error) {
498498
SubscriptionAddonService: config.SubscriptionAddonService,
499499
SubscriptionWorkflowService: config.SubscriptionWorkflowService,
500500
SubscriptionService: config.SubscriptionService,
501+
AddonService: config.Addon,
501502
NamespaceDecoder: config.NamespaceDecoder,
502503
Logger: config.Logger,
503504
},

‎openmeter/subscription/addon/http/create.go‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99

1010
"github.com/openmeterio/openmeter/api"
1111
"github.com/openmeterio/openmeter/openmeter/productcatalog"
12+
"github.com/openmeterio/openmeter/openmeter/productcatalog/addon"
1213
"github.com/openmeterio/openmeter/openmeter/subscription"
1314
subscriptionaddon "github.com/openmeterio/openmeter/openmeter/subscription/addon"
1415
subscriptionworkflow "github.com/openmeterio/openmeter/openmeter/subscription/workflow"
@@ -59,12 +60,20 @@ func (h *handler) CreateSubscriptionAddon() CreateSubscriptionAddonHandler {
5960
func(ctx context.Context, req CreateSubscriptionAddonRequest) (CreateSubscriptionAddonResponse, error) {
6061
var def CreateSubscriptionAddonResponse
6162

62-
currentView, err := h.SubscriptionService.GetView(ctx, req.SubscriptionID)
63+
// The add-on's own rate cards are serialized in the response, so reject a unit_config
64+
// add-on (the v1 shape cannot represent it) before it lands on the subscription. We do
65+
// NOT reject based on the subscription's plan
66+
addonToAdd, err := h.AddonService.GetAddon(ctx, addon.GetAddonInput{
67+
NamespacedID: models.NamespacedID{
68+
Namespace: req.SubscriptionID.Namespace,
69+
ID: req.AddonInput.AddonID,
70+
},
71+
})
6372
if err != nil {
6473
return def, err
6574
}
6675

67-
if currentView.Spec.HasUnitConfig() {
76+
if addonToAdd.AsProductCatalogAddon().HasUnitConfig() {
6877
return def, productcatalog.ErrUnitConfigNotRepresentable
6978
}
7079

‎openmeter/subscription/addon/http/get.go‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import (
66
"net/http"
77

88
"github.com/openmeterio/openmeter/api"
9-
"github.com/openmeterio/openmeter/openmeter/productcatalog"
109
subscriptionaddon "github.com/openmeterio/openmeter/openmeter/subscription/addon"
1110
"github.com/openmeterio/openmeter/pkg/framework/commonhttp"
1211
"github.com/openmeterio/openmeter/pkg/framework/transport/httptransport"
@@ -52,10 +51,6 @@ func (h *handler) GetSubscriptionAddon() GetSubscriptionAddonHandler {
5251
return GetSubscriptionAddonResponse{}, err
5352
}
5453

55-
if view.Spec.HasUnitConfig() {
56-
return GetSubscriptionAddonResponse{}, productcatalog.ErrUnitConfigNotRepresentable
57-
}
58-
5954
return MapSubscriptionAddonToResponse(view, *res)
6055
},
6156
commonhttp.JSONResponseEncoderWithStatus[GetSubscriptionAddonResponse](http.StatusOK),

‎openmeter/subscription/addon/http/handler.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"net/http"
88

99
"github.com/openmeterio/openmeter/openmeter/namespace/namespacedriver"
10+
"github.com/openmeterio/openmeter/openmeter/productcatalog/addon"
1011
"github.com/openmeterio/openmeter/openmeter/subscription"
1112
subscriptionaddon "github.com/openmeterio/openmeter/openmeter/subscription/addon"
1213
subscriptionworkflow "github.com/openmeterio/openmeter/openmeter/subscription/workflow"
@@ -25,6 +26,7 @@ type HandlerConfig struct {
2526
SubscriptionAddonService subscriptionaddon.Service
2627
SubscriptionWorkflowService subscriptionworkflow.Service
2728
SubscriptionService subscription.Service
29+
AddonService addon.Service
2830
NamespaceDecoder namespacedriver.NamespaceDecoder
2931
Logger *slog.Logger
3032
}

0 commit comments

Comments
 (0)