fix(hub): honor subject filter in queue/recent endpoint - #81
Open
DominikPinsel wants to merge 3 commits into
Open
fix(hub): honor subject filter in queue/recent endpoint#81DominikPinsel wants to merge 3 commits into
DominikPinsel wants to merge 3 commits into
Conversation
The MCP list_recent_events tool advertised a filter parameter and forwarded it as ?filter=... to /api/v1/queue/recent, but the hub handler only read count and connector — the filter was silently dropped and callers always got all event types. - add eventqueue.ParseSubjectFilter for subject patterns of the form '<connector>.<eventType>' with '*' (single token) and trailing '>' (any remainder) wildcards; event type is derived from the webhook event-type header (any header ending in '-Event', falling back to a 'type' entry), mirroring trigger.CanonicalEventType - RecentEvents applies the filter in SQL (jsonb header scan), so it is evaluated before the LIMIT - handleQueueRecent now reads the filter query parameter - update the MCP tool description to match the implemented semantics Closes #77
…filter # Conflicts: # services/mcp/internal/tools/events.go
Owner
Author
|
@review-agent please review |
There was a problem hiding this comment.
Review Summary
Overall: Approve
Correct, well-scoped fix for #77: the filter parameter is now actually parsed, applied in SQL before the LIMIT, and documented in the MCP tool description. Tests are solid and CI is green.
Blockers
- None.
Suggestions
services/hub/internal/eventqueue/store_filter_test.go—TestStore_RecentEventsSubjectFilteris gated onTEST_DB_URL, so the SQL side of the filter (thejsonb_each_text/headers->>'type'condition inRecentEvents) is not exercised in CI — only the pure-GoParseSubjectFilter/Matchespaths are. This is consistent with the existing store tests, so not a blocker, but a CI job with a postgres service container would cover the most failure-prone part of this change (e.g. jsonb column typing, theILIKE '%-event'escaping).services/hub/internal/api/handlers_eventqueue.go:284— if a caller passes bothconnector=githubandfilter=forgejo.push, the two conditions are ANDed and the result is always empty. That's a defensible intersection semantic, but a short doc note on the endpoint (or a 400 on conflicting params) would spare future callers some confusion.
Nits
services/hub/internal/eventqueue/store.go—SubjectFilter.Matchesis only used by tests in this PR (the query path uses SQL). Fine to keep as the package's reference implementation of the semantics, just noting it.
What's Done Well
- Filtering is applied in SQL before the
LIMIT, socountcorrectly means "up to N matching events" — the subtle correctness point of this fix. ParseSubjectFilteris pure, table-driven tested (22 cases), and impossible-to-match patterns short-circuit to an empty result instead of hitting the database.- Event-type derivation mirrors
trigger.CanonicalEventType(any*-Eventheader withtypefallback), keeping filter semantics aligned with trigger matching. - The MCP tool description and the stale
agent.olli.>example were updated to match the implemented behavior, satisfying both acceptance criteria from #77. - Integration test inserts uniquely-suffixed rows and cleans up after itself.
Testing
Adequate. Unit tests cover parser and matcher comprehensively, including case-insensitivity, wildcards, and malformed patterns; the integration test covers the SQL path against a real database (verified locally per the PR description). Only gap is CI coverage of the SQL path, captured as a suggestion above.
|
@DominikPinsel LGTM — approved. |
Resolve merge conflicts by integrating the PR's SubjectFilter feature into main's new EventFilter/QueryEvents/CountEvents architecture (PR #82): - store.go: keep SubjectFilter/ParseSubjectFilter/Matches from the PR; add Subject field to EventFilter; apply subject filter in conditions() so both QueryEvents and RecentEvents honor it; RecentEvents keeps its 5-param signature and delegates to QueryEvents with the Subject set. - handlers_events.go: use main's QueryEvents with EventFilter (the listEvents endpoint does not use subject filtering). - handlers_eventqueue.go: auto-merged, calls the 5-param RecentEvents.
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
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.
Closes #77
Problem
list_recent_eventsadvertised afilterparameter and forwarded it as?filter=...to/api/v1/queue/recent, buthandleQueueRecentonly readcountandconnector— the filter was silently dropped and callers always got all event types.Changes
Implemented option 1 from the issue (hub understands the filter):
eventqueue.ParseSubjectFilter— subject patterns of the form<connector>.<eventType>, case-insensitive, with*matching a single token and a trailing>matching any remainder (e.g.forgejo.push,forgejo.*,*.push). The event type is derived the same way as trigger matching (trigger.CanonicalEventType): any header ending in-Event, falling back to a generictypeentry.Store.RecentEventsapplies the filter in SQL (ajsonb_each_textscan of the stored headers), so filtering happens before theLIMIT—countstill means "up to N matching events".handleQueueRecentreads thefilterquery parameter.agent.olli.>example is gone).Patterns that cannot match (e.g. subjects deeper than two levels with non-wildcard tokens) short-circuit to an empty result instead of hitting the database.
Acceptance criteria
list_recent_eventswithfilterreturns only matching eventsTests
ParseSubjectFilterandSubjectFilter.Matches(table-driven, 22 cases)TestStore_RecentEventsSubjectFilter(gated onTEST_DB_URL, consistent with the existing store tests) — verified locally against postgres:17-alpine with migration 0014 applied: all 8 filter cases pass, including theheaders->>typefallback and case-insensitive matchinggo build,go test(hub eventqueue + api, mcp tools) andgolangci-lint runall pass.