fix(productcatalog): rate card validation#4268
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two validation errors and enforces that non-flat (usage-based) rate cards must supply a ChangesUsage-based rate-card validation & wiring
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant API as ProductCatalog API
participant DB as Datastore
participant FeatureSvc as Feature store (via API/v1)
Test->>FeatureSvc: Create metered Feature (meter slug)
FeatureSvc-->>Test: Return FeatureID, FeatureKey
Test->>API: Submit RateCard (usage-based) with FeatureKey/FeatureID
API->>DB: Persist RateCard + meta
API->>DB: Load related Feature (resolveFeatures)
DB-->>API: Return Feature (includes MeterID)
alt Feature.MeterID != nil
API-->>Test: Accept RateCard
else Feature.MeterID == nil
API-->>Test: Return validation error (requires metered feature)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
chrisgacsal
previously approved these changes
May 1, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
e2e/addons_v3_test.go (2)
288-325: ⚡ Quick winAdd one negative case for the new validation.
This subtest only proves the metered happy path. A quick failure case for a missing or non-metered feature would make the new rule much harder to regress.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/addons_v3_test.go` around lines 288 - 325, Add a negative subtest to TestV3AddonInstanceTypePriceCompatibility that exercises the new validation by providing an incompatible combination (e.g., instanceType = apiv3.AddonInstanceTypeSingle with a flat rate card, or apiv3.AddonInstanceTypeMultiple with a unit rate card that references a non-metered/missing feature) and expect http.StatusBadRequest; add the case to the cases slice using the existing helper functions (validFlatRateCard, validUnitRateCard, createTestFeature as needed) and update the assertions after c.CreateAddon(body) to check tc.expectedStatus and assert addon is nil when the status is not http.StatusCreated (and non-nil when it is).
155-185: ⚡ Quick winAssert the feature refs on the round-trip too.
This test now pays the setup cost to create metered features, but it never checks that
Feature.Idsurvives create/publish/get. A regression that accepts the request and drops the reference would still pass here.♻️ Tiny follow-up
gotUnit, ok := byKey[unit.Key] require.True(t, ok, "unit rate card missing on round-trip") +require.NotNil(t, gotUnit.Feature) +assert.Equal(t, unitFeatureID, gotUnit.Feature.Id) unitPrice, err := gotUnit.Price.AsBillingPriceUnit() require.NoError(t, err, "unit price should decode as BillingPriceUnit") @@ gotGraduated, ok := byKey[graduated.Key] require.True(t, ok, "graduated rate card missing on round-trip") +require.NotNil(t, gotGraduated.Feature) +assert.Equal(t, graduatedFeatureID, gotGraduated.Feature.Id) gradPrice, err := gotGraduated.Price.AsBillingPriceGraduated() require.NoError(t, err, "graduated price should decode as BillingPriceGraduated")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/addons_v3_test.go` around lines 155 - 185, The test currently verifies RateCards round-trip but doesn't assert that the metered feature references survive create/publish/get; update the assertions after retrieving got (from c.GetAddon(addon.Id)) to verify each returned rate card still references the original feature IDs: for gotFlat, gotUnit, and gotGraduated assert that the rate card's feature reference field (e.g. Feature.Id or FeatureRef.Id on apiv3.BillingRateCard) equals the original feature id variables used when creating the rate cards (the variables named flat, unit, graduated or their associated feature ids), so any regression that drops the Feature.Id will fail the test. Ensure you use the exact field name on apiv3.BillingRateCard (Feature or FeatureRef) present in the codebase when adding these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/addons_v3_test.go`:
- Around line 288-325: Add a negative subtest to
TestV3AddonInstanceTypePriceCompatibility that exercises the new validation by
providing an incompatible combination (e.g., instanceType =
apiv3.AddonInstanceTypeSingle with a flat rate card, or
apiv3.AddonInstanceTypeMultiple with a unit rate card that references a
non-metered/missing feature) and expect http.StatusBadRequest; add the case to
the cases slice using the existing helper functions (validFlatRateCard,
validUnitRateCard, createTestFeature as needed) and update the assertions after
c.CreateAddon(body) to check tc.expectedStatus and assert addon is nil when the
status is not http.StatusCreated (and non-nil when it is).
- Around line 155-185: The test currently verifies RateCards round-trip but
doesn't assert that the metered feature references survive create/publish/get;
update the assertions after retrieving got (from c.GetAddon(addon.Id)) to verify
each returned rate card still references the original feature IDs: for gotFlat,
gotUnit, and gotGraduated assert that the rate card's feature reference field
(e.g. Feature.Id or FeatureRef.Id on apiv3.BillingRateCard) equals the original
feature id variables used when creating the rate cards (the variables named
flat, unit, graduated or their associated feature ids), so any regression that
drops the Feature.Id will fail the test. Ensure you use the exact field name on
apiv3.BillingRateCard (Feature or FeatureRef) present in the codebase when
adding these assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 868fb748-f743-4a73-83a0-e7b427c46993
📒 Files selected for processing (9)
e2e/addons_v3_test.goe2e/config.yamle2e/v3helpers_test.goopenmeter/productcatalog/addon/service/addon.goopenmeter/productcatalog/errors.goopenmeter/productcatalog/plan/adapter/adapter_test.goopenmeter/productcatalog/plan/service/plan.goopenmeter/subscription/addon/diff/restore_test.goopenmeter/subscription/workflow/service/addon_test.go
✅ Files skipped from review due to trivial changes (1)
- e2e/config.yaml
chrisgacsal
previously approved these changes
May 2, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openmeter/subscription/repo/mapping.go (1)
131-137: 💤 Low valueTiny optional: consider a local copy instead of
&item.Key
featureKey = &item.Keyis GC-safe sinceitemis heap-allocated and the GC will keep it alive. That said, a local copy makes it clearer that the returned struct doesn't implicitly hold a reference to the DB entity:✨ Optional tweak
- if featureKey == nil && item.Price != nil && item.Price.Type() != productcatalog.FlatPriceType { - featureKey = &item.Key - } + if featureKey == nil && item.Price != nil && item.Price.Type() != productcatalog.FlatPriceType { + key := item.Key + featureKey = &key + }Totally fine to leave as-is though — this is purely a readability/intent thing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/subscription/repo/mapping.go` around lines 131 - 137, The code currently assigns featureKey = &item.Key which works but can ambiguously tie the returned pointer to the DB entity; instead create a local copy of item.Key and assign the pointer to that local (e.g. copyKey := item.Key; featureKey = ©Key) when setting featureKey inside the conditional that checks item.Price and productcatalog.FlatPriceType (referencing featureKey, item.Key, item.Price, and productcatalog.FlatPriceType).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/e2e_test.go`:
- Around line 1563-1565: Replace context.Background() with t.Context() in the
ListFeaturesWithResponse call (the call to ListFeaturesWithResponse) so the
request is tied to the test lifecycle; also in the "Slow: Reset testing" subtest
update the Get/List call that currently passes nil params to instead pass a
filtered params object (e.g., MeterSlug with the meterSlug slice used above) and
assert on the filtered results rather than require.Len(t, features, 1) so the
test is deterministic even when other features exist.
---
Nitpick comments:
In `@openmeter/subscription/repo/mapping.go`:
- Around line 131-137: The code currently assigns featureKey = &item.Key which
works but can ambiguously tie the returned pointer to the DB entity; instead
create a local copy of item.Key and assign the pointer to that local (e.g.
copyKey := item.Key; featureKey = ©Key) when setting featureKey inside the
conditional that checks item.Price and productcatalog.FlatPriceType (referencing
featureKey, item.Key, item.Price, and productcatalog.FlatPriceType).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a91e78bd-f635-435e-836f-0d59f1ffb45a
📒 Files selected for processing (3)
e2e/e2e_test.goopenmeter/productcatalog/plan/adapter/mapping.goopenmeter/subscription/repo/mapping.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: András Tóth <4157749+tothandras@users.noreply.github.com>
chrisgacsal
approved these changes
May 4, 2026
chrisgacsal
approved these changes
May 4, 2026
chrisgacsal
added
the
release-note/ignore
This was referenced May 4, 2026
This was referenced May 11, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Tests