feat(subscription addon): v3 assign add-on to a subscription endpoint#4392
Open
borosr wants to merge 12 commits into
Open
feat(subscription addon): v3 assign add-on to a subscription endpoint#4392borosr wants to merge 12 commits into
borosr wants to merge 12 commits into
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:
📝 WalkthroughWalkthroughAdds a POST API to create subscription add‑ons (TypeSpec + model changes), maps requests into the subscription workflow input, implements a create handler that invokes SubscriptionWorkflowService, and wires the workflow service into server routing and config. ChangesCreate Subscription Add-on Feature
Sequence DiagramsequenceDiagram
participant Client
participant CreateAddonHandler
participant SubscriptionAddonService
participant SubscriptionWorkflowService
Client->>CreateAddonHandler: POST /subscriptions/{id}/addons (CreateRequest)
CreateAddonHandler->>CreateAddonHandler: Parse request and resolve namespace
CreateAddonHandler->>SubscriptionAddonService: List existing add-ons
alt Add-on exists
CreateAddonHandler->>SubscriptionWorkflowService: Update existing add-on (workflow)
else Add-on is new
CreateAddonHandler->>SubscriptionWorkflowService: AddAddon (workflow)
end
SubscriptionWorkflowService-->>CreateAddonHandler: Created/updated add-on
CreateAddonHandler->>Client: 201 Created + SubscriptionAddon JSON
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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: 2
🤖 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/create.go`:
- Around line 64-76: The handler currently performs an implicit upsert: when an
existing addon (found via lo.Find on subsAdds.Items matching
request.AddonInput.AddonID) is present it calls
SubscriptionWorkflowService.ChangeAddonQuantity but still behaves like a Create
(returns 201). Change this to strict-create behavior: if sAdd is found, do NOT
call ChangeAddonQuantity and instead return a 409 Conflict (with a clear error
message) so clients know the addon already exists; otherwise proceed to call
SubscriptionWorkflowService.AddAddon as before. Ensure the branch that
previously called ChangeAddonQuantity (using
SubscriptionWorkflowService.ChangeAddonQuantity and variables sAdd,
request.SubscriptionID, subscriptionworkflow.ChangeAddonQuantityWorkflowInput)
is removed and replaced by the conflict response, and update any tests or
documentation to reflect the create-only contract.
- Line 87: The operation name passed to httptransport.WithOperationName is
incorrect ("create-plan-addon")—update the call in the subscription addon
handler to use the correct operation name "create-subscription-addon" so
traces/metrics align with the endpoint; locate the
httptransport.WithOperationName invocation in the create subscription addon
handler (the call shown as httptransport.WithOperationName("create-plan-addon"))
and replace the string accordingly.
🪄 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: 1559f8b3-689b-4674-adb5-8920d50634b3
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (9)
api/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/create.goapi/v3/handlers/subscriptions/subscriptionaddons/handler.goapi/v3/server/routes.goapi/v3/server/server.goopenmeter/server/server.go
borosr
force-pushed
the
feat/attach-subscription-addon-v3
branch
from
abb1276 to
5b8f77e
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
api/v3/handlers/subscriptions/subscriptionaddons/create.go (1)
64-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCreate endpoint still behaves like upsert, which conflicts with the declared contract.
Right now, when the add-on already exists, this path updates quantity (
ChangeAddonQuantity) and still responds as201 Created. That makescreate-subscription-addonambiguous for clients. Please either make this strict-create (conflict on existing add-on) or explicitly model upsert semantics and status codes in both spec and handler.As per coding guidelines, "The declared API should be accurate, in parity with the actual implementation, and easy to understand for the user."
Also applies to: 84-84
🤖 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/create.go` around lines 64 - 76, The handler currently upserts by calling SubscriptionWorkflowService.ChangeAddonQuantity when an existing addon is found; change this to strict-create semantics by detecting existing addon (the lo.Find check that returns sAdd) and returning an HTTP 409 Conflict (with a clear message) instead of calling SubscriptionWorkflowService.ChangeAddonQuantity, leaving SubscriptionWorkflowService.AddAddon to be called only in the else branch and returning 201 Created on successful AddAddon; alternatively, if you prefer upsert semantics, update the API spec and this handler to document/model upsert and return the appropriate status (e.g., 200/201) consistently—pick one approach and make the codepaths around ChangeAddonQuantity and AddAddon match that choice.
🤖 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.
Duplicate comments:
In `@api/v3/handlers/subscriptions/subscriptionaddons/create.go`:
- Around line 64-76: The handler currently upserts by calling
SubscriptionWorkflowService.ChangeAddonQuantity when an existing addon is found;
change this to strict-create semantics by detecting existing addon (the lo.Find
check that returns sAdd) and returning an HTTP 409 Conflict (with a clear
message) instead of calling SubscriptionWorkflowService.ChangeAddonQuantity,
leaving SubscriptionWorkflowService.AddAddon to be called only in the else
branch and returning 201 Created on successful AddAddon; alternatively, if you
prefer upsert semantics, update the API spec and this handler to document/model
upsert and return the appropriate status (e.g., 200/201) consistently—pick one
approach and make the codepaths around ChangeAddonQuantity and AddAddon match
that choice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a87d802d-712f-436c-9fb8-df1ed4447bea
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (9)
api/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/create.goapi/v3/handlers/subscriptions/subscriptionaddons/handler.goapi/v3/server/routes.goapi/v3/server/server.goopenmeter/server/server.go
✅ Files skipped from review due to trivial changes (1)
- openmeter/server/server.go
borosr
force-pushed
the
feat/attach-subscription-addon-v3
branch
from
5b8f77e to
3d3ce34
Compare
borosr
force-pushed
the
feat/attach-subscription-addon-v3
branch
from
d5d2649 to
104af03
Compare
borosr
force-pushed
the
feat/attach-subscription-addon-v3
branch
from
db09ff3 to
7ea1582
Compare
borosr
force-pushed
the
feat/attach-subscription-addon-v3
branch
from
7ea1582 to
5fa8a4a
Compare
Contributor
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1
to
+3
| -- create index "subscriptionaddon_namespace_subscription_id_addon_id" to table: "subscription_addons" | ||
| -- atlas:nolint MF101 | ||
| CREATE UNIQUE INDEX "subscriptionaddon_namespace_subscription_id_addon_id" ON "subscription_addons" ("namespace", "subscription_id", "addon_id") WHERE (deleted_at IS NULL); |
Contributor
There was a problem hiding this comment.
atlas:nolint MF101 suppresses data-safety check without a cleanup step
MF101 fires because adding a unique index to an existing table fails at deploy time if there are pre-existing rows that violate the constraint. The v1 API's AddAddon workflow had an application-level duplicate guard, but that guard ran in a separate step before Create, leaving a window where concurrent requests or retries could have inserted duplicates into subscription_addons. A DELETE / UPDATE deduplication CTE before the CREATE UNIQUE INDEX statement (or a pre-migration data audit confirming no duplicates exist) would close this risk and make the nolint suppression unnecessary.
borosr
force-pushed
the
feat/attach-subscription-addon-v3
branch
from
5fa8a4a to
1c703e3
Compare
GAlexIHU
reviewed
Jun 17, 2026
| Annotations( | ||
| entsql.IndexWhere("deleted_at IS NULL"), | ||
| ). | ||
| Unique(), |
Contributor
There was a problem hiding this comment.
is it unique? how's quantity handled?
Contributor
Author
There was a problem hiding this comment.
@tothandras what do you think about that? Should we keep this unique or support multiple addon attachment to the same subscription?
borosr
force-pushed
the
feat/attach-subscription-addon-v3
branch
2 times, most recently
from
a176b02 to
3bc5ab1
Compare
Signed-off-by: Andras Toth <4157749+tothandras@users.noreply.github.com>
borosr
force-pushed
the
feat/attach-subscription-addon-v3
branch
from
cbb2126 to
457539d
Compare
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
3 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.
Add new v3 endpoint to assign an add-on to a subscription
What
Add new V3 endpoint for assign an add-on to a subscription
POST /api/v3/openmeter/subscriptions/{subscriptionID}/addons.Testing
curl --request POST \ --url http://localhost:8888/api/v3/openmeter/subscriptions/{subscriptionID}/addons \ --header 'Content-Type: application/json' \ --data '{ "name": "addon", "quantity": 1, "timing": "immediate", "addon": { "id": "<addonID>" } }'Summary by CodeRabbit