Skip to content

Feat(subscription addon): GetByID endpoint#4318

Merged
borosr merged 9 commits into
mainfrom
feat/subscription-addon-get
May 18, 2026
Merged

Feat(subscription addon): GetByID endpoint#4318
borosr merged 9 commits into
mainfrom
feat/subscription-addon-get

Conversation

@borosr

@borosr borosr commented May 7, 2026

Copy link
Copy Markdown
Contributor

Example request:

curl --request GET \
  --url 'http://localhost:8888/api/v3/openmeter/subscriptions/01KQZ4K3R4YT29RS928Z1VC92T/addons/01KQZ4S6447Y41HZ3WMWQZ7QTS'

Summary by CodeRabbit

  • New Features

    • Added a GET API to fetch subscription add-on associations, returning addon reference, quantity with effective timestamp, active-from/active-to window, audit metadata, and supporting lookup by add-on ID or by subscription+addon identifier.
  • Tests

    • Updated subscription add-on service tests to cover the new retrieval and lookup behaviors.

Review Change Stack

@borosr borosr self-assigned this May 7, 2026
@borosr
borosr requested a review from a team as a code owner May 7, 2026 13:17
@borosr borosr added the release-note/ignore Ignore this change when generating release notes label May 7, 2026
@coderabbitai

coderabbitai Bot commented May 7, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds 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.

Changes

Subscription Addon Retrieval

Layer / File(s) Summary
API Specification
api/spec/packages/aip/src/subscriptions/*, api/spec/packages/aip/src/openmeter.tsp, api/spec/packages/aip/src/konnect.tsp
Adds Subscriptions.SubscriptionAddon model, imports, SubscriptionAddonOperations GET and SubscriptionAddonEndpoints route.
Generated API & Routing
api/v3/api.gen.go
Regenerates apiv3.SubscriptionAddon type; adds ServerInterface/GetSubscriptionAddon, Unimplemented stub, ServerInterfaceWrapper binding, route registration, and updated embedded swagger blob.
Domain Input & Validation
openmeter/subscription/addon/service.go
Adds GetSubscriptionAddonInput (embeds NamespacedID, adds subscriptionId and addonIdOrKey) and Validate(); updates service Get signature.
Repository Query Logic
openmeter/subscription/addon/repo/subscriptionaddon.go, openmeter/subscription/addon/repository.go
Repository Get now accepts structured input and branches: direct ID lookup or namespace+subscription lookup matching addon by ID or key.
Service Implementation & Call Sites
openmeter/subscription/addon/service/service.go, openmeter/subscription/workflow/service/addon.go
Service validates input and delegates to repository; Create and ChangeQuantity refetch using GetSubscriptionAddonInput.
v3 Handler & Conversion
api/v3/handlers/subscriptions/subscriptionaddons/*
Adds v3 handler package and GetSubscriptionAddon handler; ToAPISubscriptionAddon computes active window, quantity, and labels.
Server Wiring
api/v3/server/server.go, api/v3/server/routes.go, openmeter/server/server.go
Config adds SubscriptionAddonService; NewServer conditionally constructs addon handler; routes register GetSubscriptionAddon.
App HTTP Handler & Tests
openmeter/subscription/addon/http/get.go, openmeter/subscription/addon/service/create_test.go, openmeter/server/server_test.go
App handler and tests/mocks updated to construct and pass GetSubscriptionAddonInput; test mocks signature updated.
Formatting / Misc
openmeter/server/server.go
Reformatting of v3 server Config literal (no logic changes).

Sequence Diagram

sequenceDiagram
  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)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • openmeterio/openmeter#2706: Modifies subscription add-on service and validations; overlaps with service/repository changes here.

Suggested labels

release-note/feature, area/subscriptions

Suggested reviewers

  • tothandras
  • GAlexIHU
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically identifies the main change: adding a GET endpoint for retrieving subscription addons by ID, which aligns perfectly with the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/subscription-addon-get

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Missing 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 hitting GET /subscriptions/A/addons/B where addon B belongs to subscription C will get back a 200 with the addon data mashed against subscription A's view, which is both incorrect and confusing.

The workflow service already does this right (see openmeter/subscription/workflow/service/addon.go line 133):

if subsAdd.SubscriptionID != subscriptionID.ID { ... }

A quick guard right after the Get call 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 win

Not-found message can render with an empty ID.

When the alt branch (no params.ID, lookup by SubscriptionID + AddonIDOrKey) hits a miss, the error reads subscription addon not found with 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() requires SubscriptionAddonService, so the nil-guard in NewServer() is redundant

Both SubscriptionAddonService (line 207) and SubscriptionService (line 169) are required in Validate(), so NewServer() is never called without them. The if config.SubscriptionAddonService != nil && config.SubscriptionService != nil condition is always true and may confuse future readers into thinking the handler is optional (unlike the intentionally optional chargesHandler).

✂️ 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 value

The len(periods) == 0 guard is unreachable — worth cleaning up

After GetInstanceAt(now) returns found = true at line 20, there's at least one instance, so a.GetInstances() always returns a non-empty slice. The len(periods) == 0 branch 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 redundant Union(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 win

Mock update looks good, but the new route has no smoke test 🙂

The NoopSubscriptionAddonService.Get signature is correctly aligned with the updated interface. However, TestRoutes has no test case for the newly added GET /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 value

Small refactor idea: collapse the two branches into a single Where chain.

Totally optional, but the two branches share Namespace filtering and the asymmetry (optional SubscriptionID in 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8d3a5a0 and e7b1a69.

⛔ Files ignored due to path filters (1)
  • api/v3/openapi.yaml is excluded by !**/openapi.yaml
📒 Files selected for processing (20)
  • api/spec/packages/aip/src/konnect.tsp
  • api/spec/packages/aip/src/openmeter.tsp
  • api/spec/packages/aip/src/subscriptions/index.tsp
  • api/spec/packages/aip/src/subscriptions/operations.tsp
  • api/spec/packages/aip/src/subscriptions/subscriptionaddon.tsp
  • api/v3/api.gen.go
  • api/v3/handlers/subscriptions/subscriptionaddons/convert.go
  • api/v3/handlers/subscriptions/subscriptionaddons/get.go
  • api/v3/handlers/subscriptions/subscriptionaddons/handler.go
  • api/v3/server/routes.go
  • api/v3/server/server.go
  • openmeter/server/server.go
  • openmeter/server/server_test.go
  • openmeter/subscription/addon/http/get.go
  • openmeter/subscription/addon/repo/subscriptionaddon.go
  • openmeter/subscription/addon/repository.go
  • openmeter/subscription/addon/service.go
  • openmeter/subscription/addon/service/create_test.go
  • openmeter/subscription/addon/service/service.go
  • openmeter/subscription/workflow/service/addon.go

Comment thread api/v3/handlers/subscriptions/subscriptionaddons/convert.go Outdated
Comment thread api/v3/handlers/subscriptions/subscriptionaddons/get.go Outdated
Comment thread openmeter/subscription/addon/service.go
Comment thread openmeter/subscription/addon/service.go Outdated
Comment thread openmeter/subscription/addon/service/create_test.go Outdated

@GAlexIHU GAlexIHU left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment thread openmeter/subscription/addon/service.go
Comment thread openmeter/subscription/addon/service.go Outdated
Comment thread api/v3/handlers/subscriptions/subscriptionaddons/get.go
@borosr
borosr force-pushed the feat/subscription-addon-get branch from 0f67a4e to 4ed9ff1 Compare May 12, 2026 10:21
@borosr
borosr force-pushed the feat/subscription-addon-get branch from 4ed9ff1 to 24cedcc Compare May 13, 2026 08:15
@borosr
borosr force-pushed the feat/subscription-addon-get branch from 24cedcc to 6efe000 Compare May 13, 2026 08:16
@borosr
borosr requested a review from GAlexIHU May 13, 2026 08:16
Comment thread openmeter/subscription/addon/service.go Outdated
tothandras
tothandras previously approved these changes May 13, 2026
@borosr
borosr dismissed stale reviews from tothandras and GAlexIHU via e8e6322 May 13, 2026 13:25
@borosr
borosr force-pushed the feat/subscription-addon-get branch from e8e6322 to 46f6e22 Compare May 14, 2026 08:44
@borosr
borosr force-pushed the feat/subscription-addon-get branch from 46f6e22 to 6999ac9 Compare May 15, 2026 11:18
GAlexIHU
GAlexIHU previously approved these changes May 15, 2026
Comment thread api/v3/handlers/subscriptions/subscriptionaddons/convert.go Outdated
@borosr
borosr force-pushed the feat/subscription-addon-get branch from 5c4a559 to 1da6c8c Compare May 18, 2026 14:11
@borosr
borosr requested a review from tothandras May 18, 2026 14:11
@borosr
borosr merged commit 1831c73 into main May 18, 2026
30 checks passed
@borosr
borosr deleted the feat/subscription-addon-get branch May 18, 2026 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note/ignore Ignore this change when generating release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants