Merged
Conversation
Flo4604
requested review from
MichaelUnkey,
chronark,
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
|
Warning Rate limit exceeded@chronark has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRefactors identity lookups from a multi-row query to a single-row query and propagates that change across handlers and generated DB code; updates key-listing query to use identity_id (sql.NullString) and STRAIGHT_JOIN; adjusts tests and a tooling parseInt call. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant DB
participant KeysQuery as Key Listing
rect rgb(240,255,240)
note right of Handler: Identity resolution + key listing (new)
Client->>Handler: List keys (may include ExternalId)
alt ExternalId provided
Handler->>DB: FindIdentityByExternalID / FindIdentityByID
DB-->>Handler: IdentityRow (or NotFound / error)
Handler->>KeysQuery: ListLiveKeysByKeySpaceID(IdentityID: sql.NullString)
else No ExternalId
Handler->>KeysQuery: ListLiveKeysByKeySpaceID(IdentityID: NULL)
end
KeysQuery->>DB: Execute key-list query with (? IS NULL OR k.identity_id = ?)
DB-->>Handler: Keys result set
Handler-->>Client: 200 OK + keys
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
Contributor
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/artillery/create-keys.ts (2)
91-91: Remove unnecessary template literal.The template literal on this line contains no interpolation or special characters and should be a regular string literal.
Apply this diff to fix the linter warning:
- console.error(`Error response body:`, errorBody); + console.error("Error response body:", errorBody);
43-48: Remove unused interface.The
CreateKeyErrorinterface is defined but never used in the codebase. Error handling is currently done vianullreturns and console logging.Apply this diff to remove the unused interface:
-interface CreateKeyError { - index: number; - error: string; - statusCode: number; - response?: any; -} -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
go/apps/api/routes/v2_apis_list_keys/handler.go(2 hunks)go/apps/api/routes/v2_identities_delete_identity/handler.go(1 hunks)go/apps/api/routes/v2_identities_get_identity/handler.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/404_test.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/handler.go(1 hunks)go/pkg/db/identity_find.sql_generated.go(1 hunks)go/pkg/db/identity_find_with_ratelimits.sql_generated.go(0 hunks)go/pkg/db/key_list_live_by_key_space_id.sql_generated.go(4 hunks)go/pkg/db/querier_generated.go(3 hunks)go/pkg/db/queries/identity_find.sql(1 hunks)go/pkg/db/queries/identity_find_with_ratelimits.sql(0 hunks)go/pkg/db/queries/key_list_live_by_key_space_id.sql(1 hunks)tools/artillery/create-keys.ts(1 hunks)
💤 Files with no reviewable changes (2)
- go/pkg/db/identity_find_with_ratelimits.sql_generated.go
- go/pkg/db/queries/identity_find_with_ratelimits.sql
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/job_detect_changes.yaml:111-114
Timestamp: 2025-08-08T16:10:00.224Z
Learning: Repo: unkeyed/unkey — When imeyer writes “make me an issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/runbook-freshness-check.yaml:157-173
Timestamp: 2025-08-08T14:59:52.283Z
Learning: Repo unkeyed/unkey: When a CI/workflow fix is deferred, imeyer prefers a thorough GitHub issue be opened with sections (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References) and assigned to imeyer, including backlinks to the originating PR and comment.
📚 Learning: 2025-06-02T11:09:58.791Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3292
File: apps/dashboard/lib/trpc/routers/key/create.ts:11-14
Timestamp: 2025-06-02T11:09:58.791Z
Learning: In the unkey codebase, TypeScript and the env() function implementation already provide sufficient validation for environment variables, so additional runtime error handling for missing env vars is not needed.
Applied to files:
tools/artillery/create-keys.ts
📚 Learning: 2025-09-01T08:29:10.199Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3895
File: go/pkg/db/key_list_live_by_auth_id.sql_generated.go:98-105
Timestamp: 2025-09-01T08:29:10.199Z
Learning: In Unkey's ListLiveKeysByKeyAuthID query, adding ka.workspace_id = k.workspace_id constraint negatively impacts index performance. Workspace validation is handled upstream at the API level before the query is called, making the additional constraint unnecessary.
Applied to files:
go/pkg/db/queries/key_list_live_by_key_space_id.sqlgo/pkg/db/key_list_live_by_key_space_id.sql_generated.gogo/pkg/db/querier_generated.go
📚 Learning: 2025-04-08T09:34:24.576Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 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/queries/key_list_live_by_key_space_id.sqlgo/pkg/db/querier_generated.go
📚 Learning: 2025-10-30T15:10:52.743Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Applied to files:
go/pkg/db/queries/key_list_live_by_key_space_id.sqlgo/pkg/db/key_list_live_by_key_space_id.sql_generated.gogo/pkg/db/querier_generated.gogo/pkg/db/queries/identity_find.sql
📚 Learning: 2024-10-20T07:05:55.471Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 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_key_space_id.sqlgo/pkg/db/key_list_live_by_key_space_id.sql_generated.gogo/pkg/db/querier_generated.go
📚 Learning: 2024-11-29T15:15:47.308Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In `apps/api/src/routes/v1_keys_updateKey.ts`, the code intentionally handles `externalId` and `ownerId` separately for clarity. The `ownerId` field will be removed in the future, simplifying the code.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-04-18T20:01:33.812Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3151
File: go/apps/api/openapi/gen.go:221-233
Timestamp: 2025-04-18T20:01:33.812Z
Learning: For identity deletion operations in the Unkey API, identityId takes precedence over externalId when both are provided in the request body.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
🧬 Code graph analysis (5)
go/pkg/db/identity_find.sql_generated.go (1)
go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)
go/apps/api/routes/v2_identities_get_identity/handler.go (3)
go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(46-50)go/pkg/db/models_generated.go (1)
Identity(643-652)go/pkg/codes/unkey_data.go (1)
Data(112-165)
go/apps/api/routes/v2_identities_delete_identity/handler.go (3)
go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(46-50)go/apps/api/openapi/gen.go (1)
Identity(161-173)go/pkg/codes/unkey_data.go (1)
Data(112-165)
go/pkg/db/querier_generated.go (3)
go/pkg/partition/db/database.go (1)
DBTX(10-10)go/pkg/db/identity_find.sql_generated.go (2)
FindIdentityParams(46-50)FindIdentityRow(52-62)go/pkg/db/models_generated.go (1)
Identity(643-652)
go/apps/api/routes/v2_identities_update_identity/handler.go (3)
go/pkg/db/identity_find.sql_generated.go (2)
FindIdentityParams(46-50)FindIdentityRow(52-62)go/pkg/codes/unkey_data.go (1)
Data(112-165)go/pkg/db/custom_types.go (1)
RatelimitInfo(25-33)
🪛 GitHub Actions: autofix.ci
tools/artillery/create-keys.ts
[error] 91-91: lint/style/noUnusedTemplateLiteral: Do not use template literals if interpolation and special-character handling are not needed.
[error] 43-43: lint/correctness/noUnusedVariables: This interface is unused.
⏰ 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 (2)
tools/artillery/create-keys.ts (1)
155-156: LGTM: Explicit radix parameter improves parsing safety.Adding the radix parameter
10toNumber.parseInt()is a best practice that ensures consistent base-10 parsing and avoids potential ambiguity with octal or hexadecimal notation.go/pkg/db/querier_generated.go (1)
1578-1581: Restore identity-id filtering in key list query.The new predicate
? = '' OR (i.workspace_id = k.workspace_id AND i.external_id = ?)drops thek.identity_id = ?branch that callers relied on. Requests that supply an identity ID now hit the first branch (non-empty string) and then fail the external-id comparison, so the endpoint returns zero rows even though matching keys exist. Please reintroduce the identity-id lookup—ideally via the planned UNION split betweenk.identity_idandi.external_id—so both identifiers continue working.⛔ Skipped due to learnings
Learnt from: Flo4604 Repo: unkeyed/unkey PR: 3895 File: go/pkg/db/key_list_live_by_auth_id.sql_generated.go:98-105 Timestamp: 2025-09-01T08:29:10.199Z Learning: In Unkey's ListLiveKeysByKeyAuthID query, adding ka.workspace_id = k.workspace_id constraint negatively impacts index performance. Workspace validation is handled upstream at the API level before the query is called, making the additional constraint unnecessary.Learnt from: Flo4604 Repo: unkeyed/unkey PR: 4190 File: go/internal/services/keys/verifier.go:51-53 Timestamp: 2025-10-30T15:10:52.743Z Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.Learnt from: Flo4604 Repo: unkeyed/unkey PR: 4292 File: go/apps/api/openapi/openapi-generated.yaml:386-387 Timestamp: 2025-11-10T11:50:46.258Z Learning: Repo unkeyed/unkey: It’s acceptable for pagination properties (e.g., V2IdentitiesListIdentitiesResponseBody.pagination) to omit x-go-type-skip-optional-pointer and x-go-type-skip-optional-pointer-with-omitzero; do not flag this in future reviews.Learnt from: ogzhanolguncu Repo: unkeyed/unkey PR: 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.Learnt from: ogzhanolguncu Repo: unkeyed/unkey PR: 2872 File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39 Timestamp: 2025-04-08T09:34:24.576Z Learning: When querying or updating namespaces in the Unkey dashboard, always scope the operations to the current workspace using `eq(table.workspaceId, ctx.workspace.id)` to prevent cross-workspace access.
chronark
approved these changes
Nov 10, 2025
Member
Author
|
dont mind me want to have a look at rabbit comment first |
Flo4604
force-pushed
the
fix/list-keys-perf
branch
from
b3d47e9 to
2fe20b5
Compare
ogzhanolguncu
approved these changes
Nov 10, 2025
|
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
go/apps/api/routes/v2_apis_list_keys/200_test.go(3 hunks)go/apps/api/routes/v2_apis_list_keys/handler.go(3 hunks)go/apps/api/routes/v2_identities_delete_identity/handler.go(1 hunks)go/apps/api/routes/v2_identities_get_identity/handler.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/404_test.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/handler.go(1 hunks)go/pkg/db/identity_find.sql_generated.go(1 hunks)go/pkg/db/identity_find_with_ratelimits.sql_generated.go(0 hunks)go/pkg/db/key_list_live_by_key_space_id.sql_generated.go(4 hunks)go/pkg/db/querier_generated.go(3 hunks)go/pkg/db/queries/identity_find.sql(1 hunks)go/pkg/db/queries/identity_find_with_ratelimits.sql(0 hunks)go/pkg/db/queries/key_list_live_by_key_space_id.sql(1 hunks)tools/artillery/create-keys.ts(1 hunks)
💤 Files with no reviewable changes (2)
- go/pkg/db/identity_find_with_ratelimits.sql_generated.go
- go/pkg/db/queries/identity_find_with_ratelimits.sql
🚧 Files skipped from review as they are similar to previous changes (3)
- tools/artillery/create-keys.ts
- go/apps/api/routes/v2_identities_update_identity/404_test.go
- go/pkg/db/queries/identity_find.sql
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/job_detect_changes.yaml:111-114
Timestamp: 2025-08-08T16:10:00.224Z
Learning: Repo: unkeyed/unkey — When imeyer writes “make me an issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/runbook-freshness-check.yaml:157-173
Timestamp: 2025-08-08T14:59:52.283Z
Learning: Repo unkeyed/unkey: When a CI/workflow fix is deferred, imeyer prefers a thorough GitHub issue be opened with sections (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References) and assigned to imeyer, including backlinks to the originating PR and comment.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
📚 Learning: 2025-09-01T08:29:10.199Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3895
File: go/pkg/db/key_list_live_by_auth_id.sql_generated.go:98-105
Timestamp: 2025-09-01T08:29:10.199Z
Learning: In Unkey's ListLiveKeysByKeyAuthID query, adding ka.workspace_id = k.workspace_id constraint negatively impacts index performance. Workspace validation is handled upstream at the API level before the query is called, making the additional constraint unnecessary.
Applied to files:
go/pkg/db/key_list_live_by_key_space_id.sql_generated.gogo/pkg/db/queries/key_list_live_by_key_space_id.sqlgo/pkg/db/querier_generated.go
📚 Learning: 2025-10-30T15:10:52.743Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Applied to files:
go/pkg/db/key_list_live_by_key_space_id.sql_generated.gogo/pkg/db/queries/key_list_live_by_key_space_id.sqlgo/apps/api/routes/v2_apis_list_keys/200_test.gogo/pkg/db/querier_generated.gogo/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-04-08T09:34:24.576Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 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_key_space_id.sql_generated.gogo/pkg/db/queries/key_list_live_by_key_space_id.sql
📚 Learning: 2025-08-27T13:46:00.608Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/key_find_live_by_id.sql_generated.go:15-16
Timestamp: 2025-08-27T13:46:00.608Z
Learning: FindLiveKeyByID doesn't need to select pending_migration_id since it's only used in key management operations that don't care about migration state. The migration flow uses other queries like FindKeyForVerification.
Applied to files:
go/pkg/db/key_list_live_by_key_space_id.sql_generated.go
📚 Learning: 2025-07-17T14:24:20.403Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3631
File: go/pkg/db/bulk_keyring_insert.sql.go:23-25
Timestamp: 2025-07-17T14:24:20.403Z
Learning: In go/pkg/db/bulk_keyring_insert.sql.go and similar bulk insert generated files, hardcoded zero values for fields like size_approx and size_last_updated_at are intentional and reflect the original SQL query structure, not missing parameters.
Applied to files:
go/pkg/db/key_list_live_by_key_space_id.sql_generated.gogo/pkg/db/queries/key_list_live_by_key_space_id.sql
📚 Learning: 2024-10-20T07:05:55.471Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 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_key_space_id.sql_generated.gogo/pkg/db/queries/key_list_live_by_key_space_id.sqlgo/pkg/db/querier_generated.go
📚 Learning: 2025-08-14T16:25:48.167Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/200_test.go
📚 Learning: 2025-10-21T09:45:47.560Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 4107
File: go/pkg/clickhouse/key_verifications_test.go:20-20
Timestamp: 2025-10-21T09:45:47.560Z
Learning: In ClickHouse tests (e.g., go/pkg/clickhouse/key_verifications_test.go), parallel execution with t.Parallel() is safe when tests use workspaceID-based isolation. Each test generates a unique workspaceID (via uid.New(uid.WorkspacePrefix)), and all database operations are scoped to that workspaceID, providing logical isolation even when tests share a single ClickHouse instance.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/200_test.go
📚 Learning: 2025-11-10T11:50:46.258Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4292
File: go/apps/api/openapi/openapi-generated.yaml:386-387
Timestamp: 2025-11-10T11:50:46.258Z
Learning: Repo unkeyed/unkey: It’s acceptable for pagination properties (e.g., V2IdentitiesListIdentitiesResponseBody.pagination) to omit x-go-type-skip-optional-pointer and x-go-type-skip-optional-pointer-with-omitzero; do not flag this in future reviews.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-07-16T15:38:53.491Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3606
File: go/pkg/db/replica.go:8-11
Timestamp: 2025-07-16T15:38:53.491Z
Learning: For debugging database replica usage in go/pkg/db/replica.go, it's acceptable to mark QueryRowContext operations as "success" even though SQL errors only surface during row.Scan() calls. The timing metrics are the primary concern for debugging replica performance patterns.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
🧬 Code graph analysis (8)
go/pkg/db/key_list_live_by_key_space_id.sql_generated.go (1)
go/pkg/db/types/null_string.go (1)
NullString(10-10)
go/pkg/db/identity_find.sql_generated.go (1)
go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)
go/apps/api/routes/v2_identities_delete_identity/handler.go (3)
go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(46-50)go/apps/api/openapi/gen.go (1)
Identity(161-173)go/pkg/codes/unkey_data.go (1)
Data(112-165)
go/apps/api/routes/v2_apis_list_keys/200_test.go (2)
go/pkg/testutil/seed/seed.go (2)
CreateIdentityRequest(368-373)CreateKeyRequest(189-209)go/pkg/db/models_generated.go (1)
Key(654-678)
go/pkg/db/querier_generated.go (5)
go/pkg/partition/db/database.go (1)
DBTX(10-10)go/pkg/db/identity_find.sql_generated.go (2)
FindIdentityParams(46-50)FindIdentityRow(52-62)go/pkg/db/identity_find_by_external_id.sql_generated.go (1)
FindIdentityByExternalIDParams(20-24)go/pkg/db/models_generated.go (1)
Identity(643-652)go/pkg/db/identity_find_by_id.sql_generated.go (1)
FindIdentityByIDParams(20-24)
go/apps/api/routes/v2_identities_update_identity/handler.go (3)
go/pkg/db/identity_find.sql_generated.go (2)
FindIdentityParams(46-50)FindIdentityRow(52-62)go/pkg/codes/unkey_data.go (1)
Data(112-165)go/pkg/db/custom_types.go (1)
RatelimitInfo(25-33)
go/apps/api/routes/v2_apis_list_keys/handler.go (2)
go/pkg/db/types/null_string.go (1)
NullString(10-10)go/pkg/db/identity_find_by_external_id.sql_generated.go (1)
FindIdentityByExternalIDParams(20-24)
go/apps/api/routes/v2_identities_get_identity/handler.go (3)
go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(46-50)go/apps/api/openapi/gen.go (1)
Identity(161-173)go/pkg/codes/unkey_data.go (1)
Data(112-165)
⏰ 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: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
🔇 Additional comments (5)
go/pkg/db/queries/key_list_live_by_key_space_id.sql (1)
87-87: Please confirm EXPLAIN eliminates the key-space scan.The optional identity clause is now
(? IS NULL OR k.identity_id = ?). MySQL should fold the first branch away once a non-nullidentity_idis bound, but to be sure #4234 is closed we need anEXPLAIN(both with and without the filter) showing the plan hits the(key_auth_id, identity_id)path instead of scanning the full key space. Could you capture and drop that plan in the thread?go/apps/api/routes/v2_identities_delete_identity/handler.go (1)
74-91: LGTM! Improved identity lookup and error handling.The refactor from
FindIdentityWithRatelimits(returning a slice) toFindIdentity(returning a single row) is cleaner and aligns with PR objective #4236. The explicit not-found error handling usingdb.IsNotFound(err)provides better clarity and appropriate error codes.go/apps/api/routes/v2_identities_get_identity/handler.go (1)
53-70: LGTM! Consistent identity lookup refactor.The changes mirror those in the delete handler, providing consistent identity lookup behavior across the API. The explicit not-found handling is clear and uses appropriate error codes.
go/pkg/db/querier_generated.go (1)
281-313: LGTM! UNION ALL pattern enables index usage.The
FindIdentityquery uses UNION ALL to search by eitheridorexternal_id, allowing MySQL to use indexes on both fields. This pattern avoids the OR condition that would prevent index usage, addressing PR objective #4234.go/pkg/db/identity_find.sql_generated.go (1)
13-118: LGTM! UNION ALL implementation is correct.The generated
FindIdentityquery correctly implements the UNION ALL pattern, with theIdentityparameter properly bound to both branches (lines 98 and 102) to match against eitheridorexternal_id. The JSON aggregation of ratelimits is handled correctly, and result scanning includes all fields.
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (11/10/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
chronark
approved these changes
Nov 10, 2025
ogzhanolguncu
approved these changes
Nov 10, 2025
chronark
approved these changes
Nov 10, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes #4236
Fixes #4234
Type of change
How should this be tested?
Tests
Run explain queries against db and see that they dont scan 100k+ rows.
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated