fix: notification issues/test message #2763
Conversation
Fix entitlement.reset serialization issues Use billing service for test invoice creation, so that the invoice is always up to date with latest schemantics.
|
Warning Rate limit exceeded@turip has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis set of changes introduces enhancements and refactoring to the billing and notification components. The invoice simulation flow is updated to allow simulation using either an existing customer ID or a full customer object, with corresponding adjustments in data validation, service logic, and mapping to API models. Notification event handling is expanded to support a new event type, Changes
Possibly related PRs
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 4
🔭 Outside diff range comments (2)
openmeter/notification/httpdriver/mapping.go (1)
535-547: 🛠️ Refactor suggestionSecond nil-pointer risk inside payload mapper
FromEventAsEntitlementResetPayloadagain assumese.Payload.EntitlementReset
is non-nil. Either add a guard or document the pre-condition very visibly.func FromEventAsEntitlementResetPayload(e notification.Event) api.NotificationEventResetPayload { - return api.NotificationEventResetPayload{ + if e.Payload.EntitlementReset == nil { + // This should never happen – the caller is expected to validate – but protect anyway. + return api.NotificationEventResetPayload{} + } + return api.NotificationEventResetPayload{openmeter/notification/internal/rule.go (1)
94-98:⚠️ Potential issueEvent type is incorrect for entitlement-reset test payload
newTestEntitlementResetPayload()returns a payload whose meta‐type isBalanceThreshold, so callers requestingEntitlementResetreceive the wrong event type.- Type: notification.EventTypeBalanceThreshold, + Type: notification.EventTypeEntitlementReset,This will otherwise cause rule-matching logic to ignore the test event.
🧹 Nitpick comments (8)
openmeter/ent/schema/notification.go (2)
304-313: Avoid duplicate serialization logic & handlenilDatacleanlyThe two new
EntitlementResetbranches are a verbatim copy of the existing pattern.
Although that works, the switch is now getting unwieldy and easy to forget when a new
event type is added.
- Consider extracting the common
serde := ruleConfigSerde[...] { … }construction
into a helper to reduce copy-paste.- When
config.EntitlementResetisnil,json.Marshalwill emit"data":null.
If you prefer to omit empty fields (consistent with the API), add an
omitemptytag to theDatafield or make the field non-pointer in the
wrapper and set it conditionally.
358-374: Missingnil-check during deserialization could panic later
serde.Datais always initialised (¬ification.EntitlementResetRuleConfig{}), but
if the inbound JSON contains"data":nullwe end up with an empty struct instead of
nil, whereas other branches leave the pointernil.
For consistency – and to be able to distinguish “no config supplied” –
explicitly setserde.Data = nilwhen the raw json value isnull.openmeter/notification/httpdriver/mapping.go (2)
500-505: Minor: keepFromEventTypein lexicographic (or logical) orderPlacing the new case after
BalanceThresholdkeeps the list sorted and makes future
diffs cleaner.
566-599: Avoid mutating input & tighten app-mapping logic
event.Invoice.Workflow.Apps = nilmutates the argument.
If the caller re-uses the sameEventInvoiceelsewhere this could be surprising.
Create a shallow copy instead.Also, you silently ignore invalid
event.Apps.*(emptyType). Consider returning
an error if all three app objects are empty – that would signal an upstream bug
instead of emitting an invoice with an emptyappsblock.- // Prefer the apps from the event - event.Invoice.Workflow.Apps = nil + // Work on a copy so the original event is not mutated. + evCopy := event + evCopy.Invoice.Workflow.Apps = nilopenmeter/billing/httpdriver/invoice.go (1)
623-642: MapInvoiceCustomerToAPI: avoid nil map allocation on every callWhen
a == nilyou still allocateout.Addresseslater on. Early-return
avoids that extra allocation:- out := api.BillingParty{ - Id: lo.ToPtr(c.CustomerID), - Name: lo.EmptyableToPtr(c.Name), - } - - if a != nil { - out.Addresses = lo.ToPtr([]api.Address{ … }) - } - - return out + if a == nil { + return api.BillingParty{ + Id: lo.ToPtr(c.CustomerID), + Name: lo.EmptyableToPtr(c.Name), + } + } + + return api.BillingParty{ + Id: lo.ToPtr(c.CustomerID), + Name: lo.EmptyableToPtr(c.Name), + Addresses: lo.ToPtr([]api.Address{ … }), + }A micro-optimisation, but this function may be called in tight loops.
openmeter/billing/service/invoice.go (2)
1099-1112: Avoid mutating caller-suppliedLineobjects in-place
lo.Maprewrites the slice elements directly, which means any*billing.Lineinstances the caller re-uses after callingSimulateInvoicewill now contain mutated state (namespace, currency, IDs, timestamps, invoice-ID).
Defensive copying keeps the API free from surprising side-effects.- invoice.Lines = billing.NewLineChildren( - lo.Map(inputLines, func(line *billing.Line, _ int) *billing.Line { - line.Namespace = input.Namespace - ... - return line - }), - ) + invoice.Lines = billing.NewLineChildren( + lo.Map(inputLines, func(src *billing.Line, _ int) *billing.Line { + cloned := *src // shallow copy – deep-copy nested structs if needed + cloned.Namespace = input.Namespace + ... + return &cloned + }), + )
1120-1128: Duplicate per-line validation – consider removing
invoice.Validate()already validates child lines; the additional loop immediately afterwards validates each line again, doubling the work.Unless you need a different validation rule set here, you can safely drop the second pass and return the (already aggregated) issues from
invoice.Validate().openmeter/notification/internal/rule.go (1)
165-176: Preferclock.Now()for deterministic timestamps in testsUsing wall-clock time makes golden-file / snapshot tests flaky.
The billing service already depends on theclockpackage – re-use it here for the same reasons explained in the billing comment.- now := time.Now().Truncate(time.Second).In(time.UTC) + now := clock.Now().Truncate(time.Second).In(time.UTC)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
openmeter/billing/httpdriver/invoice.go(4 hunks)openmeter/billing/invoice.go(1 hunks)openmeter/billing/service/invoice.go(5 hunks)openmeter/ent/schema/notification.go(2 hunks)openmeter/notification/consumer/entitlementsnapshot.go(2 hunks)openmeter/notification/httpdriver/handler.go(3 hunks)openmeter/notification/httpdriver/mapping.go(3 hunks)openmeter/notification/httpdriver/rule.go(1 hunks)openmeter/notification/internal/rule.go(3 hunks)openmeter/server/router/router.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
openmeter/notification/httpdriver/rule.go (3)
openmeter/notification/internal/rule.go (1)
EventGeneratorInput(33-37)openmeter/notification/event.go (2)
EventType(26-26)CreateEventInput(136-148)pkg/models/model.go (1)
NamespacedModel(180-182)
openmeter/server/router/router.go (1)
app/common/billing.go (1)
Billing(25-28)
openmeter/ent/schema/notification.go (2)
openmeter/notification/entitlements.go (2)
EventTypeEntitlementReset(12-12)EntitlementResetRuleConfig(118-120)openmeter/notification/rule.go (2)
RuleConfigMeta(71-73)RuleConfig(80-89)
openmeter/notification/httpdriver/mapping.go (4)
openmeter/notification/entitlements.go (1)
EventTypeEntitlementReset(12-12)api/api.gen.go (4)
NotificationEventTypeEntitlementsReset(559-559)Entitlement(2497-2499)Feature(2946-2975)Subject(6290-6307)api/client/go/client.gen.go (4)
NotificationEventTypeEntitlementsReset(516-516)Entitlement(2297-2299)Feature(2714-2743)Subject(5757-5774)api/client/javascript/src/client/schemas.ts (3)
Entitlement(10367-10367)Feature(10392-10392)Subject(10676-10676)
🔇 Additional comments (11)
openmeter/server/router/router.go (1)
293-298: Properly injected billing service for notification event generation.The notification handler now accepts the billing service as a dependency, which is needed for generating test events with realistic invoice payloads. This integration allows for more accurate notification simulation during testing.
openmeter/notification/consumer/entitlementsnapshot.go (3)
5-5: Added fmt import for error wrapping.This import is needed for the improved error handling below.
19-21: Improved error context for better traceability.The error from
handleAsSnapshotEventis now properly wrapped with contextual information, making debugging easier when issues occur.
25-27: Improved error context for better traceability.The error from
handleAsEntitlementResetEventis now properly wrapped with contextual information, making debugging easier when issues occur.openmeter/notification/httpdriver/rule.go (2)
318-324: Improved test event generation with dynamic payload creation.The code now uses the new
testEventGeneratorto dynamically generate test event payloads based on the rule type and namespace, replacing the previous static payload approach. This change makes test events more realistic and consistent with actual production events.
329-329: Updated to use dynamically generated test event.The code now uses the dynamically generated test event as the payload for the created notification event.
openmeter/notification/httpdriver/handler.go (5)
8-8: Added billing service import.This import enables the notification handler to access the billing service functionality.
11-11: Updated internal package import.This change ensures access to the new test event generation functionality implemented in the internal package.
47-50: Added test event generator field to handler struct.The handler now includes a
testEventGeneratorfield, which will be used to dynamically generate test event payloads for notification rules.
65-65: Added billing service parameter to constructor.The notification handler constructor now accepts a billing service parameter, which is necessary for initializing the test event generator.
69-73: Updated handler initialization with test event generator.The handler now properly initializes the test event generator using the provided billing service, enabling dynamic generation of test notification events.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
openmeter/notification/httpdriver/mapping.go (1)
459-470: Add more diagnostic context to error wrappingGreat job extracting the payload conversion into
FromEventAsBalanceThresholdPayloadand guarding against a nil payload – that removes the panic path.
To make troubleshooting easier when this branch still fails (e.g. malformed event coming from storage), consider injecting the event ID and type into the wrapped error before bubbling it up. This helps ops trace the exact record that triggered the failure without having to add extra logs later.- return event, fmt.Errorf("failed to cast notification event payload: %w", err) + return event, fmt.Errorf("failed to cast balance-threshold payload for event %s (%s): %w", + e.ID, e.Type, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openmeter/billing/service/invoice.go(5 hunks)openmeter/notification/httpdriver/mapping.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/billing/service/invoice.go
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: CI
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Developer environment
- GitHub Check: Commit hooks
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
openmeter/notification/httpdriver/mapping.go (1)
513-515: 👍 Correct event-type mapping addedMapping
notification.EventTypeEntitlementResettoapi.NotificationEventTypeEntitlementsResetfills the missing link noted in previous reviews. Looks good.
turip
force-pushed
the
fix/notification-errors
branch
from
d07aa57 to
8317e57
Compare
2 participants
Overview
Fix entitlement.reset serialization issues
When apps are not present in the eventinvoice let's not throw an error, instead send empty apps (schema allows)
Use billing service for test invoice creation, so that the invoice is always up to date with latest schemantics.
Summary by CodeRabbit