SubscriptionAddon API #2536
SubscriptionAddon API #2536
Conversation
📝 WalkthroughWalkthroughThe update introduces comprehensive support for managing subscription addons, encompassing new types, methods, and endpoints across various API definitions and client-side schemas. It includes enhancements to server interfaces, OpenAPI specifications, TypeSpec documentation, and router implementations. New operations for listing, creating, retrieving, and updating subscription addons are defined, along with associated models and error handling. Corresponding methods in the router are also provided, though they are currently unimplemented. Collectively, these changes extend the API’s functionality for subscription management. Changes
Possibly related PRs
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 ✨ Finishing Touches
🪧 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: 1
🧹 Nitpick comments (17)
api/openapi.cloud.yaml (3)
9391-9403: Review of AddonInstanceType Schema.The newly added
AddonInstanceTypeschema is correctly implemented with the enumerated values "single" and "multiple." For improved clarity, please update the description to correct the minor typo ("more then once" should read "more than once").
17200-17242: Clear Schema for SubscriptionAddonCreate.The
SubscriptionAddonCreatemodel is concise and clearly defines the required fields (name,key,activeFrom) along with their respective validation constraints. Adding example values for some of these fields (likekeyoractiveFrom) in the schema could further enhance the documentation clarity.
17243-17245: Incomplete Definition for SubscriptionAlignment.The snippet for the
SubscriptionAlignmentschema appears incomplete, showing only the declaration of the type and the start of properties without further detail. Please complete the schema with the necessary properties and validations to ensure it meets the API’s documentation and functional requirements.openmeter/server/router/subscriptionaddon.go (3)
7-11: Parameter name inconsistency with route definition.The route comment mentions
/api/v1/subscriptions/{subscriptionId}/addons/{addonId}in other functions, but the parameters are named differently in the methods. Consider consistent naming across routes and parameters.
21-23: Parameter name inconsistency in GetSubscriptionAddon.The comment indicates
{addonId}in the route, but the parameter is namedsubscriptionAddonId. Consider renaming for consistency:- func (a *Router) GetSubscriptionAddon(w http.ResponseWriter, r *http.Request, subscriptionId string, subscriptionAddonId string) { + func (a *Router) GetSubscriptionAddon(w http.ResponseWriter, r *http.Request, subscriptionId string, addonId string) {
27-29: Parameter name inconsistency in CancelSubscriptionAddon.Similar to GetSubscriptionAddon, there's a name mismatch between the route parameter and method parameter:
- func (a *Router) CancelSubscriptionAddon(w http.ResponseWriter, r *http.Request, subscriptionId string, subscriptionAddonId string) { + func (a *Router) CancelSubscriptionAddon(w http.ResponseWriter, r *http.Request, subscriptionId string, addonId string) {api/spec/src/productcatalog/routes.tsp (2)
474-474: Consider addressing the pagination comment.There's a TODO comment asking whether pagination should be implemented. For consistency with other list endpoints in the API, consider implementing pagination, especially if the number of addons per subscription could grow over time.
- @path subscriptionId: ULID, // Should we implement pagination? This should never be a large list. + @path subscriptionId: ULID, + ...OpenMeter.QueryPagination,
486-487: Parameter name inconsistency with router implementation.The TypeSpec defines the parameter as
subscriptionAddonIdwhile the Go router implementation usesaddonId. Consider standardizing on one name across the codebase.Choose either:
- Change the TypeSpec to match the Go implementation:
- @path subscriptionAddonId: ULID, + @path addonId: ULID,
- Change the Go implementation to match the TypeSpec (as suggested in previous comments).
api/openapi.yaml (3)
8211-8366: Endpoints for Creating and Listing Subscription Addons
The POST and GET methods under/api/v1/subscriptions/{subscriptionId}/addonsare defined with detailed parameter specifications, response codes, and error-handling schemas. A couple of suggestions:
- The POST endpoint’s description ("Create a new subscription addon, either providing the key or the id of the addon") could benefit from clarifying whether the API expects one of these identifiers or if both are acceptable.
- Ensure that the regex pattern used for
subscriptionIdis consistent with other parts of the API for uniformity.- The inclusion of
x-internal: trueis noted; please verify that this internal designation is properly communicated to the consumers of your API.
8445-8531: Cancellation of Subscription Addon
The cancellation endpoint at/api/v1/subscriptions/{subscriptionId}/addons/{subscriptionAddonId}/canceluses POST and defines a request body containing atimingproperty (viaSubscriptionTiming). Consider the following:
- Clarify in the endpoint description that if
timingis omitted, the cancellation is immediate.- Optionally, consider renaming the property (e.g., to
cancelTiming) to more clearly communicate its purpose.
18172-18214: SubscriptionAddonCreate Schema Enhancements
TheSubscriptionAddonCreateschema is well structured for resource creation, mandatingname,key, andactiveFrom. To further improve the documentation and usability of this schema, consider adding example values for these properties.api/client/javascript/src/client/schemas.ts (2)
1222-1222: Consider mentioning a recommended alternative in the deprecation notice.
Marking this plan-related operation as deprecated is clear. However, it's good practice to specify what consumers should use instead (e.g., a preferred endpoint or method) to ensure a smooth transition./** * @deprecated + * @see Use createNewPlanDraft instead * @description Create a new draft version from plan. */
1851-1856: Fix grammar in theAddonInstanceTypedescription.
Use "more than once" instead of "more then once."- * Single instance addons can be added to subscription only once while addons with multiple type can be added more then once. + * Single instance addons can be added to a subscription only once, whereas addons with the "multiple" type can be added more than once.api/api.gen.go (4)
6772-6776: Clarify default cancellation timing
The fieldTiminginCancelSubscriptionAddonJSONBodyis optional. Make sure you define a clear default behavior ifTimingis omitted (e.g., immediate vs. end-of-period).Want me to open a new issue or provide sample code to handle default cancellation timing?
10379-10401: Implement or track these unimplemented stubs
The placeholder methods inUnimplementedpresently returnhttp.StatusNotImplemented. Be sure to either implement them or track them as future tasks to ensure the new functionalities are complete.Would you like me to open a new issue to track the required implementation for these methods?
15097-15108: Add route documentation for edge cases
These new route definitions look correct. Consider documenting possible responses like “404 Not Found” if the subscription doesn't exist or “400 Bad Request” for malformed requests, to guide clients effectively.
15772-16059: Consider separating large embedded Swagger data
This extensive block likely represents embedded Swagger documentation. Large base64 blocks can be cumbersome to maintain in version control. Consider referencing an external file or adopting a more modular approach to keep the repository size manageable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/api.gen.go(8 hunks)api/client/javascript/src/client/schemas.ts(7 hunks)api/openapi.cloud.yaml(3 hunks)api/openapi.yaml(2 hunks)api/spec/src/cloud/main.tsp(1 hunks)api/spec/src/productcatalog/routes.tsp(2 hunks)api/spec/src/productcatalog/subscriptionaddon.tsp(1 hunks)openmeter/server/router/subscriptionaddon.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
api/api.gen.go (4)
openmeter/ent/db/billinginvoice/where.go (2)
Description(220-222)UpdatedAt(83-85)openmeter/productcatalog/ratecard.go (2)
RateCards(323-323)RateCard(28-38)openmeter/productcatalog/subscription/plan.go (1)
RateCard(130-133)openmeter/productcatalog/subscription/http/handler.go (1)
Handler(17-27)
🔇 Additional comments (27)
api/spec/src/cloud/main.tsp (1)
218-221: Properly implemented SubscriptionAddons interfaceThe addition of the SubscriptionAddons interface follows the established pattern in the codebase. The interface correctly extends the base OpenMeter.ProductCatalog.SubscriptionAddons interface and is properly annotated with the appropriate route and tag.
api/openapi.cloud.yaml (2)
7898-8224: Comprehensive and Detailed API Endpoint Definitions.The new endpoints for managing subscription addons (creation, listing, retrieval, and cancellation) are defined thoroughly with clear operations, parameters, and response specifications. The consistent use of error response schemas (e.g., BadRequestProblemResponse, UnauthorizedProblemResponse, etc.) and the inclusion of the custom regex patterns for identifiers contribute to a robust API design. Please verify that the
x-internal: trueflag and the overall integration with middleware meet your internal access expectations.
17087-17198: Robust Definition for SubscriptionAddon.The
SubscriptionAddonschema is well designed and includes all necessary properties such asid,name, timestamps, and a clear set of validations (including regex patterns for IDs and the use of enumerations via$ref: '#/components/schemas/AddonInstanceType'). The read-only flags on fields likeid,createdAt, and others ensure proper immutability for client responses. Please double-check that these constraints align with your backend validation logic.api/spec/src/productcatalog/subscriptionaddon.tsp (5)
1-4: Imports look good!The imports include the necessary TypeSpec HTTP utilities, common types, and the ratecards module which will be used in the SubscriptionAddon model.
13-17: Model definition correctly inherits from base resources.The SubscriptionAddon model properly extends UniqueResource and CadencedResource, which is consistent with other entity models in the system.
18-24: Version property implementation looks good.The version property correctly includes documentation, summary, validation constraints, and visibility settings. The default value of 1 is appropriate for new resources.
47-53: RateCards implementation looks good.The rateCards array property correctly references the RateCard type from the imported ratecards.tsp file with appropriate visibility and summary annotations.
38-38:Details
✅ Verification successful
Verify AddonInstanceType is defined.
The property uses AddonInstanceType which isn't imported directly in this file.
🏁 Script executed:
#!/bin/bash # Look for definition of AddonInstanceType grep -r "AddonInstanceType" --include="*.tsp" .Length of output: 349
AddonInstanceType is correctly defined and available.
- The grep output shows that
AddonInstanceTypeis defined in./api/spec/src/productcatalog/addon.tsp(as an enum).- Its usage in
subscriptionaddon.tspis valid as it references the type defined elsewhere, which means no direct import is required in this file.api/spec/src/productcatalog/routes.tsp (6)
2-2: Import statement correctly added.The import for the subscriptionaddon.tsp file has been properly added to make the SubscriptionAddon type available in this file.
441-443: Route definition and tag look good.The route path
/api/v1/subscriptions/{subscriptionId}/addonsfollows the RESTful convention and is appropriately tagged with "Subscriptions".
447-464: Create operation looks good.The create operation correctly defines the necessary parameters, request body type, and response types including appropriate error responses.
468-475: List operation implementation looks good.The list operation for subscription addons is properly defined, but consider implementing pagination as noted in a previous comment.
492-507: Cancel operation implementation looks good.The cancel operation correctly includes the timing option for scheduled cancellation, similar to the subscription cancellation endpoint.
451-451:Details
❓ Verification inconclusive
Consider if these endpoints should be internal.
All subscription addon operations are marked with
@extension("x-internal", true). Verify if this is intentional or if some endpoints should be accessible externally.Also applies to: 472-472, 484-484, 497-497
🏁 Script executed:
#!/bin/bash # Check if other subscription-related endpoints are also marked as internal grep -A 3 '@operationId(".*Subscription' --include="*.tsp" ./api/spec/src | grep -B 2 '@extension("x-internal"'Length of output: 150
Below is a new script to re-run the verification without causing directory errors. It searches within all “*.tsp” files under the spec directory for both subscription endpoints and the internal marker:
Action Required: Verify Endpoint Exposure
Please confirm manually whether the subscription endpoints in
api/spec/src/productcatalog/routes.tsp—specifically at lines 451, 472, 484, and 497—are intentionally left without the@extension("x-internal", true)annotation. All other subscription addon endpoints appear to be marked as internal. If the omission is unintentional, the endpoints should be updated accordingly.
🏁 Script executed:
#!/bin/bash echo "=== Subscription Endpoint Declarations (OperationId) ===" find ./api/spec/src -type f -name "*.tsp" -exec grep -H '@operationId(".*Subscription' {} \; echo "" echo "=== Endpoints Marked as Internal (x-internal) ===" find ./api/spec/src -type f -name "*.tsp" -exec grep -H '@extension("x-internal", true)' {} \;Length of output: 2448
Action: Please confirm the internal/external intent for these endpoints
Our latest verification shows that the file
api/spec/src/productcatalog/routes.tspcontains multiple occurrences of@extension("x-internal", true)alongside subscription endpoint definitions (e.g.,@operationId("createSubscription"), etc.). However, the snippet at line 451 (and similarly at lines 472, 484, and 497) only shows the call tocreate(without the surrounding context, so it isn’t entirely clear from this diff snippet whether those endpoints are already annotated as internal or not.Please manually verify that the endpoints at these lines are intended to be marked internal. In particular:
- Confirm if the internal annotation is applied directly to the endpoints at lines 451, 472, 484, and 497.
- If some endpoints are meant to be externally accessible, adjust the annotations accordingly.
api/openapi.yaml (4)
8367-8444: Get Subscription Addon Endpoint Validation
The GET endpoint at/api/v1/subscriptions/{subscriptionId}/addons/{subscriptionAddonId}is well-documented with clear parameters and comprehensive error responses. Please verify that the referenced schema (SubscriptionAddon) accurately reflects the data structure returned by your implementation.
8533-8535: Unrelated Subscription Cancellation Endpoint
The endpoint/api/v1/subscriptions/{subscriptionId}/cancelappears immediately after the addon endpoints. Given that this PR focuses on SubscriptionAddon API enhancements, please verify that including or modifying this subscription cancellation endpoint was intentional.
18059-18171: SubscriptionAddon Schema Definition
TheSubscriptionAddonschema is defined with appropriate fields, including identifiers, timestamps (all marked as readOnly where expected), and required properties. The regex pattern to validate identifiers is consistent with other API entities. Please double-check that the reference ininstanceTypetoAddonInstanceTypecorrectly links to the intended schema elsewhere in your specification.
18215-18217: Review of SubscriptionAlignment Schema Declaration
The snippet forSubscriptionAlignmentappears at the tail end of the hunk. Since this schema isn’t directly tied to the SubscriptionAddon API enhancements, please confirm whether its inclusion here is deliberate and that it is complete as per your design requirements.api/client/javascript/src/client/schemas.ts (5)
7416-7541: Comprehensive documentation forSubscriptionAddonandSubscriptionAddonCreate.
These schemas provide thorough coverage of fields and descriptions. The use of read-only for IDs and timestamps is appropriate, and optional fields are clearly marked. The naming is consistent and self-explanatory.
8386-8386: Re-export ofAddonInstanceTypelooks good.
The type re-export provides convenience and clarity for consumers in other modules. No issues found.
8794-8796: Re-export ofSubscriptionAddonandSubscriptionAddonCreate.
Exposing these newly introduced types is logical and consistent with existing patterns for re-exports.
19146-19543:Details
✅ Verification successful
Well-defined operations for subscription addon management.
The endpoints for listing, creating, retrieving, and canceling subscription addons match the project's established status code conventions and error structures. The optionaltimingfield for cancellation is an excellent addition for specifying when an addon is canceled.Consider verifying the usage and tests of the
timingproperty:
🏁 Script executed:
#!/bin/bash # Description: Find references to the 'timing' property in the codebase to confirm proper handling and testing. rg -A 5 "timing"Length of output: 52757
Subscription addon operations and timing integration verified
The usage of thetimingproperty has been thoroughly validated across the codebase. Extensive references in the business logic, API endpoints, client schemas, and tests (notably in files such asopenmeter/subscription/timing.go,openmeter/subscription/service.go, and various test suites) confirm that both immediate cancellations and next‐billing-cycle scenarios are handled correctly. The integration within the JavaScript and Go client libraries also demonstrates consistency with our established status code conventions and error structures.No further changes are required.
1658-1721:Details
❓ Verification inconclusive
Verify path parameters for
/api/v1/subscriptions/{subscriptionId}/addons.
The interface currently setspath?: never, yet the endpoint includes{subscriptionId}. Please confirm the path parameter is correctly handled elsewhere in the code or update the interface to include the subscription ID explicitly.
🏁 Script executed:
#!/bin/bash # Description: Search for references to path parameters for the '/api/v1/subscriptions/{subscriptionId}/addons' endpoint. rg -A 8 "/api/v1/subscriptions/\{subscriptionId\}/addons"Length of output: 12383
Action Required: Verify the handling of the subscriptionId path parameter for
/api/v1/subscriptions/{subscriptionId}/addons.The OpenAPI specifications (in both
api/openapi.yamlandapi/openapi.cloud.yaml) and the Go router (e.g. inopenmeter/server/router/subscriptionaddon.goandapi/api.gen.go) correctly require asubscriptionIdin the URL. However, in the TypeScript client schemas (api/client/javascript/src/client/schemas.ts), theparametersfield for this endpoint (and similar ones) is set withpath?: never. Please confirm whether this behavior is intentional—perhaps relying on an external mechanism to inject the path parameter—or if the interface should be updated to explicitly include the requiredsubscriptionId.
- Files to review:
api/client/javascript/src/client/schemas.ts(endpoints for/api/v1/subscriptions/{subscriptionId}/addons, etc.)api/openapi.yamlandapi/openapi.cloud.yaml(which define the path parameter)- Relevant Go router files in
openmeter/server/router/and generated API code inapi/api.gen.goapi/api.gen.go (4)
5143-5210: Validate and ensure proper handling of user-provided fields
The structsSubscriptionAddonandSubscriptionAddonCreatecontain several user-provided fields (e.g.,Name,Key,Description). Consider verifying they meet the documented constraints (length, optionality, etc.) at runtime to prevent invalid or malicious input from being stored or processed.Would you like me to generate a shell script that searches for validation checks of these structs across the codebase?
6943-6948: New type aliases look consistent
These aliases align with the new entities (SubscriptionAddonCreateandCancelSubscriptionAddonJSONBody) and make the codebase more readable.
9689-9700: Consider elaborating on error handling
The new interface methods (ListSubscriptionAddons,CreateSubscriptionAddon, etc.) do not specify how errors like “subscription not found” or “access denied” should be handled. Consider returning distinct status codes or error responses for better clarity.Could you confirm if these methods are handled upstream or if they need specific error logic?
14323-14439: Middleware logic appears correct
The route middleware functions properly map path parameters and pass them along to the handler. This is an appropriate approach for binding path variables and ensures consistent error handling.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (14)
api/openapi.cloud.yaml (3)
7898-7983: Ensure Consistency & Reusability in the Create Subscription Addon EndpointThe POST endpoint for creating a subscription addon is thorough with well‐defined parameters, responses, and error handling. One recommendation is to consider abstracting the common parameter schema (e.g. the subscriptionId with its regex pattern) into a reusable component. This will help reduce duplication and promote consistency across endpoints.
9394-9401: Correct the Grammar in the AddonInstanceType SchemaThe new
AddonInstanceTypeschema correctly defines an enum for “single” and “multiple.” Please update the description to fix the grammatical error—change “more then once” to “more than once.”
Suggested diff:- Single instance addons can be added to subscription only once while addons with multiple type can be added more then once. + Single instance addons can be added to a subscription only once while addons with multiple types can be added more than once.
17087-17243: Review the SubscriptionAddon and SubscriptionAddonCreate SchemasThe definitions for
SubscriptionAddonandSubscriptionAddonCreateare comprehensive and provide detailed validation rules, required fields, and examples. A few points to consider:
- Ensure that the regex patterns used for identifiers (for id, addonId, and subscriptionId) are consistent across your API.
- In the
SubscriptionAddonCreatemodel, while the required properties (name, key, and activeFrom) are well defined, consider adding example values to enhance clarity for API consumers.- Verify that the descriptions and titles precisely communicate the intended usage (for instance, confirm that “A subscription addon, a concrete instance of an addon” aligns with your domain terminology).
Overall, these schema additions look solid and well integrated into the API specification.
openmeter/server/router/subscriptionaddon.go (2)
7-11: Placeholder implementation for listing subscription addons.This is a good start with the NotImplemented status. Consider adding a TODO comment to indicate future implementation plans.
// List subscription addons // (GET /api/v1/subscriptions/{subscriptionId}/addons) func (a *Router) ListSubscriptionAddons(w http.ResponseWriter, r *http.Request, subscriptionId string) { + // TODO: Implement ListSubscriptionAddons endpoint w.WriteHeader(http.StatusNotImplemented) }
13-17: Placeholder implementation for creating subscription addons.This is a good start with the NotImplemented status. Consider adding a TODO comment to indicate future implementation plans.
// Create a subscription addon // (POST /api/v1/subscriptions/{subscriptionId}/addons) func (a *Router) CreateSubscriptionAddon(w http.ResponseWriter, r *http.Request, subscriptionId string) { + // TODO: Implement CreateSubscriptionAddon endpoint w.WriteHeader(http.StatusNotImplemented) }api/openapi.yaml (4)
8211-8293: Clarify and Enhance the POST Subscription Addon Creation Endpoint
The newly introduced POST endpoint for creating a subscription addon is very detailed, with a comprehensive list of response codes and proper parameter validation (e.g., the strict pattern forsubscriptionId). To further improve clarity:
- Consider expanding the description to detail when to provide the addon "key" versus the addon "id".
- Optionally, add example values in the requestBody section to illustrate typical payloads.
Also, verify that the use ofx-internal: trueis intentional and well-documented for consumers of this API.
8294-8365: Solid GET Endpoint for Listing Subscription Addons
The GET operation for listing subscription addons is implemented with consistent parameter validation and error handling.
- It might be useful to include examples or notes on what an empty list response looks like to better guide API consumers.
Otherwise, the design and response structure look good.
8444-8533: Review the Cancel Subscription Addon Endpoint
The cancellation endpoint is clearly defined and includes a requestBody with atimingfield (referencing theSubscriptionTimingschema). Consider the following:
- Clarify in the description whether the
timingattribute is optional and, if omitted, confirm that the addon cancellation is processed immediately.- Double-check that the error response codes align with your business logic for cancellation scenarios.
18056-18216: Review and Refine SubscriptionAddon Schemas
The new schemasSubscriptionAddonandSubscriptionAddonCreateare comprehensive and cover all essential fields. A couple of suggestions:
- Ensure consistent title capitalization and naming conventions (e.g., the title for
addonIdis given as "AddonID" whilesubscriptionIdis "SubscriptionID"; consider making these stylistically uniform).- It may be beneficial to add example values in the
SubscriptionAddonCreateschema, similar to those provided in theSubscriptionAddonschema, to aid consumers in constructing valid request bodies.api/client/javascript/src/client/schemas.ts (1)
1851-1856: Fix minor grammatical issue in documentation.
The sentence "addons with multiple type can be added more then once" contains small errors. Consider updating it to:"Addons with type 'multiple' can be added more than once."
api/spec/src/productcatalog/routes.tsp (2)
473-474: Consider implementing pagination for future scalabilityThe comment raises a valid question about pagination. While the list may currently be small, implementing pagination would provide consistency with other list operations in the codebase (like in the Plans and Addons interfaces) and ensure future scalability.
Consider updating the list method to match the pattern used in other list operations:
list( @path subscriptionId: ULID, + ...OpenMeter.QueryPagination, + ...OpenMeter.QueryOrdering<SubscriptionAddonOrderBy>, -): SubscriptionAddon[] | OpenMeter.NotFoundError | OpenMeter.CommonErrors; +): OpenMeter.PaginatedResponse<SubscriptionAddon> | OpenMeter.NotFoundError | OpenMeter.CommonErrors;This would require defining a
SubscriptionAddonOrderBytype if it doesn't already exist.
441-507: Consider adding an update operation for SubscriptionAddonsThe interface currently provides create, list, get, and cancel operations, but lacks an update operation. Other resource interfaces in the file (Plans, Addons) include update operations. If subscription addons have properties that may need modification after creation, consider adding an update operation.
api/api.gen.go (2)
6943-6947: Ensure distinct request bodies fit future extension needs.
CreateSubscriptionAddonJSONRequestBodyandCancelSubscriptionAddonJSONRequestBodyare aliased directly toSubscriptionAddonCreateandCancelSubscriptionAddonJSONBody, respectively. This is fine for now but could limit future modifications if these request bodies must diverge from the underlying model.
10379-10401: Track unimplemented handlers to avoid forgotten endpoints.The unimplemented handlers in
Unimplementedcurrently returnhttp.StatusNotImplemented. Ensure this is tracked in your backlog or future PRs so these stubs aren’t left unimplemented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/api.gen.go(8 hunks)api/client/javascript/src/client/schemas.ts(7 hunks)api/openapi.cloud.yaml(3 hunks)api/openapi.yaml(2 hunks)api/spec/src/cloud/main.tsp(1 hunks)api/spec/src/productcatalog/routes.tsp(2 hunks)api/spec/src/productcatalog/subscriptionaddon.tsp(1 hunks)openmeter/server/router/subscriptionaddon.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
api/api.gen.go (7)
openmeter/ent/db/entitlement/where.go (5)
ActiveFrom(95-97)ActiveTo(100-102)CreatedAt(75-77)DeletedAt(85-87)UpdatedAt(80-82)openmeter/ent/db/subscription/where.go (7)
ActiveFrom(90-92)ActiveTo(95-97)CreatedAt(75-77)DeletedAt(85-87)Description(110-112)Name(105-107)UpdatedAt(80-82)api/client/go/client.gen.go (8)
CreatedAt(470-470)Key(472-472)Metadata(3441-3441)RateCard(4247-4249)UpdatedAt(473-473)SubscriptionTiming(5036-5038)SubscriptionAddonCreate(4765-4783)CancelSubscriptionAddonJSONBody(6230-6233)openmeter/billing/profile.go (1)
Metadata(22-22)openmeter/productcatalog/ratecard.go (2)
RateCards(323-323)RateCard(28-38)openmeter/productcatalog/subscription/plan.go (1)
RateCard(130-133)openmeter/productcatalog/subscription/http/handler.go (1)
Handler(17-27)
🔇 Additional comments (30)
api/openapi.cloud.yaml (4)
7984-8056: Review of the List Subscription Addons EndpointThe GET endpoint for listing subscription addons is implemented correctly with proper response definitions and parameter validation. The error responses and use of the x‑internal flag are consistent with the overall API design.
8057-8134: Validate the Get Subscription Addon EndpointThis GET endpoint, which retrieves a subscription addon by its ID, reuses the subscriptionId and subscriptionAddonId patterns correctly. Please verify that the response schema reference (
#/components/schemas/SubscriptionAddon) is defined and kept in sync with the underlying model.
8135-8223: Assess the Cancel Subscription Addon Endpoint’s RequestBodyThe POST endpoint for canceling a subscription addon is well structured. The inclusion of a request body with a “timing” property (referencing
SubscriptionTiming) provides useful flexibility for immediate versus scheduled cancellations. Verify that the business rules for cancellation (e.g. defaulting to immediate cancellation if “timing” is omitted) are clearly documented in the consumer-facing docs.
8224-8225: Clarify the Cancel Subscription Endpoint ContextAt the tail end of this hunk, an endpoint
/api/v1/subscriptions/{subscriptionId}/cancelis introduced. Confirm that its separation from the SubscriptionAddon-related endpoints is intentional and that its response handling remains consistent with your service’s cancellation flows.api/spec/src/cloud/main.tsp (1)
217-221: Well-structured API interface addition.The new subscription addons interface follows the established pattern in this codebase, correctly extending the base interface and using appropriate routing and tagging.
api/spec/src/productcatalog/subscriptionaddon.tsp (7)
1-9: LGTM! Appropriate imports for the subscription addon model.The imports correctly include the necessary typespec modules and related type definitions.
10-17: Clear model definition with proper inheritance.The model appropriately extends
UniqueResourceandglobal.CadencedResourceto inherit common properties.
18-24: Appropriate version property definition.The version property is well-defined with proper annotations and constraints.
26-32: Well-documented addonId property.The addonId property is correctly typed as ULID with appropriate visibility and summary annotations.
40-46: Well-defined subscriptionId property.The subscriptionId property is properly typed as ULID with appropriate visibility settings.
47-53: Clear rateCards array property.The rateCards property is well-defined as an array of RateCard objects with proper visibility settings.
33-39:Details
✅ Verification successful
Verify AddonInstanceType enumeration.
The instanceType property references
AddonInstanceTypewhich should be defined somewhere in the imported files. Ensure it's properly defined and includes the "single" and "multiple" values mentioned in the comments.
🏁 Script executed:
#!/bin/bash # Check if AddonInstanceType is defined rg --type=typescript "AddonInstanceType" api/spec/src/Length of output: 89
I'm updating the verification by re-running the search without the unsupported file type flag. Please run the following shell script to search for both an enum and a type alias declaration for
AddonInstanceTypewithin theapi/spec/src/directory:This should help verify whether
AddonInstanceTypeis defined correctly to include the "single" and "multiple" values as expected.
🏁 Script executed:
#!/bin/bash echo "Searching for enum definition of AddonInstanceType:" rg "enum\s+AddonInstanceType" api/spec/src/ echo "" echo "Searching for type alias definition of AddonInstanceType:" rg "type\s+AddonInstanceType" api/spec/src/Length of output: 403
I'm updating the verification by extracting the full definition of the
AddonInstanceTypeenum. Please run the following script to display all lines from the enum in order to verify that it includes the"single"and"multiple"members:
🏁 Script executed:
#!/bin/bash echo "Extracting enum details for AddonInstanceType from api/spec/src/productcatalog/addon.tsp:" sed -n '/enum\s\+AddonInstanceType/,/}/p' api/spec/src/productcatalog/addon.tspLength of output: 341
AddonInstanceType Enum Verified
The
AddonInstanceTypeenum is correctly defined inapi/spec/src/productcatalog/addon.tspand includes both"single"and"multiple"values as expected. The implementation insubscriptionaddon.tspaccurately references this enum, so no changes are required.openmeter/server/router/subscriptionaddon.go (1)
1-6: LGTM! Simple and appropriate package and imports.The package and imports are correctly defined for the router implementation.
api/openapi.yaml (2)
8366-8443: Validate GET Endpoint for Retrieving a Specific Subscription Addon
The endpoint to retrieve a single subscription addon by its ID is consistent with the others.
- Ensure that the path parameters
subscriptionIdandsubscriptionAddonIdfollow the exact same pattern and examples as used in the creation and listing endpoints.
This helps maintain consistency across the API.
8533-8535: Clarify Role of the Cancel Subscription Endpoint
Just after defining the add-on cancellation endpoint, there is an existing endpoint for/api/v1/subscriptions/{subscriptionId}/cancel.
- Confirm that this endpoint is meant to coexist with the new SubscriptionAddon endpoints, and that there is no risk of routing conflicts.
- If these functionalities differ, consider adding documentation or renaming to clearly distinguish between canceling a subscription addon and canceling an entire subscription.
api/client/javascript/src/client/schemas.ts (6)
1222-1225: Clarify and finalize deprecation usage.
The doc comment indicates that this endpoint is now deprecated. Please verify whether all references to this endpoint have been removed or updated to reflect any newer alternatives.Would you like me to search the codebase for references to this endpoint to ensure it’s no longer used?
1658-1721: Endpoints for managing subscription addons look consistent.
All newly introduced routes follow a sensible resource-based design and align well with standard REST conventions. Great job!
7416-7540: Confirm correct usage of Date object fields.
These fields (activeFrom,activeTo, etc.) are typed asDate, but JSON responses typically return them as strings. Please confirm that the client either parses them intoDateobjects or that the schema accurately reflects any string-based date fields.
8386-8386: Export of AddonInstanceType is properly handled.
Re-exporting this type is clean and straightforward. No concerns.
8794-8796: New SubscriptionAddon and SubscriptionAddonCreate exports match the schemas.
Both types cohesively mirror the underlying schema definitions. Looks good!
19146-19543: Comprehensive operations for subscription addon lifecycle.
The newly added operations (list, create, get, and cancel) provide thorough coverage, including detailed error responses. This is well-structured and consistent. Great work!api/spec/src/productcatalog/routes.tsp (3)
441-507: Well-structured implementation for the SubscriptionAddons interfaceThe new SubscriptionAddons interface follows the established pattern of the codebase and implements CRUD operations (create, list, get, cancel) with clear documentation and appropriate routing. The operations include proper error handling with relevant error types.
450-451: Confirm internal-only API designationAll operations in the SubscriptionAddons interface are marked with
@extension("x-internal", true), indicating they are internal APIs. Ensure this is intentional and aligns with the overall API design strategy.Please confirm if these endpoints should remain internal-only or if they should be exposed to external API consumers.
Also applies to: 471-472, 483-484, 496-497
2-2:Details
❓ Verification inconclusive
Confirm imported file contains necessary types
The file imports
subscriptionaddon.tspwhich presumably contains the SubscriptionAddon and related types. Ensure this file is properly implemented with all required types referenced in this interface.
🏁 Script executed:
#!/bin/bash # Check if the imported file exists and contains necessary type definitions cat api/spec/src/productcatalog/subscriptionaddon.tsp 2>/dev/null | grep -E "SubscriptionAddon|SubscriptionTiming"Length of output: 165
Action Required: Confirm Complete Type Definitions in Imported File
The
subscriptionaddon.tspfile clearly defines theSubscriptionAddonmodel as expected:@friendlyName("SubscriptionAddon") model SubscriptionAddon {However, the verification did not reveal any definition for
SubscriptionTiming. Please verify ifSubscriptionTimingis required for the interface. If it is needed, kindly updatesubscriptionaddon.tsp(or the appropriate file) to include its definition; otherwise, ensure that references toSubscriptionTimingare removed or correctly redirected.api/api.gen.go (6)
5143-5210: Add field validation checks to ensure data consistency.The new
SubscriptionAddonandSubscriptionAddonCreatestructs define several fields—e.g.,ActiveFrom,ActiveTo,RateCards, etc.—without validation. Consider enforcing stricter validation to ensure required fields are present (likeKeyandName), optional fields are used properly, and that time ranges are correct (e.g.,ActiveToshould be afterActiveFrom). You may also wish to confirm whetherKeymust be globally unique.
6772-6776: Confirm that cancellation timing logic is correctly implemented.
CancelSubscriptionAddonJSONBodyintroduces an optionalTiming. Ensure the server-side logic properly interprets null (immediate cancellation) vs. future cancellation. It’s easy to overlook corner cases here, so please verify that this field’s logic is tested.
9689-9700: Add further documentation or examples for new API methods.Methods
ListSubscriptionAddons,CreateSubscriptionAddon,GetSubscriptionAddon, andCancelSubscriptionAddonappear in the interface but lack implementation detail here. Consider adding doc comments or usage examples for clarity, especially if external users depend on these endpoints.
14323-14440: Operation middleware appears consistent with existing patterns.Your new middleware for subscription addons aligns with typical parameter extraction and error handling patterns. This looks good. Just confirm logging, rate-limiting, or authentication middlewares (if applicable) attach here as needed.
15097-15108: New routes successfully added.These routes for listing, creating, retrieving, and canceling subscription addons are consistently named and appear to follow the established RESTful style.
15572-16059: Embedded swagger specification chunk.This large embedded section appears to be generated content for the Swagger/OpenAPI specification. No action is required unless you need to confirm that the generated spec is up-to-date and correct.
GAlexIHU
force-pushed
the
galexi/addons-api
branch
from
aecab87 to
73bbe6d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (15)
api/spec/src/productcatalog/subscriptionaddon.tsp (2)
37-42: Document the AddonInstanceType enum valuesThe
instanceTypeproperty refers toAddonInstanceTypeenum, but the enum values aren't documented here. For better clarity, consider adding documentation about the possible values and their implications.Either add an enum definition or improve the documentation:
/** - * The instanceType of the addon. Can be "single" or "multiple". + * The instanceType of the addon. + * - "single": Only one instance allowed per subscription, quantity must be 1 + * - "multiple": Multiple instances allowed, quantity can be > 1 */ @visibility(Lifecycle.Read) @summary("InstanceType") instanceType: AddonInstanceType;
63-76: Add identifiers to SubscriptionAddonRateCard modelThe
SubscriptionAddonRateCardmodel doesn't inherit fromUniqueResourceunlike the parentSubscriptionAddonmodel. Consider adding a unique identifier or explaining why this model doesn't need one.@friendlyName("SubscriptionAddonRateCard") model SubscriptionAddonRateCard { + /** + * Unique identifier for the subscription addon rate card. + */ + @summary("ID") + @visibility(Lifecycle.Read) + id: ULID; + /** * The rate card. */ @summary("Rate card") rateCard: RateCard; // Rest of the properties... }api/openapi.cloud.yaml (4)
8602-8684: Ensure Correct Definition for the Create SubscriptionAddon EndpointThe POST operation for creating a subscription addon is well detailed with explicit parameter definitions, error responses, and a clearly referenced request body schema. Please verify that:
- The ULID pattern for the
subscriptionId(and similar fields elsewhere) is consistent with your backend validations.- The use of
x-internal: truealigns with your intended API exposure.
8685-8756: Review the List SubscriptionAddons EndpointThe GET operation for listing subscription addons correctly returns an array of
SubscriptionAddonobjects and covers multiple error cases. Ensure that:
- The error responses and parameter validations (including the ULID pattern) are consistent with other endpoints.
- The
x-internal: trueflag is applied intentionally as this endpoint might be hidden from public docs.
8835-8923: Check the Cancel SubscriptionAddon EndpointThe cancellation endpoint includes a request body with a nested
timingschema. Please consider:
- Adding an example for the
timingparameter to improve clarity for API consumers.- Verifying that the cancellation logic (which depends on this timing detail) correctly interprets the passed parameters.
18197-18390: Review Schema Definitions for SubscriptionAddon EntitiesThe newly introduced schemas—SubscriptionAddon, SubscriptionAddonCreate, and SubscriptionAddonRateCard—are well-structured and include comprehensive validations and descriptions. Notes:
- The required properties, regex patterns (for ULIDs), and readOnly flags are consistently applied.
- Consider including additional examples (or expanding the existing ones) for some fields (e.g.,
quantityoractiveTo) to further aid API users.api/spec/src/productcatalog/routes.tsp (3)
484-485: Consider addressing the pagination question in the code commentThe comment about pagination implementation raises a valid question. Even if the list will typically be small, implementing pagination from the start provides scalability and consistency with other collection endpoints.
Consider adding pagination support consistent with other list endpoints:
list( @path subscriptionId: ULID, // Should we implement pagination? This should never be a large list. - ): SubscriptionAddon[] | OpenMeter.NotFoundError | OpenMeter.CommonErrors; + + ...OpenMeter.QueryPagination, + ): OpenMeter.PaginatedResponse<SubscriptionAddon> | OpenMeter.NotFoundError | OpenMeter.CommonErrors;
452-454: Consider adding filtering capabilities to the list endpointThe list operation could benefit from filtering capabilities similar to how the Plans and Addons interfaces support filtering by various attributes.
You might want to add filtering by status or creation date to allow clients to efficiently query for specific subsets of subscription addons.
500-518: Consider adding an unschedule-cancelation methodThe main Subscriptions interface has an unschedule-cancelation endpoint (lines 430-438), but there's no equivalent for subscription addons.
For consistency, consider adding an unschedule-cancelation method to the SubscriptionAddons interface to allow users to reverse a scheduled cancellation.
api/openapi.yaml (4)
8347-8430: Comprehensive POST Endpoint for Creating a Subscription AddonThe new POST endpoint (
/api/v1/subscriptions/{subscriptionId}/addons) is well defined: it clearly specifies the operationId (createSubscriptionAddon), summary, description, required path parameter with a strict pattern, detailed response codes with corresponding content schemas, and a required request body referencingSubscriptionAddonCreate. The inclusion ofx-internal: trueindicates that this endpoint is for internal use, which is acceptable if that is the intended design.Consider adding a short note (or even an example snippet) in the description of the request body to illustrate the expected payload structure. This might help client developers better understand required formats.
8580-8669: Clear Cancellation Endpoint for a Subscription AddonThe POST endpoint for
/api/v1/subscriptions/{subscriptionId}/addons/{subscriptionAddonId}/cancelis clearly defined. It includes the necessary path parameters, detailed responses, and a request body for cancellation timing. The use ofallOf: [ $ref: '#/components/schemas/SubscriptionTiming' ]for thetimingproperty (along with a descriptive note stating that cancellation is immediate if not provided) adds clarity.It might be beneficial to briefly note in the description whether the cancellation is processed synchronously or triggers asynchronous behavior, in case there is any delay or follow-up action required. Additionally, ensure that the referenced
SubscriptionTimingschema is defined and well documented elsewhere in the specification.
18360-18477: Robust Definition for theSubscriptionAddonSchemaThe new
SubscriptionAddonschema is comprehensive. It correctly lists all required properties (such asid,name,createdAt,updatedAt,key,activeFrom,version,addonId,quantity,instanceType,subscriptionId, andrateCards) with clear details on data types, patterns, descriptions, and examples. This precision in the schema will help ensure strict adherence to expected data formats.Consider including examples for additional properties (like
nameorinstanceType) to further improve client-side documentation and reduce ambiguity.
18548-18550: IncompleteSubscriptionAlignmentSchema DefinitionThe diff shows the start of a
SubscriptionAlignmentschema definition (type: object) but it appears truncated. Ensure that the remainder of this schema (including its properties, required fields, and descriptions) is fully defined as intended. This will prevent client misinterpretations and potential integration issues.Would you like guidance on completing this schema now, or should we address it in a subsequent update?
api/api.gen.go (2)
9931-9942: Consider expanded documentation for new interface methods.
Adding docstrings or references for newly introduced methods (e.g.,ListSubscriptionAddons,CreateSubscriptionAddon) would help other developers understand expectations, parameter constraints, and typical usage.
10627-10649: Implement the unimplemented addon endpoints.
These methods currently returnhttp.StatusNotImplemented. If you plan to release soon, consider at least stubs or partial implementations to facilitate integration testing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Makefile(1 hunks)api/api.gen.go(8 hunks)api/client/javascript/src/client/schemas.ts(4 hunks)api/openapi.cloud.yaml(2 hunks)api/openapi.yaml(2 hunks)api/spec/src/cloud/main.tsp(1 hunks)api/spec/src/productcatalog/routes.tsp(2 hunks)api/spec/src/productcatalog/subscriptionaddon.tsp(1 hunks)openmeter/server/router/subscriptionaddon.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/spec/src/cloud/main.tsp
- openmeter/server/router/subscriptionaddon.go
🧰 Additional context used
🧬 Code Definitions (2)
api/client/javascript/src/client/schemas.ts (2)
api/client/go/client.gen.go (3)
SubscriptionAddon(4933-4981)SubscriptionAddonCreate(4984-5005)SubscriptionAddonRateCard(5008-5014)api/api.gen.go (3)
SubscriptionAddon(5251-5299)SubscriptionAddonCreate(5302-5323)SubscriptionAddonRateCard(5326-5332)
api/api.gen.go (4)
api/client/go/client.gen.go (6)
AddonInstanceType(753-753)Metadata(3630-3630)SubscriptionAddonRateCard(5008-5014)RateCard(4454-4456)SubscriptionTiming(5267-5269)SubscriptionAddonCreate(4984-5005)api/client/javascript/src/client/schemas.ts (6)
AddonInstanceType(8835-8835)Metadata(9123-9123)SubscriptionAddonRateCard(9256-9257)RateCard(9212-9212)SubscriptionTiming(9278-9278)SubscriptionAddonCreate(9254-9255)openmeter/productcatalog/subscription/plan.go (1)
RateCard(130-133)openmeter/productcatalog/subscription/http/handler.go (1)
Handler(17-28)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test
- GitHub Check: Quickstart
- GitHub Check: Lint
- GitHub Check: E2E
- GitHub Check: Developer environment
- GitHub Check: CI
- GitHub Check: Commit hooks
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (28)
Makefile (1)
21-21: Potential inconsistency in OpenAPI generation workflowThe
gen-apitarget no longer depends onupdate-openapi-cloud, but line 17 still generates the OpenAPI cloud specification withdagger call --source .:default generate openapicloud -o ./api/openapi.cloud.yaml.This might create confusion about how the cloud specification should be generated and maintained going forward.
Consider either:
- Removing line 17 if the cloud specification is no longer needed
- Retaining the explicit
update-openapi-cloudtarget for clarity- Adding a comment explaining why the cloud specification is still generated but not as a separate target
api/spec/src/productcatalog/subscriptionaddon.tsp (2)
11-57: Consider adding missing customer information to the SubscriptionAddon modelThe model currently lacks a
customerIdproperty, which was previously suggested in comments. For better tracking and filtering capabilities, it would be beneficial to include this field.Add a
customerIdproperty to facilitate filtering addons by customer:model SubscriptionAddon { ...UniqueResource; ...global.CadencedResource; // Existing properties... + /** + * The ID of the customer. + */ + @summary("CustomerID") + @visibility(Lifecycle.Read) + customerId: ULID; + // Rest of the properties... }
54-56: Consider adding Create visibility to rateCardsThe
rateCardsproperty is only visible for reading, but it might be necessary to define them when creating a subscription addon. If this is intentional, add a comment explaining how rate cards are assigned./** * The rate cards of the addon. */ - @visibility(Lifecycle.Read) + @visibility(Lifecycle.Read, Lifecycle.Create) @summary("Rate cards") rateCards: SubscriptionAddonRateCard[];api/openapi.cloud.yaml (1)
8757-8834: Validate the Get SubscriptionAddon EndpointThis endpoint for retrieving a specific subscription addon is clearly defined with proper parameter validation for both
subscriptionIdandsubscriptionAddonId. A few points to consider:
- Confirm that the regex patterns correctly enforce the expected identifier format.
- Ensure that all described error responses correctly map to your backend logic.
api/spec/src/productcatalog/routes.tsp (2)
451-518: Well structured SubscriptionAddons interface with comprehensive CRUD operationsThe new SubscriptionAddons interface follows consistent patterns with other interfaces in the file and provides a complete set of operations for addon lifecycle management. The routes are properly nested under subscriptions, following RESTful practices.
516-516:Details
✅ Verification successful
Ensure SubscriptionTiming type is properly defined
The
timingparameter referencesSubscriptionTiming, which should be defined in an imported file. Verify that this type is properly defined and accessible.
🏁 Script executed:
#!/bin/bash # Check if SubscriptionTiming is defined in the codebase grep -r "SubscriptionTiming" --include="*.tsp" .Length of output: 1058
Verified – SubscriptionTiming Type is Properly Defined
The shell script output confirms that the
SubscriptionTimingtype is defined and correctly imported (see occurrences inapi/spec/src/productcatalog/subscription.tsp). No additional changes are required in this context.api/openapi.yaml (4)
8430-8502: Well-Structured GET Endpoint for Listing Subscription AddonsThe GET operation for
/api/v1/subscriptions/{subscriptionId}/addonsis complete and consistent with the POST endpoint. It specifies the required path parameter, uses a standard response (returning an array ofSubscriptionAddonitems) and mirrors the comprehensive error responses seen in the POST endpoint.No issues detected with this segment.
8502-8580: Accurate GET Endpoint for Retrieving a Specific Subscription AddonThe GET endpoint at
/api/v1/subscriptions/{subscriptionId}/addons/{subscriptionAddonId}correctly defines both required path parameters along with consistent response structures. The use of the same strict pattern for bothsubscriptionIdandsubscriptionAddonIdreinforces consistency across the API.This segment is clear and correctly implemented.
18478-18525: Concise and FocusedSubscriptionAddonCreateSchemaThe
SubscriptionAddonCreateschema defines the minimal set of required properties for creating a subscription addon. It clearly enforces validations (e.g., string lengths, patterns) on fields such asname,key, andactiveFrom. The separation of required versus optional fields (with properties likeactiveTobeing optional) is handled properly.This segment is well implemented and follows best practices.
18526-18547: AppropriateSubscriptionAddonRateCardSchema for Rate Card DetailsThe
SubscriptionAddonRateCardschema is defined to capture rate card details and associated subscription item IDs. Its use ofallOfto reference an existingRateCardschema promotes reuse, and the definition ofaffectedSubscriptionItemIdswith proper pattern and array validations is solid.Everything in this segment aligns well with expected API standards.
api/client/javascript/src/client/schemas.ts (11)
1772-1795: Comprehensive API endpoints for subscription addons managementThese new endpoints follow RESTful principles with well-defined methods for listing and creating subscription addons. The implementation correctly includes detailed JSDoc comments describing each operation's purpose.
1796-1815: Well-structured endpoint for retrieving subscription addon detailsThis GET endpoint is properly defined with appropriate path parameters and consistent with the API's pattern for retrieving resources by ID.
1816-1835: Clear cancellation endpoint with descriptive documentationThe cancel endpoint is well-designed with a POST method, which is appropriate for action-based operations. The documentation clearly explains that this effectively unsubscribes from the addon.
7836-7930: Complete and well-documented SubscriptionAddon schemaThe schema definition includes all necessary fields with appropriate types, readability flags, and thorough JSDoc comments. The structure aligns with the Go implementation from the relevant code snippets.
7931-7970: Well-defined SubscriptionAddonCreate schema for input validationThis schema appropriately includes only the fields that should be modifiable during creation, following the pattern of separating create/update models from the main resource model.
7971-7983: Clear SubscriptionAddonRateCard schema with proper relationshipsThe schema correctly defines the relationship between subscription addons and their rate cards, maintaining consistency with the Go implementation.
9253-9257: Proper type exports for new schema componentsThe type exports follow the established pattern in the codebase and make the new types available for use throughout the application.
20384-20477: Complete listSubscriptionAddons operation with comprehensive error handlingThe operation is well-defined with appropriate parameters, response structure, and thorough error handling for all relevant HTTP status codes.
20478-20584: Well-structured createSubscriptionAddon operationThis operation correctly includes the necessary request body schema and returns the created resource on success. All potential error responses are properly handled.
20585-20679: Complete getSubscriptionAddon operation with proper path parametersThe operation correctly includes both subscription and subscription addon IDs as path parameters and returns the appropriate resource. Error handling is comprehensive.
20680-20781: Well-designed cancelSubscriptionAddon operation with optional timing parameterThe cancel operation appropriately allows for immediate or scheduled cancellation through the optional timing parameter. The documentation makes the default behavior clear.
Consider ensuring that the implementation of this operation in the backend correctly handles the case when timing is not provided, as mentioned in the comment "If not provided, the addon will be canceled immediately."
api/api.gen.go (7)
5301-5323: Review default values forMetadata.
Metadatais allowed to benil, which could result in unexpected behavior when adding or retrieving metadata. Consider enforcing a default non-nil object or validating its usage in your service logic.
5325-5332: Looks good forSubscriptionAddonRateCard.
No immediate issues detected. The usage ofAffectedSubscriptionItemIdsandRateCardfields appears consistent with the domain context.
6894-6898: Confirm fallback timing or defaults for cancelation.
TheTimingfield is optional; an omission defaults to immediate cancellation. Double-check this logic aligns with your business requirements so that the user experience is unambiguous.
7062-7067: Create/Cancel request bodies align with existing patterns.
AliasingSubscriptionAddonCreateandCancelSubscriptionAddonJSONBodyfor request bodies is consistent with your existing approach for typed endpoints. This looks solid.
14596-14712: Parameter binding logic appears correct.
Each middleware function correctly bindssubscriptionIdand, when applicable,subscriptionAddonId. Error handling is delegated toErrorHandlerFunc. Overall, this is consistent and maintainable.
15358-15369: New routes match the introduced methods.
The router definitions for listing, creating, retrieving, and canceling subscription addons line up with the newly added interface methods, enhancing clarity and consistency.
15832-16333: Double-check for embedded secrets or sensitive data in the swagger block.
This large data block—likely an updated Swagger spec—can sometimes embed tokens or references. Verify that no private keys, tokens, or confidential info are exposed.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
openmeter/server/router/subscriptionaddon.go (2)
15-17: 🛠️ Refactor suggestionAdd request body handling and TODO comment.
This endpoint is missing both:
- Request body parsing to get the subscription addon creation data
- A TODO comment to highlight that this method needs implementation
// Create a subscription addon // (POST /api/v1/subscriptions/{subscriptionId}/addons) func (a *Router) CreateSubscriptionAddon(w http.ResponseWriter, r *http.Request, subscriptionId string) { + // TODO: Implement CreateSubscriptionAddon endpoint + // Parse request body + // var req api.SubscriptionAddonCreate + // if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + // http.Error(w, err.Error(), http.StatusBadRequest) + // return + // } w.WriteHeader(http.StatusNotImplemented) }
27-29: 🛠️ Refactor suggestionAdd request body handling and TODO comment.
This endpoint is missing both:
- Request body parsing to get the subscription addon update data
- A TODO comment to highlight that this method needs implementation
// Update a subscription addon // (PATCH /api/v1/subscriptions/{subscriptionId}/addons/{subscriptionAddonId}) func (a *Router) UpdateSubscriptionAddon(w http.ResponseWriter, r *http.Request, subscriptionId string, subscriptionAddonId string) { + // TODO: Implement UpdateSubscriptionAddon endpoint + // Parse request body + // var req api.SubscriptionAddonUpdate + // if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + // http.Error(w, err.Error(), http.StatusBadRequest) + // return + // } w.WriteHeader(http.StatusNotImplemented) }
🧹 Nitpick comments (18)
openmeter/server/router/subscriptionaddon.go (1)
1-30: Add error handling, validation, and authorization checks.All endpoints should include:
- Input validation for parameters like
subscriptionIdandsubscriptionAddonId- Authorization checks to ensure the requester has permissions to access the subscription data
- Proper error handling with appropriate HTTP status codes and error messages
Consider implementing a middleware or helper functions to handle these common concerns across all endpoints.
api/openapi.cloud.yaml (2)
8685-8756: List Subscription Addons Endpoint DefinitionThe GET endpoint for listing subscription addons is clearly articulated using the correct parameter and response definitions. The response returns an array of
SubscriptionAddonobjects with appropriate error handling. Consider adding pagination details in the future if the number of addons may be large, as this can help maintain performance and usability.
18338-18392: New Schema: SubscriptionAddonCreateThe
SubscriptionAddonCreateschema clearly defines the inputs needed to create a new subscription addon. Required properties likename,key,activeFrom,quantity, andtimingare well validated. Verify that thetimingproperty, which referencesSubscriptionTiming, accurately reflects the intended logic for timeline entry creation.api/spec/src/productcatalog/subscriptionaddon.tsp (2)
73-78: Clarify available values forAddonInstanceType.It may help future maintainers or API consumers if you explicitly specify the choices for
AddonInstanceType(e.g.,"single" | "multiple") in this file or reference documentation. Otherwise, consumers might be unsure which string values are acceptable.
114-127: Allowing zero quantity in timeline segments.Your example demonstrates a segment with
quantity: 0. If maintaining $0 or inactive segments is intentional, it would be beneficial to document situations in which a zero quantity segment is important. Otherwise, consider removing it for clarity.api/spec/src/productcatalog/routes.tsp (2)
476-476: Reword the list endpoint documentation for clarity."List all addons of a subscription. In the returned list will match to a set unique by addonId."
Consider updating this wording to improve readability, for example:
"List all addons of a subscription. The returned list is deduplicated by
addonId."- * List all addons of a subscription. In the returned list will match to a set unique by addonId. + * List all addons of a subscription. The returned list is deduplicated by `addonId`.
479-485: Consider pagination for the list endpoint.Although you noted that the addon list is typically small, consider adding at least basic pagination support or clarifying that pagination is not needed given the expected data volume. This helps ensure consistency with other list endpoints in the API.
api/openapi.yaml (10)
8347-8430: Clarify and Confirm SubscriptionAddon Creation Endpoint.
The POST endpoint for creating a subscription addon is comprehensive, covering parameters, responses, and the required request body. Please ensure that the description explicitly clarifies when to provide the key versus the addon ID. Also, verify that the subscriptionId pattern is consistently reused elsewhere in the API.
8430-8501: Improve Clarity in GET List Endpoint Description.
The GET endpoint description currently states, "List all addons of a subscription. In the returned list will match to a set unique by addonId." Consider rephrasing for improved clarity, for example: "List all subscription addons, ensuring each addonId appears only once in the returned set."
8578-8662: Refine Description for PATCH Update Operation.
The PATCH endpoint for updating a subscription addon is functionally correct and properly references theSubscriptionAddonUpdateschema. It would be helpful to clarify the description—specifically, explicitly mention which properties can be updated (e.g., quantity adjustments for adding or canceling instances).
8663-8665: Cancellation Endpoint Verification.
A new POST endpoint for/api/v1/subscriptions/{subscriptionId}/cancelwith operationIdcancelSubscriptionis introduced. Please confirm that this endpoint belongs in the current API file and consider providing additional details (such as a summary and response examples) to ensure consistency with the SubscriptionAddon endpoints.
18351-18497: Review & Validate SubscriptionAddon Schema Definition.
TheSubscriptionAddonschema is well-structured, including a comprehensive set of required fields such asid,name, timestamps, and others. Please verify that includingquantityAtas a required field aligns with your data model, and consider adding a note in the description to clarify its purpose in relation to the creation and update processes.
18498-18552: Verify SubscriptionAddonCreate Schema Requirements.
TheSubscriptionAddonCreateschema mandates fields likename,key,activeFrom,quantity, andtiming. Ensure that thetimingproperty correctly references theSubscriptionTimingschema defined elsewhere, and consider providing an example value for improved clarity for API consumers.
18553-18574: SubscriptionAddonRateCard Schema Evaluation.
TheSubscriptionAddonRateCardschema aptly requiresrateCardandaffectedSubscriptionItemIdsto represent rate card data. Please confirm that the pattern for subscription item IDs is consistent with other parts of the API, and that the description sufficiently explains the relationship between rate cards and subscription items.
18575-18596: Timeline Segment Schema Review.
TheSubscriptionAddonTimelineSegmentschema effectively captures timeline details with requiredactiveFromandquantityfields. Consider evaluating if theactiveTofield should also be required when defined (for past segments) or if its optional nature adequately supports representing ongoing periods.
18597-18638: Update Schema: SubscriptionAddonUpdate.
TheSubscriptionAddonUpdateschema correctly defines optional properties for updating a subscription addon. Ensure that the server-side implementation properly supports partial updates and that any validation rules for these properties are clearly documented.
18639-18641: Incomplete Schema: SubscriptionAlignment.
TheSubscriptionAlignmentschema appears to be introduced but is not fully defined. If this is intentional (to be completed in a future update), consider adding a TODO or placeholder note so that reviewers and consumers are aware of its intended structure.api/api.gen.go (1)
10666-10689: Implementation stubs are unimplemented.All methods return 501 Not Implemented. Let me know if you’d like assistance creating a skeletal implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/api.gen.go(7 hunks)api/client/javascript/src/client/schemas.ts(4 hunks)api/openapi.cloud.yaml(2 hunks)api/openapi.yaml(2 hunks)api/spec/src/productcatalog/routes.tsp(2 hunks)api/spec/src/productcatalog/subscriptionaddon.tsp(1 hunks)openmeter/server/router/subscriptionaddon.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
api/client/javascript/src/client/schemas.ts (2)
api/client/go/client.gen.go (5)
SubscriptionAddon(4933-4987)SubscriptionAddonCreate(4990-5014)SubscriptionAddonRateCard(5017-5023)SubscriptionAddonTimelineSegment(5026-5035)SubscriptionAddonUpdate(5038-5059)api/api.gen.go (5)
SubscriptionAddon(5251-5305)SubscriptionAddonCreate(5308-5332)SubscriptionAddonRateCard(5335-5341)SubscriptionAddonTimelineSegment(5344-5353)SubscriptionAddonUpdate(5356-5377)
api/api.gen.go (2)
api/client/go/client.gen.go (7)
Metadata(3630-3630)SubscriptionAddonRateCard(5017-5023)SubscriptionAddonTimelineSegment(5026-5035)SubscriptionTiming(5312-5314)RateCard(4454-4456)SubscriptionAddonCreate(4990-5014)SubscriptionAddonUpdate(5038-5059)openmeter/productcatalog/subscription/http/handler.go (1)
Handler(17-28)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (36)
openmeter/server/router/subscriptionaddon.go (2)
9-11: Add TODO comment and missing implementation.This endpoint is currently a stub returning "Not Implemented". Add a TODO comment to highlight that this method needs implementation.
#!/bin/bash # Check for any planned/tracked implementation work gh issue list --search "in:title subscription addon implementation" --state open
21-23: Add TODO comment for implementation.This endpoint is currently a stub returning "Not Implemented". Add a TODO comment to highlight that this method needs implementation.
// Get subscription addon // (GET /api/v1/subscriptions/{subscriptionId}/addons/{subscriptionAddonId}) func (a *Router) GetSubscriptionAddon(w http.ResponseWriter, r *http.Request, subscriptionId string, subscriptionAddonId string) { + // TODO: Implement GetSubscriptionAddon endpoint w.WriteHeader(http.StatusNotImplemented) }api/openapi.cloud.yaml (8)
8602-8684: Create Subscription Addon Endpoint DefinitionThe new POST endpoint for creating a subscription addon is comprehensively defined. It specifies the necessary path parameter (
subscriptionId), detailed response codes (from 201 success to various error conditions), and a request body that references theSubscriptionAddonCreateschema. The ULID pattern forsubscriptionIdis applied consistently.
8757-8834: Get Subscription Addon Endpoint DefinitionThe GET endpoint for retrieving a specific subscription addon by its ID is well-structured. It correctly uses two path parameters (
subscriptionIdandsubscriptionAddonId) and provides consistent error responses. Please verify that the example values and the ULID regex pattern are fully aligned with your overall API standards.
8835-8917: Update Subscription Addon Endpoint DefinitionThe PATCH endpoint for updating a subscription addon is defined with clarity. It takes two path parameters and includes a request body referencing the
SubscriptionAddonUpdateschema. The description explains that this operation allows for changes in quantity. Ensure that partial updates are handled correctly in the backend implementation, particularly for fields that might be optional.
8918-8920: Cancellation Endpoint InclusionThe diff appends a cancellation endpoint (
/api/v1/subscriptions/{subscriptionId}/cancel) immediately following the SubscriptionAddon endpoints. Please confirm that this endpoint is intended to be included within this PR since the primary focus appears to be on SubscriptionAddon support.
18194-18337: New Schema: SubscriptionAddonThe
SubscriptionAddonschema is comprehensive and well-documented. It includes all necessary required fields such asid,name,createdAt, and others, along with proper validations (e.g., the ULID regex pattern) and readOnly attributes for immutable properties. Ensure these definitions are consistent with other similar schemas in the API.
18393-18415: New Schema: SubscriptionAddonRateCardThe
SubscriptionAddonRateCardschema effectively encapsulates rate card details, including the associated subscription item IDs. The field validations and the readOnly flag foraffectedSubscriptionItemIdsare appropriate. This schema appears to support the necessary rate card data without redundancies.
18416-18437: New Schema: SubscriptionAddonTimelineSegmentThis schema accurately defines a timeline segment for a subscription addon with clear requirements for
activeFrom,activeTo, andquantity. The provided example demonstrates how timeline data should be structured. The design supports continuous timeline representation effectively.
18438-18478: New Schema: SubscriptionAddonUpdateThe
SubscriptionAddonUpdateschema allows for updating addon properties such asname,description,metadata,activeFrom,activeTo,quantity, andtiming. The relaxed requirements compared to the create schema are appropriate; however, please verify that allowing updates toquantity(especially for single instance addons) aligns with intended business logic.api/spec/src/productcatalog/subscriptionaddon.tsp (1)
53-71: Consider timeline boundary validations.Your timeline examples include transitions from quantity=1 to quantity=0 and back to 1 across different timestamps. If continuity or chronological ordering is crucial, consider adding constraints or clarifying logic on overlapping/invalid intervals.
api/spec/src/productcatalog/routes.tsp (2)
2-2: Import usage looks correct.The direct import of
subscriptionaddon.tspintoroutes.tspensures that types (e.g.,SubscriptionAddon) are correctly referenced in this interface. No concerns here.
501-507: Confirm concurrency and partial update needs.When updating subscription addons, there could be concurrency concerns or partial updates. If multiple updates happen concurrently, define whether the last write wins or if you require an ETag-like strategy. Additionally, confirm that partial updates (e.g., only changing
quantity) do not inadvertently remove other properties.api/openapi.yaml (1)
8502-8577: Review Specific SubscriptionAddon Retrieval.
The GET method for retrieving a specific subscription addon is well defined with consistent parameter patterns and response references. No issues detected; the implementation aligns with OpenAPI standards.api/client/javascript/src/client/schemas.ts (12)
1772-1795: API endpoint definitions look goodThe implementation for subscription addon endpoints follows RESTful conventions with clearly defined HTTP methods and descriptive comments. The nested structure correctly maintains the relationship between subscriptions and their addons.
1796-1819: Well-structured single addon endpointsThe implementation for retrieving and updating a specific subscription addon is properly defined with appropriate HTTP methods and comprehensive documentation. The endpoint path structure properly includes both the subscription ID and subscription addon ID.
7820-7943: Complete and well-documented SubscriptionAddon schemaThe SubscriptionAddon schema is thorough and includes all necessary fields with detailed JSDoc comments. The inclusion of examples and clear descriptions for each field will make the API more developer-friendly.
I particularly like the detailed documentation for the timeline field with a comprehensive example that shows how to track quantity changes over time.
7944-7989: Well-defined SubscriptionAddonCreate schemaThe create operation schema includes all the necessary fields with good validation requirements. The timing field correctly references the SubscriptionTiming type for consistency with other subscription operations.
7990-8002: Clear RateCard relationship definitionThe SubscriptionAddonRateCard schema appropriately defines the relationship between rate cards and subscription items, with proper readonly annotations where applicable.
8003-8022: Effective timeline segmentation schemaThe TimelineSegment schema provides a clean approach to tracking changes over time. The readonly quantity field correctly prevents direct modification of historical data.
8023-8063: Comprehensive update schemaThe update schema correctly makes all fields optional, allowing partial updates. The inclusion of timing information ensures that changes can be scheduled appropriately.
9333-9341: Clean type exportsType exports are correctly defined to make the schema components accessible throughout the codebase.
20468-20561: Complete list operation with error handlingThe listSubscriptionAddons operation is well-defined with appropriate parameters and comprehensive error handling. All standard HTTP error responses are covered.
20562-20667: Thorough create operationThe createSubscriptionAddon operation includes all necessary parameters, request body definition, and complete error handling, including conflict scenarios (409).
20668-20762: Well-implemented get operationThe getSubscriptionAddon operation is properly structured with path parameters for both the subscription and addon IDs. Error handling is comprehensive.
20763-20861: Complete update operationThe updateSubscriptionAddon operation correctly includes path parameters and the request body definition, with appropriate response types and error handling.
api/api.gen.go (10)
5250-5305: Enforce single-instance quantity constraint.The docstring states that
Quantitymust always be1whenInstanceTypeissingle, but there is no actual validation logic to enforce it. This was flagged in a previous review and remains relevant.
5307-5332: Confirm validation for field constraints.Several fields (e.g.,
Namewith length limits) do not appear to have explicit validation. Please verify if these validations occur elsewhere in the codebase (e.g. service layer) or add them here if needed.
5334-5341: Looks good forSubscriptionAddonRateCard.
No issues detected in this struct; it is straightforward and clearly documented.
5343-5353: No concerns withSubscriptionAddonTimelineSegment.
The timeline segment fields (start, end, and quantity) are clearly documented, and no correctness issues are apparent.
5355-5377: Reinforce single-instance quantity constraint again.Similar to
SubscriptionAddon,SubscriptionAddonUpdatealso declaresQuantityas “Always 1” for single-instance addons. Please confirm that it’s enforced or validate accordingly.
7098-7106: Type aliases for JSON request bodies look fine.
These aliases map directly to the corresponding structs without introducing new logic. No issues here.
9967-9981: New interface methods are well-defined.
They introduce standard CRUD-like operations for subscription addons. Implementation details can be handled elsewhere. No immediate concerns.
14635-14751: No issues with middleware param handling.
The auto-generated binding logic appears correct and follows chi’s parameter binding patterns.
15397-15408: New routes for subscription addons.
The route definitions for listing, creating, retrieving, and updating subscription addons match the corresponding methods. No issues identified.
15871-16380: Embedded specification data looks fine.
This section appears to be auto-generated Swagger or OpenAPI content. No concerns regarding correctness or security.
GAlexIHU
force-pushed
the
galexi/addons-api
branch
from
74466b4 to
faf15a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
api/openapi.cloud.yaml (1)
18492-18782:⚠️ Potential issueRobust Schema Definitions with Incomplete
SubscriptionAlignmentSchemaThe schemas for
SubscriptionAddon,SubscriptionAddonCreate,SubscriptionAddonRateCard,SubscriptionAddonTimelineSegment, andSubscriptionAddonUpdateare defined in a detailed and consistent manner. All required properties have appropriate types, constraints, and examples, ensuring clarity for both documentation and validation.One point of concern is the
SubscriptionAlignmentschema (lines 18780-18782), which appears incomplete—only the type and an empty properties placeholder are provided. This echoes previous feedback regarding an incomplete definition and may lead to inconsistencies in API documentation generation. Please review and complete this schema with the intended properties.Tags:
🧹 Nitpick comments (5)
api/spec/src/productcatalog/routes.tsp (1)
476-486: Consider implementing pagination for the list endpointThe comment on line 484 raises a valid question about pagination. While the list of subscription addons for a single subscription may typically be small, it's a best practice to implement pagination for list endpoints to handle unexpected growth and ensure consistent behavior across the API.
Consider adding pagination support similar to how it's implemented in the
Plans.list()method:list( @path subscriptionId: ULID, // Should we implement pagination? This should never be a large list. + ...OpenMeter.QueryPagination, + ...OpenMeter.QueryOrdering<SubscriptionAddonOrderBy>, - ): SubscriptionAddon[] | OpenMeter.NotFoundError | OpenMeter.CommonErrors; + ): OpenMeter.PaginatedResponse<SubscriptionAddon> | OpenMeter.NotFoundError | OpenMeter.CommonErrors;This would require defining a
SubscriptionAddonOrderBytype as well.api/openapi.yaml (2)
8507-8578: Subscription Addon Listing (GET method)
The GET method for listing subscription addons is implemented with proper parameter validation and comprehensive error responses. The description could be clarified for better readability—for example, consider rephrasing "In the returned list will match to a set unique by addonId" to "The returned list contains unique entries based on addonId."
8657-8740: Update Subscription Addon (PATCH method)
The PATCH endpoint is correctly structured, with parameters forsubscriptionIdandsubscriptionAddonIdand a request body that references theSubscriptionAddonUpdateschema. It is advisable to document the update semantics clearly (e.g., whether partial updates are allowed) in the API documentation.api/client/javascript/src/client/schemas.ts (2)
1776-1799: Consider clarifying documentation and verifying path parameter usage.
The description at lines 1784–1785 is somewhat unclear ("In the returned list will match to a set unique by addonId"). Also, this path currently setspath?: never, which may be confusing if you intend to pass{subscriptionId}. Please confirm that the path parameter is recognized by your code generator or clarify the doc to ensure correct usage.
8015-8258: Clarify quantity behavior for multiple-instance addons.
You note that quantity is “Always 1 for single instance add-ons,” but the schema also supports a “multiple” instance type. Consider clarifying how to handle the quantity field for multiple instances to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Makefile(1 hunks)api/client/javascript/src/client/schemas.ts(4 hunks)api/openapi.cloud.yaml(2 hunks)api/openapi.yaml(2 hunks)api/spec/src/cloud/main.tsp(1 hunks)api/spec/src/productcatalog/routes.tsp(2 hunks)api/spec/src/productcatalog/subscriptionaddon.tsp(1 hunks)openmeter/server/router/subscriptionaddon.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- api/spec/src/cloud/main.tsp
- openmeter/server/router/subscriptionaddon.go
- Makefile
- api/spec/src/productcatalog/subscriptionaddon.tsp
🧰 Additional context used
🧬 Code Definitions (1)
api/client/javascript/src/client/schemas.ts (2)
api/api.gen.go (5)
SubscriptionAddon(5402-5456)SubscriptionAddonCreate(5459-5483)SubscriptionAddonRateCard(5486-5492)SubscriptionAddonTimelineSegment(5495-5504)SubscriptionAddonUpdate(5507-5528)api/client/go/client.gen.go (5)
SubscriptionAddon(5084-5138)SubscriptionAddonCreate(5141-5165)SubscriptionAddonRateCard(5168-5174)SubscriptionAddonTimelineSegment(5177-5186)SubscriptionAddonUpdate(5189-5210)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test
- GitHub Check: Quickstart
- GitHub Check: Lint
- GitHub Check: E2E
- GitHub Check: Developer environment
- GitHub Check: CI
- GitHub Check: Commit hooks
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (20)
api/openapi.cloud.yaml (1)
8676-8997: Comprehensive Endpoint Definitions with Minor Clarification NeededThe new endpoints for creating, listing, retrieving, and updating subscription addons are defined thoroughly with proper request parameters, response schemas, and error handling. The consistent use of path parameters (with ULID regex patterns) and the inclusion of the
x-internal: trueflag across operations are well implemented.However, please clarify the intention behind the trailing endpoint (
/api/v1/subscriptions/{subscriptionId}/cancelat lines 8996-8997). Since the PR objectives focus on SubscriptionAddon APIs, confirm whether this cancellation endpoint is part of the intended changes or an unrelated addition.Tags:
api/spec/src/productcatalog/routes.tsp (6)
2-2: Import statement for subscriptionaddon module addedProper import statement for the SubscriptionAddon types that will be used in the new interface.
452-454: Review route path naming for consistencyThe route path uses
/api/v1/subscriptions/{subscriptionId}/addonswhich follows RESTful conventions. However, there was a previous comment about using/subscriptionaddonsinstead. The current implementation appears more appropriate as it properly represents the resource hierarchy and matches the RESTful pattern used elsewhere in the codebase.
455-475: Create endpoint for subscription addons looks goodThe create endpoint is well-defined with appropriate parameters, return types, and error handling. The
@extension("x-internal", true)marker is consistent with other addon-related endpoints in the codebase.
487-499: Endpoint for retrieving subscription addon by ID looks goodThe get endpoint is properly defined with all necessary parameters and return types.
500-515: Consider alternative ID strategy for subscription addonsThe previous comment questioned whether to key by addonId/addonKey+Version instead of SubscriptionAddonId. While the current implementation using SubscriptionAddonId is valid, consider whether a composite key might be more intuitive for API users.
Explore whether there are use cases where API consumers would prefer to reference subscription addons by their addon identity rather than by a separate subscription-addon ID. This depends on how subscriptions and addons are managed in your system and how consumers tend to interact with them.
Additionally, the update endpoint appears to allow only changing quantity and cancellation. If there are other properties that should be updatable, ensure they're explicitly mentioned in the documentation.
452-515: Overall implementation of SubscriptionAddons interfaceThe implementation of the SubscriptionAddons interface follows the pattern established by other interfaces in the file, with appropriate HTTP methods, route definitions, and error handling. The interface is properly tagged and all endpoints are marked as internal, consistent with the Addons interface.
One consideration: If subscription addons have a lifecycle similar to subscriptions (like cancellation, scheduling, etc.), you might want to add specific endpoints for these operations rather than handling them through the update endpoint. This would make the API more explicit and easier to use.
api/openapi.yaml (7)
8424-8507: Endpoint Creation for Subscription Addon (POST method)
The new POST endpoint/api/v1/subscriptions/{subscriptionId}/addonsis well-defined. ThesubscriptionIdparameter is validated using a robust pattern, and the request body correctly references theSubscriptionAddonCreateschema. All expected success and error responses are accounted for.
8579-8657: Retrieve Specific Subscription Addon (GET method)
The GET endpoint/api/v1/subscriptions/{subscriptionId}/addons/{subscriptionAddonId}is consistent with the design of the POST endpoint. It includes detailed parameter validation (for bothsubscriptionIdandsubscriptionAddonId) and well-structured response objects.
18655-18798: SubscriptionAddon Schema Definition
The newSubscriptionAddonschema is comprehensive and enforces required fields such asid,name,createdAt,updatedAt,key,activeFrom,version,addonId,quantityAt,quantity,timeline,instanceType,subscriptionId, andrateCards. The use of readOnly flags and pattern validations (e.g., for ULID) appears consistent and robust. Verify that including properties likedeletedAtandtimelinealigns with the intended business logic.
18799-18854: SubscriptionAddonCreate Schema
TheSubscriptionAddonCreateschema mandates essential fields for addon creation includingname,key,activeFrom,quantity, andtiming. It is important to ensure that the role of thetimingproperty (which triggers timeline entries) is clearly explained in the API documentation.
18855-18876: SubscriptionAddonRateCard Schema
TheSubscriptionAddonRateCardschema is well-designed with required properties such asrateCardandaffectedSubscriptionItemIds. MarkingaffectedSubscriptionItemIdsas readOnly is a good practice to maintain data integrity.
18876-18898: SubscriptionAddonTimelineSegment Schema
TheSubscriptionAddonTimelineSegmentschema straightforwardly enforces timeline tracking through required fields likeactiveFromandquantity. The readOnly attribute onquantityhelps ensure that historical timeline data remains unaltered.
18898-18939: SubscriptionAddonUpdate Schema
The update schema provides flexibility for partial updates by making properties likename,description,metadata,activeFrom,activeTo,quantity, andtimingoptional. Ensure that the validation constraints here remain consistent with those in the creation schema.api/client/javascript/src/client/schemas.ts (6)
1800-1823: Endpoints appear sound.
ThegetSubscriptionAddonandupdateSubscriptionAddonoperations look properly documented and structured. It's good to see a PATCH endpoint for partial updates.
9550-9558: Type exports look consistent.
Exporting the newly introduced schemas as types is straightforward and aligns with typical usage.
20800-20893: List subscription addons endpoint definition looks good.
It properly returns an array ofSubscriptionAddonobjects and includes comprehensive error responses.
20894-21000: Create subscription addon endpoint definition looks good.
Returns a 201 Created response on success, and includes sensible coverage for error states.
21001-21095: Get subscription addon endpoint is well-defined.
Allows retrieval of a single addon with thorough error reporting for various conditions.
21096-21194: Update subscription addon endpoint is well-defined.
Responds with a 200 status and the updated addon on success, with robust error coverage.
tothandras
left a comment
tothandras
left a comment
There was a problem hiding this comment.
Let's circle back later to the update endpoint.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
api/openapi.cloud.yaml (1)
18829-18831: Incomplete Schema Definition: SubscriptionAlignment
TheSubscriptionAlignmentschema appears incomplete or truncated (only the header and a placeholder for properties are shown). This issue was previously flagged; please complete the definition to ensure proper API documentation generation and consistency.api/spec/src/productcatalog/subscriptionaddon.tsp (1)
65-71: Enforce quantity constraints for single instance add-ons.The comment states that quantity should "Always be 1 for single instance add-ons", but there's no validation ensuring this relationship in the model definition.
Consider adding validation to enforce this business rule, such as:
- Using a discriminated union type based on
instanceType- Adding runtime validation in your implementation
- At minimum, adding a
@minValue(1)annotation to the quantity field, similar to what you've done for the version field/** * The quantity of the addon. Always 1 for single instance add-ons. */ @summary("Quantity") @visibility(Lifecycle.Read, Lifecycle.Create, Lifecycle.Update) @example(1) +@minValue(1) quantity: integer;api/openapi.yaml (2)
8740-8742:⚠️ Potential issueCancel Subscription Endpoint Incompleteness
The endpoint/api/v1/subscriptions/{subscriptionId}/cancelis introduced with a POST method and an operationId (cancelSubscription); however, its definition is incomplete—it lacks parameters, a requestBody, and response details.Note: This is similar to previous feedback on incomplete endpoint definitions.
18989-18991:⚠️ Potential issueIncomplete
SubscriptionAlignmentSchema
TheSubscriptionAlignmentschema appears to be incomplete—it is defined with a type but no further properties or descriptions are provided. Please complete the schema with the necessary properties and descriptions or remove it if it is no longer required.
🧹 Nitpick comments (11)
api/openapi.cloud.yaml (1)
8679-8834: Endpoint Definitions: Create and List Subscription Addons
The POST endpoint for creating a subscription addon and the GET endpoint for listing addons are defined with appropriate parameters, response codes, and validations. The use of a strict regular expression for thesubscriptionIdis consistent with the API’s conventions. One suggestion is to review and possibly clarify the GET endpoint’s description regarding uniqueness byaddonIdfor better readability.api/spec/src/productcatalog/subscriptionaddon.tsp (5)
15-48: Consider refining the addon property structure.The
addonproperty contains nested properties that duplicate information also present at the top level (e.g.,addonId). This nested structure could potentially cause confusion or data inconsistency issues.Consider whether this partial embedding of addon properties is necessary, or if a reference to the full addon would be more appropriate.
80-98: Consider adding validation for timeline segment continuity.The timeline property contains segments that are described as "sorted and continuous" in the comment, but there's no explicit validation to ensure this. If possible, consider adding validation to ensure segments don't overlap and maintain continuity.
115-132: Consider improving documentation for SubscriptionAddonRateCard.The model
SubscriptionAddonRateCardconnects rate cards to subscription items, but the exact purpose and relationship could benefit from more detailed documentation, especially regarding how it affects subscription pricing or billing.
134-147: Add validation for quantity in timeline segments.The
quantityproperty inSubscriptionAddonTimelineSegmentlacks validation. Consider adding a@minValue(0)annotation since quantity should logically be a non-negative value./** * The quantity of the addon for the given period. */ @summary("Quantity") @visibility(Lifecycle.Read) +@minValue(0) quantity: integer;
1-148: Consider adding operational documentation and cross-references.The models are well-structured but would benefit from additional documentation:
- Add examples of how these models interact with subscription management workflows
- Document the relationship between
SubscriptionAddonand other entities likeSubscriptionandAddon- Consider adding cross-references to related API operations that use these models
This would help developers understand the full context and proper usage of these models.
api/openapi.yaml (4)
8424-8506: POST Endpoint for Creating a Subscription Addon
The new POST endpoint (/api/v1/subscriptions/{subscriptionId}/addons) is well-detailed with clear operationId, parameter validation (including a strict regex forsubscriptionId), comprehensive response definitions, and proper usage of the request body referencing theSubscriptionAddonCreateschema.Recommendation: Consider clarifying the description slightly to explicitly mention whether the client should provide the addon key or the addon id (and under what conditions) if both are supported.
8507-8578: GET Endpoint for Listing Subscription Addons
The GET method under the same path is properly defined with required path parameters and a response that returns an array ofSubscriptionAddonobjects. The error responses and response content are consistent with the POST endpoint.Suggestion: The description "List all addons of a subscription. In the returned list will match to a set unique by addonId." might be rephrased for clarity—for example, "List all addons for a subscription, ensuring uniqueness based on addonId."
18655-18813:SubscriptionAddonSchema Review
TheSubscriptionAddonschema is comprehensive and lists all critical properties (such asid,name,createdAt,updatedAt,activeFrom,addon,addonId, etc.). One point to consider is whether requiring bothaddon(an object) andaddonId(a string) might be redundant or if they serve distinct purposes. Aligning the documentation to clarify their roles could improve maintainability.
18814-18899:SubscriptionAddonCreateSchema Validation
TheSubscriptionAddonCreateschema is well-defined with required fields such asname,activeFrom,addon,quantity, andtiming. It captures the minimal information needed to create a subscription addon.Suggestion: Verify that the field
timingis consistent with its intended use (possibly relating to timeline creation) and consider adding more context in its description if needed.api/client/javascript/src/client/schemas.ts (1)
8015-8298: Consider refining theaddon?: Record<string, never>property.In
SubscriptionAddonUpdate, theaddonfield is declared asRecord<string, never>, which may limit clarity and future flexibility. If you intend partial updates to an addon's structure, consider a more descriptive partial type or anunknowntype if the structure is uncertain, along with validation logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/client/javascript/src/client/schemas.ts(4 hunks)api/openapi.cloud.yaml(2 hunks)api/openapi.yaml(2 hunks)api/spec/src/productcatalog/subscriptionaddon.tsp(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
api/client/javascript/src/client/schemas.ts (2)
api/api.gen.go (5)
SubscriptionAddon(5402-5462)SubscriptionAddonCreate(5465-5501)SubscriptionAddonRateCard(5504-5510)SubscriptionAddonTimelineSegment(5513-5522)SubscriptionAddonUpdate(5525-5549)api/client/go/client.gen.go (5)
SubscriptionAddon(5084-5144)SubscriptionAddonCreate(5147-5183)SubscriptionAddonRateCard(5186-5192)SubscriptionAddonTimelineSegment(5195-5204)SubscriptionAddonUpdate(5207-5231)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (17)
api/openapi.cloud.yaml (7)
8835-8994: Endpoint Definitions: Retrieve and Update a Single Subscription Addon
The GET and PATCH endpoints for handling a single subscription addon are well-structured with clearly defined parameters and response schemas. The PATCH description mentions both increasing the quantity and canceling current instances; please verify that this accurately reflects the intended business logic (especially for add-ons that are strictly single-instance vs. multiple-instance).
8995-8997: Endpoint Inclusion: Cancel Subscription
The inclusion of the/api/v1/subscriptions/{subscriptionId}/cancelendpoint appears alongside the subscription addon routes, even though it is not directly related to addon operations as outlined in the PR objectives. Please confirm whether its inclusion here is intentional or if it should be handled separately.
18495-18653: Schema Review: SubscriptionAddon
TheSubscriptionAddonschema is comprehensive, listing all required properties with clear descriptions, patterns, and examples. The detailed constraints (e.g., for IDs, names, timestamps) help ensure consistent data integrity.
18654-18738: Schema Review: SubscriptionAddonCreate
The create schema properly specifies required fields such asname,activeFrom,addon,quantity, andtimingalong with validation rules. This should facilitate proper client-side validation during addon creation.
18739-18760: Schema Review: SubscriptionAddonRateCard
TheSubscriptionAddonRateCarddefinition is correctly structured and clearly describes the association between rate cards and subscription items. The use of a reference to the sharedRateCardschema promotes consistency.
18761-18783: Schema Review: SubscriptionAddonTimelineSegment
This schema clearly defines the timeline segments using start/end timestamps and quantity values. The provided examples and constraints should help maintain a continuous and sorted timeline as intended.
18784-18828: Schema Review: SubscriptionAddonUpdate
The update schema covers the optional fields that can be modified, maintaining consistency with the create model and enforcing proper validation rules. Ensure that downstream logic properly handles the optional nature of these fields.api/spec/src/productcatalog/subscriptionaddon.tsp (2)
1-6: Clean imports and organization.The imports and namespace declaration are clear and appropriate for a TypeSpec file.
7-14: Well-defined model structure with clear inheritance.The
SubscriptionAddonmodel inherits fromResourceandglobal.CadencedResource, which seems appropriate for a subscription-related entity.api/openapi.yaml (5)
8579-8656: GET Endpoint for Retrieving a Specific Subscription Addon
The endpoint/api/v1/subscriptions/{subscriptionId}/addons/{subscriptionAddonId}for fetching a single subscription addon is clearly defined. Parameter validations (including regex patterns) and detailed response schemas support robust API behavior.
8657-8733: PATCH Endpoint for Updating a Subscription Addon
The PATCH method is implemented with a clear operationId, detailed error responses, and a request body linked to theSubscriptionAddonUpdateschema. The description adequately explains that it allows modifications such as changing the quantity or cancellation of instances.Request Verification: Ensure that the allowed update fields in the
SubscriptionAddonUpdateschema are properly validated against business rules.
18900-18921:SubscriptionAddonRateCardSchema
TheSubscriptionAddonRateCardschema cleanly defines its required properties with a proper reference to the existingRateCardschema and specifies the array foraffectedSubscriptionItemIds. The schema appears correctly structured and documented.
18921-18943:SubscriptionAddonTimelineSegmentSchema Review
The timeline segment definition is clear and includes essential properties such asactiveFrom,activeTo, andquantity. The provided example makes its usage intuitive.
18944-18988:SubscriptionAddonUpdateSchema Completeness
TheSubscriptionAddonUpdateschema offers flexibility with optional fields for updating a subscription addon. This allows for partial updates, which is good.Request Verification: Confirm that updates for fields like
activeFromandactiveToare handled as per business requirements and that any constraints (if any) are enforced within the business logic.api/client/javascript/src/client/schemas.ts (3)
9590-9598: Exports for Subscription Addon Types look correct.These exports are suitably descriptive and align with the newly introduced schemas. They will help in ensuring consistent type usage across the codebase. Great job!
20840-21234:Details
❓ Verification inconclusive
Verify error handling and response codes coverage.
The newly added operations (list, create, get, and update) carefully detail responses for various status codes. Ensure test coverage includes these error conditions (400, 404, 409, 412, etc.) to prevent regressions.
🏁 Script executed:
#!/bin/bash # Description: Verify test coverage of new subscriptionAddon endpoints with relevant error codes. rg -A 10 "describe('.*addon.*" test/ rg -A 10 "it('.*400.*" test/ rg -A 10 "it('.*404.*" test/ rg -A 10 "it('.*409.*" test/ rg -A 10 "it('.*412.*" test/Length of output: 516
Manual Verification Required: Robust Test Coverage for Error Conditions
The schema updates for the subscription addon endpoints (list, create, get, and update) include detailed error responses for various HTTP status codes. However, the automated checks intended to verify test coverage for these error conditions (400, 404, 409, 412, etc.) encountered regex errors due to unescaped groups.
- Please re-run the test coverage checks using corrected patterns (or verify manually) to ensure that tests exist for these error scenarios.
- Confirm that tests for these endpoints validate error responses appropriately, so that regressions in error handling are prevented.
1776-1823:Details
✅ Verification successful
Ensure path parameters are typed in TypeScript definitions.
Here,
path?: neverfor the subscription ID path parameter may be an artifact of code generation. Typically, path parameters should be strongly typed (e.g.,{ path: { subscriptionId: string } }). Verify that the client code indeed passessubscriptionIdcorrectly to avoid runtime errors.Please run the following script to search for references of
listSubscriptionAddonsorcreateSubscriptionAddonin the server-handling code to confirm proper usage of the path parameter:
🏁 Script executed:
#!/bin/bash # Description: Confirm server handlers exist for listSubscriptionAddons and createSubscriptionAddon with typed param usage rg -A 5 'listSubscriptionAddons' rg -A 5 'createSubscriptionAddon'Length of output: 4204
Validated: Path Parameter Typing is Correct
The endpoint definitions may show
path?: never, but this is intentional since the actual parameter types are inherited from the referenced operations. The grep results confirm that bothlistSubscriptionAddonsandcreateSubscriptionAddonin the operations definitions correctly specify the path parameter as{ subscriptionId: string }. No changes are needed on the client side.
GAlexIHU
force-pushed
the
galexi/addons-api
branch
from
76aeb39 to
35d042a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
api/openapi.cloud.yaml (1)
18829-18831:⚠️ Potential issueIncomplete Schema: SubscriptionAlignment
TheSubscriptionAlignmentschema appears to be incomplete—it lists the type and starts thepropertiessection but does not provide any property definitions. This is similar to previous concerns noted in older commits.
- Action Required: Please complete the schema definition or remove it if it is not required.
+ # TODO: Define the properties for SubscriptionAlignment or remove this schema if unused.api/spec/src/productcatalog/subscriptionaddon.tsp (1)
65-71: 🛠️ Refactor suggestionAdd validation for quantity based on instanceType
There's a comment stating that quantity should "Always be 1 for single instance add-ons", but there's no validation enforcing this relationship in the model definition.
Consider adding a validation annotation to enforce this business rule:
/** * The quantity of the addon. Always 1 for single instance add-ons. */ @summary("Quantity") @visibility(Lifecycle.Read, Lifecycle.Create, Lifecycle.Update) @example(1) + @minValue(1) quantity: integer;Additionally, consider adding a more explicit constraint in documentation:
/** - * The quantity of the addon. Always 1 for single instance add-ons. + * The quantity of the addon. + * IMPORTANT: Must be exactly 1 when instanceType is "single". + * Can be any positive integer when instanceType is "multiple". */
🧹 Nitpick comments (13)
api/openapi.cloud.yaml (4)
8679-8761: Endpoints for Creating and Listing Subscription Addons
The new POST endpoint for creating a subscription addon and the GET endpoint for listing addons are well structured. The parameters, request body, response codes, and error responses are consistent and follow best practices.
- Suggestion: Consider rewording the GET endpoint’s description for clarity. For example, change "List all addons of a subscription. In the returned list will match to a set unique by addonId." to something like "List all subscription addons, ensuring each addonId is unique in the result set."
8913-8994: Endpoint for Updating a Subscription Addon (PATCH)
The update endpoint is comprehensive and mirrors the design of the create operation. It confirms that updates (such as quantity changes) are handled via a dedicated request body using theSubscriptionAddonUpdateschema.
- Suggestion: It might be helpful to include additional notes (or even a comment within your documentation) on how partial updates are processed—for example, whether changing the quantity always creates a new timeline entry.
8995-8997: Cancellation Endpoint Inclusion
The POST endpoint for canceling a subscription (/api/v1/subscriptions/{subscriptionId}/cancel) appears at the end of the hunk. If this endpoint was not directly part of the subscription addon work, please verify that its inclusion does not confuse its purpose.
18495-18653: Schema: SubscriptionAddon
The newSubscriptionAddonschema is detailed and well organized with all required fields, appropriate patterns, examples, and read-only attributes where necessary. The nestedaddonobject and timeline array are defined clearly.
- Suggestion: Review the timeline example to ensure it aligns with the business logic expected for continuous periods.
api/spec/src/productcatalog/subscriptionaddon.tsp (3)
110-113: Consider adding examples for rateCards propertyThe rateCards property lacks examples unlike other complex properties in this model (like timeline). Adding examples would improve documentation and make the API easier to understand for consumers.
@visibility(Lifecycle.Read) @summary("Rate cards") + @example(#[ + #{ + rateCard: #{ + id: "01HXYZ123ABC456DEF789GHIJK", + key: "base_rate", + name: "Base Rate", + currency: "USD", + unitAmounts: #[#{amount: 1000, unitAmount: 500}] + }, + affectedSubscriptionItemIds: ["01HXYZ123ABC456DEF789GHIJK"] + } + ]) rateCards: SubscriptionAddonRateCard[];
119-132: Consider adding validation for affectedSubscriptionItemIds arrayThe
affectedSubscriptionItemIdsproperty could benefit from validation to ensure it's not empty when provided./** * The IDs of the subscription items that this rate card belongs to. */ @summary("Affected subscription item IDs") @visibility(Lifecycle.Read) + @minItems(1) affectedSubscriptionItemIds: ULID[];
98-99: Add validation to ensure timeline entries are chronologically orderedThe comment states "The returned periods are sorted and continuous," but there's no validation to enforce this constraint.
Consider adding explicit validation or a clearer documentation note explaining how this ordering is enforced in the implementation, as this appears to be a critical business rule that should be consistently maintained.
api/spec/src/productcatalog/routes.tsp (2)
477-478: Clarify the uniqueness constraint in the list operation documentationThe comment "In the returned list will match to a set unique by addonId" is not clear and appears grammatically incorrect.
/** - * List all addons of a subscription. In the returned list will match to a set unique by addonId. + * List all addons of a subscription. Each addon in the returned list is uniquely identified by its addonId. */
501-502: Improve documentation clarity for update operationThe current description could be more precise about what the update operation allows.
/** - * Updates a subscription addon (allows changing the quantity: purchasing more instances or cancelling the current instances) + * Updates a subscription addon. Allows changing the quantity (purchasing more instances) + * or setting quantity to zero (effectively cancelling the current instances). */api/openapi.yaml (4)
8424-8507: Subscription Addon Creation Endpoint (POST)
The endpoint is clearly defined with a required path parameter (subscriptionId)—including a strict regex pattern—and a request body linked to theSubscriptionAddonCreateschema. All common response status codes (201, 400, 401, etc.) are documented.Consider providing an example payload within the request body documentation to further aid API consumers.
8507-8578: List Subscription Addons Endpoint (GET)
This endpoint effectively retrieves a list of subscription addons, returning an array ofSubscriptionAddonobjects.The description "List all addons of a subscription. In the returned list will match to a set unique by addonId" could be rephrased for clarity (e.g., "Lists all subscription addons, ensuring each addon is unique by addonId").
8657-8731: Update Subscription Addon Endpoint (PATCH)
The PATCH endpoint supports updates via a request body based on theSubscriptionAddonUpdateschema. The description indicates that it supports quantity changes and cancellation actions.Consider clarifying the behavior for "cancelling the current instances" to remove any ambiguity for API consumers.
18814-18899: SubscriptionAddonCreate Schema Definition
TheSubscriptionAddonCreateschema correctly lists the required fields for creation—such asname,activeFrom,addon,quantity, andtiming.Review the nested
addonproperty: the absence of a separatenamefield for the addon resource is noted; please confirm that this omission is intentional and adequately documented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Makefile(1 hunks)api/client/javascript/src/client/schemas.ts(4 hunks)api/openapi.cloud.yaml(2 hunks)api/openapi.yaml(2 hunks)api/spec/src/cloud/main.tsp(1 hunks)api/spec/src/productcatalog/routes.tsp(2 hunks)api/spec/src/productcatalog/subscriptionaddon.tsp(1 hunks)openmeter/server/router/subscriptionaddon.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- api/spec/src/cloud/main.tsp
- openmeter/server/router/subscriptionaddon.go
🧰 Additional context used
🧬 Code Definitions (1)
api/client/javascript/src/client/schemas.ts (2)
api/api.gen.go (5)
SubscriptionAddon(5402-5462)SubscriptionAddonCreate(5465-5501)SubscriptionAddonRateCard(5504-5510)SubscriptionAddonTimelineSegment(5513-5522)SubscriptionAddonUpdate(5525-5549)api/client/go/client.gen.go (5)
SubscriptionAddon(5084-5144)SubscriptionAddonCreate(5147-5183)SubscriptionAddonRateCard(5186-5192)SubscriptionAddonTimelineSegment(5195-5204)SubscriptionAddonUpdate(5207-5231)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test
- GitHub Check: Quickstart
- GitHub Check: Lint
- GitHub Check: E2E
- GitHub Check: Developer environment
- GitHub Check: CI
- GitHub Check: Commit hooks
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (23)
api/openapi.cloud.yaml (5)
8835-8912: Endpoint for Retrieving a Subscription Addon
The GET endpoint for/api/v1/subscriptions/{subscriptionId}/addons/{subscriptionAddonId}is defined correctly. The path parameters and response details (including error responses) are consistent with the other endpoints.
18654-18739: Schema: SubscriptionAddonCreate
TheSubscriptionAddonCreatemodel is correctly defined with the required properties. The inclusion of fields liketiming(referencingSubscriptionTiming) is well integrated.
18740-18760: Schema: SubscriptionAddonRateCard
This schema is implemented correctly, referencing theRateCardschema and ensuring thataffectedSubscriptionItemIdsadheres to the defined pattern.
18761-18783: Schema: SubscriptionAddonTimelineSegment
The timeline segment schema adequately captures the period (withactiveFromandactiveTo) and the associated quantity. It is clear and consistent with the other models.
18784-18828: Schema: SubscriptionAddonUpdate
TheSubscriptionAddonUpdateschema lists all optional fields correctly and is structured to allow partial updates. Ensure that your backend logic properly handles these optional values.api/spec/src/productcatalog/routes.tsp (1)
483-485:Details
❓ Verification inconclusive
Address pagination question for subscription addons list endpoint
There's a comment asking "Should we implement pagination? This should never be a large list." Consider making an explicit decision about pagination.
While the list of addons for a single subscription might be small now, consider future scalability:
- If you expect the list to always remain small (e.g., <100 items), you can remove the comment and document this decision.
- If there's a possibility of growth, implement pagination for consistency with other list endpoints.
Consider running a query to check similar endpoints in the codebase:
🏁 Script executed:
#!/bin/bash # Look for other list endpoints to see if they use pagination rg -A 10 "@operationId\(\"list" api/spec/src/productcatalog/routes.tsp | grep -E "QueryPagination|@list"Length of output: 105
Pagination Decision Needed for Subscription Addons Endpoint
It appears there’s no clear, established pagination pattern in this file. If you’ve confirmed that the list of addons for a subscription will consistently remain small (e.g. <100 items), please update the comment to document this deliberate decision and remove the pagination query note. However, if there’s any potential for growth or for consistency with other list endpoints (even if they aren’t using pagination yet), consider implementing pagination.
Please manually verify whether similar endpoints elsewhere in the codebase follow a pagination pattern before finalizing this decision.
api/openapi.yaml (5)
8579-8656: Get Subscription Addon Endpoint (GET)
The endpoint for retrieving a specific subscription addon is well-defined with clear path parameters for bothsubscriptionIdandsubscriptionAddonId. The response schema is correctly referenced asSubscriptionAddon.
18655-18813: SubscriptionAddon Schema Definition
TheSubscriptionAddonschema is comprehensive, including required fields such asid,name,createdAt,updatedAt, and related properties. The field constraints (including regex patterns and examples) improve clarity and data validation.Ensure that field names (e.g.,
quantityAt) accurately convey their purpose. If necessary, consider additional clarification in the property descriptions.
18900-18921: SubscriptionAddonRateCard Schema Definition
TheSubscriptionAddonRateCardschema is well-defined, requiring both therateCardandaffectedSubscriptionItemIdsproperties. Reusing the$ref: '#/components/schemas/RateCard'ensures consistency and reusability in your API design.
18922-18943: SubscriptionAddonTimelineSegment Schema Definition
This schema clearly specifies the timeline segment with required properties (activeFromandquantity) and includes a helpful example.
18944-18988: SubscriptionAddonUpdate Schema Definition
TheSubscriptionAddonUpdateschema allows for flexible updates by including optional fields likename,description,metadata, and timing. Its structure is consistent with pattern usage in the create schema.api/client/javascript/src/client/schemas.ts (12)
1776-1799: API endpoints for subscription addons look well-structured.The new API endpoints for subscription addons follow RESTful principles with appropriate HTTP methods and hierarchical resource paths. The documentation is clear and descriptive.
1800-1823: LGTM: Detailed endpoint for individual subscription addon operations.The endpoint for retrieving and updating a specific subscription addon is well-defined with appropriate HTTP methods and comprehensive documentation.
8015-8150: Well-structured SubscriptionAddon schema with comprehensive documentation.The SubscriptionAddon schema is thoroughly documented with appropriate field types, descriptions, and examples. The readonly fields are correctly marked, and the schema structure aligns well with the corresponding Go implementation in api/api.gen.go.
8151-8219: Create operation model follows the established pattern.The SubscriptionAddonCreate schema properly defines all necessary fields for creating a new subscription addon. The required fields are appropriately marked, and all fields have clear descriptions.
8220-8252: Rate card and timeline segment schemas are properly defined.Both the SubscriptionAddonRateCard and SubscriptionAddonTimelineSegment schemas are well-structured with clear field definitions and appropriate documentation.
8253-8286: Verify the addon field type in SubscriptionAddonUpdate.The
addonfield is typed asRecord<string, never>, which means an empty object. This doesn't match the Go implementation in api/api.gen.go which uses*map[string]interface{}(nullable map of string to any value). Consider updating the TypeScript type to better reflect the Go implementation.- addon?: Record<string, never> + addon?: Record<string, unknown>
8287-8298: Update operation model structure is consistent.The remainder of the SubscriptionAddonUpdate schema is well-structured with appropriate optional fields.
9590-9598: Type exports are properly defined.All the new schema types are correctly exported, making them available for use throughout the codebase.
20840-20933: List operation is well-defined with comprehensive error handling.The listSubscriptionAddons operation definition includes appropriate parameters and response types, with comprehensive error handling for all relevant HTTP status codes.
20934-21039: Create operation includes all necessary request and response definitions.The createSubscriptionAddon operation is well-structured with appropriate request body validation and comprehensive error responses, including conflict handling.
21040-21134: Get operation provides clear path parameter requirements.The getSubscriptionAddon operation correctly defines the required path parameters and includes comprehensive error handling.
21135-21234: Update operation follows consistent patterns with other API definitions.The updateSubscriptionAddon operation correctly defines path parameters, request body, and response types with appropriate error handling.
| @route("/api/v1/subscriptions/{subscriptionId}/addons") | ||
| @tag("Subscriptions") | ||
| interface SubscriptionAddons { | ||
| /** | ||
| * Create a new subscription addon, either providing the key or the id of the addon. | ||
| */ | ||
| @post | ||
| @operationId("createSubscriptionAddon") | ||
| @summary("Create subscription addon") | ||
| @extension("x-internal", true) | ||
| create( | ||
| @path subscriptionId: ULID, | ||
|
|
||
| @body | ||
| request: TypeSpec.Rest.Resource.ResourceCreateModel<SubscriptionAddon>, | ||
| ): | ||
| | { | ||
| @statusCode _: 201; | ||
| @body body: SubscriptionAddon; | ||
| } | ||
| | OpenMeter.ConflictError | ||
| | OpenMeter.NotFoundError | ||
| | OpenMeter.CommonErrors; | ||
|
|
||
| /** | ||
| * List all addons of a subscription. In the returned list will match to a set unique by addonId. | ||
| */ | ||
| @get | ||
| @operationId("listSubscriptionAddons") | ||
| @summary("List subscription addons") | ||
| @extension("x-internal", true) | ||
| list( | ||
| @path subscriptionId: ULID, // Should we implement pagination? This should never be a large list. | ||
| ): SubscriptionAddon[] | OpenMeter.NotFoundError | OpenMeter.CommonErrors; | ||
|
|
||
| /** | ||
| * Get a subscription addon by id. | ||
| */ | ||
| @get | ||
| @route("/{subscriptionAddonId}") | ||
| @operationId("getSubscriptionAddon") | ||
| @summary("Get subscription addon") | ||
| @extension("x-internal", true) | ||
| get( | ||
| @path subscriptionId: ULID, | ||
| @path subscriptionAddonId: ULID, | ||
| ): SubscriptionAddon | OpenMeter.NotFoundError | OpenMeter.CommonErrors; | ||
|
|
||
| /** | ||
| * Updates a subscription addon (allows changing the quantity: purchasing more instances or cancelling the current instances) | ||
| */ | ||
| @patch | ||
| @route("/{subscriptionAddonId}") | ||
| @operationId("updateSubscriptionAddon") | ||
| @summary("Update subscription addon") | ||
| @extension("x-internal", true) | ||
| update( | ||
| @path subscriptionId: ULID, | ||
| @path subscriptionAddonId: ULID, | ||
|
|
||
| @body | ||
| body: TypeSpec.Rest.Resource.ResourceCreateOrUpdateModel<SubscriptionAddon>, | ||
| ): SubscriptionAddon | OpenMeter.NotFoundError | OpenMeter.CommonErrors; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation parameters for create and update operations
The create and update operations don't have explicit validation for the request body, particularly for validating that quantity is 1 for single instance add-ons.
Consider adding validation logic in these operations to ensure that the requested quantity matches the constraints of the addon's instanceType. This would help prevent invalid requests from reaching your implementation.
| /api/v1/subscriptions/{subscriptionId}/cancel: | ||
| post: | ||
| operationId: cancelSubscription |
There was a problem hiding this comment.
Cancel Subscription Endpoint Incomplete
The /api/v1/subscriptions/{subscriptionId}/cancel endpoint is only partially defined (only a post operation with an operationId is provided).
Please complete the definition by adding parameters, a detailed description, request body (if applicable), and response schemas—or clearly mark it as a placeholder if intended for future implementation.
| SubscriptionAlignment: | ||
| type: object | ||
| properties: |
There was a problem hiding this comment.
Incomplete SubscriptionAlignment Schema
The SubscriptionAlignment schema appears to be incomplete—it specifies type: object and begins a properties: block without any defined properties.
Please provide a full definition for this schema (or remove it if not needed) to avoid leaving API consumers with an undefined object structure.
2 participants
SubscriptionAddon API
Defines types & API routes for SubscriptionAddons (Addon instances).
Summary by CodeRabbit
Summary by CodeRabbit
New Features
API Enhancements