fix(alerts): declare the event types each resource can deliver, reject and stop offering the rest - #30587
fix(alerts): declare the event types each resource can deliver, reject and stop offering the rest#30587manerow wants to merge 1 commit into
Conversation
✅ TypeScript Types Auto-UpdatedThe generated TypeScript types have been automatically updated based on JSON schema changes in this PR. |
fbccf37 to
4f1338f
Compare
f4e0c77 to
0ae5868
Compare
…t and stop offering the rest
0ae5868 to
35188bb
Compare
Code Review ✅ Approved 2 resolved / 2 findingsAdds backend event-type declarations and save-time validation for notification subscriptions, restricting the alert builder to resource-compatible event types. No issues found. ✅ 2 resolved✅ Quality: clearFields comment contradicts strict enforcement behavior
✅ Edge Case: Event-type validation ignores filter Effect (rejects EXCLUDE too)
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source |
|
⚪ Playwright Results — workflow was cancelledValidated commit ✅ 744 passed · ❌ 0 failed · 🟡 2 flaky · ⏭️ 5 skipped · 🧰 0 lifecycle flaky Pipeline and setup failures (1)
PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 1h 19m 27s ⏱️ Max setup 3m 0s · max shard execution 16m 5s · max shard-job elapsed before upload 29m 46s · reporting 6s 🌐 196.91 requests/attempt · 2.53 app boots/UI scenario · 23.94% common-shard skew Optimization targets still in progress:
🟡 2 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |



Fixes #30557
Fixes #29040
Both halves of the "silent no-op" complaint in #27889: the server declares what each resource can
deliver and rejects the rest, and the alert builder offers only what the server declares. They ship
together so the UI can never present a combination the API will refuse.
Problem
An alert can be saved with event types its resource can never produce, and it then delivers nothing
with no error and no empty state. Reproduced on
main:Two blind spots:
GET /v1/events/subscriptions/notification/resourcesreturned only
name,supportedFilters,supportedActions,containerEntities, so the builderfalls back to the whole 26-value
EventTypeenum.AlertUtil.getFilterRulerejects afilter the resource does not support, but never inspects the arguments inside it.
What each resource can actually deliver
Derived from the emitters rather than from the enum:
glossaryTerm,topic, …)entityCreated,entityUpdated,entitySoftDeleted,entityDeleted,entityRestoredplusthreadCreated,threadUpdated,postCreated,postUpdated, because #28122 routes a conversation to the alert of the entity it is abouttable,dashboard,pipeline,chart,mlmodelentityFieldsChangedconversationtasktaskResolved/taskClosed, still emitted by the legacy Recognizer path (#30559 retires them)alllogicalTestCaseAddedandentityLineageAdded/Updated/Deleted; it short-circuits the resource gate, so it is the only resource that sees events whoseentityType(lineage,testSuite) is not a notification resourceNever declared, because nothing can deliver them:
entityNoChange,taskCreated,taskUpdated,the five
suggestion*,userLogin,userLogout.Three of those deserve a note, since they are easy to assume otherwise:
entityFieldsChangedis not generic. Only usage reporting emits it (UsageRepository:121-125), for exactly five entities.QueryRepositoryalso emits it, butChangeEventHandler:57-59drops everyqueryandworkflowevent before insert.userLogin/userLogoutnever reachchange_event.AuditLogRepository.writeAuthEventbuilds aChangeEvent, butwrite()only inserts anAuditLogRecord. Verified on a running instance: 22 logins, all inaudit_log_event, none inchange_event.entityNoChangeis a sentinel thatChangeEventHandler:77explicitly skips.Changes (server)
filterResourceDescriptor.jsongainssupportedEventTypes, so the descriptor already served at/notification/resourcesnow carries the declaration and the UI needs no new endpoint.ResourceEventTypesencodes the table above: two family constants, a usage-resource set, explicit entries forall,conversationandtask, and the entity+thread default for everything else.EventSubscriptionResource.getNotificationsFilterDescriptors()attaches it where it already maps filter names onto rules.AlertUtil.validateAndBuildFilteringConditionsvalidatesfilterByEventTypevalues for notification alerts and throwsBadRequestExceptionnaming the resource and the value, with the message next toresourceTypeNotFoundinCatalogExceptionMessage.The input descriptor (
EventSubResourceDescriptor.json) is deliberately untouched: the list isderived, not hand-maintained, since 28 hand-written lists are how the current descriptor drifted.
Observability is unaffected:
EntityObservabilityFilterDescriptor.jsonoffers nofilterByEventType.Changes (UI)
AlertsUtil.tsxbuilt the Event Type field from the wholeEventTypeenum(
getSelectOptionsFromEnum(EventType)), regardless of the selected resource. It now uses theresource's
supportedEventTypes, falling back to the enum when a resource declares none, whichkeeps a newer UI working against an older server and leaves observability alone (its descriptors
carry no
filterByEventType).supportedEventTypesis threaded exactly like the existingcontainerEntities: derived where thedescriptor is already looked up (
AddNotificationPage, andAlertConfigDetailsfor view mode),passed into
ObservabilityFormFiltersItem, and forwarded throughgetConditionalFieldintogetFieldByArgumentType. One new pure helper,getSelectOptionsFromValues, sits besidegetSelectOptionsFromEnumand uses the samestartCaselabel transform, which keeps the Playwrightselectors (
[title="${startCase(eventType)}"]) and the view-mode labels working.No new i18n strings.
An alert saved before this change that stores a now-unadvertised value still shows it as a
removable chip, labelled with the raw value, so it can be cleared. Verified in the browser rather
than assumed.
Behaviour change worth noting in release notes
Enforcement is strict for create and update alike, for filters that include an event type. An
alert upgraded from an older release that includes a now-unreachable value (
suggestionCreated,taskResolved, …) must have that value removed before its next save; the 400 names it. The value wasalready dead, so nothing that used to fire stops firing. #29039's migration clears those values
wholesale.
An
EXCLUDEfilter is left alone: excluding an event type the resource cannot produce is a harmlessno-op, not an alert that can never fire, so it is not worth blocking a save over.
Tests
AlertsUtil.test.tsx: the field offers only the values it is given, and falls back to the whole enum when given none.ResourceEventTypesTest: per-resource assertions, plus an anti-drift test asserting that the union of every declared list and the never-deliverable set is exactlyEventType.values(), so adding an enum value without deciding where it belongs fails the build.EventTypeValidationTest: the gate, includingtableacceptingentityFieldsChangedwhileglossaryTermrejects it, and an unknown string being rejected rather than throwing.EventSubscriptionResourceIT: 400 on an unreachable value naming it, a reachable value still saving, and/notification/resourcesservingsupportedEventTypesover the wire.Verified on a local deploy
And in the alert builder, on a
Glossary Termalert: searching the Event Type dropdown for"sugg" or "fields" returns No data, while "thread" offers Thread Created and Thread Updated.
The pre-existing alert that stores
suggestionCreatedstill renders it as a removable chip.Greptile Summary
The PR aligns notification alert configuration with the event types each resource can actually emit.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as Alert Builder participant API as Event Subscription API participant Registry as Resource Descriptor Registry participant Validator as AlertUtil UI->>API: GET notification resources API->>Registry: Build resource descriptors Registry-->>API: supportedFilters + supportedEventTypes API-->>UI: Resource capabilities UI->>UI: Limit event-type options for selected resource UI->>API: Create or update subscription API->>Validator: Validate filterByEventType values Validator->>Registry: Check resource event capabilities alt Every included event type is supported Validator-->>API: Valid filtering rules API-->>UI: Subscription saved else Unsupported or unknown event type Validator-->>API: BadRequestException API-->>UI: 400 naming resource and event type endReviews (5): Last reviewed commit: "fix(alerts): declare the event types eac..." | Re-trigger Greptile