-
Notifications
You must be signed in to change notification settings - Fork 607
feat: removal of refillInterval #2709
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
Changes from all commits
b320bce
c38f7e0
eeea567
cd95856
da2f8f7
130f749
5cbccbc
0617d38
84b3929
ffbb21d
ea6fd74
624bb92
a55c17d
68f270e
932b4b6
775fc54
16df3fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||||||||||||||||||
| lastRefillAt: k.lastRefillAt?.getTime(), | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| : undefined, | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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.", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
|
||
| }, | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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({ | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
These files need to be updated to maintain consistency with the PR objective of removing
🔗 Analysis chainVerify schema consistency across the codebase The change to make 🏁 Scripts executedThe 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({ | ||||||||||||||||||||
|
|
@@ -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) { | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||||
| throw new UnkeyApiError({ | ||||||||||||||||||||
| code: "BAD_REQUEST", | ||||||||||||||||||||
| message: "refill.amount must be set if you are using refill.", | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
Comment on lines
+323
to
+328
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Remove redundant validation This validation for - 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 | ||||||||||||||||||||
| */ | ||||||||||||||||||||
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix inconsistency with refillDay default value According to the PR objectives, - refillDay: req.refill?.interval === "daily" ? null : req?.refill?.refillDay ?? 1,
+ refillDay: req.refill?.interval === "monthly" ? (req?.refill?.refillDay ?? 1) : null,📝 Committable suggestion
Suggested change
|
||||||||||||||||||||
| deletedAt: null, | ||||||||||||||||||||
| enabled: req.enabled, | ||||||||||||||||||||
| environment: req.environment ?? null, | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, | ||
| }, | ||
| }), | ||
|
|
@@ -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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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!, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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"; | ||
|
|
@@ -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(), | ||
|
|
@@ -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; | ||
| }; | ||
| }; | ||
|
|
||
|
|
@@ -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({ | ||
|
|
@@ -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); | ||
| } | ||
|
|
||
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If remaining being |
||
| !form.watch("limitEnabled") === true | ||
| } | ||
| name="refill.amount" | ||
|
|
@@ -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} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| /> | ||
| </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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If remaining being |
||
| !form.watch("limitEnabled") === true | ||
|
Comment on lines
+183
to
+185
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can do this |
||
| } | ||
| 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> | ||
| )} | ||
| /> | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.