feat(subscription addon): List endpoint#4337
Merged
Merged
Conversation
borosr
added
the
release-note/ignore
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:
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 (16)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughAdds GET /openmeter/subscriptions/{subscriptionId}/addons: TypeSpec model and operation, generated server glue, handler and conversion, service/repository ordering, and server wiring for paginated, optionally-sorted listing. ChangesSubscription Add‑ons List Endpoint
Sequence DiagramsequenceDiagram
participant Client
participant Router as Server
participant Handler as ListSubscriptionAddonsHandler
participant Service as SubscriptionAddonService
participant Repo as SubscriptionAddonRepository
Client->>Router: GET /openmeter/subscriptions/{subId}/addons?page=1&sort=created_at
Router->>Handler: ListSubscriptionAddons(subId, params)
Handler->>Handler: resolve namespace, validate page, parse sort
Handler->>Service: List(namespace, subscriptionId, pagination, orderBy, order)
Service->>Repo: List(filter with ordering)
Repo->>Repo: Apply ORDER BY (id|created_at|updated_at) and direction
Repo-->>Service: [SubscriptionAddon...]
Handler->>Handler: toAPISubscriptionAddon conversion loop (union periods, set quantity)
Handler-->>Client: PagePaginatedResponse<SubscriptionAddon> (HTTP 200)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
![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: 1
🧹 Nitpick comments (3)
openmeter/subscription/addon/repo/subscriptionaddon.go (1)
90-104: ⚖️ Poor tradeoffAdd indexes on
created_atandupdated_atfor sorting efficiency.The ordering logic looks clean and sensible, but I noticed the schema doesn't define indexes on the
created_atandupdated_atcolumns that the code orders by. Since this is a list endpoint doing database sorts, those unindexed timestamp columns could become a performance bottleneck as the table grows. Quick fix: add them to theIndexes()method in theSubscriptionAddonschema.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/subscription/addon/repo/subscriptionaddon.go` around lines 90 - 104, The SubscriptionAddon listing sorts by created_at and updated_at (see switch on filter.OrderBy in subscriptionaddon repo) but the schema lacks indexes on those timestamp columns; update the SubscriptionAddon ent schema's Indexes() method to add indexes for created_at and updated_at (and consider a composite index if you commonly sort/filter with other fields) so queries using ByCreatedAt and ByUpdatedAt ordering are efficient as the table grows.api/v3/handlers/subscriptions/subscriptionaddons/convert.go (1)
15-15: 💤 Low valueUnused parameter: consider removing or using the subscription view.
The
subscription.SubscriptionViewparameter isn't used in the conversion. If it's intended for future use, that's fine—but if not, removing it would clean things up a bit.♻️ Suggested simplification
-func toAPISubscriptionAddon(_ subscription.SubscriptionView, addon subscriptionaddon.SubscriptionAddon) (apiv3.SubscriptionAddon, error) { +func toAPISubscriptionAddon(addon subscriptionaddon.SubscriptionAddon) (apiv3.SubscriptionAddon, error) {Then update the call site in
list.goline 100 accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v3/handlers/subscriptions/subscriptionaddons/convert.go` at line 15, The function toAPISubscriptionAddon currently accepts an unused subscription.SubscriptionView parameter; remove that unused parameter from the signature so it becomes toAPISubscriptionAddon(addon subscriptionaddon.SubscriptionAddon) (apiv3.SubscriptionAddon, error), then update all callers of toAPISubscriptionAddon to pass only the addon value (or, if the view is intended to be used, integrate it into the conversion logic instead of leaving it unused); ensure you adjust imports/signatures where the function is referenced so compilation succeeds.openmeter/subscription/addon/service.go (1)
62-66: ⚡ Quick winOrder type lacks a validation method, so validation would need custom implementation.
You're right that validating
OrderalongsideOrderBywould be consistent. However,Order(fromsortx) is an enum-like type with predefined constants (OrderAsc,OrderDesc,OrderDefault,OrderNone) and doesn't have aValidate()method likeOrderBydoes. If you'd like to validate it, you'd need to either add aValidate()method to thesortx.Ordertype or add inline validation here. SinceOrderis type-safe through its predefined constants, it's probably a lower priority, but worth considering if other parts of the codebase follow a stricter validation pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/subscription/addon/service.go` around lines 62 - 66, Order (type sortx.Order) isn't validated because it lacks a Validate method; add a validation step alongside the existing OrderBy check in the same block: if i.Order is set (or not equal to a safe default) ensure it matches one of the allowed constants (OrderAsc, OrderDesc, OrderDefault, OrderNone) and append an error to errs when it doesn't; alternatively, implement a Validate() method on sortx.Order that performs this check and call i.Order.Validate() similarly to i.OrderBy.Validate().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v3/handlers/subscriptions/subscriptionaddons/list.go`:
- Around line 93-96: Remove the unnecessary subscription view fetch by deleting
the call to h.subscriptionService.GetView(ctx, req.SubscriptionID) and its error
handling in list.go (the variables view and err are unused), and update the
conversion function signature to match by removing the unused parameter in
toAPISubscriptionAddon (and corresponding change in convert.go) so the list
handler calls toAPISubscriptionAddon without passing a subscription view.
---
Nitpick comments:
In `@api/v3/handlers/subscriptions/subscriptionaddons/convert.go`:
- Line 15: The function toAPISubscriptionAddon currently accepts an unused
subscription.SubscriptionView parameter; remove that unused parameter from the
signature so it becomes toAPISubscriptionAddon(addon
subscriptionaddon.SubscriptionAddon) (apiv3.SubscriptionAddon, error), then
update all callers of toAPISubscriptionAddon to pass only the addon value (or,
if the view is intended to be used, integrate it into the conversion logic
instead of leaving it unused); ensure you adjust imports/signatures where the
function is referenced so compilation succeeds.
In `@openmeter/subscription/addon/repo/subscriptionaddon.go`:
- Around line 90-104: The SubscriptionAddon listing sorts by created_at and
updated_at (see switch on filter.OrderBy in subscriptionaddon repo) but the
schema lacks indexes on those timestamp columns; update the SubscriptionAddon
ent schema's Indexes() method to add indexes for created_at and updated_at (and
consider a composite index if you commonly sort/filter with other fields) so
queries using ByCreatedAt and ByUpdatedAt ordering are efficient as the table
grows.
In `@openmeter/subscription/addon/service.go`:
- Around line 62-66: Order (type sortx.Order) isn't validated because it lacks a
Validate method; add a validation step alongside the existing OrderBy check in
the same block: if i.Order is set (or not equal to a safe default) ensure it
matches one of the allowed constants (OrderAsc, OrderDesc, OrderDefault,
OrderNone) and append an error to errs when it doesn't; alternatively, implement
a Validate() method on sortx.Order that performs this check and call
i.Order.Validate() similarly to i.OrderBy.Validate().
🪄 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: a95d64b5-f7d5-4ccd-a15f-db1d312e0e5f
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (15)
api/spec/packages/aip/src/konnect.tspapi/spec/packages/aip/src/openmeter.tspapi/spec/packages/aip/src/subscriptions/index.tspapi/spec/packages/aip/src/subscriptions/operations.tspapi/spec/packages/aip/src/subscriptions/subscriptionaddon.tspapi/v3/api.gen.goapi/v3/handlers/subscriptions/subscriptionaddons/convert.goapi/v3/handlers/subscriptions/subscriptionaddons/handler.goapi/v3/handlers/subscriptions/subscriptionaddons/list.goapi/v3/server/routes.goapi/v3/server/server.goopenmeter/server/server.goopenmeter/subscription/addon/repo/subscriptionaddon.goopenmeter/subscription/addon/repository.goopenmeter/subscription/addon/service.go
tothandras
requested changes
May 11, 2026
| namespace Subscriptions; | ||
|
|
||
| /** | ||
| * SubscriptionAddon represents an addon associated with a subscription. |
Contributor
There was a problem hiding this comment.
Don't use Go-style comments.
Suggested change
| * SubscriptionAddon represents an addon associated with a subscription. | |
| * Addon purchased with a subscription. |
Collaborator
Author
There was a problem hiding this comment.
thanks, fixed. Should I change this in the PlanAddons comment as well?
borosr
force-pushed
the
feat/subscription-addons-list
branch
from
1f5f8ce to
d1418e0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/v3/openapi.yaml (1)
2562-2569: ⚡ Quick winDocument supported sort attributes for this endpoint.
At Line 2562,
sortis exposed but the operation does not define which fields are accepted (unlike other list endpoints). Adding this avoids trial-and-error and client-side ambiguity.📌 Proposed doc refinement
- name: sort in: query required: false + description: |- + Sort subscription add-ons returned in the response. + Supported sort attributes are: + - `id` + - `active_from` (default) + - `active_to` + - `created_at` + + The `asc` suffix is optional as the default sort order is ascending. + The `desc` suffix is used to specify a descending order. schema: $ref: '#/components/schemas/SortQuery' explode: false style: form🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v3/openapi.yaml` around lines 2562 - 2569, The operation exposes the query parameter named "sort" (schema $ref: '#/components/schemas/SortQuery') but doesn't document which sort attributes are allowed; update this endpoint's parameter definition to list the supported sort fields (matching the style used by other list endpoints) by either adding a descriptive "description" enumerating allowed attributes and direction or by extending/overriding the SortQuery schema with an enum of accepted fields and examples; ensure the parameter stays named "sort" and retains explode/style settings so clients can programmatically discover valid sort keys without trial-and-error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v3/handlers/subscriptions/subscriptionaddons/convert.go`:
- Line 18: Replace the blind error discard on the addon.GetInstanceAt(now) call
by checking the returned error: call addon.GetInstanceAt(now) and if err is nil
use inst as before; if err indicates the expected “no active instance at this
timestamp” condition (the specific sentinel/error type your codebase uses),
treat it as Quantity=0; otherwise propagate or return the error so unexpected
failures are not silently ignored. Ensure you reference addon.GetInstanceAt, the
returned err and inst variables, and only suppress the known “no active
instance” error while returning any other error up the call stack.
- Line 44: The ActiveFrom assignment currently uses lo.FromPtrOr(union.From,
now) which makes the output time-dependent; change the fallback to a stable
deterministic value (for example addon.CreatedAt) or make the API field
nullable: replace the second argument of lo.FromPtrOr from now to a stable
timestamp (e.g., addon.CreatedAt) or adjust the conversion so ActiveFrom is set
to nil when union.From is nil; update the ActiveFrom assignment in the
conversion function that constructs the addon DTO (the code using lo.FromPtrOr,
union.From and ActiveFrom) accordingly.
---
Nitpick comments:
In `@api/v3/openapi.yaml`:
- Around line 2562-2569: The operation exposes the query parameter named "sort"
(schema $ref: '#/components/schemas/SortQuery') but doesn't document which sort
attributes are allowed; update this endpoint's parameter definition to list the
supported sort fields (matching the style used by other list endpoints) by
either adding a descriptive "description" enumerating allowed attributes and
direction or by extending/overriding the SortQuery schema with an enum of
accepted fields and examples; ensure the parameter stays named "sort" and
retains explode/style settings so clients can programmatically discover valid
sort keys without trial-and-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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e47d131-71c5-45b8-877b-0053d51250f9
📒 Files selected for processing (16)
api/spec/packages/aip/src/konnect.tspapi/spec/packages/aip/src/openmeter.tspapi/spec/packages/aip/src/subscriptions/index.tspapi/spec/packages/aip/src/subscriptions/operations.tspapi/spec/packages/aip/src/subscriptions/subscriptionaddon.tspapi/v3/api.gen.goapi/v3/handlers/subscriptions/subscriptionaddons/convert.goapi/v3/handlers/subscriptions/subscriptionaddons/handler.goapi/v3/handlers/subscriptions/subscriptionaddons/list.goapi/v3/openapi.yamlapi/v3/server/routes.goapi/v3/server/server.goopenmeter/server/server.goopenmeter/subscription/addon/repo/subscriptionaddon.goopenmeter/subscription/addon/repository.goopenmeter/subscription/addon/service.go
✅ Files skipped from review due to trivial changes (2)
- api/spec/packages/aip/src/subscriptions/index.tsp
- openmeter/server/server.go
🚧 Files skipped from review as they are similar to previous changes (10)
- api/v3/server/routes.go
- openmeter/subscription/addon/repo/subscriptionaddon.go
- api/v3/handlers/subscriptions/subscriptionaddons/handler.go
- api/spec/packages/aip/src/openmeter.tsp
- api/spec/packages/aip/src/subscriptions/subscriptionaddon.tsp
- openmeter/subscription/addon/repository.go
- api/spec/packages/aip/src/konnect.tsp
- openmeter/subscription/addon/service.go
- api/spec/packages/aip/src/subscriptions/operations.tsp
- api/v3/server/server.go
| now := clock.Now() | ||
|
|
||
| // If no instance is active at `now`, quantity stays 0. | ||
| inst, _ := addon.GetInstanceAt(now) |
Contributor
There was a problem hiding this comment.
Handle GetInstanceAt errors selectively instead of dropping all errors.
Line 18 ignores the error completely, so non-“not active at this timestamp” failures can silently become Quantity=0. Please only suppress the expected “no active instance” case and propagate other errors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/v3/handlers/subscriptions/subscriptionaddons/convert.go` at line 18,
Replace the blind error discard on the addon.GetInstanceAt(now) call by checking
the returned error: call addon.GetInstanceAt(now) and if err is nil use inst as
before; if err indicates the expected “no active instance at this timestamp”
condition (the specific sentinel/error type your codebase uses), treat it as
Quantity=0; otherwise propagate or return the error so unexpected failures are
not silently ignored. Ensure you reference addon.GetInstanceAt, the returned err
and inst variables, and only suppress the known “no active instance” error while
returning any other error up the call stack.
| }, | ||
| Quantity: inst.Quantity, | ||
| QuantityAt: now, | ||
| ActiveFrom: lo.FromPtrOr(union.From, now), |
Contributor
There was a problem hiding this comment.
Use a stable fallback for ActiveFrom (not now).
Line 44 makes ActiveFrom time-dependent when union.From is nil, so the same addon can return different values across requests. Prefer a deterministic fallback (e.g., addon.CreatedAt) or make the API field nullable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/v3/handlers/subscriptions/subscriptionaddons/convert.go` at line 44, The
ActiveFrom assignment currently uses lo.FromPtrOr(union.From, now) which makes
the output time-dependent; change the fallback to a stable deterministic value
(for example addon.CreatedAt) or make the API field nullable: replace the second
argument of lo.FromPtrOr from now to a stable timestamp (e.g., addon.CreatedAt)
or adjust the conversion so ActiveFrom is set to nil when union.From is nil;
update the ActiveFrom assignment in the conversion function that constructs the
addon DTO (the code using lo.FromPtrOr, union.From and ActiveFrom) accordingly.
tothandras
previously approved these changes
May 11, 2026
tothandras
force-pushed
the
feat/subscription-addons-list
branch
from
76bedd3 to
6b5599e
Compare
tothandras
approved these changes
May 11, 2026
This was referenced May 19, 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.
Example request:
curl --request GET \ --url 'http://localhost:8888/api/v3/openmeter/subscriptions/01KQZ4K3R4YT29RS928Z1VC92T/addons'Summary by CodeRabbit
New Features
Documentation