Skip to content
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

feat: removal of refillInterval #2709

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion apps/api/src/pkg/key_migration/message.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ export type MessageBody = {
keyAuthId: string;
rootKeyId: string;
prefix?: string;

name?: string;
hash: string;
start?: string;
Expand Down
4 changes: 2 additions & 2 deletions apps/api/src/routes/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export const keySchema = z
}),
refill: z
.object({
interval: z.enum(["daily", "monthly"]).openapi({
MichaelUnkey marked this conversation as resolved.
Show resolved Hide resolved
interval: z.enum(["daily", "monthly"]).optional().openapi({
description:
"Determines the rate at which verifications will be refilled. When 'daily' is set for 'interval' 'refillDay' will be set to null.",
example: "daily",
Expand All @@ -69,7 +69,7 @@ export const keySchema = z
}),
refillDay: z.number().min(1).max(31).default(1).nullable().openapi({
description:
"The day verifications will refill each month, when interval is set to 'monthly'. Value is not zero-indexed making 1 the first day of the month. If left blank it will default to the first day of the month. When 'daily' is set for 'interval' 'refillDay' will be set to null.",
"The amount will refill on the day of the month specified on `refillDay`. If `refillDay` beyond the last day in the month, it will refill on the last day of the month. If left empty, it will refill daily.",
example: 15,
}),
lastRefillAt: z.number().int().optional().openapi({
Expand Down
6 changes: 3 additions & 3 deletions apps/api/src/routes/v1_apis_listKeys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -318,11 +318,11 @@ export const registerV1ApisListKeys = (app: App) =>
: undefined,
remaining: k.remaining ?? undefined,
refill:
k.refillInterval && k.refillAmount && k.lastRefillAt
k.refillAmount && k.lastRefillAt
? {
interval: k.refillInterval,
interval: k.refillInterval ?? undefined,
amount: k.refillAmount,
refillDay: k.refillInterval === "monthly" && k.refillDay ? k.refillDay : null,
refillDay: k.refillDay ?? null,
Comment on lines +321 to +325
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove interval from response to align with PR objectives

The PR objective is to remove the refillInterval concept, but the response still includes the interval field. This could cause confusion for API consumers.

Consider updating the refill object construction:

          k.refillAmount && k.lastRefillAt
            ? {
-                interval: k.refillInterval ?? undefined,
                 amount: k.refillAmount,
                 refillDay: k.refillDay ?? null,
                 lastRefillAt: k.lastRefillAt?.getTime(),
              }
            : undefined,
📝 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.

Suggested change
k.refillAmount && k.lastRefillAt
? {
interval: k.refillInterval,
interval: k.refillInterval ?? undefined,
amount: k.refillAmount,
refillDay: k.refillInterval === "monthly" && k.refillDay ? k.refillDay : null,
refillDay: k.refillDay ?? null,
k.refillAmount && k.lastRefillAt
? {
amount: k.refillAmount,
refillDay: k.refillDay ?? null,

lastRefillAt: k.lastRefillAt?.getTime(),
}
: undefined,
Expand Down
2 changes: 1 addition & 1 deletion apps/api/src/routes/v1_keys_createKey.error.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ test("reject invalid refill config when daily interval has non-null refillDay",
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.",
message: "When interval is set to 'daily', 'refillDay' must be null.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Test case needs to be updated for new refill logic

This test case appears to be testing the old behavior where daily interval cannot have a refillDay. However, according to the PR objectives, we're removing the refillInterval concept entirely. This test should either be removed or updated to reflect the new behavior.

Consider removing this test case entirely since it's testing deprecated behavior, or update it to test the new requirements where:

  • Monthly interval should always have refillDay as 1
  • Daily interval should have refillDay as null

},
});
});
37 changes: 23 additions & 14 deletions apps/api/src/routes/v1_keys_createKey.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ When validating a key, we will return this back to you, so you can clearly ident
}),
refill: z
.object({
interval: z.enum(["daily", "monthly"]).openapi({
MichaelUnkey marked this conversation as resolved.
Show resolved Hide resolved
interval: z.enum(["daily", "monthly"]).optional().openapi({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Schema consistency needs to be updated across multiple files

The verification reveals inconsistencies in the interval schema definition across different files:

  • v1_keys_createKey.ts: interval is correctly marked as optional
  • v1_migrations_enqueueKeys.ts: interval is not optional
  • v1_keys_updateKey.ts: interval is not optional
  • schema.ts: interval is correctly marked as optional

These files need to be updated to maintain consistency with the PR objective of removing refillInterval:

  • apps/api/src/routes/v1_migrations_enqueueKeys.ts
  • apps/api/src/routes/v1_keys_updateKey.ts
🔗 Analysis chain

Verify schema consistency across the codebase

The change to make interval optional aligns with the PR objective. Let's verify this change is consistent across all schema definitions.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining non-optional interval definitions in schemas
# and verify the change is consistent across the codebase

# Search for schema definitions containing interval
rg -A 5 'interval.*enum.*daily.*monthly' 

# Search for any remaining refillInterval references
rg 'refillInterval'

Length of output: 10139

description: "Unkey will automatically refill verifications at the set interval.",
}),
amount: z.number().int().min(1).positive().openapi({
Expand Down Expand Up @@ -313,18 +313,27 @@ export const registerV1KeysCreateKey = (app: App) =>
message: "remaining must be greater than 0.",
});
}
if ((req.remaining === null || req.remaining === undefined) && req.refill?.interval) {
throw new UnkeyApiError({
code: "BAD_REQUEST",
message: "remaining must be set if you are using refill.",
});
}
if (req.refill?.refillDay && req.refill.interval === "daily") {
throw new UnkeyApiError({
code: "BAD_REQUEST",
message: "when interval is set to 'daily', 'refillDay' must be null.",
});
if (req.refill) {
if (req.remaining === null || req.remaining === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments below.

throw new UnkeyApiError({
code: "BAD_REQUEST",
message: "remaining must be set if you are using refill.",
});
}
if (!req.refill.amount) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 amount will cause false negative here.

throw new UnkeyApiError({
code: "BAD_REQUEST",
message: "refill.amount must be set if you are using refill.",
});
}
Comment on lines +323 to +328
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove redundant validation

This validation for refill.amount is redundant as it's already enforced by the schema definition (required field with min(1).positive()).

-      if (!req.refill.amount) {
-        throw new UnkeyApiError({
-          code: "BAD_REQUEST",
-          message: "refill.amount must be set if you are using refill.",
-        });
-      }

if (req.refill.interval === "daily" && req.refill.refillDay) {
throw new UnkeyApiError({
code: "BAD_REQUEST",
message: "When interval is set to 'daily', 'refillDay' must be null.",
});
}
}

/**
* Set up an api for production
*/
Expand Down Expand Up @@ -373,10 +382,10 @@ export const registerV1KeysCreateKey = (app: App) =>
ratelimitLimit: req.ratelimit?.limit ?? req.ratelimit?.refillRate,
ratelimitDuration: req.ratelimit?.duration ?? req.ratelimit?.refillInterval,
remaining: req.remaining,
refillInterval: req.refill?.interval,
refillInterval: req.refill?.interval ?? null,
refillDay: req.refill?.interval === "daily" ? null : req?.refill?.refillDay ?? 1,
refillAmount: req.refill?.amount,
lastRefillAt: req.refill?.interval ? new Date() : null,
lastRefillAt: null,
Comment on lines +385 to +388
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix inconsistency with refillDay default value

According to the PR objectives, refillDay should be set to 1 only when the interval is monthly, but the current implementation sets it to 1 by default even when the interval is null.

-        refillDay: req.refill?.interval === "daily" ? null : req?.refill?.refillDay ?? 1,
+        refillDay: req.refill?.interval === "monthly" ? (req?.refill?.refillDay ?? 1) : null,
📝 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.

Suggested change
refillInterval: req.refill?.interval ?? null,
refillDay: req.refill?.interval === "daily" ? null : req?.refill?.refillDay ?? 1,
refillAmount: req.refill?.amount,
lastRefillAt: req.refill?.interval ? new Date() : null,
lastRefillAt: null,
refillInterval: req.refill?.interval ?? null,
refillDay: req.refill?.interval === "monthly" ? (req?.refill?.refillDay ?? 1) : null,
refillAmount: req.refill?.amount,
lastRefillAt: null,

deletedAt: null,
enabled: req.enabled,
environment: req.environment ?? null,
Expand Down
18 changes: 8 additions & 10 deletions apps/api/src/routes/v1_keys_getKey.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,20 +150,18 @@ export const registerV1KeysGetKey = (app: App) =>
updatedAt: key.updatedAtM ?? undefined,
expires: key.expires?.getTime() ?? undefined,
remaining: key.remaining ?? undefined,
refill:
key.refillInterval && key.refillAmount
? {
interval: key.refillInterval,
amount: key.refillAmount,
refillDay: key.refillInterval === "monthly" ? key.refillDay : null,
lastRefillAt: key.lastRefillAt?.getTime(),
}
: undefined,
refill: key.refillAmount
? {
interval: key.refillInterval ?? undefined,
amount: key.refillAmount,
refillDay: key.refillDay ?? null,
lastRefillAt: key.lastRefillAt?.getTime(),
}
: undefined,
ratelimit:
key.ratelimitAsync !== null && key.ratelimitLimit !== null && key.ratelimitDuration !== null
? {
async: key.ratelimitAsync,

type: key.ratelimitAsync ? "fast" : ("consistent" as any),
limit: key.ratelimitLimit,
duration: key.ratelimitDuration,
Expand Down
26 changes: 19 additions & 7 deletions apps/api/src/routes/v1_migrations_createKey.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,25 +123,31 @@ When validating a key, we will return this back to you, so you can clearly ident
}),
refill: z
.object({
interval: z.enum(["daily", "monthly"]).openapi({
interval: z.enum(["monthly", "daily"]).optional().openapi({
description:
"Unkey will automatically refill verifications at the set interval.",
"The interval at which we will refill the remaining verifications.",
}),
amount: z.number().int().min(1).positive().openapi({
description:
"The number of verifications to refill for each occurrence is determined individually for each key.",
}),
refillDay: z.number().min(1).max(31).optional().openapi({
description:
"The day verifications will refill each month, when interval is set to 'monthly'",
}),
refillDay: z
.number()
.min(1)
.max(31)
.optional()
.openapi({
description: `The day of the month, when we will refill the remaining verifications. To refill on the 15th of each month, set 'refillDay': 15.
If the day does not exist, for example you specified the 30th and it's february, we will refill them on the last day of the month instead.`,
}),
})
.optional()
.openapi({
description:
"Unkey enables you to refill verifications for each key at regular intervals.",
example: {
interval: "daily",
interval: "monthly",
refillDay: 15,
amount: 100,
},
}),
Expand Down Expand Up @@ -403,6 +409,12 @@ export const registerV1MigrationsCreateKeys = (app: App) =>

const hash = key.plaintext ? await sha256(key.plaintext) : key.hash!.value;

if (key.refill?.interval === "monthly" && key.refill?.refillDay === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments below.

key.refill.refillDay = 1;
}
if (key.refill?.interval === "daily" && key.refill?.refillDay !== undefined) {
key.refill.refillDay = undefined;
}
keys.push({
id: key.keyId,
keyAuthId: api.keyAuthId!,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,6 @@ import {
} from "@/components/ui/form";
import { Input } from "@/components/ui/input";
import { Label } from "@/components/ui/label";
import {
Select,
SelectContent,
SelectItem,
SelectTrigger,
SelectValue,
} from "@/components/ui/select";
import { Switch } from "@/components/ui/switch";
import { toast } from "@/components/ui/toaster";
import { trpc } from "@/lib/trpc/client";
Expand All @@ -42,27 +35,14 @@ const formSchema = z.object({
remaining: z.coerce.number().positive({ message: "Please enter a positive number" }).optional(),
refill: z
.object({
interval: z.enum(["none", "daily", "monthly"]),
amount: z.coerce
.number()
.int()
.min(1, {
message: "Please enter the number of uses per interval",
})
.positive()
.optional(),
refillDay: z.coerce
.number({
errorMap: (issue, { defaultError }) => ({
message:
issue.code === "invalid_type"
? "Refill day must be an integer between 1 and 31"
: defaultError,
}),
})
.int()
.min(1)
.max(31)
amount: z
.literal("")
.transform(() => undefined)
.or(z.coerce.number().int().positive()),
refillDay: z
.literal("")
.transform(() => undefined)
.or(z.coerce.number().int().max(31).positive())
.optional(),
})
.optional(),
Expand All @@ -72,9 +52,8 @@ type Props = {
id: string;
workspaceId: string;
remaining: number | null;
refillInterval: "daily" | "monthly" | null;
refillAmount: number | null;
refillDay: number | null;
refillDay: number | null | undefined;
};
};

Expand All @@ -90,18 +69,16 @@ export const UpdateKeyRemaining: React.FC<Props> = ({ apiKey }) => {
limitEnabled: apiKey.remaining ? true : false,
remaining: apiKey.remaining ? apiKey.remaining : undefined,
refill: {
interval: apiKey.refillInterval === null ? "none" : apiKey.refillInterval,
amount: apiKey.refillAmount ? apiKey.refillAmount : undefined,
refillDay:
apiKey.refillInterval === "monthly" && apiKey.refillDay ? apiKey.refillDay : undefined,
refillDay: apiKey.refillDay ?? undefined,
},
},
});
const resetLimited = () => {
// set them to undefined so the form resets properly.
form.resetField("remaining", undefined);
form.resetField("refill.amount", undefined);
form.resetField("refill.interval", { defaultValue: "none" });
form.resetField("refill.refillDay", undefined);
form.resetField("refill", undefined);
};
const updateRemaining = trpc.key.update.remaining.useMutation({
Expand All @@ -116,23 +93,20 @@ export const UpdateKeyRemaining: React.FC<Props> = ({ apiKey }) => {
});

async function onSubmit(values: z.infer<typeof formSchema>) {
if (values.refill?.interval !== "none" && !values.refill?.amount) {
if (!values.refill?.amount && values.refill?.refillDay) {
form.setError("refill.amount", {
message: "Please enter the number of uses per interval",
message: "Please enter the number of uses per interval or remove the refill day",
});
return;
}
if (values.refill.interval !== "none" && values.remaining === undefined) {
if (values.remaining === undefined) {
MichaelUnkey marked this conversation as resolved.
Show resolved Hide resolved
form.setError("remaining", { message: "Please enter a value" });
return;
}
if (values.limitEnabled === false) {
delete values.refill;
delete values.remaining;
}
if (values.refill?.interval === "none") {
delete values.refill;
}
await updateRemaining.mutateAsync(values);
}

Expand Down Expand Up @@ -176,37 +150,11 @@ export const UpdateKeyRemaining: React.FC<Props> = ({ apiKey }) => {
</FormItem>
)}
/>

<FormField
control={form.control}
name="refill.interval"
render={({ field }) => (
<FormItem className="">
<FormLabel>Refill Rate</FormLabel>
<Select
onValueChange={field.onChange}
defaultValue="none"
value={field.value}
disabled={!form.watch("limitEnabled") === true}
>
<SelectTrigger>
<SelectValue />
</SelectTrigger>
<SelectContent>
<SelectItem value="none">None</SelectItem>
<SelectItem value="daily">Daily</SelectItem>
<SelectItem value="monthly">Monthly</SelectItem>
</SelectContent>
</Select>
<FormMessage />
</FormItem>
)}
/>
<FormField
control={form.control}
disabled={
form.watch("refill.interval") === "none" ||
form.watch("refill.interval") === undefined ||
form.watch("remaining") === undefined ||
form.watch("remaining") === null ||
Comment on lines +156 to +157
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If remaining being 0 is not a concern let's just do !form.watch("remaining")

!form.watch("limitEnabled") === true
}
name="refill.amount"
Expand All @@ -219,34 +167,40 @@ export const UpdateKeyRemaining: React.FC<Props> = ({ apiKey }) => {
className="w-full"
type="number"
{...field}
value={form.watch("refill.interval") === "none" ? undefined : field.value}
value={form.getValues("limitEnabled") ? field.value : undefined}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getValues won't get updated when limitEnabled changes, so that might cause some issues. Just giving heads up.

/>
</FormControl>
<FormDescription>
Enter the number of uses to refill per interval.
</FormDescription>
<FormMessage defaultValue="Please enter a value if interval is selected" />
<FormMessage />
</FormItem>
)}
/>
<FormField
control={form.control}
disabled={form.watch("refill.interval") !== "monthly"}
disabled={
form.watch("remaining") === undefined ||
form.watch("remaining") === null ||
Comment on lines +183 to +184
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If remaining being 0 is not a concern let's just do !form.watch("remaining")

!form.watch("limitEnabled") === true
Comment on lines +183 to +185
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can do this !form.watch("limitEnabled") instead of !form.watch("limitEnabled") === true

}
name="refill.refillDay"
render={({ field }) => (
<FormItem className="mt-4">
<FormLabel>Day of the month to refill uses</FormLabel>
<FormLabel>Day of the month to refill uses.</FormLabel>
<FormControl>
<Input
placeholder="1"
placeholder="day of the month"
className="w-full"
type="number"
{...field}
value={form.getValues("limitEnabled") ? field.value : undefined}
/>
</FormControl>
<FormDescription>Enter the day to refill monthly.</FormDescription>
<FormMessage defaultValue="Please enter a value if interval of monthly is selected" />
<FormDescription>
Enter the day to refill monthly or leave blank for daily refill
</FormDescription>
<FormMessage />
</FormItem>
)}
/>
Expand Down
Loading
Loading