feat(api): add v3 list events endpoint#4211
Merged
tothandras merged 2 commits intomainfrom Apr 23, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughInline deep-object pagination queries and remove small page models; add IngestedEvent and a ListMeteringEvents API with cursor pagination, filters, and sort; wire meterevent service through handlers/server; adopt store_row_id tie-break and stored_at/sort options across adapter, service, streaming queries, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as Events Handler
participant Service as MeterEvent Service
participant Adapter as Streaming Adapter
participant DB as ClickHouse
Client->>Handler: GET /events?page[after]=...&filter...&sort=...
Handler->>Handler: parse/validate page, filter, sort
Handler->>Service: ListEventsV2(params{SortBy,SortOrder,StoredAt,Limit,Cursor})
Service->>Adapter: stream query with SortBy/StoredAt/SortOrder
Adapter->>DB: Execute SQL (ORDER BY <sortCol> <dir>, store_row_id <dir>, WHERE ...)
DB-->>Adapter: rows
Adapter-->>Service: events (with StoreRowID, SortBy)
Service->>Service: build cursors using SortBy timestamp + StoreRowID
Service-->>Handler: items + pagination metadata
Handler->>Handler: convert items -> API IngestedEvent, wrap cursor response
Handler-->>Client: 200 CursorPaginatedResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v3/response/cursorpagination.go (1)
55-65:⚠️ Potential issue | 🟡 MinorHeads up:
nextcan point to an empty page when the last page is exactly full.Using
len(items) == pageSizeas the "there's more" signal produces a false positive whenever the total count is an exact multiple ofpageSize:nextwill be emitted, the client follows it, and gets back an empty page. The usual fix is the "fetch N+1" trick at the query layer — requestpageSize+1, return at mostpageSize, and only setnextwhen the extra row actually came back.Given this is the only caller today (
api/v3/handlers/events/list.go), it'd be worth threading a real "has more" bool fromListEventsV2through to here rather than inferring it from page fullness. Happy to sketch that out if useful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/response/cursorpagination.go` around lines 55 - 65, The pagination code currently sets result.Meta.Page.Next based on len(items) == pageSize which yields a false positive when total count is an exact multiple of pageSize; instead, change the caller ListEventsV2 to return an explicit hasMore (bool) and thread that through to this cursor pagination routine, then set result.Meta.Page.Next only when hasMore is true; locate the logic around items, pageSize and Cursor().Encode() and replace the len(items)==pageSize check with the new hasMore flag (or alternatively implement the fetch-(pageSize+1) behavior at ListEventsV2 and convert it into hasMore before calling this code).
🧹 Nitpick comments (5)
api/spec/packages/aip/src/events/event.tsp (1)
87-126: Nice addition —IngestedEventcleanly wraps the CloudEvent plus ingestion metadata, and the example is helpful.One tiny nit: the example uses
subject: "customer_key"while theMeteringEventexample above uses"customer-id". Not a blocker, but aligning them (or at least keeping the same naming convention) would make the docs read more consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/packages/aip/src/events/event.tsp` around lines 87 - 126, Update the example inside the IngestedEvent model so the CloudEvent subject value matches the MeteringEvent example naming convention; locate the example block near the IngestedEvent model and change subject: "customer_key" to subject: "customer-id" (or otherwise match the prior MeteringEvent example) so the documentation reads consistently; references: IngestedEvent model and the MeteringEvent example used earlier.api/v3/handlers/events/convert.go (2)
23-23: Small thing onTime.
nullable.NewNullableWithValue[api.DateTime](e.Time)will always emit a non-null timestamp, even ife.Timeis the zero value. CloudEvents'timeis technically optional and the spec here declares it asShared.DateTime | null. If there's any path where the source event lackstime, consider emitting null instead of a zero timestamp:if !e.Time.IsZero() { event.Time = nullable.NewNullableWithValue[api.DateTime](e.Time) }Feel free to ignore if
Timeis guaranteed populated upstream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/events/convert.go` at line 23, The Time field is always being set to a non-null zero timestamp via nullable.NewNullableWithValue[api.DateTime](e.Time); change the construction that sets event.Time (the code that uses e.Time and nullable.NewNullableWithValue) to only assign a non-null value when e.Time.IsZero() is false, and otherwise emit a null/empty nullable value (e.g., the zero nullable) so CloudEvents' optional time is represented as null when upstream time is absent.
32-32: Use the generated constantMeteringEventDatacontenttypeApplicationjsoninstead of casting the string.The codebase already has
MeteringEventDatacontenttypeApplicationjsondefined in api/v3/api.gen.go — using it will avoid potential typos and keeps things in sync if the enum value ever changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/events/convert.go` at line 32, Replace the manual cast to the string literal in the assignment to event.Datacontenttype with the generated enum constant; specifically, in convert.go where event.Datacontenttype is set (currently using api.MeteringEventDatacontenttype("application/json")), use the generated MeteringEventDatacontenttypeApplicationjson constant instead so the line becomes nullable.NewNullableWithValue[api.MeteringEventDatacontenttype](api.MeteringEventDatacontenttypeApplicationjson).api/spec/packages/aip/src/events/operations.tsp (1)
88-98: Considerdescas the default sort for event listings.Docs say the default sort order is ascending on
ingested_at. For an events feed, most users reach for "show me the most recent first" — defaulting todesc(or at least calling out that they'll likely wantingested_at desc) tends to match expectations better. Not a correctness issue, just a UX nudge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/packages/aip/src/events/operations.tsp` around lines 88 - 98, The comment for the query parameter sort?: Common.SortQuery currently says the default sort is ascending on ingested_at; change the documentation and any default behavior to use ingested_at desc instead (or explicitly state that ingested_at desc is the recommended default for event listings). Update the JSDoc above sort?: Common.SortQuery to list ingested_at (default: ingested_at desc) and, if there is server-side code that applies a default sort when sort is absent, change that default to use ingested_at descending (or add a clear note pointing callers to request ingested_at desc). Ensure any examples or tests referencing the old default are updated to reflect the new default ordering.openmeter/meterevent/service.go (1)
80-92: Minor: the emptyEventSortFieldTimecase + trailing return is a bit confusing.The
case streaming.EventSortFieldTime:branch has no body, so it just falls out of the switch and hits the barereturnat the bottom — same behavior asdefault. It works, but it reads like an accidental fallthrough. Consider collapsing them for clarity.♻️ Suggested tidy-up
func (e Event) Cursor() pagination.Cursor { switch e.SortBy { case streaming.EventSortFieldIngestedAt: return pagination.NewCursor(e.IngestedAt, e.ID) case streaming.EventSortFieldStoredAt: return pagination.NewCursor(e.StoredAt, e.ID) - case streaming.EventSortFieldTime: - default: - return pagination.NewCursor(e.Time, e.ID) + default: + return pagination.NewCursor(e.Time, e.ID) } - - return pagination.NewCursor(e.Time, e.ID) }One small thing worth double-checking: if a new
EventSortFieldis added later (e.g., a new timestamp column), the default branch silently falls back toTime— which would break keyset pagination against a query ordered by the new column. Might be worth a// NOTE:so the next person updatingEventSortFieldremembers to extend this too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/meterevent/service.go` around lines 80 - 92, The switch in Event.Cursor() currently has an empty case for streaming.EventSortFieldTime and a trailing return which is confusing; update the switch to collapse the Time case and default into a single branch that returns pagination.NewCursor(e.Time, e.ID) (remove the extra return after the switch), and add a short NOTE comment above the default/Time branch reminding future maintainers to update this switch when adding new EventSortField values so keyset pagination stays in sync with ordering changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/handlers/events/list_test.go`:
- Line 18: Replace uses of context.Background() in the test file with the
test-bound context from t.Context() (e.g., in the top-level setup where ctx :=
context.Background() and in TestParseEventSort at the noted location), and
remove the now-unused "context" import; ensure any variables named ctx are
assigned from t.Context() so the test context is cancelled with the test
lifecycle.
In `@api/v3/handlers/events/list.go`:
- Around line 110-121: You're passing result.Items ([]meterevent.Event) into
NewCursorPaginationResponse which expects the converted type and will call
.Cursor() on the wrong type; change the call to NewCursorPaginationResponse to
pass items (the []api.MeteringIngestedEvent returned by
toAPIMeteringIngestedEvent) so the pagination metadata matches
ListMeteringEventsResponse.Data, and simplify the pageSize logic by using the
already-computed pageSize variable instead of lo.FromPtrOr(req.Limit, ...).
Ensure references to items, result.Items, NewCursorPaginationResponse,
ListMeteringEventsResponse, pageSize, and req.Limit are updated accordingly.
---
Outside diff comments:
In `@api/v3/response/cursorpagination.go`:
- Around line 55-65: The pagination code currently sets result.Meta.Page.Next
based on len(items) == pageSize which yields a false positive when total count
is an exact multiple of pageSize; instead, change the caller ListEventsV2 to
return an explicit hasMore (bool) and thread that through to this cursor
pagination routine, then set result.Meta.Page.Next only when hasMore is true;
locate the logic around items, pageSize and Cursor().Encode() and replace the
len(items)==pageSize check with the new hasMore flag (or alternatively implement
the fetch-(pageSize+1) behavior at ListEventsV2 and convert it into hasMore
before calling this code).
---
Nitpick comments:
In `@api/spec/packages/aip/src/events/event.tsp`:
- Around line 87-126: Update the example inside the IngestedEvent model so the
CloudEvent subject value matches the MeteringEvent example naming convention;
locate the example block near the IngestedEvent model and change subject:
"customer_key" to subject: "customer-id" (or otherwise match the prior
MeteringEvent example) so the documentation reads consistently; references:
IngestedEvent model and the MeteringEvent example used earlier.
In `@api/spec/packages/aip/src/events/operations.tsp`:
- Around line 88-98: The comment for the query parameter sort?: Common.SortQuery
currently says the default sort is ascending on ingested_at; change the
documentation and any default behavior to use ingested_at desc instead (or
explicitly state that ingested_at desc is the recommended default for event
listings). Update the JSDoc above sort?: Common.SortQuery to list ingested_at
(default: ingested_at desc) and, if there is server-side code that applies a
default sort when sort is absent, change that default to use ingested_at
descending (or add a clear note pointing callers to request ingested_at desc).
Ensure any examples or tests referencing the old default are updated to reflect
the new default ordering.
In `@api/v3/handlers/events/convert.go`:
- Line 23: The Time field is always being set to a non-null zero timestamp via
nullable.NewNullableWithValue[api.DateTime](e.Time); change the construction
that sets event.Time (the code that uses e.Time and
nullable.NewNullableWithValue) to only assign a non-null value when
e.Time.IsZero() is false, and otherwise emit a null/empty nullable value (e.g.,
the zero nullable) so CloudEvents' optional time is represented as null when
upstream time is absent.
- Line 32: Replace the manual cast to the string literal in the assignment to
event.Datacontenttype with the generated enum constant; specifically, in
convert.go where event.Datacontenttype is set (currently using
api.MeteringEventDatacontenttype("application/json")), use the generated
MeteringEventDatacontenttypeApplicationjson constant instead so the line becomes
nullable.NewNullableWithValue[api.MeteringEventDatacontenttype](api.MeteringEventDatacontenttypeApplicationjson).
In `@openmeter/meterevent/service.go`:
- Around line 80-92: The switch in Event.Cursor() currently has an empty case
for streaming.EventSortFieldTime and a trailing return which is confusing;
update the switch to collapse the Time case and default into a single branch
that returns pagination.NewCursor(e.Time, e.ID) (remove the extra return after
the switch), and add a short NOTE comment above the default/Time branch
reminding future maintainers to update this switch when adding new
EventSortField values so keyset pagination stays in sync with ordering changes.
🪄 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
Run ID: c2e01bf0-2cfd-47eb-8d89-7301fa5abd81
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (18)
api/spec/packages/aip/src/common/pagination.tspapi/spec/packages/aip/src/events/event.tspapi/spec/packages/aip/src/events/operations.tspapi/v3/api.gen.goapi/v3/handlers/events/convert.goapi/v3/handlers/events/handler.goapi/v3/handlers/events/list.goapi/v3/handlers/events/list_test.goapi/v3/response/cursorpagination.goapi/v3/server/routes.goapi/v3/server/server.goopenmeter/meterevent/adapter/event.goopenmeter/meterevent/service.goopenmeter/meterevent/service_test.goopenmeter/server/server.goopenmeter/streaming/clickhouse/event_query_v2.goopenmeter/streaming/clickhouse/event_query_v2_test.goopenmeter/streaming/eventparams.go
GAlexIHU
reviewed
Apr 23, 2026
gergely-kurucz-konghq
requested changes
Apr 23, 2026
Contributor
gergely-kurucz-konghq
left a comment
gergely-kurucz-konghq
left a comment
There was a problem hiding this comment.
Minor comments for clarification, otherwise LGTM!
chrisgacsal
reviewed
Apr 23, 2026
chrisgacsal
requested changes
Apr 23, 2026
tothandras
force-pushed
the
feat/api-v3-events-list
branch
from
ddf5ef6 to
59faa2d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
api/v3/handlers/events/list.go (1)
197-209:⚠️ Potential issue | 🟠 MajorDon’t union
eqandoeqforcustomer_id.If a request includes both, this turns
eq=A+oeq=[B,C]intocustomer_id IN (A,B,C), which broadens the filter instead of preserving conjunctive filter semantics. Please either reject the combination or compute the intersection.🛡️ Safer minimal fix
var values []string + if f.Eq != nil && len(f.Oeq) > 0 { + return nil, filterError(ctx, "filter[customer_id]", errors.New("eq and oeq cannot be combined for customer_id")) + } + if f.Eq != nil { values = append(values, *f.Eq) }A test covering
Eq+Oeqtogether would be great here too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/events/list.go` around lines 197 - 209, The current code unions f.Eq and f.Oeq into a single IN list (creating filter.FilterString{In: &values}), which incorrectly broadens customer_id semantics when both Eq and Oeq are provided; change the logic to reject the combination: if f.Eq != nil && len(f.Oeq) > 0 return a validation/error (instead of building values), otherwise keep the existing behavior for only-Eq or only-Oeq; update the surrounding handler/function that consumes this builder to propagate the error and add a unit test that sends Eq+Oeq together and asserts the request is rejected.
🧹 Nitpick comments (1)
openmeter/streaming/clickhouse/event_query_v2_test.go (1)
119-176: Add cursor coverage for non-default sort columns.Nice coverage for sort and cursor separately. I’d add at least one case combining
CursorwithSortBy: ingested_atorstored_at, since that’s the key path where the predicate must switch columns too.🧪 Example test case
{ name: "query with explicit stored_at sort", query: queryEventsTableV2{ Database: "openmeter", EventsTableName: "om_events", Params: streaming.ListEventsV2Params{ Namespace: "my_namespace", SortBy: streaming.EventSortFieldStoredAt, }, }, wantSQL: "SELECT id, type, subject, source, time, data, ingested_at, stored_at, store_row_id FROM openmeter.om_events WHERE namespace = ? ORDER BY stored_at DESC, store_row_id DESC LIMIT ?", wantArgs: []interface{}{"my_namespace", 100}, }, + { + name: "query with explicit stored_at sort and cursor", + query: queryEventsTableV2{ + Database: "openmeter", + EventsTableName: "om_events", + Params: streaming.ListEventsV2Params{ + Namespace: "my_namespace", + Cursor: &pagination.Cursor{ + Time: cursorTime, + ID: cursorID, + }, + SortBy: streaming.EventSortFieldStoredAt, + }, + }, + wantSQL: "SELECT id, type, subject, source, time, data, ingested_at, stored_at, store_row_id FROM openmeter.om_events WHERE namespace = ? AND stored_at <= ? AND (stored_at < ? OR store_row_id < ?) ORDER BY stored_at DESC, store_row_id DESC LIMIT ?", + wantArgs: []interface{}{"my_namespace", cursorTime.Unix(), cursorTime.Unix(), cursorID, 100}, + },As per coding guidelines, "Make sure the tests are comprehensive and cover the changes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/streaming/clickhouse/event_query_v2_test.go` around lines 119 - 176, Add tests that exercise Cursor combined with non-default SortBy values so the cursor predicate switches columns; specifically add one or two table-driven cases in event_query_v2_test.go using queryEventsTableV2 with Params including Cursor (pagination.Cursor with Time and ID) and SortBy set to streaming.EventSortFieldIngestedAt and streaming.EventSortFieldStoredAt respectively, and assert the generated SQL (wantSQL) and args (wantArgs) expect the cursor comparisons to use ingested_at (or stored_at) columns and the correct ordering and placeholders (e.g., "AND ingested_at >= ? AND (ingested_at > ? OR store_row_id > ?)" / "AND stored_at >= ? AND (stored_at > ? OR store_row_id > ?)") matching the existing patterns used for time-based cursor tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/spec/packages/aip/src/events/operations.tsp`:
- Around line 88-99: The doc comment for the sort query is inaccurate: the
implementation treats an omitted sort as "time desc", but when a client
explicitly sets sort=time or sort=stored_at the parsing logic (ParseSortBy)
defaults to ascending. Update the comment above the sort?: Common.SortQuery
declaration to state that omitting the parameter defaults to "time desc",
whereas supplying an attribute without an asc/desc suffix (e.g. sort=time or
sort=stored_at) will default to ascending per ParseSortBy; also keep examples of
using the asc/desc suffix to override.
- Around line 49-52: The spec exposes customer_id as Common.ULIDFieldFilter but
the handler (api/v3/handlers/events/list.go) only supports eq and oeq; narrow
the contract by replacing customer_id?: Common.ULIDFieldFilter with an
endpoint-specific filter type (e.g., Events.ULIDEqualityFilter) that only
includes eq and oeq, or at minimum update the JSDoc above customer_id to
explicitly document that only eq and oeq are supported; reference the
customer_id field and Common.ULIDFieldFilter and the events list handler to
ensure parity.
In `@openmeter/meterevent/service.go`:
- Around line 239-241: The code validates p.SortBy but not p.SortOrder, allowing
invalid sort directions to default to descending; add validation for p.SortOrder
right after the SortBy check—either call p.SortOrder.Validate() if that method
exists or explicitly check p.SortOrder against the allowed constants (e.g.,
SortOrderAsc, SortOrderDesc) and append an error like fmt.Errorf("sort order:
%w", err) (or fmt.Errorf("sort order: invalid value %q", p.SortOrder)) to errs
so bad callers are rejected rather than silently treated as descending.
---
Duplicate comments:
In `@api/v3/handlers/events/list.go`:
- Around line 197-209: The current code unions f.Eq and f.Oeq into a single IN
list (creating filter.FilterString{In: &values}), which incorrectly broadens
customer_id semantics when both Eq and Oeq are provided; change the logic to
reject the combination: if f.Eq != nil && len(f.Oeq) > 0 return a
validation/error (instead of building values), otherwise keep the existing
behavior for only-Eq or only-Oeq; update the surrounding handler/function that
consumes this builder to propagate the error and add a unit test that sends
Eq+Oeq together and asserts the request is rejected.
---
Nitpick comments:
In `@openmeter/streaming/clickhouse/event_query_v2_test.go`:
- Around line 119-176: Add tests that exercise Cursor combined with non-default
SortBy values so the cursor predicate switches columns; specifically add one or
two table-driven cases in event_query_v2_test.go using queryEventsTableV2 with
Params including Cursor (pagination.Cursor with Time and ID) and SortBy set to
streaming.EventSortFieldIngestedAt and streaming.EventSortFieldStoredAt
respectively, and assert the generated SQL (wantSQL) and args (wantArgs) expect
the cursor comparisons to use ingested_at (or stored_at) columns and the correct
ordering and placeholders (e.g., "AND ingested_at >= ? AND (ingested_at > ? OR
store_row_id > ?)" / "AND stored_at >= ? AND (stored_at > ? OR store_row_id >
?)") matching the existing patterns used for time-based cursor tests.
🪄 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
Run ID: b61c351a-42a8-4eea-9215-e8b187ea1a95
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (10)
api/spec/packages/aip/src/events/operations.tspapi/v3/api.gen.goapi/v3/handlers/events/list.goapi/v3/handlers/events/list_test.goopenmeter/meterevent/adapter/event.goopenmeter/meterevent/service.goopenmeter/meterevent/service_test.goopenmeter/streaming/clickhouse/event_query_v2.goopenmeter/streaming/clickhouse/event_query_v2_test.goopenmeter/streaming/eventparams.go
🚧 Files skipped from review as they are similar to previous changes (3)
- openmeter/meterevent/service_test.go
- openmeter/streaming/eventparams.go
- openmeter/streaming/clickhouse/event_query_v2.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/spec/packages/aip/src/events/event.tsp (1)
1-3:⚠️ Potential issue | 🟠 MajorAdd missing
Customersimport to resolve compilation error.Line 115 uses
Customers.Customer, but the file only imports../shared/index.tsp, which doesn't export the Customers namespace. This will cause TypeSpec compilation to fail. Addimport "../customers/index.tsp";near the top with the other imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/packages/aip/src/events/event.tsp` around lines 1 - 3, The TypeSpec file imports openapi and ../shared/index.tsp but omits the Customers namespace used later (Customers.Customer), causing compilation failure; add an import for the customers module (import "../customers/index.tsp";) alongside the existing imports at the top of the file so the Customers namespace is available to the event.tsp definitions referencing Customers.Customer.
♻️ Duplicate comments (1)
openmeter/meterevent/service.go (1)
239-241:⚠️ Potential issue | 🟡 MinorSort order still needs validation too.
SortByis validated now, but an invalidSortOrdercan still slip through and be treated like descending by the SQL layer. Keeping this as a duplicate note since it was already called out.Small validation patch
if err := p.SortBy.Validate(); err != nil { errs = append(errs, fmt.Errorf("sort by: %w", err)) } + + switch p.SortOrder { + case "", sortx.OrderAsc, sortx.OrderDesc: + default: + errs = append(errs, fmt.Errorf("sort order: invalid value %q", p.SortOrder)) + } if p.Limit != nil && *p.Limit < 1 {#!/bin/bash # Verify the available sortx.Order constants / validation helpers before applying the patch. rg -n -C3 'type\s+Order|OrderAsc|OrderDesc|func\s+\(.*Order.*\)\s+Validate' --iglob '*.go'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/meterevent/service.go` around lines 239 - 241, The code validates p.SortBy but not the sort order; ensure you also validate the sort order (e.g., call the Validate method on the sort order type or use the provided helper such as sortx.Order.Validate / OrderAsc/OrderDesc validation) and append any error to errs with a clear prefix like fmt.Errorf("sort order: %w"); locate where p.SortBy.Validate() is called and add the analogous validation call for the SortOrder field (or p.SortOrder.Validate()) and error handling.
🧹 Nitpick comments (1)
openmeter/streaming/clickhouse/event_query_v2_test.go (1)
119-175: Add cursor coverage foringested_at/stored_atsorts.Nice coverage for the new
ORDER BYcases. The spicy bit isSortBy+Cursor: please add cases that assert the cursor predicate switches toingested_atandstored_at, not just the ordering. That would catch regressions where the SQL orders by one column but pages bytime.Suggested test shape
+ { + name: "query with explicit ingested_at sort and cursor", + query: queryEventsTableV2{ + Database: "openmeter", + EventsTableName: "om_events", + Params: streaming.ListEventsV2Params{ + Namespace: "my_namespace", + Cursor: &pagination.Cursor{ + Time: cursorTime, + ID: cursorID, + }, + SortBy: streaming.EventSortFieldIngestedAt, + }, + }, + wantSQL: "SELECT id, type, subject, source, time, data, ingested_at, stored_at, store_row_id FROM openmeter.om_events WHERE namespace = ? AND ingested_at <= ? AND (ingested_at < ? OR store_row_id < ?) ORDER BY ingested_at DESC, store_row_id DESC LIMIT ?", + wantArgs: []interface{}{"my_namespace", cursorTime.Unix(), cursorTime.Unix(), cursorID, 100}, + }, + { + name: "query with explicit stored_at sort and cursor", + query: queryEventsTableV2{ + Database: "openmeter", + EventsTableName: "om_events", + Params: streaming.ListEventsV2Params{ + Namespace: "my_namespace", + Cursor: &pagination.Cursor{ + Time: cursorTime, + ID: cursorID, + }, + SortBy: streaming.EventSortFieldStoredAt, + }, + }, + wantSQL: "SELECT id, type, subject, source, time, data, ingested_at, stored_at, store_row_id FROM openmeter.om_events WHERE namespace = ? AND stored_at <= ? AND (stored_at < ? OR store_row_id < ?) ORDER BY stored_at DESC, store_row_id DESC LIMIT ?", + wantArgs: []interface{}{"my_namespace", cursorTime.Unix(), cursorTime.Unix(), cursorID, 100}, + },As per coding guidelines,
**/*_test.go: “Make sure the tests are comprehensive and cover the changes.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/streaming/clickhouse/event_query_v2_test.go` around lines 119 - 175, Add test cases in the same table-driven slice that exercise queryEventsTableV2 with Params.Cursor plus Params.SortBy set to streaming.EventSortFieldIngestedAt and streaming.EventSortFieldStoredAt; for each case assert wantSQL contains the cursor predicate using ingested_at (e.g. "ingested_at >= ? AND (ingested_at > ? OR store_row_id > ?)") or stored_at respectively and the matching ORDER BY (e.g. "ORDER BY ingested_at DESC, store_row_id DESC" or ASC if SortOrder set), and set wantArgs to include the cursor time (cursorTime.Unix()) and cursor ID in the same positions as the existing time-cursor test so the test verifies both the paging predicate and the ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/meterevent/adapter/event.go`:
- Around line 129-134: The code may emit a misleading NextCursor when
params.Limit is nil because effectiveLimit falls back to
meterevent.MaximumLimit; change the logic in the block that sets
result.NextCursor so it only computes and assigns a cursor when params.Limit !=
nil and len(meterEvents) > 0 and len(meterEvents) == effectiveLimit (i.e., add a
check for params.Limit != nil before calling meterEvents[len-1].Cursor()),
keeping the existing use of effectiveLimit, Cursor(), and the
ListEventsV2-related behavior otherwise unchanged.
---
Outside diff comments:
In `@api/spec/packages/aip/src/events/event.tsp`:
- Around line 1-3: The TypeSpec file imports openapi and ../shared/index.tsp but
omits the Customers namespace used later (Customers.Customer), causing
compilation failure; add an import for the customers module (import
"../customers/index.tsp";) alongside the existing imports at the top of the file
so the Customers namespace is available to the event.tsp definitions referencing
Customers.Customer.
---
Duplicate comments:
In `@openmeter/meterevent/service.go`:
- Around line 239-241: The code validates p.SortBy but not the sort order;
ensure you also validate the sort order (e.g., call the Validate method on the
sort order type or use the provided helper such as sortx.Order.Validate /
OrderAsc/OrderDesc validation) and append any error to errs with a clear prefix
like fmt.Errorf("sort order: %w"); locate where p.SortBy.Validate() is called
and add the analogous validation call for the SortOrder field (or
p.SortOrder.Validate()) and error handling.
---
Nitpick comments:
In `@openmeter/streaming/clickhouse/event_query_v2_test.go`:
- Around line 119-175: Add test cases in the same table-driven slice that
exercise queryEventsTableV2 with Params.Cursor plus Params.SortBy set to
streaming.EventSortFieldIngestedAt and streaming.EventSortFieldStoredAt; for
each case assert wantSQL contains the cursor predicate using ingested_at (e.g.
"ingested_at >= ? AND (ingested_at > ? OR store_row_id > ?)") or stored_at
respectively and the matching ORDER BY (e.g. "ORDER BY ingested_at DESC,
store_row_id DESC" or ASC if SortOrder set), and set wantArgs to include the
cursor time (cursorTime.Unix()) and cursor ID in the same positions as the
existing time-cursor test so the test verifies both the paging predicate and the
ordering.
🪄 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
Run ID: e0f4181e-a31b-40bf-bc6d-acca5fc2b23e
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (18)
api/spec/packages/aip/src/common/pagination.tspapi/spec/packages/aip/src/events/event.tspapi/spec/packages/aip/src/events/operations.tspapi/v3/api.gen.goapi/v3/handlers/events/convert.goapi/v3/handlers/events/handler.goapi/v3/handlers/events/list.goapi/v3/handlers/events/list_test.goapi/v3/response/cursorpagination.goapi/v3/server/routes.goapi/v3/server/server.goopenmeter/meterevent/adapter/event.goopenmeter/meterevent/service.goopenmeter/meterevent/service_test.goopenmeter/server/server.goopenmeter/streaming/clickhouse/event_query_v2.goopenmeter/streaming/clickhouse/event_query_v2_test.goopenmeter/streaming/eventparams.go
✅ Files skipped from review due to trivial changes (3)
- openmeter/meterevent/service_test.go
- openmeter/streaming/eventparams.go
- api/v3/handlers/events/list.go
🚧 Files skipped from review as they are similar to previous changes (3)
- api/v3/server/routes.go
- openmeter/streaming/clickhouse/event_query_v2.go
- api/v3/handlers/events/convert.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/v3/handlers/events/list_test.go (2)
65-112: Great sort parsing coverage — add the empty string case as well.
parseEventSorttreatsniland""as the same default path, but onlynilis covered here. A quickapi.SortQuery("")subtest would guard that explicit branch.🧪 Suggested test addition
t.Run("nil returns empty values", func(t *testing.T) { field, order, err := parseEventSort(ctx, nil) require.NoError(t, err) require.Equal(t, streaming.EventSortField(""), field) require.Equal(t, sortx.Order(""), order) }) + + t.Run("empty sort returns empty values", func(t *testing.T) { + sort := api.SortQuery("") + field, order, err := parseEventSort(ctx, &sort) + require.NoError(t, err) + require.Equal(t, streaming.EventSortField(""), field) + require.Equal(t, sortx.Order(""), order) + }) t.Run("time defaults to asc", func(t *testing.T) {As per coding guidelines, "
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/events/list_test.go` around lines 65 - 112, Add a subtest to TestParseEventSort that passes an explicit empty api.SortQuery("") into parseEventSort to mirror the existing "nil returns empty values" case; call field, order, err := parseEventSort(ctx, &sort) where sort := api.SortQuery(""), require no error, and assert that field equals streaming.EventSortField("") and order equals sortx.Order("") so the explicit empty-string branch is covered just like the nil branch.
16-63: Nice filter coverage — one tiny branch is still worth pinning down.Could we add the non-nil empty filter case too?
fromAPICustomerIDFilterhas a separatelen(values) == 0path, so this would lock down behavior for&api.ULIDFieldFilter{}as distinct fromnil.🧪 Suggested test addition
t.Run("nil filter returns nil", func(t *testing.T) { out, err := fromAPICustomerIDFilter(ctx, nil) require.NoError(t, err) require.Nil(t, out) }) + + t.Run("empty filter returns nil", func(t *testing.T) { + out, err := fromAPICustomerIDFilter(ctx, &api.ULIDFieldFilter{}) + require.NoError(t, err) + require.Nil(t, out) + }) t.Run("eq maps to In with one element", func(t *testing.T) {As per coding guidelines, "
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/events/list_test.go` around lines 16 - 63, Add a test case exercising the non-nil empty filter path in fromAPICustomerIDFilter: call fromAPICustomerIDFilter(ctx, &api.ULIDFieldFilter{}) and assert no error, that the returned filter is non-nil, that out.In is non-nil, and that len(*out.In) == 0 so the behavior is locked down as distinct from the nil-filter case already covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/v3/handlers/events/list_test.go`:
- Around line 65-112: Add a subtest to TestParseEventSort that passes an
explicit empty api.SortQuery("") into parseEventSort to mirror the existing "nil
returns empty values" case; call field, order, err := parseEventSort(ctx, &sort)
where sort := api.SortQuery(""), require no error, and assert that field equals
streaming.EventSortField("") and order equals sortx.Order("") so the explicit
empty-string branch is covered just like the nil branch.
- Around line 16-63: Add a test case exercising the non-nil empty filter path in
fromAPICustomerIDFilter: call fromAPICustomerIDFilter(ctx,
&api.ULIDFieldFilter{}) and assert no error, that the returned filter is
non-nil, that out.In is non-nil, and that len(*out.In) == 0 so the behavior is
locked down as distinct from the nil-filter case already covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c82883c-1254-4b16-b70a-10fc6cb36b82
📒 Files selected for processing (1)
api/v3/handlers/events/list_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/v3/handlers/events/list_test.go (1)
33-38: Tiny nit: guard theout.Inderef for consistency.The
eqsubtest above checksrequire.NotNil(t, out.In)before dereferencing, but thisoeqcase jumps straight to*out.In. If the mapping ever regresses to leaveInnil, you'll get a panic instead of a clean assertion failure. Cheap to stay symmetric:♻️ Proposed tweak
t.Run("oeq maps to In", func(t *testing.T) { out, err := fromAPICustomerIDFilter(ctx, &api.ULIDFieldFilter{Oeq: []string{"a", "b"}}) require.NoError(t, err) require.NotNil(t, out) + require.NotNil(t, out.In) require.Equal(t, []string{"a", "b"}, *out.In) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/events/list_test.go` around lines 33 - 38, The oeq subtest in list_test.go dereferences out.In without checking for nil; update the "oeq maps to In" test that calls fromAPICustomerIDFilter to first assert require.NotNil(t, out.In) (matching the "eq" subtest) before calling require.Equal on *out.In to avoid a panic if the mapping regresses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/v3/handlers/events/list_test.go`:
- Around line 33-38: The oeq subtest in list_test.go dereferences out.In without
checking for nil; update the "oeq maps to In" test that calls
fromAPICustomerIDFilter to first assert require.NotNil(t, out.In) (matching the
"eq" subtest) before calling require.Equal on *out.In to avoid a panic if the
mapping regresses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9e6f6982-2dde-4c30-adf3-6cd27b2b2faa
📒 Files selected for processing (2)
api/v3/handlers/events/list.goapi/v3/handlers/events/list_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/v3/handlers/events/list.go
tothandras
force-pushed
the
feat/api-v3-events-list
branch
from
21fe06b to
94b3758
Compare
tothandras
force-pushed
the
feat/api-v3-events-list
branch
from
94b3758 to
12a529b
Compare
turip
approved these changes
Apr 23, 2026
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
5 participants
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.
Summary by CodeRabbit
New Features
Improvements
Tests