feat(addons): list filters id, name, key, status, currency#4243
Merged
Conversation
borosr
added
the
release-note/ignore
Contributor
📝 WalkthroughWalkthroughAdds deepObject-style filtering and sorting to the add-on listing API: new API filter model and generated types, handler parsing/binding for Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler
participant Service
participant Adapter
participant Database
Client->>Handler: GET /addons?filter[name]=foo&sort=name:desc
Handler->>Handler: Parse `filter` and `sort` params
Handler->>Service: ListAddons(ctx, ListAddonsRequest{ Name: FilterString{Contains:"foo"}, OrderBy: name desc })
Service->>Adapter: List(ctx, input)
Adapter->>Adapter: Apply filters via filter.ApplyToQuery()
Adapter->>Adapter: Build predicates (name LIKE, status IN, etc.) and ORDER BY
Adapter->>Database: Execute SELECT ... WHERE ... ORDER BY ...
Database-->>Adapter: Rows
Adapter-->>Service: []Addon
Service-->>Handler: []Addon
Handler-->>Client: 200 OK JSON
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 |
borosr
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: 3
🧹 Nitpick comments (1)
openmeter/productcatalog/addon/service/service_test.go (1)
31-32: Uset.Context()in both test setups.The same root-context pattern appears in the main suite and the new list suite; switching to
t.Context()keeps cancellation bound to the test harness. As per coding guidelines, in tests prefert.Context()when atesting.Tortesting.TBis available instead of usingcontext.Background().Also applies to: 449-450
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/addon/service/service_test.go` around lines 31 - 32, Replace manual root contexts created with context.WithCancel(context.Background()) in the tests with the testing harness context t.Context(); specifically, in the test setup where you do ctx, cancel := context.WithCancel(context.Background()) / defer cancel(), change to ctx := t.Context() so cancellation is bound to the test harness (apply the same change to the other occurrence around lines 449-450).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/api-filters/SKILL.md:
- Around line 253-258: The documentation heading incorrectly states "three
additional layers" while the bulleted list contains four items; update the
heading or the list so the count matches—either change the heading to "four
additional layers" or remove/merge one list item; ensure references in the text
to Parser tests (api/v3/filters/parse_test.go), Converter tests
(api/v3/filters/convert_test.go and FromAPIFilter* helpers), Adapter tests
(adapter_test.go and any filter.ApplyToQuery additions), and Handler/adapter
integration tests remain accurate after the edit.
In `@openmeter/productcatalog/addon/adapter/adapter_test.go`:
- Around line 219-230: The subtest "ByCurrencyFilter" isn't verifying the filter
because only one addon exists; create a control addon with a different currency
(e.g., insert a second addon before calling env.AddonRepository.ListAddons) or
explicitly assert the exact result set instead of NotEmpty: call
env.AddonRepository.ListAddons with ListAddonsInput (Namespaces, Currency:
&filter.FilterString{Eq: ¤cyStr}) and then require that listAddonV1.Items
has length 1 and that the returned item's ID (or Currency) matches addonV1Input
(or expected currency), ensuring the filter actually limits results; use the
existing names addonV1Input, namespace, and ListAddons to locate where to add
the setup and assertions.
In `@openmeter/productcatalog/addon/httpdriver/addon.go`:
- Around line 61-69: The code in the request builder doesn't forward the new
name filter so filter[name] is dropped; in the same block where params.Id,
params.Key, and params.Currency are mapped to req.ID, req.Key, and req.Currency,
add the equivalent mapping for params.Name to req.Name using
&filter.FilterString{In: params.Name} (mirror the pattern used for Key/Currency)
so the name filter is propagated to the service/adapter layers.
---
Nitpick comments:
In `@openmeter/productcatalog/addon/service/service_test.go`:
- Around line 31-32: Replace manual root contexts created with
context.WithCancel(context.Background()) in the tests with the testing harness
context t.Context(); specifically, in the test setup where you do ctx, cancel :=
context.WithCancel(context.Background()) / defer cancel(), change to ctx :=
t.Context() so cancellation is bound to the test harness (apply the same change
to the other occurrence around lines 449-450).
🪄 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: c444dd5c-71d2-42a4-aba2-8bc61b203c9a
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (12)
.agents/skills/api-filters/SKILL.mdapi/spec/packages/aip/src/productcatalog/operations.tspapi/v3/api.gen.goapi/v3/filters/convert.goapi/v3/handlers/addons/list.goopenmeter/productcatalog/addon.goopenmeter/productcatalog/addon/adapter/adapter_test.goopenmeter/productcatalog/addon/adapter/addon.goopenmeter/productcatalog/addon/httpdriver/addon.goopenmeter/productcatalog/addon/service.goopenmeter/productcatalog/addon/service/addon.goopenmeter/productcatalog/addon/service/service_test.go
Comment on lines
+253
to
+258
| Write tests at three additional layers: | ||
|
|
||
| 1. **Parser tests** (`api/v3/filters/parse_test.go`): the parse layer is already covered for the generic operator surface; only add cases when introducing a new filter type or operator. | ||
| 2. **Converter tests** (`api/v3/filters/convert_test.go`): only when adding a new `FromAPIFilter*` helper. | ||
| 3. **Handler/adapter integration tests:** the important layer for new endpoints — assert that representative `?filter[...]=` query strings produce the expected results. Cover at minimum: | ||
| 3. **Adapter tests** (`adapter_test.go`): If you made changes to the adapter (e.g., adding `filter.ApplyToQuery` for new fields) that are not already covered by existing tests, add specific test cases to verify the Ent query generation for these fields. | ||
| 4. **Handler/adapter integration tests:** the important layer for new endpoints — assert that representative `?filter[...]=` query strings produce the expected results. Cover at minimum: |
Contributor
There was a problem hiding this comment.
Keep the layer count consistent.
The heading says three additional layers, but the list now has four items. Tiny doc mismatch, but it will trip up anyone following the workflow.
🧰 Tools
🪛 LanguageTool
[style] ~257-~257: Consider shortening or rephrasing this to strengthen your wording.
Context: ...ter tests** (adapter_test.go): If you made changes to the adapter (e.g., adding `filter.Apply...
(MAKE_CHANGES)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/api-filters/SKILL.md around lines 253 - 258, The
documentation heading incorrectly states "three additional layers" while the
bulleted list contains four items; update the heading or the list so the count
matches—either change the heading to "four additional layers" or remove/merge
one list item; ensure references in the text to Parser tests
(api/v3/filters/parse_test.go), Converter tests (api/v3/filters/convert_test.go
and FromAPIFilter* helpers), Adapter tests (adapter_test.go and any
filter.ApplyToQuery additions), and Handler/adapter integration tests remain
accurate after the edit.
tothandras
previously approved these changes
Apr 28, 2026
…er, also fix the linter
tothandras
approved these changes
Apr 28, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/v3/handlers/addons/list.go (1)
56-107: Consider extracting repeated filter/sort parse error mapping.This block works, but there’s a lot of repeated
NewBadRequestErrorconstruction. A tiny helper would make this easier to maintain and less error-prone when adding the next filter.♻️ Small refactor sketch
+ badQueryParam := func(field string, err error) error { + return apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ + {Field: field, Reason: err.Error(), Source: apierrors.InvalidParamSourceQuery}, + }) + } if params.Filter != nil { id, err := filters.FromAPIFilterULID(params.Filter.Id) if err != nil { - return ListAddonsRequest{}, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ - {Field: "filter[id]", Reason: err.Error(), Source: apierrors.InvalidParamSourceQuery}, - }) + return ListAddonsRequest{}, badQueryParam("filter[id]", err) } req.ID = id // ...same pattern for key/name/currency/status... } if params.Sort != nil { sort, err := request.ParseSortBy(*params.Sort) if err != nil { - return ListAddonsRequest{}, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ - {Field: "sort", Reason: err.Error(), Source: apierrors.InvalidParamSourceQuery}, - }) + return ListAddonsRequest{}, badQueryParam("sort", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/addons/list.go` around lines 56 - 107, The parsing block for params.Filter and params.Sort repeats apierrors.NewBadRequestError construction for each parse error (see filters.FromAPIFilterULID, FromAPIFilterString, FromAPIFilterStringExact, FromAPIStatusFilter, request.ParseSortBy used when building ListAddonsRequest from params.Filter/params.Sort); extract a small helper (e.g., mapParseErr(ctx, err, field string) error) that wraps the incoming error into apierrors.NewBadRequestError with the appropriate apierrors.InvalidParameters entry (using the provided field name like "filter[id]", "filter[key]", "sort", etc.), then call that helper at each parse failure to replace the repeated NewBadRequestError blocks so the parsing code only returns mapParseErr(ctx, err, "<field>") on error.
🤖 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/productcatalog/addon/service.go`:
- Around line 100-131: ListAddonsInput.Validate() currently skips validating the
Status field; update the method to validate Status the same way as
ID/Key/Name/Currency by checking i.Status (or if it's a slice, iterate its
entries) and calling each entry's Validate(), appending any returned errors to
errs; keep the final return using
models.NewNillableGenericValidationError(errors.Join(errs...)) so Status
validation errors are included in the aggregated result.
---
Nitpick comments:
In `@api/v3/handlers/addons/list.go`:
- Around line 56-107: The parsing block for params.Filter and params.Sort
repeats apierrors.NewBadRequestError construction for each parse error (see
filters.FromAPIFilterULID, FromAPIFilterString, FromAPIFilterStringExact,
FromAPIStatusFilter, request.ParseSortBy used when building ListAddonsRequest
from params.Filter/params.Sort); extract a small helper (e.g., mapParseErr(ctx,
err, field string) error) that wraps the incoming error into
apierrors.NewBadRequestError with the appropriate apierrors.InvalidParameters
entry (using the provided field name like "filter[id]", "filter[key]", "sort",
etc.), then call that helper at each parse failure to replace the repeated
NewBadRequestError blocks so the parsing code only returns mapParseErr(ctx, err,
"<field>") on error.
🪄 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: ce7f33da-3248-4b18-8351-66346b8e7eaa
📒 Files selected for processing (4)
api/v3/filters/convert.goapi/v3/handlers/addons/list.goopenmeter/productcatalog/addon.goopenmeter/productcatalog/addon/service.go
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/addonsendpoint with afilter[id],filter[name],filter[key],filter[status]andfilter[currency]query params, also implement sort query param.Notes for review
Testing guide:
curl --request GET \ --url 'http://localhost:8888/api/v3/openmeter/addons?sort=key%20asc'curl --request GET \ --url 'http://localhost:8888/api/v3/openmeter/addons?filter%5Bname%5D%5Bcontains%5D=Hello&filter%5Bkey%5D%5Beq%5D=meterid1'The response should look like this:
{ "data": [ { "created_at": "2026-04-27T11:10:18.647683+02:00", "currency": "USD", "description": "Entry-level USD single-instance addon for smoke testing", "id": "01KQ738ARQ612TBY4WWZX6X34Y", "instance_type": "single", "key": "smokeaddonbasic", "labels": {}, "name": "Basic Addon", "rate_cards": [ { "billing_cadence": "P1M", "key": "basefee", "labels": {}, "name": "Base Fee", "payment_term": "in_arrears", "price": { "amount": "10", "type": "flat" } } ], "status": "draft", "updated_at": "2026-04-27T11:10:18.647691+02:00", "version": 1 }, { "created_at": "2026-04-27T11:10:18.68824+02:00", "currency": "USD", "description": "Professional USD multi-instance addon for smoke testing", "id": "01KQ738AT0A1F4GSY2HKCXDXB6", "instance_type": "multiple", "key": "smokeaddonpro", "labels": {}, "name": "Professional Addon", "rate_cards": [ { "billing_cadence": "P1M", "key": "profee", "labels": {}, "name": "Professional Fee", "payment_term": "in_arrears", "price": { "amount": "50", "type": "flat" } } ], "status": "draft", "updated_at": "2026-04-27T11:10:18.688241+02:00", "version": 1 }, { "created_at": "2026-04-27T11:10:18.696221+02:00", "currency": "EUR", "description": "Enterprise EUR single-instance addon for smoke testing", "id": "01KQ738AT833ANG27QFCH0D2ZV", "instance_type": "single", "key": "smokeaddonenterprise", "labels": {}, "name": "Enterprise Addon", "rate_cards": [ { "billing_cadence": "P1M", "key": "entfee", "labels": {}, "name": "Enterprise Fee", "payment_term": "in_arrears", "price": { "amount": "100", "type": "flat" } } ], "status": "draft", "updated_at": "2026-04-27T11:10:18.696222+02:00", "version": 1 }, { "created_at": "2026-04-27T11:10:18.703948+02:00", "currency": "EUR", "description": "Basic EUR multi-instance addon for smoke testing — will be archived", "id": "01KQ738ATFKJ8K6RVF9WVZZX67", "instance_type": "multiple", "key": "smokeaddonbasiceu", "labels": {}, "name": "Basic European Addon", "rate_cards": [ { "billing_cadence": "P1M", "key": "eufee", "labels": {}, "name": "EU Base Fee", "payment_term": "in_arrears", "price": { "amount": "15", "type": "flat" } } ], "status": "draft", "updated_at": "2026-04-27T11:10:18.703948+02:00", "version": 1 } ], "meta": { "page": { "size": 20, "number": 1, "total": 4 } } }Summary by CodeRabbit
New Features
Documentation
Tests