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.

Sign up for GitHub

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

feat: removal of refillInterval #2709

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
b320bce
removal of refillInterval
Dec 5, 2024
c38f7e0
Eliminated refs to refillInterval
Dec 5, 2024
eeea567
More fixes for types and tests using refillInterval
Dec 6, 2024
cd95856
Merge branch 'main' of https://github.com/unkeyed/unkey into remove-r…
Dec 9, 2024
da2f8f7
migration changes
Dec 9, 2024
130f749
reverted change
Dec 9, 2024
5cbccbc
Merge branch 'main' of https://github.com/unkeyed/unkey into remove-r…
Dec 9, 2024
0617d38
Merge branch 'main' of https://github.com/unkeyed/unkey into remove-r…
Dec 13, 2024
84b3929
save before pull main
Dec 16, 2024
ffbb21d
Merge branch 'main' of https://github.com/unkeyed/unkey into remove-r…
Dec 16, 2024
ea6fd74
fixed checks for correct data in refill
Dec 16, 2024
624bb92
partial Fixes
Dec 16, 2024
a55c17d
changed new and update key form
Dec 16, 2024
68f270e
Merge branch 'main' of https://github.com/unkeyed/unkey into remove-r…
Dec 17, 2024
932b4b6
revert api changes
Dec 17, 2024
775fc54
mod refill migration
Dec 17, 2024
16df3fc
Refined migration
Dec 17, 2024
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/handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ export async function migrateKey(
createdAt: new Date(),
createdAtM: Date.now(),
expires: message.expires ? new Date(message.expires) : null,
refillInterval: message.refill?.interval,
refillAmount: message.refill?.amount,
refillDay: message.refill?.refillDay,
enabled: message.enabled,
Expand Down
3 changes: 1 addition & 2 deletions apps/api/src/pkg/key_migration/message.ts
View file
Open in desktop
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 All @@ -14,7 +13,7 @@ export type MessageBody = {
permissions?: string[];
expires?: number;
remaining?: number;
refill?: { interval: "daily" | "monthly"; amount: number; refillDay?: number };
refill?: { amount: number; refillDay?: number };
ratelimit?: { async: boolean; limit: number; duration: number };
enabled: boolean;
environment?: string;
Expand Down
4 changes: 2 additions & 2 deletions apps/api/src/routes/schema.ts
View file
Open in desktop
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({
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
5 changes: 2 additions & 3 deletions apps/api/src/routes/v1_apis_listKeys.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,11 +318,10 @@ export const registerV1ApisListKeys = (app: App) =>
: undefined,
remaining: k.remaining ?? undefined,
refill:
k.refillInterval && k.refillAmount && k.lastRefillAt
k.refillAmount && k.lastRefillAt
? {
interval: k.refillInterval,
amount: k.refillAmount,
refillDay: k.refillInterval === "monthly" && k.refillDay ? k.refillDay : null,
refillDay: k.refillDay ?? null,
lastRefillAt: k.lastRefillAt?.getTime(),
}
: undefined,
Expand Down
32 changes: 0 additions & 32 deletions apps/api/src/routes/v1_keys_createKey.error.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,35 +118,3 @@ test("when key recovery is not enabled", async (t) => {
},
});
});

test("reject invalid refill config when daily interval has non-null refillDay", async (t) => {
const h = await IntegrationHarness.init(t);

const root = await h.createRootKey([`api.${h.resources.userApi.id}.create_key`]);

const res = await h.post<V1KeysCreateKeyRequest, V1KeysCreateKeyResponse>({
url: "/v1/keys.createKey",
headers: {
"Content-Type": "application/json",
Authorization: `Bearer ${root.key}`,
},
body: {
byteLength: 16,
apiId: h.resources.userApi.id,
remaining: 10,
refill: {
amount: 100,
refillDay: 4,
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.",
},
});
});
31 changes: 0 additions & 31 deletions apps/api/src/routes/v1_keys_createKey.happy.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,35 +467,4 @@ describe("with externalId", () => {
expect(key!.identity!.id).toEqual(identity.id);
});
});
describe("Should default first day of month if none provided", () => {
test("should provide default value", async (t) => {
const h = await IntegrationHarness.init(t);
const root = await h.createRootKey([`api.${h.resources.userApi.id}.create_key`]);

const res = await h.post<V1KeysCreateKeyRequest, V1KeysCreateKeyResponse>({
url: "/v1/keys.createKey",
headers: {
"Content-Type": "application/json",
Authorization: `Bearer ${root.key}`,
},
body: {
apiId: h.resources.userApi.id,
remaining: 10,
refill: {
interval: "monthly",
amount: 20,
refillDay: undefined,
},
},
});

expect(res.status, `expected 200, received: ${JSON.stringify(res, null, 2)}`).toBe(200);

const key = await h.db.primary.query.keys.findFirst({
where: (table, { eq }) => eq(table.id, res.body.keyId),
});
expect(key).toBeDefined();
expect(key!.refillDay).toEqual(1);
});
});
});
34 changes: 18 additions & 16 deletions apps/api/src/routes/v1_keys_createKey.ts
View file
Open in desktop
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({
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 All @@ -132,7 +132,6 @@ When validating a key, we will return this back to you, so you can clearly ident
description:
"Unkey enables you to refill verifications for each key at regular intervals.",
example: {
interval: "monthly",
amount: 100,
refillDay: 15,
},
Expand Down Expand Up @@ -313,18 +312,21 @@ 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.",
-        });
-      }

}

/**
* Set up an api for production
*/
Expand Down Expand Up @@ -373,10 +375,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,
refillDay: req.refill?.interval === "daily" ? null : req?.refill?.refillDay ?? 1,
refillInterval: req.refill?.interval ?? null,
refillDay: req?.refill?.refillDay ?? null,
refillAmount: req.refill?.amount,
lastRefillAt: req.refill?.interval ? new Date() : null,
lastRefillAt: null,
deletedAt: null,
enabled: req.enabled,
environment: req.environment ?? null,
Expand Down
17 changes: 8 additions & 9 deletions apps/api/src/routes/v1_keys_getKey.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,15 +150,14 @@ 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
? {
Expand Down
1 change: 0 additions & 1 deletion apps/api/src/routes/v1_keys_updateKey.happy.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1054,7 +1054,6 @@ describe("When refillDay is omitted.", () => {
expect(found).toBeDefined();
expect(found?.remaining).toEqual(10);
expect(found?.refillAmount).toEqual(130);
expect(found?.refillInterval).toEqual("monthly");
expect(found?.refillDay).toEqual(1);
});
});
Expand Down
2 changes: 0 additions & 2 deletions apps/api/src/routes/v1_keys_updateKey.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,12 +389,10 @@ export const registerV1KeysUpdate = (app: App) =>

if (typeof req.refill !== "undefined") {
if (req.refill === null) {
changes.refillInterval = null;
changes.refillAmount = null;
changes.refillDay = null;
changes.lastRefillAt = null;
} else {
changes.refillInterval = req.refill.interval;
changes.refillAmount = req.refill.amount;
changes.refillDay = req.refill.refillDay ?? 1;
}
Expand Down
36 changes: 0 additions & 36 deletions apps/api/src/routes/v1_migrations_createKey.error.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,39 +112,3 @@ test("reject invalid ratelimit config", async (t) => {
expect(res.status).toEqual(400);
expect(res.body.error.code).toEqual("BAD_REQUEST");
});
test("reject invalid refill config when daily interval has non-null refillDay", async (t) => {
const h = await IntegrationHarness.init(t);
const { key } = await h.createRootKey(["*"]);

const res = await h.post<V1MigrationsCreateKeysRequest, ErrorResponse>({
url: "/v1/migrations.createKeys",
headers: {
"Content-Type": "application/json",
Authorization: `Bearer ${key}`,
},
body: [
{
start: "x",
hash: {
value: "x",
variant: "sha256_base64",
},
apiId: h.resources.userApi.id,
remaining: 10,
refill: {
amount: 100,
refillDay: 4,
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.",
},
});
});
2 changes: 0 additions & 2 deletions apps/api/src/routes/v1_migrations_createKey.happy.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,6 @@ describe("Should default to first day of month if none provided", () => {
enabled: true,
remaining: 10,
refill: {
interval: "monthly",
amount: 100,
refillDay: undefined,
},
Expand All @@ -536,7 +535,6 @@ describe("Should default to first day of month if none provided", () => {
expect(found).toBeDefined();
expect(found?.remaining).toEqual(10);
expect(found?.refillAmount).toEqual(100);
expect(found?.refillInterval).toEqual("monthly");
expect(found?.refillDay).toEqual(1);
expect(found?.hash).toEqual(hash);
});
Expand Down
54 changes: 34 additions & 20 deletions apps/api/src/routes/v1_migrations_createKey.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,25 +123,30 @@ 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",
refillDay: 15,
amount: 100,
},
}),
Expand Down Expand Up @@ -378,11 +383,19 @@ export const registerV1MigrationsCreateKeys = (app: App) =>
});
}

if ((key.remaining === null || key.remaining === undefined) && key.refill?.interval) {
throw new UnkeyApiError({
code: "BAD_REQUEST",
message: "remaining must be set if you are using refill.",
});
if (key.refill) {
if (key.remaining === null || key.remaining === undefined) {
throw new UnkeyApiError({
code: "BAD_REQUEST",
message: "remaining must be set if you are using refill.",
});
}
if (!key.refill.amount) {
throw new UnkeyApiError({
code: "BAD_REQUEST",
message: "refill.amount must be set if you are using refill.",
});
}
}

if (!key.hash && !key.plaintext) {
Expand All @@ -391,18 +404,19 @@ export const registerV1MigrationsCreateKeys = (app: App) =>
message: "provide either `hash` or `plaintext`",
});
}
if (key.refill?.refillDay && key.refill.interval === "daily") {
throw new UnkeyApiError({
code: "BAD_REQUEST",
message: "when interval is set to 'daily', 'refillDay' must be null.",
});
}

/**
* Set up an api for production
*/

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 All @@ -420,8 +434,8 @@ export const registerV1MigrationsCreateKeys = (app: App) =>
ratelimitLimit: key.ratelimit?.limit ?? key.ratelimit?.refillRate ?? null,
ratelimitDuration: key.ratelimit?.refillInterval ?? key.ratelimit?.refillInterval ?? null,
remaining: key.remaining ?? null,
refillInterval: key.refill?.interval ?? null,
refillDay: key.refill?.interval === "daily" ? null : key?.refill?.refillDay ?? 1,
refillInterval: null,
refillDay: key?.refill?.refillDay ?? 1,
refillAmount: key.refill?.amount ?? null,
deletedAt: null,
enabled: key.enabled ?? true,
Expand Down
Loading
Loading