refactor: use namespacedID for notification resources #3547
Conversation
chrisgacsal
added
the
release-note/ignore
📝 WalkthroughWalkthroughThe PR refactors the notification system to replace the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
openmeter/notification/channel.go (1)
192-207: Delegate identifier validation to NamespacedID.Validate for consistency.Reuse the shared validator instead of manual checks.
Apply these diffs:
func (i UpdateChannelInput) Validate() error { var errs []error - if i.Namespace == "" { - errs = append(errs, errors.New("namespace is required")) - } - - if i.ID == "" { - errs = append(errs, errors.New("id is required")) - } + if err := i.NamespacedID.Validate(); err != nil { + errs = append(errs, err) + }func (i GetChannelInput) Validate() error { var errs []error - - if i.Namespace == "" { - errs = append(errs, errors.New("namespace is required")) - } - - if i.ID == "" { - errs = append(errs, errors.New("id is required")) - } + if err := (models.NamespacedID)(i).Validate(); err != nil { + errs = append(errs, err) + } return models.NewNillableGenericValidationError(errors.Join(errs...)) }Also applies to: 213-236, 250-261
openmeter/notification/eventhandler/reconcile.go (1)
111-113: Clarify variable naming; fix misleading error text.We iterate events, not delivery statuses. Rename the loop var and adjust the earlier error message.
Apply:
- if err != nil { - return fmt.Errorf("failed to fetch notification delivery statuses for reconciliation: %w", err) - } + if err != nil { + return fmt.Errorf("failed to fetch notification events for reconciliation: %w", err) + }- for _, deliveryStatus := range out.Items { - if err = h.reconcileEvent(ctx, &deliveryStatus); err != nil { + for _, event := range out.Items { + if err = h.reconcileEvent(ctx, &event); err != nil { errs = append(errs, fmt.Errorf("failed to reconcile notification event [namespace=%s event.id=%s]: %w", - deliveryStatus.Namespace, deliveryStatus.ID, err), + event.Namespace, event.ID, err), ) } }Also applies to: 119-124
openmeter/notification/adapter/entitymapping.go (1)
85-87: NIT: "deserialize" instead of "serialize".We are unmarshalling JSON.
- return nil, fmt.Errorf("failed to serialize notification event payload: %w", err) + return nil, fmt.Errorf("failed to deserialize notification event payload: %w", err)openmeter/notification/eventhandler/dispatch.go (1)
104-111: Align sent channel IDs with filtered webhook channels.We filter delivery statuses to webhook channels but still send with event.Rule.ID. Likely mismatch. Prefer sending to the actual webhook channel IDs we’ve computed.
Apply after building the map:
webhookChannelIDMap := lo.FilterSliceToMap(event.Rule.Channels, func(item notification.Channel) (string, struct{}, bool) { if item.Type == notification.ChannelTypeWebhook { return item.ID, struct{}{}, true } return "", struct{}{}, false }) + + // Use the same set for sending + sendIn.Channels = lo.Keys(webhookChannelIDMap) + if len(sendIn.Channels) == 0 { + return nil // nothing to dispatch via webhook + }Please confirm webhook.SendMessageInput.Channels expects channel IDs (not rule IDs). If it does expect rule IDs, consider renaming the field or adding a comment to avoid confusion.
Also applies to: 112-121, 123-128
openmeter/notification/rule.go (1)
42-44: Refactor UpdateRuleInput.Validate() to use NamespacedID.Validate() for consistency.Verification confirms NamespacedID.Validate() is available and Rule.Validate() uses it correctly. However, UpdateRuleInput.Validate() still contains duplicate manual checks (lines 250–256, openmeter/notification/rule.go) instead of delegating to the embedded NamespacedID.Validate() method. This creates inconsistency. Replace the manual Namespace and ID validation with a call to i.NamespacedID.Validate() to align with Rule's approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
openmeter/notification/adapter/deliverystatus.go(1 hunks)openmeter/notification/adapter/entitymapping.go(4 hunks)openmeter/notification/channel.go(1 hunks)openmeter/notification/deliverystatus.go(3 hunks)openmeter/notification/event.go(1 hunks)openmeter/notification/eventhandler/dispatch.go(2 hunks)openmeter/notification/eventhandler/reconcile.go(1 hunks)openmeter/notification/rule.go(2 hunks)test/notification/event.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
openmeter/notification/channel.go (1)
pkg/models/id.go (1)
NamespacedID(7-10)
openmeter/notification/event.go (1)
pkg/models/id.go (1)
NamespacedID(7-10)
test/notification/event.go (1)
pkg/models/id.go (1)
NamespacedID(7-10)
openmeter/notification/adapter/entitymapping.go (5)
pkg/models/id.go (1)
NamespacedID(7-10)openmeter/ent/db/notificationchannel/where.go (2)
Namespace(70-72)ID(15-17)openmeter/ent/db/notificationevent/where.go (2)
Namespace(70-72)ID(15-17)openmeter/ent/db/notificationeventdeliverystatus/where.go (2)
Namespace(70-72)ID(15-17)openmeter/ent/db/notificationrule/where.go (2)
Namespace(70-72)ID(15-17)
openmeter/notification/adapter/deliverystatus.go (3)
openmeter/ent/db/notificationeventdeliverystatus.go (2)
NotificationEventDeliveryStatus(19-43)NotificationEventDeliveryStatus(64-79)pkg/models/id.go (1)
NamespacedID(7-10)openmeter/notification/adapter/entitymapping.go (1)
EventDeliveryStatusFromDBEntity(121-136)
openmeter/notification/rule.go (1)
pkg/models/id.go (1)
NamespacedID(7-10)
openmeter/notification/eventhandler/dispatch.go (4)
openmeter/notification/rule.go (1)
Rule(17-33)openmeter/notification/channel.go (2)
Channel(37-51)ChannelTypeWebhook(14-14)openmeter/notification/deliverystatus.go (1)
UpdateEventDeliveryStatusInput(148-157)pkg/models/id.go (1)
NamespacedID(7-10)
openmeter/notification/deliverystatus.go (1)
pkg/models/id.go (1)
NamespacedID(7-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Artifacts / Container image
- GitHub Check: Lint
- GitHub Check: Code Generators
- GitHub Check: Build
- GitHub Check: Test
- GitHub Check: Migration Checks
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
test/notification/event.go (1)
344-347: LGTM on switching to NamespacedID in test input.Matches the new update path and mapping.
openmeter/notification/event.go (1)
51-53: LGTM on embedding NamespacedID in Event.Public model is now consistent across notification entities.
openmeter/notification/deliverystatus.go (3)
49-49: LGTM! Clean refactoring to NamespacedID.The change from
models.NamespacedModeltomodels.NamespacedIDsimplifies the struct while maintaining all necessary identification fields.
166-168: LGTM! Cleaner validation approach.Delegating to
NamespacedID.Validate()centralizes the validation logic and eliminates redundant manual checks. This is a cleaner and more maintainable approach.
148-157: No issues found—all callers properly use the ID field.Verification confirms both instantiations of
UpdateEventDeliveryStatusInputin the codebase correctly set theIDfield through the embeddedNamespacedID:
test/notification/event.go:343: SetsNamespacedIDwith delivery status IDopenmeter/notification/eventhandler/dispatch.go:123: SetsNamespacedIDfrom existing delivery statusNo lingering references to the old
EventIDorChannelIDpattern exist. All interface signatures and implementations are consistent. The refactoring is complete.
2 participants
Overview
Use
models.NamespacedIDfor notification resource entities.Summary by CodeRabbit