feat(subscription addon): v3 assign add-on to a subscription endpoint#4392
Open
borosr wants to merge 2 commits into
Open
feat(subscription addon): v3 assign add-on to a subscription endpoint#4392borosr wants to merge 2 commits into
borosr wants to merge 2 commits into
Conversation
borosr
added
the
release-note/ignore
Contributor
📝 WalkthroughWalkthroughAdds a POST API to create subscription add‑ons (TypeSpec contract + timing field), maps requests to the subscription workflow input, implements a create-or-update handler, and wires the SubscriptionWorkflowService into server routing and config. ChangesCreate Subscription Add-on Feature
Sequence DiagramsequenceDiagram
participant Client
participant CreateAddonHandler
participant SubscriptionAddonService
participant SubscriptionWorkflowService
participant Client2 as Client (response)
Client->>CreateAddonHandler: POST /subscriptions/{id}/addons
CreateAddonHandler->>CreateAddonHandler: Parse request + resolve namespace
CreateAddonHandler->>SubscriptionAddonService: List existing add-ons
alt Add-on exists
CreateAddonHandler->>SubscriptionWorkflowService: ChangeAddonQuantity
else Add-on is new
CreateAddonHandler->>SubscriptionWorkflowService: AddAddon
end
SubscriptionWorkflowService-->>CreateAddonHandler: Created/updated add-on
CreateAddonHandler->>Client2: 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 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: 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
Comment on lines
+64
to
+76
| // If the addon is already present, we'll change the quantity instead as a convenience | ||
| if sAdd, ok := lo.Find(subsAdds.Items, func(subAdd subscriptionaddon.SubscriptionAddon) bool { | ||
| return subAdd.Addon.ID == request.AddonInput.AddonID | ||
| }); ok { | ||
| _, added, err = h.SubscriptionWorkflowService.ChangeAddonQuantity(ctx, request.SubscriptionID, subscriptionworkflow.ChangeAddonQuantityWorkflowInput{ | ||
| SubscriptionAddonID: sAdd.NamespacedID, | ||
| Quantity: request.AddonInput.InitialQuantity, | ||
| Timing: request.AddonInput.Timing, | ||
| }) | ||
| } else { | ||
| // Otherwise, we'll create it as per usual | ||
| _, added, err = h.SubscriptionWorkflowService.AddAddon(ctx, request.SubscriptionID, request.AddonInput) | ||
| } |
Contributor
There was a problem hiding this comment.
Create endpoint currently behaves as implicit upsert.
This path updates quantity when the add-on already exists, but the API contract/documentation describes a create operation. It also still responds with 201 Created for the update branch, which is pretty confusing for clients. Please either make this endpoint strictly create (e.g., conflict on existing add-on) or formally model/document upsert semantics and status codes.
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 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.
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
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
1 participant
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