feat: override methods in sdk #2668
Conversation
chronark
requested review from
perkinsjr,
mcstepp and
MichaelUnkey
as code owners
🦋 Changeset detectedLatest commit: a92f1d7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a ratelimit override API within the "@unkey/api" package, allowing users to adjust rate limits under certain conditions. Additionally, modifications are made to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
.changeset/wild-spies-punch.md (1)
4-5: Enhance the changeset description for better changelog clarity.The current description is too brief. Consider expanding it to include:
- What the ratelimit override API does
- Key features or capabilities
- Basic usage example
- Any important considerations
Example:
-Add ratelimit override API +Add ratelimit override API to dynamically adjust rate limits + +- Introduces new methods: getOverride, listOverrides, setOverride, deleteOverride +- Allows dynamic adjustment of rate limits under specific conditions +- Includes validation and error handling for override operations +- Example: await client.setOverride({ ... })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.changeset/wild-spies-punch.md(1 hunks)apps/api/src/routes/v1_keys_getVerifications.ts(2 hunks)packages/api/src/client.ts(3 hunks)packages/api/src/openapi.d.ts(35 hunks)
🧰 Additional context used
🪛 Biome
apps/api/src/routes/v1_keys_getVerifications.ts
[error] 253-253: Expected a default, or a case but instead found 'verifications'.
Expected a default, or a case here.
(parse)
[error] 255-256: Expected a statement but instead found 'case "RATE_LIMITED":
verifications[d.time].rateLimited += d.count'.
Expected a statement here.
(parse)
[error] 258-259: Expected a statement but instead found 'case "USAGE_EXCEEDED":
verifications[d.time].usageExceeded += d.count'.
Expected a statement here.
(parse)
[error] 264-265: expected , but instead found return
Remove return
(parse)
[error] 256-257: This code is unreachable
... because this statement will break the flow of the code beforehand
(lint/correctness/noUnreachable)
[error] 259-260: This code is unreachable
... because this statement will break the flow of the code beforehand
(lint/correctness/noUnreachable)
🔇 Additional comments (10)
apps/api/src/routes/v1_keys_getVerifications.ts (2)
119-122: LGTM: Identity data structure is well-formed
The identity object structure is clean and properly typed.
265-274: LGTM: Response formatting is clean and type-safe
The response structure is well-formatted and maintains type safety through proper parsing of time values and consistent object structure.
packages/api/src/client.ts (4)
9-27: LGTM! Well-structured type definition with clear documentation.
The UnkeyOptions type effectively uses TypeScript's discriminated unions with the never type to ensure mutually exclusive options. The documentation clearly explains the deprecation of token in favor of rootKey.
80-90: LGTM! Clear separation of GET and POST request types.
The ApiRequest type union provides good type safety by ensuring GET requests can't have a body and POST requests can't have query parameters.
94-105: LGTM! Well-defined Result type with explicit error handling.
The Result type effectively uses discriminated unions to ensure type safety between success and error cases. The error structure is comprehensive with all necessary fields.
379-432: LGTM! Well-implemented rate limit override methods.
The new rate limit override methods are well-structured and consistent with the existing codebase patterns. They properly leverage:
- Type safety through OpenAPI types
- Consistent error handling
- Proper HTTP method usage
- Common fetch implementation with retry logic
However, let's verify the OpenAPI types are properly defined for these new endpoints.
✅ Verification successful
OpenAPI types and routes are properly defined and consistent
The verification confirms that:
- All four rate limit override endpoints have proper OpenAPI route definitions
- The endpoints use consistent authentication with
bearerAuth - Input validation is implemented using Zod schemas
- The types in the client match the route definitions
- Request/response schemas are properly defined with appropriate content types
The implementation is type-safe and follows the API's established patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify OpenAPI types for new rate limit override endpoints
# Check if the OpenAPI types exist for all new endpoints
rg -A 2 '"\/v1\/ratelimits\.(get|list|set|delete)Override"' --type ts
Length of output: 188
Script:
#!/bin/bash
# Description: Verify OpenAPI types for new rate limit override endpoints with corrected regex
# Check if the OpenAPI types exist for all new endpoints
rg -A 2 '/v1/ratelimits\.(get|list|set|delete)Override' --type ts
Length of output: 10912
Script:
#!/bin/bash
# Let's verify the OpenAPI route definitions for these endpoints
# Check route definitions
rg -A 10 'path: "/v1/ratelimits\.(get|list|set|delete)Override",' --type ts
Length of output: 2872
packages/api/src/openapi.d.ts (4)
9-10: LGTM: Type helper improvements
The parentheses addition in the XOR type and the recursive OneOf implementation improve type safety and readability.
533-533: LGTM: Enhanced verification status codes
The expanded set of verification status codes provides more granular error reporting, particularly for authorization and permission-related scenarios.
559-563: LGTM: Improved permission query formatting
The reformatted PermissionQuery type enhances readability while maintaining the same logical structure.
Line range hint 2673-2882: LGTM: Well-structured migration endpoints
The new migration endpoints provide comprehensive functionality for batch operations with proper validation and configuration options. The documentation is thorough and includes all necessary parameters.
| // case "VALID": | ||
| verifications[d.time].success += d.count; | ||
| break; | ||
| break; | ||
| case "RATE_LIMITED": | ||
| verifications[d.time].rateLimited += d.count; | ||
| break; | ||
| verifications[d.time].rateLimited += d.count; | ||
| break; | ||
| case "USAGE_EXCEEDED": | ||
| verifications[d.time].usageExceeded += d.count; | ||
| break; | ||
| verifications[d.time].usageExceeded += d.count; | ||
| break; |
There was a problem hiding this comment.
Fix broken switch statement syntax and logic
There are several critical issues in the switch statement:
- The syntax is incorrect, leading to parsing errors
- The "VALID" case is commented out, which means successful verifications aren't being counted
- The break statements are unreachable due to incorrect structure
Apply this fix to restore proper functionality:
switch (d.outcome) {
- // case "VALID":
- verifications[d.time].success += d.count;
- break;
- case "RATE_LIMITED":
-verifications[d.time].rateLimited += d.count;
-break;
- case "USAGE_EXCEEDED":
-verifications[d.time].usageExceeded += d.count;
-break;
+ case "VALID":
+ verifications[d.time].success += d.count;
+ break;
+ case "RATE_LIMITED":
+ verifications[d.time].rateLimited += d.count;
+ break;
+ case "USAGE_EXCEEDED":
+ verifications[d.time].usageExceeded += d.count;
+ break;
+ default:
+ logger.warn(`Unknown verification outcome: ${d.outcome}`);
}📝 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.
| // case "VALID": | |
| verifications[d.time].success += d.count; | |
| break; | |
| break; | |
| case "RATE_LIMITED": | |
| verifications[d.time].rateLimited += d.count; | |
| break; | |
| verifications[d.time].rateLimited += d.count; | |
| break; | |
| case "USAGE_EXCEEDED": | |
| verifications[d.time].usageExceeded += d.count; | |
| break; | |
| verifications[d.time].usageExceeded += d.count; | |
| break; | |
| switch (d.outcome) { | |
| case "VALID": | |
| verifications[d.time].success += d.count; | |
| break; | |
| case "RATE_LIMITED": | |
| verifications[d.time].rateLimited += d.count; | |
| break; | |
| case "USAGE_EXCEEDED": | |
| verifications[d.time].usageExceeded += d.count; | |
| break; | |
| default: | |
| logger.warn(`Unknown verification outcome: ${d.outcome}`); | |
| } |
🧰 Tools
🪛 Biome
[error] 253-253: Expected a default, or a case but instead found 'verifications'.
Expected a default, or a case here.
(parse)
[error] 255-256: Expected a statement but instead found 'case "RATE_LIMITED":
verifications[d.time].rateLimited += d.count'.
Expected a statement here.
(parse)
[error] 258-259: Expected a statement but instead found 'case "USAGE_EXCEEDED":
verifications[d.time].usageExceeded += d.count'.
Expected a statement here.
(parse)
[error] 256-257: This code is unreachable
... because this statement will break the flow of the code beforehand
(lint/correctness/noUnreachable)
[error] 259-260: This code is unreachable
... because this statement will break the flow of the code beforehand
(lint/correctness/noUnreachable)
| @@ -249,27 +249,27 @@ | |||
| verifications[d.time] = { success: 0, rateLimited: 0, usageExceeded: 0 }; | |||
| } | |||
| switch (d.outcome) { | |||
| case "VALID": | |||
| // case "VALID": | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the git history and code context, I can now provide a final response:
Commenting out "VALID" case appears to be unintentional
The change was introduced in commit a92f1d7 "feat: override methods in sdk" but seems to be an accidental formatting change rather than an intentional feature modification because:
- The code still increments the success counter (
verifications[d.time].success += d.count) - The case is only commented out, not properly removed
- Other verification types (RATE_LIMITED, USAGE_EXCEEDED) remain active
- The change appears alongside formatting-only modifications in the commit
This should be fixed by uncommenting the "VALID" case to maintain proper verification counting.
🔗 Analysis chain
Verify the intent of commenting out the "VALID" case
The "VALID" case has been commented out, which means successful verifications are not being counted. Is this intentional and related to the rate limit override API changes mentioned in the PR objectives?
Let me gather more context about this specific file and the changes around this code section.
Let me try one more verification to understand if this is part of recent changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related changes in verification handling
rg -A 5 "case \"VALID\""
Length of output: 2152
Script:
#!/bin/bash
# Get the full context of the verification handling in this file
rg -B 10 -A 10 "case \"VALID\"" apps/api/src/routes/v1_keys_getVerifications.ts
# Check git history for this specific line
git log -L '/case "VALID":/apps/api/src/routes/v1_keys_getVerifications.ts'
Length of output: 922
Script:
#!/bin/bash
# Check recent commits that modified this file
git log --patch apps/api/src/routes/v1_keys_getVerifications.ts
# Also check if there are any related test files
fd "v1_keys_getVerifications" --type f
Length of output: 14265
| /** | ||
| * @description The name of the ratelimit. | ||
| * @example tokens | ||
| */ | ||
| name: string; | ||
| /** | ||
| * @description Optionally override how expensive this operation is and how many tokens are deducted from the current limit. | ||
| * @default 1 | ||
| */ | ||
| cost?: number; | ||
| /** @description Optionally override the limit. */ | ||
| limit?: number; | ||
| /** @description Optionally override the ratelimit window duration. */ | ||
| duration?: number; | ||
| }[]; |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now generate the final response as we have comprehensive information about the usage of deprecated rate limit properties across the codebase.
Migration required for rate limit configuration changes
Several files need updates to handle the rate limit property changes:
apps/api/src/routes/v1_keys_*.ts: Update rate limit handling to use newasyncproperty instead oftype: "fast"and consolidaterefillRate/refillIntervaltolimit/durationapps/dashboard/lib/zod-helper.ts: Remove or update the deprecated rate limit type enumapps/api/src/benchmarks/ratelimit_latency.test.ts: Update test configuration to use new propertiesapps/api/src/routes/legacy_*.ts: Legacy routes need to be updated or marked for deprecation
🔗 Analysis chain
Verify rate limit property migrations
The rate limit configuration has significant changes:
- New
asyncproperty for controlling rate limit behavior - Deprecation of
type,refillRate, andrefillIntervalproperties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for usage of deprecated rate limit properties
echo "Checking for usage of deprecated rate limit properties..."
rg -A 2 'type.*fast|type.*consistent|refillRate|refillInterval' --type ts
# Look for existing rate limit configurations that need migration
echo "Checking for rate limit configurations that need updating..."
ast-grep --pattern 'ratelimit: {
$$$
}'
Length of output: 24744
Summary by CodeRabbit
Release Notes
New Features
Improvements
keyIdandownerId.These updates aim to enhance user experience and streamline API interactions.