feat(notification): add annotations and metadata fields #3533
Conversation
📝 WalkthroughWalkthroughAdds system-managed read-only Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (9)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/notification/channel.go (2)
143-143: Fix assertion message (update).Message mentions create instead of update.
- require.NoError(t, err, "Creating channel must not return error") + require.NoError(t, err, "Updating channel must not return error")
182-182: Fix assertion message (get).Message mentions delete instead of get.
- require.NoError(t, err, "Deleting channel must not return error") + require.NoError(t, err, "Getting channel must not return error")
♻️ Duplicate comments (5)
openmeter/ent/db/mutation.go (5)
39055-39055: Same pointer-to-map note applies hereSame aliasing risk as hunk 1 for NotificationEventMutation.annotations.
39607-39613: Match SetField leniency (duplicate of hunk 7 suggestion)Consider accepting map[string]interface{} and converting to models.Annotations as in hunk 7.
39831-39831: Same pointer-to-map note applies (DeliveryStatus)Same aliasing risk; apply shallow-copy in setter. Also, confirm metadata is intentionally not on DeliveryStatus.
40443-40449: SetField leniency (duplicate of hunk 7 suggestion)Accept plain map[string]interface{} to reduce surprises.
41479-41492: SetField leniency (duplicate of hunk 7 suggestion)Same suggestion to accept map[string]interface{} and convert to models.Annotations.
🧹 Nitpick comments (27)
api/client/javascript/src/zod/index.ts (1)
9147-9171: Consolidate duplicate constants with identical values.Multiple constants are defined with the same value (256) but different names:
createNotificationRuleBodyNameMaxcreateNotificationRuleBodyNameMaxOnecreateNotificationRuleBodyNameMaxTwocreateNotificationRuleBodyNameMaxThreeThe same pattern appears for update notification rules (lines 9382-9406).
Since this appears to be generated code, consider updating the generator to reuse a single constant across all variants, which would simplify maintenance and reduce duplication:
export const notificationRuleBodyNameMax = 256 as constThen reference this single constant in all schema variants.
test/notification/event.go (1)
349-352: Good coverage for annotations round‑trip on delivery status update.Tests assert both state and annotations; this is useful. Consider adding one case with empty/nil annotations to verify clearing behavior.
Also applies to: 364-366, 376-378
openmeter/notification/deliverystatus.go (1)
162-164: Consider validation and docs for Update input annotations.If annotations have constraints (e.g., max size/keys), add validation similar to State.Validate(). Also document whether passing nil clears vs leaves unchanged.
openmeter/ent/db/notificationchannel.go (1)
31-34: Consistency: Metadata type alignment.Channel.Metadata here is map[string]string while other layers/tests use models.Metadata. If you want a single alias across layers, consider switching schema type to models.Metadata and re-generate; otherwise keep as-is intentionally.
Can you confirm models.Metadata is a type alias to map[string]string and that this divergence is intentional in Ent-generated structs?
test/notification/channel.go (1)
102-151: Optional: add test for clearing metadata via null on update.To validate spec behavior, add an update that sets Metadata to nil and assert it clears in the result.
openmeter/ent/db/notificationrule_update.go (1)
61-84: Read-only annotations exposed in update builders — confirm intentPublic Set/Clear for annotations/metadata are fine for internal/system writes, but if API is read-only for annotations, ensure service layer never wires user input here. Otherwise, mark field immutable in schema to avoid accidental writes.
openmeter/notification/httpdriver/mapping.go (2)
46-48: Prefer EmptyableToPtr to omit empty annotations/metadataTo keep API output consistent (omit when empty), switch ToPtr(...) to EmptyableToPtr(...). This avoids emitting explicit nulls.
Apply:
@@ - Annotations: lo.ToPtr(FromAnnotations(c.Annotations)), - Metadata: lo.ToPtr(FromMetadata(c.Metadata)), + Annotations: lo.EmptyableToPtr(FromAnnotations(c.Annotations)), + Metadata: lo.EmptyableToPtr(FromMetadata(c.Metadata)), @@ - Annotations: lo.ToPtr(api.Annotations(e.Annotations)), + Annotations: lo.EmptyableToPtr(api.Annotations(e.Annotations)),Also applies to: 489-489
85-87: Standardize metadata mapping via AsMetadata(...)Use the AsMetadata helper everywhere for symmetry and future validation hooks.
Apply:
@@ func AsChannelWebhookCreateRequest(...) - Metadata: lo.FromPtr(r.Metadata), + Metadata: AsMetadata(r.Metadata), @@ func AsChannelWebhookUpdateRequest(...) - Metadata: AsMetadata(r.Metadata), + Metadata: AsMetadata(r.Metadata), @@ func AsRuleBalanceThresholdCreateRequest(...) - Metadata: AsMetadata(r.Metadata), + Metadata: AsMetadata(r.Metadata), @@ func AsRuleBalanceThresholdUpdateRequest(...) - Metadata: AsMetadata(r.Metadata), + Metadata: AsMetadata(r.Metadata), @@ func AsRuleEntitlementResetCreateRequest(...) - Metadata: lo.FromPtr(r.Metadata), + Metadata: AsMetadata(r.Metadata), @@ func AsRuleEntitlementResetUpdateRequest(...) - Metadata: AsMetadata(r.Metadata), + Metadata: AsMetadata(r.Metadata), @@ func AsRuleInvoiceCreatedCreateRequest(...) - Metadata: AsMetadata(r.Metadata), + Metadata: AsMetadata(r.Metadata), @@ func AsRuleInvoiceCreatedUpdateRequest(...) - Metadata: AsMetadata(r.Metadata), + Metadata: AsMetadata(r.Metadata), @@ func AsRuleInvoiceUpdatedCreateRequest(...) - Metadata: AsMetadata(r.Metadata), + Metadata: AsMetadata(r.Metadata), @@ func AsRuleInvoiceUpdatedUpdateRequest(...) - Metadata: AsMetadata(r.Metadata), + Metadata: AsMetadata(r.Metadata),Also applies to: 116-117, 138-139, 161-162, 182-183, 204-205, 223-224, 243-244, 262-263, 282-283
api/client/javascript/src/client/schemas.ts (8)
7921-7925: Confirm null vs undefined for metadata on create/updateIf this is a Create* schema, consider dropping “| null” (let omit mean “use default”). Keep “| null” for Update/Patch to explicitly clear. Please confirm API behavior and align docs.
8046-8054: DeliveryStatus: exposing annotations looks good; minor JSDoc nitLGTM exposing read-only annotations here. JSDoc “Notification channel the delivery status associated with.” reads awkwardly; consider “Notification channel the delivery status is associated with.”
8254-8263: Rules read schema: prefer Readonly for annotations; document metadata clearing
- Wrap annotations with Readonly<...> to prevent accidental mutation by consumers.
- Add “Send null to clear; omit to leave unchanged” note to metadata description for consistency.
- readonly annotations?: components['schemas']['Annotations'] + readonly annotations?: Readonly<components['schemas']['Annotations']>
8295-8299: Rules create/update: validate necessity of metadata | nullIf this is Create*, consider metadata?: Metadata (no null). Reserve “| null” for Update/Patch to signal clear. Please confirm OpenAPI and backend behavior.
8405-8414: Rules read schema (features variant): same Readonly + semanticsApply Readonly<...> to annotations and clarify metadata null vs omit semantics in the JSDoc.
- readonly annotations?: components['schemas']['Annotations'] + readonly annotations?: Readonly<components['schemas']['Annotations']>
8441-8445: Rules create/update (features variant): confirm null semanticsSame as above: confirm whether “| null” is required on Create; if not, drop it to reduce ambiguity.
8509-8518: InvoiceCreated read schema: immutable annotations + metadata doc
- Use Readonly<...> for annotations.
- Add explicit null vs omit guidance for metadata.
- readonly annotations?: components['schemas']['Annotations'] + readonly annotations?: Readonly<components['schemas']['Annotations']>
8603-8612: InvoiceUpdated read schema: same Readonly + semanticsMirror earlier read models: make annotations Readonly and clarify metadata behavior.
- readonly annotations?: components['schemas']['Annotations'] + readonly annotations?: Readonly<components['schemas']['Annotations']>openmeter/ent/db/notificationevent_update.go (1)
365-371: LGTM: JSON handling in single-entity updateClear/set semantics look standard; Clear should result in NULL (optional field).
Optional improvement: consider a higher-level “MergeAnnotations(map[string]interface{})” in business layer to avoid full overwrite patterns.
openmeter/ent/schema/notification.go (1)
126-127: Add MetadataMixin to NotificationEvent and NotificationEventDeliveryStatus for parityVerification confirms the parity gap: NotificationChannel (lines 25-33) and NotificationRule (lines 71-79) both include
MetadataMixin, but NotificationEvent (lines 122-128) and NotificationEventDeliveryStatus (lines 174-180) omit it. Additionally, NotificationEvent and NotificationEventDeliveryStatus are also missingTimeMixinthat other notification entities include. Apply the suggested diff to addMetadataMixinto NotificationEvent. For full parity, also consider addingTimeMixin.openmeter/ent/db/mutation.go (7)
38068-38069: Pointer-to-map aliasing and read-only enforcementUsing pointers to maps is fine for ent, but it aliases caller state. Consider shallow-copying in setters to avoid accidental mutation. Also, since annotations are system-managed/read-only at API level, confirm they’re not writable via public surfaces.
38344-38441: Shallow-copy maps in setters to prevent external aliasingCopy incoming maps before storing to avoid later caller mutations changing the pending mutation.
func (m *NotificationChannelMutation) SetAnnotations(value models.Annotations) { - m.annotations = &value + cp := make(models.Annotations, len(value)) + for k, v := range value { + cp[k] = v + } + m.annotations = &cp } ... func (m *NotificationChannelMutation) SetMetadata(value map[string]string) { - m.metadata = &value + cp := make(map[string]string, len(value)) + for k, v := range value { + cp[k] = v + } + m.metadata = &cp }
38812-38828: Be lenient in SetField for annotations typeSetField currently requires models.Annotations. Accept plain map[string]interface{} and convert to reduce friction.
case notificationchannel.FieldAnnotations: - v, ok := value.(models.Annotations) - if !ok { - return fmt.Errorf("unexpected type %T for field %s", value, name) - } - m.SetAnnotations(v) + switch v := value.(type) { + case models.Annotations: + m.SetAnnotations(v) + case map[string]interface{}: + m.SetAnnotations(models.Annotations(v)) + default: + return fmt.Errorf("unexpected type %T for field %s", value, name) + } return nil
39210-39258: Shallow-copy annotations in setters (NotificationEvent)Mirror the channel fix to avoid aliasing.
func (m *NotificationEventMutation) SetAnnotations(value models.Annotations) { - m.annotations = &value + cp := make(models.Annotations, len(value)) + for k, v := range value { + cp[k] = v + } + m.annotations = &cp }
39987-40035: Shallow-copy annotations in setters (DeliveryStatus)Same map copy mitigation as above.
func (m *NotificationEventDeliveryStatusMutation) SetAnnotations(value models.Annotations) { - m.annotations = &value + cp := make(models.Annotations, len(value)) + for k, v := range value { + cp[k] = v + } + m.annotations = &cp }
40678-40679: Pointer-to-map + consistency with Metadata typeSame aliasing note. Also, consider standardizing metadata to a typedef (models.Metadata) for consistency with models package.
40957-41054: Shallow-copy in setters (NotificationRule) + API read-only enforcementApply shallow-copy for both annotations and metadata as with channel. Also confirm API layer keeps annotations read-only for clients.
func (m *NotificationRuleMutation) SetAnnotations(value models.Annotations) { - m.annotations = &value + cp := make(models.Annotations, len(value)) + for k, v := range value { + cp[k] = v + } + m.annotations = &cp } ... func (m *NotificationRuleMutation) SetMetadata(value map[string]string) { - m.metadata = &value + cp := make(map[string]string, len(value)) + for k, v := range value { + cp[k] = v + } + m.metadata = &cp }openmeter/ent/db/migrate/schema.go (2)
1741-1749: Consider concurrent index creation for GIN.On large tables, create GIN indexes CONCURRENTLY to avoid long locks; ent migrate won’t do this automatically. Use a manual migration for production.
1904-1912: Same note: build GIN indexes concurrently in prod.Replication of prior advice for rule annotations index.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
api/client/javascript/src/client/schemas.ts(11 hunks)api/client/javascript/src/zod/index.ts(16 hunks)api/openapi.cloud.yaml(21 hunks)api/openapi.yaml(21 hunks)api/spec/src/notification/channel.tsp(2 hunks)api/spec/src/notification/event.tsp(1 hunks)api/spec/src/notification/rule.tsp(2 hunks)openmeter/ent/db/migrate/schema.go(9 hunks)openmeter/ent/db/mutation.go(36 hunks)openmeter/ent/db/notificationchannel.go(6 hunks)openmeter/ent/db/notificationchannel/notificationchannel.go(2 hunks)openmeter/ent/db/notificationchannel/where.go(1 hunks)openmeter/ent/db/notificationchannel_create.go(6 hunks)openmeter/ent/db/notificationchannel_update.go(5 hunks)openmeter/ent/db/notificationevent.go(5 hunks)openmeter/ent/db/notificationevent/notificationevent.go(2 hunks)openmeter/ent/db/notificationevent/where.go(1 hunks)openmeter/ent/db/notificationevent_create.go(9 hunks)openmeter/ent/db/notificationevent_update.go(4 hunks)openmeter/ent/db/notificationeventdeliverystatus.go(6 hunks)openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go(2 hunks)openmeter/ent/db/notificationeventdeliverystatus/where.go(1 hunks)openmeter/ent/db/notificationeventdeliverystatus_create.go(6 hunks)openmeter/ent/db/notificationeventdeliverystatus_update.go(5 hunks)openmeter/ent/db/notificationrule.go(6 hunks)openmeter/ent/db/notificationrule/notificationrule.go(2 hunks)openmeter/ent/db/notificationrule/where.go(1 hunks)openmeter/ent/db/notificationrule_create.go(6 hunks)openmeter/ent/db/notificationrule_update.go(5 hunks)openmeter/ent/db/predicate/predicate.go(0 hunks)openmeter/ent/db/runtime.go(0 hunks)openmeter/ent/db/setorclear.go(3 hunks)openmeter/ent/schema/notification.go(4 hunks)openmeter/notification/adapter/channel.go(2 hunks)openmeter/notification/adapter/deliverystatus.go(1 hunks)openmeter/notification/adapter/entitymapping.go(3 hunks)openmeter/notification/adapter/rule.go(2 hunks)openmeter/notification/channel.go(3 hunks)openmeter/notification/deliverystatus.go(2 hunks)openmeter/notification/httpdriver/mapping.go(17 hunks)openmeter/notification/rule.go(3 hunks)openmeter/notification/service/channel.go(1 hunks)test/notification/channel.go(5 hunks)test/notification/event.go(3 hunks)test/notification/rule.go(5 hunks)
💤 Files with no reviewable changes (2)
- openmeter/ent/db/predicate/predicate.go
- openmeter/ent/db/runtime.go
🧰 Additional context used
🧬 Code graph analysis (29)
openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (3)
openmeter/ent/db/notificationchannel/notificationchannel.go (1)
FieldAnnotations(29-29)openmeter/ent/db/notificationevent/notificationevent.go (1)
FieldAnnotations(22-22)openmeter/ent/db/notificationrule/notificationrule.go (1)
FieldAnnotations(29-29)
openmeter/ent/db/notificationrule/notificationrule.go (3)
openmeter/ent/db/notificationchannel/notificationchannel.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)openmeter/ent/db/notificationevent/notificationevent.go (1)
FieldAnnotations(22-22)openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
FieldAnnotations(22-22)
openmeter/ent/db/notificationevent.go (2)
pkg/models/annotation.go (1)
Annotations(3-3)openmeter/ent/db/notificationchannel/notificationchannel.go (1)
FieldAnnotations(29-29)
openmeter/ent/db/notificationchannel/where.go (9)
openmeter/ent/db/notificationevent/where.go (2)
AnnotationsIsNil(155-157)AnnotationsNotNil(160-162)openmeter/ent/db/notificationeventdeliverystatus/where.go (2)
AnnotationsIsNil(165-167)AnnotationsNotNil(170-172)openmeter/ent/db/notificationrule/where.go (4)
AnnotationsIsNil(295-297)AnnotationsNotNil(300-302)MetadataIsNil(305-307)MetadataNotNil(310-312)openmeter/ent/db/notificationchannel.go (2)
NotificationChannel(19-47)NotificationChannel(68-87)openmeter/ent/schema/notification.go (5)
NotificationChannel(21-23)NotificationChannel(25-33)NotificationChannel(35-52)NotificationChannel(54-58)NotificationChannel(60-65)openmeter/ent/db/notificationchannel/notificationchannel.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)openmeter/ent/db/notificationevent/notificationevent.go (1)
FieldAnnotations(22-22)openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
FieldAnnotations(22-22)openmeter/ent/db/notificationrule/notificationrule.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)
openmeter/ent/db/notificationrule.go (3)
openmeter/ent/db/notificationchannel/notificationchannel.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)openmeter/ent/db/notificationevent/notificationevent.go (1)
FieldAnnotations(22-22)openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
FieldAnnotations(22-22)
openmeter/ent/db/notificationeventdeliverystatus_update.go (1)
openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
FieldAnnotations(22-22)
openmeter/ent/db/notificationchannel/notificationchannel.go (3)
openmeter/ent/db/notificationevent/notificationevent.go (1)
FieldAnnotations(22-22)openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
FieldAnnotations(22-22)openmeter/ent/db/notificationrule/notificationrule.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)
openmeter/ent/db/notificationchannel.go (1)
openmeter/ent/db/notificationchannel/notificationchannel.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)
openmeter/notification/rule.go (3)
api/client/javascript/src/client/schemas.ts (2)
Annotations(11516-11516)Metadata(11860-11860)api/api.gen.go (2)
Annotations(1042-1042)Metadata(4964-4964)api/client/go/client.gen.go (2)
Annotations(957-957)Metadata(4497-4497)
openmeter/ent/db/notificationeventdeliverystatus/where.go (6)
openmeter/ent/db/notificationchannel/where.go (2)
AnnotationsIsNil(295-297)AnnotationsNotNil(300-302)openmeter/ent/db/notificationevent/where.go (2)
AnnotationsIsNil(155-157)AnnotationsNotNil(160-162)openmeter/ent/db/notificationrule/where.go (2)
AnnotationsIsNil(295-297)AnnotationsNotNil(300-302)openmeter/ent/db/notificationeventdeliverystatus.go (2)
NotificationEventDeliveryStatus(19-43)NotificationEventDeliveryStatus(64-79)openmeter/ent/db/predicate/predicate.go (1)
NotificationEventDeliveryStatus(199-199)openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
FieldAnnotations(22-22)
openmeter/ent/db/notificationeventdeliverystatus_create.go (1)
openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
FieldAnnotations(22-22)
openmeter/ent/db/notificationchannel_update.go (1)
openmeter/ent/db/notificationchannel/notificationchannel.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)
openmeter/ent/db/notificationeventdeliverystatus.go (1)
openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
FieldAnnotations(22-22)
openmeter/ent/db/notificationrule/where.go (6)
openmeter/ent/db/notificationchannel/where.go (4)
AnnotationsIsNil(295-297)AnnotationsNotNil(300-302)MetadataIsNil(305-307)MetadataNotNil(310-312)openmeter/ent/db/notificationevent/where.go (2)
AnnotationsIsNil(155-157)AnnotationsNotNil(160-162)openmeter/ent/db/notificationeventdeliverystatus/where.go (2)
AnnotationsIsNil(165-167)AnnotationsNotNil(170-172)openmeter/ent/db/notificationrule.go (2)
NotificationRule(19-47)NotificationRule(79-98)openmeter/ent/db/predicate/predicate.go (1)
NotificationRule(202-202)openmeter/ent/db/notificationrule/notificationrule.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)
openmeter/ent/db/setorclear.go (4)
openmeter/ent/db/notificationchannel_update.go (2)
NotificationChannelUpdate(22-26)NotificationChannelUpdateOne(331-336)pkg/models/annotation.go (1)
Annotations(3-3)openmeter/ent/db/notificationeventdeliverystatus_update.go (2)
NotificationEventDeliveryStatusUpdate(22-26)NotificationEventDeliveryStatusUpdateOne(261-266)openmeter/ent/db/notificationrule_update.go (2)
NotificationRuleUpdate(23-27)NotificationRuleUpdateOne(413-418)
openmeter/notification/adapter/deliverystatus.go (1)
openmeter/ent/db/notificationeventdeliverystatus/where.go (1)
Reason(95-97)
openmeter/ent/db/notificationevent/notificationevent.go (6)
openmeter/ent/db/notificationchannel/notificationchannel.go (1)
FieldAnnotations(29-29)openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
FieldAnnotations(22-22)openmeter/ent/db/notificationrule/notificationrule.go (1)
FieldAnnotations(29-29)openmeter/ent/db/entitlement/entitlement.go (1)
FieldAnnotations(67-67)openmeter/ent/db/addon/addon.go (1)
FieldAnnotations(48-48)openmeter/ent/db/subscriptionitem/subscriptionitem.go (1)
FieldAnnotations(31-31)
openmeter/ent/schema/notification.go (1)
pkg/framework/entutils/mixins.go (5)
AnnotationsMixin(157-159)AnnotationsMixin(162-170)AnnotationsMixin(172-181)MetadataMixin(141-143)MetadataMixin(146-154)
openmeter/ent/db/notificationrule_create.go (1)
openmeter/ent/db/notificationrule/notificationrule.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)
openmeter/ent/db/notificationchannel_create.go (1)
openmeter/ent/db/notificationchannel/notificationchannel.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)
openmeter/ent/db/notificationevent_create.go (4)
pkg/models/annotation.go (1)
Annotations(3-3)openmeter/ent/db/notificationevent.go (2)
NotificationEvent(20-40)NotificationEvent(74-89)openmeter/ent/schema/notification.go (5)
NotificationEvent(118-120)NotificationEvent(122-128)NotificationEvent(130-148)NotificationEvent(150-161)NotificationEvent(163-168)openmeter/ent/db/notificationevent/notificationevent.go (2)
FieldAnnotations(22-22)FieldPayload(30-30)
openmeter/ent/db/notificationevent/where.go (6)
openmeter/ent/db/notificationchannel/where.go (2)
AnnotationsIsNil(295-297)AnnotationsNotNil(300-302)openmeter/ent/db/notificationeventdeliverystatus/where.go (2)
AnnotationsIsNil(165-167)AnnotationsNotNil(170-172)openmeter/ent/db/notificationrule/where.go (2)
AnnotationsIsNil(295-297)AnnotationsNotNil(300-302)openmeter/ent/db/predicate/predicate.go (1)
NotificationEvent(196-196)openmeter/ent/db/notificationchannel/notificationchannel.go (1)
FieldAnnotations(29-29)openmeter/ent/db/notificationevent/notificationevent.go (1)
FieldAnnotations(22-22)
openmeter/notification/httpdriver/mapping.go (3)
api/client/javascript/src/client/schemas.ts (2)
Annotations(11516-11516)Metadata(11860-11860)api/api.gen.go (2)
Annotations(1042-1042)Metadata(4964-4964)api/client/go/client.gen.go (2)
Annotations(957-957)Metadata(4497-4497)
test/notification/event.go (4)
api/client/javascript/src/client/schemas.ts (1)
Annotations(11516-11516)api/api.gen.go (1)
Annotations(1042-1042)api/client/go/client.gen.go (1)
Annotations(957-957)openmeter/notification/deliverystatus.go (2)
EventDeliveryStatusStateFailed(14-14)EventDeliveryStatusStateSuccess(13-13)
openmeter/ent/db/notificationrule_update.go (1)
openmeter/ent/db/notificationrule/notificationrule.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)
openmeter/ent/db/mutation.go (7)
api/api.gen.go (2)
Annotations(1042-1042)Metadata(4964-4964)pkg/models/annotation.go (1)
Annotations(3-3)openmeter/ent/db/notificationchannel/notificationchannel.go (1)
FieldAnnotations(29-29)openmeter/ent/db/notificationevent/notificationevent.go (1)
FieldAnnotations(22-22)openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
FieldAnnotations(22-22)openmeter/ent/db/notificationrule/notificationrule.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)pkg/models/metadata.go (1)
Metadata(9-9)
openmeter/notification/channel.go (1)
api/api.gen.go (2)
Annotations(1042-1042)Metadata(4964-4964)
test/notification/rule.go (3)
api/client/javascript/src/client/schemas.ts (2)
Metadata(11860-11860)Annotations(11516-11516)api/api.gen.go (2)
Metadata(4964-4964)Annotations(1042-1042)api/client/go/client.gen.go (2)
Metadata(4497-4497)Annotations(957-957)
openmeter/ent/db/notificationevent_update.go (3)
pkg/models/annotation.go (1)
Annotations(3-3)openmeter/ent/db/notificationevent/notificationevent.go (2)
FieldAnnotations(22-22)FieldPayload(30-30)openmeter/ent/db/notificationevent/where.go (1)
Payload(85-87)
⏰ 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). (9)
- GitHub Check: Lint
- GitHub Check: Artifacts / Container image
- GitHub Check: Test
- GitHub Check: Migration Checks
- GitHub Check: Code Generators
- GitHub Check: Build
- GitHub Check: SAST (semgrep)
- GitHub Check: Repository Scan
- GitHub Check: Analyze (go)
🔇 Additional comments (121)
openmeter/notification/adapter/deliverystatus.go (1)
132-133: LGTM! Consistent annotation support added.The addition of
SetAnnotations(params.Annotations)follows the established builder pattern and aligns with the PR's objective to add annotation fields across notification entities.openmeter/ent/db/notificationrule/notificationrule.go (1)
28-31: LGTM! Field constants and column definitions correctly generated.The addition of
FieldAnnotationsandFieldMetadataconstants and their inclusion in theColumnsarray is consistent with similar entities in the codebase. The field ordering (after standard fields, before business logic fields) follows a logical structure.Note: This is generated code from the ent schema. Any concerns should be addressed at the schema definition level.
Also applies to: 67-68
openmeter/ent/db/notificationrule_create.go (5)
78-88: Create builder: Annotations/Metadata wiring looks consistent.Setters align with existing mutation patterns; types look correct (Annotations via models.Annotations, Metadata via map[string]string).
If annotations are meant to be system-managed, confirm that external inputs cannot populate them via this path (see adapters).
314-321: Persisting JSON fields in createSpec is correct.Using field.TypeJSON for both is appropriate; no issues spotted.
456-472: Upsert helpers are complete and symmetric.Set/Update/Clear cover both fields; good parity.
Double-check desired Upsert(UpdateNewValues) behavior: should annotations/metadata be updated on conflict, or should they be ignored like ID/Namespace/CreatedAt/Type?
Also applies to: 474-491
626-667: UpsertOne passthrough methods look good.No functional concerns.
979-1019: UpsertBulk passthrough methods look good.Consistent with single-row upsert.
openmeter/notification/adapter/channel.go (1)
90-93: Remove this review comment—the premise is incorrect.Annotations are intentionally user-settable structures (map[string]interface{}), not system-managed fields. The codebase uniformly persists client-supplied annotations across all adapters (channels, rules, events, delivery statuses). The annotations.go file defines predefined annotation keys as constants, and the TODO comment deprecates a specific key, not the annotation-setting pattern itself. Annotations and Metadata are treated identically as user-data structures throughout the notification service by design.
Likely an incorrect or invalid review comment.
openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
21-22: LGTM! Field constant added consistently.The
FieldAnnotationsconstant and its inclusion in theColumnsslice align with the pattern used across other notification entities.Also applies to: 50-50
openmeter/ent/db/notificationevent/notificationevent.go (1)
21-22: LGTM! Field constant added consistently.The annotations field is properly integrated following the same pattern as other notification entities.
Also applies to: 55-55
openmeter/ent/db/notificationeventdeliverystatus/where.go (1)
164-172: LGTM! Nil predicates added for annotations field.The
AnnotationsIsNil()andAnnotationsNotNil()predicates follow the standard pattern for nullable JSONB fields and align with similar predicates in other notification entities.api/spec/src/notification/event.tsp (2)
54-54: Documentation typo corrected.Good catch fixing "delivery sattus" to "delivery status".
60-65: LGTM! Annotations field properly defined.The annotations field correctly specifies:
- Read-only visibility for system-managed metadata
- Optional property with appropriate documentation
- Consistent with the pattern across other notification entities
openmeter/ent/db/notificationeventdeliverystatus.go (5)
6-6: LGTM! Required imports added.The
encoding/jsonandmodelspackage imports are necessary for JSON-based annotations handling.Also applies to: 15-15
25-26: LGTM! Annotations field added to struct.The field type
models.Annotationsis consistent with other notification entities and properly tagged.
68-69: LGTM! Scan allocation for JSONB field.The
[]byteallocation for the annotations column is the correct approach for JSONB fields.
101-108: LGTM! JSON unmarshaling with proper error handling.The implementation correctly:
- Type-checks the scanned value
- Validates non-nil and non-empty before unmarshaling
- Wraps errors with appropriate context
189-191: LGTM! String representation includes annotations.Including annotations in the debug string output aids troubleshooting.
openmeter/notification/adapter/entitymapping.go (3)
37-38: LGTM! Annotations and Metadata propagated for Channel.The mapping correctly includes both system-managed annotations and user-provided metadata from the DB entity.
78-79: LGTM! Annotations and Metadata propagated for Rule.The mapping correctly includes both fields from the DB entity, consistent with the Channel mapping.
134-134: LGTM! Annotations propagated for EventDeliveryStatus.The mapping correctly includes annotations. Note that EventDeliveryStatus does not have a Metadata field, which is consistent with the schema design where only Channel and Rule have user-provided metadata.
openmeter/ent/db/notificationrule.go (5)
6-6: LGTM! Required imports added.The imports support JSON handling for both annotations and metadata fields.
Also applies to: 15-15
31-34: LGTM! Annotations and Metadata fields added.Both fields are properly typed:
Annotationsusesmodels.Annotations(system-managed)Metadatausesmap[string]string(user-provided)
83-84: LGTM! Scan allocations for JSONB fields.Both fields correctly allocate
[]bytefor JSONB column scanning.
139-154: LGTM! JSON unmarshaling with consistent error handling.Both fields follow the same pattern with proper type checking, nil/empty validation, and error wrapping.
239-244: LGTM! String representation includes both fields.Both annotations and metadata are included in debug output for troubleshooting.
openmeter/notification/service/channel.go (1)
72-77: LGTM! Annotations and Metadata preserved during webhook update.The update correctly includes
AnnotationsandMetadatato ensure these fields are not lost when updating the channel with the webhook signing secret. This prevents inadvertent clearing of system-managed annotations during the creation flow.openmeter/ent/db/notificationevent.go (3)
26-27: LGTM: added Annotations field on NotificationEvent.Type choice matches models.Annotations. No issues.
78-80: LGTM: JSON scan/unmarshal for annotations.Correct handling of NULL/empty and errors; consistent with other entities.
Also applies to: 111-118
192-195: LGTM: include annotations in String().Helpful for debugging.
openmeter/ent/db/notificationchannel.go (2)
72-73: LGTM: JSON handling for annotations and metadata.Correct scan targets and robust unmarshalling with clear error messages.
Also applies to: 128-143
223-228: LGTM: include annotations/metadata in String().Good for diagnostics.
openmeter/ent/db/notificationevent/where.go (1)
154-162: LGTM: predicates for annotations nullability.Consistent with other entities; useful for filtering.
api/spec/src/notification/channel.tsp (1)
74-87: Annotations (RO) and Metadata (RW): confirm update semantics.Visibility is correct. Please confirm:
- annotations are not writable via HTTP (RO only).
- metadata supports clear-on-update via null and is persisted end-to-end.
test/notification/channel.go (4)
35-43: LGTM: include annotations/metadata in CreateChannelInput.Covers propagation on create.
62-64: LGTM: assert annotations/metadata on create result.Validates round-trip.
132-139: LGTM: include annotations/metadata in UpdateChannelInput.Covers update path correctly.
149-151: LGTM: assert annotations/metadata on update result.Checks persistence after update.
openmeter/ent/db/notificationrule/where.go (1)
294-312: LGTM! Predicate helpers follow established patterns.The nil-check predicates for Annotations and Metadata fields are correctly implemented and consistent with existing predicate patterns (e.g., DeletedAtIsNil/DeletedAtNotNil).
openmeter/ent/db/notificationchannel/notificationchannel.go (1)
28-31: LGTM! Field constants and columns correctly extended.The addition of FieldAnnotations and FieldMetadata constants and their inclusion in the Columns slice follows the established pattern across notification entities.
Also applies to: 58-59
api/spec/src/notification/rule.tsp (2)
51-52: LGTM! Appropriate validation constraints.The addition of length constraints (1-256 characters) for the name field provides reasonable bounds for user input.
71-83: LGTM! Proper field visibility definitions.The annotations field is correctly marked as read-only (Lifecycle.Read), while metadata is exposed for Create/Update operations. This aligns with the PR intent to distinguish system-managed annotations from user-managed metadata.
test/notification/rule.go (2)
239-246: Verify that annotations are properly protected at the API layer.Similar to create operations, the test sets Annotations in UpdateRuleInput, but the API spec marks annotations as read-only. Verify that the HTTP API layer properly prevents users from modifying annotations via update requests.
Also applies to: 256-257
48-55: HTTP API layer correctly filters user-provided annotations. No action required.The API layer properly protects annotations as read-only:
- HTTP handlers unmarshal
NotificationRuleCreateRequestfrom user requests but conversion functions (AsRuleBalanceThresholdCreateRequest, etc. inopenmeter/notification/httpdriver/mapping.go) deliberately omit theAnnotationsfield when mapping to domain models- User-provided annotations from the HTTP request are silently dropped during conversion
- Domain-layer tests verify that internal code can set annotations for system use, which is intentional
The test populating
AnnotationsinNewCreateRuleInput()represents internal/system operations at the domain level, not HTTP API behavior.openmeter/ent/db/notificationchannel/where.go (1)
294-312: LGTM! Predicate helpers follow established patterns.The nil-check predicates are correctly implemented and consistent with the same predicates added to NotificationRule and other notification entities.
openmeter/notification/channel.go (2)
40-41: LGTM! Clean field additions to domain model.The embedding of models.Annotations and models.Metadata extends the Channel struct appropriately.
157-160: Verify annotations handling at the API layer.While the domain models include Annotations fields in CreateChannelInput and UpdateChannelInput, the API spec marks annotations as read-only. Ensure the HTTP API layer properly filters out any user-provided annotation values, preventing users from setting or modifying this system-managed field.
Also applies to: 205-208
openmeter/notification/rule.go (2)
20-21: LGTM! Clean field additions to domain model.The embedding of models.Annotations and models.Metadata extends the Rule struct appropriately.
187-190: Verify annotations handling at the API layer.The domain models include Annotations fields in CreateRuleInput and UpdateRuleInput. While this may be intentional for internal system use, verify that the HTTP API layer properly prevents users from setting or modifying annotations, ensuring they remain system-managed and read-only as defined in the API specification.
Also applies to: 243-246
openmeter/ent/db/notificationeventdeliverystatus_update.go (2)
34-44: LGTM! Update methods follow established patterns.The SetAnnotations and ClearAnnotations methods are correctly implemented for both update builders, routing through the mutation layer as expected.
Also applies to: 268-278
185-190: LGTM! SQL save logic properly handles annotations.The sqlSave implementations correctly handle both setting and clearing the Annotations field through the update spec, consistent with how other optional fields are managed.
Also applies to: 449-454
openmeter/ent/db/notificationrule_update.go (1)
282-293: JSON persistence for annotations/metadata looks correctUsing field.TypeJSON for both fields and handling clear paths matches ent/jsonb expectations. No further issues.
Also applies to: 697-708
openmeter/notification/httpdriver/mapping.go (2)
51-66: Helper casts are correctFromAnnotations/FromMetadata correctly pass through types and handle nil.
354-356: Rule/event mappings propagate annotations/metadata correctlyGood use of EmptyableToPtr to avoid emitting empty objects.
Also applies to: 392-394, 416-418, 440-442, 472-473
openmeter/ent/db/notificationeventdeliverystatus_create.go (2)
35-40: Create-path support for annotations is correctly wiredSetter + createSpec field assignment for JSON annotations look good.
Also applies to: 271-274
367-383: Upsert annotations API is completeSet/Update/Clear variants are present for single and bulk upserts. No issues.
Also applies to: 487-506, 784-804
api/client/javascript/src/client/schemas.ts (2)
8541-8544: InvoiceCreated create/update: keep annotations out; confirm metadata nullLooks correct to exclude annotations. Please confirm “null clears” behavior is supported server-side and documented.
8635-8638: InvoiceUpdated create/update: confirm metadata null; good to exclude annotationsSame note as other request schemas: verify null vs undefined semantics and keep annotations excluded.
openmeter/ent/schema/notification.go (4)
30-31: LGTM: Channel mixinsAdding Annotations + Metadata via entutils mixins is consistent and gives you JSONB + GIN on annotations out of the box.
76-77: LGTM: Rule mixinsSame here—consistent with Channel and aligns field shapes at the schema layer.
390-411: AnnotationsValueScanner is actively used—ignore this review commentThe variable is referenced in at least two other files:
subscription.go:142andaddon.go:45. It is not unused and should not be deleted.Likely an incorrect or invalid review comment.
178-179: Incorrect review comment—DeliveryStatus follows the established patternNotificationEvent lacks MetadataMixin, and NotificationEventDeliveryStatus follows the same pattern. The codebase shows intentional differentiation: NotificationChannel and NotificationRule include MetadataMixin, while event-related entities do not. This is not an inconsistency requiring correction.
Likely an incorrect or invalid review comment.
openmeter/ent/db/notificationevent_update.go (3)
32-36: LGTM: Update builder setters for annotationsSet/Clear methods match the mutation API and models.Annotations type.
Also applies to: 38-42
220-224: LGTM: UpdateOne setters for annotationsAPI mirrors the bulk builder; naming consistent.
Also applies to: 226-230
147-153: LGTM: JSON handling in bulk update is correctThe
field.TypeJSONusage for annotations is verified correct. TheAnnotationsMixindefines this field asfield.JSON("annotations", models.Annotations{})with JSONB backing for Postgres, matching the bulk update logic at lines 147-153.openmeter/ent/db/mutation.go (24)
38687-38687: Capacity prealloc is fineIncreasing to 10 is harmless and adequate.
38700-38705: Field tracking extended — OKAnnotations/metadata added to Fields() — consistent with new setters.
38734-38737: GetField switch extended — OKCases for annotations/metadata look correct.
38763-38766: OldField switch extended — OKOldAnnotations/OldMetadata wiring is consistent.
38887-38892: ClearedFields tracking — OKAnnotations/metadata included in cleared set.
38913-38918: ClearField additions — OKClearAnnotations/ClearMetadata paths are correct.
38942-38947: ResetField additions — OKResetAnnotations/ResetMetadata wiring is correct.
39535-39537: Fields() includes annotations — OKConsistent with setter.
39560-39561: GetField case — OKAnnotations retrieval added correctly.
39581-39582: OldField case — OKOldAnnotations path correct.
39703-39705: ResetField — OKResetAnnotations added properly.
40353-40353: Capacity prealloc — OK8 is fine; just a hint.
40388-40389: GetField case — OKAnnotations handled.
40413-40414: OldField case — OKOldAnnotations wiring correct.
40522-40524: ClearedFields — OKAnnotations included.
40542-40544: ClearField — OKClearAnnotations path correct.
40559-40561: ResetField — OKResetAnnotations added properly.
41354-41354: Capacity prealloc — OK10 is fine.
41367-41372: Fields list — OKAnnotations/metadata integrated correctly.
41401-41404: GetField cases — OKReturn paths correct.
41430-41433: OldField cases — OKOldAnnotations/OldMetadata wired correctly.
41554-41559: ClearedFields — OKAnnotations/metadata included.
41580-41585: ClearField — OKClearers added correctly.
41609-41614: ResetField — OKResetters added correctly.
openmeter/ent/db/notificationchannel_update.go (5)
18-19: Import use looks correct.models.Annotations is referenced below; no issues.
60-83: LGTM: annotations/metadata setters on Update.Signatures match schema types; clear, consistent.
245-256: LGTM: JSONB fields handled correctly in SQL spec.Using field.TypeJSON and ClearField is correct for nullable JSONB.
364-386: LGTM: annotations/metadata setters on UpdateOne.Parity with batch Update maintained.
579-590: LGTM: JSONB handling in UpdateOne SQL spec.Clear and set paths look correct.
openmeter/ent/db/setorclear.go (3)
2355-2381: LGTM: Nil-safe setters for NotificationChannel.Simple, consistent Set-or-Clear helpers.
2411-2423: LGTM: Nil-safe setters for DeliveryStatus annotations.Matches pattern used elsewhere.
2453-2480: LGTM: Nil-safe setters for NotificationRule.Consistent across Update and UpdateOne.
openmeter/ent/db/notificationevent_create.go (3)
36-41: LGTM: annotations setter on Create.Matches models.Annotations and schema JSONB.
348-359: LGTM: payload upsert helpers added.Set/Update methods correctly map to notificationevent.FieldPayload.
Also applies to: 441-454, 703-716
196-197: Signature change verified as safe: no external callers of createSpec() detected.The ripgrep search found zero external usages of
NotificationEventCreate.createSpec()outside the file. SincecreateSpecis private (lowercase identifier in Go) and only appears in its definition, the signature change carries no risk of breaking external code.openmeter/ent/db/notificationchannel_create.go (5)
18-19: LGTM: models import is required.
77-88: LGTM: annotations/metadata setters on Create.Types align; good addition.
298-305: LGTM: createSpec sets JSONB fields.Using field.TypeJSON and populating _node mirrors update path.
424-458: LGTM: upsert support for annotations/metadata across One and Bulk.Set/Update/Clear methods are complete and consistent.
Also applies to: 442-458, 594-635, 601-607, 615-635, 947-988, 975-981
424-458: Guard read-only semantics at API layer.If annotations are system-managed, ensure external inputs can’t set them; keep setting confined to internal flows.
openmeter/ent/db/migrate/schema.go (8)
1717-1719: LGTM: NotificationChannels annotations/metadata columns.Nullable JSONB suits optional system-managed data.
1756-1759: LGTM: namespace_type index column references updated.Column[7] maps to type after new fields; correct.
1766-1766: LGTM: NotificationEvents annotations column added as JSONB.
1799-1815: LGTM: NotificationEvents indexes (annotations GIN, namespace_id/type).Column indexes align: annotations[2], type[4].
1822-1822: LGTM: DeliveryStatus annotations column (JSONB).
1847-1855: LGTM: DeliveryStatus indexes updated.annotations GIN and composite namespace_event_id_channel_id/state use correct column positions.
Also applies to: 1864-1870
1880-1882: LGTM: NotificationRules annotations/metadata columns added.
1919-1922: LGTM: NotificationRules namespace_type index corrected to new column positions.api/openapi.yaml (4)
20789-20790: Consistent and well-scoped name field validation.The addition of
minLength: 1andmaxLength: 256constraints to allnamefields across notification schemas is a solid validation improvement that prevents empty or excessively long names. This constraint is applied consistently across all notification variants (channels and all rule types).Also applies to: 20849-20850, 21331-21332, 21393-21394, 21546-21547, 21599-21600, 21689-21690, 21735-21736, 21810-21811, 21856-21857
20860-20866: Metadata field correctly scoped to request schemas.The
metadatafield appears in both response schemas (alongsideannotations) and in create/update request schemas (withoutannotations). This is the correct pattern: system-managedannotationsare read-only and appear only in responses, while user-editablemetadataappears in requests to allow clients to set or update it.Also applies to: 21404-21410, 21610-21616, 21746-21752, 21867-21873
21051-21056: Annotations added to delivery status.The
annotationsfield is correctly added toNotificationEventDeliveryStatuswith read-only designation, extending the pattern to delivery tracking metadata.
20800-20812: Schema definitions verified and properly structured.The referenced schema definitions for both
AnnotationsandMetadataexist in the OpenAPI spec and are correctly structured:
Annotations: type object with emptyadditionalProperties, marked as system-managedMetadata: type object with string-valuedadditionalProperties, marked as user-editableThe code patterns follow best practices with
annotationsmarkedreadOnly: trueandmetadatamarkednullable: true.api/openapi.cloud.yaml (4)
20085-20086: Name field validation constraints properly applied.The addition of
minLength: 1andmaxLength: 256constraints across all notification resourcenamefields provides consistent input validation. These bounds are reasonable for user-friendly identifiers.Also applies to: 20145-20146, 20627-20628, 20689-20690, 20842-20843, 21031-21032, 21106-21107, 21152-21153
20156-20162: Metadata fields in create requests correctly exclude annotations.Create request schemas (
NotificationChannelCreateRequest,NotificationRuleCreateRequestvariants) appropriately include only themetadatafield and exclude the read-onlyannotationsfield. This enforces the expected API contract where annotations are system-generated.Also applies to: 20700-20706, 20906-20912, 21042-21048, 21163-21169
20347-20352: Annotations field added to EventDeliveryStatus.The addition of the read-only
annotationsfield toNotificationEventDeliveryStatusis appropriate for tracking system-managed metadata at the delivery level. Note that this schema does not include ametadatafield, which appears intentional (metadata may not be applicable at the delivery status level).Please confirm that omitting
metadatafromNotificationEventDeliveryStatusis intentional and aligns with the business logic for delivery status tracking.
20096-20108: Annotations and Metadata fields added consistently to response schemas.Verification confirms both schema definitions are properly defined and correctly referenced:
Annotations(line 13086): type object with system-managed read-only semanticsMetadata(line 19485): type object with user-manageable key-value pairsThe pattern is well-structured:
annotations(read-only, system-managed) appears in response schemas onlymetadata(nullable, user-managed) appears in both response and create request schemas- All $ref pointers to centralized schema definitions are valid and resolve correctly
This correctly distinguishes between system-managed (read-only) and user-provided (writable) fields.
chrisgacsal
force-pushed
the
feat/notification-annotations
branch
2 times, most recently
from
2fe992b to
da60eda
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
openmeter/notification/adapter/rule.go (2)
100-101: Keep annotations server-managed; don’t set from params.CreateRule currently persists client-supplied annotations. If annotations are read-only/system-managed, remove this call (or reject non-empty input) to avoid policy drift. Same fix applies to UpdateRule below.
Apply:
- SetAnnotations(params.Annotations). SetMetadata(params.Metadata).
195-196: Same here: block client-supplied annotations on update.UpdateRule should not accept annotations from params; drop the setter (or hard-fail on non-empty).
- SetAnnotations(params.Annotations). SetMetadata(params.Metadata).openmeter/notification/deliverystatus.go (1)
48-51: JSON field name bug: embedded Annotations marshals as “Annotations”.Use a named field with the proper JSON tag so the API exposes
annotations.type EventDeliveryStatus struct { models.NamespacedModel - models.Annotations + Annotations models.Annotations `json:"annotations,omitempty"`
🧹 Nitpick comments (7)
tools/migrate/migrations/20251026114744_notification_metadata_annotations.up.sql (1)
3-12: Consider concurrent index creation (if your migrator supports non-transactional steps).CREATE INDEX can block writes on large tables. If your migration runner allows it, prefer
CREATE INDEX CONCURRENTLYin Postgres to reduce lock time; otherwise keep as-is. Also, if metadata will be filtered, consider a GIN on metadata as well.Please confirm whether migrations run inside a single transaction (CONCURRENTLY would then be invalid).
openmeter/notification/rule.go (2)
20-21: Prefer named fields with JSON tags over embedding map typesEmbedding non-struct types (models.Annotations, models.Metadata) makes JSON shape harder to control and surfaces “Annotations”/“Metadata” (capitalized) by default. Use named fields with explicit tags.
Apply:
type Rule struct { models.NamespacedModel models.ManagedModel - models.Annotations - models.Metadata + Annotations models.Annotations `json:"annotations,omitempty"` + Metadata models.Metadata `json:"metadata,omitempty"` // ID is the unique identifier for Rule. ID string `json:"id"`
187-191: Annotations are system‑managed; don’t accept them in inputs (or enforce empty)Create/Update inputs expose Annotations though the API treats them as read‑only. Either drop these fields or validate they’re empty to avoid accidental writes from internal callers.
Option A — remove from inputs:
type CreateRuleInput struct { ... - // Metadata - Metadata models.Metadata - // Annotations - Annotations models.Annotations + // Metadata + Metadata models.Metadata } type UpdateRuleInput struct { ... - // Metadata - Metadata models.Metadata - // Annotations - Annotations models.Annotations + // Metadata + Metadata models.Metadata }Option B — keep, but enforce empty in validators (showing snippet outside this hunk):
// In CreateRuleInput.Validate(): if len(i.Annotations) > 0 { errs = append(errs, errors.New("annotations are system-managed and cannot be set")) } // In UpdateRuleInput.Validate(): if len(i.Annotations) > 0 { errs = append(errs, errors.New("annotations are system-managed and cannot be set")) }If Annotations must remain for internal system flows, confirm they’re not persisted from external HTTP handlers.
Also applies to: 243-247
openmeter/notification/httpdriver/mapping.go (3)
46-48: Unify metadata/annotations mapping: omit empties and use AsMetadata
- Prefer EmptyableToPtr so empty maps are omitted rather than serialized as {}.
- Use AsMetadata for all request→domain conversions to avoid type alias pitfalls and for consistency with other call sites.
Apply:
--- FromChannelWebhook - Annotations: lo.ToPtr(FromAnnotations(c.Annotations)), - Metadata: lo.ToPtr(FromMetadata(c.Metadata)), + Annotations: lo.EmptyableToPtr(FromAnnotations(c.Annotations)), + Metadata: lo.EmptyableToPtr(FromMetadata(c.Metadata)),--- AsChannelWebhookCreateRequest - Metadata: lo.FromPtr(r.Metadata), + Metadata: AsMetadata(r.Metadata),--- AsRuleEntitlementResetCreateRequest - Metadata: lo.FromPtr(r.Metadata), + Metadata: AsMetadata(r.Metadata),--- FromEvent - Annotations: lo.ToPtr(api.Annotations(e.Annotations)), + Annotations: lo.EmptyableToPtr(api.Annotations(e.Annotations)),Please confirm the API schema marks these fields as optional; if so, omitting empties keeps responses concise and consistent with other mappers.
Also applies to: 85-86, 182-183, 489-490
51-66: Helpers look goodFromAnnotations/FromMetadata correctly handle nil maps; they’re safe to reuse across the file. Consider using them in FromRule* too for uniformity (style-only).
354-356: Consistent optional fields on rule/event outputsUsing EmptyableToPtr for rule outputs and delivery status annotations is good. Consider using the same for channel and event annotations (see earlier comment) to keep behavior uniform.
Also applies to: 392-394, 416-418, 440-442, 472-473
openmeter/ent/db/notificationchannel_update.go (1)
245-256: JSONB set/clear wiring is correct; consider metadata type consistency.Persistence uses field.TypeJSON as expected. Note: elsewhere metadata types vary (e.g., Subject uses map[string]interface{}). Consider standardizing metadata typing across entities for future flexibility. Based on relevant code.
Also applies to: 579-590
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tools/migrate/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (40)
openmeter/ent/db/migrate/schema.go(9 hunks)openmeter/ent/db/mutation.go(36 hunks)openmeter/ent/db/notificationchannel.go(6 hunks)openmeter/ent/db/notificationchannel/notificationchannel.go(2 hunks)openmeter/ent/db/notificationchannel/where.go(1 hunks)openmeter/ent/db/notificationchannel_create.go(6 hunks)openmeter/ent/db/notificationchannel_update.go(5 hunks)openmeter/ent/db/notificationevent.go(5 hunks)openmeter/ent/db/notificationevent/notificationevent.go(2 hunks)openmeter/ent/db/notificationevent/where.go(1 hunks)openmeter/ent/db/notificationevent_create.go(9 hunks)openmeter/ent/db/notificationevent_update.go(4 hunks)openmeter/ent/db/notificationeventdeliverystatus.go(6 hunks)openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go(2 hunks)openmeter/ent/db/notificationeventdeliverystatus/where.go(1 hunks)openmeter/ent/db/notificationeventdeliverystatus_create.go(6 hunks)openmeter/ent/db/notificationeventdeliverystatus_update.go(5 hunks)openmeter/ent/db/notificationrule.go(6 hunks)openmeter/ent/db/notificationrule/notificationrule.go(2 hunks)openmeter/ent/db/notificationrule/where.go(1 hunks)openmeter/ent/db/notificationrule_create.go(6 hunks)openmeter/ent/db/notificationrule_update.go(5 hunks)openmeter/ent/db/predicate/predicate.go(0 hunks)openmeter/ent/db/runtime.go(0 hunks)openmeter/ent/db/setorclear.go(3 hunks)openmeter/ent/schema/notification.go(4 hunks)openmeter/notification/adapter/channel.go(2 hunks)openmeter/notification/adapter/deliverystatus.go(1 hunks)openmeter/notification/adapter/entitymapping.go(3 hunks)openmeter/notification/adapter/rule.go(2 hunks)openmeter/notification/channel.go(3 hunks)openmeter/notification/deliverystatus.go(2 hunks)openmeter/notification/httpdriver/mapping.go(17 hunks)openmeter/notification/rule.go(3 hunks)openmeter/notification/service/channel.go(1 hunks)test/notification/channel.go(5 hunks)test/notification/event.go(3 hunks)test/notification/rule.go(5 hunks)tools/migrate/migrations/20251026114744_notification_metadata_annotations.down.sql(1 hunks)tools/migrate/migrations/20251026114744_notification_metadata_annotations.up.sql(1 hunks)
💤 Files with no reviewable changes (2)
- openmeter/ent/db/runtime.go
- openmeter/ent/db/predicate/predicate.go
🚧 Files skipped from review as they are similar to previous changes (9)
- openmeter/notification/adapter/deliverystatus.go
- openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go
- openmeter/notification/adapter/channel.go
- openmeter/ent/db/notificationeventdeliverystatus/where.go
- openmeter/notification/service/channel.go
- openmeter/ent/db/notificationchannel/where.go
- test/notification/event.go
- openmeter/ent/db/notificationrule/where.go
- openmeter/ent/db/notificationchannel_create.go
🧰 Additional context used
🧬 Code graph analysis (23)
openmeter/ent/db/notificationeventdeliverystatus_update.go (1)
openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
FieldAnnotations(22-22)
test/notification/rule.go (3)
api/client/javascript/src/client/schemas.ts (2)
Metadata(11860-11860)Annotations(11516-11516)api/api.gen.go (2)
Metadata(4964-4964)Annotations(1042-1042)api/client/go/client.gen.go (2)
Metadata(4497-4497)Annotations(957-957)
openmeter/ent/db/notificationevent.go (2)
pkg/models/annotation.go (1)
Annotations(3-3)openmeter/ent/db/notificationevent/notificationevent.go (1)
FieldAnnotations(22-22)
openmeter/ent/db/notificationchannel/notificationchannel.go (3)
openmeter/ent/db/notificationevent/notificationevent.go (1)
FieldAnnotations(22-22)openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
FieldAnnotations(22-22)openmeter/ent/db/notificationrule/notificationrule.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)
openmeter/ent/db/notificationchannel.go (1)
openmeter/ent/db/notificationchannel/notificationchannel.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)
openmeter/ent/db/notificationevent/where.go (5)
openmeter/ent/db/notificationchannel/where.go (2)
AnnotationsIsNil(295-297)AnnotationsNotNil(300-302)openmeter/ent/db/notificationevent.go (2)
NotificationEvent(20-40)NotificationEvent(74-89)openmeter/ent/schema/notification.go (5)
NotificationEvent(118-120)NotificationEvent(122-128)NotificationEvent(130-148)NotificationEvent(150-161)NotificationEvent(163-168)openmeter/ent/db/predicate/predicate.go (1)
NotificationEvent(196-196)openmeter/ent/db/notificationevent/notificationevent.go (1)
FieldAnnotations(22-22)
openmeter/ent/db/notificationrule.go (3)
openmeter/ent/db/notificationchannel/notificationchannel.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)openmeter/ent/db/notificationevent/notificationevent.go (1)
FieldAnnotations(22-22)openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
FieldAnnotations(22-22)
openmeter/notification/httpdriver/mapping.go (3)
api/client/javascript/src/client/schemas.ts (2)
Annotations(11516-11516)Metadata(11860-11860)api/api.gen.go (2)
Annotations(1042-1042)Metadata(4964-4964)api/client/go/client.gen.go (2)
Annotations(957-957)Metadata(4497-4497)
openmeter/ent/db/notificationevent/notificationevent.go (6)
openmeter/ent/db/notificationchannel/notificationchannel.go (1)
FieldAnnotations(29-29)openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
FieldAnnotations(22-22)openmeter/ent/db/notificationrule/notificationrule.go (1)
FieldAnnotations(29-29)openmeter/ent/db/entitlement/entitlement.go (1)
FieldAnnotations(67-67)openmeter/ent/db/addon/addon.go (1)
FieldAnnotations(48-48)openmeter/ent/db/subscriptionitem/subscriptionitem.go (1)
FieldAnnotations(31-31)
openmeter/notification/rule.go (3)
api/client/javascript/src/client/schemas.ts (2)
Annotations(11516-11516)Metadata(11860-11860)api/api.gen.go (2)
Annotations(1042-1042)Metadata(4964-4964)api/client/go/client.gen.go (2)
Annotations(957-957)Metadata(4497-4497)
openmeter/ent/db/notificationevent_update.go (3)
pkg/models/annotation.go (1)
Annotations(3-3)openmeter/ent/db/notificationevent/notificationevent.go (2)
FieldAnnotations(22-22)FieldPayload(30-30)openmeter/ent/db/notificationevent/where.go (1)
Payload(85-87)
openmeter/ent/db/notificationchannel_update.go (1)
openmeter/ent/db/notificationchannel/notificationchannel.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)
openmeter/ent/db/migrate/schema.go (5)
openmeter/ent/db/entitlement/entitlement.go (1)
Columns(136-162)openmeter/ent/db/subject/subject.go (1)
Columns(47-57)openmeter/ent/db/customer/customer.go (1)
Columns(112-132)openmeter/ent/db/billinginvoiceline/billinginvoiceline.go (1)
Columns(190-227)openmeter/ent/db/subscriptionitem/subscriptionitem.go (1)
Columns(105-129)
openmeter/notification/channel.go (1)
api/api.gen.go (2)
Annotations(1042-1042)Metadata(4964-4964)
openmeter/ent/db/notificationeventdeliverystatus.go (1)
openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
FieldAnnotations(22-22)
openmeter/ent/db/notificationrule_create.go (1)
openmeter/ent/db/notificationrule/notificationrule.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)
openmeter/ent/schema/notification.go (1)
pkg/framework/entutils/mixins.go (5)
AnnotationsMixin(157-159)AnnotationsMixin(162-170)AnnotationsMixin(172-181)MetadataMixin(141-143)MetadataMixin(146-154)
openmeter/ent/db/setorclear.go (4)
openmeter/ent/db/notificationchannel_update.go (2)
NotificationChannelUpdate(22-26)NotificationChannelUpdateOne(331-336)pkg/models/annotation.go (1)
Annotations(3-3)openmeter/ent/db/notificationeventdeliverystatus_update.go (2)
NotificationEventDeliveryStatusUpdate(22-26)NotificationEventDeliveryStatusUpdateOne(261-266)openmeter/ent/db/notificationrule_update.go (2)
NotificationRuleUpdate(23-27)NotificationRuleUpdateOne(413-418)
openmeter/ent/db/notificationrule_update.go (1)
openmeter/ent/db/notificationrule/notificationrule.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)
openmeter/ent/db/notificationrule/notificationrule.go (3)
openmeter/ent/db/notificationchannel/notificationchannel.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)openmeter/ent/db/notificationevent/notificationevent.go (1)
FieldAnnotations(22-22)openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
FieldAnnotations(22-22)
openmeter/ent/db/mutation.go (6)
pkg/models/annotation.go (1)
Annotations(3-3)openmeter/ent/db/notificationchannel/notificationchannel.go (1)
FieldAnnotations(29-29)openmeter/ent/db/notificationevent/notificationevent.go (1)
FieldAnnotations(22-22)openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
FieldAnnotations(22-22)openmeter/ent/db/notificationrule/notificationrule.go (2)
FieldAnnotations(29-29)FieldMetadata(31-31)pkg/models/metadata.go (1)
Metadata(9-9)
openmeter/ent/db/notificationeventdeliverystatus_create.go (1)
openmeter/ent/db/notificationeventdeliverystatus/notificationeventdeliverystatus.go (1)
FieldAnnotations(22-22)
openmeter/ent/db/notificationevent_create.go (4)
pkg/models/annotation.go (1)
Annotations(3-3)openmeter/ent/db/notificationevent.go (2)
NotificationEvent(20-40)NotificationEvent(74-89)openmeter/ent/schema/notification.go (5)
NotificationEvent(118-120)NotificationEvent(122-128)NotificationEvent(130-148)NotificationEvent(150-161)NotificationEvent(163-168)openmeter/ent/db/notificationevent/notificationevent.go (2)
FieldAnnotations(22-22)FieldPayload(30-30)
⏰ 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). (9)
- GitHub Check: Code Generators
- GitHub Check: Artifacts / Container image
- GitHub Check: Test
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: Build
- GitHub Check: Repository Scan
- GitHub Check: SAST (semgrep)
- GitHub Check: Analyze (go)
🔇 Additional comments (41)
openmeter/notification/adapter/entitymapping.go (3)
37-39: LGTM: channel mapping includes annotations/metadata.Populating read-only annotations and optional metadata from DB → API is correct.
78-80: LGTM: rule mapping includes annotations/metadata.
134-135: LGTM: delivery status mapping includes annotations.tools/migrate/migrations/20251026114744_notification_metadata_annotations.down.sql (1)
1-12: LGTM: down migration mirrors the up migration (order is safe).openmeter/ent/db/notificationchannel.go (1)
6-16: LGTM: JSONB scan/assign for annotations/metadata is correct.Nil/empty handling and String() updates look good.
Also applies to: 31-35, 72-74, 128-143, 223-228
openmeter/ent/db/notificationeventdeliverystatus.go (1)
6-16: LGTM: annotations JSON handling added to delivery status.Scan/unmarshal and String() formatting are consistent with other entities.
Also applies to: 25-27, 68-70, 101-108, 189-191
openmeter/ent/db/notificationrule_update.go (1)
61-84: LGTM: ent update builders handle JSON fields consistently.Set/Clear for annotations and metadata are properly wired into sqlSave with TypeJSON field type across NotificationRuleUpdate and NotificationRuleUpdateOne (lines 282–293, 446–469, 697–708). Field constants confirmed and generated code verified.
openmeter/notification/deliverystatus.go (1)
162-164: No issues found—delivery status updates are internal-only and not exposed via HTTP.The
UpdateEventDeliveryStatusInputstruct is used exclusively by the internal event dispatch mechanism inopenmeter/notification/eventhandler/dispatch.goand is never exposed via the HTTP API. There is no HTTP endpoint, handler, or request model that accepts delivery status updates from clients. The HTTP API only provides read-only access throughListEventsandGetEventhandlers. Annotations in delivery status are system-managed and updated solely by internal event handlers, not by client requests.Likely an incorrect or invalid review comment.
openmeter/ent/db/notificationevent/where.go (1)
154-162: LGTM!The nil-check predicates for the Annotations field follow the established pattern and are implemented correctly.
openmeter/ent/db/notificationrule.go (1)
31-34: LGTM!The Annotations and Metadata fields are correctly integrated with proper JSON unmarshaling, error handling, and nil checks.
Also applies to: 83-84, 139-154
openmeter/ent/db/notificationevent.go (1)
26-27: LGTM!The Annotations field is correctly implemented with proper JSON scanning and unmarshaling logic consistent with other notification entities.
Also applies to: 78-79, 111-118
openmeter/ent/db/notificationchannel/notificationchannel.go (1)
28-31: LGTM!The field constants and Columns slice are correctly updated to include Annotations and Metadata.
Also applies to: 58-59
test/notification/rule.go (1)
48-55: LGTM!The test coverage for Annotations and Metadata is comprehensive, validating the fields across create, update, and get operations.
Also applies to: 162-163, 239-246, 256-257, 298-302
test/notification/channel.go (1)
35-42: LGTM!The test coverage for Annotations and Metadata on channels is comprehensive and consistent with the rule tests.
Also applies to: 62-63, 132-139, 149-150, 190-191
openmeter/notification/channel.go (1)
40-41: LGTM!The Annotations and Metadata fields are cleanly integrated into the Channel model and input types via embedding and explicit fields.
Also applies to: 157-160, 205-208
openmeter/ent/db/notificationrule/notificationrule.go (1)
28-31: LGTM!The field constants and Columns slice are correctly updated, consistent with the pattern used in other notification entities.
Also applies to: 67-68
openmeter/ent/db/notificationeventdeliverystatus_update.go (1)
34-45: LGTM: update builders support annotations (set/clear) and persist correctlySet/Clear methods and SQL spec updates for FieldAnnotations are correct and align with the JSONB mixin. No further changes needed.
Consider adding/update tests that:
- set annotations, then update to another value;
- clear annotations and assert column becomes NULL (not {}).
Also applies to: 185-190, 268-279, 449-454
openmeter/notification/httpdriver/mapping.go (1)
138-139: Consistent use of AsMetadata in Rule create/update mappersGood consistency here. No changes needed.
Also applies to: 161-162, 223-224, 243-244, 262-263, 282-283
openmeter/ent/schema/notification.go (2)
30-32: LGTM: annotations/metadata mixins appliedMixins are correctly added to channel, rule, event, and delivery status. This aligns schema with JSONB columns and index behavior from entutils.
Confirm migrations include the new JSONB columns and GIN indexes for these tables.
Also applies to: 76-78, 126-127, 178-179
390-411: AnnotationsValueScanner is actively used and should not be removedThe verification shows
AnnotationsValueScanneris referenced in two active locations:
openmeter/ent/schema/subscription.go(line 142)openmeter/ent/schema/addon.go(line 45)Removing this custom scanner would break these field definitions. The review comment's premise that it is unused is incorrect.
Likely an incorrect or invalid review comment.
openmeter/ent/db/notificationevent/notificationevent.go (1)
21-22: No action required—Annotations field and JSON unmarshalling are already correctly implemented.The struct includes an
Annotationsfield typed asmodels.Annotations(line 27),scanValueshandles the field by allocating a[]byte(line 78), andassignValuesproperly unmarshals the JSON bytes into theAnnotationsfield with error handling (lines 115–120). Reads will not return zero values.openmeter/ent/db/mutation.go (1)
38068-38069: LGTM! Generated code follows consistent patterns.The additions of
annotationsandmetadatafields across all notification-related mutation types are correctly implemented:
- NotificationChannelMutation: Both
annotationsandmetadatafields added- NotificationEventMutation:
annotationsfield added- NotificationEventDeliveryStatusMutation:
annotationsfield added- NotificationRuleMutation: Both
annotationsandmetadatafields addedAll implementations follow the standard Ent mutation pattern with proper:
- Field declarations using correct types (
models.Annotationsandmap[string]string)- Setter/getter/clear/reset methods
- Integration into Fields(), Field(), OldField(), SetField(), ClearedFields(), ClearField(), and ResetField()
- Type assertions with appropriate error handling
- Field count updates in slice allocations
Also applies to: 38344-38440, 38687-38705, 38734-38737, 38763-38766, 38812-38825, 38887-38892, 38913-38918, 38942-38947, 39055-39055, 39210-39257, 39535-39537, 39560-39561, 39581-39582, 39607-39613, 39703-39705, 39831-39831, 39987-40034, 40353-40359, 40388-40389, 40413-40414, 40443-40449, 40522-40524, 40542-40544, 40559-40561, 40678-40679, 40957-41053, 41354-41372, 41401-41404, 41430-41433, 41479-41492, 41554-41559, 41580-41585, 41609-41614
openmeter/ent/db/notificationevent_update.go (2)
146-154: Annotations JSON persistence looks correct.Using field.TypeJSON for SetField/ClearField matches models.Annotations and JSONB column. LGTM.
32-42: Public setters/clearers for annotations added consistently.Surface matches other entities; no gaps spotted.
Also applies to: 220-231
openmeter/ent/db/notificationchannel_update.go (1)
60-82: Builder API additions for annotations/metadata are consistent.Methods align with mutation surface; signatures look good.
Also applies to: 364-386
openmeter/ent/db/setorclear.go (3)
2355-2367: Nil-safe SetOrClear helpers for NotificationChannel.Pattern matches existing helpers; covers both annotations and metadata. LGTM.
Also applies to: 2369-2381
2411-2423: Nil-safe SetOrClear for delivery status annotations.Good parity with update builders. LGTM.
2453-2465: Nil-safe SetOrClear for NotificationRule.Consistent with channel helpers. LGTM.
Also applies to: 2467-2479
openmeter/ent/db/migrate/schema.go (4)
1822-1822: DeliveryStatus: annotations column + GIN index added; state/namespace indexes updated.Offsets align (state at [7]). LGTM.
Also applies to: 1847-1855, 1862-1870
1880-1886: NotificationRule: annotations/metadata columns + GIN index added; index offsets updated.namespace_type uses [1],[7] (type). Looks correct.
Also applies to: 1904-1912, 1919-1922
1766-1766: NotificationEvent: annotations column + indexes verified correctly.Verified that
notificationevent_namespace_idindex remains intentionally non-unique (Unique: false, line 1808). Index targets columns [1] and [0]; annotations field at column [2]. All changes confirmed.
1717-1719: Migration verified; GIN index and column offsets confirmed.The SQL migration exists and correctly creates the GIN index "notificationchannel_annotations" on the annotations column. Column offsets are accurate: namespace=[1], type=[7]. Note that the metadata column is added but intentionally does not have a corresponding GIN index—this pattern is consistent across all three notification tables (notification_channels, notification_rules, and notification_event_delivery_status).
openmeter/ent/db/notificationeventdeliverystatus_create.go (1)
35-39: Test coverage for annotations paths is incomplete; manual verification needed.Integration test in
test/notification/event.goverifies annotations in theUpdateEventDeliveryStatusflow, but explicit coverage of create with annotations,UpdateAnnotations(), andClearAnnotations()semantics was not found. Verify that all three paths are adequately tested before merging.openmeter/ent/db/notificationrule_create.go (3)
78-88: LGTM! Field setters follow the established pattern.The
SetAnnotationsandSetMetadatamethods correctly forward to the mutation layer and return the builder for method chaining, consistent with other field setters in this generated file.
314-321: LGTM! CreateSpec correctly handles the new JSON fields.Both
annotationsandmetadataare properly written to the spec and node when provided, usingfield.TypeJSONfor JSONB storage.
456-490: LGTM! Comprehensive upsert support for new fields.All three upsert variants (
NotificationRuleUpsert,NotificationRuleUpsertOne,NotificationRuleUpsertBulk) correctly implement Set/Update/Clear operations for bothannotationsandmetadatafields, following the established pattern for optional fields.Also applies to: 626-666, 979-1019
openmeter/ent/db/notificationevent_create.go (5)
36-40: LGTM! SetAnnotations follows the established pattern.The method correctly forwards to the mutation layer and maintains the builder pattern for method chaining.
229-232: LGTM! Annotations field correctly handled in createSpec.The field is properly written to both the spec and node using
field.TypeJSONfor JSONB storage, consistent with the pattern innotificationrule_create.go.
330-346: LGTM! Complete upsert support for annotations field.All upsert variants correctly implement Set/Update/Clear operations for the
annotationsfield, maintaining consistency across the builder types.Also applies to: 420-439, 682-701
348-358: LGTM! Payload upsert methods added for completeness.The
SetPayloadandUpdatePayloadmethods were added to all upsert variants (NotificationEventUpsert,NotificationEventUpsertOne,NotificationEventUpsertBulk), extending the existing payload field support to upsert operations. This aligns with the pattern of supporting all mutable fields in upsert scenarios.Also applies to: 441-453, 703-715
196-196: No issues found. The createSpec signature change is safe.The method is unexported and all internal callers within
openmeter/ent/db/notificationevent_create.gohave been properly updated. No external references to this method exist elsewhere in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openmeter/notification/adapter/rule.go (1)
148-148: Typo in error message."failed top delete notification rule" → "failed to delete notification rule".
- return fmt.Errorf("failed top delete notification rule: %w", err) + return fmt.Errorf("failed to delete notification rule: %w", err)test/notification/rule.go (1)
171-179: Bug: NewCreateRuleInput called with wrong argument order (namespace missing).Signature is NewCreateRuleInput(namespace, name, channels...). Several calls pass only two args, treating name as namespace. This can break namespace scoping and channel linkage.
Apply the following fixes:
- 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: 214-216, 263-266, 280-284
♻️ Duplicate comments (2)
openmeter/notification/adapter/rule.go (1)
100-101: Do not accept client-provided annotations; keep them server-managed.Create/Update persist params.Annotations. If annotations are intended to be system-managed/read-only (as discussed), this is a policy and security gap. Either drop SetAnnotations or reject non-empty input here.
Apply either of the following diffs (recommended: remove):
Option A — remove writes:
- SetAnnotations(params.Annotations). SetMetadata(params.Metadata).and
- SetAnnotations(params.Annotations). SetMetadata(params.Metadata).Option B — guard and error (if you must keep the call): validate params.Annotations is empty and return a 4xx before Save; otherwise ignore and let server compute annotations.
Also applies to: 195-196
test/notification/channel.go (1)
187-187: Good fix to the assertion message.The assertion message now correctly states "must be equal" to match the
assert.Equalcall. This was previously flagged in past reviews.
🧹 Nitpick comments (4)
openmeter/notification/httpdriver/mapping.go (2)
46-47: Inconsistent patterns for Annotations/Metadata conversion.There are two different patterns for converting from internal models to API types:
Channel mapping (lines 46-47): Uses helper functions with
lo.ToPtrAnnotations: lo.ToPtr(FromAnnotations(c.Annotations)), Metadata: lo.ToPtr(FromMetadata(c.Metadata)),Rule mappings (lines 354-355, 392-393, 416-417, 440-441): Direct cast with
lo.EmptyableToPtrAnnotations: lo.EmptyableToPtr(api.Annotations(r.Annotations)), Metadata: lo.EmptyableToPtr(api.Metadata(r.Metadata)),The second pattern is preferable because
lo.EmptyableToPtrreturnsnilfor empty maps, which works better with JSONomitemptytags. However, consistency across the codebase is important.Consider standardizing on one approach. If keeping the helpers, update lines 46-47 to use
lo.EmptyableToPtr:- Annotations: lo.ToPtr(FromAnnotations(c.Annotations)), - Metadata: lo.ToPtr(FromMetadata(c.Metadata)), + Annotations: lo.EmptyableToPtr(FromAnnotations(c.Annotations)), + Metadata: lo.EmptyableToPtr(FromMetadata(c.Metadata)),Alternatively, update rule mappings to use the helpers for consistency (though the current rule approach is technically better for JSON serialization).
Also applies to: 354-355, 392-393, 416-417, 440-441
489-489: Consider usinglo.EmptyableToPtrfor consistency.This line uses
lo.ToPtrwhile the delivery status annotations on line 472 and rule mappings throughout the file uselo.EmptyableToPtr. UsingEmptyableToPtris preferable because it returnsnilfor empty maps, which serializes better to JSON withomitemptytags.Apply this diff:
- Annotations: lo.ToPtr(api.Annotations(e.Annotations)), + Annotations: lo.EmptyableToPtr(api.Annotations(e.Annotations)),test/notification/event.go (1)
349-351: Tests assume annotations echo input; confirm policy or adjust assertions.If annotations are server-managed, don’t send them in UpdateEventDeliveryStatusInput and assert server-populated keys instead (e.g., presence and expected mapping), not strict equality to input.
Would you like me to update the tests to assert that:
- status.State matches, and
- status.Annotations contains "state" with string(test.Input.State) (without sending Annotations in the input)?
Also applies to: 363-365, 376-377
test/notification/rule.go (1)
48-55: Tests make annotations client-writable; align with intended mutability.These tests set and expect Annotations round-tripped on Rule create/update/get. If annotations are system-managed, remove Annotations from inputs and adjust assertions to match server-produced values; keep Metadata checks as-is.
I can submit an updated test diff once you confirm the intended policy.
Also applies to: 162-164, 239-246, 256-257, 298-303
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
openmeter/notification/adapter/channel.go(2 hunks)openmeter/notification/adapter/deliverystatus.go(1 hunks)openmeter/notification/adapter/entitymapping.go(3 hunks)openmeter/notification/adapter/rule.go(2 hunks)openmeter/notification/channel.go(3 hunks)openmeter/notification/deliverystatus.go(2 hunks)openmeter/notification/httpdriver/mapping.go(17 hunks)openmeter/notification/rule.go(3 hunks)openmeter/notification/service/channel.go(1 hunks)test/notification/channel.go(5 hunks)test/notification/event.go(3 hunks)test/notification/rule.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- openmeter/notification/deliverystatus.go
- openmeter/notification/service/channel.go
- openmeter/notification/adapter/deliverystatus.go
- openmeter/notification/rule.go
🧰 Additional context used
🧬 Code graph analysis (5)
test/notification/event.go (4)
api/client/javascript/src/client/schemas.ts (1)
Annotations(11516-11516)api/api.gen.go (1)
Annotations(1042-1042)api/client/go/client.gen.go (1)
Annotations(957-957)openmeter/notification/deliverystatus.go (2)
EventDeliveryStatusStateFailed(14-14)EventDeliveryStatusStateSuccess(13-13)
openmeter/notification/channel.go (1)
api/api.gen.go (2)
Annotations(1042-1042)Metadata(4964-4964)
openmeter/notification/httpdriver/mapping.go (3)
api/client/javascript/src/client/schemas.ts (2)
Annotations(11516-11516)Metadata(11860-11860)api/api.gen.go (2)
Annotations(1042-1042)Metadata(4964-4964)api/client/go/client.gen.go (2)
Annotations(957-957)Metadata(4497-4497)
test/notification/rule.go (3)
api/client/javascript/src/client/schemas.ts (2)
Metadata(11860-11860)Annotations(11516-11516)api/api.gen.go (2)
Metadata(4964-4964)Annotations(1042-1042)api/client/go/client.gen.go (2)
Metadata(4497-4497)Annotations(957-957)
openmeter/notification/adapter/channel.go (1)
openmeter/notification/adapter/adapter.go (1)
Config(16-19)
⏰ 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). (4)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: Code Generators
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
openmeter/notification/httpdriver/mapping.go (1)
51-95: LGTM! Helper functions are well-structured.The three helper functions provide clean nil-safe conversions between internal and API types for annotations and metadata.
openmeter/notification/adapter/entitymapping.go (1)
37-39: LGTM: read-path mapping extended correctly.Populating Annotations/Metadata on Channel, Rule, and EventDeliveryStatus from DB looks consistent with the model.
Also applies to: 78-80, 134-135
openmeter/notification/channel.go (4)
40-41: LGTM! Embedded fields follow existing struct patterns.The embedded
AnnotationsandMetadatafields are consistent with the existingNamespacedModelandManagedModelpattern in the struct.
199-199: Nice catch on the grammar fix.Good improvement changing "user defined" to "user-defined".
205-208: Same validation concern as CreateChannelInput.Similar to the
CreateChannelInput, these fields lack validation. The verification script generated for lines 157-160 will also cover this input struct.
157-160: No validation is required for Metadata and Annotations fields.Based on verification of the codebase, the
Validate()method intentionally does not validate these map fields. The codebase design shows that optional map fields (Metadata, Annotations) are not validated at the struct level, consistent with how similar optional fields are handled elsewhere. The fields follow the established pattern, and validation can be delegated to custom validators viaValidateWith()if needed by callers. The code is correct as-is.test/notification/channel.go (5)
35-42: LGTM! Test data properly initializes new fields.The test data setup for
MetadataandAnnotationsis appropriate for integration testing.
62-63: LGTM! Proper assertions for new fields.The assertions correctly verify that
AnnotationsandMetadataare persisted during channel creation.
132-139: LGTM! Update test properly covers new fields.The test data correctly includes
MetadataandAnnotationswith updated values to verify that updates work as expected.
149-150: LGTM! Update assertions verify persistence.The assertions correctly verify that updated
AnnotationsandMetadatavalues are persisted.
190-191: LGTM! Get assertions verify data retrieval.The assertions correctly verify that
AnnotationsandMetadataare retrieved properly from the channel.
chrisgacsal
changed the title
chrisgacsal
force-pushed
the
feat/notification-annotations
branch
from
da60eda to
618e7fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
openmeter/notification/adapter/rule.go (1)
100-102: Consider defensive validation for annotations to prevent future policy violations.The adapter unconditionally calls
SetAnnotations(params.Annotations), which currently receives nil from HTTP requests but creates a latent risk if the HTTP mapping layer is later modified to accept client annotations. To enforce that annotations remain system-managed, either remove theSetAnnotationscall if annotations are never meant to be client-writable, or add explicit validation rejecting non-emptyparams.Annotations.The same concern applies to
UpdateRuleat lines 195-196.openmeter/notification/adapter/channel.go (1)
90-93: Consider defensive validation for annotations to prevent future policy violations.Same concern as in
rule.go: The adapter unconditionally callsSetAnnotations(params.Annotations), which currently receives nil from HTTP requests but could accept client annotations if the HTTP mapping is later modified. To maintain annotations as system-managed, either remove theSetAnnotationscall or add validation to reject non-emptyparams.Annotations.The same applies to
UpdateChannelat lines 172-174.
🧹 Nitpick comments (1)
openmeter/notification/httpdriver/mapping.go (1)
85-86: Inconsistent metadata conversion helper usage.Line 85 uses
lo.FromPtr(r.Metadata)whileAsChannelWebhookUpdateRequest(line 116) and all rule mapping functions use theAsMetadatahelper. For consistency and maintainability, consider usingAsMetadata(r.Metadata)here as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
openmeter/notification/adapter/channel.go(2 hunks)openmeter/notification/adapter/deliverystatus.go(1 hunks)openmeter/notification/adapter/entitymapping.go(3 hunks)openmeter/notification/adapter/rule.go(2 hunks)openmeter/notification/channel.go(3 hunks)openmeter/notification/deliverystatus.go(2 hunks)openmeter/notification/httpdriver/mapping.go(17 hunks)openmeter/notification/rule.go(3 hunks)openmeter/notification/service/channel.go(1 hunks)test/notification/channel.go(5 hunks)test/notification/event.go(3 hunks)test/notification/rule.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- openmeter/notification/adapter/deliverystatus.go
- openmeter/notification/deliverystatus.go
- openmeter/notification/channel.go
- openmeter/notification/adapter/entitymapping.go
🧰 Additional context used
🧬 Code graph analysis (5)
openmeter/notification/httpdriver/mapping.go (3)
api/client/javascript/src/client/schemas.ts (2)
Annotations(11516-11516)Metadata(11860-11860)api/api.gen.go (2)
Annotations(1042-1042)Metadata(4964-4964)api/client/go/client.gen.go (2)
Annotations(957-957)Metadata(4497-4497)
openmeter/notification/adapter/channel.go (1)
openmeter/notification/adapter/adapter.go (1)
Config(16-19)
openmeter/notification/rule.go (3)
api/client/javascript/src/client/schemas.ts (2)
Annotations(11516-11516)Metadata(11860-11860)api/api.gen.go (2)
Annotations(1042-1042)Metadata(4964-4964)api/client/go/client.gen.go (2)
Annotations(957-957)Metadata(4497-4497)
test/notification/rule.go (3)
api/client/javascript/src/client/schemas.ts (2)
Metadata(11860-11860)Annotations(11516-11516)api/api.gen.go (2)
Metadata(4964-4964)Annotations(1042-1042)api/client/go/client.gen.go (2)
Metadata(4497-4497)Annotations(957-957)
test/notification/event.go (4)
api/client/javascript/src/client/schemas.ts (1)
Annotations(11516-11516)api/api.gen.go (1)
Annotations(1042-1042)api/client/go/client.gen.go (1)
Annotations(957-957)openmeter/notification/deliverystatus.go (2)
EventDeliveryStatusStateFailed(14-14)EventDeliveryStatusStateSuccess(13-13)
⏰ 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). (9)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: Lint
- GitHub Check: Migration Checks
- GitHub Check: Test
- GitHub Check: Code Generators
- GitHub Check: Build
- GitHub Check: SAST (semgrep)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
test/notification/event.go (1)
349-377: Good test coverage for annotations on delivery status updates.The tests appropriately verify that annotations set via
UpdateEventDeliveryStatusInputare correctly persisted and returned. This provides good coverage for the new annotations field on delivery statuses.test/notification/channel.go (1)
35-63: Comprehensive test coverage for channel annotations and metadata.The tests properly verify that both
AnnotationsandMetadatafields are correctly persisted and retrieved across Create, Update, and Get operations. This provides solid coverage for the new fields.test/notification/rule.go (1)
48-163: Thorough test coverage for rule annotations and metadata.The tests comprehensively validate that
AnnotationsandMetadataare correctly handled throughout the rule lifecycle (Create, Update, Get). The test helper setup and assertions ensure proper field propagation.openmeter/notification/httpdriver/mapping.go (3)
51-95: Clean helper functions for annotations and metadata conversion.The
FromAnnotations,FromMetadata, andAsMetadatahelpers provide clear, nil-safe conversions between internal models and API types. The naming convention (From* for read path, As* for write path) is consistent and intuitive.
120-284: Rule mapping functions correctly enforce annotation policy.All
AsRule*CreateRequestandAsRule*UpdateRequestfunctions consistently:
- Accept
Metadatafrom client requests via theAsMetadatahelper- Do NOT accept
Annotationsfrom client requestsThis properly enforces the policy that annotations are read-only from the client perspective while metadata is writable.
320-543: FromRule and FromEvent functions properly expose annotations.*The output mapping functions correctly include
AnnotationsandMetadatain API responses usinglo.EmptyableToPtrto avoid pointers to empty maps. This ensures clients can read system-managed annotations while the write path (As* functions) prevents client modification.
2 participants
Overview
Add
annotationsandmetadatafields to notification resources.Summary by CodeRabbit
New Features
Improvements