feat(api): define routes for customer entitlements #3134
Conversation
📝 WalkthroughWalkthroughThis change introduces a comprehensive customer entitlement management feature across the API specification, server interface, HTTP routing, client schemas, and validation logic. It adds endpoints and supporting types for creating, listing, retrieving, deleting, overriding, and resetting customer entitlements and their grants, as well as querying entitlement history. Most server handlers are currently unimplemented stubs. Changes
Estimated code review effort5 (~150 minutes) 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. 🔧 Biome (1.9.4)api/client/javascript/src/zod/index.tsThe --json option is unstable/experimental and its output might change between patches/minor releases. ... [truncated 99999116 characters] ... ueryPageDefault)\n .describe('Page index.\n\nDefault is 1.'),\n pageSize: zod.coerce\n .number()\n .min(1)\n .max(listMarketplaceListingsQueryPageSizeMax)\n .default(listMarketplaceListingsQueryPageSizeDefault)\n .describe('The maximum number of items per page.\n\nDefault is 100.'),\n})\n\n/**\n * Get a marketplace listing by type.\n * @sBiome encountered an unexpected error This is a bug in Biome, not an error in your code, and we would appreciate it if you could report it to https://github.com/biomejs/biome/issues/ along with the following information to help us fixing the issue: Source Location: crates/biome_console/src/lib.rs:149:14 ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 12
🧹 Nitpick comments (6)
openmeter/server/router/entitlement.go (1)
122-126: Placeholder implementation - ensure tracking for future development.This handler correctly defines the route structure but calls an unimplemented function. Consider tracking this in your project management system to ensure implementation is completed.
api/client/javascript/src/zod/index.ts (1)
7485-7686: Extract common schema fields to reduce duplication.The three entitlement type schemas share many common fields (featureId, featureKey, metadata, usagePeriod) with identical validation rules. This duplication makes the code harder to maintain.
Consider extracting common fields into a shared schema:
const entitlementCommonFields = { featureId: zod.coerce .string() .regex(createCustomerEntitlementBodyFeatureIdRegExp) .optional() .describe( 'The feature the subject is entitled to use.\nEither featureKey or featureId is required.' ), featureKey: zod.coerce .string() .min(1) .max(createCustomerEntitlementBodyFeatureKeyMax) .regex(createCustomerEntitlementBodyFeatureKeyRegExp) .optional() .describe( 'The feature the subject is entitled to use.\nEither featureKey or featureId is required.' ), metadata: zod .record(zod.string(), zod.coerce.string()) .describe( 'Set of key-value pairs.\nMetadata can be used to store additional information about a resource.' ) .optional() .describe('Additional metadata for the feature.'), }; const usagePeriodSchema = zod .object({ anchor: zod.coerce .date() .optional() .describe('A date-time anchor to base the recurring period on.'), interval: zod.coerce .string() .or( zod .enum(['DAY', 'WEEK', 'MONTH', 'YEAR']) .describe( 'The unit of time for the interval.\nOne of: `day`, `week`, `month`, or `year`.' ) ) .describe('Period duration for the recurrence') .describe('The unit of time for the interval.'), }) .describe('Recurring period with an interval and an anchor.');Then use object spread in the discriminated union branches to include these common fields.
api/client/javascript/src/client/schemas.ts (1)
16309-16312: Consider using a single date type for consistencyThe
fromandtoparameters use a union typeDate | string, which could lead to type ambiguity. Consider standardizing onstringformat (ISO 8601) for API consistency, as dates are typically serialized as strings in JSON APIs.Apply this diff for consistency:
- from?: Date | string - to?: Date | string + from?: string + to?: stringapi/openapi.cloud.yaml (1)
4013-4017: Spelling & grammatical polishMinor typos in public documentation sections can erode perceived quality:
accross➔acrosscontinous➔continuousseperated➔separatedzerod➔zeroeddoesnt➔doesn'tWindowsize(description) ➔Window sizeFixing these before publishing the spec avoids propagating errors into generated SDK docs.
Also applies to: 4015-4016, 4015-4016
api/openapi.yaml (2)
4012-4017: Several typos in the history endpoint description
accross,continous,seperated➜across,continuous,separated.
Minor, but these leak into generated docs and SDK JSDoc.
3548-3556: Consider advertising501while handlers are stubbed
All new routes return rich error objects, but there is no501 Not Implementedresponse.
Until server logic lands, explicitly modelling501helps client generators and avoids silently mapping every un-implemented call to500.Example:
+ '501': + description: This operation is defined but not yet implemented. + content: + application/problem+json: + schema: + $ref: '#/components/schemas/NotImplementedProblemResponse'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/api.gen.go(7 hunks)api/client/javascript/src/client/schemas.ts(2 hunks)api/client/javascript/src/zod/index.ts(1 hunks)api/openapi.cloud.yaml(1 hunks)api/openapi.yaml(1 hunks)api/spec/src/cloud/main.tsp(1 hunks)api/spec/src/entitlements/customer.tsp(2 hunks)openmeter/server/router/entitlement.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Artifacts / Container image
- GitHub Check: Artifacts / Benthos Collector Container image
- GitHub Check: CI
- GitHub Check: E2E
- GitHub Check: Quickstart
- GitHub Check: Commit hooks
- GitHub Check: Lint
- GitHub Check: Developer environment
- GitHub Check: Migration Checks
- GitHub Check: Build
- GitHub Check: Test
- GitHub Check: Analyze (go)
🔇 Additional comments (24)
api/spec/src/cloud/main.tsp (2)
185-189: Well-structured interface definition following established conventions.The
CustomerEntitlementsinterface properly extends the base interface and follows the consistent patterns used throughout the file for route mapping, tagging, and naming conventions.
191-195: Consistent interface definition with appropriate resource identification.The
CustomerEntitlementinterface correctly usesfeatureKeyas the path parameter, which aligns with entitlement domain semantics and maintains consistency with the established API patterns.openmeter/server/router/entitlement.go (10)
114-120: Well-implemented handler following established patterns.The
GetCustomerAccesshandler properly delegates to the existing customer handler with appropriate parameter mapping and follows the consistent implementation pattern used throughout the router.
128-132: Properly structured placeholder with correct parameter handling.The handler structure and parameter types are correctly defined, maintaining consistency with other list operations in the router.
134-138: Correct parameter naming aligned with domain semantics.The use of
featureKeyas the identifier parameter is semantically appropriate for entitlement operations and consistent with the API specification.
140-144: Consistent deletion handler structure.The handler properly follows the established pattern for deletion operations with appropriate HTTP method mapping and parameter handling.
146-150: Appropriate HTTP method and route structure for override operations.The use of PUT method with the
/overridesuffix clearly indicates the specialized nature of this operation while maintaining RESTful conventions.
152-156: Well-structured grant listing handler with proper parameter handling.The handler correctly nests grant operations under the customer entitlement context and maintains consistent parameter handling patterns.
158-162: Proper RESTful nesting of grant operations under entitlements.The handler correctly structures grants as sub-resources of entitlements, following established RESTful conventions and maintaining consistency with the API hierarchy.
164-171: Correctly implemented handler with proper parameter mapping.The handler properly delegates to the existing customer handler with appropriate parameter structure, maintaining consistency with other implemented handlers in the router.
173-177: History handler with appropriate GET method and parameter structure.The handler correctly uses GET method for history retrieval and maintains consistent parameter handling patterns with other customer entitlement operations.
179-183: Reset handler with proper POST method for state-changing operation.The handler appropriately uses POST method for the reset operation, which changes entitlement state, following established conventions for non-idempotent operations.
api/client/javascript/src/zod/index.ts (1)
7795-7908: Well-structured grant creation schema with comprehensive validation.The grant creation schema is properly designed with:
- Appropriate validation rules for amounts, priorities, and dates
- Clear documentation explaining complex behaviors like rollover calculations
- Proper use of optional fields with sensible defaults
api/openapi.cloud.yaml (2)
4129-4132: Non-idempotent semantics on aPUTthat returns 201
overrideCustomerEntitlementuses HTTP verbPUTyet creates a brand-new resource and responds with201 Created.
PUT is expected to be idempotent and generally returns200or204when it replaces a resource at the addressed URI.
If the operation semantically creates a sibling resource (new entitlement ULID) consider:
- Switching to
POSTon/overrideor- Keeping
PUTbut returning200 OK/204 No Contentand treating the override as true replacement.This matters for client caching, automatic retries, and OpenAPI tooling that infers idempotency.
Also applies to: 4175-4179
3856-3863:orderBydefault undocumented in schema
GrantOrderByis referenced withdefault: updatedAton the parameter level. Ensure the underlying enum schema includesupdatedAt; otherwise generators may treat the default as invalid and emit warnings.
If the schema already covers it, disregard.api/spec/src/entitlements/customer.tsp (2)
10-24: LGTM!The
Customerinterface is well-designed with appropriate HTTP semantics and clear documentation.
1-234: Well-structured API specification with comprehensive documentationThe customer entitlements API specification is well-designed with clear separation of concerns, proper HTTP semantics, and detailed documentation. The endpoints cover all necessary CRUD operations and advanced features like grants, history, and usage resets.
The only systematic issue is the inconsistent parameter typing mentioned in the previous comments.
api/api.gen.go (7)
7550-7575: LGTM! Well-structured parameter definitions.The parameter structs are properly defined with appropriate Go struct tags, correct use of pointer types for optional parameters, and good documentation. The required
WindowSizeparameter for history queries makes sense from an API design perspective.
8311-8321: LGTM! Consistent request body type aliases.The JSON request body type aliases follow the established naming convention and properly map to the underlying input types for all relevant operations.
11622-11648: LGTM! Comprehensive ServerInterface method definitions.The new methods provide complete CRUD and operational capabilities for customer entitlements. The method signatures are consistent with existing patterns and properly use the parameter structs defined earlier in the file.
12201-12253: LGTM! Proper unimplemented method stubs.The unimplemented stubs correctly return HTTP 501 status and have method signatures that exactly match the ServerInterface definitions. This follows the established pattern for generated code.
14434-14792: LGTM! Comprehensive middleware handler implementations.The middleware handlers properly extract and validate path and query parameters, implement consistent error handling, and correctly apply the middleware chain. The parameter binding logic correctly handles both required and optional parameters, with appropriate validation for the required
windowSizeparameter in the history endpoint.
18214-18240: LGTM! Well-designed RESTful route definitions.The routes follow proper RESTful patterns with logical URL hierarchy and correct HTTP method mappings. The route structure is consistent with existing API patterns and provides comprehensive coverage for customer entitlement operations.
19390-19644: Generated swagger specification content.This appears to be base64-encoded OpenAPI specification data. As generated content, it doesn't require detailed review and should be consistent with the API specification.
There was a problem hiding this comment.
Actionable comments posted: 15
🔭 Outside diff range comments (1)
openmeter/server/router/entitlement.go (1)
3-9: Missing import for unimplemented package.The code references
unimplementedpackage functions throughout the new handlers, but the import statement is missing. This will cause compilation errors.Add the missing import:
import ( "net/http" "github.com/openmeterio/openmeter/api" customerdriver "github.com/openmeterio/openmeter/openmeter/customer/httpdriver" entitlementdriver "github.com/openmeterio/openmeter/openmeter/entitlement/driver" + "github.com/openmeterio/openmeter/openmeter/server/router/unimplemented" )
🧹 Nitpick comments (12)
api/client/javascript/src/zod/index.ts (2)
7485-7684: Refactor discriminated union to reduce code duplicationThe schema has significant duplication across the three entitlement types. Consider extracting common fields into a base schema.
+// Base schema for common entitlement fields +const baseEntitlementSchema = zod.object({ + featureId: zod.coerce + .string() + .regex(ULID_REGEX) + .optional() + .describe( + 'The feature the subject is entitled to use.\nEither featureKey or featureId is required.' + ), + featureKey: zod.coerce + .string() + .min(1) + .max(FEATURE_KEY_MAX_LENGTH) + .regex(FEATURE_KEY_REGEX) + .optional() + .describe( + 'The feature the subject is entitled to use.\nEither featureKey or featureId is required.' + ), + metadata: zod + .record(zod.string(), zod.coerce.string()) + .describe( + 'Set of key-value pairs.\nMetadata can be used to store additional information about a resource.' + ) + .optional() + .describe('Additional metadata for the feature.'), + usagePeriod: zod + .object({ + anchor: zod.coerce + .date() + .optional() + .describe('A date-time anchor to base the recurring period on.'), + interval: zod.coerce + .string() + .or( + zod + .enum(['DAY', 'WEEK', 'MONTH', 'YEAR']) + .describe( + 'The unit of time for the interval.\nOne of: `day`, `week`, `month`, or `year`.' + ) + ) + .describe('Period duration for the recurrence') + .describe('The unit of time for the interval.'), + }) + .describe('Recurring period with an interval and an anchor.') + .optional() + .describe('The usage period associated with the entitlement.'), +}); export const createCustomerEntitlementBody = zod .discriminatedUnion('type', [ - zod.object({ - // ... duplicate fields ... + baseEntitlementSchema.extend({ type: zod.enum(['metered']), + usagePeriod: baseEntitlementSchema.shape.usagePeriod.unwrap(), isSoftLimit: zod.coerce.boolean().optional(), // ... other metered-specific fields ... - }), + }).describe('Create inputs for metered entitlement'), // Similar refactoring for boolean and static types ])
7524-7529: Track deprecated field removalThe
isUnlimitedfield is marked as deprecated. Consider adding a TODO comment with a target version/date for removal to ensure it doesn't remain indefinitely.Would you like me to create an issue to track the removal of this deprecated field across all schemas where it appears?
api/openapi.cloud.yaml (4)
3528-3542:EntitlementCreateInputsvsEntitlementCreateInput– naming driftCreate/Override endpoints reference
EntitlementCreateInputs(plural) whereas other input objects follow singular*Inputconvention (ResetEntitlementUsageInput,EntitlementGrantCreateInput).
Pick one form and stick to it to prevent duplicate models in generated SDKs.Also applies to: 3604-3612, 4205-4213
3860-3863: Placedefaultinside component schema, not in each parameter
orderByand similar parameters declaredefault: updatedAtdirectly.
OpenAPI tooling ignores per-parameterdefaultwhen$refpoints to a schema – put the default in#/components/schemas/GrantOrderByinstead to avoid divergence.Also applies to: 4102-4105
4015-4018: Typos in descriptive text
continous→continuous,seperated→separated,zerod→zeroed out.
Minor, but docs feed directly into client SDKs & portals.Also applies to: 4218-4221
3856-3863: Boolean/enum query params: unnecessaryexplode: false/style: formnoiseFor simple scalar types, the defaults are already
formwithexplode=true. Over-specifying with non-default combinations can confuse codegens (e.g., Java). Consider deleting those two lines unless you truly needexplode=false.Also applies to: 4054-4066
api/openapi.yaml (4)
3609-3611: Inconsistent schema naming (EntitlementCreateInputsvsEntitlementGrantCreateInput)The plural “Inputs” breaks the otherwise singular naming convention used for
...CreateInput. Aligning the names eliminates cognitive load and prevents accidental duplication if code-gen tooling lower-cases / singularises identifiers.- $ref: '#/components/schemas/EntitlementCreateInputs' + $ref: '#/components/schemas/EntitlementCreateInput'Replicate the rename in the schema definition and the
overriderequest body reference.Also applies to: 4211-4211
4012-4016: Fix typos in entitlement history description
accross,continous, andseperatedhurt professionalism and searchability.- Returns historical balance and usage data for the entitlement. The queried history can span accross multiple reset events. - BurndownHistory returns a continous history of segments, where the segments are seperated by events ... + Returns historical balance and usage data for the entitlement. The queried history can span across multiple reset events. + BurndownHistory returns a continuous history of segments, where the segments are separated by events ...
3621-3626: Add a short description forincludeDeletedquery parameterEvery other parameter is documented; this one is not. A one-liner such as “Whether to include soft-deleted entitlements in the response” keeps the API self-explaining.
4217-4219: Spellcheck: “zerod” → “zeroed”Minor polish for the reset endpoint description.
api/spec/src/entitlements/customer.tsp (2)
168-171: Fix typos in documentation.Found spelling errors in the documentation comments.
- * Returns historical balance and usage data for the entitlement. The queried history can span accross multiple reset events. + * Returns historical balance and usage data for the entitlement. The queried history can span across multiple reset events.- * BurndownHistory returns a continous history of segments, where the segments are seperated by events that changed either the grant burndown priority or the usage period. + * BurndownHistory returns a continuous history of segments, where the segments are separated by events that changed either the grant burndown priority or the usage period.
26-220: Well-designed API specification with comprehensive entitlement management.The API design effectively covers all aspects of customer entitlement management:
- ✅ Clear separation of concerns between entitlement lifecycle and grant management
- ✅ Comprehensive documentation explaining business rules and constraints
- ✅ Proper error handling with appropriate HTTP status codes
- ✅ Support for advanced features like grant recurrence, rollover, and historical queries
- ✅ Zero-downtime migration support via the override endpoint
The specification provides a solid foundation for implementing the customer entitlements feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/api.gen.go(7 hunks)api/client/javascript/src/client/schemas.ts(2 hunks)api/client/javascript/src/zod/index.ts(1 hunks)api/openapi.cloud.yaml(1 hunks)api/openapi.yaml(1 hunks)api/spec/src/cloud/main.tsp(1 hunks)api/spec/src/entitlements/customer.tsp(2 hunks)openmeter/server/router/entitlement.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: CI
- GitHub Check: E2E
- GitHub Check: Quickstart
- GitHub Check: Commit hooks
- GitHub Check: Lint
- GitHub Check: Developer environment
- GitHub Check: Build
- GitHub Check: Test
- GitHub Check: Analyze (go)
🔇 Additional comments (15)
api/spec/src/cloud/main.tsp (1)
185-195: Well-structured API interface definitions.The new
CustomerEntitlementsandCustomerEntitlementinterfaces follow established patterns in the codebase with appropriate routing, tagging, and extension structure. The routes logically organize customer entitlement operations under the customer namespace.openmeter/server/router/entitlement.go (4)
114-120: Proper implementation pattern for existing functionality.The
GetCustomerAccesshandler correctly delegates to the existing customer handler with appropriate parameter mapping, following established patterns in the codebase.
122-162: Unimplemented handlers align with PR objectives.The customer entitlement CRUD operations are appropriately stubbed with
unimplementedplaceholders, which matches the PR objective of defining routes with unimplemented responses. The handler signatures and parameter passing are correctly structured for future implementation.
164-171: Existing entitlement value functionality properly integrated.The
GetCustomerEntitlementValuehandler correctly leverages existing customer handler functionality with proper parameter mapping, maintaining consistency with the established implementation patterns.
173-183: Additional unimplemented endpoints follow consistent pattern.The entitlement history and usage reset handlers are appropriately stubbed and follow the same consistent pattern as other unimplemented handlers in this file.
api/client/javascript/src/client/schemas.ts (2)
772-930: Well-structured REST API path definitionsThe new customer entitlement endpoints follow REST conventions properly with clear resource hierarchy and appropriate HTTP methods. The JSDoc comments provide good documentation for each endpoint's purpose.
15744-16614: Comprehensive operation definitions with proper error handlingThe operation definitions are well-structured with:
- Complete request/response type definitions
- Consistent error response patterns across all operations
- Appropriate use of schema components
- Clear parameter specifications for path and query parameters
The required
windowSizeparameter forgetCustomerEntitlementHistoryis appropriately defined for time-series data queries.api/spec/src/entitlements/customer.tsp (2)
14-24: LGTM! Well-structured endpoint definition.The new
getCustomerAccessendpoint is properly defined with appropriate decorators, clear documentation, and comprehensive error handling.
222-233: LGTM! Clear and well-structured model definition.The
CustomerAccessmodel is appropriately designed with:
- Clear documentation explaining the data structure
- Proper use of
Record<EntitlementValue>for flexible key-value mapping- Correct read-only visibility constraint
api/api.gen.go (6)
7550-7576: LGTM: Parameter struct definitions are well-structured.The parameter structs follow consistent patterns with proper use of pointer types for optional parameters and include helpful documentation comments, especially for the time-based parameters in
GetCustomerEntitlementHistoryParams.
8311-8322: LGTM: Request body type aliases follow established patterns.The type aliases provide clear mappings between HTTP request bodies and Go types with consistent naming conventions.
12201-12254: LGTM: Stub implementations follow correct pattern.The unimplemented stub methods correctly return HTTP 501 status, which is appropriate for endpoints that haven't been implemented yet. This follows the established pattern in the codebase.
14434-14793: LGTM: Middleware handlers are well-implemented.The middleware handlers follow consistent patterns with proper parameter binding, error handling, and middleware chain application. The implementation correctly handles both path and query parameters with appropriate validation.
18214-18241: LGTM: Route registrations follow RESTful conventions.The HTTP route registrations use appropriate HTTP methods and follow logical RESTful patterns for customer entitlement CRUD operations. The route structure is well-organized and consistent.
19387-19645: Generated embedded API specification data.This appears to be embedded OpenAPI specification data that was automatically generated. This type of embedded content is typical for generated API code and should not be manually modified.
| .describe('Recurring period with an interval and an anchor.') | ||
| .describe('The usage period associated with the entitlement.'), | ||
| }) | ||
| .describe('Create inpurs for metered entitlement'), |
There was a problem hiding this comment.
Fix typo in schema description
- .describe('Create inpurs for metered entitlement'),
+ .describe('Create inputs for metered entitlement'),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .describe('Create inpurs for metered entitlement'), | |
| .describe('Create inputs for metered entitlement'), |
🤖 Prompt for AI Agents
In api/client/javascript/src/zod/index.ts at line 7580, there is a typo in the
schema description where "inpurs" should be corrected. Fix the typo by changing
"inpurs" to "inputs" in the .describe() method to ensure the description is
accurate and clear.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
api/client/javascript/src/client/schemas.ts (1)
16117-16204: Missing 404 response for non-existent entitlementsThe
listCustomerEntitlementGrantsoperation doesn't include a 404 response when the entitlement (identified byfeatureKey) doesn't exist. This is inconsistent with other operations likegetCustomerEntitlement,getCustomerEntitlementHistory, andresetCustomerEntitlementUsagethat properly handle non-existent entitlements.Add a 404 response definition to maintain consistency:
listCustomerEntitlementGrants: { parameters: { query?: { includeDeleted?: boolean orderBy?: components['schemas']['GrantOrderBy'] } header?: never path: { customerIdOrKey: components['schemas']['ULIDOrExternalKey'] featureKey: string } cookie?: never } requestBody?: never responses: { /** @description The request has succeeded. */ headers: { [name: string]: unknown } content: { 'application/json': components['schemas']['EntitlementGrant'][] } } /** @description The server cannot or will not process the request due to something that is perceived to be a client error (e.g., malformed request syntax, invalid request message framing, or deceptive request routing). */ headers: { [name: string]: unknown } content: { 'application/problem+json': components['schemas']['BadRequestProblemResponse'] } } /** @description The request has not been applied because it lacks valid authentication credentials for the target resource. */ headers: { [name: string]: unknown } content: { 'application/problem+json': components['schemas']['UnauthorizedProblemResponse'] } } /** @description The server understood the request but refuses to authorize it. */ headers: { [name: string]: unknown } content: { 'application/problem+json': components['schemas']['ForbiddenProblemResponse'] } } + /** @description The origin server did not find a current representation for the target resource or is not willing to disclose that one exists. */ + 404: { + headers: { + [name: string]: unknown + } + content: { + 'application/problem+json': components['schemas']['NotFoundProblemResponse'] + } + } /** @description One or more conditions given in the request header fields evaluated to false when tested on the server. */ headers: { [name: string]: unknown } content: { 'application/problem+json': components['schemas']['PreconditionFailedProblemResponse'] } }api/openapi.cloud.yaml (2)
3690-3703: Path-level parameters still repeated in every operation
customerIdOrKeyandfeatureKeyare re-declared for each verb under the same path, bloating the spec and increasing maintenance overhead. Move them to theparameters:array directly under the path object and let the operations inherit them.This was raised in a previous review but remains unresolved.
Also applies to: 3768-3781
4159-4161: 201 Created with HTTP PUT is semantically inconsistentPUT is idempotent and should return 200/204 when the target URI is known. Returning 201 implies server-side resource creation typical of POST.
Same concern was flagged earlier for this endpoint.
api/openapi.yaml (3)
3836-3853: Docs say “id or featureKey” but path only supportsfeatureKey
The description still claims the entitlement can be addressed by id or featureKey, yet the path parameter only allowsfeatureKey. Either update the wording or offer an alternate path that accepts an id to avoid misleading integrators.
Same feedback was given earlier.
4136-4164:overrideCustomerEntitlementmust use POST, not PUT – non-idempotent route still incorrect
The operation creates a new resource and soft-deletes the old one, so every call mutates state differently (non-idempotent). Re-usingPUTviolates HTTP semantics and will break retries, caching and client SDK assumptions.
This was already pointed out in the previous review and remains unfixed.- /api/v1/customers/{customerIdOrKey}/entitlements/{featureKey}/override: - put: + /api/v1/customers/{customerIdOrKey}/entitlements/{featureKey}/override: + post:(Keep the
201response or switch to200/204as appropriate.)
3694-3701: InlinefeatureKeyschema duplicated across endpoints – factor into a shared component
The exact same min/max/pattern block is repeated dozens of times, increasing maintenance risk. Define it once undercomponents/parameters/FeatureKey(or similar) and$refit everywhere:- - name: featureKey - in: path - required: true - schema: - type: string - minLength: 1 - maxLength: 64 - pattern: ^[a-z0-9]+(?:_[a-z0-9]+)*$ + - $ref: '#/components/parameters/FeatureKey'This keeps validation consistent and DRY.
🧹 Nitpick comments (2)
api/openapi.cloud.yaml (2)
3623-3629: Nit: unnecessaryexplode: falsefor a primitive booleanFor a simple
booleanquery parameter (includeDeleted) the default serialization is alreadystyle: form, explode: true. Unless there is a strong reason to force the non-exploded form (?includeDeleted=falseinstead of the equally valid?includeDeleted=true), you can drop thestyle/explodefields to keep the spec leaner.
3530-3542: Consider idempotency safeguards on entitlement creationCreating entitlements is a side-effectful action that clients may retry on network errors. Adding an
Idempotency-Keyheader (as used in other OpenMeter write endpoints) would let the server de-duplicate accidental retries and avoid 409 conflicts or duplicate rows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/api.gen.go(7 hunks)api/client/javascript/src/client/schemas.ts(2 hunks)api/client/javascript/src/zod/index.ts(1 hunks)api/openapi.cloud.yaml(1 hunks)api/openapi.yaml(1 hunks)api/spec/src/entitlements/customer.tsp(2 hunks)
🧠 Learnings (5)
api/openapi.cloud.yaml (2)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
Learnt from: chrisgacsal
PR: #3008
File: openmeter/notification/httpdriver/errors.go:19-20
Timestamp: 2025-06-18T14:53:37.191Z
Learning: In the OpenMeter codebase, webhook.ValidationError should return HTTP 500 Internal Server Error rather than 400 Bad Request because it represents internal validation errors that should not be exposed to clients via the HTTP API. This is a deliberate design decision to avoid exposing internal implementation details.
api/client/javascript/src/client/schemas.ts (1)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
api/openapi.yaml (1)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
api/api.gen.go (2)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
Learnt from: chrisgacsal
PR: #2699
File: openmeter/productcatalog/planaddon/service/service_test.go:210-211
Timestamp: 2025-04-21T08:32:31.689Z
Learning: In productcatalog.UsageBasedRateCard, the BillingCadence field is a non-pointer isodate.Period, while in productcatalog.FlatFeeRateCard, BillingCadence is a pointer type (*isodate.Period). This means MonthPeriod should be used directly for UsageBasedRateCard (not &MonthPeriod).
api/spec/src/entitlements/customer.tsp (1)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
🚧 Files skipped from review as they are similar to previous changes (1)
- api/client/javascript/src/zod/index.ts
🧰 Additional context used
🧠 Learnings (5)
api/openapi.cloud.yaml (2)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
Learnt from: chrisgacsal
PR: #3008
File: openmeter/notification/httpdriver/errors.go:19-20
Timestamp: 2025-06-18T14:53:37.191Z
Learning: In the OpenMeter codebase, webhook.ValidationError should return HTTP 500 Internal Server Error rather than 400 Bad Request because it represents internal validation errors that should not be exposed to clients via the HTTP API. This is a deliberate design decision to avoid exposing internal implementation details.
api/client/javascript/src/client/schemas.ts (1)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
api/openapi.yaml (1)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
api/api.gen.go (2)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
Learnt from: chrisgacsal
PR: #2699
File: openmeter/productcatalog/planaddon/service/service_test.go:210-211
Timestamp: 2025-04-21T08:32:31.689Z
Learning: In productcatalog.UsageBasedRateCard, the BillingCadence field is a non-pointer isodate.Period, while in productcatalog.FlatFeeRateCard, BillingCadence is a pointer type (*isodate.Period). This means MonthPeriod should be used directly for UsageBasedRateCard (not &MonthPeriod).
api/spec/src/entitlements/customer.tsp (1)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
🔇 Additional comments (12)
api/openapi.cloud.yaml (1)
3695-3703: 👍 ConsistentfeatureKeyvalidation across routesAll entitlement endpoints now apply identical length and regex guards to
featureKey, fixing the earlier validation gap.Also applies to: 3776-3781, 3850-3853
api/spec/src/entitlements/customer.tsp (4)
10-24: Well-structured Customer interface with clear documentation.The
getCustomerAccessmethod is properly defined with appropriate routing, parameter types, and return types. The documentation clearly explains the endpoint's purpose.
26-108: Excellent API design with comprehensive documentation.The
CustomerEntitlementsinterface demonstrates solid REST API design principles:
- Proper HTTP methods and status codes (201 for creation, 204 for deletion)
- Clear constraint documentation (one active entitlement per featureKey, immutability)
- Well-designed override functionality with detailed explanation
- Appropriate error handling with specific error types
The extensive documentation will be valuable for API consumers understanding the entitlement lifecycle.
110-220: Sophisticated entitlement management interface with excellent documentation.The
CustomerEntitlementinterface demonstrates advanced functionality:
- Grant management: Comprehensive documentation explaining recurrence, rollover rules, and priority systems
- History querying: Flexible time-based queries with windowing support and timezone handling
- Reset functionality: Well-explained usage period management with billing cycle integration
- Proper defaults: Good use of default values (e.g.,
windowTimeZonedefaults to "UTC")The extensive documentation for grant creation (lines 128-140) is particularly valuable, clearly explaining complex concepts like priority, recurrence, and rollover behavior.
222-233: Clean and well-documented model definition.The
CustomerAccessmodel is appropriately structured:
- Uses
Record<EntitlementValue>for the entitlements map, providing type safety- Clear documentation explaining the key-value relationship
- Proper use of
@visibility(Lifecycle.Read)annotation for read-only dataapi/api.gen.go (7)
7550-7575: Parameter structs are well-defined.The parameter structures follow Go conventions with proper struct tags for form and JSON binding. The use of pointer types for optional parameters and clear field documentation is appropriate.
8311-8321: Request body type aliases follow standard patterns.The type aliases provide clear operation-specific naming while properly reusing underlying types. This is consistent with OpenAPI code generation patterns.
11622-11648: Server interface methods are consistently defined.The method signatures properly use
ULIDOrExternalKeyfor customer identification consistently across all operations. The interface covers comprehensive CRUD operations for entitlements and grants with appropriate parameter types.
12201-12253: Unimplemented handlers provide appropriate placeholder behavior.The stub implementations correctly return HTTP 501 (Not Implemented) status, which is the appropriate response for unimplemented endpoints. Parameter types are consistent with the interface definitions.
14434-14792: Middleware functions follow consistent patterns for parameter handling.The parameter binding and validation logic is well-implemented with proper error handling. The consistent use of runtime binding functions and error reporting follows OpenAPI generation best practices.
18214-18240: Route registrations follow proper RESTful conventions.The HTTP method assignments and URL patterns are appropriate for the operations. The nested resource structure (customers/{id}/entitlements/{key}/grants) follows REST best practices.
19390-19644: Auto-generated swagger specification data.This embedded base64 data represents the swagger specification and is auto-generated. No review needed for encoded specification content.
GAlexIHU
force-pushed
the
galexi/feat/customer-entitlement-api
branch
from
25c3d9e to
8a19610
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (11)
api/client/javascript/src/zod/index.ts (5)
7454-7483: Eliminate duplicate constants and regex patterns to improve maintainability.Multiple identical regex patterns and constants are defined for the same validation rules, violating the DRY principle and making maintenance error-prone.
Based on past review feedback, these duplicate constants should be consolidated into shared definitions that can be reused across different schemas.
7580-7580: Fix typo in schema description.- .describe('Create inpurs for metered entitlement'), + .describe('Create inputs for metered entitlement'),
8051-8285: Eliminate massive schema duplication between create and override operations.The entire override entitlement schema (234+ lines) is an exact duplicate of the create entitlement schema, severely violating DRY principles and creating significant maintenance burden.
Since both operations accept identical input structures, they should share the same schema:
-export const overrideCustomerEntitlementBodyFeatureKeyMax = 64 -// ... (remove all duplicate constants and schema definition) -export const overrideCustomerEntitlementBody = zod - .discriminatedUnion('type', [ - // ... (remove entire duplicate schema) - ]) - .describe('Create inputs for entitlement') +// Reuse the create entitlement body schema +export const overrideCustomerEntitlementBody = createCustomerEntitlementBody;This would eliminate over 200 lines of duplicate code while maintaining functionality.
8180-8180: Fix repeated typo in override schema description.- .describe('Create inpurs for metered entitlement'), + .describe('Create inputs for metered entitlement'),
7691-7693: Reuse shared regex constants instead of redefining them.The ULID regex pattern is duplicated across multiple schemas. This should reference a shared constant defined once.
Consider defining a shared constant like:
+export const ULID_REGEX = new RegExp('^[0-7][0-9A-HJKMNP-TV-Za-hjkmnp-tv-z]{25}$')Then reuse it:
-export const listCustomerEntitlementsPathCustomerIdOrKeyRegExpOne = new RegExp( - '^[0-7][0-9A-HJKMNP-TV-Za-hjkmnp-tv-z]{25}$' -) +export const listCustomerEntitlementsPathCustomerIdOrKeyRegExpOne = ULID_REGEXapi/client/javascript/src/client/schemas.ts (1)
16138-16226: Missing 404 response for non-existent entitlementsThe
listCustomerEntitlementGrantsoperation doesn't include a 404 response when the entitlement (identified byfeatureKey) doesn't exist. This is inconsistent with other operations that interact with specific entitlements.Consider adding a 404 response to handle cases where the entitlement doesn't exist, similar to
getCustomerEntitlementand other operations:+ /** @description The origin server did not find a current representation for the target resource or is not willing to disclose that one exists. */ + 404: { + headers: { + [name: string]: unknown + } + content: { + 'application/problem+json': components['schemas']['NotFoundProblemResponse'] + } + }api/openapi.yaml (3)
3694-3701:featureKeyschema is duplicated instead of referencing a shared component
Inline string schema (lines 3695-3701) diverges from other endpoints and the suggested#/components/parameters/FeatureKey. Re-introduces validation drift already pointed out in an earlier review.
3836-3840: Description still claims entitlement can be referenced by id but path only allows featureKey
The mismatch flagged earlier is unchanged (lines 3836-3839). Either update the wording or add the missing path/parameter.
4136-4164: PUT still violates HTTP idempotency contract – use POST
TheoverrideCustomerEntitlementroute is again declared withput(lines 4136-4156) and returns 201 Created (lines 4158-4164), proving the operation is not idempotent. This exact concern was raised previously and remains unresolved.api/openapi.cloud.yaml (2)
3528-3547: Path-level parameters are still repeated – please DRY them up
customerIdOrKeyis declared twice inside the same path (POST&GET). Defining it once under the path’sparameters:node removes duplication and guarantees that future operations stay in sync./api/v1/customers/{customerIdOrKey}/entitlements: - post: + parameters: + - $ref: '#/components/parameters/CustomerIdOrKey' + post: operationId: createCustomerEntitlement ... - get: + get: operationId: listCustomerEntitlementsSame applies to every other path where
customerIdOrKey/featureKeyare copy-pasted per operation.
Reducing YAML noise improves maintainability and keeps the generated types identical across operations.Also applies to: 3617-3621
4159-4161:PUTshould not return 201 Created
overrideCustomerEntitlementis an idempotent overwrite, but the success response is 201 Created.
REST conventions expect 200 (or 204) for successfulPUTs; 201 is reserved forPOSTwhen the server creates a brand-new resource at a URI it chooses.- '201': + '200': description: Successful override.Alternatives:
- Keep
PUT, switch to 200/204, or- Change the verb to
POSTif you genuinely “create”.
🧹 Nitpick comments (2)
api/openapi.yaml (1)
4021-4025: Typos in public API docs
accross,continous,seperated→across,continuous,separated. Public-facing spelling errors harm credibility.- Returns historical balance and usage data for the entitlement. The queried history can span accross multiple reset events. - BurndownHistory returns a continous history of segments, where the segments are seperated by events that changed either the grant burndown priority or the usage period. + Returns historical balance and usage data for the entitlement. The queried history can span across multiple reset events. + BurndownHistory returns a continuous history of segments, where the segments are separated by events that changed either the grant burndown priority or the usage period.api/openapi.cloud.yaml (1)
4021-4027: Spelling / grammar typos in public docs
accross,continous,seperatedshould beacross,continuous,separated. Small, but these strings surface in generated docs & SDKs.- Returns historical balance and usage data for the entitlement. The queried history can span accross multiple reset events. - BurndownHistory returns a continous history of segments, where the segments are seperated by events ... + Returns historical balance and usage data for the entitlement. The queried history can span across multiple reset events. + BurndownHistory returns a continuous history of segments, where the segments are separated by events ...Polishing descriptions maintains professionalism.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/api.gen.go(7 hunks)api/client/javascript/src/client/schemas.ts(2 hunks)api/client/javascript/src/zod/index.ts(1 hunks)api/openapi.cloud.yaml(1 hunks)api/openapi.yaml(1 hunks)api/spec/src/cloud/main.tsp(1 hunks)api/spec/src/entitlements/customer.tsp(2 hunks)openmeter/server/router/entitlement.go(2 hunks)
🧠 Learnings (5)
api/openapi.yaml (1)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
api/openapi.cloud.yaml (2)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
Learnt from: chrisgacsal
PR: #3008
File: openmeter/notification/httpdriver/errors.go:19-20
Timestamp: 2025-06-18T14:53:37.191Z
Learning: In the OpenMeter codebase, webhook.ValidationError should return HTTP 500 Internal Server Error rather than 400 Bad Request because it represents internal validation errors that should not be exposed to clients via the HTTP API. This is a deliberate design decision to avoid exposing internal implementation details.
api/client/javascript/src/client/schemas.ts (1)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
api/api.gen.go (2)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
Learnt from: chrisgacsal
PR: #2699
File: openmeter/productcatalog/planaddon/service/service_test.go:210-211
Timestamp: 2025-04-21T08:32:31.689Z
Learning: In productcatalog.UsageBasedRateCard, the BillingCadence field is a non-pointer isodate.Period, while in productcatalog.FlatFeeRateCard, BillingCadence is a pointer type (*isodate.Period). This means MonthPeriod should be used directly for UsageBasedRateCard (not &MonthPeriod).
api/spec/src/entitlements/customer.tsp (1)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
🚧 Files skipped from review as they are similar to previous changes (2)
- api/spec/src/cloud/main.tsp
- openmeter/server/router/entitlement.go
🧰 Additional context used
🧠 Learnings (5)
api/openapi.yaml (1)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
api/openapi.cloud.yaml (2)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
Learnt from: chrisgacsal
PR: #3008
File: openmeter/notification/httpdriver/errors.go:19-20
Timestamp: 2025-06-18T14:53:37.191Z
Learning: In the OpenMeter codebase, webhook.ValidationError should return HTTP 500 Internal Server Error rather than 400 Bad Request because it represents internal validation errors that should not be exposed to clients via the HTTP API. This is a deliberate design decision to avoid exposing internal implementation details.
api/client/javascript/src/client/schemas.ts (1)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
api/api.gen.go (2)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
Learnt from: chrisgacsal
PR: #2699
File: openmeter/productcatalog/planaddon/service/service_test.go:210-211
Timestamp: 2025-04-21T08:32:31.689Z
Learning: In productcatalog.UsageBasedRateCard, the BillingCadence field is a non-pointer isodate.Period, while in productcatalog.FlatFeeRateCard, BillingCadence is a pointer type (*isodate.Period). This means MonthPeriod should be used directly for UsageBasedRateCard (not &MonthPeriod).
api/spec/src/entitlements/customer.tsp (1)
Learnt from: chrisgacsal
PR: #2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: CI
- GitHub Check: E2E
- GitHub Check: Quickstart
- GitHub Check: Commit hooks
- GitHub Check: Developer environment
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
api/client/javascript/src/zod/index.ts (1)
8323-8344: Well-structured reset schema with appropriate validation.The reset schema design is solid with proper optional parameters for effective time, overage preservation, and anchor retention settings.
api/client/javascript/src/client/schemas.ts (2)
772-930: Well-structured API path definitionsThe customer entitlement endpoint paths are well-organized and follow RESTful conventions. The comprehensive documentation clearly explains each endpoint's purpose and behavior. HTTP method assignments are appropriate and unused methods are properly excluded.
15765-16635: Excellent consistency in operation definitionsThe operation definitions demonstrate excellent consistency in:
- Parameter typing (customerIdOrKey properly uses
ULIDOrExternalKey)- Response structure patterns
- Error handling with comprehensive HTTP status codes
- Request/response body schemas
The operations are well-structured and follow established API patterns.
api/openapi.yaml (2)
3538-3541: Contradiction: “cannot modify” vs dedicated override endpoint
The doc states an entitlement “cannot be modified, only deleted” but the spec later offers/overridefor zero-downtime changes. Clarify the lifecycle rules or remove the conflicting sentence to avoid integrator confusion.
3608-3611: Inconsistent schema naming:EntitlementCreateInputsvs singular forms elsewhere
Most create operations use singular (…CreateInput), but this one referencesEntitlementCreateInputs(plural) which may not exist or will break code generation. Confirm the actual component name and align.api/spec/src/entitlements/customer.tsp (4)
13-24: Well-designed customer access interface.The
Customerinterface is cleanly implemented with proper REST conventions, comprehensive error handling, and clear documentation. The single responsibility of retrieving overall customer access is well-defined.
26-108: Comprehensive entitlements management interface with excellent documentation.The
CustomerEntitlementsinterface is well-architected with:
- Clear documentation of entitlement types and business constraints
- Proper REST conventions and HTTP status codes
- Comprehensive error handling for various scenarios
- Well-designed override functionality for zero-downtime updates
- Appropriate immutability constraints clearly communicated
The extensive documentation will be valuable for API consumers understanding the entitlement system's behavior and limitations.
110-220: Sophisticated entitlement management interface with comprehensive functionality.The
CustomerEntitlementinterface demonstrates advanced entitlement management capabilities:
- Detailed grant system with priority, recurrence, and rollover mechanics
- Historical data querying with flexible windowing and timezone support
- Manual reset functionality with period anchor adjustment
- Excellent documentation explaining complex business rules and behaviors
- Proper route organization for different operation types
The complexity of the grant and rollover system is well-documented, making it accessible to API consumers despite the sophisticated underlying mechanics.
222-233: Clean and focused model design.The
CustomerAccessmodel is well-designed with:
- Clear purpose and structure for mapping feature access
- Appropriate use of
Record<EntitlementValue>for key-value mapping- Proper read-only visibility annotation
- Good documentation explaining the mapping structure
api/api.gen.go (7)
7571-7596: Parameter definitions look well-structured.The new parameter structs are properly defined with appropriate Go struct tags, correct pointer usage for optional fields, and descriptive comments following RFC 3339 format specifications.
8332-8342: JSON request body type aliases are correctly defined.The type aliases follow standard OpenAPI code generation patterns and provide proper mapping between HTTP request bodies and existing input types.
11643-11669: Interface methods are consistently defined.The ServerInterface methods use consistent parameter types (ULIDOrExternalKey for customerIdOrKey), follow REST conventions, and provide comprehensive coverage of customer entitlement operations. The type inconsistency issues from previous reviews have been properly addressed.
12222-12274: Stub implementations are appropriate for initial API definition.The unimplemented methods correctly return HTTP 501 Not Implemented status and have signatures that match the interface definitions exactly. This is appropriate for the API-first development approach described in the PR objectives.
14455-14813: Middleware handlers implement proper parameter parsing and error handling.The handlers correctly parse path and query parameters, handle required vs optional parameter validation appropriately, and apply middleware chains in the standard reverse order. The error handling properly returns InvalidParamFormatError and RequiredParamError as needed.
18235-18261: Route registrations follow RESTful conventions correctly.The HTTP routes use appropriate methods for each operation, follow consistent URL patterns with proper path parameter definitions, and correctly map to their corresponding middleware handlers.
19413-19669: Embedded Swagger specification content is auto-generated.This base64-encoded content represents the embedded OpenAPI specification and is automatically generated. No manual review is needed for this section.
| export const getCustomerEntitlementPathFeatureKeyRegExp = new RegExp( | ||
| '^[a-z0-9]+(?:_[a-z0-9]+)*$' | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate regex patterns.
The feature key regex pattern is redefined here but should reuse a shared constant to maintain consistency and reduce duplication.
This regex pattern appears multiple times throughout the file and should be defined once as a shared constant.
🤖 Prompt for AI Agents
In api/client/javascript/src/zod/index.ts around lines 7728 to 7730, the regex
pattern for feature keys is duplicated. To fix this, locate the existing shared
constant for this regex pattern elsewhere in the file or create one if it
doesn't exist, then replace this local definition with a reference to that
shared constant to ensure consistency and reduce duplication.
| export const deleteCustomerEntitlementPathFeatureKeyRegExp = new RegExp( | ||
| '^[a-z0-9]+(?:_[a-z0-9]+)*$' | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address continued regex pattern duplication.
The feature key regex pattern continues to be duplicated across schemas instead of using shared constants.
This is part of the broader pattern of regex duplication throughout the file that should be addressed systematically.
🤖 Prompt for AI Agents
In api/client/javascript/src/zod/index.ts around lines 7764 to 7766, the regex
pattern for deleteCustomerEntitlementPathFeatureKeyRegExp is duplicated across
multiple schemas. To fix this, extract the regex pattern string into a shared
constant variable at the top of the file and then use that constant to create
the RegExp instance wherever needed. This will eliminate duplication and improve
maintainability.
| export const listCustomerEntitlementGrantsPathFeatureKeyRegExp = new RegExp( | ||
| '^[a-z0-9]+(?:_[a-z0-9]+)*$' | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consistent regex duplication issue.
The feature key regex pattern is duplicated again here, contributing to the overall maintenance burden.
Part of the systematic regex duplication that should be addressed across the entire file.
🤖 Prompt for AI Agents
In api/client/javascript/src/zod/index.ts around lines 7798 to 7800, the regex
pattern for feature keys is duplicated, increasing maintenance complexity. To
fix this, define the regex pattern once as a constant at a central location in
the file and reuse this constant wherever the pattern is needed, replacing all
duplicate inline regex definitions with references to this single constant.
| export const createCustomerEntitlementGrantPathFeatureKeyRegExp = new RegExp( | ||
| '^[a-z0-9]+(?:_[a-z0-9]+)*$' | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address regex pattern duplication in grant schemas.
The feature key regex pattern is redefined yet again, adding to the maintenance complexity.
This duplication should be resolved as part of the broader consolidation effort across all schemas.
🤖 Prompt for AI Agents
In api/client/javascript/src/zod/index.ts around lines 7849 to 7851, the regex
pattern for feature keys is duplicated, increasing maintenance complexity.
Refactor by extracting this regex pattern into a single shared constant or
utility that can be imported and reused across all grant schemas, eliminating
redundant definitions and centralizing pattern management.
| export const getCustomerEntitlementHistoryPathFeatureKeyRegExp = new RegExp( | ||
| '^[a-z0-9]+(?:_[a-z0-9]+)*$' | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Feature key regex duplication continues.
The same regex pattern duplication issue affects the history endpoint schemas.
Consolidating these patterns would significantly improve maintainability across all endpoint schemas.
🤖 Prompt for AI Agents
In api/client/javascript/src/zod/index.ts around lines 7966 to 7968, the regex
pattern for feature keys is duplicated in the history endpoint schemas. To fix
this, extract the regex pattern into a single shared constant and replace all
instances of the duplicated regex with this constant. This consolidation will
improve maintainability by centralizing the pattern definition.
| export const resetCustomerEntitlementUsagePathFeatureKeyRegExp = new RegExp( | ||
| '^[a-z0-9]+(?:_[a-z0-9]+)*$' | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Final instance of feature key regex duplication.
Even in the reset endpoint, the feature key regex pattern is redefined instead of using a shared constant.
This completes the pattern of regex duplication throughout the entire file that should be systematically addressed.
🤖 Prompt for AI Agents
In api/client/javascript/src/zod/index.ts around lines 8298 to 8300, the regex
pattern for feature keys is duplicated instead of using a shared constant. To
fix this, identify the existing shared constant for the feature key regex
pattern and replace the inline regex definition in the
resetCustomerEntitlementUsagePathFeatureKeyRegExp declaration with a reference
to that shared constant, ensuring consistent reuse and avoiding duplication.
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/EntitlementCreateInputs' | ||
| get: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent input type naming – plural vs singular
EntitlementCreateInputs (plural) vs EntitlementGrantCreateInput (singular) breaks naming consistency and is confusing for consumers.
Recommend harmonising on the singular “…Input” across the board:
- $ref: '#/components/schemas/EntitlementCreateInputs'
+ $ref: '#/components/schemas/EntitlementCreateInput'(and rename the schema accordingly).
Keeps the API surface predictable and avoids unnecessary mental overhead.
Also applies to: 4014-4017
🤖 Prompt for AI Agents
In api/openapi.cloud.yaml around lines 3609 to 3612, the schema name
EntitlementCreateInputs uses a plural form inconsistent with other input types
like EntitlementGrantCreateInput, which are singular. Rename
EntitlementCreateInputs to EntitlementCreateInput to maintain consistent
singular naming for input types. Also, apply the same renaming approach to the
schema references around lines 4014 to 4017 to ensure uniformity across the API
specification.
2 participants
Overview
Adds routes to API definition with unimplemented responses
Notes for reviewer
Summary by CodeRabbit