feat(features): add key and name filters and sort to the features list operation#4204
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds optional Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as Handler (api/v3/handlers/features/list.go)
participant Validator as Validator (connector.go)
participant Adapter as Adapter (adapter/feature.go)
participant DB as Database
Client->>Handler: ListFeatures(key?, name?, sort?, meter_id?)
Handler->>Handler: Parse key & name filters
Handler->>Handler: Parse sort parameter
Handler->>Validator: Validate params (filters + order_by)
Validator-->>Handler: Validation result
alt validation fails
Handler-->>Client: 400 Bad Request (InvalidParameters)
else
Handler->>Adapter: ListFeatures(validated params)
Adapter->>Adapter: Apply Key filter
Adapter->>Adapter: Apply Name filter
Adapter->>Adapter: Apply MeterID filter
Adapter->>DB: Execute filtered & sorted query
DB-->>Adapter: Results
Adapter-->>Handler: Features list
Handler-->>Client: 200 OK with results
end
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 |
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/features/operations.tsp (1)
50-69:⚠️ Potential issue | 🟡 MinorSort attribute docs don't match the implementation.
Heads up — the doc comment lists supported sort attributes as
id,name(default),created_at, but the backendFeatureOrderByinopenmeter/productcatalog/feature/connector.goonly acceptskey,name,created_at,updated_at. So:
sort=id ascwill pass through the API layer and then fail validation in the connector ("invalid feature order by: id"), which is a confusing user experience.keyandupdated_atare actually supported but undocumented.name (default)is misleading — the handler doesn't apply a default order whensortis omitted; no ordering is set on the query at all.Also, while you're refreshing this filter block, consider mentioning the new
filter[key]andfilter[name]alongside the existingfilter[meter_id]example so the spec is self-describing.📝 Suggested doc tweak
/** * Sort features returned in the response. * Supported sort attributes are: - * - `id` - * - `name` (default) + * - `key` + * - `name` * - `created_at` + * - `updated_at` * * The `asc` suffix is optional as the default sort order is ascending. * The `desc` suffix is used to specify a descending order. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/packages/aip/src/features/operations.tsp` around lines 50 - 69, The docs for the features list are out of sync with the backend: update the sort and filter comments so they match the connector's accepted values and behavior—replace the listed sort attributes (`id`, `name`, `created_at`) with the actual supported FeatureOrderBy values (`key`, `name`, `created_at`, `updated_at`), remove the misleading claim that `name` is the default (the handler does not apply a default order), and note that invalid values (e.g., `id`) will be rejected by FeatureOrderBy in openmeter/productcatalog/feature/connector.go; also expand the filter examples to document filter[key] and filter[name] alongside the existing filter[meter_id] usage and ensure the `@query` types (sort?: Common.SortQuery, filter?: ListFeaturesParamsFilter) remain unchanged.
🧹 Nitpick comments (2)
openmeter/productcatalog/adapter/feature_test.go (1)
253-316: Solid coverage — consider exercising a non-Eqoperator too.The new sub-cases cleanly verify the happy path and empty result for both filters. As a nice-to-have, could you add one case using
Contains(e.g.,filter[name][contains]=Helloas per the PR description) so the end-to-end filter-operator parsing is exercised in tests, not just equality? Totally optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/adapter/feature_test.go` around lines 253 - 316, Add a small subcase that exercises the non-Eq operator by calling feature.ListFeatures with feature.ListFeaturesParams where Name (or Key) is set to &filter.FilterString{Contains: lo.ToPtr("feature")} (or "Hello" per PR description), then assert the returned Items include the created testFeature (len>0 and matching Name/Key) and that an unrelated contains value returns zero items; update the existing test blocks in openmeter/productcatalog/adapter/feature_test.go (the test using testFeature, connector.CreateFeature and connector.ListFeatures) to include this Contains check so operator parsing is covered end-to-end.api/v3/handlers/features/list.go (1)
84-94: Parsing is clean — consider validating the sort field here for a nicer 400.Tiny suggestion: if a user passes e.g.
sort=id asc,request.ParseSortBysucceeds, the handler storesfeature.FeatureOrderBy("id"), and the failure only surfaces deeper viaListFeaturesParams.Validate()as a generic validation error. If you validate up front, you can return the same structuredapierrors.InvalidParameters{Field: "sort", ...}shape you already use for the other params, which is easier for API consumers to reason about.💡 Optional tweak
req.OrderBy = feature.FeatureOrderBy(sort.Field) + if err := req.OrderBy.Validate(); err != nil { + return ListFeaturesRequest{}, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ + {Field: "sort", Reason: err.Error(), Source: apierrors.InvalidParamSourceQuery}, + }) + } req.Order = sort.Order.ToSortxOrder()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/features/list.go` around lines 84 - 94, After parsing sort via request.ParseSortBy, validate the parsed sort.Field before assigning to req.OrderBy: check that the value maps to a known feature.FeatureOrderBy (e.g., via an existing enum/validator or a switch/lookup of allowed fields) and if it is invalid return the same structured bad-request (apierrors.NewBadRequestError with apierrors.InvalidParameters{Field: "sort", Reason: err.Error(), Source: apierrors.InvalidParamSourceQuery}) instead of deferring to ListFeaturesParams.Validate(); update the block around params.Sort handling (where request.ParseSortBy is called and req.OrderBy = feature.FeatureOrderBy(...)) to perform this early validation and produce the consistent 400 response.
🤖 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/api.gen.go`:
- Around line 5102-5110: The generated doc for the Sort field is missing "key"
as a supported sort attribute; update the source API spec (OpenAPI/Swagger or
the generator input that defines the features list/sort description) to include
`key` in the supported sort attributes (and any enum or examples that drive
SortQuery), then re-run the code generator to regenerate api.gen.go so the Sort
*SortQuery comment and any related validation/enums include `key` (target
symbols: the Sort field on the features request model and the SortQuery
definition).
---
Outside diff comments:
In `@api/spec/packages/aip/src/features/operations.tsp`:
- Around line 50-69: The docs for the features list are out of sync with the
backend: update the sort and filter comments so they match the connector's
accepted values and behavior—replace the listed sort attributes (`id`, `name`,
`created_at`) with the actual supported FeatureOrderBy values (`key`, `name`,
`created_at`, `updated_at`), remove the misleading claim that `name` is the
default (the handler does not apply a default order), and note that invalid
values (e.g., `id`) will be rejected by FeatureOrderBy in
openmeter/productcatalog/feature/connector.go; also expand the filter examples
to document filter[key] and filter[name] alongside the existing filter[meter_id]
usage and ensure the `@query` types (sort?: Common.SortQuery, filter?:
ListFeaturesParamsFilter) remain unchanged.
---
Nitpick comments:
In `@api/v3/handlers/features/list.go`:
- Around line 84-94: After parsing sort via request.ParseSortBy, validate the
parsed sort.Field before assigning to req.OrderBy: check that the value maps to
a known feature.FeatureOrderBy (e.g., via an existing enum/validator or a
switch/lookup of allowed fields) and if it is invalid return the same structured
bad-request (apierrors.NewBadRequestError with
apierrors.InvalidParameters{Field: "sort", Reason: err.Error(), Source:
apierrors.InvalidParamSourceQuery}) instead of deferring to
ListFeaturesParams.Validate(); update the block around params.Sort handling
(where request.ParseSortBy is called and req.OrderBy =
feature.FeatureOrderBy(...)) to perform this early validation and produce the
consistent 400 response.
In `@openmeter/productcatalog/adapter/feature_test.go`:
- Around line 253-316: Add a small subcase that exercises the non-Eq operator by
calling feature.ListFeatures with feature.ListFeaturesParams where Name (or Key)
is set to &filter.FilterString{Contains: lo.ToPtr("feature")} (or "Hello" per PR
description), then assert the returned Items include the created testFeature
(len>0 and matching Name/Key) and that an unrelated contains value returns zero
items; update the existing test blocks in
openmeter/productcatalog/adapter/feature_test.go (the test using testFeature,
connector.CreateFeature and connector.ListFeatures) to include this Contains
check so operator parsing is covered end-to-end.
🪄 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: 46fc0fed-af53-4efe-bf19-218b5046cfa3
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (6)
api/spec/packages/aip/src/features/operations.tspapi/v3/api.gen.goapi/v3/handlers/features/list.goopenmeter/productcatalog/adapter/feature.goopenmeter/productcatalog/adapter/feature_test.goopenmeter/productcatalog/feature/connector.go
tothandras
requested changes
Apr 22, 2026
Comment on lines
+21
to
+34
| /** | ||
| * Filter features by meter ID. | ||
| */ | ||
| meter_id?: Common.ULIDFieldFilter; | ||
|
|
||
| /** | ||
| * Filter features by key. | ||
| */ | ||
| key?: Common.StringFieldFilter; | ||
|
|
||
| /** | ||
| * Filter features by name. | ||
| */ | ||
| name?: Common.StringFieldFilter; |
Contributor
There was a problem hiding this comment.
We should add back these, not to end up with allOf
Suggested change
| /** | |
| * Filter features by meter ID. | |
| */ | |
| meter_id?: Common.ULIDFieldFilter; | |
| /** | |
| * Filter features by key. | |
| */ | |
| key?: Common.StringFieldFilter; | |
| /** | |
| * Filter features by name. | |
| */ | |
| name?: Common.StringFieldFilter; | |
| #suppress "@openmeter/api-spec-aip/doc-decorator" "shared model" | |
| meter_id?: Common.ULIDFieldFilter; | |
| #suppress "@openmeter/api-spec-aip/doc-decorator" "shared model" | |
| key?: Common.StringFieldFilter; | |
| #suppress "@openmeter/api-spec-aip/doc-decorator" "shared model" | |
| name?: Common.StringFieldFilter; |
| } | ||
| req.MeterIDs = meterIDs | ||
|
|
||
| key, err := filters.FromAPIFilterString(params.Filter.Key) |
Contributor
There was a problem hiding this comment.
params.Filter.Key nil check is missing
Contributor
Author
There was a problem hiding this comment.
hey, the filters.FromAPIFilterString is doing an early exit if the params.Filter.Key is nil
| }) | ||
| } | ||
|
|
||
| req.OrderBy = feature.FeatureOrderBy(sort.Field) |
Contributor
There was a problem hiding this comment.
Should we validate the supported order by's here? I know we'll return NewGenericValidationError from the service, just wondering.
Contributor
Author
There was a problem hiding this comment.
I'm validating this field inside the ListFeaturesParams.Validate() and I didn't see the reason to validate it in both layers.
1cd38c8 to
10412a3
Compare
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 `@api/spec/packages/aip/src/features/operations.tsp`:
- Around line 23-26: The operation docs currently list only the meter_id filter
while the filter model now exposes key?: Common.StringFieldFilter and name?:
Common.StringFieldFilter; update the operation documentation text that
references the filter model (the sections showing meter_id) to include both key
and name alongside meter_id, and mirror this change in the other occurrence
noted (around the second block at lines ~56-60); ensure the docs describe that
key and name accept the same StringFieldFilter options as meter_id so generated
API docs accurately reflect the implemented filters.
🪄 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: db9f3451-44d5-4ca7-b1b2-060db7c75e61
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (2)
api/spec/packages/aip/src/features/operations.tspapi/v3/api.gen.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/v3/api.gen.go
tothandras
approved these changes
Apr 22, 2026
This was referenced May 1, 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.
Overview
Extend the
GET /api/v3/openmeter/featuresendpoint with afilter[name]and `filter[key] query params, also implement sort query param.Notes for review
Testing guide:
curl --request GET \ --url 'http://localhost:8888/api/v3/openmeter/features?sort=key%20asc'curl --request GET \ --url 'http://localhost:8888/api/v3/openmeter/features?filter%5Bname%5D%5Bcontains%5D=Hello&filter%5Bkey%5D%5Beq%5D=meterid1'The response should look like this:
{ "data": [ { "created_at": "2026-04-22T09:07:37.105283Z", "description": "Hello feature 1", "id": "01KPT73T0H9A4TZA3TXV1ASYDB", "key": "meterid1", "labels": {}, "meter": { "id": "01KPQW85CS1ZV4XW1BTAZF6GG0" }, "name": "Hello feature 1", "updated_at": "2026-04-22T09:07:37.105283Z" }, { "created_at": "2026-04-22T09:07:29.850728Z", "description": "Hello feature 2", "id": "01KPT73JXT05XS0M0DRHQ26933", "key": "meterid2", "labels": {}, "meter": { "id": "01KPQW85CS1ZV4XW1BTAZF6GG0" }, "name": "Hello feature 2", "updated_at": "2026-04-22T09:07:29.850729Z" }, { "created_at": "2026-04-21T11:26:15.615382Z", "description": "Hello feature 3", "id": "01KPQWMYHZKQE6QKWRQ9PAT7M7", "key": "meterid3", "labels": {}, "meter": { "id": "01KPQW85CS1ZV4XW1BTAZF6GG0" }, "name": "Hello feature 3", "updated_at": "2026-04-21T11:26:15.615383Z" } ], "meta": { "page": { "size": 20, "number": 1, "total": 3 } } }Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests