feat: allow-developer-to-set-a-custom-refill-time-when-using-the #2114
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
|
Warning Rate limit exceeded@MichaelUnkey has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new optional property, Changes
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! 🙏 |
…ill-time-when-using-the
…-allow-developer-to-set-a-custom-refill-time-when-using-the
…en-using-the' of https://github.com/unkeyed/unkey into eng-1324-allow-developer-to-set-a-custom-refill-time-when-using-the
…-allow-developer-to-set-a-custom-refill-time-when-using-the
…-allow-developer-to-set-a-custom-refill-time-when-using-the
| @@ -317,6 +317,7 @@ export const registerV1ApisListKeys = (app: App) => | |||
| ? { | |||
| interval: k.refillInterval, | |||
| amount: k.refillAmount, | |||
| refillDay: k.refillInterval === "monthly" ? k.refillDay : null, | |||
There was a problem hiding this comment.
what if k.refillInterval is monthly but k.refillDay is null?
There was a problem hiding this comment.
This should not ever happy. Monthly will default to 1 if No value. So we should never have a null value unless daily. But i gess we should check for it anyway.
| @@ -467,4 +467,37 @@ describe("with externalId", () => { | |||
| expect(key!.identity!.id).toEqual(identity.id); | |||
| }); | |||
| }); | |||
| describe("Should default last day of month if none provided", () => { | |||
There was a problem hiding this comment.
Could you explain this test case to me?
Let me explain my thought process:
If this were to run today:
const date = new Date();
// -> 2024-09-26T19:37:51.482Z
const lastDate = date.getMonth() + 1;
// -> 9 (october but 0 indexed)
Why do we want the default `refillDay` to be 9?
| @@ -326,6 +343,12 @@ export const registerV1KeysCreateKey = (app: App) => | |||
| : Promise.resolve(null), | |||
| ]); | |||
| const newKey = await retry(5, async (attempt) => { | |||
| const date = new Date(); | |||
There was a problem hiding this comment.
why are we using the last day of a month as default?
also nitpick: move this out of the retry, it doesn't fail
| ); | ||
| io.logger.info(`found ${keys.length} keys with daily refill set`); | ||
| // const keysWithRefill = keys.length; | ||
| if (today !== lastDayOfMonth) { |
There was a problem hiding this comment.
I had to wrap my head around how this works
it seems to be correct though
can we make it a bit easier to understand?
Maybe it's just the time though, I'm tired
| @@ -44,36 +67,44 @@ client.defineJob({ | |||
| remaining: key.refillAmount, | |||
| lastRefillAt: new Date(), | |||
| }) | |||
| .where(eq(schema.keys.id, key.id)); | |||
| .where(eq(schema.keys.id, key.id)) | |||
There was a problem hiding this comment.
this prevents the call from throwing, which prevents trigger from knowing it failed, and you're not logging with trigger's logger either. Do we even know when this goes wrong in prod?
apps/workflows/jobs/refill-daily.ts
Outdated
| }); | ||
| }) | ||
| .catch((_error) => { | ||
| console.error(`failed to update Audit Logs for ${key.id}`); |
There was a problem hiding this comment.
is console.error logging to triggers dashboard?
There was a problem hiding this comment.
I think it was just to have some error. I assume this will still work with just a throw error??
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
tools/migrate/tinybird-export.ts (3)
6-7: Consider removing or conditionally executing the console.log statement.While this log statement is useful for debugging and tracking the export process, it's generally not recommended to leave console.log statements in production code. Consider one of the following options:
- Remove the console.log statement before merging to production.
- Wrap it in a conditional statement that only executes in non-production environments.
- Use a proper logging library that can be configured based on the environment.
If you decide to keep it, you could refactor it to use a logging library or a conditional statement:
if (process.env.NODE_ENV !== 'production') { console.log({ cursor }); }Or using a logging library:
import logger from './your-logging-library'; logger.debug({ cursor });
17-18: Consider removing or conditionally executing the console.log statement.Similar to the previous log statement, while this is useful for debugging and tracking the number of records fetched, it's generally not recommended to leave console.log statements in production code. Consider applying the same solution as suggested for the previous log statement.
If you decide to keep it, you could refactor it to use a logging library or a conditional statement:
if (process.env.NODE_ENV !== 'production') { console.log("found", body.data.length); }Or using a logging library:
import logger from './your-logging-library'; logger.debug("found", body.data.length);
Line range hint
1-28: Consider adding error handling to the main function.The overall structure and logic of the
mainfunction look good. However, it lacks error handling for the fetch operation and file writing. This could lead to silent failures or incomplete data exports if an error occurs during the process.Consider adding try-catch blocks to handle potential errors. Here's a suggested refactor:
async function main() { const exportFile = Bun.file("./export_audit_log.json"); const writer = exportFile.writer(); let cursor = ""; try { do { if (process.env.NODE_ENV !== 'production') { console.log({ cursor }); } const res = await fetch( `https://api.tinybird.co/v0/pipes/export_audit_log.json?cursor=${cursor}`, { headers: { Authorization: `Bearer ${process.env.TINYBIRD_TOKEN}`, }, }, ); if (!res.ok) { throw new Error(`HTTP error! status: ${res.status}`); } const body = (await res.json()) as { data: { auditLogId: string }[] }; if (process.env.NODE_ENV !== 'production') { console.log("found", body.data.length); } for (const row of body.data) { await writer.write(JSON.stringify(row) + "\n"); } cursor = body.data.at(-1)?.auditLogId ?? ""; } while (cursor); } catch (error) { console.error("An error occurred during the export process:", error); } finally { await writer.end(); } } main().catch(error => console.error("Unhandled error in main:", error));This refactored version:
- Adds a try-catch block to handle potential errors during the fetch and write operations.
- Checks if the fetch response is ok before proceeding.
- Uses async/await for the file writing to properly handle potential errors.
- Ensures the writer is closed in a finally block.
- Adds error handling for the main function call itself.
These changes will make the script more robust and easier to debug if issues occur during the export process.
tools/migrate/main.ts (1)
Line range hint
31-43: Overall, the changes enhance tracking without affecting functionality.The added console.log statements improve the ability to track and debug the migration process without impacting the core functionality. This aligns well with the PR objectives.
As a minor suggestion, consider using a logging library like Winston or Pino instead of console.log for more structured and configurable logging in the future. This would allow for easier log management and potential integration with monitoring tools.
apps/api/src/routes/v1_migrations_createKey.error.test.ts (1)
115-150: Approve with suggestions for improvementThe new test case effectively checks for invalid refill configuration when a daily interval has a non-null refillDay. The structure and assertions are correct. However, consider the following suggestions for improvement:
- Make the test name more specific, e.g., "reject refill config with daily interval and non-null refillDay".
- Use a named constant for the refillDay value (4) to improve readability.
- Consider asserting all relevant properties of the error object in the response.
Example improvement:
test("reject refill config with daily interval and non-null refillDay", async (t) => { // ... (existing setup code) const INVALID_REFILL_DAY = 4; const res = await h.post<V1MigrationsCreateKeysRequest, ErrorResponse>({ // ... (existing request code) body: [ { // ... (other properties) refill: { amount: 100, refillDay: INVALID_REFILL_DAY, interval: "daily", }, }, ], }); expect(res.status).toEqual(400); expect(res.body).toMatchObject({ error: { code: "BAD_REQUEST", docs: "https://unkey.dev/docs/api-reference/errors/code/BAD_REQUEST", message: "when interval is set to 'daily', 'refillDay' must be null.", // Consider adding other relevant properties to assert }, }); });These changes would enhance the test's clarity and maintainability.
apps/api/src/routes/v1_migrations_createKey.happy.test.ts (2)
501-501: Test case name could be more specificThe current test name "should provide default value" is somewhat vague. It would be helpful to specify what default value is being tested.
Consider renaming the test case to be more specific:
- test("should provide default value", async (t) => { + test("should default refillDay to 1 when not provided", async (t) => {
525-525: Explicit undefined valueWhile setting
refillDaytoundefinedis correct, it might be beneficial to add a comment explaining why this is done explicitly, asundefinedis the default value for an omitted property.Consider adding a comment to explain the explicit
undefined:- refillDay: undefined, + refillDay: undefined, // Explicitly set to undefined to test default behaviorapps/api/src/routes/v1_keys_updateKey.ts (2)
356-360: Consider renaminglastDayOfMonthfor clarityThe logic for determining the refill day is correct. However, when
refillDayis provided and valid, the variablelastDayOfMonthno longer represents the last day of the month. Consider renaming it to something likerefillDayOfMonthfor better clarity.Here's a suggested change:
const date = new Date(); -let lastDayOfMonth = new Date(date.getFullYear(), date.getMonth() + 1, 0).getDate(); -if (req.refill?.refillDay && req?.refill?.refillDay <= lastDayOfMonth) { - lastDayOfMonth = req.refill.refillDay; +let refillDayOfMonth = new Date(date.getFullYear(), date.getMonth() + 1, 0).getDate(); +if (req.refill?.refillDay && req?.refill?.refillDay <= refillDayOfMonth) { + refillDayOfMonth = req.refill.refillDay; }
Line range hint
1-1: Reminder: Add test cases for the newrefillDayfeatureThe implementation of the
refillDayfeature looks good, but I don't see any new tests added. Please ensure that you add appropriate test cases to cover this new functionality, including edge cases and error scenarios.Would you like assistance in creating these test cases or opening a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
- apps/api/src/routes/schema.ts (2 hunks)
- apps/api/src/routes/v1_apis_listKeys.ts (1 hunks)
- apps/api/src/routes/v1_keys_createKey.happy.test.ts (1 hunks)
- apps/api/src/routes/v1_keys_createKey.ts (4 hunks)
- apps/api/src/routes/v1_keys_updateKey.error.test.ts (2 hunks)
- apps/api/src/routes/v1_keys_updateKey.happy.test.ts (1 hunks)
- apps/api/src/routes/v1_keys_updateKey.ts (4 hunks)
- apps/api/src/routes/v1_migrations_createKey.error.test.ts (1 hunks)
- apps/api/src/routes/v1_migrations_createKey.happy.test.ts (1 hunks)
- apps/api/src/routes/v1_migrations_createKey.ts (3 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (9 hunks)
- apps/dashboard/lib/trpc/routers/key/create.ts (2 hunks)
- apps/docs/libraries/ts/sdk/keys/get.mdx (1 hunks)
- apps/docs/libraries/ts/sdk/keys/update.mdx (2 hunks)
- apps/workflows/jobs/refill-daily.ts (3 hunks)
- internal/db/src/schema/key_migrations.ts (1 hunks)
- tools/migrate/auditlog-import.ts (2 hunks)
- tools/migrate/main.ts (2 hunks)
- tools/migrate/tinybird-export.ts (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- tools/migrate/auditlog-import.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- apps/api/src/routes/schema.ts
- apps/api/src/routes/v1_apis_listKeys.ts
- apps/api/src/routes/v1_keys_createKey.happy.test.ts
- apps/api/src/routes/v1_keys_createKey.ts
- apps/api/src/routes/v1_keys_updateKey.error.test.ts
- apps/api/src/routes/v1_keys_updateKey.happy.test.ts
- apps/api/src/routes/v1_migrations_createKey.ts
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx
- apps/dashboard/lib/trpc/routers/key/create.ts
- apps/docs/libraries/ts/sdk/keys/get.mdx
- apps/docs/libraries/ts/sdk/keys/update.mdx
- apps/workflows/jobs/refill-daily.ts
- internal/db/src/schema/key_migrations.ts
🧰 Additional context used
📓 Learnings (1)
apps/api/src/routes/v1_migrations_createKey.error.test.ts (1)
Learnt from: MichaelUnkey PR: unkeyed/unkey#2114 File: apps/api/src/routes/v1_keys_updateKey.error.test.ts:0-0 Timestamp: 2024-09-27T15:20:05.475Z Learning: In the `v1/keys.updateKey` endpoint, the server validates the refill configuration before checking if the key exists. Therefore, tests can assert validation errors without needing to create the key first.
🔇 Additional comments (5)
tools/migrate/main.ts (2)
31-32: LGTM: Useful logging for tracking migration progress.The added console.log statement provides valuable information for tracking the creation of new identities during the migration process. The comment to ignore the linter warning is appropriate in this context.
42-43: LGTM: Helpful logging for tracking key-identity connections.The added console.log statement effectively tracks the connection between identities and keys during the migration process. The comment to ignore the linter warning is appropriate in this context.
apps/api/src/routes/v1_keys_updateKey.ts (3)
154-157: LGTM: NewrefillDayfield added correctlyThe new
refillDayfield is correctly implemented as an optional number between 1 and 31, with a clear OpenAPI description explaining its purpose for monthly refills.
340-345: LGTM: Proper validation forrefillDayThe added validation logic correctly prevents setting
refillDaywhen the interval is "daily", throwing an informative error message.
391-391: LGTM: CorrectrefillDayassignmentThe
refillDayis correctly set based on the refill interval. If you accept the suggestion to renamelastDayOfMonthtorefillDayOfMonth, remember to update it here as well.
| @@ -496,3 +496,47 @@ test("migrate and verify a key", async (t) => { | |||
| expect(verifyRes.status).toBe(200); | |||
| expect(verifyRes.body.valid).toEqual(true); | |||
| }); | |||
|
|
|||
| describe("Should default last day of month if none provided", () => { | |||
There was a problem hiding this comment.
Incorrect test suite description
The describe block's description is inconsistent with the actual behavior being tested. The test is checking for the default to be the first day of the month, not the last day.
Consider changing the description to:
-describe("Should default last day of month if none provided", () => {
+describe("Should default to first day of month if none provided", () => {📝 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("Should default last day of month if none provided", () => { | |
| describe("Should default to first day of month if none provided", () => { |
| expect(found?.remaining).toEqual(10); | ||
| expect(found?.refillAmount).toEqual(130); | ||
| expect(found?.refillInterval).toEqual("monthly"); | ||
| expect(found?.refillDay).toEqual(1); |
There was a problem hiding this comment.
Verify all relevant properties
The test correctly verifies several properties of the created key. However, there's a discrepancy between the requested refillAmount (100) and the verified amount (130).
Investigate why the refillAmount is 130 instead of 100 as specified in the request. If this is expected behavior, add a comment explaining the discrepancy. If not, update either the request or the assertion to match the expected behavior.
Also, consider adding an assertion to verify that the hash matches the one provided in the request:
expect(found?.remaining).toEqual(10);
-expect(found?.refillAmount).toEqual(130);
+expect(found?.refillAmount).toEqual(100); // Or explain why it's 130 if that's the expected behavior
expect(found?.refillInterval).toEqual("monthly");
expect(found?.refillDay).toEqual(1);
+expect(found?.hash).toEqual(hash);Committable suggestion was skipped due to low confidence.
…-allow-developer-to-set-a-custom-refill-time-when-using-the
…en-using-the' of https://github.com/unkeyed/unkey into eng-1324-allow-developer-to-set-a-custom-refill-time-when-using-the
What does this PR do?
Eng-1324
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Release Notes
New Features
refillDayfor specifying the day of the month for refills, applicable when the refill interval is set to "monthly."refillDay.Documentation
refillDay, including its default behavior and usage examples.Bug Fixes
Tests
refillDayparameter and ensure proper error handling for invalid configurations.