feat(api): add validation errors to v3 ingested events#4222
Merged
tothandras merged 2 commits intomainfrom Apr 24, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new validation error model and exposes optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as API Handler\n(ListMeteringEvents)
participant Conv as Converter\n(toAPIMeteringIngestedEvent)
participant Backend as Backend/DB
rect rgba(135,206,250,0.5)
Client->>API: GET /events?sort=...
API->>API: validate query (fromAPIEventSort, fromAPICustomerIDFilter)
end
rect rgba(144,238,144,0.5)
API->>Backend: Query events with filters/sort
Backend-->>API: events (with possible validation errors)
end
rect rgba(255,228,181,0.5)
API->>Conv: convert events -> API models (include ValidationErrors)
Conv-->>API: MeteringIngestedEvent (+ValidationErrors)
end
API-->>Client: 200 OK (events JSON)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 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 |
tothandras
force-pushed
the
fix/v3-openapi
branch
from
07e68f1 to
dbe6e94
Compare
tothandras
changed the title
![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
🧹 Nitpick comments (2)
api/v3/handlers/events/convert.go (2)
52-73: Small simplification opportunity.The function works correctly, just a bit busier than it needs to be — the early
len(errs) == 0guard is redundant (lo.FilterMapon an empty slice yields an empty result, which the laterlen(result) == 0check already handles), and&resultworks fine in place oflo.ToPtr(result)sinceresultis already addressable. Totally optional, feel free to ignore if you prefer the current shape.♻️ Optional tidy-up
func toAPIMeteringIngestedEventValidationErrors(errs []error) *[]api.MeteringIngestedEventValidationError { - if len(errs) == 0 { - return nil - } - result := lo.FilterMap(errs, func(err error, _ int) (api.MeteringIngestedEventValidationError, bool) { if err == nil { return api.MeteringIngestedEventValidationError{}, false } return api.MeteringIngestedEventValidationError{ Code: "validation_error", Message: err.Error(), }, true }) if len(result) == 0 { return nil } - return lo.ToPtr(result) + return &result }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/events/convert.go` around lines 52 - 73, In toAPIMeteringIngestedEventValidationErrors remove the redundant early guard that checks len(errs) == 0 (lo.FilterMap already handles empty slices) and return a pointer to result using &result instead of lo.ToPtr(result); keep the lo.FilterMap logic and the final nil check for empty result unchanged.
57-66: Every validation error gets the same"validation_error"code — that defeats the "machine readable" promise.The TypeSpec docs describe
codeas "The machine readable code of the error", but right now every error (meter-not-found, data parse failure, missing customer, etc.) collapses to the same constant string. Consumers won't be able to branch on it programmatically, which is the whole point of having a separatecodefield alongsidemessage.Looking at
validateEventsinopenmeter/meterevent/adapter/event.go(lines 227–272), the different error sources are already distinguishable at the origin — it'd be great to carry that distinction through rather than flatten it here. A couple of options:
- Define typed sentinel errors (e.g.,
ErrNoMeterMatch,ErrNoCustomer,ErrMeterParse) inmetereventand useerrors.Is/errors.Ashere to pick a stable code.- Or introduce a small
ValidationErrorstruct (withCode,Message, optionalAttributes) onmeterevent.Eventinstead of[]error, so the producer owns the code and this converter is a trivial mapping.The second option also lines up nicely with the
attributesfield that's in the spec but currently unset.♻️ Sketch of the sentinel-based approach
- return api.MeteringIngestedEventValidationError{ - Code: "validation_error", - Message: err.Error(), - }, true + code := "validation_error" + switch { + case errors.Is(err, meterevent.ErrNoMeterMatch): + code = "no_meter_match" + case errors.Is(err, meterevent.ErrNoCustomer): + code = "no_customer" + case errors.As(err, new(*meter.ParseError)): + code = "event_data_parse_error" + } + return api.MeteringIngestedEventValidationError{ + Code: code, + Message: err.Error(), + }, true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/events/convert.go` around lines 57 - 66, The current mapping in convert.go collapses all validation errors to the literal "validation_error"; update the mapping in the conversion (the lo.FilterMap over errs that returns api.MeteringIngestedEventValidationError) to inspect each error using errors.Is / errors.As against sentinel errors or typed errors from the meterevent package (e.g., meterevent.ErrNoMeterMatch, meterevent.ErrNoCustomer, meterevent.ErrMeterParse or a meterevent.ValidationError type) and assign a stable machine-readable Code per error kind (e.g., "meter_not_found", "no_customer", "meter_parse_error"), preserve the original message in Message, and populate Attributes on api.MeteringIngestedEventValidationError where available so callers can programmatically branch on Code and use Attributes for extra context; adjust imports to include errors and meterevent as needed.
🤖 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/event.tsp`:
- Around line 127-130: Update the docstring for the optional field
validation_errors on the IngestedEvent type so it refers to the singular event;
change the text from "The validation errors of the ingested events." to a
singular form such as "The validation errors of the ingested event." Ensure you
update the comment above validation_errors?: IngestedEventValidationError[] to
reflect the singular phrasing.
- Around line 152-157: The validation error objects created by
toAPIMeteringIngestedEventValidationErrors are not populating the attributes
field declared as attributes?: Record<unknown>; update
toAPIMeteringIngestedEventValidationErrors in api/v3/handlers/events/convert.go
to set attributes using the existing helper
ToAPIValidationAttributes(issue.Attributes()) (same pattern as the addons
handler) when mapping each issue so the event validation errors include the
correct attributes payload.
---
Nitpick comments:
In `@api/v3/handlers/events/convert.go`:
- Around line 52-73: In toAPIMeteringIngestedEventValidationErrors remove the
redundant early guard that checks len(errs) == 0 (lo.FilterMap already handles
empty slices) and return a pointer to result using &result instead of
lo.ToPtr(result); keep the lo.FilterMap logic and the final nil check for empty
result unchanged.
- Around line 57-66: The current mapping in convert.go collapses all validation
errors to the literal "validation_error"; update the mapping in the conversion
(the lo.FilterMap over errs that returns
api.MeteringIngestedEventValidationError) to inspect each error using errors.Is
/ errors.As against sentinel errors or typed errors from the meterevent package
(e.g., meterevent.ErrNoMeterMatch, meterevent.ErrNoCustomer,
meterevent.ErrMeterParse or a meterevent.ValidationError type) and assign a
stable machine-readable Code per error kind (e.g., "meter_not_found",
"no_customer", "meter_parse_error"), preserve the original message in Message,
and populate Attributes on api.MeteringIngestedEventValidationError where
available so callers can programmatically branch on Code and use Attributes for
extra context; adjust imports to include errors and meterevent as needed.
🪄 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: 4c22d090-a47f-40f4-a1c2-4a7948ed0bd8
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (3)
api/spec/packages/aip/src/events/event.tspapi/v3/api.gen.goapi/v3/handlers/events/convert.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
api/v3/handlers/events/convert.go (2)
60-81: Every validation error collapses to the sameCode: "validation_error". 🏷️Since the backend
meterevent.Event.ValidationErrorsis just[]error(peropenmeter/meterevent/service.go), there's no richer info to map today — that's fine, and matches what the schema allows. Just flagging two follow-ups worth tracking:
- The
attributesfield declared in the TypeSpec model will always be omitted here. That's consistent with the current backend shape (plainerror), so this isn't a bug — but it means clients will see a perfectly uniformcodefor two semantically different failures (unknown meter vs. missing customer, seeopenmeter/meterevent/adapter/event.go:226-270). Consider promoting those to a small typed error (with a code + optional attributes) at the source so this converter can propagate something meaningful.- Minor:
lo.FilterMapalready returns an empty (non-nil) slice when no elements match, so the secondlen(result) == 0guard is a safety net. Not harmful, just redundant — you could drop it or keep it, your call.Non-blocking for this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/events/convert.go` around lines 60 - 81, toAPIMeteringIngestedEventValidationErrors currently collapses all backend errors (meterevent.Event.ValidationErrors) to a single Code "validation_error"; to fix, introduce small typed validation errors in the meterevent package (e.g., ErrUnknownMeter, ErrMissingCustomer or a ValidationError interface that exposes Code() string and Attributes() map[string]string) and return those from openmeter/meterevent/adapter/event.go so this converter can inspect error types and map them to distinct api.MeteringIngestedEventValidationError.Code and Attributes; then update toAPIMeteringIngestedEventValidationErrors to type-assert each error from errs, use the provided Code()/Attributes() when present (fall back to "validation_error"), and optionally remove the redundant second empty-slice check (len(result) == 0).
154-160: The suffix detection viastrings.Fields(*sort)couples fragile re-parsing toParseSortBy's implementation.You're checking
len(strings.Fields(*sort)) == 1to infer "no asc/desc suffix provided," which means you're re-doing part ofParseSortBy's parsing logic. If that function ever changes how it splits input (different separators, whitespace handling, etc.), this check can silently drift out of sync and break the default-to-desc behavior.A cleaner approach: have
ParseSortByexpose whether the order was explicitly set (e.g., a bool flag, or a nullable order type), then branch on that. If that's not feasible right now, at least add a comment tying this logic toParseSortBy's contract so the coupling is explicit for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/events/convert.go` around lines 154 - 160, Replace the fragile re-parsing of *sort with an explicit signal from ParseSortBy: modify ParseSortBy to return (parsed, bool orderExplicit) or make parsed.Order nullable/optional, then in convert.go use that flag (or nil-check on parsed.Order) instead of len(strings.Fields(*sort)) to decide whether to override with sortx.OrderDesc; if you can't change ParseSortBy now, add a clear comment above the current check referencing ParseSortBy's contract and why strings.Fields(*sort) must match its splitting behavior, and mark a TODO to refactor to an explicit flag (referencing ParseSortBy, parsed.Order, ToSortxOrder, and sortx.OrderDesc).api/spec/packages/aip/src/events/event.tsp (1)
126-158: Schema looks good — clean model split betweenIngestedEventandIngestedEventValidationError. 🎯The singular doc phrasing on line 128 is already sorted, and marking all fields of
IngestedEventValidationErrorasLifecycle.Readmatches the fact these are only ever returned on reads. Nice.One small thought for the future: the
codefield is declared as a machine-readable code, but it's currently just a free-formstringand the Go converter sets it to the literal"validation_error"for every error. If you ever want clients to branch on specific validation failures (e.g.,no_meter_found,no_customer_found), consider either promotingcodeto an enum or at least documenting the expected values + adding an@example. Non-blocking for this PR.🤖 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 126 - 158, The IngestedEventValidationError.model uses a free-form string for the machine-readable code (field code) which prevents clients from branching on specific errors; update the model IngestedEventValidationError to either (a) replace code: string with a typed enum of expected codes (e.g., no_meter_found, no_customer_found, validation_error) or (b) keep string but add an `@example` and a short doc list of expected values in the model comment, and then update the Go converter logic that currently sets "validation_error" to instead emit the specific enum/value names (or documented examples) so clients can reliably switch on code.api/v3/handlers/events/list.go (1)
128-220: Consider reintroducing a small helper for these filter error responses. 🧹The summary mentions
filterErrorwas removed in favor of inlineapierrors.NewBadRequestErrorcalls, but the result is the same ~7 lines duplicated seven times, differing only in theFieldname. That's quite a bit of ceremony for what is conceptually "filter X failed to parse → 400".A tiny local helper keeps this readable and keeps field/source conventions in one place:
♻️ Example refactor
+func newFilterBadRequestError(ctx context.Context, field string, err error) error { + return apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ + { + Field: field, + Reason: err.Error(), + Source: apierrors.InvalidParamSourceQuery, + }, + }) +} + func applyFilters(ctx context.Context, req *ListMeteringEventsRequest, f *api.ListEventsParamsFilter) error { id, err := filters.FromAPIFilterString(f.Id) if err != nil { - return apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ - { - Field: "filter[id]", - Reason: err.Error(), - Source: apierrors.InvalidParamSourceQuery, - }, - }) + return newFilterBadRequestError(ctx, "filter[id]", err) } req.ID = id // ...same treatment for source/subject/type/time/ingested_at/stored_at }As per coding guidelines: "In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them."
🤖 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 128 - 220, The applyFilters function repeats the same apierrors.NewBadRequestError construction for each filter parse failure; add a small local helper (e.g., filterError or newFilterBadRequest) inside applyFilters that accepts (ctx context.Context, err error, name string) and returns apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{{Field: fmt.Sprintf("filter[%s]", name), Reason: err.Error(), Source: apierrors.InvalidParamSourceQuery}}), then replace each duplicated error block (for Id, Source, Subject, Type, Time, IngestedAt, StoredAt) to call that helper (keep the existing calls to filters.FromAPIFilterString and filters.FromAPIFilterDateTime and fromAPICustomerIDFilter unchanged).
🤖 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/spec/packages/aip/src/events/event.tsp`:
- Around line 126-158: The IngestedEventValidationError.model uses a free-form
string for the machine-readable code (field code) which prevents clients from
branching on specific errors; update the model IngestedEventValidationError to
either (a) replace code: string with a typed enum of expected codes (e.g.,
no_meter_found, no_customer_found, validation_error) or (b) keep string but add
an `@example` and a short doc list of expected values in the model comment, and
then update the Go converter logic that currently sets "validation_error" to
instead emit the specific enum/value names (or documented examples) so clients
can reliably switch on code.
In `@api/v3/handlers/events/convert.go`:
- Around line 60-81: toAPIMeteringIngestedEventValidationErrors currently
collapses all backend errors (meterevent.Event.ValidationErrors) to a single
Code "validation_error"; to fix, introduce small typed validation errors in the
meterevent package (e.g., ErrUnknownMeter, ErrMissingCustomer or a
ValidationError interface that exposes Code() string and Attributes()
map[string]string) and return those from openmeter/meterevent/adapter/event.go
so this converter can inspect error types and map them to distinct
api.MeteringIngestedEventValidationError.Code and Attributes; then update
toAPIMeteringIngestedEventValidationErrors to type-assert each error from errs,
use the provided Code()/Attributes() when present (fall back to
"validation_error"), and optionally remove the redundant second empty-slice
check (len(result) == 0).
- Around line 154-160: Replace the fragile re-parsing of *sort with an explicit
signal from ParseSortBy: modify ParseSortBy to return (parsed, bool
orderExplicit) or make parsed.Order nullable/optional, then in convert.go use
that flag (or nil-check on parsed.Order) instead of len(strings.Fields(*sort))
to decide whether to override with sortx.OrderDesc; if you can't change
ParseSortBy now, add a clear comment above the current check referencing
ParseSortBy's contract and why strings.Fields(*sort) must match its splitting
behavior, and mark a TODO to refactor to an explicit flag (referencing
ParseSortBy, parsed.Order, ToSortxOrder, and sortx.OrderDesc).
In `@api/v3/handlers/events/list.go`:
- Around line 128-220: The applyFilters function repeats the same
apierrors.NewBadRequestError construction for each filter parse failure; add a
small local helper (e.g., filterError or newFilterBadRequest) inside
applyFilters that accepts (ctx context.Context, err error, name string) and
returns apierrors.NewBadRequestError(ctx, err,
apierrors.InvalidParameters{{Field: fmt.Sprintf("filter[%s]", name), Reason:
err.Error(), Source: apierrors.InvalidParamSourceQuery}}), then replace each
duplicated error block (for Id, Source, Subject, Type, Time, IngestedAt,
StoredAt) to call that helper (keep the existing calls to
filters.FromAPIFilterString and filters.FromAPIFilterDateTime and
fromAPICustomerIDFilter unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0761a894-8691-44be-bc57-fe5fc3e311ec
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (5)
api/spec/packages/aip/src/events/event.tspapi/v3/api.gen.goapi/v3/handlers/events/convert.goapi/v3/handlers/events/list.goapi/v3/handlers/events/list_test.go
✅ Files skipped from review due to trivial changes (2)
- api/v3/handlers/events/list_test.go
- api/v3/api.gen.go
borosr
approved these changes
Apr 24, 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
2 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
Bug Fixes / Behavior Changes