Feat(subscription addon): GetByID endpoint#4318
Conversation
|
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 SubscriptionAddon TypeSpec and routed endpoints, regenerates apiv3 types and routing, introduces validated GetSubscriptionAddonInput, updates repository queries and service signatures to accept structured input, adds v3 handlers and conversion, wires server routes and config, and updates tests and call sites. ChangesSubscription Addon Retrieval
Sequence DiagramsequenceDiagram
participant Client
participant AppHandler
participant V3Handler
participant Service
participant Repo
Client->>AppHandler: GET /openmeter/subscriptions/{subscriptionId}/addons/{subscriptionAddonId}
AppHandler->>V3Handler: forward request (bind path params)
V3Handler->>Service: Get(GetSubscriptionAddonInput)
Service->>Repo: Get(GetSubscriptionAddonInput)
Repo-->>Service: domain SubscriptionAddon
Service-->>V3Handler: domain SubscriptionAddon
V3Handler->>Client: 200 JSON (apiv3.SubscriptionAddon)
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 |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openmeter/subscription/addon/http/get.go (1)
41-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing ownership check: addon's subscription isn't validated against the URL's
subscriptionId.The handler fetches the addon by its own ID, then separately fetches the subscription view using
req.SubscriptionID— but it never verifies that the two are actually related. A caller hittingGET /subscriptions/A/addons/Bwhere addonBbelongs to subscriptionCwill get back a 200 with the addon data mashed against subscriptionA's view, which is both incorrect and confusing.The workflow service already does this right (see
openmeter/subscription/workflow/service/addon.goline 133):if subsAdd.SubscriptionID != subscriptionID.ID { ... }A quick guard right after the
Getcall would fix it:🛠 Proposed fix
res, err := h.SubscriptionAddonService.Get(ctx, subscriptionaddon.GetSubscriptionAddonInput{ NamespacedID: req.SubscriptionAddonID, }) if err != nil { return GetSubscriptionAddonResponse{}, err } + +if res.SubscriptionID != req.SubscriptionID.ID { + return GetSubscriptionAddonResponse{}, models.NewGenericNotFoundError( + fmt.Errorf("subscription addon %s not found in subscription %s", req.SubscriptionAddonID.ID, req.SubscriptionID.ID), + ) +} view, err := h.SubscriptionService.GetView(ctx, req.SubscriptionID)🤖 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/http/get.go` around lines 41 - 55, The handler is missing an ownership check: after calling h.SubscriptionAddonService.Get (result stored in res), validate that res.SubscriptionID equals req.SubscriptionID and return an appropriate not-found/permission error if they differ before calling h.SubscriptionService.GetView or MapSubscriptionAddonToResponse; update the anonymous handler to perform this comparison (use res.SubscriptionID and req.SubscriptionID) and short-circuit with an error when they don't match.openmeter/subscription/addon/repo/subscriptionaddon.go (1)
75-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNot-found message can render with an empty ID.
When the alt branch (no
params.ID, lookup bySubscriptionID+AddonIDOrKey) hits a miss, the error readssubscription addon not foundwith a blank where the ID should be — not super helpful for debugging or for clients parsing the message.🪄 Suggested tweak
if err != nil { if db.IsNotFound(err) { + identifier := params.ID + if identifier == "" { + identifier = fmt.Sprintf("subscription=%s addon=%s", params.SubscriptionID, params.AddonIDOrKey) + } return nil, models.NewGenericNotFoundError( - fmt.Errorf("subscription addon %s not found", params.ID), + fmt.Errorf("subscription addon %s not found", identifier), ) } return nil, err }🤖 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 75 - 83, The not-found error message currently always embeds params.ID and can be empty; update the error construction in the lookup failure branch (where models.NewGenericNotFoundError is created) to include useful identifiers: if params.ID is non-empty keep "subscription addon <ID> not found", otherwise include the alternative keys like params.SubscriptionID and params.AddonIDOrKey (e.g. "subscription addon not found for subscription=<SubscriptionID> addon=<AddonIDOrKey>"); modify the error creation call (models.NewGenericNotFoundError) accordingly so clients and logs get meaningful context.
🧹 Nitpick comments (4)
api/v3/server/server.go (1)
207-209: 💤 Low value
Validate()requiresSubscriptionAddonService, so the nil-guard inNewServer()is redundantBoth
SubscriptionAddonService(line 207) andSubscriptionService(line 169) are required inValidate(), soNewServer()is never called without them. Theif config.SubscriptionAddonService != nil && config.SubscriptionService != nilcondition is always true and may confuse future readers into thinking the handler is optional (unlike the intentionally optionalchargesHandler).✂️ Proposed simplification
- var subscriptionAddonsH subscriptionaddonshandler.Handler - if config.SubscriptionAddonService != nil && config.SubscriptionService != nil { - subscriptionAddonsH = subscriptionaddonshandler.New(resolveNamespace, config.SubscriptionAddonService, httptransport.WithErrorHandler(config.ErrorHandler)) - } + subscriptionAddonsH := subscriptionaddonshandler.New(resolveNamespace, config.SubscriptionAddonService, httptransport.WithErrorHandler(config.ErrorHandler))Also applies to: 318-321
🤖 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/server/server.go` around lines 207 - 209, NewServer currently checks config.SubscriptionAddonService and config.SubscriptionService before creating the subscription handler, but Validate() already requires both SubscriptionAddonService and SubscriptionService, so remove the redundant nil-guard in NewServer: always construct the subscription handler (the code that references config.SubscriptionAddonService and config.SubscriptionService) and drop the surrounding if-check; keep the existing conditional logic only for the optional chargesHandler. Update both occurrences mentioned (the initial handler construction and the similar block at the later occurrence) so they unconditionally build the subscription handler while leaving chargesHandler conditional.api/v3/handlers/subscriptions/subscriptionaddons/convert.go (1)
20-31: 💤 Low valueThe
len(periods) == 0guard is unreachable — worth cleaning upAfter
GetInstanceAt(now)returnsfound = trueat line 20, there's at least one instance, soa.GetInstances()always returns a non-empty slice. Thelen(periods) == 0branch can never be reached and silently misleads future readers into thinking it's a meaningful guard.✂️ Proposed clean-up
periods := lo.Map(a.GetInstances(), func(i subscriptionaddon.SubscriptionAddonInstance, _ int) timeutil.OpenPeriod { return i.AsPeriod() }) - if len(periods) == 0 { - return apiv3.SubscriptionAddon{}, errors.New("no instances found") - } - union := lo.Reduce(periods, func(agg timeutil.OpenPeriod, item timeutil.OpenPeriod, _ int) timeutil.OpenPeriod { + union := lo.Reduce(periods[1:], func(agg timeutil.OpenPeriod, item timeutil.OpenPeriod, _ int) timeutil.OpenPeriod { return agg.Union(item) }, periods[0])Skipping
periods[0]in the reduce also avoids the redundantUnion(x, x)on the first iteration.🤖 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` around lines 20 - 31, The len(periods) == 0 check is unreachable after GetInstanceAt(now) returns found, so remove that guard and simplify the logic: compute periods via lo.Map(a.GetInstances(), ...) as before, then when reducing/unioning periods avoid the redundant Union(x,x) by starting the reduction from periods[1] (i.e., take periods[0] as the initial accumulator and iterate the rest) rather than always iterating from index 0; update any reduction code that references periods to use that pattern and keep uses of subscriptionaddon.SubscriptionAddonInstance.AsPeriod and GetInstances/GetInstanceAt intact.openmeter/server/server_test.go (1)
1683-1685: ⚡ Quick winMock update looks good, but the new route has no smoke test 🙂
The
NoopSubscriptionAddonService.Getsignature is correctly aligned with the updated interface. However,TestRouteshas no test case for the newly addedGET /api/v3/openmeter/subscriptions/{subscriptionId}/addons/{subscriptionAddonId}route. Even a simple 200/404 smoke test (similar to"get meter") would catch routing regressions early.💡 Example smoke-test entry to add to the test table in
TestRoutes+ { + name: "get subscription addon", + req: testRequest{ + method: http.MethodGet, + path: "/api/v3/openmeter/subscriptions/01KQZ4K3R4YT29RS928Z1VC92T/addons/01KQZ4S6447Y41HZ3WMWQZ7QTS", + }, + res: testResponse{ + // NoopSubscriptionAddonService.Get returns (nil, nil), so the handler + // should respond with 404 or 200 depending on your not-found contract. + status: http.StatusNotFound, + }, + },🤖 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/server/server_test.go` around lines 1683 - 1685, Add a smoke-test entry to the TestRoutes table to exercise the new GET endpoint for subscription addons: create a test row named like "get subscription addon" that sends a GET to /api/v3/openmeter/subscriptions/{subscriptionId}/addons/{subscriptionAddonId} (use literal UUIDs or the same helpers used by other tests), expect a 200 (and optionally add a second row expecting 404 for a missing addon), and ensure the test harness uses the NoopSubscriptionAddonService implementation so the route is wired and exercised; update the TestRoutes table in TestRoutes to mirror the style of the existing "get meter" case (method, path, expected status, and any auth headers) so routing regressions are caught.openmeter/subscription/addon/repo/subscriptionaddon.go (1)
54-72: 💤 Low valueSmall refactor idea: collapse the two branches into a single
Wherechain.Totally optional, but the two branches share
Namespacefiltering and the asymmetry (optionalSubscriptionIDin the ID-branch vs required in the alt-branch) is a bit easy to miss when scanning. Something like below reads more uniformly and keeps the conditional filters localized:♻️ Sketch
- query := querySubscriptionAddon(repo.db.SubscriptionAddon.Query()) - - if params.ID != "" { - query = query.Where( - dbsubscriptionaddon.ID(params.ID), - dbsubscriptionaddon.Namespace(params.Namespace), - ) - if params.SubscriptionID != "" { - query = query.Where(dbsubscriptionaddon.SubscriptionID(params.SubscriptionID)) - } - } else { - query = query.Where( - dbsubscriptionaddon.Namespace(params.Namespace), - dbsubscriptionaddon.SubscriptionID(params.SubscriptionID), - dbsubscriptionaddon.HasAddonWith(addondb.Or(addondb.ID(params.AddonIDOrKey), addondb.Key(params.AddonIDOrKey))), - ) - } + query := querySubscriptionAddon(repo.db.SubscriptionAddon.Query()). + Where(dbsubscriptionaddon.Namespace(params.Namespace)) + + if params.ID != "" { + query = query.Where(dbsubscriptionaddon.ID(params.ID)) + } else { + query = query.Where(dbsubscriptionaddon.HasAddonWith( + addondb.Or(addondb.ID(params.AddonIDOrKey), addondb.Key(params.AddonIDOrKey)), + )) + } + if params.SubscriptionID != "" { + query = query.Where(dbsubscriptionaddon.SubscriptionID(params.SubscriptionID)) + }As per coding guidelines, "make readability and maintainability a priority, even potentially suggest restructuring the code to improve them."
🤖 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 54 - 72, The Get method duplicates Namespace filtering across two branches; refactor Get by building a single query via querySubscriptionAddon(repo.db.SubscriptionAddon.Query()) then always apply dbsubscriptionaddon.Namespace(params.Namespace) and conditionally append dbsubscriptionaddon.ID(params.ID) and dbsubscriptionaddon.SubscriptionID(params.SubscriptionID) (only when non-empty) and, when ID is empty, append dbsubscriptionaddon.HasAddonWith(addondb.Or(addondb.ID(params.AddonIDOrKey), addondb.Key(params.AddonIDOrKey))); update function Get and the local variable querySubscriptionAddon usage so the filters are chained uniformly rather than split across the if/else branches.
🤖 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`:
- Around line 33-50: The code dereferences union.From when building the
apiv3.SubscriptionAddon (ActiveFrom: *union.From) without a nil check; add a
defensive nil guard after computing union (from lo.Reduce over periods) that
returns an error (e.g., "active_from could not be resolved: no instance has a
start time") if union.From == nil, and only then construct and return the
apiv3.SubscriptionAddon with ActiveFrom set to *union.From; reference the union
variable, the Union() results from periods (CadencedModel.AsPeriod()), and the
apiv3.SubscriptionAddon/ActiveFrom field when making this change.
In `@api/v3/handlers/subscriptions/subscriptionaddons/get.go`:
- Line 46: The handler currently dereferences the addon pointer returned from
h.service.Get by calling ToAPISubscriptionAddon(*a) which will panic if
h.service.Get returns (nil, nil); add a nil check for the returned value (the
variable `a` from h.service.Get) before dereferencing, and handle the nil case
appropriately (e.g., return a not-found response or a clear error) instead of
calling ToAPISubscriptionAddon when `a` is nil.
In `@openmeter/subscription/addon/service.go`:
- Around line 56-65: The error message when i.ID is empty incorrectly mentions
"subscription id or key" even though only SubscriptionID is checked; update the
error text in the validation branch that references i.SubscriptionID (the
conditional that appends errors when i.ID=="" and i.SubscriptionID=="") to say
"subscription id must be provided if assignment id is not provided" (matching
the actual field checked); leave the AddonIDOrKey message unchanged.
- Around line 49-73: The unconditional call to i.NamespacedID.Validate() in
GetSubscriptionAddonInput.Validate blocks the alternate path because it requires
an ID even when i.ID is meant to be empty; change the logic to validate
Namespace separately (e.g. check i.NamespacedID.Namespace is non-empty) before
branching, then remove the unconditional NamespacedID.Validate() call and
instead validate the assignment ID only in the else-branch (parse i.ID with
ulid.Parse) and validate the subscription path inside the if i.ID == "" branch
(ensure i.SubscriptionID and i.AddonIDOrKey are present and parse
i.SubscriptionID with ulid.Parse); keep collecting errors into errs and return
errors.Join(errs...) as before so each branch enforces its own ID requirements
without blocking the alternate path.
In `@openmeter/subscription/addon/service/create_test.go`:
- Around line 496-498: The test uses context.Background() when calling
SubscriptionAddonService.Get with GetSubscriptionAddonInput; replace
context.Background() with the test-provided context t.Context() so the call to
SubscriptionAddonService.Get(...) uses t.Context() and the test's
cancellation/lifecycle is respected.
---
Outside diff comments:
In `@openmeter/subscription/addon/http/get.go`:
- Around line 41-55: The handler is missing an ownership check: after calling
h.SubscriptionAddonService.Get (result stored in res), validate that
res.SubscriptionID equals req.SubscriptionID and return an appropriate
not-found/permission error if they differ before calling
h.SubscriptionService.GetView or MapSubscriptionAddonToResponse; update the
anonymous handler to perform this comparison (use res.SubscriptionID and
req.SubscriptionID) and short-circuit with an error when they don't match.
In `@openmeter/subscription/addon/repo/subscriptionaddon.go`:
- Around line 75-83: The not-found error message currently always embeds
params.ID and can be empty; update the error construction in the lookup failure
branch (where models.NewGenericNotFoundError is created) to include useful
identifiers: if params.ID is non-empty keep "subscription addon <ID> not found",
otherwise include the alternative keys like params.SubscriptionID and
params.AddonIDOrKey (e.g. "subscription addon not found for
subscription=<SubscriptionID> addon=<AddonIDOrKey>"); modify the error creation
call (models.NewGenericNotFoundError) accordingly so clients and logs get
meaningful context.
---
Nitpick comments:
In `@api/v3/handlers/subscriptions/subscriptionaddons/convert.go`:
- Around line 20-31: The len(periods) == 0 check is unreachable after
GetInstanceAt(now) returns found, so remove that guard and simplify the logic:
compute periods via lo.Map(a.GetInstances(), ...) as before, then when
reducing/unioning periods avoid the redundant Union(x,x) by starting the
reduction from periods[1] (i.e., take periods[0] as the initial accumulator and
iterate the rest) rather than always iterating from index 0; update any
reduction code that references periods to use that pattern and keep uses of
subscriptionaddon.SubscriptionAddonInstance.AsPeriod and
GetInstances/GetInstanceAt intact.
In `@api/v3/server/server.go`:
- Around line 207-209: NewServer currently checks
config.SubscriptionAddonService and config.SubscriptionService before creating
the subscription handler, but Validate() already requires both
SubscriptionAddonService and SubscriptionService, so remove the redundant
nil-guard in NewServer: always construct the subscription handler (the code that
references config.SubscriptionAddonService and config.SubscriptionService) and
drop the surrounding if-check; keep the existing conditional logic only for the
optional chargesHandler. Update both occurrences mentioned (the initial handler
construction and the similar block at the later occurrence) so they
unconditionally build the subscription handler while leaving chargesHandler
conditional.
In `@openmeter/server/server_test.go`:
- Around line 1683-1685: Add a smoke-test entry to the TestRoutes table to
exercise the new GET endpoint for subscription addons: create a test row named
like "get subscription addon" that sends a GET to
/api/v3/openmeter/subscriptions/{subscriptionId}/addons/{subscriptionAddonId}
(use literal UUIDs or the same helpers used by other tests), expect a 200 (and
optionally add a second row expecting 404 for a missing addon), and ensure the
test harness uses the NoopSubscriptionAddonService implementation so the route
is wired and exercised; update the TestRoutes table in TestRoutes to mirror the
style of the existing "get meter" case (method, path, expected status, and any
auth headers) so routing regressions are caught.
In `@openmeter/subscription/addon/repo/subscriptionaddon.go`:
- Around line 54-72: The Get method duplicates Namespace filtering across two
branches; refactor Get by building a single query via
querySubscriptionAddon(repo.db.SubscriptionAddon.Query()) then always apply
dbsubscriptionaddon.Namespace(params.Namespace) and conditionally append
dbsubscriptionaddon.ID(params.ID) and
dbsubscriptionaddon.SubscriptionID(params.SubscriptionID) (only when non-empty)
and, when ID is empty, append
dbsubscriptionaddon.HasAddonWith(addondb.Or(addondb.ID(params.AddonIDOrKey),
addondb.Key(params.AddonIDOrKey))); update function Get and the local variable
querySubscriptionAddon usage so the filters are chained uniformly rather than
split across the if/else branches.
🪄 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: f4b1bd0f-cf95-47be-833a-b32a6957a9ec
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (20)
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/get.goapi/v3/handlers/subscriptions/subscriptionaddons/handler.goapi/v3/server/routes.goapi/v3/server/server.goopenmeter/server/server.goopenmeter/server/server_test.goopenmeter/subscription/addon/http/get.goopenmeter/subscription/addon/repo/subscriptionaddon.goopenmeter/subscription/addon/repository.goopenmeter/subscription/addon/service.goopenmeter/subscription/addon/service/create_test.goopenmeter/subscription/addon/service/service.goopenmeter/subscription/workflow/service/addon.go
6ef417c to
b78efd7
Compare
b78efd7 to
34962e1
Compare
34962e1 to
646a10c
Compare
646a10c to
9b390ba
Compare
GAlexIHU
left a comment
There was a problem hiding this comment.
the way you chose to do this feels a bit weird for me. can you please just copy the implementation/api from somewhere else where we do this?
0f67a4e to
4ed9ff1
Compare
4ed9ff1 to
24cedcc
Compare
24cedcc to
6efe000
Compare
e8e6322 to
46f6e22
Compare
46f6e22 to
6999ac9
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: András Tóth <4157749+tothandras@users.noreply.github.com>
5c4a559 to
1da6c8c
Compare
Example request:
curl --request GET \ --url 'http://localhost:8888/api/v3/openmeter/subscriptions/01KQZ4K3R4YT29RS928Z1VC92T/addons/01KQZ4S6447Y41HZ3WMWQZ7QTS'Summary by CodeRabbit
New Features
Tests