fix: feature resolve in subscription - #4896
Conversation
📝 WalkthroughWalkthroughThe subscription service now resolves rate-card features before creating or updating subscriptions. It validates feature key and ID pairs, pins resolved IDs, rejects unknown or mismatched references, and applies different archived-feature rules for creation and updates. ChangesSubscription feature validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SubscriptionService
participant FeatureResolver
participant Persistence
Client->>SubscriptionService: create or update subscription
SubscriptionService->>FeatureResolver: validate item feature references
FeatureResolver-->>SubscriptionService: resolved references or error
SubscriptionService->>Persistence: persist valid subscription
Persistence-->>Client: return subscription result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if !options.IgnoreArchived { | ||
| if feat.ArchivedAt != nil && clock.Now().UTC().After(feat.ArchivedAt.UTC()) { | ||
| errs = append(errs, models.ErrorWithFieldPrefix(rateCardFieldSelector, ErrRateCardFeatureArchived)) | ||
| } | ||
| } |
There was a problem hiding this comment.
When an existing subscription references a feature archived after creation, validateUpdate passes IgnoreArchived, but the production resolver excludes archived features before this check runs. Resolution therefore returns ErrRateCardFeatureNotFound, causing unrelated subscription edits to remain blocked.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: openmeter/productcatalog/ratecard.go
Line: 1089-1093
Comment:
**Archived features remain unresolved**
When an existing subscription references a feature archived after creation, `validateUpdate` passes `IgnoreArchived`, but the production resolver excludes archived features before this check runs. Resolution therefore returns `ErrRateCardFeatureNotFound`, causing unrelated subscription edits to remain blocked.
**Knowledge Base Used:**
- [Subscription](https://app.greptile.com/openmeter/-/custom-context/knowledge-base/openmeterio/openmeter/-/docs/subscription.md)
- [Product Catalog](https://app.greptile.com/openmeter/-/custom-context/knowledge-base/openmeterio/openmeter/-/docs/productcatalog.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openmeter/productcatalog/ratecard.go (1)
1044-1056: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the archival-policy contract.
Add Go documentation for
ValidateRateCardsWithFeaturesOptionsandValidateRateCardsWithFeatures. State thatIgnoreArchivedbypasses only the archived-feature check. State that feature identity and meter validation still run.As per coding guidelines, “Document domain helpers whose names compress important business semantics, including observable behavior and why excluded cases are excluded.”
🤖 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/ratecard.go` around lines 1044 - 1056, Add Go doc comments for ValidateRateCardsWithFeaturesOptions and ValidateRateCardsWithFeatures that describe the archival-policy contract: IgnoreArchived bypasses only archived-feature validation, while feature identity and meter validation continue to run. Include why archived features are excluded when the option is enabled, without changing implementation 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.
Inline comments:
In `@openmeter/productcatalog/ratecard.go`:
- Around line 1089-1092: Update the archival check in the feature validation
flow to reject features when the current UTC time is equal to or later than
feat.ArchivedAt, replacing the strictly-after comparison while preserving the
existing IgnoreArchived guard and error handling.
---
Nitpick comments:
In `@openmeter/productcatalog/ratecard.go`:
- Around line 1044-1056: Add Go doc comments for
ValidateRateCardsWithFeaturesOptions and ValidateRateCardsWithFeatures that
describe the archival-policy contract: IgnoreArchived bypasses only
archived-feature validation, while feature identity and meter validation
continue to run. Include why archived features are excluded when the option is
enabled, without changing implementation behavior.
🪄 Autofix
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 Plus
Run ID: dd151d6d-b8f6-44d9-9576-024f9aaa14e3
📒 Files selected for processing (8)
openmeter/productcatalog/ratecard.goopenmeter/productcatalog/ratecard_test.goopenmeter/subscription/README.mdopenmeter/subscription/service/README.mdopenmeter/subscription/service/feature.goopenmeter/subscription/service/feature_resolution_test.goopenmeter/subscription/service/service.goopenmeter/subscription/service/servicevalidation.go
💤 Files with no reviewable changes (1)
- openmeter/subscription/service/service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- openmeter/subscription/README.md
- openmeter/subscription/service/README.md
| if !options.IgnoreArchived { | ||
| if feat.ArchivedAt != nil && clock.Now().UTC().After(feat.ArchivedAt.UTC()) { | ||
| errs = append(errs, models.ErrorWithFieldPrefix(rateCardFieldSelector, ErrRateCardFeatureArchived)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject a feature at its archival timestamp.
At Line 1090, a feature remains valid when clock.Now() equals feat.ArchivedAt. Use an inclusive comparison so creation cannot accept a feature at the instant it becomes archived.
Proposed fix
- if feat.ArchivedAt != nil && clock.Now().UTC().After(feat.ArchivedAt.UTC()) {
+ if feat.ArchivedAt != nil && !clock.Now().UTC().Before(feat.ArchivedAt.UTC()) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !options.IgnoreArchived { | |
| if feat.ArchivedAt != nil && clock.Now().UTC().After(feat.ArchivedAt.UTC()) { | |
| errs = append(errs, models.ErrorWithFieldPrefix(rateCardFieldSelector, ErrRateCardFeatureArchived)) | |
| } | |
| if !options.IgnoreArchived { | |
| if feat.ArchivedAt != nil && !clock.Now().UTC().Before(feat.ArchivedAt.UTC()) { | |
| errs = append(errs, models.ErrorWithFieldPrefix(rateCardFieldSelector, ErrRateCardFeatureArchived)) | |
| } |
🤖 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/ratecard.go` around lines 1089 - 1092, Update the
archival check in the feature validation flow to reject features when the
current UTC time is equal to or later than feat.ArchivedAt, replacing the
strictly-after comparison while preserving the existing IgnoreArchived guard and
error handling.
2 participants
Overview
What
Added feature validation for subscription items during create and edit, including feature-only items without entitlements.
Added PostgreSQL-backed regression tests and updated the subscription domain documentation.
Why
Feature validation previously happened indirectly during entitlement scheduling.
Feature-only items skipped that path, allowing subscriptions to reference nonexistent or mismatched features.
How
The subscription service now batch-resolves every item feature before creating or updating a subscription.
It rejects:
The existing key-based storage model remains unchanged, so no database migration was required.
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
The PR adds subscription-wide feature-reference validation during create and update, including feature-only items, and documents the intended archive behavior.
Confidence Score: 4/5
The PR is not yet safe to merge because updates referencing archived feature snapshots remain blocked.
The update path requests archived references be ignored, but its resolver excludes those features before validation can apply that policy, leaving the previously reported subscription-edit failure reachable.
Files Needing Attention: openmeter/productcatalog/ratecard.go, openmeter/productcatalog/featureresolver/resolver.go, openmeter/subscription/service/servicevalidation.go
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD U[Update existing subscription] --> V[validateUpdate with IgnoreArchived] V --> R[Production feature resolver] R --> L[ListFeatures IncludeArchived=false] L --> N[Archived feature omitted] N --> E[ErrRateCardFeatureNotFound] E --> B[Update blocked before archive check]Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "refactor: use validation instead of reso..." | Re-trigger Greptile
Context used: