feat(api): add add-on assignments #2658
Conversation
chrisgacsal
added
area/api
release-note/feature
📝 WalkthroughWalkthroughThe changes introduce new API endpoints and data models for managing plan add-ons. The OpenAPI specification now includes endpoints for listing, creating, retrieving, updating, and deleting plan add-ons, along with new schemas. In the product catalog modules, a new TypeScript file defines the Changes
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (9)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (2)
api/spec/src/productcatalog/routes.tsp (1)
166-179: Introduction ofPlanAddonsinterface and thelistmethod
- Declaring a separate
PlanAddonsinterface helps isolate plan add-on–specific functionality.- The
listmethod returningPlanAddon[]is straightforward for enumerating assigned add-ons.- Consider adding pagination or searching if the number of add-ons can grow.
api/openapi.yaml (1)
6805-6879: Review GET /api/v1/plans/{planId}/addons Endpoint Parameter Consistency
In this GET operation, theplanIdparameter uses a pattern that allows two formats (^[a-z0-9]+(?:_[a-z0-9]+)*$|^[0-7][0-9A-HJKMNP-TV-Za-hjkmnp-tv-z]{25}$). Please verify that supporting both formats here is intentional and that this dual-pattern approach is consistently reflected across the API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/openapi.yaml(2 hunks)api/spec/src/productcatalog/main.tsp(1 hunks)api/spec/src/productcatalog/planaddon.tsp(1 hunks)api/spec/src/productcatalog/routes.tsp(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Developer environment
- GitHub Check: Commit hooks
- GitHub Check: Build
- GitHub Check: Test
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🔇 Additional comments (15)
api/spec/src/productcatalog/main.tsp (1)
17-17: Seamless import to incorporate plan add-onsAdding
import "./planaddon.tsp";correctly integrates the plan add-on definitions into the main product catalog specification. No issues found.api/spec/src/productcatalog/planaddon.tsp (4)
1-4: Relevant import statementsThe imports for
@typespec/http, localtypes.tsp, andratecards.tspappear valid, ensuring accessible definitions for HTTP operations and typed references.
5-9: Consistent namespace usageUsing
namespace OpenMeter.ProductCatalog;aligns well with the existing product catalog modules. Theusingstatements for TypeSpec utilities are properly declared.
10-50: StructuredPlanAddonmodel with nestedaddonobject
- The
@friendlyName("PlanAddon")and docstrings (lines 10–12) offer clarity for external references.- Nesting the
addonobject with fields likeid,key,version, andinstanceTypecleanly encapsulates add-on attributes.- Visibility annotations (e.g.,
@visibility(Lifecycle.Read, Lifecycle.Create)) are well-chosen for partial or read-only fields.
52-66: Additional properties for PlanAddon
- The
fromPlanPhaseproperty meaningfully indicates the specific plan phase from which the add-on should become purchasable. Consider defining a stricter type if valid phases are known in advance.- The optional
maxQuantityproperty is a good addition for multi-instance add-ons. Ensure proper validation logic in higher-level components.api/spec/src/productcatalog/routes.tsp (5)
82-88: Renamed parameter tobodySwitching from
requesttobodyclarifies that this parameter represents the request body for updating a plan. This naming convention is more intuitive and consistent.
94-95: Explicit route forgetPlanmethodThe
@route("/{planId}")annotation is a clear and consistent way to specify the path parameter.
118-119: Explicit route fordeletePlanDefining
@route("/{planId}")aligns with standard RESTful deletion endpoints for identifying the resource by its ID.
180-198:createPlanAddonmethod
- Returning a 201 status with the newly created
PlanAddonis REST-consistent.- Handling
ConflictError,NotFoundError, andCommonErrorscomprehensively covers typical error states.
226-242:deletePlanAddonmethodReturning 204 upon successful deletion is in line with RESTful practices. This endpoint should cleanly remove the assignment without returning a resource body.
api/openapi.yaml (5)
6880-6961: Review POST /api/v1/plans/{planId}/addons Endpoint Parameter Restrictions
The POST operation for creating a plan add-on assignment enforces a stricter pattern (^[0-7][0-9A-HJKMNP-TV-Za-hjkmnp-tv-z]{25}$) for theplanIdparameter compared to the GET endpoint. Please confirm if this tighter validation is intentional or if the patterns should be aligned across endpoints for consistency.
7045-7122: Review GET /api/v1/plans/{planId}/addons/{planAddonId} Response Schema
The GET operation for a specific plan add-on assignment returns a schema reference to#/components/schemas/SubscriptionAddon, while the creation endpoint returns aPlanAddon. This introduces a naming inconsistency between endpoints handling what appears to be the same resource. Please verify whether usingSubscriptionAddonis intentional or if it should be unified withPlanAddonfor consistency.
7123-7198: Review DELETE /api/v1/plans/{planId}/addons/{planAddonId} Endpoint
The DELETE operation correctly defines required parameters and returns a 204 status code; no issues noted.
17812-18002: Verify New Schema Definitions for PlanAddon, PlanAddonCreate, and PlanAddonReplaceUpdate
The newly introduced schemas for managing plan add-ons are well defined with appropriate property types, required fields, and descriptive metadata. Ensure that these schemas align with the intended resource representation across the API. In particular, consider verifying that the resource naming (e.g.,PlanAddonvs.SubscriptionAddon) is consistent throughout endpoint responses and documentation.
6962-7044:Details
❓ Verification inconclusive
Inconsistency in Update Response Schema for PUT Operation
The PUT /api/v1/plans/{planId}/addons/{planAddonId} endpoint returns a 200 response with a schema reference to#/components/schemas/Plan, which is unexpected for an add-on update operation. It might be more consistent to return the updated add-on resource (e.g.,PlanAddonor another appropriate schema). Please verify whether this is intentional or requires correction.
Attention: Verify Response Schema for UpdatePlanAddon Operation
It appears that the PUT endpoint
/api/v1/plans/{planId}/addons/{planAddonId}returns a 200 response referencing the#/components/schemas/Planschema, even though the request body uses thePlanAddonReplaceUpdateschema. This mismatch suggests that the update operation might be expected to return the updated add-on resource (e.g., using aPlanAddonschema or similar). Please review whether the response schema is intentional—if the design is to return the full updated plan, confirm that this behavior is consistently documented; otherwise, consider updating the response schema to better reflect the add-on update.Items to Review:
- File:
api/openapi.yaml(Lines 6962-7044)- Request body schema:
PlanAddonReplaceUpdate- Response 200 schema reference: Currently set to
Plan
chrisgacsal
force-pushed
the
feat/productcatalog
branch
2 times, most recently
from
e8adc7f to
30a9e04
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🔭 Outside diff range comments (3)
openmeter/server/router/planaddon.go (2)
1-39: 🛠️ Refactor suggestionMissing import for JSON handling and response utilities
The implementation suggestions require JSON handling and response utilities that are not currently imported.
package router -import "net/http" +import ( + "encoding/json" + "net/http" +) // You'll also need to define a respondJSON function or import it from a utility package // func respondJSON(w http.ResponseWriter, statusCode int, data interface{}) { // w.Header().Set("Content-Type", "application/json") // w.WriteHeader(statusCode) // if data != nil { // json.NewEncoder(w).Encode(data) // } // }
1-39: 💡 Verification agent🧩 Analysis chain
Add addonHandler field initialization in Router constructor
Ensure that the
addonHandlerfield in theRouterstruct is properly initialized when the router is created.
🏁 Script executed:
#!/bin/bash # Check if there's a constructor for the Router that initializes handlers # Look for NewRouter or similar constructor functions grep -r "func New.*Router" --include="*.go" .Length of output: 380
Router constructor missing addonHandler initialization
Please update the Router's constructor (in
openmeter/server/router/router.go, inside theNewRouterfunction) to initialize theaddonHandlerfield. Currently, when creating a new Router instance, there’s no setup foraddonHandler, which can lead to runtime issues when invoking the plan add-on endpoints. For example, consider initializing it similarly to your other handlers (e.g., using a helper likeNewAddonHandler) and include appropriate configuration or dependency injections as needed.api/spec/src/productcatalog/routes.tsp (1)
2-3:⚠️ Potential issueMissing import for PlanAddon type
The new
PlanAddonsinterface uses thePlanAddontype, but there's no import for a file containing this type definition.import "../rest.tsp"; import "./subscriptionaddon.tsp"; +import "./planaddon.tsp";
♻️ Duplicate comments (2)
api/spec/src/productcatalog/routes.tsp (2)
199-212: Incorrect return type for updatePlanAddon methodThe method returns
Planinstead ofPlanAddon. This appears to be an oversight since the method is for updating plan add-ons.- ): Plan | OpenMeter.NotFoundError | OpenMeter.CommonErrors; + ): PlanAddon | OpenMeter.NotFoundError | OpenMeter.CommonErrors;
213-225: Incorrect return type for getPlanAddon methodThe method returns
SubscriptionAddonbut it's namedgetPlanAddon. This suggests a naming discrepancy.- ): SubscriptionAddon | OpenMeter.NotFoundError | OpenMeter.CommonErrors; + ): PlanAddon | OpenMeter.NotFoundError | OpenMeter.CommonErrors;
🧹 Nitpick comments (1)
api/spec/src/productcatalog/routes.tsp (1)
229-229: Incorrect documentation referenceThe documentation refers to deleting a "plan" instead of a "plan addon".
- * Once a plan is deleted it cannot be undeleted. + * Once a plan addon is deleted it cannot be undeleted.
🛑 Comments failed to post (5)
openmeter/server/router/planaddon.go (5)
21-24: 🛠️ Refactor suggestion
Implement router method to delete plan addon
This method needs implementation to handle the deletion of plan addons through the
addonHandler.func (a *Router) DeletePlanAddon(w http.ResponseWriter, r *http.Request, planId string, planAddonId string) { - //TODO implement me - panic("implement me") + // Delete the addon using the addonHandler + if err := a.addonHandler.DeletePlanAddon(r.Context(), planId, planAddonId); err != nil { + // Handle specific error types appropriately + // e.g., NotFound errors should return 404 + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + // Respond with no content for successful deletion + w.WriteHeader(http.StatusNoContent) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (a *Router) DeletePlanAddon(w http.ResponseWriter, r *http.Request, planId string, planAddonId string) { // Delete the addon using the addonHandler if err := a.addonHandler.DeletePlanAddon(r.Context(), planId, planAddonId); err != nil { // Handle specific error types appropriately // e.g., NotFound errors should return 404 http.Error(w, err.Error(), http.StatusInternalServerError) return } // Respond with no content for successful deletion w.WriteHeader(http.StatusNoContent) }
28-31: 🛠️ Refactor suggestion
Implement router method to get plan addon
This method needs implementation to retrieve a specific plan addon using the
addonHandler.func (a *Router) GetPlanAddon(w http.ResponseWriter, r *http.Request, planId string, planAddonId string) { - //TODO implement me - panic("implement me") + // Get the addon using the addonHandler + addon, err := a.addonHandler.GetPlanAddon(r.Context(), planId, planAddonId) + if err != nil { + // Handle specific error types appropriately + // e.g., NotFound errors should return 404 + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + // Respond with the retrieved addon + respondJSON(w, http.StatusOK, addon) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (a *Router) GetPlanAddon(w http.ResponseWriter, r *http.Request, planId string, planAddonId string) { // Get the addon using the addonHandler addon, err := a.addonHandler.GetPlanAddon(r.Context(), planId, planAddonId) if err != nil { // Handle specific error types appropriately // e.g., NotFound errors should return 404 http.Error(w, err.Error(), http.StatusInternalServerError) return } // Respond with the retrieved addon respondJSON(w, http.StatusOK, addon) }
14-17: 🛠️ Refactor suggestion
Implement router method to create plan addon
This method needs to be implemented to handle the creation of plan addons by using the
addonHandler.func (a *Router) CreatePlanAddon(w http.ResponseWriter, r *http.Request, planId string) { - //TODO implement me - panic("implement me") + // Parse the request body + var addonData planAddonRequest + if err := json.NewDecoder(r.Body).Decode(&addonData); err != nil { + http.Error(w, "Invalid request body", http.StatusBadRequest) + return + } + + // Create the addon using the addonHandler + addon, err := a.addonHandler.CreatePlanAddon(r.Context(), planId, addonData) + if err != nil { + // Handle specific error types appropriately + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + // Respond with the created addon + respondJSON(w, http.StatusCreated, addon) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (a *Router) CreatePlanAddon(w http.ResponseWriter, r *http.Request, planId string) { // Parse the request body var addonData planAddonRequest if err := json.NewDecoder(r.Body).Decode(&addonData); err != nil { http.Error(w, "Invalid request body", http.StatusBadRequest) return } // Create the addon using the addonHandler addon, err := a.addonHandler.CreatePlanAddon(r.Context(), planId, addonData) if err != nil { // Handle specific error types appropriately http.Error(w, err.Error(), http.StatusInternalServerError) return } // Respond with the created addon respondJSON(w, http.StatusCreated, addon) }
7-10: 🛠️ Refactor suggestion
Implement router method with addonHandler
This method has a placeholder implementation. When implementing, you should use the
addonHandlerfrom theRouterstruct to perform the actual operation.func (a *Router) ListPlanAddons(w http.ResponseWriter, r *http.Request, planId string) { - //TODO implement me - panic("implement me") + // Example implementation - adjust based on your actual addonHandler methods + addons, err := a.addonHandler.ListPlanAddons(r.Context(), planId) + if err != nil { + // Handle the error appropriately + // You might want to check for specific error types and return different status codes + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + // Respond with the list of addons + // Use appropriate helper methods to encode the response + respondJSON(w, http.StatusOK, addons) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (a *Router) ListPlanAddons(w http.ResponseWriter, r *http.Request, planId string) { // Example implementation - adjust based on your actual addonHandler methods addons, err := a.addonHandler.ListPlanAddons(r.Context(), planId) if err != nil { // Handle the error appropriately // You might want to check for specific error types and return different status codes http.Error(w, err.Error(), http.StatusInternalServerError) return } // Respond with the list of addons // Use appropriate helper methods to encode the response respondJSON(w, http.StatusOK, addons) }
35-38: 🛠️ Refactor suggestion
Implement router method to update plan addon
This method needs implementation to update an existing plan addon through the
addonHandler.func (a *Router) UpdatePlanAddon(w http.ResponseWriter, r *http.Request, planId string, planAddonId string) { - //TODO implement me - panic("implement me") + // Parse the request body + var addonData planAddonRequest + if err := json.NewDecoder(r.Body).Decode(&addonData); err != nil { + http.Error(w, "Invalid request body", http.StatusBadRequest) + return + } + + // Update the addon using the addonHandler + updatedAddon, err := a.addonHandler.UpdatePlanAddon(r.Context(), planId, planAddonId, addonData) + if err != nil { + // Handle specific error types appropriately + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + // Respond with the updated addon + respondJSON(w, http.StatusOK, updatedAddon) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (a *Router) UpdatePlanAddon(w http.ResponseWriter, r *http.Request, planId string, planAddonId string) { // Parse the request body var addonData planAddonRequest if err := json.NewDecoder(r.Body).Decode(&addonData); err != nil { http.Error(w, "Invalid request body", http.StatusBadRequest) return } // Update the addon using the addonHandler updatedAddon, err := a.addonHandler.UpdatePlanAddon(r.Context(), planId, planAddonId, addonData) if err != nil { // Handle specific error types appropriately http.Error(w, err.Error(), http.StatusInternalServerError) return } // Respond with the updated addon respondJSON(w, http.StatusOK, updatedAddon) }
chrisgacsal
force-pushed
the
feat/productcatalog
branch
from
30a9e04 to
0b935cf
Compare
chrisgacsal
force-pushed
the
feat/productcatalog
branch
from
0b935cf to
d2ad4d6
Compare
2 participants
Overview
Add
apispec for add-on assignments to manage relationship between plans adn add-ons.Summary by CodeRabbit
New Features
Enhancements