perf: use seperate query to look at identity and key ratelimits when l…#3895
perf: use seperate query to look at identity and key ratelimits when l…#3895
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRemoved WorkspaceID from ListLiveKeysByKeyAuthID call site and from generated DB params; the SQL no longer filters by workspace_id. The query’s ratelimits projection was reworked into a UNION-derived table aggregated to JSON and additional identity/encryption fields were exposed in the SELECT. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as v2 List Keys Handler
participant DB as DB: ListLiveKeysByKeyAuthID
Client->>API: GET /v2/apis/.../keys?identity&cursor&limit
API->>DB: ListLiveKeysByKeyAuthID(KeyAuthID, IDCursor, Identity, Limit)
note right of DB #e6f4ea: SQL filters by key_auth_id and identity/id cursor\nNo workspace_id filter
DB-->>API: Rows (including combined ratelimits JSON + extra identity/encryption fields)
API-->>Client: JSON response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Graphite Automations"Notify author when CI fails" took an action on this PR • (08/29/25)1 teammate was notified to this PR based on Andreas Thomas's automation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
go/apps/api/routes/v2_apis_list_keys/handler.go (2)
186-192: Prevent duplicate first record on paginated fetch.Given the current SQL uses
>=, the first row of the next page duplicates the last row of the previous page. Add a defensive drop of the duplicate (and keep limit+1 for hasMore) so behavior is correct even before the SQL fix lands.- // Handle pagination - hasMore := len(keyResults) > limit + // Handle pagination (drop duplicate if cursor was inclusive) + if cursor != "" && len(keyResults) > 0 && keyResults[0].ID == cursor { + keyResults = keyResults[1:] + } + hasMore := len(keyResults) > limit var nextCursor *string if hasMore { - nextCursor = ptr.P(keyResults[len(keyResults)-1].ID) - keyResults = keyResults[:limit] + nextCursor = ptr.P(keyResults[len(keyResults)-1].ID) + keyResults = keyResults[:limit] }After switching SQL to
>, this guard remains harmless.
340-372: Nil guard when attaching identity ratelimits.
response.Identitycan be nil; settingresponse.Identity.Ratelimitswill panic. Guard it.- if len(identityRatelimits) > 0 { - response.Identity.Ratelimits = &identityRatelimits - } + if len(identityRatelimits) > 0 { + if response.Identity == nil { + response.Identity = &openapi.Identity{} + } + response.Identity.Ratelimits = &identityRatelimits + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
go/apps/api/routes/v2_apis_list_keys/handler.go(1 hunks)go/pkg/db/key_list_live_by_auth_id.sql_generated.go(2 hunks)go/pkg/db/querier_generated.go(1 hunks)go/pkg/db/queries/key_list_live_by_auth_id.sql(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow comprehensive documentation guidelines for Go code as described in go/GO_DOCUMENTATION_GUIDELINES.md
Every public function/type in Go code must be documented
Prefer interfaces for testability in Go code
Use AIDEV-* comments for complex/important code in Go services
Files:
go/apps/api/routes/v2_apis_list_keys/handler.gogo/pkg/db/querier_generated.gogo/pkg/db/key_list_live_by_auth_id.sql_generated.go
**/*.{env,js,ts,go}
📄 CodeRabbit inference engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/apps/api/routes/v2_apis_list_keys/handler.gogo/pkg/db/querier_generated.gogo/pkg/db/key_list_live_by_auth_id.sql_generated.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2872
File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39
Timestamp: 2025-04-08T09:34:24.576Z
Learning: In the Unkey dashboard, when making database queries involving workspaces, use `ctx.workspace.id` directly instead of fetching the workspace separately for better performance and security.
📚 Learning: 2025-04-08T09:34:24.576Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2872
File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39
Timestamp: 2025-04-08T09:34:24.576Z
Learning: In the Unkey dashboard, when making database queries involving workspaces, use `ctx.workspace.id` directly instead of fetching the workspace separately for better performance and security.
Applied to files:
go/pkg/db/key_list_live_by_auth_id.sql_generated.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / Test
🔇 Additional comments (7)
go/pkg/db/queries/key_list_live_by_auth_id.sql (1)
78-79: Fix pagination duplication: use exclusive cursor.
k.id >= sqlc.arg(id_cursor)repeats the last record on the next page. Switch to>to make the cursor exclusive.- AND k.id >= sqlc.arg(id_cursor) + AND k.id > sqlc.arg(id_cursor)⛔ Skipped due to learnings
Learnt from: Flo4604 PR: unkeyed/unkey#3677 File: go/pkg/db/queries/ratelimit_override_list_by_namespace_id.sql:7-8 Timestamp: 2025-07-30T10:46:56.453Z Learning: In the Unkey codebase, cursor pagination uses a "limit + 1" approach where queries fetch one extra record beyond the requested limit. The cursor is set to the ID of this extra record (which is not returned to the client), so using `>=` in the WHERE clause is correct because the next page should start from that cursor ID.Learnt from: ogzhanolguncu PR: unkeyed/unkey#2896 File: internal/clickhouse/src/ratelimits.ts:468-592 Timestamp: 2025-02-21T11:15:16.185Z Learning: The cursor logic in getRatelimitOverviewLogs query uses (time, request_id) < (cursorTime, cursorRequestId) comparison which works well with descending order but may need adjustment for ascending sorts based on real usage patterns.go/pkg/db/key_list_live_by_auth_id.sql_generated.go (3)
101-106: Param removal LGTM.Dropping
WorkspaceIDfromListLiveKeysByKeyAuthIDParamsmatches the SQL and reduces bind overhead.
90-96: Make cursor exclusive in generated SQL.Reflect the
>change in the generated query and regenerate.- AND k.id >= ? + AND k.id > ?Re-run
sqlc generate.⛔ Skipped due to learnings
Learnt from: Flo4604 PR: unkeyed/unkey#3677 File: go/pkg/db/queries/ratelimit_override_list_by_namespace_id.sql:7-8 Timestamp: 2025-07-30T10:46:56.453Z Learning: In the Unkey codebase, cursor pagination uses a "limit + 1" approach where queries fetch one extra record beyond the requested limit. The cursor is set to the ID of this extra record (which is not returned to the client), so using `>=` in the WHERE clause is correct because the next page should start from that cursor ID.
231-238: Bind order confirmed correct; no changes required.go/pkg/db/querier_generated.go (1)
1365-1450: Doc block matches current (but flawed) SQL; will need regen after ratelimits/cursor fixes.No functional changes here; just ensure comments are updated post-SQL adjustments for future readers.
After applying the SQL diffs, re-run
sqlc generateand verify this doc shows a flat ratelimits array andk.id > ?.go/apps/api/routes/v2_apis_list_keys/handler.go (2)
172-176: Param update LGTM.Using only KeyAuthID, IDCursor, Identity, and Limit aligns with the new query.
167-176: Workspace scoping is safe without an extra filter. key_auth_id is a globally unique PK, and we load the API and verify api.WorkspaceID == auth.AuthorizedWorkspaceID before calling ListLiveKeysByKeyAuthID, so all returned keys belong to that workspace.
|
Thank you for following the naming conventions for pull request titles! 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
go/pkg/db/queries/key_list_live_by_auth_id.sql (1)
1-95: Indexing checklist to lock in the perf win.Ensure these indexes exist to guarantee the 40 ms target under load:
- keys(key_auth_id, id) covering the seek ORDER BY/LIMIT.
- ratelimits(key_id), ratelimits(identity_id) for both UNION branches.
- keys_roles(key_id), roles_permissions(role_id), keys_permissions(key_id), permissions(slug), roles(name).
If any are missing, add them in a forward-compatible migration.
go/pkg/db/key_list_live_by_auth_id.sql_generated.go (1)
145-149: Prefer json.RawMessage over interface{} for JSON columns.Map JSON columns (roles, permissions, role_permissions, ratelimits) to json.RawMessage via sqlc.yaml overrides for safer typing and fewer type assertions. Regenerate after config change.
Example sqlc.yaml snippet:
overrides: - db_type: "json" go_type: type: "encoding/json.RawMessage"
♻️ Duplicate comments (4)
go/pkg/db/queries/key_list_live_by_auth_id.sql (3)
10-22: Stabilize JSON ordering inside roles aggregation.Move ORDER BY inside JSON_ARRAYAGG to make ordering deterministic; remove the subquery ORDER BY.
- (SELECT JSON_ARRAYAGG( - JSON_OBJECT( - 'id', r.id, - 'name', r.name, - 'description', r.description - ) - ) - FROM keys_roles kr - JOIN roles r ON r.id = kr.role_id - WHERE kr.key_id = k.id - ORDER BY r.name), + (SELECT JSON_ARRAYAGG( + JSON_OBJECT( + 'id', r.id, + 'name', r.name, + 'description', r.description + ) ORDER BY r.name + ) + FROM keys_roles kr + JOIN roles r ON r.id = kr.role_id + WHERE kr.key_id = k.id),
24-38: Stabilize JSON ordering inside direct permissions aggregation.Same pattern; order inside the aggregator.
- (SELECT JSON_ARRAYAGG( - JSON_OBJECT( - 'id', p.id, - 'name', p.name, - 'slug', p.slug, - 'description', p.description - ) - ) - FROM keys_permissions kp - JOIN permissions p ON kp.permission_id = p.id - WHERE kp.key_id = k.id - ORDER BY p.slug), + (SELECT JSON_ARRAYAGG( + JSON_OBJECT( + 'id', p.id, + 'name', p.name, + 'slug', p.slug, + 'description', p.description + ) ORDER BY p.slug + ) + FROM keys_permissions kp + JOIN permissions p ON kp.permission_id = p.id + WHERE kp.key_id = k.id),
40-55: Stabilize JSON ordering inside role-derived permissions aggregation.Ditto for role_permissions.
- (SELECT JSON_ARRAYAGG( - JSON_OBJECT( - 'id', p.id, - 'name', p.name, - 'slug', p.slug, - 'description', p.description - ) - ) - FROM keys_roles kr - JOIN roles_permissions rp ON kr.role_id = rp.role_id - JOIN permissions p ON rp.permission_id = p.id - WHERE kr.key_id = k.id - ORDER BY p.slug), + (SELECT JSON_ARRAYAGG( + JSON_OBJECT( + 'id', p.id, + 'name', p.name, + 'slug', p.slug, + 'description', p.description + ) ORDER BY p.slug + ) + FROM keys_roles kr + JOIN roles_permissions rp ON kr.role_id = rp.role_id + JOIN permissions p ON rp.permission_id = p.id + WHERE kr.key_id = k.id),go/pkg/db/key_list_live_by_auth_id.sql_generated.go (1)
21-67: Propagate JSON ordering fix from source SQL and regenerate.Roles/permissions aggregations still order outside JSON_ARRAYAGG. Update the source SQL as suggested and re-run sqlc so this constant matches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
go/pkg/db/key_list_live_by_auth_id.sql_generated.go(2 hunks)go/pkg/db/querier_generated.go(1 hunks)go/pkg/db/queries/key_list_live_by_auth_id.sql(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow comprehensive documentation guidelines for Go code as described in go/GO_DOCUMENTATION_GUIDELINES.md
Every public function/type in Go code must be documented
Prefer interfaces for testability in Go code
Use AIDEV-* comments for complex/important code in Go services
Files:
go/pkg/db/querier_generated.gogo/pkg/db/key_list_live_by_auth_id.sql_generated.go
**/*.{env,js,ts,go}
📄 CodeRabbit inference engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/pkg/db/querier_generated.gogo/pkg/db/key_list_live_by_auth_id.sql_generated.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2872
File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39
Timestamp: 2025-04-08T09:34:24.576Z
Learning: In the Unkey dashboard, when making database queries involving workspaces, use `ctx.workspace.id` directly instead of fetching the workspace separately for better performance and security.
📚 Learning: 2024-10-20T07:05:55.471Z
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.
Applied to files:
go/pkg/db/queries/key_list_live_by_auth_id.sqlgo/pkg/db/key_list_live_by_auth_id.sql_generated.go
📚 Learning: 2025-04-08T09:34:24.576Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2872
File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39
Timestamp: 2025-04-08T09:34:24.576Z
Learning: In the Unkey dashboard, when making database queries involving workspaces, use `ctx.workspace.id` directly instead of fetching the workspace separately for better performance and security.
Applied to files:
go/pkg/db/key_list_live_by_auth_id.sql_generated.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
🔇 Additional comments (5)
go/pkg/db/queries/key_list_live_by_auth_id.sql (2)
86-86: Leave the inclusive cursor check (>=)—it’s correct under our “limit + 1” pagination.
We fetch one extra row (LIMIT … + 1) and set the cursor to that row’s ID (which isn’t returned), so the next page must include>= id_cursorto avoid skipping items.Likely an incorrect or invalid review comment.
81-85: Multi-tenant guard: confirm key_auth_id scoped to caller’s workspace
Listing now filters solely onkey_auth_id—double-check the handler only ever passes a key_auth_id tied toctx.workspace.id(e.g. via an API lookup filtered byworkspace_id). Optionally enforce at SQL with a cheap consistency check:WHERE k.key_auth_id = sqlc.arg(key_auth_id) + AND ka.workspace_id = k.workspace_idgo/pkg/db/querier_generated.go (1)
1365-1455: Generated doc block reflects removed workspace filter; keep it in sync after SQL tweaks.If you adopt the JSON ordering and pagination changes suggested in the SQL, re-run sqlc to update this block and the compiled query string. Do not hand-edit.
go/pkg/db/key_list_live_by_auth_id.sql_generated.go (2)
98-101: Cursor condition mirrors source; switch to strict ‘>’ if you change SQL.After adjusting the SQL to k.id > ?, regenerate so this stays consistent.
247-254: Verify arg binding order matches SQL after any edits.If you touch the WHERE predicates, ensure the placeholders and arg order remain: KeyAuthID, IDCursor, Identity, Identity, Identity, Limit. Regenerate to avoid runtime mismatches.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go/pkg/db/key_list_live_by_auth_id.sql_generated.go (1)
146-149: Avoid interface{} for JSON columns; use json.RawMessage for zero-copy.Map aggregated JSON fields (roles, permissions, role_permissions, ratelimits) to json.RawMessage via sqlc.yaml override or CAST(... AS JSON) if needed.
Example sqlc.yaml snippet:
overrides: - db_type: "json" go_type: "encoding/json.RawMessage"Regenerate to update field types.
♻️ Duplicate comments (2)
go/pkg/db/querier_generated.go (1)
1326-1348: Add ORDER BY to ratelimits aggregation for stable output.Specify ORDER BY id inside JSON_ARRAYAGG over combined_rl.
go/pkg/db/key_list_live_by_auth_id.sql_generated.go (1)
69-91: Ratelimits JSON: add ORDER BY for deterministic array.Order by id within JSON_ARRAYAGG over combined_rl.
- (SELECT JSON_ARRAYAGG( - JSON_OBJECT( + (SELECT JSON_ARRAYAGG( + JSON_OBJECT( 'id', id, 'name', name, 'key_id', key_id, 'identity_id', identity_id, 'limit', `limit`, 'duration', duration, 'auto_apply', auto_apply = 1 - ) - ) + ) ORDER BY id + ) FROM (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
go/pkg/db/key_list_live_by_auth_id.sql_generated.go(2 hunks)go/pkg/db/querier_generated.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow comprehensive documentation guidelines for Go code as described in go/GO_DOCUMENTATION_GUIDELINES.md
Every public function/type in Go code must be documented
Prefer interfaces for testability in Go code
Use AIDEV-* comments for complex/important code in Go services
Files:
go/pkg/db/key_list_live_by_auth_id.sql_generated.gogo/pkg/db/querier_generated.go
**/*.{env,js,ts,go}
📄 CodeRabbit inference engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/pkg/db/key_list_live_by_auth_id.sql_generated.gogo/pkg/db/querier_generated.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2872
File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39
Timestamp: 2025-04-08T09:34:24.576Z
Learning: In the Unkey dashboard, when making database queries involving workspaces, use `ctx.workspace.id` directly instead of fetching the workspace separately for better performance and security.
📚 Learning: 2024-10-20T07:05:55.471Z
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.
Applied to files:
go/pkg/db/key_list_live_by_auth_id.sql_generated.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
go/pkg/db/querier_generated.go (1)
1271-1277: Adding identity/encryption fields looks correct.New columns align with downstream row struct and improve API surface.
chronark
left a comment
chronark
left a comment
There was a problem hiding this comment.
looks fine to me, but please address/resolve rabbits comments
|
resolved. |
2 participants
fix performance when listing keys for an api
to 40ish ms
What does this PR do?
Fixes #3893
Type of change
How should this be tested?
Run the query against a develop db and look at the speed / use explain to see that we are using indexes propery with high filter values.
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Refactor