chore: migrate single rate limits to multi rate limits#3328
Conversation
|
📝 WalkthroughWalkthroughThis change migrates rate limit configuration from embedded fields on key records to a dedicated ratelimits table, introducing support for multiple named ratelimits per key with an Changes
Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant API
participant DB
Dashboard->>API: Create/Update Key (with ratelimits array)
API->>DB: Insert/Update key record
API->>DB: Insert/Update ratelimit records (with autoApply flags)
DB-->>API: Confirmation
API-->>Dashboard: Key and ratelimit data
Dashboard->>API: List/Get Key(s)
API->>DB: Query key(s) with ratelimits relation
DB-->>API: Key(s) + ratelimits[]
API-->>Dashboard: Key(s) + ratelimits[]
sequenceDiagram
participant API
participant DB
API->>DB: Verify Key (fetch key + ratelimits)
API->>DB: For each ratelimit where autoApply=true, apply rate limit
DB-->>API: Rate limit check results
API-->>Client: Verification result (with applied ratelimits)
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code Graph Analysis (1)apps/dashboard/lib/trpc/routers/key/updateRatelimit.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms (15)
🔇 Additional comments (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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! 🙏 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
chronark
requested review from
MichaelUnkey,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🔭 Outside diff range comments (9)
apps/dashboard/lib/trpc/routers/key/create.ts (1)
89-102:autoApplymay beundefined– add a fallback
ratelimit.autoApplyis typed as optional in the UI layer.
Passingundefinedhere will insert a DBNULL, which is likely to violate theNOT NULL DEFAULT falseconstraint onratelimits.auto_apply.- autoApply: ratelimit.autoApply, + // fallback to DB default (false) if the flag is omitted + autoApply: ratelimit.autoApply ?? false,apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.schema.ts (1)
255-263: Minor: align default object with updated schemaWhen
autoApplygains a default, you can drop the explicitautoApply: truehere to keep the default source-of-truth single.- autoApply: true, + // autoApply defaults to trueapps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/ratelimit-setup.tsx (1)
48-55: New rule helper should mirror default stateThe helper used by “Add additional ratelimit” also hard-codes
autoApply: false. Consider inheriting the current “Default” or explicitly asking the user.apps/dashboard/lib/trpc/routers/api/keys/api-query.ts (1)
224-230: Sub-query for identity filter can degrade performanceThe
EXISTS ( SELECT 1 FROM ${identities} … )sub-query is executed once per key whenidentitiesFromInputis provided. On large keyspaces this quickly becomes the dominant cost.
Consider aLEFT JOIN identities ON identities.id = key.identityIdcombined with a singleWHEREclause; the optimiser can then use indexes onexternal_idand avoid N×sub-query evaluation.internal/db/src/schema/identity.ts (1)
64-71: Missing CHECK to enforce XOR betweenkeyIdandidentityIdThe comment says “Either keyId or identityId may be defined, not both”, yet the table has no check-constraint guaranteeing this invariant.
Leaving it to application code risks accidental data corruption.+ /* ensure exactly one of keyId / identityId is non-null */ + CHECK ( + (key_id IS NOT NULL AND identity_id IS NULL) + OR + (key_id IS NULL AND identity_id IS NOT NULL) + ),MySQL 8.0+ supports
CHECKconstraints; DRIZZLE will emit them.apps/api/src/routes/v1_keys_getKey.ts (1)
143-174: Endpoint still exposes a single ratelimit – loses multi-limit informationThe handler collapses
key.ratelimitsto the first"default"entry and returns it under the legacyratelimitfield, discarding:
- additional named limits attached to the key;
- the
autoApplyflag;- whether the limit is per-identity or per-key.
If external clients need the full multi-ratelimit picture, consider returning an array:
- ratelimit: ratelimit ? { …legacyShape } : undefined, + ratelimits: key.ratelimits.map(({ name, limit, duration, autoApply }) => ({ + name, + limit, + duration, + autoApply, + })),A versioned response schema keeps backward compatibility for old consumers.
apps/dashboard/lib/trpc/routers/key/updateRatelimit.ts (1)
85-111:autoApplyflag ignored on create / updateNew ratelimits are stored without
autoApply, and updates never touch the flag.
Down-stream verification relies onautoApply=trueto pick defaults, so keys created/edited here may silently lose rate-limit enforcement.- await tx.insert(schema.ratelimits).values({ + await tx.insert(schema.ratelimits).values({ id: newId("ratelimit"), keyId: input.keyId, duration: ratelimit.refillInterval, limit: ratelimit.limit, name: ratelimit.name, workspaceId: ctx.workspaceId, + autoApply: ratelimit.autoApply ?? true, createdAt: Date.now(), updatedAt: null, });Also add
autoApplyhandling in the update branch.apps/api/src/pkg/keys/service.ts (2)
610-614: Left-over debug log
console.warn("ABC")slipped in—pollutes logs and may leak stack traces in production. Remove before merging.
720-723: Incorrect limit value returned
limit: ratelimits.default?.limitreturnsundefinedunless a ratelimit named"default"exists, even when another limit triggered. Either returnratelimit.limitfrom the triggered limiter result, or look up byres.val.triggered. Current response can mislead clients.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (31)
apps/api/src/pkg/cache/index.ts(1 hunks)apps/api/src/pkg/cache/namespaces.ts(3 hunks)apps/api/src/pkg/key_migration/handler.ts(1 hunks)apps/api/src/pkg/keys/service.ts(4 hunks)apps/api/src/routes/legacy_apis_listKeys.ts(2 hunks)apps/api/src/routes/legacy_keys_createKey.ts(1 hunks)apps/api/src/routes/v1_apis_listKeys.ts(3 hunks)apps/api/src/routes/v1_keys_createKey.ts(1 hunks)apps/api/src/routes/v1_keys_deleteKey.ts(2 hunks)apps/api/src/routes/v1_keys_getKey.ts(4 hunks)apps/api/src/routes/v1_keys_getVerifications.ts(3 hunks)apps/api/src/routes/v1_keys_updateKey.happy.test.ts(5 hunks)apps/api/src/routes/v1_keys_updateKey.ts(3 hunks)apps/api/src/routes/v1_keys_verifyKey.multilimit.test.ts(1 hunks)apps/api/src/routes/v1_keys_verifyKey.test.ts(4 hunks)apps/api/src/routes/v1_migrations_createKey.ts(0 hunks)apps/api/src/routes/v1_migrations_enqueueKeys.happy.test_disabled.ts(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/ratelimit-setup.tsx(5 hunks)apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.schema.ts(2 hunks)apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.utils.ts(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/index.tsx(0 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/edit-ratelimits/index.tsx(0 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/hooks/use-edit-ratelimits.ts(1 hunks)apps/dashboard/components/dashboard/root-key-table/index.tsx(0 hunks)apps/dashboard/lib/trpc/routers/api/keys/api-query.ts(1 hunks)apps/dashboard/lib/trpc/routers/key/create.ts(1 hunks)apps/dashboard/lib/trpc/routers/key/updateRatelimit.ts(1 hunks)apps/docs/apis/features/ratelimiting/overview.mdx(1 hunks)internal/clickhouse/src/keys/keys.ts(4 hunks)internal/db/src/schema/identity.ts(1 hunks)tools/migrate/ratelimit-migrate.ts(1 hunks)
💤 Files with no reviewable changes (4)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/_components/components/table/components/actions/components/edit-ratelimits/index.tsx
- apps/dashboard/components/dashboard/root-key-table/index.tsx
- apps/api/src/routes/v1_migrations_createKey.ts
- apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/index.tsx
🧰 Additional context used
🧠 Learnings (1)
apps/dashboard/lib/trpc/routers/api/keys/api-query.ts (1)
Learnt from: chronark
PR: unkeyed/unkey#2294
File: apps/api/src/pkg/keys/service.ts:268-271
Timestamp: 2024-10-20T07:05:55.471Z
Learning: In `apps/api/src/pkg/keys/service.ts`, `ratelimitAsync` is a table relation, not a column selection. When querying, ensure that table relations are included appropriately, not as columns.
🧬 Code Graph Analysis (8)
apps/api/src/pkg/key_migration/handler.ts (2)
internal/db/src/index.ts (1)
schema(8-8)internal/id/src/generate.ts (1)
newId(33-52)
apps/dashboard/lib/trpc/routers/key/create.ts (2)
apps/api/src/pkg/keys/service.ts (1)
ratelimit(675-725)apps/dashboard/lib/trpc/trpc.ts (1)
ratelimit(84-111)
apps/api/src/routes/v1_keys_createKey.ts (2)
apps/dashboard/lib/db.ts (1)
db(5-26)internal/id/src/generate.ts (1)
newId(33-52)
apps/api/src/routes/v1_keys_verifyKey.multilimit.test.ts (1)
internal/id/src/generate.ts (1)
newId(33-52)
tools/migrate/ratelimit-migrate.ts (3)
apps/dashboard/lib/db.ts (1)
db(5-26)internal/db/src/index.ts (1)
schema(8-8)internal/id/src/generate.ts (1)
newId(33-52)
apps/api/src/routes/v1_keys_verifyKey.test.ts (2)
internal/id/src/generate.ts (1)
newId(33-52)internal/db/src/index.ts (1)
schema(8-8)
apps/dashboard/lib/trpc/routers/key/updateRatelimit.ts (1)
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.schema.ts (1)
ratelimitSchema(254-266)
apps/api/src/pkg/cache/namespaces.ts (1)
internal/db/src/types.ts (5)
Key(4-4)EncryptedKey(34-34)Ratelimit(43-43)Api(7-7)KeyAuth(13-13)
🪛 LanguageTool
apps/docs/apis/features/ratelimiting/overview.mdx
[uncategorized] ~172-~172: Possible missing comma found.
Context: ...This approach works well for user-based quotas where multiple API keys should share th...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./apps/dashboard
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
apps/api/src/pkg/cache/index.ts (1)
32-40: Confirm cache-buster alignment across all consumersChanging the Cloudflare
cacheBusterfrom"v8"to"v9"is fine, but make sure every place that builds or purges the Cloudflare namespace (CI scripts, cron jobs, dashboards, etc.) is bumped as well; otherwise we’ll end up with two parallel caches that never invalidate each other.
No code change required if this was already coordinated.apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.utils.ts (1)
121-131: Good default – auto-apply enabled
SettingautoApply: truein the default ratelimit payload matches the existing single-ratelimit behaviour and avoids surprise “no ratelimit applied” cases. 👍apps/api/src/routes/v1_migrations_enqueueKeys.happy.test_disabled.ts (1)
548-556: Tests updated correctlyThe assertions now reflect the multi-ratelimit model and guard both
limitandduration. Nice coverage! 🚀apps/api/src/pkg/key_migration/handler.ts (1)
88-97: PersistautoApply/type, and guard against duplicate “default” rowsThe migration only copies
limit&duration.
autoApplydefaults tofalse; migrated keys will suddenly stop being rate-limited unless explicitly re-enabled.typeis lost – important if the key relied on “consistent” limiting.- A prior migration run could already have inserted the “default” ratelimit; the unique
(name,keyId)index will now throw.if (message.ratelimit) { + const exists = await tx.query.ratelimits.findFirst({ + where: (t, { and, eq }) => and(eq(t.keyId, keyId), eq(t.name, 'default')), + }); + if (!exists) { await tx.insert(schema.ratelimits).values({ id: newId("ratelimit"), workspaceId: message.workspaceId, keyId: keyId, limit: message.ratelimit.limit, duration: message.ratelimit.duration, name: "default", + autoApply: true, + type: message.ratelimit.type ?? 'fast', }); } }[ suggest_essential_refactor ]
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/ratelimit-setup.tsx (1)
31-40: Default rule shipped asautoApply: falseconflicts with API behaviour
useEffectseeds the first rule withautoApply: false, yet the backend forces the inserted rule toautoApply: true.
End-users will see a switch that is off although the rule will always be applied, leading to confusing UX and potential support tickets.
Recommend defaulting totruehere or reflecting the server value after submission.apps/api/src/routes/v1_keys_updateKey.happy.test.ts (2)
92-104: Tests look goodAssertions correctly validate the presence and shape of the new
ratelimitsrelation.
832-845: Appreciate additional assertion on empty ratelimitsEnsuring the relation is empty when not updated covers an important regression path.
apps/api/src/routes/v1_keys_verifyKey.multilimit.test.ts (1)
708-729: Great coverage for default auto-apply pathThe test explicitly inserts a
defaultrule withautoApply: trueand verifies fallback behaviour – solid addition.internal/db/src/schema/identity.ts (1)
80-85: Uniqueness with NULLs is ineffective
UNIQUE (name, keyId)andUNIQUE (name, identityId)permit multiple rows whenkeyId/identityIdisNULL(MySQL treats every NULL as distinct).
If you introduce a row with both foreign columns NULL, duplicates bypass both indexes.Depending on business rules you may want:
- an additional
UNIQUE (name) WHERE key_id IS NULL AND identity_id IS NULL, or- the CHECK constraint above to forbid the “both NULL” case.
apps/dashboard/lib/trpc/routers/key/updateRatelimit.ts (1)
65-74: Timestamp field mismatch (createdAtvscreatedAtM)Other tables use the
…AtMconvention (createdAtM,updatedAtM).
Storing mixed formats complicates queries & migrations. Confirm the ratelimits table indeed expectscreatedAtin ms; otherwise align the field name.internal/clickhouse/src/keys/keys.ts (2)
234-234: Removed trailing whitespace afterWITH—purely cosmetic.
This formatting tweak has no impact on query execution and improves code cleanliness.
256-256: Trimmed whitespace beforeSELECT—formatting only.
This change is purely cosmetic and maintains the same SQL semantics.
| ratelimits: true, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Loading ratelimits eagerly is OK but might be overkill
For a delete operation we never touch ratelimit rows. Fetching them adds an extra join with no functional gain and bloats the cache entry.
Optional: drop ratelimits: true from the with clause.
🤖 Prompt for AI Agents
In apps/api/src/routes/v1_keys_deleteKey.ts at lines 84 to 85, the code eagerly
loads ratelimits with the delete operation, which is unnecessary since ratelimit
rows are not used here. To fix this, remove the `ratelimits: true` entry from
the `with` clause to avoid the extra join and reduce cache bloat.
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.schema.ts
Show resolved
Hide resolved
| # Create a key with manual ratelimit | ||
| curl -X POST https://api.unkey.dev/v1/keys.createKey \ | ||
| -H "Authorization: Bearer <UNKEY_ROOT_KEY>" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{ | ||
| "apiId": "api_123", | ||
| "ratelimits": [ | ||
| { | ||
| "name": "expensive-operations" | ||
| } | ||
| ] | ||
| }' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Example omits mandatory limit / duration fields
ratelimits.createKey payload shows only "name" for the manual ratelimit, but the backend requires limit and duration. Readers copying this snippet will hit validation errors. Add realistic limit/duration values (or explain the defaults if they truly are optional).
🤖 Prompt for AI Agents
In apps/docs/apis/features/ratelimiting/overview.mdx around lines 50 to 61, the
example curl command for creating a key with manual ratelimit omits the required
"limit" and "duration" fields in the "ratelimits" object. To fix this, add
realistic "limit" and "duration" values to the ratelimit entry in the JSON
payload to match backend validation requirements or clarify if these fields have
defaults and are optional.
There was a problem hiding this comment.
good catch, @coderabbitai please create an issue for this
There was a problem hiding this comment.
🧩 Analysis chain
@chronark I'll create an issue for this documentation problem right away.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the missing mandatory fields in the documentation example
gh issue create \
--title "Documentation: Manual ratelimit example missing required limit and duration fields" \
--body "The documentation example for creating a key with manual ratelimit in \`apps/docs/apis/features/ratelimiting/overview.mdx\` (around lines 50-61) shows only the \`name\` field but omits the mandatory \`limit\` and \`duration\` fields that are required by backend validation.
This will cause validation errors for users who copy the example directly.
**Location:** \`apps/docs/apis/features/ratelimiting/overview.mdx\` lines 50-61
**Related PR:** https://github.com/unkeyed/unkey/pull/3328
**Comment:** https://github.com/unkeyed/unkey/pull/3328#discussion_r2152667944
**Reported by:** @chronark" \
--assignee chronark
Length of output: 772
@chronark Issue created! 🎯
GitHub Issue: #3387
The issue has been assigned to you and includes all the relevant details about the missing limit and duration fields in the manual ratelimit documentation example, along with backlinks to this PR and comment.
There was a problem hiding this comment.
🧩 Analysis chain
@chronark I'll create an issue for the missing required fields in the documentation example.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the documentation problem
gh issue create \
--title "Documentation: Manual ratelimit example missing required fields" \
--body "The example curl command for creating a key with manual ratelimit in the rate limiting overview documentation is missing required fields.
**Problem:**
In \`apps/docs/apis/features/ratelimiting/overview.mdx\` around lines 50-61, the example shows:
\`\`\`json
\"ratelimits\": [
{
\"name\": \"expensive-operations\"
}
]
\`\`\`
However, the backend validation requires \`limit\` and \`duration\` fields for ratelimit objects. Readers copying this example will encounter validation errors.
**Context:**
This issue was identified during review of PR #3328 which migrated from single to multi rate limits.
**References:**
- PR: https://github.com/unkeyed/unkey/pull/3328
- Comment: https://github.com/unkeyed/unkey/pull/3328#discussion_r2152667944
- Requested by: @chronark" \
--assignee chronark
Length of output: 926
Perfect! I've successfully created issue #3388 for the documentation problem: #3388
The issue documents the missing limit and duration fields in the manual ratelimit example and has been assigned to you. It includes all the relevant context and backlinks to this PR and comment.
🐰
( )
/ \
| ◕ ◕|
\ ‿ /
------
Mission accomplished!
|
@perkinsjr hold on, I found some bugs when updating keys |
|
Sounds good! |
|
the autoApply setting doesn’t get saved I think, and some issues with the form validation |
|
@perkinsjr fixed, just wait for the redeploy |
|
@chronark is it still not passing the correct value? Last time I check it was working |
|
it was for the creation |
|
Ahh gotcha lemme know if you guys need anything |
|
@chronark is the API deployed to preview? I was trying to test without pulling the PR into local. If it is... This doesn't work. I did the following:
The Key itself in the UI at least has the two R/Ls and has one set to auto apply. |
|
probably another PR got deployedto preview in the meantime |
|
I can check again tomorrow |
|
No worries, I will run locally today / tomorrow and go through it there also |
|
Locally this looks good but a large majority of the CIs are failing. You might want to look at coderabbit to see if you want to make any other changes. Will approve once all that is resolved. |
3 participants
What does this PR do?
We're no longer using the rate limits defined in the keys table, all of them have been migrated to the
ratelimitstable, allowing users to specify multiple rate limits per key (just like they already can for identities)All rate limits have been migrated and the DB is updated.
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
autoApplyflag and adjust unique constraints on rate limits.Documentation
Tests