Fix customer subject validation #2385
Conversation
📝 WalkthroughWalkthroughThis pull request modifies the subscription and customer validation logic across multiple modules. Key changes include adding a new Changes
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
app/common/subscription.go(1 hunks)openmeter/subscription/service/service.go(4 hunks)openmeter/subscription/subscriptionspec.go(2 hunks)openmeter/subscription/validators/customer/validator.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: CI
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
app/common/subscription.go (1)
95-95: Adoption of extended validator constructorPassing
customerServiceintoNewValidatoraligns with the enhanced validation needs. The error handling is straightforward and gracefully prevents partial initialization.openmeter/subscription/service/service.go (6)
60-60: Comment update onlyNo functional logic is introduced or modified here; the updated comment is clear.
83-89: Currency mismatch check for billable subscriptionsRequiring the customer’s currency to match the subscription currency helps avoid inconsistencies. The check is succinct and properly returns an error on mismatch.
151-152: Auto-set customer currency for first paid subscriptionAssigning the subscription’s currency when the customer’s currency is unset is sensible. However, be mindful of concurrency or simultaneous subscription requests that might overwrite or race.
Would you like a script to detect concurrency handling patterns in the repository (e.g., usage of transactions) to confirm consistency?
210-220: Comment update onlyThis mirrors the create method’s approach to fetching and validating the customer. No additional logic changes introduced.
223-230: Duplicate subject key validation logicSee the comment for lines 73-81 regarding factoring out the subject key checks into a shared utility.
232-239: Duplicate currency mismatch checkSee the comment for lines 83-89 for consistency suggestions.
openmeter/subscription/validators/customer/validator.go (1)
30-31: New field in Validator structAdding
customerServiceto theValidatorstruct aligns with the revised constructor. This maintains clarity and consistency in referencing customer data.openmeter/subscription/subscriptionspec.go (3)
154-158: Looks good!This new method checks if any subscription phase has entitlements by delegating to each phase's
HasEntitlements()method. The implementation usinglo.SomeBy()is efficient and returns early once it finds a phase with entitlements.
160-164: Looks good!This method follows the same pattern as
HasEntitlements(), delegating to each phase'sHasBillables()method to determine if the subscription has any billable items.
166-170: Looks good!This method checks for metered billables across all phases using the same delegation pattern. The implementation is consistent with the other helper methods in this section.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
openmeter/subscription/subscriptionspec.go(2 hunks)openmeter/subscription/validators/customer/validator.go(1 hunks)
🔇 Additional comments (8)
openmeter/subscription/validators/customer/validator.go (3)
26-26: Field assignment looks correct.
Storing thecustomerServicein theValidatoris consistent with the new constructor requirement.
33-33: Field addition is consistent.
IntroducingcustomerServicein theValidatorstruct ensures the validator can now perform customer-based validations.
36-78: Revisit subject key comparison for potential ordering issues.
When comparing subject keys to prevent modifications, the code currently checks each index. If reordering identical keys is meant to be inconsequential, consider using an order-agnostic approach to avoid false positives.openmeter/subscription/subscriptionspec.go (5)
154-158: Utility method appears correct.
HasEntitlements()succinctly checks phases usinglo.SomeBy, providing clear and efficient logic.
160-164: Good use of functional style for checking billables.
HasBillables()similarly reads well, and leverageslo.SomeByfor succinctness.
166-170: Metered billables check is concise.
HasMeteredBillables()efficiently identifies metered billables across phases.
391-395: Entitlements check in phase spec is straightforward.
Usinglo.SomeByon flattened items is both clear and consistent.
397-401: Metered billables check in phase spec is consistent.
Mirrors the approach used at the subscription level, maintaining code clarity.
| func NewValidator(subscriptionService subscription.Service, customerService customer.Service) (*Validator, error) { | ||
| if subscriptionService == nil { | ||
| return nil, fmt.Errorf("subscription service is required") | ||
| } | ||
| if customerService == nil { | ||
| return nil, fmt.Errorf("customer service is required") | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ensure coverage for nil-check guard clauses.
The non-nil checks for subscriptionService and customerService are appropriate. Consider adding corresponding test cases to guarantee expected error behavior when either dependency is missing.
GAlexIHU
force-pushed
the
fix-customer-subject-validation
branch
from
0aff9ac to
14cd381
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
openmeter/subscription/validators/customer/validator.go (1)
50-72: Increase test coverage for partial or reordered subject keys.Consider adding unit tests to confirm (1) that partially updating subject keys is correctly handled, and (2) that any disallowed reordering scenario indeed returns an error. This helps assure validations remain consistent across future changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
openmeter/subscription/service/service.go(4 hunks)openmeter/subscription/subscriptionspec.go(2 hunks)app/common/subscription.go(1 hunks)openmeter/subscription/validators/customer/validator.go(1 hunks)openmeter/subscription/subscriptionspec.go(1 hunks)openmeter/subscription/validators/customer/validator.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/common/subscription.go
- openmeter/subscription/service/service.go
- openmeter/subscription/subscriptionspec.go
- openmeter/subscription/subscriptionspec.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
openmeter/subscription/validators/customer/validator.go (2)
16-16: Add a nil check forcustomerServicein the constructor.Currently, there is no guard clause verifying that
customerServiceis not nil. Including one will prevent potential runtime errors and make the guard clause for dependencies complete.
33-72: Consider whether reordering subject keys should be allowed.Right now, a small reordering of keys triggers an error for a customer with active subscriptions, which may or may not align with business requirements. If reordering is inconsequential, use an order-agnostic comparison to avoid false negatives.
| } | ||
|
|
||
| if cust == nil { | ||
| return def, fmt.Errorf("customer is nil") |
There was a problem hiding this comment.
its different error case, err != nil handles that. this case shouldn't happen
| return def, fmt.Errorf("customer is nil") | ||
| } | ||
|
|
||
| if _, err := cust.UsageAttribution.GetSubjectKey(); err != nil { |
There was a problem hiding this comment.
Should you handle error differently to nil subject?
There was a problem hiding this comment.
The only error is it not having a value. The reason that's the only error is because GetSubjectKey is heuristic, it just selects the first subjectkey. Until subject-customer relation is not figured out properly i'd push this issue out
| if cust.Currency != nil { | ||
| if string(*cust.Currency) != string(spec.Currency) { |
There was a problem hiding this comment.
nit
| if cust.Currency != nil { | |
| if string(*cust.Currency) != string(spec.Currency) { | |
| if cust.Currency != nil && string(*cust.Currency) != string(spec.Currency) { |
Overview
Summary by CodeRabbit
SubscriptionPhaseSpecfor checking entitlements and billables, improving readability and efficiency.