-
Notifications
You must be signed in to change notification settings - Fork 152
fix(api): planaddon spec #2685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(api): planaddon spec #2685
Conversation
📝 WalkthroughWalkthroughThis change adds an optional 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 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (9)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/openapi.cloud.yaml (1)
17954-17956: Enhance descriptions for clarity
The new descriptions on theaddonproperty and the overallPlanAddonCreateschema improve discoverability. Please verify the indentation underpropertiesremains consistent with other component definitions.api/client/javascript/src/client/schemas.ts (1)
7271-7281: Improve field order for better readability.The required
addonfield has been moved after the optionalmetadatafield. While this doesn't affect functionality, it's generally a good practice to place required fields before optional ones for better developer experience.PlanAddonCreate: { /** + * Addon + * @description The add-on to create. + */ + addon: { + /** + * @description The ID of the add-on. + * @example 01G65Z755AFWAKHE12NY0CQ9FH + */ + id: string + } + /** * Metadata * @description Additional metadata for the resource. */ metadata?: components['schemas']['Metadata'] - /** - * Addon - * @description The add-on to create. - */ - addon: { - /** - * @description The ID of the add-on. - * @example 01G65Z755AFWAKHE12NY0CQ9FH - */ - id: string - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/api.gen.go(5 hunks)api/client/go/client.gen.go(5 hunks)api/client/javascript/src/client/schemas.ts(6 hunks)api/openapi.cloud.yaml(5 hunks)api/openapi.yaml(5 hunks)api/spec/src/productcatalog/planaddon.tsp(2 hunks)api/spec/src/productcatalog/routes.tsp(2 hunks)openmeter/productcatalog/planaddon/httpdriver/mapping.go(4 hunks)openmeter/productcatalog/planaddon/httpdriver/planaddon.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
openmeter/productcatalog/planaddon/httpdriver/planaddon.go (3)
api/client/javascript/src/client/schemas.ts (1)
PlanAddonReplaceUpdate(9855-9856)api/api.gen.go (1)
PlanAddonReplaceUpdate(5007-5017)api/client/go/client.gen.go (1)
PlanAddonReplaceUpdate(4560-4570)
api/client/go/client.gen.go (3)
api/api.gen.go (2)
Metadata(4277-4277)PlanAddonReplaceUpdate(5007-5017)openmeter/billing/profile.go (1)
Metadata(22-22)openmeter/productcatalog/plan/httpdriver/mapping.go (1)
FromPlanPhase(65-85)
api/api.gen.go (4)
api/client/go/client.gen.go (2)
Metadata(3873-3873)PlanAddonReplaceUpdate(4560-4570)openmeter/billing/profile.go (1)
Metadata(22-22)openmeter/productcatalog/plan/httpdriver/mapping.go (1)
FromPlanPhase(65-85)openmeter/ent/db/planaddon/where.go (1)
MaxQuantity(104-106)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: Developer environment
- GitHub Check: CI
- GitHub Check: Lint
- GitHub Check: Commit hooks
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (36)
openmeter/productcatalog/planaddon/httpdriver/planaddon.go (1)
156-156: Update handler to match renamed request type.The change from
api.PlanAddonUpdatetoapi.PlanAddonReplaceUpdatealigns with the renamed model in the API specifications and client libraries. This update properly handles the new model that includes an optionalMetadatafield.api/spec/src/productcatalog/planaddon.tsp (2)
21-26: Good addition of metadata support.Adding the
metadatafield to thePlanAddonmodel with appropriate visibility annotations for read, create, and update operations is well-structured. This matches the PR objective of supporting metadata in plan add-on assignments.
92-114: Well-structured model for create operations.The new
PlanAddonCreatemodel correctly inherits properties fromPlanAddonwhile customizing theaddonproperty to only include the ID field. This specialization provides better type safety for create operations.api/spec/src/productcatalog/routes.tsp (2)
219-219: Improved type safety for create operation.Changing from a generic
ResourceCreateModel<PlanAddon>to the more specificPlanAddonCreatemodel improves type safety and provides better documentation of the expected request structure.
240-240: Updated update operation to use proper resource model.Switching to
TypeSpec.Rest.Resource.ResourceReplaceModel<PlanAddon>correctly aligns with the renamed and restructured update model. This change ensures consistent handling of metadata fields in update operations.api/client/go/client.gen.go (5)
4512-4517: Good addition of metadata field to thePlanAddonstructAdding the optional metadata field to the plan add-on model aligns with the PR objective of enhancing the API spec for plan add-on assignments.
4522-4539: Good improvement to struct documentation and metadata field additionThe updated comment clarifies this is a "plan add-on assignment create request" and the metadata field is added consistently with the same pattern used elsewhere in the codebase.
4559-4570: Appropriate model rename and metadata field additionRenaming from
PlanAddonUpdatetoPlanAddonReplaceUpdatebetter reflects that this is a replace operation. The updated comment and metadata field addition match the PR's stated purpose.
7131-7131: Correctly updated type alias for the renamed structThe type alias
UpdatePlanAddonJSONRequestBodynow references the renamedPlanAddonReplaceUpdatestruct, maintaining consistency throughout the API.
37035-37053: Updated encoded OpenAPI specificationThis change updates the encoded OpenAPI specification that's embedded in the generated client code to reflect the model changes.
api/api.gen.go (6)
4962-4963: Addition of Metadata field to PlanAddon structThe addition of the
Metadatafield to thePlanAddonstruct aligns with the PR objective of adding metadata support to plan add-on assignments. The field is properly defined as an optional pointer to theMetadatatype.
4984-4985: Addition of Metadata field to PlanAddonCreate structGood addition of the
Metadatafield to the creation request model, maintaining consistency across the API. This ensures metadata can be specified when creating plan add-on assignments.
5006-5007: Renamed PlanAddonUpdate to PlanAddonReplaceUpdate with Metadata fieldThe struct has been appropriately renamed from
PlanAddonUpdatetoPlanAddonReplaceUpdateto better reflect its purpose, and theMetadatafield has been added, consistent with other model changes.Also applies to: 5015-5016
7640-7641: Updated RequestBody type to use renamed structThe type alias for the update request body has been properly updated to use the renamed struct
PlanAddonReplaceUpdate.
16869-17091: Updated embedded Swagger specificationThe embedded Swagger specification has been updated to reflect the model changes, including the addition of the metadata field and the renaming of the update model.
17092-17424:Details
❓ Verification inconclusive
Verify impact of changes on existing clients
As these changes modify the API contract by renaming types and adding new fields, ensure that existing clients are updated or can handle these changes gracefully.
Run the following script to check for any direct usage of the old type name:
🏁 Script executed:
#!/bin/bash # Search for any references to PlanAddonUpdate that might need to be updated echo "Checking for references to the old type name PlanAddonUpdate:" rg -A 2 -B 2 "PlanAddonUpdate" --type goLength of output: 28422
Verify PlanAddonUpdate rename and new fields across the codebase
We ran a search for references to the old type name
PlanAddonUpdateand found it still used in several internal packages. Before merging breaking changes to the API contract, please ensure existing clients and integrations are updated (or made backward‑compatible) for the renamed type and any added fields:• productcatalog/planaddon/assert.go
• productcatalog/planaddon/event.go
• productcatalog/planaddon/adapter/adapter_test.go
• productcatalog/planaddon/service/service_test.go
• productcatalog/planaddon/service/planaddon.go
• ent/db/client.go
• ent/db/planaddon_update.go
• ent/db/setorclear.goIf your API clients import any of these types or rely on JSON payloads emitted by the service, update them accordingly or provide compatibility shims.
api/openapi.yaml (5)
18247-18251: Added optionalmetadatafield toPlanAddonschema
You’ve correctly extended thePlanAddonschema with an optionalmetadataproperty using the sharedMetadatacomponent. It’s not inrequired, so existing clients won’t break.
18304-18314: EnhancedPlanAddonCreatewithaddonrequirement andmetadatasupport
- Added
- addonto therequiredlist, ensuring the add-on ID must be provided.- Introduced an optional
metadataproperty, composed viaallOffromMetadata.- Reordered
fromPlanPhaseandaddonto match the intended operation sequence.
18331-18333: Documentedaddonobject and create request
- Added a clear
descriptionandtitlefor the inlineaddonobject.- Provided a top‑level
descriptionfor thePlanAddonCreaterequest schema.
This improves readability and auto‑generated docs.
18369-18378: IntroducedPlanAddonReplaceUpdatewithmetadata
- Defined the
PlanAddonReplaceUpdateschema, matching the renamed update model.- Included an optional
metadataproperty composed from the sharedMetadataschema.- Kept
fromPlanPhaseas the sole required field, since you cannot change the add‑on ID during an update.
7164-7164:Details
✅ Verification successful
Updated schema reference for PUT operation
The PUT operation now correctly references the newPlanAddonReplaceUpdateschema, aligning with the renamed update model.As a sanity check, ensure there are no remaining references to the old
PlanAddonUpdateschema across the spec:
🏁 Script executed:
#!/bin/bash # Verify no residual PlanAddonUpdate references rg -n "PlanAddonUpdate"Length of output: 9679
🏁 Script executed:
#!/bin/bash # Search for any lingering references to the old schema in the OpenAPI spec rg -n "PlanAddonUpdate" api/openapi.yamlLength of output: 40
Confirm no lingering PlanAddonUpdate references
Ranrg -n "PlanAddonUpdate" api/openapi.yamlagainst the OpenAPI spec—no matches found. The spec is clean.api/openapi.cloud.yaml (4)
7165-7165: Rename request schema toPlanAddonReplaceUpdate
The request body schema reference has been updated to the newly namedPlanAddonReplaceUpdate, which aligns with the PR’s goal of unifying the update model.
17870-17874: Add optionalmetadatato thePlanAddonschema
Introducing themetadatafield viaallOfwith the sharedMetadataschema correctly makes it optional and non‑breaking for existing clients.
17927-17937: IncludemetadatainPlanAddonCreaterequest
ThePlanAddonCreateschema now exposesmetadataand explicitly requiresaddon, ensuring that clients must provide the add‑on while keeping metadata optional.
17992-18001: Verify fields inPlanAddonReplaceUpdateschema
ThePlanAddonReplaceUpdatemodel currently requires onlyfromPlanPhaseand addsmetadataas optional. Confirm that omitting fields likeaddon(or other properties) fromrequiredis intentional for the replace/update contract, and that clients can still perform full replacements as expected.openmeter/productcatalog/planaddon/httpdriver/mapping.go (6)
4-5: Good addition of the lo library.Adding the
github.com/samber/lopackage provides useful utility functions likeToPtrthat simplify pointer handling in Go.
30-30: LGTM - Consistent metadata field handling.The addition of the metadata field to the response structure properly aligns with the PR objectives to add metadata support for plan add-on assignments.
50-62: Clean implementation of metadata conversion.The
FromMetadatafunction follows the same pattern as the existingFromAnnotationsfunction, maintaining code consistency while properly handling nil/empty metadata cases.
65-69: Looks good - Proper nil metadata handling.The implementation properly checks if metadata is nil before attempting to convert it, preventing potential nil pointer dereferences.
Also applies to: 75-75
85-91: API signature update properly implemented.The function signature has been correctly updated to use
api.PlanAddonReplaceUpdateinstead ofapi.PlanAddonUpdate, aligning with the broader API changes mentioned in the summary. The metadata handling is consistent with other functions while usinglo.ToPtrappropriately for pointer conversion.Also applies to: 96-96
106-118: Clean implementation of reverse metadata conversion.The
AsMetadatafunction properly handles the conversion from API to internal model metadata, following the same pattern as other conversion functions and correctly handling empty maps.api/client/javascript/src/client/schemas.ts (5)
7208-7212: LGTM: Added metadata field to PlanAddon interface.The addition of an optional metadata field with proper JSDoc documentation aligns with the PR objectives to support metadata in plan add-on assignments.
7253-7259: LGTM: Added JSDoc and metadata field to PlanAddonCreate model.The metadata field has been properly added with descriptive JSDoc comments, maintaining code consistency with other API models.
7309-7314: LGTM: Renamed and updated PlanAddonReplaceUpdate model.The model has been renamed from PlanAddonUpdate to PlanAddonReplaceUpdate and enhanced with the metadata field. This change improves semantics by clarifying that this is for a replace operation.
9855-9856: LGTM: Updated type export for renamed model.The exported type has been correctly updated to match the renamed schema.
19219-19219: LGTM: Updated request body schema reference.The operation now correctly references the renamed PlanAddonReplaceUpdate schema, maintaining consistency across the API.
fe68fce to
965ee72
Compare
Overview
Fix plan add-on assignment HTTP API spec:
metadataSummary by CodeRabbit