fix: List keys return enabled [merge after 3328]#3385
Conversation
chronark
requested review from
MichaelUnkey,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
📝 WalkthroughWalkthroughThe changes add the enabled property to the response of the /v1/apis.listKeys API endpoint and introduce a corresponding test to verify that the enabled status is correctly returned for each key. No other logic or tests were modified. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Server
participant Database
Client->>API_Server: GET /v1/apis.listKeys
API_Server->>Database: Query keys (including enabled status)
Database-->>API_Server: Return keys with enabled field
API_Server-->>Client: Respond with keys including enabled property
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (22)
🔇 Additional comments (2)
✨ 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! 🙏 |
|
ok my local git is in a bad state, we should merge #3328 first |
chronark
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
apps/dashboard/lib/trpc/routers/api/keys/api-query.ts (1)
208-226: Replace rawidentities.external_idstring with typed column referenceHard-coding the column name breaks Drizzle’s type-safety and silently fails if the schema ever changes.
-case "is": - condition = sql`identities.external_id = ${value}`; +case "is": + condition = sql`${identities.externalId} = ${value}`;Do the same for all four LIKE branches below.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
apps/api/src/routes/v1_apis_listKeys.happy.test.ts(1 hunks)apps/api/src/routes/v1_apis_listKeys.ts(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/ratelimit-setup.tsx(2 hunks)apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.schema.ts(2 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/updateRatelimit.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/log-details/index.tsx
- 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
🧰 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 (1)
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)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/api/src/routes/v1_apis_listKeys.ts (1)
339-340: Double-check the OpenAPI schema now includesenabledThe handler appends
enabled: k.enabled, butkeySchema(imported above) must expose this property; otherwise type-safety guarantees and generated docs drift.Please verify
apps/api/src/routes/schema.ts(or equivalent) was updated accordingly.
If not, extend the schema:enabled: z.boolean(),apps/dashboard/lib/trpc/routers/key/updateRatelimit.ts (1)
100-110:autoApplyflag is parsed but never persistedThe rule insert/update statements ignore the new
autoApplyproperty introduced in the
ratelimit schema.
If the column does not exist this is fine, but if the flag is meant to influence runtime behaviour it will be lost here, leading to confusing UX.Please confirm whether
autoApplyshould be stored (and add a column + mapping) or remove it from the schema to avoid a false promise.apps/dashboard/lib/trpc/routers/api/keys/api-query.ts (1)
13-25:enabledcolumn wired through correctlyThe field is added to
DatabaseKey,KeyDetails, and the query column list—looks consistent.Also applies to: 37-61, 260-270
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/ratelimit-setup.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/ratelimit-setup.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/create-key.schema.ts
Show resolved
Hide resolved
...hId]/_components/components/table/components/actions/components/hooks/use-edit-ratelimits.ts
Show resolved
Hide resolved
1 participant
What does this PR do?
Looks like we never returned the value of
enabledissue for this PR, please re-review our Contributing Guide and create an issue -->
Type of change
How should this be tested?
call the /v1/apis.listKeys endpoint and check for the
enabledfieldChecklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Tests