feat: don't allow listing or getting unit config equipped plans on v1 - OM-409#4679
Merged
Conversation
rolosp
changed the title
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 typed ChangesUnit config detection and validation
Plan filtering and mutation guards
Add-on filtering and mutation guards
Subscription and plan add-on safeguards
End-to-end coverage
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant V3Catalog
participant V1PlanAPI
participant V1SubscriptionAPI
participant V1AddonAPI
V3Catalog->>V1PlanAPI: create configured and plain plans
V1PlanAPI-->>V3Catalog: reject configured reads and mutations
V3Catalog->>V1SubscriptionAPI: create subscription by configured plan key
V1SubscriptionAPI-->>V3Catalog: reject subscription read
V3Catalog->>V1AddonAPI: list subscription add-ons
V1AddonAPI-->>V3Catalog: reject configured subscription surface
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
|
should we extend this boundary to every v1 response that serializes rate cards? add-on GET/LIST, mutations, etc... can return an object with |
rolosp
force-pushed
the
feat/om-409-exclude-unit-config-from-v1
branch
from
July 10, 2026 14:46
2323906 to
a26e0d2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/productcatalog/addon/adapter/adapter_test.go`:
- Around line 453-454: In TestListAddonsExcludeUnitConfig, replace
context.Background() with t.Context() so the test context follows the test
lifecycle.
🪄 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: 64c97b9f-bb11-42a2-84ba-ab78f8c2e064
📒 Files selected for processing (19)
e2e/v1readsurface_unitconfig_test.goopenmeter/productcatalog/addon/adapter/adapter_test.goopenmeter/productcatalog/addon/adapter/addon.goopenmeter/productcatalog/addon/httpdriver/addon.goopenmeter/productcatalog/addon/service.goopenmeter/productcatalog/errors.goopenmeter/productcatalog/http/mapping.goopenmeter/productcatalog/http/mapping_test.goopenmeter/productcatalog/plan/adapter/adapter_test.goopenmeter/productcatalog/plan/adapter/plan.goopenmeter/productcatalog/plan/httpdriver/plan.goopenmeter/productcatalog/plan/plan.goopenmeter/productcatalog/plan/plan_test.goopenmeter/productcatalog/plan/service.goopenmeter/productcatalog/ratecard.goopenmeter/productcatalog/ratecard_test.goopenmeter/productcatalog/subscription/http/get.goopenmeter/subscription/subscriptionspec.goopenmeter/subscription/subscriptionspec_test.go
✅ Files skipped from review due to trivial changes (3)
- openmeter/productcatalog/addon/service.go
- openmeter/productcatalog/errors.go
- openmeter/subscription/subscriptionspec_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- openmeter/productcatalog/subscription/http/get.go
- openmeter/productcatalog/plan/plan.go
- openmeter/subscription/subscriptionspec.go
- openmeter/productcatalog/plan/adapter/plan.go
- openmeter/productcatalog/ratecard.go
- openmeter/productcatalog/plan/plan_test.go
- openmeter/productcatalog/plan/httpdriver/plan.go
- openmeter/productcatalog/plan/service.go
- e2e/v1readsurface_unitconfig_test.go
- openmeter/productcatalog/plan/adapter/adapter_test.go
- openmeter/productcatalog/ratecard_test.go
Comment on lines
+453
to
+454
| func TestListAddonsExcludeUnitConfig(t *testing.T) { | ||
| ctx := context.Background() |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test file around the cited lines and nearby context.
FILE="openmeter/productcatalog/addon/adapter/adapter_test.go"
wc -l "$FILE"
sed -n '430,480p' "$FILE"
# Check for t.Context() usage in the same file and surrounding tests.
rg -n 't\.Context\(\)|context\.Background\(\)|context\.TODO\(\)' "$FILE" openmeter/productcatalog/addon/adapter -g '*_test.go'Repository: openmeterio/openmeter
Length of output: 2117
Use t.Context() here
It matches the test lifecycle better than context.Background().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openmeter/productcatalog/addon/adapter/adapter_test.go` around lines 453 -
454, In TestListAddonsExcludeUnitConfig, replace context.Background() with
t.Context() so the test context follows the test lifecycle.
Source: Coding guidelines
rolosp
force-pushed
the
feat/om-409-exclude-unit-config-from-v1
branch
2 times, most recently
from
July 13, 2026 19:07
8b22826 to
5e0a664
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/productcatalog/addon/service.go`:
- Around line 411-413: Update the nested ArchiveAddon request in the v1 publish
flow to pass RejectUnitConfig from the incoming params, ensuring version
transitions reject an active addon with unit_config when requested. Add a
regression test covering a representable new version replacing an active
configured version.
In `@openmeter/productcatalog/addon/service/addon.go`:
- Around line 345-348: Extend the validation around RejectUnitConfig in the
add-on update flow to inspect incoming params.RateCards for unit_config, not
only add.AsProductCatalogAddon().HasUnitConfig(). Reject the update with
productcatalog.ErrUnitConfigNotRepresentable when either the existing add-on or
the resolved incoming/merged rate-card candidate contains unit_config, before
calling UpdateAddon.
In `@openmeter/productcatalog/plan/service.go`:
- Around line 154-156: Update CreatePlanInput.Validate to honor the embedded
RejectUnitConfig flag by adding the same unit_config conversion rejection used
during mutation validation to its existing error aggregation. If create
operations are intentionally unsupported, remove the flag from CreatePlanInput
instead.
In `@openmeter/subscription/addon/http/create.go`:
- Around line 62-70: Extend the addon creation validation around
currentView.Spec.HasUnitConfig() to also load the addon identified by
req.AddonInput.AddonID and reject it when its rate card has UnitConfig. Perform
this addon-side check before the addon merge or creation path, returning
productcatalog.ErrUnitConfigNotRepresentable, while preserving the existing
subscription check.
🪄 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: 092ff0bc-a204-4b58-ad2e-81804b38bc85
📒 Files selected for processing (36)
e2e/v1readsurface_unitconfig_test.goopenmeter/productcatalog/addon.goopenmeter/productcatalog/addon/adapter/adapter_test.goopenmeter/productcatalog/addon/adapter/addon.goopenmeter/productcatalog/addon/httpdriver/addon.goopenmeter/productcatalog/addon/service.goopenmeter/productcatalog/addon/service/addon.goopenmeter/productcatalog/errors.goopenmeter/productcatalog/http/mapping.goopenmeter/productcatalog/plan.goopenmeter/productcatalog/plan/adapter/adapter_test.goopenmeter/productcatalog/plan/adapter/plan.goopenmeter/productcatalog/plan/httpdriver/plan.goopenmeter/productcatalog/plan/plan.goopenmeter/productcatalog/plan/plan_test.goopenmeter/productcatalog/plan/service.goopenmeter/productcatalog/plan/service/plan.goopenmeter/productcatalog/plan/service_test.goopenmeter/productcatalog/plan_test.goopenmeter/productcatalog/planaddon/httpdriver/planaddon.goopenmeter/productcatalog/planaddon/service.goopenmeter/productcatalog/planaddon/service/planaddon.goopenmeter/productcatalog/ratecard.goopenmeter/productcatalog/ratecard_test.goopenmeter/productcatalog/subscription/http/change.goopenmeter/productcatalog/subscription/http/get.goopenmeter/productcatalog/subscription/http/migrate.goopenmeter/productcatalog/subscription/service.goopenmeter/productcatalog/subscription/service/change.goopenmeter/productcatalog/subscription/service/migrate.goopenmeter/subscription/addon/http/create.goopenmeter/subscription/addon/http/get.goopenmeter/subscription/addon/http/list.goopenmeter/subscription/addon/http/update.goopenmeter/subscription/subscriptionspec.goopenmeter/subscription/subscriptionspec_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- openmeter/productcatalog/plan/plan_test.go
- openmeter/productcatalog/plan/plan.go
- openmeter/subscription/subscriptionspec_test.go
- openmeter/productcatalog/plan/adapter/plan.go
- openmeter/productcatalog/errors.go
- openmeter/productcatalog/plan/adapter/adapter_test.go
- openmeter/productcatalog/subscription/http/get.go
- openmeter/productcatalog/ratecard.go
- openmeter/subscription/subscriptionspec.go
- openmeter/productcatalog/addon/adapter/adapter_test.go
- openmeter/productcatalog/addon/adapter/addon.go
Comment on lines
411
to
413
|
|
||
| // EffectiveFrom defines the time from the Addon is going to be unpublished. | ||
| EffectiveTo time.Time `json:"effectiveTo,omitempty"` |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Propagate RejectUnitConfig when archiving the previous version.
The v1 publish handler sets this flag, but the nested ArchiveAddon call at Lines 458-464 omits it. If the new version is representable but the currently active version has unit_config, v1 publish still archives that configured resource instead of rejecting the mutation.
Pass RejectUnitConfig: params.RejectUnitConfig to the nested request and add a regression test for this version-transition case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openmeter/productcatalog/addon/service.go` around lines 411 - 413, Update the
nested ArchiveAddon request in the v1 publish flow to pass RejectUnitConfig from
the incoming params, ensuring version transitions reject an active addon with
unit_config when requested. Add a regression test covering a representable new
version replacing an active configured version.
Comment on lines
+345
to
+348
| if params.RejectUnitConfig && add.AsProductCatalogAddon().HasUnitConfig() { | ||
| return nil, productcatalog.ErrUnitConfigNotRepresentable | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject unit_config in the incoming update payload too.
This check only inspects the existing add-on. A v1 caller can update a clean add-on with params.RateCards containing unit_config; those cards are resolved and then passed to UpdateAddon, allowing v1 to create a resource it cannot represent. Check the incoming rate cards as well, or validate the merged candidate before persisting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openmeter/productcatalog/addon/service/addon.go` around lines 345 - 348,
Extend the validation around RejectUnitConfig in the add-on update flow to
inspect incoming params.RateCards for unit_config, not only
add.AsProductCatalogAddon().HasUnitConfig(). Reject the update with
productcatalog.ErrUnitConfigNotRepresentable when either the existing add-on or
the resolved incoming/merged rate-card candidate contains unit_config, before
calling UpdateAddon.
rolosp
force-pushed
the
feat/om-409-exclude-unit-config-from-v1
branch
from
July 14, 2026 10:43
5e0a664 to
38c4a3c
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/productcatalog/plan/plan.go (1)
50-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: Prefer standard-library
slices.ContainsFunc.This logic works perfectly! However, since the coding guidelines prefer standard-library
slicesandslices.ContainsFuncis already used for the equivalent check inRateCards.HasUnitConfig(), you might want to use it here for consistency. Feel free to ignore if you preferlo.SomeBy.As per coding guidelines: Prefer standard-library
slicesandmaps, orgithub.com/samber/lowhere clearer.💡 Proposed refactor
-func (p Plan) HasUnitConfig() bool { - return lo.SomeBy(p.Phases, func(ph Phase) bool { - return ph.RateCards.HasUnitConfig() - }) -} +import "slices" // Make sure to include this in the imports block + +func (p Plan) HasUnitConfig() bool { + return slices.ContainsFunc(p.Phases, func(ph Phase) bool { + return ph.RateCards.HasUnitConfig() + }) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/productcatalog/plan/plan.go` around lines 50 - 55, Optionally update Plan.HasUnitConfig to use standard-library slices.ContainsFunc instead of lo.SomeBy, matching the existing RateCards.HasUnitConfig implementation and project guidelines while preserving the current boolean behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openmeter/productcatalog/plan/plan.go`:
- Around line 50-55: Optionally update Plan.HasUnitConfig to use
standard-library slices.ContainsFunc instead of lo.SomeBy, matching the existing
RateCards.HasUnitConfig implementation and project guidelines while preserving
the current boolean behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a488c71-582f-4867-af37-7bdfe5582e15
📒 Files selected for processing (36)
e2e/v1readsurface_unitconfig_test.goopenmeter/productcatalog/addon.goopenmeter/productcatalog/addon/adapter/adapter_test.goopenmeter/productcatalog/addon/adapter/addon.goopenmeter/productcatalog/addon/httpdriver/addon.goopenmeter/productcatalog/addon/service.goopenmeter/productcatalog/addon/service/addon.goopenmeter/productcatalog/errors.goopenmeter/productcatalog/http/mapping.goopenmeter/productcatalog/plan.goopenmeter/productcatalog/plan/adapter/adapter_test.goopenmeter/productcatalog/plan/adapter/plan.goopenmeter/productcatalog/plan/httpdriver/plan.goopenmeter/productcatalog/plan/plan.goopenmeter/productcatalog/plan/plan_test.goopenmeter/productcatalog/plan/service.goopenmeter/productcatalog/plan/service/plan.goopenmeter/productcatalog/plan/service_test.goopenmeter/productcatalog/plan_test.goopenmeter/productcatalog/planaddon/httpdriver/planaddon.goopenmeter/productcatalog/planaddon/service.goopenmeter/productcatalog/planaddon/service/planaddon.goopenmeter/productcatalog/ratecard.goopenmeter/productcatalog/ratecard_test.goopenmeter/productcatalog/subscription/http/change.goopenmeter/productcatalog/subscription/http/get.goopenmeter/productcatalog/subscription/http/migrate.goopenmeter/productcatalog/subscription/service.goopenmeter/productcatalog/subscription/service/change.goopenmeter/productcatalog/subscription/service/migrate.goopenmeter/subscription/addon/http/create.goopenmeter/subscription/addon/http/get.goopenmeter/subscription/addon/http/list.goopenmeter/subscription/addon/http/update.goopenmeter/subscription/subscriptionspec.goopenmeter/subscription/subscriptionspec_test.go
🚧 Files skipped from review as they are similar to previous changes (30)
- openmeter/productcatalog/addon/adapter/addon.go
- openmeter/productcatalog/subscription/service/migrate.go
- openmeter/subscription/addon/http/list.go
- openmeter/productcatalog/subscription/http/change.go
- openmeter/productcatalog/plan/service_test.go
- openmeter/productcatalog/subscription/service.go
- openmeter/productcatalog/plan/plan_test.go
- openmeter/productcatalog/subscription/http/get.go
- openmeter/productcatalog/plan_test.go
- openmeter/subscription/addon/http/get.go
- openmeter/productcatalog/plan/adapter/adapter_test.go
- openmeter/productcatalog/addon.go
- openmeter/productcatalog/errors.go
- openmeter/subscription/addon/http/create.go
- openmeter/productcatalog/subscription/http/migrate.go
- openmeter/subscription/addon/http/update.go
- openmeter/productcatalog/addon/service/addon.go
- openmeter/productcatalog/plan.go
- openmeter/productcatalog/planaddon/service.go
- openmeter/productcatalog/plan/service/plan.go
- openmeter/productcatalog/planaddon/service/planaddon.go
- openmeter/productcatalog/planaddon/httpdriver/planaddon.go
- openmeter/productcatalog/ratecard.go
- openmeter/productcatalog/plan/httpdriver/plan.go
- openmeter/subscription/subscriptionspec.go
- openmeter/productcatalog/addon/adapter/adapter_test.go
- e2e/v1readsurface_unitconfig_test.go
- openmeter/productcatalog/plan/adapter/plan.go
- openmeter/subscription/subscriptionspec_test.go
- openmeter/productcatalog/addon/httpdriver/addon.go
rolosp
force-pushed
the
feat/om-409-exclude-unit-config-from-v1
branch
from
July 14, 2026 11:39
38c4a3c to
25aeada
Compare
GAlexIHU
approved these changes
Jul 14, 2026
rolosp
force-pushed
the
feat/om-409-exclude-unit-config-from-v1
branch
from
July 14, 2026 13:36
25aeada to
7e28ebf
Compare
rolosp
enabled auto-merge (squash)
July 14, 2026 13:41
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.
What
Excludes
unit_config-bearing plans and subscriptions from the v1 API read surface: the v1 plan list omits them, and v1 plan GET / subscription GET return a typed error instead of a silently-stripped object. Touchesopenmeter/productcatalog/*(ratecard, plan, errors, plan adapter/httpdriver, subscription http) andopenmeter/subscription/*(subscriptionspec), plus a new e2e test. No schema change, no migration — this is a read-only serialization-boundary change.Why
The v1 API's rate-card types have no
unit_configfield, so on read the v1 mapper (productcatalog/http/mapping.goFromRateCard) silently drops it. A v3-authored plan billing e.g.$X per 1000 unitscomes back over v1 looking like a plain$X per unitplan — active misrepresentation, not just missing data, and unsafe wherever v1 read consumers exist (an enablement gate for the UnitConfig epic). Read and subscribe are separable: a subscription can still be started from aunit_configplan server-side (OM-399), it just can't be faithfully serialized to v1 shapes. So the fix restricts only the read surfaces and steers authoring/consumption ofunit_configto v3.How
RateCards.HasUnitConfig()(productcatalog/ratecard.go),plan.Plan.HasUnitConfig()(plan/plan.go), andSubscriptionSpec.HasUnitConfig()+ phase-level (subscription/subscriptionspec.go, mirroring the existingHasEntitlements/HasBillables) — so exclusion is decided from object content, with no stored flag.ErrUnitConfigNotRepresentable(codeunit_config_not_representable_in_v1,productcatalog/errors.go), carrying aWithHTTPStatusCodeAttribute(400)so both v1 error encoders resolve it to a 400 at the serialization boundary (the subscription encoder derives status from that attribute).unit_configplans at the query layer via a new opt-inListPlansInput.ExcludeUnitConfig, applied as an EntNot(HasPhasesWith(HasRatecardsWith(UnitConfigNotNil, DeletedAtIsNil)))predicate beforePaginate'sCOUNT, soTotalCountand the page slice stay exact. Opt-in because the adapter is shared with v3 (api/v3/handlers/plans/list.go), which can representunit_configand must keep showing these plans; only the v1 handler sets the flag.unit_configplan with the typed 400 inGetPlan, before mapping.view.Spec.HasUnitConfig()in the GET handler (not in the sharedMapSubscriptionViewToAPImapper, which is also used by change/migrate and must stay permissive). v1 subscription LIST needs no change — its summary shape carries no rate cards.unit_configplan by key still succeeds and rates correctly; only the read surfaces are restricted.RateCards,plan.Plan,SubscriptionSpec); a DB adapter test asserting the LIST exclusion + exactTotalCount; and an e2e (e2e/v1readsurface_unitconfig_test.go) driving the full path over HTTP — plan GET → 400, plain plan GET → 200, LIST exclusion + exactTotalCount, v1 subscribe-by-key → 201, subscription GET → 400. The OM-399 subscribe-from-unit_config-plan test is retained as a regression guard.unitConfig.enabled, dev rollout first); the LIST carries no hidden-count signal by explicit decision.No production code outside the v1 read path is changed, and no schema or migration is introduced.
Summary by CodeRabbit
New Features
Bug Fixes
TotalCount/listing behavior to reflect excluded unit-config items accurately.Tests
Greptile Summary
This PR keeps unit-config resources off the v1 API read and mutation surfaces. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Reviews (7): Last reviewed commit: "fix: review comment fixes" | Re-trigger Greptile
Context used (3)