feat(subscription addon): v3 assign add-on to a subscription endpoint#4392
Conversation
borosr
added
the
release-note/ignore
|
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}
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
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
Greptile SummaryThis PR adds a v3 endpoint for assigning add-ons to subscriptions. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (13): Last reviewed commit: "feat(subscription_addon): assign addon t..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| -- 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); |
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
| Annotations( | ||
| entsql.IndexWhere("deleted_at IS NULL"), | ||
| ). | ||
| Unique(), |
There was a problem hiding this comment.
is it unique? how's quantity handled?
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?
There was a problem hiding this comment.
It should not affect the quantities, because it has a separate table in the db. I tested it manually and I was able to set multiple quantities of the same addon.
borosr
force-pushed
the
feat/attach-subscription-addon-v3
branch
3 times, most recently
from
cbb2126 to
457539d
Compare
borosr
force-pushed
the
feat/attach-subscription-addon-v3
branch
from
457539d to
cbb2126
Compare
borosr
force-pushed
the
feat/attach-subscription-addon-v3
branch
2 times, most recently
from
482f5e4 to
cce8220
Compare
borosr
force-pushed
the
feat/attach-subscription-addon-v3
branch
from
cce8220 to
3ac6261
Compare
borosr
force-pushed
the
feat/attach-subscription-addon-v3
branch
from
3ac6261 to
c2c2e5e
Compare
| -- 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); |
There was a problem hiding this comment.
This migration still suppresses Atlas' data-safety warning without cleaning up or checking existing duplicates before creating the unique index. Existing deployments can already contain multiple non-deleted (namespace, subscription_id, addon_id) rows from concurrent or retried add-on creation. PostgreSQL rejects this CREATE UNIQUE INDEX in that state, so the database migration can fail during deploy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tools/migrate/migrations/20260625085650_add_subscription_addon_unique_index.up.sql
Line: 2-3
Comment:
**Duplicate rows block migration**
This migration still suppresses Atlas' data-safety warning without cleaning up or checking existing duplicates before creating the unique index. Existing deployments can already contain multiple non-deleted `(namespace, subscription_id, addon_id)` rows from concurrent or retried add-on creation. PostgreSQL rejects this `CREATE UNIQUE INDEX` in that state, so the database migration can fail during deploy.
How can I resolve this? If you propose a fix, please make it concise.
borosr
force-pushed
the
feat/attach-subscription-addon-v3
branch
from
c2c2e5e to
d58212d
Compare
| -- 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); |
There was a problem hiding this comment.
This migration still creates the unique partial index without first removing or auditing existing active duplicates. Older databases can already contain two non-deleted rows with the same namespace, subscription_id, and addon_id from concurrent add-on attaches before this index existed. When that data is present, PostgreSQL rejects the CREATE UNIQUE INDEX statement and the deploy fails. Add a deterministic cleanup or preflight check before creating the index.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tools/migrate/migrations/20260625085650_add_subscription_addon_unique_index.up.sql
Line: 2-3
Comment:
**Duplicate rows block migration**
This migration still creates the unique partial index without first removing or auditing existing active duplicates. Older databases can already contain two non-deleted rows with the same `namespace`, `subscription_id`, and `addon_id` from concurrent add-on attaches before this index existed. When that data is present, PostgreSQL rejects the `CREATE UNIQUE INDEX` statement and the deploy fails. Add a deterministic cleanup or preflight check before creating the index.
How can I resolve this? If you propose a fix, please make it concise.
borosr
force-pushed
the
feat/attach-subscription-addon-v3
branch
from
d58212d to
b3a31a7
Compare
fix: copy-paste issue feat: throw conflict error if addon is already assigned to the subscription fix: remove double conflict checks and remove unused params from the request feat: introduce rate cards and timeline into the v3 subscription addon response refactor: make subscriptionWorkflowService name consistent fix: review chore: e2e chore: update Signed-off-by: Andras Toth <4157749+tothandras@users.noreply.github.com> fix: linter issue fix: omit get view call if no items present fix: double check the constraint error if save failed
borosr
force-pushed
the
feat/attach-subscription-addon-v3
branch
from
b3a31a7 to
61c7d04
Compare
3 participants
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