refactor: add generic feature resolver with ratecard support#4353
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:
📝 WalkthroughWalkthroughIntroduces a namespaced, batch-capable NamespacedFeatureResolver; centralizes rate-card feature resolution with ResolveFeaturesForRateCards; updates Plan and Addon services to accept the resolver; rewires binaries and test infra to provide it; and adjusts tests and RateCard APIs. ChangesFeature Resolution Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![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: 3
🧹 Nitpick comments (4)
openmeter/productcatalog/featureresolver/ratecard_test.go (1)
154-159: ⚡ Quick winPlease add negative cases for missing and conflicting feature references.
Happy path is solid, but this resolver path also needs assertions for: missing key, missing ID, and ID/key mismatch (expecting validation errors, no panic).
As per coding guidelines:
**/*_test.go: Make sure the tests are comprehensive and cover the changes.🤖 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/featureresolver/ratecard_test.go` around lines 154 - 159, Add unit tests covering negative cases for ResolveFeaturesForRateCards: create separate subtests that call ResolveFeaturesForRateCards(t.Context(), resolver, namespace, &ratecards) with (1) a ratecard whose feature reference lacks Key, (2) a ratecard whose feature reference lacks ID, and (3) a ratecard whose feature reference contains a mismatched ID and Key; in each subtest assert that ResolveFeaturesForRateCards returns an error (use require.Error or assert.Error) and does not panic, and optionally validate the error message/type to ensure it's a validation error; reuse the existing variables (features, ratecards, resolver, namespace) but modify the specific ratecard entries before calling ResolveFeaturesForRateCards so tests are focused and isolated.openmeter/productcatalog/featureresolver/namespaced_test.go (1)
126-132: ⚡ Quick winBatch test should verify feature identity, not only presence.
Right now it only checks nil/non-nil, so a wrong feature pointer/value can slip through undetected. Please also assert that each found entry maps to the expected feature ID/key.
As per coding guidelines:
**/*_test.go: Make sure the tests are comprehensive and cover the changes.🤖 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/featureresolver/namespaced_test.go` around lines 126 - 132, The test currently only checks presence in the resolved map; extend the test cases to include the expected identity (e.g., add fields like expectedID or expectedFeature to each entry) and, inside the for _, test := range tests loop, when test.expectedToFind is true assert that resolved[test.idOrKey] is the exact expected feature (either by comparing the feature ID/key field, e.g. resolved[test.idOrKey].ID == test.expectedID, or by pointer equality to test.expectedFeature) so the map entry maps to the correct feature, not just a non-nil value.openmeter/productcatalog/plan/service/service_test.go (1)
283-316: ⚡ Quick winPlease add one failing feature-resolution case here.
These edits cover the happy paths for ID-only and key-only references, but they still don't lock in what
CreatePlan/UpdatePlanshould return when a phase points at a nonexistent feature. One negative case would make the new resolver wiring much harder to regress silently.As per coding guidelines,
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.Also applies to: 433-435
🤖 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/service/service_test.go` around lines 283 - 316, Add a negative test inside the existing t.Run("NewPhase") block that asserts CreatePlan/UpdatePlan returns an error when a phase references a nonexistent feature: create a phase whose RateCard or FeatureKey points to a fake/nonexistent feature key (e.g., "missing-feature"), call the same service method used by the other subtests (the service.CreatePlan or service.UpdatePlan helper used in this file), and assert the call fails with a validation or not-found error (and does not succeed or silently drop the reference). Reference the test helper names and the CreatePlan/UpdatePlan service methods used elsewhere in service_test.go so the failing case mirrors the existing happy-path setup but expects an error.openmeter/productcatalog/addon/service/service_test.go (1)
97-99: ⚡ Quick winPlease add a missing-feature regression test too.
The happy-path coverage looks good, but this refactor also changes how
CreateAddon/UpdateAddonsurface feature lookup failures. A single case with an unknownFeatureIDorFeatureKeywould give this new resolver path a much safer guardrail.As per coding guidelines,
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.Also applies to: 183-206
🤖 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/service_test.go` around lines 97 - 99, Add a regression unit test that exercises the failure path when a referenced feature cannot be found: create tests for CreateAddon and UpdateAddon that pass an unknown FeatureID and an unknown FeatureKey (mirror the existing happy-path case that uses FeatureKey: nil and FeatureID: lo.ToPtr(...)) and assert the service returns the expected not-found/error result instead of succeeding; locate the test cases near the existing cases in service_test.go (where NewEntitlementTemplateFrom(BooleanEntitlementTemplate{}) is used) and add assertions verifying the resolver/feature-lookup path in CreateAddon and UpdateAddon returns an error for missing features.
🤖 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/featureresolver/namespaced.go`:
- Around line 135-142: The loop stores the address of the loop variable f into
the result map, causing all entries to reference the same memory; change the
loop to take a fresh variable per iteration (e.g., create a local copy like ff
:= f inside the for _, f := range features loop) and store &ff into result for
both result[f.ID] and result[f.Key] (or use an index-based loop and take the
address of features[i]) so each map entry points to its own feature instance.
- Around line 93-130: In BatchResolve (namespacedFeatureResolver.BatchResolve)
avoid paginating ListFeatures when idsOrKeys is empty by short-circuiting early:
if len(idsOrKeys) == 0 return an empty map[string]*feature.Feature and nil
error; this prevents a full namespace scan on DB-backed ListFeatures and keeps
the existing behavior when idsOrKeys is non-empty where you continue to call
r.service.ListFeatures with pagination.
In `@openmeter/productcatalog/featureresolver/ratecard.go`:
- Around line 63-68: The code dereferences potentially nil entries when
comparing IDs/keys (variables feat, f and the map features), causing a panic;
update the logic around the features[key] lookup so you first retrieve the entry
into a temp (feat := features[key]) and check for nil before accessing feat.ID
or feat.Key and likewise guard f for nil before using f.ID/f.Key; if feat is nil
set feat = f only after ensuring f != nil, and perform the key conflict check
only when both feat and f are non-nil—on conflict append the validation error to
errs as before.
---
Nitpick comments:
In `@openmeter/productcatalog/addon/service/service_test.go`:
- Around line 97-99: Add a regression unit test that exercises the failure path
when a referenced feature cannot be found: create tests for CreateAddon and
UpdateAddon that pass an unknown FeatureID and an unknown FeatureKey (mirror the
existing happy-path case that uses FeatureKey: nil and FeatureID: lo.ToPtr(...))
and assert the service returns the expected not-found/error result instead of
succeeding; locate the test cases near the existing cases in service_test.go
(where NewEntitlementTemplateFrom(BooleanEntitlementTemplate{}) is used) and add
assertions verifying the resolver/feature-lookup path in CreateAddon and
UpdateAddon returns an error for missing features.
In `@openmeter/productcatalog/featureresolver/namespaced_test.go`:
- Around line 126-132: The test currently only checks presence in the resolved
map; extend the test cases to include the expected identity (e.g., add fields
like expectedID or expectedFeature to each entry) and, inside the for _, test :=
range tests loop, when test.expectedToFind is true assert that
resolved[test.idOrKey] is the exact expected feature (either by comparing the
feature ID/key field, e.g. resolved[test.idOrKey].ID == test.expectedID, or by
pointer equality to test.expectedFeature) so the map entry maps to the correct
feature, not just a non-nil value.
In `@openmeter/productcatalog/featureresolver/ratecard_test.go`:
- Around line 154-159: Add unit tests covering negative cases for
ResolveFeaturesForRateCards: create separate subtests that call
ResolveFeaturesForRateCards(t.Context(), resolver, namespace, &ratecards) with
(1) a ratecard whose feature reference lacks Key, (2) a ratecard whose feature
reference lacks ID, and (3) a ratecard whose feature reference contains a
mismatched ID and Key; in each subtest assert that ResolveFeaturesForRateCards
returns an error (use require.Error or assert.Error) and does not panic, and
optionally validate the error message/type to ensure it's a validation error;
reuse the existing variables (features, ratecards, resolver, namespace) but
modify the specific ratecard entries before calling ResolveFeaturesForRateCards
so tests are focused and isolated.
In `@openmeter/productcatalog/plan/service/service_test.go`:
- Around line 283-316: Add a negative test inside the existing t.Run("NewPhase")
block that asserts CreatePlan/UpdatePlan returns an error when a phase
references a nonexistent feature: create a phase whose RateCard or FeatureKey
points to a fake/nonexistent feature key (e.g., "missing-feature"), call the
same service method used by the other subtests (the service.CreatePlan or
service.UpdatePlan helper used in this file), and assert the call fails with a
validation or not-found error (and does not succeed or silently drop the
reference). Reference the test helper names and the CreatePlan/UpdatePlan
service methods used elsewhere in service_test.go so the failing case mirrors
the existing happy-path setup but expects an error.
🪄 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: 394c93f3-f43b-44df-ac2d-ddf3b0759606
📒 Files selected for processing (23)
app/common/productcatalog.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire_gen.gocmd/server/wire_gen.goe2e/addons_v3_test.goopenmeter/productcatalog/addon/service/addon.goopenmeter/productcatalog/addon/service/service.goopenmeter/productcatalog/addon/service/service_test.goopenmeter/productcatalog/featureresolver.goopenmeter/productcatalog/featureresolver/namespaced.goopenmeter/productcatalog/featureresolver/namespaced_test.goopenmeter/productcatalog/featureresolver/ratecard.goopenmeter/productcatalog/featureresolver/ratecard_test.goopenmeter/productcatalog/plan/adapter/adapter_test.goopenmeter/productcatalog/plan/service/plan.goopenmeter/productcatalog/plan/service/service.goopenmeter/productcatalog/plan/service/service_test.goopenmeter/productcatalog/ratecard.goopenmeter/productcatalog/testutils/env.goopenmeter/subscription/addon/extend_test.goopenmeter/subscription/testutils/service.gotest/billing/subscription_suite.gotest/customer/testenv.go
💤 Files with no reviewable changes (1)
- openmeter/productcatalog/plan/adapter/adapter_test.go
chrisgacsal
force-pushed
the
refactor/rc-feature-resolve
branch
from
19cdcd3 to
a01c54b
Compare
chrisgacsal
force-pushed
the
refactor/rc-feature-resolve
branch
from
a01c54b to
3c960e0
Compare
chrisgacsal
force-pushed
the
refactor/rc-feature-resolve
branch
from
3c960e0 to
c17fa46
Compare
turip
previously approved these changes
May 13, 2026
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/featureresolver/namespaced.go`:
- Around line 60-85: The Resolve function currently returns a single matched
feature when both id and key are provided but only one resolves; update the
logic in Resolve (the loop over featureIDOrKey, the f variable and features map)
so that if both an id and a key were supplied the method requires both to
resolve to the same feature: if one input resolves and the other does not,
return a NewGenericNotFoundError (or a clear error indicating the missing
reference); if both resolve but to different features return the existing
NewGenericConflictError as before; also add a unit test exercising the “both
provided but only one resolves” case (and reference ResolveFeaturesForRateCards
behavior) to document the intended behavior.
🪄 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: 23466414-9fac-46f7-8b66-fb68655789d3
📒 Files selected for processing (23)
app/common/productcatalog.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire_gen.gocmd/server/wire_gen.goe2e/addons_v3_test.goopenmeter/productcatalog/addon/service/addon.goopenmeter/productcatalog/addon/service/service.goopenmeter/productcatalog/addon/service/service_test.goopenmeter/productcatalog/featureresolver.goopenmeter/productcatalog/featureresolver/namespaced.goopenmeter/productcatalog/featureresolver/namespaced_test.goopenmeter/productcatalog/featureresolver/ratecard.goopenmeter/productcatalog/featureresolver/ratecard_test.goopenmeter/productcatalog/plan/adapter/adapter_test.goopenmeter/productcatalog/plan/service/plan.goopenmeter/productcatalog/plan/service/service.goopenmeter/productcatalog/plan/service/service_test.goopenmeter/productcatalog/ratecard.goopenmeter/productcatalog/testutils/env.goopenmeter/subscription/addon/extend_test.goopenmeter/subscription/testutils/service.gotest/billing/subscription_suite.gotest/customer/testenv.go
💤 Files with no reviewable changes (1)
- openmeter/productcatalog/plan/adapter/adapter_test.go
✅ Files skipped from review due to trivial changes (4)
- cmd/jobs/internal/wire_gen.go
- openmeter/productcatalog/addon/service/service_test.go
- cmd/billing-worker/wire_gen.go
- cmd/server/wire_gen.go
🚧 Files skipped from review as they are similar to previous changes (14)
- e2e/addons_v3_test.go
- test/customer/testenv.go
- openmeter/productcatalog/testutils/env.go
- openmeter/subscription/testutils/service.go
- test/billing/subscription_suite.go
- openmeter/productcatalog/plan/service/plan.go
- openmeter/productcatalog/addon/service/service.go
- app/common/productcatalog.go
- openmeter/productcatalog/featureresolver/ratecard_test.go
- openmeter/productcatalog/ratecard.go
- openmeter/subscription/addon/extend_test.go
- openmeter/productcatalog/featureresolver/ratecard.go
- openmeter/productcatalog/addon/service/addon.go
- openmeter/productcatalog/featureresolver/namespaced_test.go
chrisgacsal
force-pushed
the
refactor/rc-feature-resolve
branch
from
c17fa46 to
8a797e1
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openmeter/productcatalog/featureresolver/namespaced.go (1)
60-76:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard unresolved entries before dereferencing
ff.Because
BatchResolveseeds every requested id/key with anilvalue,okon Line 61 only means the key was requested, not that it resolved. If theidresolves but thekeydoes not,ffisnilon the second iteration and Line 68 panics onff.ID.At minimum, skip
nilmatches before comparing IDs. If both refs are supposed to be authoritative, this is also the right spot to turn the missing side into a not-found error instead of silently succeeding.💡 Minimal fix
for _, v := range featureIDOrKey { - if ff, ok := features[v]; ok { - if f == nil { - f = ff - - continue - } - - if f.ID != ff.ID { - return nil, models.NewGenericConflictError( - fmt.Errorf("id and key reference different features: %v+", - lo.MapToSlice(features, func(key string, v *feature.Feature) string { - return fmt.Sprintf("%s => %s", key, v.ID) - }), - ), - ) - } + ff, ok := features[v] + if !ok || ff == nil { + continue + } + + if f == nil { + f = ff + + continue + } + + if f.ID != ff.ID { + return nil, models.NewGenericConflictError( + fmt.Errorf("id and key reference different features: %v+", + lo.MapToSlice(features, func(key string, v *feature.Feature) string { + return fmt.Sprintf("%s => %s", key, v.ID) + }), + ), + ) } }#!/bin/bash set -euo pipefail echo "Resolve path:" sed -n '38,85p' openmeter/productcatalog/featureresolver/namespaced.go echo echo "BatchResolve map initialization:" sed -n '112,124p' openmeter/productcatalog/featureresolver/namespaced.go echo echo "Expected check:" echo "- Resolve should not treat 'ok == true' as 'feature found' when BatchResolve pre-seeds missing identifiers with nil values." echo "- A nil guard is needed before any use of ff.ID."Also applies to: 112-124
🤖 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/featureresolver/namespaced.go` around lines 60 - 76, The loop over featureIDOrKey uses the features map entries returned by BatchResolve but treats ok==true as "found" even though entries can be nil; guard against nil ff before dereferencing (skip nil ff when setting/compare, or return a not-found error if both id and key were expected to resolve). Update the loop in namespaced.go (the block that iterates featureIDOrKey and references ff and f) to: 1) check if ff == nil and either continue (if missing side is acceptable) or return a not-found error, and 2) only compare f.ID and ff.ID when both f and ff are non-nil (preserve the existing models.NewGenericConflictError path but use lo.MapToSlice safely by skipping nil values). Ensure analogous nil-guards are applied to the same logic that appears around lines initializing BatchResolve results.
🤖 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.
Duplicate comments:
In `@openmeter/productcatalog/featureresolver/namespaced.go`:
- Around line 60-76: The loop over featureIDOrKey uses the features map entries
returned by BatchResolve but treats ok==true as "found" even though entries can
be nil; guard against nil ff before dereferencing (skip nil ff when
setting/compare, or return a not-found error if both id and key were expected to
resolve). Update the loop in namespaced.go (the block that iterates
featureIDOrKey and references ff and f) to: 1) check if ff == nil and either
continue (if missing side is acceptable) or return a not-found error, and 2)
only compare f.ID and ff.ID when both f and ff are non-nil (preserve the
existing models.NewGenericConflictError path but use lo.MapToSlice safely by
skipping nil values). Ensure analogous nil-guards are applied to the same logic
that appears around lines initializing BatchResolve results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3dbc5f6d-3659-46e9-b827-ed3ab6c0e80e
📒 Files selected for processing (23)
app/common/productcatalog.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire_gen.gocmd/server/wire_gen.goe2e/addons_v3_test.goopenmeter/productcatalog/addon/service/addon.goopenmeter/productcatalog/addon/service/service.goopenmeter/productcatalog/addon/service/service_test.goopenmeter/productcatalog/featureresolver.goopenmeter/productcatalog/featureresolver/namespaced.goopenmeter/productcatalog/featureresolver/namespaced_test.goopenmeter/productcatalog/featureresolver/ratecard.goopenmeter/productcatalog/featureresolver/ratecard_test.goopenmeter/productcatalog/plan/adapter/adapter_test.goopenmeter/productcatalog/plan/service/plan.goopenmeter/productcatalog/plan/service/service.goopenmeter/productcatalog/plan/service/service_test.goopenmeter/productcatalog/ratecard.goopenmeter/productcatalog/testutils/env.goopenmeter/subscription/addon/extend_test.goopenmeter/subscription/testutils/service.gotest/billing/subscription_suite.gotest/customer/testenv.go
💤 Files with no reviewable changes (1)
- openmeter/productcatalog/plan/adapter/adapter_test.go
✅ Files skipped from review due to trivial changes (2)
- cmd/billing-worker/wire_gen.go
- cmd/server/wire_gen.go
🚧 Files skipped from review as they are similar to previous changes (18)
- openmeter/productcatalog/testutils/env.go
- openmeter/subscription/testutils/service.go
- cmd/jobs/internal/wire_gen.go
- e2e/addons_v3_test.go
- test/customer/testenv.go
- openmeter/productcatalog/plan/service/service_test.go
- openmeter/productcatalog/addon/service/service_test.go
- openmeter/subscription/addon/extend_test.go
- openmeter/productcatalog/featureresolver/namespaced_test.go
- test/billing/subscription_suite.go
- openmeter/productcatalog/addon/service/service.go
- openmeter/productcatalog/featureresolver/ratecard_test.go
- openmeter/productcatalog/plan/service/service.go
- openmeter/productcatalog/featureresolver.go
- openmeter/productcatalog/featureresolver/ratecard.go
- openmeter/productcatalog/plan/service/plan.go
- openmeter/productcatalog/addon/service/addon.go
- app/common/productcatalog.go
chrisgacsal
force-pushed
the
refactor/rc-feature-resolve
branch
from
8a797e1 to
5e94c09
Compare
chrisgacsal
force-pushed
the
refactor/rc-feature-resolve
branch
from
5e94c09 to
1ef4c89
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
openmeter/productcatalog/featureresolver/namespaced.go (1)
96-98:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent empty-input contract for
BatchResolve.When
idsOrKeysis empty you return(nil, nil), but the non-empty path always returns a pre-populated map (never nil). Callers iterating the result withrangeare fine, but anyone doing a length check or comparing to a non-nil empty map gets surprised. Returning an empty (non-nil) map keeps the contract uniform and matches what the past review here suggested.♻️ Tiny tweak
- if len(idsOrKeys) == 0 { - return nil, nil - } + if len(idsOrKeys) == 0 { + return map[string]*feature.Feature{}, nil + }🤖 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/featureresolver/namespaced.go` around lines 96 - 98, BatchResolve currently returns (nil, nil) when idsOrKeys is empty which is inconsistent with the non-empty path that returns a non-nil map; change the empty-input branch to return an allocated empty map (same map type returned by BatchResolve) and nil error instead of nil, nil so callers always receive a non-nil map; locate the empty check around idsOrKeys in BatchResolve and replace the nil return with a new empty map instance.
🧹 Nitpick comments (1)
openmeter/productcatalog/featureresolver/ratecard_test.go (1)
205-231: ⚡ Quick winConsider adding a "partial resolution" case.
The current scenarios cover both-resolve-and-match, both-missing, and both-resolve-but-mismatch. There's no case for "one of
FeatureID/FeatureKeyis set to a real value while the other points to a non-existent feature". That path exercises the corner innamespacedFeatureResolver.Resolvewhere one map entry isnil(the spot flagged in the other file for a potential NPE). Adding it here would lock down the intended behavior and catch regressions cheaply.🤖 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/featureresolver/ratecard_test.go` around lines 205 - 231, Add a new test case in ratecard_test.go that covers the "partial resolution" scenario: create a RateCards entry where one identifier is valid and resolves to a real feature (e.g., FeatureKey set to features[0].Key) while the other identifier points to a non-existent feature (e.g., FeatureID set to a fake ID or vice versa), and assert the resolver returns productcatalog.ErrRateCardFeatureMismatch; this will exercise namespacedFeatureResolver.Resolve's path where one map entry can be nil and prevent a potential NPE by verifying the intended mismatch behavior.
🤖 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/featureresolver/namespaced.go`:
- Around line 58-78: The loop that compares resolved features (variables f and
ff over featureIDOrKey / features) can dereference a nil ff; before comparing
IDs check for nil: if ff == nil then decide semantics—either skip nil entries
when matching the same feature (continue) or, if both id and key were supplied
but only one resolved, return models.NewGenericNotFoundError (stricter
behavior). Concretely, inside the loop in BatchResolve/Resolve update the logic
to first if ff == nil { if both inputs provided return
NewGenericNotFoundError(...) else continue } else if f == nil { f = ff } else if
f.ID != ff.ID { return models.NewGenericConflictError(...) } to avoid
nil-pointer deref.
In `@openmeter/productcatalog/featureresolver/ratecard_test.go`:
- Line 72: The test case name contains a typo: change the map entry value
"sucess" to the correct "success" in the test case declaration within
ratecard_test.go (the test case name field referenced as name: "sucess"); update
any assertions or subtest t.Run calls that rely on that exact string if present
so they match the corrected "success".
---
Duplicate comments:
In `@openmeter/productcatalog/featureresolver/namespaced.go`:
- Around line 96-98: BatchResolve currently returns (nil, nil) when idsOrKeys is
empty which is inconsistent with the non-empty path that returns a non-nil map;
change the empty-input branch to return an allocated empty map (same map type
returned by BatchResolve) and nil error instead of nil, nil so callers always
receive a non-nil map; locate the empty check around idsOrKeys in BatchResolve
and replace the nil return with a new empty map instance.
---
Nitpick comments:
In `@openmeter/productcatalog/featureresolver/ratecard_test.go`:
- Around line 205-231: Add a new test case in ratecard_test.go that covers the
"partial resolution" scenario: create a RateCards entry where one identifier is
valid and resolves to a real feature (e.g., FeatureKey set to features[0].Key)
while the other identifier points to a non-existent feature (e.g., FeatureID set
to a fake ID or vice versa), and assert the resolver returns
productcatalog.ErrRateCardFeatureMismatch; this will exercise
namespacedFeatureResolver.Resolve's path where one map entry can be nil and
prevent a potential NPE by verifying the intended mismatch behavior.
🪄 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: 3264eac7-5e3a-4931-be51-ffe4fcf6c3b6
📒 Files selected for processing (23)
app/common/productcatalog.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire_gen.gocmd/server/wire_gen.goe2e/addons_v3_test.goopenmeter/productcatalog/addon/service/addon.goopenmeter/productcatalog/addon/service/service.goopenmeter/productcatalog/addon/service/service_test.goopenmeter/productcatalog/featureresolver.goopenmeter/productcatalog/featureresolver/namespaced.goopenmeter/productcatalog/featureresolver/namespaced_test.goopenmeter/productcatalog/featureresolver/ratecard.goopenmeter/productcatalog/featureresolver/ratecard_test.goopenmeter/productcatalog/plan/adapter/adapter_test.goopenmeter/productcatalog/plan/service/plan.goopenmeter/productcatalog/plan/service/service.goopenmeter/productcatalog/plan/service/service_test.goopenmeter/productcatalog/ratecard.goopenmeter/productcatalog/testutils/env.goopenmeter/subscription/addon/extend_test.goopenmeter/subscription/testutils/service.gotest/billing/subscription_suite.gotest/customer/testenv.go
💤 Files with no reviewable changes (1)
- openmeter/productcatalog/plan/adapter/adapter_test.go
✅ Files skipped from review due to trivial changes (1)
- cmd/jobs/internal/wire_gen.go
🚧 Files skipped from review as they are similar to previous changes (18)
- openmeter/subscription/testutils/service.go
- openmeter/subscription/addon/extend_test.go
- openmeter/productcatalog/testutils/env.go
- test/customer/testenv.go
- e2e/addons_v3_test.go
- test/billing/subscription_suite.go
- openmeter/productcatalog/addon/service/service_test.go
- openmeter/productcatalog/featureresolver/ratecard.go
- cmd/billing-worker/wire_gen.go
- app/common/productcatalog.go
- openmeter/productcatalog/featureresolver/namespaced_test.go
- cmd/server/wire_gen.go
- openmeter/productcatalog/addon/service/addon.go
- openmeter/productcatalog/plan/service/service.go
- openmeter/productcatalog/plan/service/service_test.go
- openmeter/productcatalog/addon/service/service.go
- openmeter/productcatalog/featureresolver.go
- openmeter/productcatalog/plan/service/plan.go
chrisgacsal
force-pushed
the
refactor/rc-feature-resolve
branch
from
1ef4c89 to
2bf3ae2
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openmeter/productcatalog/featureresolver/namespaced.go (1)
64-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat an unresolved
keyas not found before checking for conflicts.Right now, if the ID resolves first and the key lookup misses,
fstays populated from the ID path, so Line 73 reports a conflict instead of a missing key. That makes this branch asymmetric with the missing-ID case and returns a misleading error type.💡 Minimal fix
if fKey != "" { - if f == nil { - f = features[fKey] - } - - if f == nil { - errs = append(errs, models.NewGenericNotFoundError(fmt.Errorf("feature [feature.key=%s]", *key))) - } - - if f != nil && f.Key != fKey { + ff := features[fKey] + if ff == nil { + errs = append(errs, models.NewGenericNotFoundError(fmt.Errorf("feature [feature.key=%s]", fKey))) + } else if f == nil { + f = ff + } else if f.ID != ff.ID { errs = append(errs, models.NewGenericConflictError(fmt.Errorf("feature [feature.id=%s feature.key=%s]", fID, fKey))) } }🤖 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/featureresolver/namespaced.go` around lines 64 - 75, When handling the fKey branch in the namespaced resolver, ensure a missing key is treated as "not found" before doing any conflict checks: look up features[fKey] into a temporary (e.g., found := features[fKey]); if found == nil append models.NewGenericNotFoundError(fmt.Errorf("feature [feature.key=%s]", *key)) and set f = nil (or return), otherwise set f = found; only perform the models.NewGenericConflictError check if f is non-nil and its Key differs from fKey (use fID and fKey as existing identifiers).
🧹 Nitpick comments (1)
openmeter/productcatalog/featureresolver/namespaced.go (1)
89-109: ⚡ Quick winNormalize
idsOrKeysbefore callingListFeatures.
Resolvecan feed this with""for the unused side, and batch callers can pass duplicates. The result map collapses those anyway, but the DB query and page size are still built from the larger raw slice.♻️ Small cleanup
func (r *namespacedFeatureResolver) BatchResolve(ctx context.Context, idsOrKeys ...string) (map[string]*feature.Feature, error) { @@ - if len(idsOrKeys) == 0 { + normalized := make([]string, 0, len(idsOrKeys)) + seen := make(map[string]struct{}, len(idsOrKeys)) + for _, v := range idsOrKeys { + if v == "" { + continue + } + if _, ok := seen[v]; ok { + continue + } + seen[v] = struct{}{} + normalized = append(normalized, v) + } + + if len(normalized) == 0 { return nil, nil } features, err := pagination.CollectAll(ctx, pagination.NewPaginator(func(ctx context.Context, page pagination.Page) (pagination.Result[feature.Feature], error) { return r.service.ListFeatures(ctx, feature.ListFeaturesParams{ - IDsOrKeys: idsOrKeys, + IDsOrKeys: normalized, Namespace: r.namespace, IncludeArchived: false, Page: page, }) - }), min(len(idsOrKeys), 100)) + }), min(len(normalized), 100)) @@ - result := lo.SliceToMap(idsOrKeys, func(item string) (string, *feature.Feature) { + result := lo.SliceToMap(normalized, func(item string) (string, *feature.Feature) {As per coding guidelines, "Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks."
🤖 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/featureresolver/namespaced.go` around lines 89 - 109, The BatchResolve implementation should normalize the incoming idsOrKeys slice before using it in the DB query and pagination: inside namespacedFeatureResolver.BatchResolve, remove empty strings and deduplicate values from the idsOrKeys parameter (so Resolve's "" placeholders are dropped and duplicates collapsed) and then pass that normalized slice to pagination.NewPaginator / r.service.ListFeatures and use min(len(normalizedSlice), 100) for the page size; update any references to idsOrKeys in this function to use the normalized slice when constructing feature.ListFeaturesParams and when computing the paginator size.
🤖 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.
Duplicate comments:
In `@openmeter/productcatalog/featureresolver/namespaced.go`:
- Around line 64-75: When handling the fKey branch in the namespaced resolver,
ensure a missing key is treated as "not found" before doing any conflict checks:
look up features[fKey] into a temporary (e.g., found := features[fKey]); if
found == nil append models.NewGenericNotFoundError(fmt.Errorf("feature
[feature.key=%s]", *key)) and set f = nil (or return), otherwise set f = found;
only perform the models.NewGenericConflictError check if f is non-nil and its
Key differs from fKey (use fID and fKey as existing identifiers).
---
Nitpick comments:
In `@openmeter/productcatalog/featureresolver/namespaced.go`:
- Around line 89-109: The BatchResolve implementation should normalize the
incoming idsOrKeys slice before using it in the DB query and pagination: inside
namespacedFeatureResolver.BatchResolve, remove empty strings and deduplicate
values from the idsOrKeys parameter (so Resolve's "" placeholders are dropped
and duplicates collapsed) and then pass that normalized slice to
pagination.NewPaginator / r.service.ListFeatures and use
min(len(normalizedSlice), 100) for the page size; update any references to
idsOrKeys in this function to use the normalized slice when constructing
feature.ListFeaturesParams and when computing the paginator size.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a8b9e00e-e7d1-4758-9b82-e499090e53f8
📒 Files selected for processing (23)
app/common/productcatalog.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire_gen.gocmd/server/wire_gen.goe2e/addons_v3_test.goopenmeter/productcatalog/addon/service/addon.goopenmeter/productcatalog/addon/service/service.goopenmeter/productcatalog/addon/service/service_test.goopenmeter/productcatalog/featureresolver.goopenmeter/productcatalog/featureresolver/namespaced.goopenmeter/productcatalog/featureresolver/namespaced_test.goopenmeter/productcatalog/featureresolver/ratecard.goopenmeter/productcatalog/featureresolver/ratecard_test.goopenmeter/productcatalog/plan/adapter/adapter_test.goopenmeter/productcatalog/plan/service/plan.goopenmeter/productcatalog/plan/service/service.goopenmeter/productcatalog/plan/service/service_test.goopenmeter/productcatalog/ratecard.goopenmeter/productcatalog/testutils/env.goopenmeter/subscription/addon/extend_test.goopenmeter/subscription/testutils/service.gotest/billing/subscription_suite.gotest/customer/testenv.go
💤 Files with no reviewable changes (1)
- openmeter/productcatalog/plan/adapter/adapter_test.go
✅ Files skipped from review due to trivial changes (2)
- cmd/billing-worker/wire_gen.go
- cmd/server/wire_gen.go
🚧 Files skipped from review as they are similar to previous changes (18)
- e2e/addons_v3_test.go
- openmeter/productcatalog/addon/service/service_test.go
- openmeter/subscription/addon/extend_test.go
- test/billing/subscription_suite.go
- test/customer/testenv.go
- openmeter/productcatalog/plan/service/service.go
- openmeter/productcatalog/addon/service/service.go
- openmeter/productcatalog/featureresolver/namespaced_test.go
- openmeter/productcatalog/ratecard.go
- openmeter/productcatalog/plan/service/plan.go
- openmeter/productcatalog/plan/service/service_test.go
- openmeter/productcatalog/addon/service/addon.go
- openmeter/productcatalog/testutils/env.go
- cmd/jobs/internal/wire_gen.go
- openmeter/subscription/testutils/service.go
- openmeter/productcatalog/featureresolver/ratecard_test.go
- openmeter/productcatalog/featureresolver/ratecard.go
- app/common/productcatalog.go
chrisgacsal
force-pushed
the
refactor/rc-feature-resolve
branch
from
2bf3ae2 to
4663be0
Compare
tothandras
previously approved these changes
May 13, 2026
turip
previously approved these changes
May 13, 2026
chrisgacsal
force-pushed
the
refactor/rc-feature-resolve
branch
from
4663be0 to
cbe3887
Compare
turip
approved these changes
May 14, 2026
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
3 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.
Overview
Note for the Revier
This PR relies on #4352.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests