feat(api): add aip filters to meters api#4170
Merged
tothandras merged 4 commits intomainfrom Apr 21, 2026
Merged
Conversation
mark-vass-konghq
added
the
release-note/feature
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:
📝 WalkthroughWalkthroughAdds a deep-object Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as Request Handler
participant Service as Service Layer
participant Adapter as Database Adapter
participant DB as Database
Client->>Handler: GET /meters?filter[key]=...&filter[name]=...
Handler->>Handler: Parse query (filters.Parse / FromAPIFilterString)
alt parse OK
Handler->>Service: ListMeters(ctx, request with Key/Name filters)
Service->>Adapter: ListMeters(ctx, params with filters)
Adapter->>Adapter: filter.ApplyToQuery for Key and Name
Adapter->>DB: Execute filtered query
DB-->>Adapter: Return rows
Adapter-->>Service: Return meters
Service-->>Handler: Return results
Handler-->>Client: Respond with items (paginated)
else parse error
Handler-->>Client: 400 Bad Request (InvalidParameters: filter)
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested Reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
🧹 Nitpick comments (3)
api/v3/handlers/meters/list.go (1)
57-73: Small DRY opportunity.The key/name parsing blocks are near-identical. If you expect more filter fields to land here soon, a tiny local helper would keep this clean; otherwise this is perfectly fine as-is.
♻️ Optional helper
parse := func(field string, in *api.FilterString) (*filter.FilterString, error) { out, err := filters.FromAPIFilterString(in) if err != nil { return nil, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ {Field: field, Reason: err.Error(), Source: apierrors.InvalidParamSourceQuery}, }) } return out, nil }Also worth a quick sanity check: based on the current
FromAPIFilterStringimplementation, the error return is never populated — so theBadRequestbranches here are defensive-only today. Not a bug, just a heads up in case you expected parse errors to surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/meters/list.go` around lines 57 - 73, The duplicate parsing logic for params.Filter.Key and params.Filter.Name should be consolidated to reduce repetition: create a small local helper (e.g., parseFilter or similar) that calls filters.FromAPIFilterString, converts any error into apierrors.NewBadRequestError using the appropriate field name, and returns the parsed *filter.FilterString; then replace the two blocks that set req.Key and req.Name with calls to this helper, referencing params.Filter, filters.FromAPIFilterString, apierrors.NewBadRequestError, req.Key and req.Name to locate the code to change.api/spec/packages/aip/src/meters/operations.tsp (1)
17-31: LGTM — clean filter model.Nicely scoped
ListMetersParamsFilterwith justkeyandnameusing the sharedCommon.StringFieldFilter. One small thought: the doc only mentionsfilter[key]; worth mentioningfilter[name]too so users don't have to guess the second field exists.📝 Optional doc tweak
- * To filter meters by key add the following query param: filter[key]=my-meter-key + * Examples: + * filter[key]=my-meter-key + * filter[name]=My%20Meter🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/packages/aip/src/meters/operations.tsp` around lines 17 - 31, Update the documentation/comments for the ListMetersParamsFilter model to explicitly mention both filter[key] and filter[name] as available query filters (not just filter[key]); locate the model definition (ListMetersParamsFilter) and its fields key and name and add/adjust the docstring so users see both filter[name] and filter[key] usage examples or descriptions.openmeter/meter/service.go (1)
88-128: Consider validating the new filter fields.
FilterStringexposesValidate()/ValidateWithComplexity()to catch multi-operator and depth violations (seepkg/filter/filter.go). SinceKey/Namecome straight from user input, it'd be nice to run them through that inListMetersParams.Validate()so bad payloads are rejected at the service boundary rather than producing weird predicates downstream.🛡️ Suggested addition
if p.EventTypes != nil { ... } + + if p.Key != nil { + if err := p.Key.Validate(); err != nil { + errs = append(errs, fmt.Errorf("invalid key filter: %w", err)) + } + } + + if p.Name != nil { + if err := p.Name.Validate(); err != nil { + errs = append(errs, fmt.Errorf("invalid name filter: %w", err)) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/meter/service.go` around lines 88 - 128, ListMetersParams.Validate() should run the user-supplied FilterString fields through the filter package validators to reject complex/malformed filters early: import the filter package (pkg/filter) and for each FilterString-typed field coming from the request (e.g. Key, Name or any other fields of type filter.FilterString on ListMetersParams) call either Validate() or ValidateWithComplexity(...) and append any returned error(s) to errs before returning errors.Join(errs...); ensure you handle pointer/slice variants (validate each element if e.g. []*filter.FilterString or []filter.FilterString) and preserve existing checks (IDFilter, SlugFilter, EventTypes) so malformed filter expressions are rejected at the service boundary.
🤖 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/meters/operations.tsp`:
- Around line 17-31: Update the documentation/comments for the
ListMetersParamsFilter model to explicitly mention both filter[key] and
filter[name] as available query filters (not just filter[key]); locate the model
definition (ListMetersParamsFilter) and its fields key and name and add/adjust
the docstring so users see both filter[name] and filter[key] usage examples or
descriptions.
In `@api/v3/handlers/meters/list.go`:
- Around line 57-73: The duplicate parsing logic for params.Filter.Key and
params.Filter.Name should be consolidated to reduce repetition: create a small
local helper (e.g., parseFilter or similar) that calls
filters.FromAPIFilterString, converts any error into
apierrors.NewBadRequestError using the appropriate field name, and returns the
parsed *filter.FilterString; then replace the two blocks that set req.Key and
req.Name with calls to this helper, referencing params.Filter,
filters.FromAPIFilterString, apierrors.NewBadRequestError, req.Key and req.Name
to locate the code to change.
In `@openmeter/meter/service.go`:
- Around line 88-128: ListMetersParams.Validate() should run the user-supplied
FilterString fields through the filter package validators to reject
complex/malformed filters early: import the filter package (pkg/filter) and for
each FilterString-typed field coming from the request (e.g. Key, Name or any
other fields of type filter.FilterString on ListMetersParams) call either
Validate() or ValidateWithComplexity(...) and append any returned error(s) to
errs before returning errors.Join(errs...); ensure you handle pointer/slice
variants (validate each element if e.g. []*filter.FilterString or
[]filter.FilterString) and preserve existing checks (IDFilter, SlugFilter,
EventTypes) so malformed filter expressions are rejected at the service
boundary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c15807c-8a3b-4040-a57c-c481a1245ea2
⛔ 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/meters/operations.tspapi/v3/api.gen.goapi/v3/handlers/meters/list.goopenmeter/meter/adapter/meter.goopenmeter/meter/service.go
mark-vass-konghq
force-pushed
the
feat/add-aip-filters-to-meters-filter
branch
from
8bb5da8 to
c32c155
Compare
mark-vass-konghq
force-pushed
the
feat/add-aip-filters-to-meters-filter
branch
from
c32c155 to
50aec55
Compare
mark-vass-konghq
force-pushed
the
feat/add-aip-filters-to-meters-filter
branch
from
50aec55 to
e91fe7e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/meters/list.go`:
- Around line 57-61: filters.Parse is populating meter.ListMetersParams (req)
with domain pkg/filter.FilterString fields that the parser doesn't recognize;
instead parse into an intermediate API struct using api/v3/filters.FilterString
and then convert to domain types before assigning to req. Change the code around
filters.Parse to: create a local intermediate struct (e.g. apiReq) with api
filter types for Key and Name, call filters.Parse(r.URL.Query(), &apiReq), then
set req.Key = filters.FromAPIFilterString(apiReq.Key) and req.Name =
filters.FromAPIFilterString(apiReq.Name) (preserving other fields), and return
the converted req; reference filters.Parse, meter.ListMetersParams,
filters.FromAPIFilterString, and the req.Key/req.Name fields when making the
change.
In `@openmeter/meter/service.go`:
- Around line 103-113: The Key and Name filter validation currently uses
FilterString.Validate() with unlimited depth; add a service-level cap before the
DB query by enforcing a max depth/node limit (e.g., MAX_FILTER_DEPTH,
MAX_FILTER_NODES) and use either a new bounded validation API on FilterString
(e.g., ValidateWithLimits(maxDepth,maxNodes)) or perform a lightweight
traversal/count on p.Key and p.Name to reject overly complex filters; update the
checks around p.Key and p.Name (the places calling FilterString.Validate()) to
call the bounded validator and append an error like "filter too complex" when
limits are exceeded.
🪄 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: 0654af6d-a5ef-4b1f-a1ff-1d3689436aed
⛔ 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/meters/operations.tspapi/v3/api.gen.goapi/v3/handlers/meters/list.goopenmeter/meter/adapter/meter.goopenmeter/meter/service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- openmeter/meter/adapter/meter.go
- api/spec/packages/aip/src/meters/operations.tsp
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openmeter/meter/service.go (1)
16-18:⚠️ Potential issue | 🟠 MajorAdd a breadth/node cap too, not just a depth cap.
ValidateWithComplexitycaps nesting depth, but a shallowor/andwith thousands of children can still pass and create a huge DB predicate. Please pair this with a max-node or max-children limit before the adapter builds the query. As per coding guidelines, “Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks.”Also applies to: 107-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/meter/service.go` around lines 16 - 18, The depth-only guard (const maxListMeterFilterDepth and ValidateWithComplexity for ListMetersParams) allows shallow AND/OR nodes with huge numbers of children; add a breadth/node cap (e.g., introduce maxListMeterFilterChildren or maxListMeterFilterNodes) and enforce it in the same validation path that ValidateWithComplexity runs so the ListMetersParams is rejected if any And/Or node exceeds the child limit; perform this check before the adapter builds the DB query (the code path that translates ListMetersParams to predicates) to prevent constructing massive DB predicates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openmeter/meter/service.go`:
- Around line 16-18: The depth-only guard (const maxListMeterFilterDepth and
ValidateWithComplexity for ListMetersParams) allows shallow AND/OR nodes with
huge numbers of children; add a breadth/node cap (e.g., introduce
maxListMeterFilterChildren or maxListMeterFilterNodes) and enforce it in the
same validation path that ValidateWithComplexity runs so the ListMetersParams is
rejected if any And/Or node exceeds the child limit; perform this check before
the adapter builds the DB query (the code path that translates ListMetersParams
to predicates) to prevent constructing massive DB predicates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 150a1e7b-2131-4d16-ba1e-9256f8c5ba0b
📒 Files selected for processing (2)
api/v3/handlers/meters/list.goopenmeter/meter/service.go
tothandras
requested changes
Apr 20, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/meter/mockadapter/meter.go`:
- Around line 33-35: The loop currently only checks params.Key.In and skips
other filter operators and the Name field; add handling for params.Name and
evaluate all supported filter.FilterString operators (e.g., Eq, In, Contains)
for both Key and Name so mock behavior matches the real adapter. Implement a
small shared helper (e.g., matchFilterString(filter *filter.FilterString, value
string) bool) and call it for params.Key and params.Name instead of directly
using slices.Contains; use meter.Key and meter.Name as the values to test and
ensure the helper covers the different operator cases used by the real adapter.
🪄 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: 68058d42-427f-40eb-af05-d04ef7ff8c5d
📒 Files selected for processing (6)
api/v3/handlers/meters/list.goapp/common/meter.goopenmeter/meter/adapter/meter.goopenmeter/meter/mockadapter/meter.goopenmeter/meter/service.goopenmeter/portal/httphandler/portal.go
🚧 Files skipped from review as they are similar to previous changes (2)
- openmeter/meter/adapter/meter.go
- openmeter/meter/service.go
mark-vass-konghq
force-pushed
the
feat/add-aip-filters-to-meters-filter
branch
from
9acc52e to
a36f6c0
Compare
mark-vass-konghq
force-pushed
the
feat/add-aip-filters-to-meters-filter
branch
from
a36f6c0 to
6fece34
Compare
mark-vass-konghq
force-pushed
the
feat/add-aip-filters-to-meters-filter
branch
from
6fece34 to
4402bc6
Compare
mark-vass-konghq
force-pushed
the
feat/add-aip-filters-to-meters-filter
branch
from
4402bc6 to
b304119
Compare
mark-vass-konghq
force-pushed
the
feat/add-aip-filters-to-meters-filter
branch
from
b304119 to
93f200a
Compare
borosr
approved these changes
Apr 21, 2026
tothandras
approved these changes
Apr 21, 2026
This was referenced 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
3 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.
@coderabbitai
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Documentation