refactor: notification validators - #3529
Conversation
chrisgacsal
added
the
release-note/ignore
📝 WalkthroughWalkthroughMultiple notification package refactors: migrate validations to models.Validator/models.GenericValidationError patterns (add Validate/ValidateWith across inputs), switch several Update/Get inputs to use NamespacedID or top-level Namespace+ID, add runtime rule/config validation, and adjust event ordering default to ID. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)openmeter/notification/service/channel.go (3)
⏰ 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). (8)
🔇 Additional comments (5)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
test/notification/event.go (3)
232-233: Fix misleading assertion messages in TestGetEventUse “event” wording.
- require.NoError(t, err, "Creating rule must not return error") - require.NotNil(t, event, "Rule must not be nil") + require.NoError(t, err, "Creating event must not return error") + require.NotNil(t, event, "Event must not be nil")
273-276: Test asserts the wrong variable; may mask failuresYou’re checking ‘event’ instead of ‘events’.
- events, err := service.ListEvents(ctx, listIn) - require.NoError(t, err, "Listing notification events must not return error") - require.NotNil(t, event, "Notification events must not be nil") + events, err := service.ListEvents(ctx, listIn) + require.NoError(t, err, "Listing notification events must not return error") + require.NotNil(t, events, "Notification events must not be nil")
307-310: Same issue: wrong NotNil target in TestListDeliveryStatusAsserting ‘event’ instead of ‘statuses’.
- statuses, err := service.ListEvents(ctx, listIn) - require.NoError(t, err, "Listing notification event delivery statuses must not return error") - require.NotNil(t, event, "Notification event delivery statuses must not be nil") + statuses, err := service.ListEvents(ctx, listIn) + require.NoError(t, err, "Listing notification event delivery statuses must not return error") + require.NotNil(t, statuses, "Notification event delivery statuses must not be nil")test/notification/rule.go (2)
161-169: Wrong namespace passed to NewCreateRuleInput across testsYou’re passing a name where a namespace is expected, which can break creation and listing.
- createIn1 := NewCreateRuleInput("NotificationListRule1", s.channel.ID) + createIn1 := NewCreateRuleInput(s.Env.Namespace(), "NotificationListRule1", s.channel.ID) - createIn2 := NewCreateRuleInput("NotificationListRule2", s.channel.ID) + createIn2 := NewCreateRuleInput(s.Env.Namespace(), "NotificationListRule2", s.channel.ID) - createIn := NewCreateRuleInput("NotificationUpdateRule1", s.channel.ID) + createIn := NewCreateRuleInput(s.Env.Namespace(), "NotificationUpdateRule1", s.channel.ID) - createIn := NewCreateRuleInput("NotificationDeleteRule1", s.channel.ID) + createIn := NewCreateRuleInput(s.Env.Namespace(), "NotificationDeleteRule1", s.channel.ID) - createIn := NewCreateRuleInput("NotificationGetRule1", s.channel.ID) + createIn := NewCreateRuleInput(s.Env.Namespace(), "NotificationGetRule1", s.channel.ID)Also applies to: 204-208, 243-249, 260-263
278-281: Fix self-equality assertions in TestGetYou’re comparing a value to itself; compare to rule2.
- assert.Equal(t, rule.Type, rule.Type, "Rule type must be the same") - assert.Equal(t, rule.Channels, rule.Channels, "Rule channels must be the same") - assert.EqualValues(t, rule.Config, rule.Config, "Rule config must be the same") + assert.Equal(t, rule.Type, rule2.Type, "Rule type must be the same") + assert.EqualValues(t, rule.Channels, rule2.Channels, "Rule channels must be the same") + assert.EqualValues(t, rule.Config, rule2.Config, "Rule config must be the same")test/notification/channel.go (1)
124-126: Tidy up assertion messages; add name assertionMinor message fixes and verify updated name.
- channel2, err := service.UpdateChannel(ctx, updateIn) - require.NoError(t, err, "Creating channel must not return error") + channel2, err := service.UpdateChannel(ctx, updateIn) + require.NoError(t, err, "Updating channel must not return error") require.NotNil(t, channel2, "Channel must not be nil") + assert.Equal(t, updateIn.Name, channel2.Name, "Channel name must match") - channel2, err := service.GetChannel(ctx, notification.GetChannelInput{ + channel2, err := service.GetChannel(ctx, notification.GetChannelInput{ Namespace: channel.Namespace, ID: channel.ID, }) - require.NoError(t, err, "Deleting channel must not return error") + require.NoError(t, err, "Getting channel must not return error") require.NotNil(t, channel2, "Channel must not be nil") assert.NotEmpty(t, channel2.ID, "Channel ID must not be empty") assert.Equal(t, channel.Namespace, channel2.Namespace, "Channel namespace must be equal") assert.Equal(t, channel.ID, channel2.ID, "Channel ID must be equal") - assert.Equal(t, channel.Disabled, channel2.Disabled, "Channel disabled must not be equal") + assert.Equal(t, channel.Disabled, channel2.Disabled, "Channel disabled must match")Also applies to: 162-163, 167-169
openmeter/notification/service/channel.go (2)
199-202: Wrong error message: says create on update pathChange to “failed to update channel”.
- return nil, fmt.Errorf("failed to create channel: %w", err) + return nil, fmt.Errorf("failed to update channel: %w", err)
85-86: Align invalid type errors with validation error schemeReturn a GenericValidationError for invalid channel type to match delete-path semantics.
- return nil, fmt.Errorf("invalid channel type: %s", channel.Type) + return nil, models.NewGenericValidationError( + fmt.Errorf("invalid channel type: %s", channel.Type), + )And similarly in UpdateChannel’s default branch.
Also applies to: 227-229
openmeter/notification/httpdriver/event.go (1)
114-116: Clarify error message on nil eventThis endpoint “gets” events, not “creates”.
- return GetEventResponse{}, errors.New("failed to create test event: nil event returned") + return GetEventResponse{}, errors.New("failed to get event: nil event returned")openmeter/notification/service/rule.go (2)
47-63: External side effects inside transaction can leave inconsistent stateUpdateWebhookChannels is called inside the DB transaction. If the transaction later rolls back, webhook changes may persist, causing drift. Prefer: commit DB first then apply webhook updates (with retries/outbox), or use a post‑commit hook.
184-206: Update: external side effects before DB updateWebhook channel updates happen before s.adapter.UpdateRule. If UpdateRule fails, webhooks are already mutated. Apply after a successful commit or via outbox to avoid divergence.
openmeter/notification/channel.go (1)
3-11: Fix import path and add net/url. Build currently breaks.The webhook import path includes an extra “openmeter” segment and fails to resolve. Also add net/url for URL validation (see next comment).
Apply this diff:
import ( "errors" "fmt" + "net/url" - "github.com/openmeterio/openmeter/openmeter/notification/webhook" + "github.com/openmeterio/openmeter/notification/webhook" "github.com/openmeterio/openmeter/pkg/models" "github.com/openmeterio/openmeter/pkg/pagination" "github.com/openmeterio/openmeter/pkg/sortx" )
🧹 Nitpick comments (9)
openmeter/notification/service/channel.go (1)
67-79: Optional: validate synthesized updateIn before adapter callDefensive check; keeps invariants even when upstream changes occur.
updateIn := notification.UpdateChannelInput{ NamespacedID: models.NamespacedID{ Namespace: params.Namespace, ID: channel.ID, }, Type: channel.Type, Name: channel.Name, Disabled: channel.Disabled, Config: channel.Config, } updateIn.Config.WebHook.SigningSecret = wb.Secret + if err := updateIn.Validate(); err != nil { + return nil, fmt.Errorf("invalid synthesized update payload: %w", err) + }openmeter/notification/httpdriver/mapping.go (1)
503-516: Optional: unify error type from FromEventType defaultFor consistency, consider returning models.NewGenericValidationError on invalid event type here too.
- default: - return "", fmt.Errorf("invalid notification event type: %s", t) + default: + return "", models.NewGenericValidationError(fmt.Errorf("invalid notification event type: %s", t))openmeter/notification/event.go (2)
166-178: CreateEventInput: consider basic required-field checksAdd namespace and ruleId presence checks to catch obvious mistakes early.
func (i CreateEventInput) Validate() error { var errs []error if err := i.Type.Validate(); err != nil { errs = append(errs, err) } + if i.Namespace == "" { + errs = append(errs, errors.New("namespace is required")) + } + if i.RuleID == "" { + errs = append(errs, errors.New("ruleId is required")) + } return models.NewNillableGenericValidationError(errors.Join(errs...)) }
22-24: Optional: return a copy from EventTypes()Avoid exposing the backing slice to callers to prevent accidental mutation.
-func EventTypes() []EventType { - return eventTypes -} +func EventTypes() []EventType { + out := make([]EventType, len(eventTypes)) + copy(out, eventTypes) + return out +}openmeter/notification/rule.go (3)
39-63: Add MaxChannelsPerRule parity to Rule.Validate.Create/Update inputs enforce MaxChannelsPerRule; Rule itself should mirror this for consistency (e.g., on upserts/backfills).
Apply this diff:
func (r Rule) Validate() error { var errs []error @@ if err := r.Config.Validate(); err != nil { errs = append(errs, err) } + + if len(r.Channels) > MaxChannelsPerRule { + errs = append(errs, fmt.Errorf("too many channels: %d > %d", len(r.Channels), MaxChannelsPerRule)) + } return models.NewNillableGenericValidationError(errors.Join(errs...)) }
193-217: Reject duplicate/empty channel IDs in CreateRuleInput.Prevent duplicates and empty IDs to avoid ambiguous routing and noisy rules.
Apply this diff:
func (i CreateRuleInput) Validate() error { var errs []error @@ if err := i.Config.Validate(); err != nil { errs = append(errs, err) } + + // Enforce unique, non-empty channels + seen := make(map[string]struct{}, len(i.Channels)) + for _, ch := range i.Channels { + if ch == "" { + errs = append(errs, errors.New("channel id must not be empty")) + continue + } + if _, ok := seen[ch]; ok { + errs = append(errs, fmt.Errorf("duplicate channel: %s", ch)) + continue + } + seen[ch] = struct{}{} + } if len(i.Channels) > MaxChannelsPerRule { errs = append(errs, fmt.Errorf("too many channels: %d > %d", len(i.Channels), MaxChannelsPerRule)) }
243-271: Mirror duplicate/empty channel ID checks in UpdateRuleInput.Same rationale as Create: avoid duplicate/empty channel references on updates.
Apply this diff:
func (i UpdateRuleInput) Validate() error { var errs []error @@ if err := i.Config.Validate(); err != nil { errs = append(errs, err) } + + // Enforce unique, non-empty channels + seen := make(map[string]struct{}, len(i.Channels)) + for _, ch := range i.Channels { + if ch == "" { + errs = append(errs, errors.New("channel id must not be empty")) + continue + } + if _, ok := seen[ch]; ok { + errs = append(errs, fmt.Errorf("duplicate channel: %s", ch)) + continue + } + seen[ch] = struct{}{} + } if len(i.Channels) > MaxChannelsPerRule { errs = append(errs, fmt.Errorf("too many channels: %d > %d", len(i.Channels), MaxChannelsPerRule)) }openmeter/notification/channel.go (2)
96-111: Validate webhook URL scheme and host.Avoid accepting malformed/unsupported URLs; require http/https and a host.
Apply this diff:
func (w WebHookChannelConfig) Validate() error { var errs []error if w.URL == "" { errs = append(errs, errors.New("missing URL")) + } else { + u, err := url.ParseRequestURI(w.URL) + if err != nil { + errs = append(errs, fmt.Errorf("invalid URL: %v", err)) + } else { + if u.Scheme != "http" && u.Scheme != "https" { + errs = append(errs, errors.New("invalid URL scheme: must be http or https")) + } + if u.Host == "" { + errs = append(errs, errors.New("invalid URL: host is required")) + } + } }
256-259: Align CustomValidator generic parameter with the alias type.Use DeleteChannelInput for consistency with DeleteRuleInput and clearer intent. Behavior is identical (type alias), but this improves readability.
Apply this diff:
var ( _ models.Validator = (*DeleteChannelInput)(nil) - _ models.CustomValidator[GetChannelInput] = (*DeleteChannelInput)(nil) + _ models.CustomValidator[DeleteChannelInput] = (*DeleteChannelInput)(nil) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
openmeter/ent/db/notificationrule_create.go(1 hunks)openmeter/ent/db/notificationrule_update.go(2 hunks)openmeter/notification/adapter/event.go(1 hunks)openmeter/notification/channel.go(9 hunks)openmeter/notification/deliverystatus.go(6 hunks)openmeter/notification/entitlements.go(5 hunks)openmeter/notification/errors.go(0 hunks)openmeter/notification/event.go(6 hunks)openmeter/notification/eventpayload.go(2 hunks)openmeter/notification/httpdriver/errors.go(1 hunks)openmeter/notification/httpdriver/event.go(1 hunks)openmeter/notification/httpdriver/mapping.go(8 hunks)openmeter/notification/invoice.go(2 hunks)openmeter/notification/rule.go(7 hunks)openmeter/notification/service/channel.go(5 hunks)openmeter/notification/service/deliverystatus.go(2 hunks)openmeter/notification/service/event.go(2 hunks)openmeter/notification/service/rule.go(5 hunks)openmeter/notification/validator.go(0 hunks)test/notification/channel.go(1 hunks)test/notification/event.go(1 hunks)test/notification/repository.go(1 hunks)test/notification/rule.go(1 hunks)
💤 Files with no reviewable changes (2)
- openmeter/notification/errors.go
- openmeter/notification/validator.go
🧰 Additional context used
🧬 Code graph analysis (17)
openmeter/notification/eventpayload.go (2)
pkg/models/errors.go (1)
NewGenericValidationError(138-140)pkg/models/validator.go (1)
Validate(16-26)
openmeter/ent/db/notificationrule_update.go (1)
pkg/models/validator.go (1)
Validate(16-26)
openmeter/notification/deliverystatus.go (3)
pkg/models/errors.go (2)
NewGenericValidationError(138-140)NewNillableGenericValidationError(129-135)pkg/models/validator.go (3)
CustomValidator(12-14)ValidatorFunc(10-10)Validate(16-26)pkg/models/id.go (1)
NamespacedID(7-10)
test/notification/rule.go (2)
pkg/models/id.go (1)
NamespacedID(7-10)openmeter/ent/db/notificationrule/where.go (4)
Namespace(70-72)ID(15-17)Name(90-92)Disabled(95-97)
openmeter/notification/service/event.go (2)
pkg/models/validator.go (1)
Validate(16-26)pkg/models/errors.go (1)
NewGenericValidationError(138-140)
openmeter/notification/invoice.go (1)
pkg/models/validator.go (3)
CustomValidator(12-14)ValidatorFunc(10-10)Validate(16-26)
openmeter/notification/service/rule.go (3)
pkg/models/validator.go (1)
Validate(16-26)openmeter/notification/service/service.go (2)
Service(20-29)Config(31-39)openmeter/notification/entitlements.go (1)
ValidateRuleConfigWithFeatures(118-169)
test/notification/repository.go (1)
openmeter/notification/entitlements.go (3)
BalanceThreshold(68-68)BalanceThresholdRuleConfig(77-82)BalanceThresholdTypeNumber(59-59)
openmeter/notification/httpdriver/errors.go (3)
pkg/framework/transport/httptransport/encoder/encoder.go (1)
ErrorEncoder(14-14)pkg/framework/commonhttp/errors.go (1)
HandleErrorIfTypeMatches(64-77)pkg/models/errors.go (1)
GenericValidationError(145-147)
openmeter/notification/adapter/event.go (3)
pkg/sortx/order.go (1)
Order(3-3)openmeter/ent/db/notificationevent/notificationevent.go (2)
ByCreatedAt(117-119)ByID(107-109)openmeter/notification/service.go (1)
OrderByID(10-10)
openmeter/notification/rule.go (3)
pkg/models/validator.go (3)
CustomValidator(12-14)ValidatorFunc(10-10)Validate(16-26)pkg/models/errors.go (2)
NewNillableGenericValidationError(129-135)NewGenericValidationError(138-140)pkg/models/id.go (1)
NamespacedID(7-10)
openmeter/notification/service/channel.go (3)
openmeter/notification/channel.go (4)
ListChannelsInput(118-127)CreateChannelInput(144-155)Channel(37-51)UpdateChannelInput(188-199)pkg/models/id.go (1)
NamespacedID(7-10)pkg/models/errors.go (1)
NewGenericValidationError(138-140)
openmeter/notification/event.go (3)
pkg/models/errors.go (2)
NewGenericValidationError(138-140)NewNillableGenericValidationError(129-135)pkg/models/validator.go (3)
CustomValidator(12-14)ValidatorFunc(10-10)Validate(16-26)pkg/models/id.go (1)
NamespacedID(7-10)
test/notification/channel.go (1)
pkg/models/id.go (1)
NamespacedID(7-10)
openmeter/notification/httpdriver/mapping.go (4)
pkg/models/errors.go (1)
NewGenericValidationError(138-140)openmeter/notification/channel.go (1)
UpdateChannelInput(188-199)pkg/models/id.go (1)
NamespacedID(7-10)openmeter/notification/rule.go (1)
UpdateRuleInput(224-237)
openmeter/notification/entitlements.go (4)
pkg/models/validator.go (3)
CustomValidator(12-14)ValidatorFunc(10-10)Validate(16-26)pkg/models/errors.go (2)
NewNillableGenericValidationError(129-135)NewGenericValidationError(138-140)openmeter/notification/service.go (1)
FeatureService(51-53)openmeter/notification/rule.go (1)
RuleConfig(98-107)
openmeter/notification/channel.go (4)
pkg/models/errors.go (2)
NewGenericValidationError(138-140)NewNillableGenericValidationError(129-135)openmeter/notification/webhook/secret.go (1)
ValidateSigningSecret(30-43)pkg/models/validator.go (3)
CustomValidator(12-14)ValidatorFunc(10-10)Validate(16-26)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). (8)
- GitHub Check: Artifacts / Container image
- GitHub Check: Test
- GitHub Check: Code Generators
- GitHub Check: Build
- GitHub Check: Lint
- GitHub Check: Migration Checks
- GitHub Check: Repository Scan
- GitHub Check: Analyze (go)
🔇 Additional comments (44)
openmeter/ent/db/notificationrule_create.go (1)
241-245: LGTM: Runtime validation added for config field.The validation correctly calls
Validate()on the config value and wraps any errors appropriately. This ensures invalid configurations are caught before database persistence.test/notification/repository.go (1)
60-68: LGTM: Test data updated to match new BalanceThresholdRuleConfig structure.The test correctly instantiates a
BalanceThresholdRuleConfigwith the requiredFeaturesandThresholdsfields, aligning with the refactored validation interfaces.openmeter/ent/db/notificationrule_update.go (1)
228-232: LGTM: Runtime validation added for config field on updates.The validation mirrors the create-path logic and ensures config values are validated during updates as well. Consistent error handling.
openmeter/notification/httpdriver/errors.go (1)
19-19: LGTM: Error type updated to generic validation error.The change from
notification.ValidationErrorto*models.GenericValidationErroraligns with the PR's objective to use a common validation error type across the notification package.openmeter/notification/eventpayload.go (1)
46-68: LGTM: Validation errors migrated to generic validation error type.All validation error returns correctly updated to use
models.NewGenericValidationError, consistent with the PR's refactoring objectives.openmeter/notification/service/event.go (4)
14-14: LGTM: Validation simplified to remove context and service parameters.The updated validation signature aligns with the refactored validator interface that no longer requires context-dependent validation.
22-22: LGTM: Validation simplified to remove context and service parameters.
30-30: LGTM: Validation simplified to remove context and service parameters.
60-60: LGTM: Disabled rule error migrated to generic validation error.The error correctly uses
models.NewGenericValidationErrorfor the disabled rule scenario, consistent with the refactoring approach.openmeter/notification/service/deliverystatus.go (3)
12-12: LGTM: Validation simplified to remove context and service parameters.The updated validation calls align with the refactored validator interface across all delivery status operations.
24-24: LGTM: Validation simplified to remove context and service parameters.
32-32: LGTM: Validation simplified to remove context and service parameters.openmeter/notification/adapter/event.go (1)
88-94: I need to verify the original implementation and understand the impact on production callers.Let me search the codebase more deeply to understand the impact:
Based on the script output analysis, I can now generate the rewritten review comment:
Verify the impact of default ordering change on reconciliation logic.
The refactor changes default event ordering from
CreatedAttoIDwhenOrderByis unspecified. Three production callers don't explicitly specifyOrderByand will be affected:
openmeter/notification/eventhandler/reconcile.go:71– fetches events for reconciliation without ordering specificationopenmeter/notification/consumer/entitlementreset.go:66– fetches last event with pagination but no explicit orderingopenmeter/notification/consumer/entitlementbalancethreshold.go:101– fetches last event with pagination but no explicit orderingConfirm whether ID-based ordering is semantically equivalent for these use cases (particularly for reconciliation, which may require consistent ordering for deterministic results). If
CreatedAtordering is required, update these callers to explicitly specifyOrderBy: notification.OrderByCreatedAt.test/notification/event.go (1)
248-250: API shape update to GetEventInput looks goodSwitch to top-level Namespace/ID is correct and matches the broader refactor.
test/notification/rule.go (1)
210-216: UpdateRuleInput: move to NamespacedID is correctThe new identification pattern (Namespace + ID) is consistent with the refactor.
test/notification/channel.go (1)
105-112: UpdateChannelInput: NamespacedID migration looks goodThe new identification shape is correct and aligns with production changes.
openmeter/notification/service/channel.go (3)
17-19: Good: input validation gates addedValidate() before service actions improves safety and error hygiene.
Also applies to: 25-27, 95-97, 148-150, 156-158
68-76: Good: internal UpdateChannelInput now uses NamespacedIDConsistent with public API and avoids mixed identification forms.
126-130: Good: use GenericValidationError for delete-guardWrapping the “assigned to rules” case as a validation error is appropriate.
openmeter/notification/httpdriver/event.go (1)
102-104: GetEvent: request construction update is correctUsing top-level Namespace/ID matches the new API.
openmeter/notification/service/rule.go (8)
17-19: Good shift to param self-validationUsing params.Validate() simplifies service logic and unifies error handling.
25-27: Create: param validation LGTMConsistent "invalid params: %w" wrapping matches the new convention.
35-38: Config validation placement is correct; ensure type coherenceValidating with ValidateRuleConfigWithFeatures before create is good. Please confirm params.Type matches params.Config.Type (or that params.Validate() enforces it).
72-74: Delete: param validation LGTM
106-109: Get: param validation LGTM
115-117: Update: param validation LGTM
126-129: Update: config validation placement is goodEarly feature check avoids wasted work later.
77-80: ****
DeleteRuleInputis a type alias toGetRuleInput(defined astype DeleteRuleInput = GetRuleInput), making them the same type at compile time. Go treats type aliases as fully interchangeable with their underlying type, so passingparamsof typeDeleteRuleInputtoGetRulewhich expectsGetRuleInputis valid and will compile without errors. No type mismatch exists.Likely an incorrect or invalid review comment.
openmeter/notification/invoice.go (1)
15-18: Validator conformance and no-op Validate are appropriateCompile-time assertions and delegating ValidateWith to models.Validate look good; a nil Validate is fine for an empty config.
Also applies to: 22-24, 26-27
openmeter/notification/httpdriver/mapping.go (4)
22-23: Consistent validation error typeReturning models.NewGenericValidationError for invalid channel type aligns with the new error model.
115-118: Rules update mappers: NamespacedID adoption LGTMUniform use of NamespacedID for rule update inputs looks correct.
Also applies to: 157-160, 194-201, 231-236
278-279: Generic validation error for invalid rule typeGood consistency with the new validation model.
497-498: Generic validation error for invalid event payload typeMatches the overall error strategy.
openmeter/notification/deliverystatus.go (5)
35-36: Enum validation uses generic validation errorGood alignment with models.GenericValidationError.
63-66: Validator conformance for ListEventsDeliveryStatusInputAssertions look correct.
89-107: List validation: aggregated errorsCollecting and joining errors with NewNillableGenericValidationError is correct.
125-141: Get validation: NamespacedID + aggregated checksClear messages and nil-able generic error are good.
163-183: Update validation: ID pair logic and state checkValidation is complete and consistent.
openmeter/notification/event.go (4)
26-29: Compile-time conformance for EventTypeGood practice to assert fmt.Stringer and Validator.
37-43: EventType.Validate returns generic validation errorMatches new validation model.
100-118: ListEventsInput: error aggregation is cleanTime range and OrderBy checks look good.
127-145: GetEventInput as NamespacedID-derived typeValidation is straightforward; aggregated error pattern is consistent.
openmeter/notification/rule.go (1)
39-63: Aggregated validation pattern is solid.Good switch to collecting field errors and returning a single models.GenericValidationError via NewNillableGenericValidationError. This improves UX and keeps validators composable.
openmeter/notification/entitlements.go (1)
88-116: Confirm valid bounds for percent thresholds.Should percent-based thresholds be bounded (e.g., 0 < value ≤ 100)? If yes, add an upper-bound check to prevent >100.
Would you like me to add the guard once confirmed?
2 participants
Overview
Reafctor validators in
notificationpackage to use commonValidationErrorand to supportValidatorandCustomValidatorinterfaces.Summary by CodeRabbit
Bug Fixes
Improvements
Tests