feat(plans): add key, name, status, currency filters and sort option#4221
Merged
feat(plans): add key, name, status, currency filters and sort option#4221
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:
📝 WalkthroughWalkthroughAdds client-controlled sorting and deepObject string filters for listPlans across spec, generated bindings, HTTP handler, adapter, and service layers; introduces Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Handler as API Handler
participant Service as Plan Service
participant Adapter as Data Adapter
participant DB as Database
Client->>Handler: GET /openmeter/plans?sort=...&filter[key]=...
Handler->>Handler: Parse sort & deepObject filter params
Handler->>Handler: Validate formats (field-scoped errors on failure)
Handler->>Service: ListPlans(ListPlansInput{Sort, Filter...})
Service->>Service: Validate filters & ordering
Service->>Adapter: buildQuery(filters, ordering)
Adapter->>Adapter: Apply filter predicates (Key, Name, Currency, Status)
Adapter->>Adapter: Apply order by clause
Adapter->>DB: Execute query
DB-->>Adapter: Results
Adapter-->>Service: Plans
Service-->>Handler: Plans
Handler-->>Client: 200 OK (sorted & filtered results)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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: 4
🧹 Nitpick comments (3)
openmeter/productcatalog/plan/service/service_test.go (1)
826-1057: Great filter coverage — consider rounding out withsortandstatuscases too.The matrix here is really nice: four plans with intentionally overlapping substrings (
erappearing in bothalpha-starterandgamma-enterprise👌), and the combined AND cases give good confidence that filters compose correctly.That said, the PR title mentions key, name, status, currency filters and sort — and this test covers key/name/currency filtering, but not:
sort(especially theOrderByKeybug fix on the adapter side — a regression test would lock that in so we don't silently slip back to version-ordering again).filter[status]— theparams.Statusbranch inListPlanshas non-trivial effective_from/effective_to logic across Active / Draft / Scheduled / Archived / Invalid.Since all four fixture plans are drafts here, a small follow-up publishing one or two of them and then exercising status + sort would nicely close the loop.
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/productcatalog/plan/service/service_test.go` around lines 826 - 1057, Add test cases to TestListPlansFilters that exercise sorting and status filtering: extend the fixture by publishing one or two of the draft plans (use the same env.Plan.* helpers that created plans, e.g. call the publish method on the created plan keys) then add tcs entries that set plan.ListPlansInput.Sort to OrderByKey (and reverse) to assert stable key ordering and a regression for the adapter's OrderByKey behavior, and add tcs that set the params.Status (the status field on plan.ListPlansInput / params.Status branch used in ListPlans) to Active/Draft/Scheduled/Archived to verify effective_from/effective_to semantics; run each case through env.Plan.ListPlans and assert expected key lists.api/v3/handlers/plans/list.go (1)
95-104:sortfield isn't validated at the handler, only downstream.Works fine in practice because
ListPlansInput.Validate()rejects unknownOrderByvalues — just flagging that the 400 returned for a bogussortfield will surface as a generic validation error rather than asort-scopedInvalidParameterlike the filter errors above. If you want consistent error shape for clients, validateplan.OrderBy(sort.Field)here and wrap it as asort-fieldInvalidParametersentry. Totally optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/plans/list.go` around lines 95 - 104, The handler currently parses params.Sort via request.ParseSortBy but doesn't validate that the resulting plan.OrderBy value is a known enum, deferring that to ListPlansInput.Validate; update the sort handling in the handler (the block using params.Sort, request.ParseSortBy, and setting req.OrderBy) to validate plan.OrderBy(sort.Field) immediately and, on invalid value, return apierrors.NewBadRequestError with apierrors.InvalidParameters containing a single entry with Field: "sort", Reason set to the validation error message, and Source: apierrors.InvalidParamSourceQuery so clients get a sort-scoped InvalidParameter instead of a generic downstream validation error. Ensure req.OrderBy only gets set after this validation succeeds.openmeter/productcatalog/plan/service.go (1)
38-54: Tiny nit:Values()allocates on every call.Not a hot path, but
OrderBy.Values()rebuilds the slice each timeValidate()runs. A package-levelvar orderByValues = []OrderBy{...}(or amap[OrderBy]struct{}for O(1) lookup) would be a touch cleaner, andValues()can just return it. Totally optional.♻️ Tiny refactor sketch
-func (f OrderBy) Values() []OrderBy { - return []OrderBy{ - OrderByID, - OrderByKey, - OrderByVersion, - OrderByCreatedAt, - OrderByUpdatedAt, - } -} +var orderByValues = []OrderBy{ + OrderByID, + OrderByKey, + OrderByVersion, + OrderByCreatedAt, + OrderByUpdatedAt, +} + +func (f OrderBy) Values() []OrderBy { return orderByValues }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/plan/service.go` around lines 38 - 54, The Values() method currently constructs a new slice every call; create a package-level variable (e.g., orderByValues := []OrderBy{OrderByID, OrderByKey, OrderByVersion, OrderByCreatedAt, OrderByUpdatedAt}) or a lookup map (e.g., orderByLookup := map[OrderBy]struct{}{}) and change OrderBy.Values() to return that variable (or remove Values and reference the lookup directly), then update OrderBy.Validate() to check membership against the package-level slice with slices.Contains(orderByValues, f) or against the map with a simple presence check to avoid repeated allocations.
🤖 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/productcatalog/operations.tsp`:
- Around line 46-55: The JSDoc for the sort query is missing the `id` attribute;
update the comment block above the `sort?: Common.SortQuery` declaration to
include `id` in the supported sort attributes so the spec matches
OrderBy.Values() in openmeter/productcatalog/plan/service.go (or alternatively
remove `id` from OrderBy.Values() if you prefer implementation-first parity) —
locate the comment near the `@query(#{ name: "sort" }) sort?: Common.SortQuery`
declaration and add `- 'id'` to the list.
- Around line 27-28: The status field is currently typed as
Common.StringFieldFilter but only exact equality operators are supported in the
handler (see the plans list handler logic that checks eq/oeq), so change the
type to the closed/strict filter to avoid implying unsupported operators:
replace status?: Common.StringFieldFilter with status?:
Common.StringFieldFilterExact (or the equivalent exact-only type used elsewhere,
e.g., charges), and update any validation or docs to only accept eq/oeq so the
type matches the actual handler behavior in the list handler.
In `@api/v3/api.gen.go`:
- Around line 5303-5310: The comment for the Sort field in the API spec repeats
the `key` sort attribute; update the source API spec (the comment/description
for the plans endpoint that documents supported sort attributes) to remove the
duplicate `key` entry, then regenerate the generated file so the docblock above
the Sort field (Sort *SortQuery `form:"sort,omitempty" json:"sort,omitempty"`)
no longer lists `key` twice.
In `@openmeter/productcatalog/plan/service.go`:
- Around line 101-108: Update the field comments for Key, Name, and Currency to
accurately reflect all operators that FilterString supports or explicitly state
which subset the plan adapter honors: mention
gt/gte/lt/lte/contains/ocontains/eq/neq/oeq/$exists (or list the exact supported
subset), and ensure the wording matches how the plan adapter and list.go status
filter handle these operators so callers aren't misled about silently dropped
ops; locate the Key/Name/Currency fields and adjust their docstrings
accordingly.
---
Nitpick comments:
In `@api/v3/handlers/plans/list.go`:
- Around line 95-104: The handler currently parses params.Sort via
request.ParseSortBy but doesn't validate that the resulting plan.OrderBy value
is a known enum, deferring that to ListPlansInput.Validate; update the sort
handling in the handler (the block using params.Sort, request.ParseSortBy, and
setting req.OrderBy) to validate plan.OrderBy(sort.Field) immediately and, on
invalid value, return apierrors.NewBadRequestError with
apierrors.InvalidParameters containing a single entry with Field: "sort", Reason
set to the validation error message, and Source:
apierrors.InvalidParamSourceQuery so clients get a sort-scoped InvalidParameter
instead of a generic downstream validation error. Ensure req.OrderBy only gets
set after this validation succeeds.
In `@openmeter/productcatalog/plan/service.go`:
- Around line 38-54: The Values() method currently constructs a new slice every
call; create a package-level variable (e.g., orderByValues :=
[]OrderBy{OrderByID, OrderByKey, OrderByVersion, OrderByCreatedAt,
OrderByUpdatedAt}) or a lookup map (e.g., orderByLookup :=
map[OrderBy]struct{}{}) and change OrderBy.Values() to return that variable (or
remove Values and reference the lookup directly), then update OrderBy.Validate()
to check membership against the package-level slice with
slices.Contains(orderByValues, f) or against the map with a simple presence
check to avoid repeated allocations.
In `@openmeter/productcatalog/plan/service/service_test.go`:
- Around line 826-1057: Add test cases to TestListPlansFilters that exercise
sorting and status filtering: extend the fixture by publishing one or two of the
draft plans (use the same env.Plan.* helpers that created plans, e.g. call the
publish method on the created plan keys) then add tcs entries that set
plan.ListPlansInput.Sort to OrderByKey (and reverse) to assert stable key
ordering and a regression for the adapter's OrderByKey behavior, and add tcs
that set the params.Status (the status field on plan.ListPlansInput /
params.Status branch used in ListPlans) to Active/Draft/Scheduled/Archived to
verify effective_from/effective_to semantics; run each case through
env.Plan.ListPlans and assert expected key lists.
🪄 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: 1dc96f57-04c4-4b28-9456-f62711b18a15
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (7)
.agents/skills/api-filters/SKILL.mdapi/spec/packages/aip/src/productcatalog/operations.tspapi/v3/api.gen.goapi/v3/handlers/plans/list.goopenmeter/productcatalog/plan/adapter/plan.goopenmeter/productcatalog/plan/service.goopenmeter/productcatalog/plan/service/service_test.go
borosr
added
the
release-note/ignore
borosr
force-pushed
the
feat/productcatalog-plans-filter
branch
from
3379e81 to
a168896
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
openmeter/productcatalog/plan/service/service_test.go (1)
829-830: Two tiny nits on an otherwise great table test.
- Line 829:
context.WithCancel(t.Context())is effectively a no-op wrapper —t.Context()is already cancelled on test cleanup. You can just usectx := t.Context()and drop thedefer cancel().- Line 958: the case name advertises "case-insensitive" but every fixture uses the exact casing
Plan, so that property isn't actually asserted. Either rename (e.g."name contains Plan (3 of 4 names)") or add a case likeContains: lo.ToPtr("plan")to genuinely exercise case-insensitivity.As per coding guidelines,
*_test.go: "prefert.Context()when atesting.Tortesting.TBis available instead ofcontext.Background()".Also applies to: 958-965
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/plan/service/service_test.go` around lines 829 - 830, Replace the unnecessary context.WithCancel wrapper in the test setup by using ctx := t.Context() and remove the defer cancel() (the current context is already cancelled on test cleanup); then update the table test case whose name advertises "case-insensitive" so it actually asserts that behavior—either rename the case to accurately describe the fixtures (e.g., "name contains Plan (3 of 4 names)") or change the case's filter to use a lowercased Contains value (e.g., set Contains: lo.ToPtr("plan")) so the test for case-insensitivity in the table entry is exercised; look for t.Context(), context.WithCancel(...) and the table case named "case-insensitive" to locate the spots to edit.openmeter/productcatalog/plan/service.go (1)
96-108: Heads-up onCurrencyvsCurrenciescoexistence.
Currency *filter.FilterStringnow sits right next to the existingCurrencies []string, and both get applied independently in the adapter (OR group + ANDApplyToQuery). It's correct, but the naming is easy to trip on — internal callers reaching for "filter by currency" may pick the wrong one and get subtly different semantics. Consider a short doc note clarifying thatCurrencyis the new request-driven filter andCurrenciesis the legacy OR-group input, or (follow-up) collapsing the two down the line.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/plan/service.go` around lines 96 - 108, Add a short clarifying comment above the Currencies and Currency fields to distinguish their semantics: state that Currencies []string is the legacy OR-group filter (matches any of the listed currencies) while Currency *filter.FilterString is the newer request-driven filter with full operators (eq/neq/contains/oeq) and is applied separately via ApplyToQuery; mention which one takes precedence or that both are combined (AND) in the adapter so callers know to pick the right field (reference the Currencies and Currency struct fields and the ApplyToQuery usage in the adapter/service).
🤖 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/plans/convert.go`:
- Around line 781-805: Tighten operator validation in fromAPIStatusFilter by
whitelisting only Eq and Oeq: if any of the other operator fields on
api.StringFieldFilter (Neq, Contains, Ocontains, Exists, Gt, Gte, Lt, Lte, etc.)
are set return apierrors.NewBadRequestError with an InvalidParameters entry for
"filter[status]" and a message that only eq and oeq are supported; additionally
validate each status string (both *f.Eq and each entry in f.Oeq) before casting
to productcatalog.PlanStatus by checking it exists in the set returned by
productcatalog.PlanStatus.Values() and return a 400 InvalidParameters error for
unknown values so invalid statuses like "bogus" are rejected instead of
propagated.
---
Nitpick comments:
In `@openmeter/productcatalog/plan/service.go`:
- Around line 96-108: Add a short clarifying comment above the Currencies and
Currency fields to distinguish their semantics: state that Currencies []string
is the legacy OR-group filter (matches any of the listed currencies) while
Currency *filter.FilterString is the newer request-driven filter with full
operators (eq/neq/contains/oeq) and is applied separately via ApplyToQuery;
mention which one takes precedence or that both are combined (AND) in the
adapter so callers know to pick the right field (reference the Currencies and
Currency struct fields and the ApplyToQuery usage in the adapter/service).
In `@openmeter/productcatalog/plan/service/service_test.go`:
- Around line 829-830: Replace the unnecessary context.WithCancel wrapper in the
test setup by using ctx := t.Context() and remove the defer cancel() (the
current context is already cancelled on test cleanup); then update the table
test case whose name advertises "case-insensitive" so it actually asserts that
behavior—either rename the case to accurately describe the fixtures (e.g., "name
contains Plan (3 of 4 names)") or change the case's filter to use a lowercased
Contains value (e.g., set Contains: lo.ToPtr("plan")) so the test for
case-insensitivity in the table entry is exercised; look for t.Context(),
context.WithCancel(...) and the table case named "case-insensitive" to locate
the spots to edit.
🪄 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: 31c53dbd-1401-4524-8867-ae6a1ee32cd0
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (8)
.agents/skills/api-filters/SKILL.mdapi/spec/packages/aip/src/productcatalog/operations.tspapi/v3/api.gen.goapi/v3/handlers/plans/convert.goapi/v3/handlers/plans/list.goopenmeter/productcatalog/plan/adapter/plan.goopenmeter/productcatalog/plan/service.goopenmeter/productcatalog/plan/service/service_test.go
✅ Files skipped from review due to trivial changes (1)
- .agents/skills/api-filters/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (3)
- api/spec/packages/aip/src/productcatalog/operations.tsp
- api/v3/api.gen.go
- api/v3/handlers/plans/list.go
tothandras
previously approved these changes
Apr 24, 2026
borosr
force-pushed
the
feat/productcatalog-plans-filter
branch
from
f47fa13 to
acbdb1c
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
api/v3/handlers/plans/convert.go (1)
781-805:⚠️ Potential issue | 🟠 MajorStatus values still aren't validated — bogus values silently return an empty page.
Narrowing the parameter to
*api.StringFieldFilterExactnicely tightened the operator surface (nice cleanup, the whitelist part of the earlier concern is basically gone). But the value-level check from that earlier comment is still open:productcatalog.PlanStatus(*f.Eq)/PlanStatus(v)happily accept any string. Downstream inadapter/plan.gonone of theslices.Contains(params.Status, …)branches match, sopredicatesstays empty andplandb.Or()ends up excluding all rows — a caller sendingfilter[status][eq]=bogusgets a silent empty result instead of a 400.Worth validating against
productcatalog.PlanStatus("").Values()and returning aBadRequestErrorfor unknown values so the failure is visible.🛠️ Suggested nudge
+ valid := productcatalog.PlanStatus("").Values() + appendStatus := func(s string, out []productcatalog.PlanStatus) ([]productcatalog.PlanStatus, error) { + ps := productcatalog.PlanStatus(s) + if !slices.Contains(valid, ps) { + err := fmt.Errorf("unknown plan status %q", s) + return nil, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{ + {Field: "filter[status]", Reason: err.Error(), Source: apierrors.InvalidParamSourceQuery}, + }) + } + return append(out, ps), nil + } + var statuses []productcatalog.PlanStatus if f.Eq != nil { - statuses = append(statuses, productcatalog.PlanStatus(*f.Eq)) + var err error + if statuses, err = appendStatus(*f.Eq, statuses); err != nil { + return nil, err + } } for _, v := range f.Oeq { - statuses = append(statuses, productcatalog.PlanStatus(v)) + var err error + if statuses, err = appendStatus(v, statuses); err != nil { + return nil, err + } }Requires adding
slicesto imports (and confirmingPlanStatus.Values()returns[]PlanStatus).#!/bin/bash # Confirm PlanStatus.Values() exists and its return type, plus check the adapter behavior on empty predicates. ast-grep --pattern 'func ($_ PlanStatus) Values() $_ { $$$ }' rg -nP '\bPlanStatus\b' --type=go -g '!**/*_test.go' -C2 | rg -n 'Values\(\)' -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/plans/convert.go` around lines 781 - 805, The fromAPIStatusFilter function currently casts arbitrary strings into productcatalog.PlanStatus which lets invalid status values silently yield an empty result; change it to validate each input against the canonical set returned by productcatalog.PlanStatus("").Values() (or the method on PlanStatus) using slices.Contains (add slices to imports) and if any value is unknown return apierrors.NewBadRequestError with an InvalidParameters entry for "filter[status]" describing the invalid value; only append validated PlanStatus values to statuses and proceed as before.openmeter/productcatalog/plan/service.go (1)
101-108:⚠️ Potential issue | 🟡 MinorDocstrings still undersell what these filters accept.
Flagged before but still standing:
*filter.FilterStringcarries more thaneq/neq/contains/oeq(there'sgt/gte/lt/lte/in/existsinpkg/filter/filter.go). Since those operators are honored by the adapter viafilter.ApplyToQuery, the current comments will mislead readers into thinking the others are dropped. Either list the full set or state "as supported bypkg/filter.FilterString" and let the type be the source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/plan/service.go` around lines 101 - 108, The docstrings for the Key, Name, and Currency fields under the plan filter underspecify supported operators; update their comments to either enumerate all operators from pkg/filter.FilterString (eq/neq/contains/oeq/gt/gte/lt/lte/in/exists) or replace the current partial list with a short phrase like "operators supported by pkg/filter.FilterString" to accurately reflect the behavior used by filter.ApplyToQuery and avoid misleading readers.
🧹 Nitpick comments (3)
openmeter/productcatalog/plan/service/service_test.go (1)
826-1057: Solid coverage for the new filter surface ✨Good mix of per-field and combined-AND cases, and
ElementsMatchkeeps things order-agnostic — exactly the right call. A couple of totally optional thoughts:
context.WithCancel(t.Context())at line 829 is a little belt-and-suspenders sincet.Context()is already cancelled at test cleanup. You can drop the wrapper (and thedefer cancel()) unless you need early cancellation mid-test.- Would be a small win to also cover
Statusand the newsortpath here, given they're part of the same PR — happy to help scaffold those if useful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/plan/service/service_test.go` around lines 826 - 1057, In TestListPlansFilters remove the redundant context.WithCancel wrapper: stop creating the cancelable ctx and defer cancel(); use t.Context() directly when calling env.Plan.ListPlans and other APIs (replace ctx with t.Context()), and delete the variables ctx and cancel and their defer to avoid double-cancellation; this change targets the TestListPlansFilters function where ctx, cancel are declared and used.api/v3/handlers/plans/list.go (1)
58-99: Filter/sort plumbing reads nicely 👌The per-field 400 mapping with the right
Field:label is exactly what clients need forinvalid_parameters. Two tiny, optional thoughts:
- The four
filter[...]blocks are structurally identical — if this grows, a small helper likeparseFilterTo(ctx, params.Filter.Key, "filter[key]", &req.Key, filters.FromAPIFilterString)would tighten things up. Totally a nice-to-have.- Unknown
sortfield values currently fall through toplan.OrderBy(sort.Field)and only get rejected inside the service'sOrderBy.Validate(). Still yields a 400 via the generic validation encoder, but theInvalidParameterswon't be scoped to"sort"the way the filter errors are. If you want symmetry, a quickif err := req.OrderBy.Validate(); err != nilright here lets you return the same shape as the filter branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/plans/list.go` around lines 58 - 99, The sort handling should validate the parsed OrderBy immediately so invalid sort fields return a 400 with an InvalidParameters entry scoped to "sort"; after calling request.ParseSortBy and setting req.OrderBy = plan.OrderBy(sort.Field) do an if err := req.OrderBy.Validate(); err != nil { return ListPlansRequest{}, apierrors.NewBadRequestError(ctx, err, apierrors.InvalidParameters{{Field: "sort", Reason: err.Error(), Source: apierrors.InvalidParamSourceQuery}}) } to mirror the filter branches. Optionally, reduce repetition in the filter block by extracting a small helper (e.g., parseFilterTo(ctx, params.Filter.Key, "filter[key]", &req.Key, filters.FromAPIFilterString)) that accepts the API value, the parameter label, a pointer to the target field on ListPlansRequest, and the appropriate parser function like filters.FromAPIFilterString or filters.FromAPIFilterStringExact.openmeter/productcatalog/plan/service.go (1)
38-54: Tiny stylistic thought onOrderBy.Values().Totally optional — the receiver
fis unused, so this could be a package-levelvar OrderByValues = []OrderBy{...}(or a top-levelfunc OrderByValues()). That said, the method form matches thePlanStatus.Values()pattern elsewhere in the codebase, so keeping it consistent is a perfectly reasonable call. Feel free to leave as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/plan/service.go` around lines 38 - 54, The receiver on OrderBy.Values() is unused; refactor to a package-level slice (e.g., var OrderByValues = []OrderBy{OrderByID, OrderByKey, OrderByVersion, OrderByCreatedAt, OrderByUpdatedAt}) or a top-level function OrderByValues() and update OrderBy.Validate() to call that new symbol instead of f.Values(); alternatively keep the method to match PlanStatus.Values() if you prefer consistency—ensure OrderBy.Validate() uses the chosen top-level symbol (OrderByValues or OrderByValues()) and remove the unused receiver from Values() if you convert it to a package-level construct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@api/v3/handlers/plans/convert.go`:
- Around line 781-805: The fromAPIStatusFilter function currently casts
arbitrary strings into productcatalog.PlanStatus which lets invalid status
values silently yield an empty result; change it to validate each input against
the canonical set returned by productcatalog.PlanStatus("").Values() (or the
method on PlanStatus) using slices.Contains (add slices to imports) and if any
value is unknown return apierrors.NewBadRequestError with an InvalidParameters
entry for "filter[status]" describing the invalid value; only append validated
PlanStatus values to statuses and proceed as before.
In `@openmeter/productcatalog/plan/service.go`:
- Around line 101-108: The docstrings for the Key, Name, and Currency fields
under the plan filter underspecify supported operators; update their comments to
either enumerate all operators from pkg/filter.FilterString
(eq/neq/contains/oeq/gt/gte/lt/lte/in/exists) or replace the current partial
list with a short phrase like "operators supported by pkg/filter.FilterString"
to accurately reflect the behavior used by filter.ApplyToQuery and avoid
misleading readers.
---
Nitpick comments:
In `@api/v3/handlers/plans/list.go`:
- Around line 58-99: The sort handling should validate the parsed OrderBy
immediately so invalid sort fields return a 400 with an InvalidParameters entry
scoped to "sort"; after calling request.ParseSortBy and setting req.OrderBy =
plan.OrderBy(sort.Field) do an if err := req.OrderBy.Validate(); err != nil {
return ListPlansRequest{}, apierrors.NewBadRequestError(ctx, err,
apierrors.InvalidParameters{{Field: "sort", Reason: err.Error(), Source:
apierrors.InvalidParamSourceQuery}}) } to mirror the filter branches.
Optionally, reduce repetition in the filter block by extracting a small helper
(e.g., parseFilterTo(ctx, params.Filter.Key, "filter[key]", &req.Key,
filters.FromAPIFilterString)) that accepts the API value, the parameter label, a
pointer to the target field on ListPlansRequest, and the appropriate parser
function like filters.FromAPIFilterString or filters.FromAPIFilterStringExact.
In `@openmeter/productcatalog/plan/service.go`:
- Around line 38-54: The receiver on OrderBy.Values() is unused; refactor to a
package-level slice (e.g., var OrderByValues = []OrderBy{OrderByID, OrderByKey,
OrderByVersion, OrderByCreatedAt, OrderByUpdatedAt}) or a top-level function
OrderByValues() and update OrderBy.Validate() to call that new symbol instead of
f.Values(); alternatively keep the method to match PlanStatus.Values() if you
prefer consistency—ensure OrderBy.Validate() uses the chosen top-level symbol
(OrderByValues or OrderByValues()) and remove the unused receiver from Values()
if you convert it to a package-level construct.
In `@openmeter/productcatalog/plan/service/service_test.go`:
- Around line 826-1057: In TestListPlansFilters remove the redundant
context.WithCancel wrapper: stop creating the cancelable ctx and defer cancel();
use t.Context() directly when calling env.Plan.ListPlans and other APIs (replace
ctx with t.Context()), and delete the variables ctx and cancel and their defer
to avoid double-cancellation; this change targets the TestListPlansFilters
function where ctx, cancel are declared and used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25743039-1af1-432b-a89e-3bd530e418be
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (8)
.agents/skills/api-filters/SKILL.mdapi/spec/packages/aip/src/productcatalog/operations.tspapi/v3/api.gen.goapi/v3/handlers/plans/convert.goapi/v3/handlers/plans/list.goopenmeter/productcatalog/plan/adapter/plan.goopenmeter/productcatalog/plan/service.goopenmeter/productcatalog/plan/service/service_test.go
✅ Files skipped from review due to trivial changes (1)
- api/v3/api.gen.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/spec/packages/aip/src/productcatalog/operations.tsp
tothandras
previously approved these changes
Apr 24, 2026
…to the list plan endpoint
borosr
force-pushed
the
feat/productcatalog-plans-filter
branch
from
6c30d32 to
fe2c876
Compare
tothandras
approved these changes
Apr 24, 2026
This was referenced Apr 28, 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/plansendpoint with afilter[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/plans?sort=key%20asc'curl --request GET \ --url 'http://localhost:8888/api/v3/openmeter/plans?filter%5Bname%5D%5Bcontains%5D=Hello&filter%5Bkey%5D%5Beq%5D=meterid1'The response should look like this:
{ "data": [ { "billing_cadence": "P1M", "created_at": "2026-04-24T09:30:43.186863+02:00", "currency": "USD", "description": "Entry-level USD plan for smoke testing", "id": "01KPZ6BTBJP4TAE4PH29D9KDW7", "key": "smokestarter", "name": "Starter Plan", "phases": [ { "key": "default", "name": "Default Phase", "rate_cards": [] } ], "pro_rating_enabled": true, "status": "draft", "updated_at": "2026-04-24T09:30:43.186865+02:00", "validation_errors": [ { "code": "plan_phase_has_no_rate_cards", "field": "$.phases[?(@.key=='default')].rateCards", "message": "plan phase must have at least one rate card" } ], "version": 1 }, { "billing_cadence": "P1M", "created_at": "2026-04-24T09:32:04.998109+02:00", "currency": "EUR", "description": "Entry-level USD plan for smoke testing", "id": "01KPZ6EA860P45GT8R1P7BFCAF", "key": "smokestarter2", "name": "Starter Plan 2", "phases": [ { "key": "default", "name": "Default Phase", "rate_cards": [] } ], "pro_rating_enabled": true, "status": "draft", "updated_at": "2026-04-24T09:32:04.998111+02:00", "validation_errors": [ { "code": "plan_phase_has_no_rate_cards", "field": "$.phases[?(@.key=='default')].rateCards", "message": "plan phase must have at least one rate card" } ], "version": 1 }, { "billing_cadence": "P1M", "created_at": "2026-04-24T09:32:25.521558+02:00", "currency": "HUF", "description": "Entry-level USD plan for smoke testing", "id": "01KPZ6EY9H7SBDDH4JY4QRX7EZ", "key": "smokestarter3", "name": "Starter Plan 3", "phases": [ { "key": "default", "name": "Default Phase", "rate_cards": [] } ], "pro_rating_enabled": true, "status": "draft", "updated_at": "2026-04-24T09:32:25.52156+02:00", "validation_errors": [ { "code": "plan_phase_has_no_rate_cards", "field": "$.phases[?(@.key=='default')].rateCards", "message": "plan phase must have at least one rate card" } ], "version": 1 } ], "meta": { "page": { "size": 20, "number": 1, "total": 3 } } }Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests