feat: notification channels v3 APIs - #4829
Open
borosr wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughNotification channels now have TypeSpec contracts, domain filtering and validation, v3 HTTP CRUD handlers, Go client support, and JavaScript SDK support. Generated API artifacts expose webhook channel models, pagination, filtering, and CRUD operations. ChangesNotification channel API contracts
Domain and server implementation
Client libraries
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GoClient
participant V3Server
participant ChannelHandler
participant NotificationService
participant NotificationAdapter
GoClient->>V3Server: Send notification-channel request
V3Server->>ChannelHandler: Route CRUD operation
ChannelHandler->>NotificationService: Convert request and invoke service
NotificationService->>NotificationAdapter: Query or persist channel
NotificationAdapter-->>NotificationService: Return result
NotificationService-->>ChannelHandler: Return domain response
ChannelHandler-->>V3Server: Encode API response
V3Server-->>GoClient: Return notification-channel response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
api/v3/handlers/notification/channels/list.go (1)
79-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider folding the repeated bad-request wrapping into one small helper.
Six filter fields repeat the same three-line
apierrors.NewBadRequestErrorblock. Only the field name changes. A tiny named helper keeps the mapping easy to scan and stops the blocks from drifting apart when a new filter arrives.♻️ Sketch of the helper
func invalidFilterParam(ctx context.Context, field string, err error) error { return apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ {Field: field, Reason: err.Error(), Source: apierrors.InvalidParamSourceQuery}, }) }id, err := filters.FromAPIFilterULID(params.Filter.Id) if err != nil { - return ListNotificationChannelsRequest{}, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ - {Field: "filter[id]", Reason: err.Error(), Source: apierrors.InvalidParamSourceQuery}, - }) + return ListNotificationChannelsRequest{}, invalidFilterParam(ctx, "filter[id]", err) } req.ID = id🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v3/handlers/notification/channels/list.go` around lines 79 - 135, Extract the repeated apierrors.NewBadRequestError construction in the filter parsing flow into a small named helper, such as invalidFilterParam, accepting the context, field name, and error. Replace each filter-specific three-line error block for ID, name, type, disabled, created_at, and updated_at with this helper while preserving the existing field names and validation behavior.openmeter/notification/service/channel_test.go (1)
134-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
defer clock.UnFreeze()instead of an explicit call.
clock.FreezeTime(tAlphaUpdated)at Line 134 is not paired withdefer clock.UnFreeze(). It is followed by an explicitclock.UnFreeze()at Line 142. Today this is safe because the explicit unfreeze runs unconditionally beforerequire.NoErrorat Line 143. Pair the freeze with a deferred unfreeze to match the repo convention and to stay safe if this block is edited later.As per coding guidelines, "Pair `clock.FreezeTime(...)` immediately with `defer clock.UnFreeze()` in the same scope."🧹 Proposed fix
clock.FreezeTime(tAlphaUpdated) + defer clock.UnFreeze() _, err := env.adapter.UpdateChannel(t.Context(), notification.UpdateChannelInput{ NamespacedID: models.NamespacedID{Namespace: ns, ID: alpha.ID}, Type: alpha.Type, Name: alpha.Name, Disabled: alpha.Disabled, Config: alpha.Config, }) - clock.UnFreeze() require.NoError(t, err, "updating alpha to advance its updated_at must not fail")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/notification/service/channel_test.go` around lines 134 - 143, In the test block that calls FreezeTime around adapter.UpdateChannel, pair it immediately with defer clock.UnFreeze() in the same scope and remove the later explicit UnFreeze call. Keep the existing update and error assertion unchanged.Source: Coding guidelines
openmeter/notification/channel.go (1)
99-115: 🔒 Security & Privacy | 🔵 TrivialNice tightening of the URL check — consider SSRF hardening as a follow-up.
The new check correctly rejects empty, malformed, non-http(s), and non-absolute URLs. That closes an obvious gap.
One thing to keep in mind: this URL is a live webhook delivery target. Format validation alone does not stop a user from pointing a webhook at an internal address (for example a private IP range or a cloud metadata endpoint). If the outbound webhook call path does not already apply an allow-list or deny private/link-local ranges, consider adding that check at request time.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/notification/channel.go` around lines 99 - 115, The current Validate method only checks URL format; add SSRF protection in the outbound webhook delivery path by enforcing the existing allow-list or rejecting private, loopback, link-local, and metadata-reserved destinations at request time. Keep WebHookChannelConfig.Validate focused on syntactic validation and ensure blocked targets cannot be requested.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/notification/adapter/channel.go`:
- Around line 30-35: Make the v3 channel-list behavior consistent for an omitted
filter.disabled value by ensuring ListChannelsInput.Disabled applies a default
false predicate, so disabled channels remain hidden unless explicitly requested.
Update the filter application in the channel adapter around
channeldb.FieldDisabled and preserve explicit disabled filter values;
alternatively, document the permissive default if that is the intended contract.
---
Nitpick comments:
In `@api/v3/handlers/notification/channels/list.go`:
- Around line 79-135: Extract the repeated apierrors.NewBadRequestError
construction in the filter parsing flow into a small named helper, such as
invalidFilterParam, accepting the context, field name, and error. Replace each
filter-specific three-line error block for ID, name, type, disabled, created_at,
and updated_at with this helper while preserving the existing field names and
validation behavior.
In `@openmeter/notification/channel.go`:
- Around line 99-115: The current Validate method only checks URL format; add
SSRF protection in the outbound webhook delivery path by enforcing the existing
allow-list or rejecting private, loopback, link-local, and metadata-reserved
destinations at request time. Keep WebHookChannelConfig.Validate focused on
syntactic validation and ensure blocked targets cannot be requested.
In `@openmeter/notification/service/channel_test.go`:
- Around line 134-143: In the test block that calls FreezeTime around
adapter.UpdateChannel, pair it immediately with defer clock.UnFreeze() in the
same scope and remove the later explicit UnFreeze call. Keep the existing update
and error assertion unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2549fe12-7b8f-4dee-b7a3-38290f62eabf
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (40)
api/spec/packages/aip-client-javascript/README.mdapi/spec/packages/aip-client-javascript/src/funcs/index.tsapi/spec/packages/aip-client-javascript/src/funcs/notifications.tsapi/spec/packages/aip-client-javascript/src/index.tsapi/spec/packages/aip-client-javascript/src/models/operations/notifications.tsapi/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip-client-javascript/src/sdk/internal.tsapi/spec/packages/aip-client-javascript/src/sdk/notifications.tsapi/spec/packages/aip/src/konnect.tspapi/spec/packages/aip/src/notifications/channel.tspapi/spec/packages/aip/src/notifications/index.tspapi/spec/packages/aip/src/notifications/operations.tspapi/spec/packages/aip/src/openmeter.tspapi/spec/packages/aip/src/shared/consts.tspapi/v3/api.gen.goapi/v3/client/README.mdapi/v3/client/client.goapi/v3/client/models_notifications.goapi/v3/client/notifications.goapi/v3/handlers/notification/channels/convert.goapi/v3/handlers/notification/channels/convert_test.goapi/v3/handlers/notification/channels/create.goapi/v3/handlers/notification/channels/delete.goapi/v3/handlers/notification/channels/error_encoder.goapi/v3/handlers/notification/channels/get.goapi/v3/handlers/notification/channels/handler.goapi/v3/handlers/notification/channels/list.goapi/v3/handlers/notification/channels/update.goapi/v3/server/routes.goapi/v3/server/server.goopenmeter/notification/adapter/channel.goopenmeter/notification/adapter/channel_test.goopenmeter/notification/channel.goopenmeter/notification/httpdriver/channel.goopenmeter/notification/service/channel.goopenmeter/notification/service/channel_test.goopenmeter/notification/service/rule.goopenmeter/server/server.gotest/notification/channel.go
borosr
force-pushed
the
feat/v3-notification-channels
branch
from
August 2, 2026 15:32
2e27f4a to
d33c926
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Notes for reviewer
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Greptile Summary
The PR adds v3 notification-channel CRUD APIs and corresponding generated JavaScript and Go SDK surfaces.
Confidence Score: 4/5
The PR is not yet safe to merge because clearing custom headers still leaves the previous values active in Svix.
The update mapper and service persist an empty header set, but the Svix updater skips its headers API when that set is empty, leaving delivery behavior inconsistent with the channel returned by the API.
Files Needing Attention: api/v3/handlers/notification/channels/convert.go, openmeter/notification/service/channel.go, openmeter/notification/webhook/svix/webhook.go
Important Files Changed
Sequence Diagram
Reviews (2): Last reviewed commit: "feat: notification channels v3 APIs" | Re-trigger Greptile
Context used: