-
Notifications
You must be signed in to change notification settings - Fork 523
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: updating name does not affect ratelimits #2693
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
📝 WalkthroughWalkthroughThe pull request introduces updates to the key management functionality within the API. It includes a new test suite for verifying that the name update of a key does not alter its rate limit settings. Additionally, 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! 🙏 |
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: 1
🧹 Outside diff range and nitpick comments (3)
apps/api/src/routes/v1_keys_updateKey.happy.test.ts (1)
1062-1130
: Consider adding edge cases to strengthen test coverage.While the current test effectively covers the basic scenario, consider adding tests for:
- Multiple consecutive name updates
- Concurrent updates of name and other non-rate-limit fields
- Edge cases for name values (empty, very long, special characters)
Example test case:
test("should preserve ratelimit config after multiple name updates", async (t) => { // ... setup code ... // First name update await h.post<V1KeysUpdateKeyRequest, V1KeysUpdateKeyResponse>({ // ... update to "name1" ... }); // Second name update await h.post<V1KeysUpdateKeyRequest, V1KeysUpdateKeyResponse>({ // ... update to "name2" ... }); // Verify rate limits remained unchanged after multiple updates const verify = await h.post<V1KeysVerifyKeyRequest, V1KeysVerifyKeyResponse>({ // ... verification code ... }); expect(verify.body.ratelimit!.limit).toBe(10); expect(verify.body.ratelimit!.remaining).toBe(9); });apps/api/src/routes/v1_keys_updateKey.ts (2)
375-387
: Simplify 'ratelimit' handling by deprecating old fieldsThe code currently handles both new and deprecated fields within
req.ratelimit
, such asasync
,type
,limit
,refillRate
,duration
, andrefillInterval
. This adds complexity and potential for errors. Consider updating the code to handle only the current fields (async
,limit
,duration
), and handle deprecation separately to simplify the logic.For example:
-if (typeof req.ratelimit !== "undefined") { - if (req.ratelimit === null) { - changes.ratelimitAsync = null; - changes.ratelimitLimit = null; - changes.ratelimitDuration = null; - } else { - changes.ratelimitAsync = - typeof req.ratelimit.async === "boolean" - ? req.ratelimit.async - : req.ratelimit.type === "fast"; - changes.ratelimitLimit = req.ratelimit.limit ?? req.ratelimit.refillRate; - changes.ratelimitDuration = req.ratelimit.duration ?? req.ratelimit.refillInterval; - } +if (req.ratelimit === null) { + changes.ratelimitAsync = null; + changes.ratelimitLimit = null; + changes.ratelimitDuration = null; +} else if (typeof req.ratelimit !== "undefined") { + changes.ratelimitAsync = req.ratelimit.async ?? false; + changes.ratelimitLimit = req.ratelimit.limit; + changes.ratelimitDuration = req.ratelimit.duration; +}Ensure that the deprecated fields are handled appropriately elsewhere if necessary.
381-384
: Clarify 'ratelimitAsync' assignment logicThe assignment of
changes.ratelimitAsync
involves fallback logic betweenreq.ratelimit.async
andreq.ratelimit.type === "fast"
. To improve readability and avoid confusion, consider adding comments explaining this logic, especially regarding the use of deprecated fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/api/src/routes/v1_keys_updateKey.happy.test.ts
(1 hunks)apps/api/src/routes/v1_keys_updateKey.ts
(2 hunks)
🔇 Additional comments (6)
apps/api/src/routes/v1_keys_updateKey.happy.test.ts (1)
1062-1130
: Well-structured test implementation!
The test suite effectively validates that updating a key's name preserves rate limit settings through:
- Comprehensive setup with proper permissions
- Clear separation of concerns (create, update, verify)
- Thorough assertions on both DB state and API behavior
apps/api/src/routes/v1_keys_updateKey.ts (5)
7-7
: Approved: Importing 'Key' type
The addition of the Key
type import is appropriate as it's used for typing the changes
object later in the code.
343-346
: Approved: Initializing 'changes' as Partial
Initializing the changes
object with Partial<Key>
ensures type safety for the fields being updated.
399-399
: Validate default value for 'refillDay'
When setting changes.refillDay
, the code defaults to 1
if req.refill.refillDay
is undefined. Ensure that this default aligns with business logic, especially since refillDay
should only be set when interval
is 'monthly'.
405-407
: Approved: Conditional database update
Updating the database only when there are changes (Object.keys(changes).length > 0
) prevents unnecessary write operations.
385-386
: Ensure proper assignment of 'ratelimitLimit' and 'ratelimitDuration'
The use of nullish coalescing with potentially undefined deprecated fields (req.ratelimit.limit ?? req.ratelimit.refillRate
and req.ratelimit.duration ?? req.ratelimit.refillInterval
) could lead to unintended undefined
values. Verify that these assignments always result in valid numbers.
To confirm, you can check all usages of these fields in the codebase:
Summary by CodeRabbit
New Features
Refactor
externalId
andownerId
).Bug Fixes