fix: resolving feature#4338
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughFeature resolution and rate card metadata handling across product catalog and subscription layers is refined: adapter mappings backfill missing feature keys when IDs exist, plan service phase iteration is fixed for in-place mutations, subscription views propagate resolved feature IDs, and subscription service feature resolution expands to handle both identifiers. Some E2E tests are skipped pending validation. ChangesFeature Resolution and Rate Card Metadata Backfill
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: 4
🧹 Nitpick comments (3)
openmeter/productcatalog/plan/adapter/mapping.go (1)
302-310: 💤 Low valueFeature key backfill workaround is consistent with addon mapping.
This mirrors the same pattern in
addon/adapter/mapping.go. Same suggestion applies here about making the error message more descriptive with namespace/ID context.🤖 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/adapter/mapping.go` around lines 302 - 310, The error returned when failing to load the feature for a ratecard is too generic; update the block that checks r.FeatureID/r.FeatureKey (the code that calls r.Edges.FeaturesOrErr() and sets meta.FeatureKey from ratecardFeature.Key) to return a descriptive error using fmt.Errorf that includes identifying context such as the ratecard/record ID and the FeatureID (or namespace) so the message reads something like "feature not loaded for ratecard <ID> featureID <FeatureID>: %w" and include the original err as the wrapped error.openmeter/productcatalog/addon/adapter/mapping.go (1)
101-109: 💤 Low valueWorkaround for missing feature key linkage works correctly.
The logic properly handles the case where
FeatureIDis set butFeatureKeyis missing by backfilling from the loaded Features edge. The error message could be slightly more helpful by including context:💬 Optional: More descriptive error message
if r.FeatureID != nil && r.FeatureKey == nil { ratecardFeature, err := r.Edges.FeaturesOrErr() if err != nil { - return nil, errors.New("feature is not loaded for ratecard") + return nil, fmt.Errorf("feature edge not loaded for ratecard %s (namespace=%s, feature_id=%s)", r.ID, r.Namespace, *r.FeatureID) }🤖 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/mapping.go` around lines 101 - 109, The error returned when Features edge is not loaded in the block that backfills FeatureKey (within mapping.go where r.FeatureID != nil && r.FeatureKey == nil uses r.Edges.FeaturesOrErr()) should include more context; update the errors.New(...) to return a descriptive error that includes identifying information such as the RateCard or FeatureID (use r.FeatureID or another available identifier) and mention that the Features edge was not loaded so callers can quickly identify the problematic entity.openmeter/subscription/service/service.go (1)
554-572: 💤 Low valueConsider extracting the unique key/ID builder for clarity.
The inline function that builds
uniqFeatureKeyOrIDsis readable but adds nesting. Extracting it to a helper method would improve flow and make it easier to unit test the deduplication logic separately.♻️ Optional: Extract helper
+func extractUniqueFeatureIdentifiers(items []subscription.SubscriptionItem) []string { + keyOrIDs := make(map[string]struct{}, len(items)) + for _, item := range items { + fKey, fID := lo.FromPtr(item.RateCard.AsMeta().FeatureKey), lo.FromPtr(item.RateCard.AsMeta().FeatureID) + if fKey != "" { + keyOrIDs[fKey] = struct{}{} + } + if fID != "" { + keyOrIDs[fID] = struct{}{} + } + } + return lo.MapToSlice(keyOrIDs, func(key string, _ struct{}) string { + return key + }) +} + func (s *service) ExpandViews(ctx context.Context, subs []subscription.Subscription) ([]subscription.SubscriptionView, error) { ... var featsOfItems pagination.Result[feature.Feature] { - uniqFeatureKeyOrIDs := func() []string { - keyOrIDS := make(map[string]struct{}, len(items)) - - for _, item := range items { - fKey, fID := lo.FromPtr(item.RateCard.AsMeta().FeatureKey), lo.FromPtr(item.RateCard.AsMeta().FeatureID) - - if fKey != "" { - keyOrIDS[fKey] = struct{}{} - } - - if fID != "" { - keyOrIDS[fID] = struct{}{} - } - } - - return lo.MapToSlice(keyOrIDS, func(key string, _ struct{}) string { - return key - }) - }() + uniqFeatureKeyOrIDs := extractUniqueFeatureIdentifiers(items)🤖 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/subscription/service/service.go` around lines 554 - 572, Extract the inline builder for uniqFeatureKeyOrIDs into a new helper function (e.g., buildUniqueFeatureKeysOrIDs(items []*SubscriptionItem) []string) that encapsulates the loop, the lo.FromPtr calls on item.RateCard.AsMeta().FeatureKey and FeatureID, the map-based deduplication and the final lo.MapToSlice conversion; replace the inline anonymous function with a call to that helper so the deduplication logic is isolated and unit-testable while preserving the exact returned []string 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 `@e2e/productcatalog_smoke_v3_test.go`:
- Around line 140-141: Remove or update the unconditional t.Skip("Skip this
test...") in e2e/productcatalog_smoke_v3_test.go so the defective rate card
validation subtests are not silently disabled; either revert the skip to
re-enable the tests (so the plan/ratecard persistence and validation modified by
the PR are exercised) or replace the skip message with a precise explanation why
the test must remain skipped (for example "validation now requires explicit
feature IDs"), and apply the same change to the other skipped call around the
related subtests (the t.Skip at the other location referenced in the comment).
- Around line 92-93: The e2e test skip in e2e/productcatalog_smoke_v3_test.go
removes validation of the multi-rate-card scenario that exercises feature
resolution changed in openmeter/subscription/service/service.go; update the test
(remove the t.Skip) to use the new dual key/ID resolution by referencing some
features by ID and others by key in the rate cards so the flat + usage-based +
graduated scenario runs, or if you prefer add a small dedicated e2e that
specifically asserts feature-by-ID resolution, and if you must keep it skipped
include a tracking issue/PR number in the skip message for accountability.
In `@openmeter/subscription/service/service.go`:
- Around line 658-670: The feature-matching lambda passed to lo.Find uses
sequential assignments to featFound which lets the second check overwrite the
first; change the predicate in that anonymous function (the one iterating
featsOfItems.Items with feature.Feature f and using fKey/fID/featFound) to use
OR logic instead of sequential assignment—e.g., compute featFound as (fKey != ""
&& f.Key == fKey) || (fID != "" && f.ID == fID) or use short-circuit if/else
checks so a key match isn’t lost when an ID check follows.
In `@openmeter/subscription/subscriptionview.go`:
- Around line 411-417: The call to item.RateCard.ChangeMeta is ignoring its
returned error; update the code around itemFeat handling so you capture the
error from item.RateCard.ChangeMeta (the lambda that sets m.FeatureID =
lo.ToPtr(itemFeat.ID) can still lead ChangeMeta to fail) and handle it
appropriately—e.g., return the error up the call stack or log it and continue
depending on surrounding semantics; ensure you reference
item.RateCard.ChangeMeta, productcatalog.RateCardMeta, and itemFeat when adding
the error check so failures to mutate the rate card are not silently dropped.
---
Nitpick comments:
In `@openmeter/productcatalog/addon/adapter/mapping.go`:
- Around line 101-109: The error returned when Features edge is not loaded in
the block that backfills FeatureKey (within mapping.go where r.FeatureID != nil
&& r.FeatureKey == nil uses r.Edges.FeaturesOrErr()) should include more
context; update the errors.New(...) to return a descriptive error that includes
identifying information such as the RateCard or FeatureID (use r.FeatureID or
another available identifier) and mention that the Features edge was not loaded
so callers can quickly identify the problematic entity.
In `@openmeter/productcatalog/plan/adapter/mapping.go`:
- Around line 302-310: The error returned when failing to load the feature for a
ratecard is too generic; update the block that checks r.FeatureID/r.FeatureKey
(the code that calls r.Edges.FeaturesOrErr() and sets meta.FeatureKey from
ratecardFeature.Key) to return a descriptive error using fmt.Errorf that
includes identifying context such as the ratecard/record ID and the FeatureID
(or namespace) so the message reads something like "feature not loaded for
ratecard <ID> featureID <FeatureID>: %w" and include the original err as the
wrapped error.
In `@openmeter/subscription/service/service.go`:
- Around line 554-572: Extract the inline builder for uniqFeatureKeyOrIDs into a
new helper function (e.g., buildUniqueFeatureKeysOrIDs(items
[]*SubscriptionItem) []string) that encapsulates the loop, the lo.FromPtr calls
on item.RateCard.AsMeta().FeatureKey and FeatureID, the map-based deduplication
and the final lo.MapToSlice conversion; replace the inline anonymous function
with a call to that helper so the deduplication logic is isolated and
unit-testable while preserving the exact returned []string 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: 47e373ad-873f-405c-97c7-255e5cc8b311
📒 Files selected for processing (10)
e2e/productcatalog_smoke_v3_test.goopenmeter/productcatalog/addon/adapter/mapping.goopenmeter/productcatalog/plan/adapter/mapping.goopenmeter/productcatalog/plan/adapter/phase.goopenmeter/productcatalog/plan/service/plan.goopenmeter/productcatalog/plan/service/service_test.goopenmeter/subscription/repo/mapping.goopenmeter/subscription/service/service.goopenmeter/subscription/service/sync_test.goopenmeter/subscription/subscriptionview.go
chrisgacsal
force-pushed
the
fix/feature-key
branch
from
22f782c to
e22850b
Compare
turip
approved these changes
May 11, 2026
GAlexIHU
approved these changes
May 11, 2026
This was referenced May 11, 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
Fix resolving features for ratecards both in product catalog and subscription services.
Prior to this fix ratecards with features referenced by their ID instead of key were not properly resolved, handled.
Notes for reviewer
Some end-to-end tests covering product catalog functionalities are temporarily disabled and are going to be fixed in a followup PR.
Summary by CodeRabbit
Bug Fixes
Tests