refactor: get rid of n+1 queries for /v2/api/listKeys#3809
refactor: get rid of n+1 queries for /v2/api/listKeys#3809ogzhanolguncu merged 13 commits intomainfrom
Conversation
|
📝 WalkthroughWalkthroughReplaces per-key DB lookups with batched ListLiveKeysByKeyAuthID and a LiveApiByID cache (ApiCache using FindLiveApiByIDRow), adds pagination, optional Vault decryption, extends KeyData (ratelimits), centralizes per-key transformation (private buildKeyResponseData), and updates tests and caches to use LiveApiByID. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant H as Handler (v2_apis_list_keys)
participant AC as ApiCache (LiveApiByID)
participant DB as DB (ListLiveKeysByKeyAuthID)
participant V as Vault (optional)
participant M as Mapper (buildKeyResponseData)
C->>H: GET /v2/apis/{apiId}/keys?limit,cursor,identity,decrypt
H->>AC: SWR(ctx, apiId, callback->FindLiveApiByID)
AC-->>H: (hit,row) or (cache.Null)
alt cache.Null or not found
H-->>C: 404 Not Found
else found
H->>DB: ListLiveKeysByKeyAuthID(keyAuthId, workspaceId, cursor, identity, limit+1)
DB-->>H: []LiveKeyRows
alt decrypt requested
loop per key
H->>V: Decrypt(encrypted_key, workspaceKeyring)
V-->>H: plaintext|null
end
end
loop per key (up to limit)
H->>M: buildKeyResponseData(KeyData, plaintext?)
M-->>H: KeyResponseData
end
H-->>C: {data:[...], hasMore, nextCursor}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 (
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ogzhanolguncu
changed the title
ogzhanolguncu
requested review from
Flo4604,
chronark,
imeyer and
perkinsjr
as code owners
|
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}
There was a problem hiding this comment.
Actionable comments posted: 11
📜 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 (5)
go/apps/api/routes/v2_apis_list_keys/build_key_response_data_test.go(1 hunks)go/apps/api/routes/v2_apis_list_keys/handler.go(2 hunks)go/pkg/db/key_list_live_by_ids.sql_generated.go(1 hunks)go/pkg/db/querier_generated.go(1 hunks)go/pkg/db/queries/key_list_live_by_ids.sql(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/build_key_response_data_test.gogo/pkg/db/key_list_live_by_ids.sql_generated.gogo/pkg/db/querier_generated.gogo/apps/api/routes/v2_apis_list_keys/handler.go
**/*_test.go
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*_test.go: Use table-driven tests in Go
Organize Go integration tests with real dependencies
Organize Go tests by HTTP status codes
Files:
go/apps/api/routes/v2_apis_list_keys/build_key_response_data_test.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/build_key_response_data_test.gogo/pkg/db/key_list_live_by_ids.sql_generated.gogo/pkg/db/querier_generated.gogo/apps/api/routes/v2_apis_list_keys/handler.go
🧠 Learnings (2)
📚 Learning: 2025-08-14T16:25:48.167Z
Learnt from: Flo4604
PR: unkeyed/unkey#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/build_key_response_data_test.go
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
PR: unkeyed/unkey#3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: For go/apps/api/openapi, oapi-codegen is used and does not support OpenAPI 3.1 union types like [T, "null"]; an overlay step is required to downconvert to 3.0-style nullable before code generation.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
🧬 Code Graph Analysis (3)
go/apps/api/routes/v2_apis_list_keys/build_key_response_data_test.go (3)
go/pkg/testutil/http.go (1)
NewHarness(52-175)go/apps/api/routes/v2_apis_list_keys/handler.go (1)
Handler(30-35)go/pkg/db/key_data.go (1)
KeyData(9-24)
go/pkg/db/querier_generated.go (1)
go/pkg/db/key_list_live_by_ids.sql_generated.go (1)
ListLiveKeysByIDsRow(106-142)
go/apps/api/routes/v2_apis_list_keys/handler.go (5)
go/pkg/ptr/pointer.go (1)
P(49-51)go/apps/api/openapi/gen.go (6)
Meta(259-262)KeyResponseData(196-230)Pagination(279-289)KeyCreditsData(170-176)Identity(141-153)RatelimitResponse(400-415)go/pkg/db/key_list_live_by_ids.sql_generated.go (1)
ListLiveKeysByIDsRow(106-142)go/pkg/db/key_find_live_by_id.sql_generated.go (1)
FindLiveKeyByIDRow(105-141)go/pkg/db/key_data.go (2)
ToKeyData(32-45)KeyData(9-24)
🪛 golangci-lint (2.2.2)
go/apps/api/routes/v2_apis_list_keys/handler.go
325-325: openapi.KeyResponseData is missing fields Credits, Expires, Identity, Meta, Name, Permissions, Plaintext, Ratelimits, Roles, UpdatedAt
(exhaustruct)
351-351: openapi.KeyCreditsData is missing field Refill
(exhaustruct)
⏰ 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_ids.sql (2)
1-12: Good move: single batched query eliminates N+1 in the list flowSelecting k.*, embedding api/key_auth/workspace, and aggregating related entities in one round-trip is the right approach for the endpoint’s throughput.
61-77: Tenant safety: constrain ratelimits by workspace_idEven though joining by key_id/identity_id largely scopes rows correctly, explicitly adding rl.workspace_id = k.workspace_id in the ratelimits subquery closes any edge-case cross-tenant leakage if IDs aren’t globally unique.
Suggested change inside the ratelimits subquery:
- FROM ratelimits rl - WHERE rl.key_id = k.id OR rl.identity_id = i.id), + FROM ratelimits rl + WHERE (rl.key_id = k.id OR rl.identity_id = i.id) + AND rl.workspace_id = k.workspace_id),Would you like me to push this constraint to ensure strict tenant isolation?
go/pkg/db/querier_generated.go (1)
1266-1357: Interface surface expanded — verify custom Querier implementations and mocks
TheListLiveKeysByIDsmethod was added to thedb.Querierinterface. Ensure any hand-written implementations or test mocks outside of the sqlc-generated code have been updated to include this new method.• Added:
ListLiveKeysByIDs(ctx context.Context, db DBTX, keyIds []string) ([]ListLiveKeysByIDsRow, error)
• No external implementations or mocks were found in automated searches, but please manually confirm there are none in your codebase.go/pkg/db/key_list_live_by_ids.sql_generated.go (1)
239-246: Empty keyIds handling is correctReplacing the slice placeholder with NULL yields WHERE k.id IN (NULL) which returns zero rows without special-casing logic. Good defensive behavior.
go/apps/api/routes/v2_apis_list_keys/handler.go (1)
8-8: No action required: slices.Sort is supported by Go ≥1.21All modules declare Go 1.23+ (the
go/go.modmodule is set to Go 1.24.4), so the standardslicespackage is available. You can safely keep usingslices.Sortingo/apps/api/routes/v2_apis_list_keys/handler.go.
go/apps/api/routes/v2_apis_list_keys/build_key_response_data_test.go
Outdated
Show resolved
Hide resolved
go/apps/api/routes/v2_apis_list_keys/build_key_response_data_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
go/apps/api/routes/v2_apis_list_keys/handler.go (6)
291-307: Avoid zero-value gaps; build response via append and remove index-based assignmentPre-allocating to len(keys) and skipping entries when keyMap misses leaves zero-value objects in the response. Build the slice via append to return only valid entries.
- responseData := make([]openapi.KeyResponseData, len(keys)) - for i, key := range keys { + responseData := make([]openapi.KeyResponseData, 0, len(keys)) + for _, key := range keys { keyWithDetails, exists := keyMap[key.Key.ID] if !exists { h.Logger.Error("key data not found", "keyId", key.Key.ID) continue } - - keyRow := db.FindLiveKeyByIDRow(keyWithDetails) - keyData := db.ToKeyData(keyRow) + keyData := db.ToKeyData(keyWithDetails) + if keyData == nil { + h.Logger.Error("failed to map key data", "keyId", key.Key.ID) + continue + } response, err := h.BuildKeyResponseData(keyData, plaintextMap[key.Key.ID]) if err != nil { return err } - responseData[i] = response + responseData = append(responseData, response) }
300-302: Compile error: invalid conversion between distinct generated row typesdb.FindLiveKeyByIDRow(keyWithDetails) attempts to cast ListLiveKeysByIDsRow to a different named struct type. Go disallows this. Pass keyWithDetails directly to db.ToKeyData and extend the KeyRow constraints to include ListLiveKeysByIDsRow.
Apply in this file (paired with the previous comment):
- keyRow := db.FindLiveKeyByIDRow(keyWithDetails) - keyData := db.ToKeyData(keyRow) + keyData := db.ToKeyData(keyWithDetails)Then update go/pkg/db/key_data.go to support the new row type:
// go/pkg/db/key_data.go (example patch) type KeyRow interface { ~FindLiveKeyByHashRow | ~FindLiveKeyByIDRow | *FindLiveKeyByHashRow | *FindLiveKeyByIDRow | ListLiveKeysByIDsRow | *ListLiveKeysByIDsRow } func ToKeyData[T KeyRow](row T) *KeyData { switch r := any(row).(type) { case FindLiveKeyByHashRow: return buildKeyData(&r) case *FindLiveKeyByHashRow: return buildKeyData(r) case FindLiveKeyByIDRow: return buildKeyDataFromID(&r) case *FindLiveKeyByIDRow: return buildKeyDataFromID(r) case ListLiveKeysByIDsRow: return buildKeyDataFromID(&r) case *ListLiveKeysByIDsRow: return buildKeyDataFromID(r) default: return nil } }If you prefer, I can draft the exact patch for key_data.go based on the current file contents.
270-287: Optional: parallelize decryption with bounded concurrency to reduce tail latencyDecrypting sequentially can add latency for large pages. Use a small worker pool with context-aware errgroup and a mutex-protected map.
- // Handle decryption if requested - plaintextMap := make(map[string]string) - if req.Decrypt != nil && *req.Decrypt { - for _, key := range keysWithDetails { - if key.EncryptedKey.Valid && key.EncryptionKeyID.Valid { - decrypted, decryptErr := h.Vault.Decrypt(ctx, &vaultv1.DecryptRequest{ - Keyring: key.WorkspaceID, - Encrypted: key.EncryptedKey.String, - }) - if decryptErr != nil { - h.Logger.Error("failed to decrypt key", - "keyId", key.ID, - "error", decryptErr, - ) - continue - } - plaintextMap[key.ID] = decrypted.GetPlaintext() - } - } - } + // Handle decryption if requested + plaintextMap := make(map[string]string) + if req.Decrypt != nil && *req.Decrypt { + var mu sync.Mutex + g, gctx := errgroup.WithContext(ctx) + sem := make(chan struct{}, 8) // bounded concurrency + for _, key := range keysWithDetails { + key := key // capture + if !key.EncryptedKey.Valid || !key.EncryptionKeyID.Valid { + continue + } + sem <- struct{}{} + g.Go(func() error { + defer func() { <-sem }() + decrypted, decryptErr := h.Vault.Decrypt(gctx, &vaultv1.DecryptRequest{ + Keyring: key.WorkspaceID, + Encrypted: key.EncryptedKey.String, + }) + if decryptErr != nil { + h.Logger.Error("failed to decrypt key", "keyId", key.ID, "error", decryptErr) + return nil // continue others + } + mu.Lock() + plaintextMap[key.ID] = decrypted.GetPlaintext() + mu.Unlock() + return nil + }) + } + _ = g.Wait() + }Add imports:
import ( "context" "database/sql" "encoding/json" + "sort" + "sync" "net/http" @@ - "github.com/unkeyed/unkey/go/pkg/vault" + "github.com/unkeyed/unkey/go/pkg/vault" "github.com/unkeyed/unkey/go/pkg/zen" + "golang.org/x/sync/errgroup" )
324-369: Fix exhaustruct lints: avoid partial struct literalsgolangci-lint exhaustruct flags partial composite literals for openapi.KeyResponseData, KeyCreditsData, and KeyCreditsRefill. Use zero-value declaration and assign fields explicitly.
- response := openapi.KeyResponseData{ - CreatedAt: keyData.Key.CreatedAtM, - Enabled: keyData.Key.Enabled, - KeyId: keyData.Key.ID, - Start: keyData.Key.Start, - } + var response openapi.KeyResponseData + response.CreatedAt = keyData.Key.CreatedAtM + response.Enabled = keyData.Key.Enabled + response.KeyId = keyData.Key.ID + response.Start = keyData.Key.Start @@ - if keyData.Key.RemainingRequests.Valid { - response.Credits = &openapi.KeyCreditsData{ - Remaining: nullable.NewNullableWithValue(int64(keyData.Key.RemainingRequests.Int32)), - } + if keyData.Key.RemainingRequests.Valid { + response.Credits = &openapi.KeyCreditsData{} + response.Credits.Remaining = nullable.NewNullableWithValue(int64(keyData.Key.RemainingRequests.Int32)) @@ - response.Credits.Refill = &openapi.KeyCreditsRefill{ - Amount: int64(keyData.Key.RefillAmount.Int32), - Interval: interval, - RefillDay: refillDay, - } + response.Credits.Refill = &openapi.KeyCreditsRefill{} + response.Credits.Refill.Amount = int64(keyData.Key.RefillAmount.Int32) + response.Credits.Refill.Interval = interval + response.Credits.Refill.RefillDay = refillDay } }
377-387: Treat identity meta JSON "null" as absentUnmarshalling "null" yields a nil map; assigning a pointer to a nil map results in "meta": null instead of omitting the field. Gate on identityMeta != nil.
- response.Identity.Meta = &identityMeta + if identityMeta != nil { + response.Identity.Meta = &identityMeta + }
449-460: Same as above: treat key meta JSON "null" as absentAvoid setting a pointer to a nil map so that “meta” is omitted instead of being explicitly null.
- response.Meta = &meta + if meta != nil { + response.Meta = &meta + }
📜 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/apps/api/routes/v2_apis_list_keys/handler.go(1 hunks)go/pkg/db/queries/key_list_live_by_ids.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.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.go
🧠 Learnings (2)
📚 Learning: 2025-07-30T10:46:56.453Z
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.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-08-14T16:26:55.283Z
Learnt from: imeyer
PR: unkeyed/unkey#3785
File: go/pkg/db/key_data.go:54-69
Timestamp: 2025-08-14T16:26:55.283Z
Learning: In the Unkey codebase, readability always trumps code optimization for line count. Struct literals with many fields should be formatted with one field per line for better readability, maintainability, and git diff clarity.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
🧬 Code Graph Analysis (1)
go/apps/api/routes/v2_apis_list_keys/handler.go (4)
go/pkg/ptr/pointer.go (1)
P(49-51)go/apps/api/openapi/gen.go (7)
Meta(259-262)KeyResponseData(196-230)Pagination(279-289)KeyCreditsData(170-176)KeyCreditsRefill(179-190)Identity(141-153)RatelimitResponse(400-415)go/pkg/db/key_list_live_by_ids.sql_generated.go (1)
ListLiveKeysByIDsRow(106-142)go/pkg/db/key_data.go (2)
ToKeyData(32-45)KeyData(9-24)
🪛 golangci-lint (2.2.2)
go/apps/api/routes/v2_apis_list_keys/handler.go
324-324: openapi.KeyResponseData is missing fields Credits, Expires, Identity, Meta, Name, Permissions, Plaintext, Ratelimits, Roles, UpdatedAt
(exhaustruct)
350-350: openapi.KeyCreditsData is missing field Refill
(exhaustruct)
⏰ 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 Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
go/apps/api/routes/v2_apis_list_keys/handler.go (1)
228-234: Pagination logic aligns with “limit+1 with >= cursor” patternThis correctly sets nextCursor to the extra item and returns only the first limit items. With the repo’s established >= semantics, this avoids duplicates while keeping pages contiguous.
[informative]
Uses the approach documented in our learnings (“limit+1” + >=). Good.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
go/apps/api/routes/v2_apis_list_keys/handler.go (8)
174-193: Optional: decrypt with bounded concurrency to reduce tail latencySequential decryption can be slow for large pages. Use a small worker pool with WaitGroup and a semaphore to parallelize while bounding concurrency.
If you want, I can provide a concrete patch using a buffered channel and sync.WaitGroup to keep imports minimal.
222-227: Fix exhaustruct lint: avoid partial composite literalgolangci-lint exhaustruct flags the partial openapi.KeyResponseData literal. Use zero-value then assign fields.
Apply:
- response := openapi.KeyResponseData{ - CreatedAt: keyData.Key.CreatedAtM, - Enabled: keyData.Key.Enabled, - KeyId: keyData.Key.ID, - Start: keyData.Key.Start, - } + var response openapi.KeyResponseData + response.CreatedAt = keyData.Key.CreatedAtM + response.Enabled = keyData.Key.Enabled + response.KeyId = keyData.Key.ID + response.Start = keyData.Key.Start
246-266: Fix exhaustruct on nested credits and refillAvoid partial composite literals for KeyCreditsData and KeyCreditsRefill to satisfy exhaustruct.
Apply:
- if keyData.Key.RemainingRequests.Valid { - response.Credits = &openapi.KeyCreditsData{ - Remaining: nullable.NewNullableWithValue(int64(keyData.Key.RemainingRequests.Int32)), - } + if keyData.Key.RemainingRequests.Valid { + response.Credits = &openapi.KeyCreditsData{} + response.Credits.Remaining = nullable.NewNullableWithValue(int64(keyData.Key.RemainingRequests.Int32)) @@ - response.Credits.Refill = &openapi.KeyCreditsRefill{ - Amount: int64(keyData.Key.RefillAmount.Int32), - Interval: interval, - RefillDay: refillDay, - } + response.Credits.Refill = &openapi.KeyCreditsRefill{} + response.Credits.Refill.Amount = int64(keyData.Key.RefillAmount.Int32) + response.Credits.Refill.Interval = interval + response.Credits.Refill.RefillDay = refillDay
275-285: Treat JSON literal null for identity meta as absentUnmarshalling "null" results in a nil map; assigning a pointer to a nil map is undesirable. Only set Meta when the map is non-nil.
Apply:
- response.Identity.Meta = &identityMeta + if identityMeta != nil { + response.Identity.Meta = &identityMeta + }
296-302: Deterministic permissions: sort slugs before assigningMap iteration order is random. Sort to keep API responses stable.
Apply:
- response.Permissions = &slugs + sort.Strings(slugs) + response.Permissions = &slugsAnd add import:
import ( "context" "encoding/json" + "sort" "net/http"
318-345: Optional: stable ordering for ratelimitsFor consistent responses across runs, sort by Name then Id before assigning.
Apply:
if len(keyRatelimits) > 0 { + sort.Slice(keyRatelimits, func(i, j int) bool { + if keyRatelimits[i].Name == keyRatelimits[j].Name { + return keyRatelimits[i].Id < keyRatelimits[j].Id + } + return keyRatelimits[i].Name < keyRatelimits[j].Name + }) response.Ratelimits = &keyRatelimits } @@ - if len(identityRatelimits) > 0 && response.Identity != nil { + if len(identityRatelimits) > 0 && response.Identity != nil { + sort.Slice(identityRatelimits, func(i, j int) bool { + if identityRatelimits[i].Name == identityRatelimits[j].Name { + return identityRatelimits[i].Id < identityRatelimits[j].Id + } + return identityRatelimits[i].Name < identityRatelimits[j].Name + }) response.Identity.Ratelimits = &identityRatelimits }
347-358: Same “null” handling for key metaGuard assignment to avoid pointer-to-nil map when meta JSON is the literal null.
Apply:
- response.Meta = &meta + if meta != nil { + response.Meta = &meta + }
198-205: Fix invalid row type conversion in Key listing handler
The handler currently callsdb.FindLiveKeyByIDRow(key)on aListLiveKeysByApiIDRow, causing a compile-time type mismatch. Instead, pass the list row directly intodb.ToKeyDataand extendKeyRowso thatToKeyDataacceptsListLiveKeysByApiIDRow.• In
go/apps/api/routes/v2_apis_list_keys/handler.go, replace lines 198–205:- for i, key := range keyResults { - keyRow := db.FindLiveKeyByIDRow(key) - keyData := db.ToKeyData(keyRow) + for i, key := range keyResults { + // Directly convert the list row once KeyRow supports ListLiveKeysByApiIDRow + keyData := db.ToKeyData(key) response, err := h.BuildKeyResponseData(keyData, plaintextMap[key.ID]) if err != nil { return err } responseData[i] = response }• In
go/pkg/db/key_data.go, extend theKeyRowinterface andToKeyDataswitch:type KeyRow interface { - FindLiveKeyByHashRow | FindLiveKeyByIDRow + FindLiveKeyByHashRow | FindLiveKeyByIDRow | ListLiveKeysByApiIDRow } func ToKeyData[T KeyRow](row T) *KeyData { switch r := any(row).(type) { @@ -15,6 +16,12 @@ func ToKeyData[T KeyRow](row T) *KeyData { case *FindLiveKeyByIDRow: return buildKeyDataFromID(r) + case ListLiveKeysByApiIDRow: + // same mapping shape as FindLiveKeyByIDRow + return buildKeyDataFromID(&r) + case *ListLiveKeysByApiIDRow: + return buildKeyDataFromID(r) default: return nil } }
📜 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(2 hunks)go/pkg/db/key_list_live_by_api_id.sql_generated.go(1 hunks)go/pkg/db/querier_generated.go(1 hunks)go/pkg/db/queries/key_list_live_by_api_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_api_id.sql_generated.gogo/apps/api/routes/v2_apis_list_keys/handler.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_api_id.sql_generated.gogo/apps/api/routes/v2_apis_list_keys/handler.go
🧠 Learnings (2)
📚 Learning: 2025-07-30T10:46:56.453Z
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.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-08-14T16:26:55.283Z
Learnt from: imeyer
PR: unkeyed/unkey#3785
File: go/pkg/db/key_data.go:54-69
Timestamp: 2025-08-14T16:26:55.283Z
Learning: In the Unkey codebase, readability always trumps code optimization for line count. Struct literals with many fields should be formatted with one field per line for better readability, maintainability, and git diff clarity.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
🧬 Code Graph Analysis (2)
go/pkg/db/querier_generated.go (1)
go/pkg/db/key_list_live_by_api_id.sql_generated.go (2)
ListLiveKeysByApiIDParams(110-116)ListLiveKeysByApiIDRow(118-154)
go/apps/api/routes/v2_apis_list_keys/handler.go (6)
go/pkg/ptr/deref.go (1)
SafeDeref(35-44)go/pkg/db/key_list_live_by_api_id.sql_generated.go (1)
ListLiveKeysByApiIDParams(110-116)go/apps/api/openapi/gen.go (6)
Identity(141-153)Meta(259-262)KeyResponseData(196-230)Pagination(279-289)KeyCreditsData(170-176)RatelimitResponse(400-415)go/pkg/ptr/pointer.go (1)
P(49-51)go/pkg/db/key_find_live_by_id.sql_generated.go (1)
FindLiveKeyByIDRow(105-141)go/pkg/db/key_data.go (2)
ToKeyData(32-45)KeyData(9-24)
🪛 golangci-lint (2.2.2)
go/apps/api/routes/v2_apis_list_keys/handler.go
222-222: openapi.KeyResponseData is missing fields Credits, Expires, Identity, Meta, Name, Permissions, Plaintext, Ratelimits, Roles, UpdatedAt
(exhaustruct)
248-248: openapi.KeyCreditsData is missing field Refill
(exhaustruct)
⏰ 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). (2)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
🔇 Additional comments (6)
go/pkg/db/queries/key_list_live_by_api_id.sql (3)
83-95: Cursor pagination is consistent with limit+1 patternUsing k.id >= sqlc.arg(id_cursor) with ORDER BY k.id ASC and LIMIT ? works with the "limit+1" approach implemented in the handler. No changes required.
60-76: Boolean normalization for auto_apply is good; verify consumers expect booleanrl.auto_apply = 1 ensures JSON output is a boolean. Confirm downstream unmarshalling (KeyData builder / response mapper) expects a JSON boolean for auto_apply, not an integer.
Would you like me to scan the repo to confirm RatelimitInfo.AutoApply is boolean everywhere it’s consumed?
12-26: Deterministic ordering inside JSON aggregatesIncluding ORDER BY in the roles/permissions subqueries yields deterministic arrays from JSON_ARRAYAGG. This is correct and avoids non-deterministic responses.
go/pkg/db/querier_generated.go (1)
1266-1362: Interface extension LGTMAdding ListLiveKeysByApiID to Querier is additive and matches the SQL. Parameter ordering (api_id, workspace_id, id_cursor, identity x3, limit) aligns with the query.
go/apps/api/routes/v2_apis_list_keys/handler.go (1)
118-124: Pagination logic matches >= cursor semanticsYou correctly set nextCursor to the extra (limit+1-th) row before trimming, avoiding boundary duplication on the next page. This follows our learned pattern.
go/pkg/db/key_list_live_by_api_id.sql_generated.go (1)
110-116: Param struct aligns with SQL placeholdersApiID, WorkspaceID, IDCursor, Identity, Limit ordering matches the query. No issues.
Flo4604
left a comment
Flo4604
left a comment
There was a problem hiding this comment.
Please check those things + the CI is not passing anymore neither locally or in github
you can run it locally by doing
go test -race -timeout=30m ./apps/api/routes/v2_apis_list_keys/... or
go test -json -race -timeout=30m ./apps/api/routes/v2_apis_list_keys/ | tparse -all -progress -smallscreen for a nicer output
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
go/apps/api/routes/v2_apis_list_keys/handler.go (5)
223-241: Optional: parallelize decryption with bounded concurrency.Sequential decrypt may add tail latency for larger pages. Consider an errgroup with a small worker pool (e.g., 8) to decrypt in parallel while preserving order.
340-346: Deterministic permissions output: sort slugs.Map iteration order is random; sort before assigning.
if len(permissionSlugs) > 0 { slugs := make([]string, 0, len(permissionSlugs)) for slug := range permissionSlugs { slugs = append(slugs, slug) } - response.Permissions = &slugs + sort.Strings(slugs) + response.Permissions = &slugs }
382-388: Optional: stabilize ratelimits ordering for consistent responses.Sort by Name then Id before assigning.
- if len(keyRatelimits) > 0 { + if len(keyRatelimits) > 0 { + sort.Slice(keyRatelimits, func(i, j int) bool { + if keyRatelimits[i].Name == keyRatelimits[j].Name { + return keyRatelimits[i].Id < keyRatelimits[j].Id + } + return keyRatelimits[i].Name < keyRatelimits[j].Name + }) response.Ratelimits = &keyRatelimits } @@ - if len(identityRatelimits) > 0 && response.Identity != nil { + if len(identityRatelimits) > 0 && response.Identity != nil { + sort.Slice(identityRatelimits, func(i, j int) bool { + if identityRatelimits[i].Name == identityRatelimits[j].Name { + return identityRatelimits[i].Id < identityRatelimits[j].Id + } + return identityRatelimits[i].Name < identityRatelimits[j].Name + }) response.Identity.Ratelimits = &identityRatelimits }
270-275: Fix exhaustruct lint: avoid partial KeyResponseData composite literal.Initialize and assign fields to satisfy exhaustruct and align with codebase preferences.
- response := openapi.KeyResponseData{ - CreatedAt: keyData.Key.CreatedAtM, - Enabled: keyData.Key.Enabled, - KeyId: keyData.Key.ID, - Start: keyData.Key.Start, - } + var response openapi.KeyResponseData + response.CreatedAt = keyData.Key.CreatedAtM + response.Enabled = keyData.Key.Enabled + response.KeyId = keyData.Key.ID + response.Start = keyData.Key.Start
294-314: Fix exhaustruct lint in nested Credits/Refill struct literals.Avoid composite literals with omitted fields.
- if keyData.Key.RemainingRequests.Valid { - response.Credits = &openapi.KeyCreditsData{ - Remaining: nullable.NewNullableWithValue(int64(keyData.Key.RemainingRequests.Int32)), - } + if keyData.Key.RemainingRequests.Valid { + response.Credits = &openapi.KeyCreditsData{} + response.Credits.Remaining = nullable.NewNullableWithValue(int64(keyData.Key.RemainingRequests.Int32)) @@ - response.Credits.Refill = &openapi.KeyCreditsRefill{ - Amount: int64(keyData.Key.RefillAmount.Int32), - Interval: interval, - RefillDay: refillDay, - } + response.Credits.Refill = &openapi.KeyCreditsRefill{} + response.Credits.Refill.Amount = int64(keyData.Key.RefillAmount.Int32) + response.Credits.Refill.Interval = interval + response.Credits.Refill.RefillDay = refillDay
📜 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 (5)
go/apps/api/routes/v2_apis_list_keys/handler.go(1 hunks)go/pkg/db/key_data.go(4 hunks)go/pkg/db/key_list_live_by_auth_id.sql_generated.go(1 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.gogo/apps/api/routes/v2_apis_list_keys/handler.gogo/pkg/db/key_data.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.gogo/apps/api/routes/v2_apis_list_keys/handler.gogo/pkg/db/key_data.go
🧠 Learnings (5)
📚 Learning: 2025-07-30T10:46:56.453Z
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.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-08-14T16:26:55.283Z
Learnt from: imeyer
PR: unkeyed/unkey#3785
File: go/pkg/db/key_data.go:54-69
Timestamp: 2025-08-14T16:26:55.283Z
Learning: In the Unkey codebase, readability always trumps code optimization for line count. Struct literals with many fields should be formatted with one field per line for better readability, maintainability, and git diff clarity.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-07-15T14:25:05.608Z
Learnt from: chronark
PR: unkeyed/unkey#3560
File: go/apps/api/routes/v2_keys_create_key/handler.go:353-466
Timestamp: 2025-07-15T14:25:05.608Z
Learning: In the Unkey codebase, input validation for API endpoints is handled at the OpenAPI schema layer, which validates request fields like permission slugs (pattern: "^[a-zA-Z0-9_]+$", length: 1-100 characters) before requests reach the handler code. This validation occurs during the zen.BindBody call in handlers.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-03-19T09:25:59.751Z
Learnt from: Flo4604
PR: unkeyed/unkey#2955
File: go/apps/api/routes/v2_identities_create_identity/handler.go:162-202
Timestamp: 2025-03-19T09:25:59.751Z
Learning: In the Unkey codebase, input validation for API endpoints is primarily handled through OpenAPI schema validation, which occurs before requests reach the handler code. For example, in the identities.createIdentity endpoint, minimum values for ratelimit duration and limit are defined in the OpenAPI schema rather than duplicating these checks in the handler.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-08-19T09:42:40.897Z
Learnt from: Flo4604
PR: unkeyed/unkey#3800
File: go/internal/services/keys/validation.go:45-52
Timestamp: 2025-08-19T09:42:40.897Z
Learning: In go/internal/services/keys/validation.go, keys with unlimited usage (RemainingRequests.Valid = false) have an early return that bypasses the usage limiter entirely. The usage limiter is only called for keys with finite remaining request counts.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
🧬 Code Graph Analysis (4)
go/pkg/db/querier_generated.go (1)
go/pkg/db/key_list_live_by_auth_id.sql_generated.go (2)
ListLiveKeysByKeyAuthIDParams(107-113)ListLiveKeysByKeyAuthIDRow(115-150)
go/pkg/db/key_list_live_by_auth_id.sql_generated.go (2)
go/pkg/rbac/permissions.go (1)
Limit(74-74)go/pkg/db/types/null_string.go (1)
NullString(10-10)
go/apps/api/routes/v2_apis_list_keys/handler.go (5)
go/pkg/ptr/deref.go (1)
SafeDeref(35-44)go/pkg/db/key_list_live_by_auth_id.sql_generated.go (1)
ListLiveKeysByKeyAuthIDParams(107-113)go/apps/api/openapi/gen.go (6)
Identity(141-153)Meta(259-262)KeyResponseData(196-230)Pagination(279-289)KeyCreditsData(170-176)RatelimitResponse(400-415)go/pkg/ptr/pointer.go (1)
P(49-51)go/pkg/db/key_data.go (2)
ToKeyData(29-46)KeyData(9-21)
go/pkg/db/key_data.go (3)
go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
FindLiveKeyByHashRow(104-140)go/pkg/db/key_find_live_by_id.sql_generated.go (1)
FindLiveKeyByIDRow(105-141)go/pkg/db/key_list_live_by_auth_id.sql_generated.go (1)
ListLiveKeysByKeyAuthIDRow(115-150)
🪛 golangci-lint (2.2.2)
go/pkg/db/key_data.go
54-54: db.KeyData is missing field Identity
(exhaustruct)
80-80: db.Api is missing fields ID, Name, WorkspaceID, IpWhitelist, AuthType, KeyAuthID, CreatedAtM, UpdatedAtM, DeletedAtM, DeleteProtection
(exhaustruct)
92-92: db.Identity is missing fields Environment, Deleted, CreatedAt, UpdatedAt
(exhaustruct)
⏰ 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: Build / Build
- GitHub Check: Test Go API Local / Test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
go/pkg/db/queries/key_list_live_by_auth_id.sql (2)
81-92: Cursor filter semantics look correct for limit+1 pagination.Using k.id >= sqlc.arg(id_cursor) aligns with our “limit+1, cursor = extra row” pattern (see team learning). No change needed.
12-25: Move ORDER BY inside JSON_ARRAYAGG for deterministic arraysIn go/pkg/db/queries/key_list_live_by_auth_id.sql, update each JSON_ARRAYAGG call to include its ORDER BY clause inside the aggregate, and add a stable sort for ratelimits:
• roles (lines 12–25): JSON_ARRAYAGG(… ORDER BY r.name)
• permissions (27–41): JSON_ARRAYAGG(… ORDER BY p.slug)
• role_permissions (43–58): JSON_ARRAYAGG(… ORDER BY p.slug)
• ratelimits (60–75): JSON_ARRAYGAG(… ORDER BY rl.name, rl.id)MySQL 8.0.14+ (and MariaDB equivalents) supports ORDER BY inside JSON_ARRAYAGG. Please confirm your database version meets this requirement; if not, leave arrays unsorted in SQL and perform sorting in Go instead.
go/pkg/db/querier_generated.go (1)
1266-1359: New interface method addition looks consistent.Signature and embedded SQL doc match the source query. After updating the SQL to move ORDER BY inside JSON_ARRAYAGG, re-generate to keep this in sync.
go/apps/api/routes/v2_apis_list_keys/handler.go (1)
202-208: Pagination logic matches >= + limit+1 pattern.You set nextCursor to the extra row and trim the slice. This avoids duplicates across pages without dropping the first row on continuation. Good.
go/pkg/db/key_list_live_by_auth_id.sql_generated.go (1)
13-105: Generated code scan order matches SQL; no issues.Once the source SQL ORDER BY changes are applied, re-run sqlc to keep this file aligned.
Also applies to: 107-150, 152-333
|
sure lmk when you want |
|
want me to go over it now or huddle tomorrow and do it together? |
|
ahh you mean cache? |
|
aah shit. Let's check it together then we can add cache too, wdyt? |
|
sure |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
go/pkg/db/queries/key_list_live_by_auth_id.sql (1)
74-82: Optional: add a covering composite index to sustain list performance.Given predicates on
(key_auth_id, workspace_id, id), consider adding a non-unique index:
CREATE INDEX idx_keys_auth_ws_id ON keys(key_auth_id, workspace_id, id);Improves page scans and keeps the planner on index range scans at scale.
go/pkg/db/key_data.go (1)
91-98: Align identity presence with other builders and fix exhaustruct on Identity.Use
IdentityTableID.Valid(join existence) instead ofIdentityID.Valid(FK presence), and avoid composite literal to satisfy exhaustruct.- if r.IdentityID.Valid { - kd.Identity = &Identity{ - ID: r.IdentityID.String, - ExternalID: r.IdentityExternalID.String, - WorkspaceID: r.WorkspaceID, - Meta: r.IdentityMeta, - } - } + if r.IdentityTableID.Valid { + ident := &Identity{} + ident.ID = r.IdentityTableID.String + ident.ExternalID = r.IdentityExternalID.String + ident.WorkspaceID = r.WorkspaceID + ident.Meta = r.IdentityMeta + kd.Identity = ident + }This matches the
buildKeyDatapath and keeps Identity omitted if the joined row is soft-deleted.go/apps/api/routes/v2_apis_list_keys/handler.go (4)
267-274: Fix exhaustruct: avoid partial composite literal for response.Use assignment style to satisfy the linter on generated OpenAPI types.
-func (h *Handler) BuildKeyResponseData(keyData *db.KeyData, plaintext string) (openapi.KeyResponseData, error) { - response := openapi.KeyResponseData{ - CreatedAt: keyData.Key.CreatedAtM, - Enabled: keyData.Key.Enabled, - KeyId: keyData.Key.ID, - Start: keyData.Key.Start, - } +func (h *Handler) BuildKeyResponseData(keyData *db.KeyData, plaintext string) (openapi.KeyResponseData, error) { + var response openapi.KeyResponseData + response.CreatedAt = keyData.Key.CreatedAtM + response.Enabled = keyData.Key.Enabled + response.KeyId = keyData.Key.ID + response.Start = keyData.Key.Start
381-387: Optional: stable ordering for ratelimits in responses.Sort
keyRatelimitsandidentityRatelimits(e.g., by Name then Id) for deterministic output across calls.- if len(keyRatelimits) > 0 { + if len(keyRatelimits) > 0 { + sort.Slice(keyRatelimits, func(i, j int) bool { + if keyRatelimits[i].Name == keyRatelimits[j].Name { + return keyRatelimits[i].Id < keyRatelimits[j].Id + } + return keyRatelimits[i].Name < keyRatelimits[j].Name + }) response.Ratelimits = &keyRatelimits } @@ - if len(identityRatelimits) > 0 && response.Identity != nil { + if len(identityRatelimits) > 0 && response.Identity != nil { + sort.Slice(identityRatelimits, func(i, j int) bool { + if identityRatelimits[i].Name == identityRatelimits[j].Name { + return identityRatelimits[i].Id < identityRatelimits[j].Id + } + return identityRatelimits[i].Name < identityRatelimits[j].Name + }) response.Identity.Ratelimits = &identityRatelimits }Remember to import sort (see previous comment).
339-345: Deterministic permissions: sort slugs before returning.Map iteration order is random. Ensure stable output by sorting.
if len(permissionSlugs) > 0 { slugs := make([]string, 0, len(permissionSlugs)) for slug := range permissionSlugs { slugs = append(slugs, slug) } + sort.Strings(slugs) response.Permissions = &slugs }Add import:
import ( "context" "encoding/json" "net/http" + "sort"
293-313: Fix exhaustruct on nested credit types; assign fields explicitly.Refactor to assignment style for
KeyCreditsDataandKeyCreditsRefill.- if keyData.Key.RemainingRequests.Valid { - response.Credits = &openapi.KeyCreditsData{ - Remaining: nullable.NewNullableWithValue(int64(keyData.Key.RemainingRequests.Int32)), - } + if keyData.Key.RemainingRequests.Valid { + response.Credits = &openapi.KeyCreditsData{} + response.Credits.Remaining = nullable.NewNullableWithValue(int64(keyData.Key.RemainingRequests.Int32)) @@ - response.Credits.Refill = &openapi.KeyCreditsRefill{ - Amount: int64(keyData.Key.RefillAmount.Int32), - Interval: interval, - RefillDay: refillDay, - } + response.Credits.Refill = &openapi.KeyCreditsRefill{} + response.Credits.Refill.Amount = int64(keyData.Key.RefillAmount.Int32) + response.Credits.Refill.Interval = interval + response.Credits.Refill.RefillDay = refillDay
📜 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 (5)
go/apps/api/routes/v2_apis_list_keys/handler.go(1 hunks)go/pkg/db/key_data.go(4 hunks)go/pkg/db/key_list_live_by_auth_id.sql_generated.go(1 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/apps/api/routes/v2_apis_list_keys/handler.gogo/pkg/db/key_list_live_by_auth_id.sql_generated.gogo/pkg/db/key_data.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/apps/api/routes/v2_apis_list_keys/handler.gogo/pkg/db/key_list_live_by_auth_id.sql_generated.gogo/pkg/db/key_data.go
🧠 Learnings (5)
📚 Learning: 2025-07-30T10:46:56.453Z
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.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-08-14T16:26:55.283Z
Learnt from: imeyer
PR: unkeyed/unkey#3785
File: go/pkg/db/key_data.go:54-69
Timestamp: 2025-08-14T16:26:55.283Z
Learning: In the Unkey codebase, readability always trumps code optimization for line count. Struct literals with many fields should be formatted with one field per line for better readability, maintainability, and git diff clarity.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-07-15T14:25:05.608Z
Learnt from: chronark
PR: unkeyed/unkey#3560
File: go/apps/api/routes/v2_keys_create_key/handler.go:353-466
Timestamp: 2025-07-15T14:25:05.608Z
Learning: In the Unkey codebase, input validation for API endpoints is handled at the OpenAPI schema layer, which validates request fields like permission slugs (pattern: "^[a-zA-Z0-9_]+$", length: 1-100 characters) before requests reach the handler code. This validation occurs during the zen.BindBody call in handlers.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-03-19T09:25:59.751Z
Learnt from: Flo4604
PR: unkeyed/unkey#2955
File: go/apps/api/routes/v2_identities_create_identity/handler.go:162-202
Timestamp: 2025-03-19T09:25:59.751Z
Learning: In the Unkey codebase, input validation for API endpoints is primarily handled through OpenAPI schema validation, which occurs before requests reach the handler code. For example, in the identities.createIdentity endpoint, minimum values for ratelimit duration and limit are defined in the OpenAPI schema rather than duplicating these checks in the handler.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-08-19T09:42:40.897Z
Learnt from: Flo4604
PR: unkeyed/unkey#3800
File: go/internal/services/keys/validation.go:45-52
Timestamp: 2025-08-19T09:42:40.897Z
Learning: In go/internal/services/keys/validation.go, keys with unlimited usage (RemainingRequests.Valid = false) have an early return that bypasses the usage limiter entirely. The usage limiter is only called for keys with finite remaining request counts.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
🧬 Code Graph Analysis (3)
go/pkg/db/querier_generated.go (1)
go/pkg/db/key_list_live_by_auth_id.sql_generated.go (2)
ListLiveKeysByKeyAuthIDParams(105-111)ListLiveKeysByKeyAuthIDRow(113-146)
go/apps/api/routes/v2_apis_list_keys/handler.go (6)
go/pkg/ptr/deref.go (1)
SafeDeref(35-44)go/pkg/db/key_list_live_by_auth_id.sql_generated.go (1)
ListLiveKeysByKeyAuthIDParams(105-111)go/apps/api/openapi/gen.go (6)
Identity(141-153)Meta(259-262)KeyResponseData(196-230)KeyCreditsData(170-176)KeyCreditsRefill(179-190)RatelimitResponse(400-415)go/pkg/db/models_generated.go (3)
Identity(714-723)EncryptedKey(694-701)Key(725-749)go/pkg/ptr/pointer.go (1)
P(49-51)go/pkg/db/key_data.go (2)
ToKeyData(29-46)KeyData(9-21)
go/pkg/db/key_data.go (3)
go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
FindLiveKeyByHashRow(104-140)go/pkg/db/key_find_live_by_id.sql_generated.go (1)
FindLiveKeyByIDRow(105-141)go/pkg/db/key_list_live_by_auth_id.sql_generated.go (1)
ListLiveKeysByKeyAuthIDRow(113-146)
🪛 golangci-lint (2.2.2)
go/pkg/db/key_data.go
54-54: db.KeyData is missing field Identity
(exhaustruct)
80-80: db.Api is missing fields ID, Name, WorkspaceID, IpWhitelist, AuthType, KeyAuthID, CreatedAtM, UpdatedAtM, DeletedAtM, DeleteProtection
(exhaustruct)
81-81: db.KeyAuth is missing fields ID, WorkspaceID, CreatedAtM, UpdatedAtM, DeletedAtM, StoreEncryptedKeys, DefaultPrefix, DefaultBytes, SizeApprox, SizeLastUpdatedAt
(exhaustruct)
82-82: db.Workspace is missing fields ID, OrgID, Name, PartitionID, Plan, Tier, StripeCustomerID, StripeSubscriptionID, BetaFeatures, Features, Subscriptions, Enabled, DeleteProtection, CreatedAtM, UpdatedAtM, DeletedAtM
(exhaustruct)
92-92: db.Identity is missing fields Environment, Deleted, CreatedAt, UpdatedAt
(exhaustruct)
🔇 Additional comments (7)
go/pkg/db/queries/key_list_live_by_auth_id.sql (2)
79-90: Pagination and filters look correct for limit+1 pattern.
k.id >= sqlc.arg(id_cursor)with ASC ordering and applying limit+1 is compatible with the handler’s “nextCursor = extra row” approach; no duplicate row across pages when clients follow the contract.
58-73: auto_apply is encoded as 0/1, which fails JSON -> bool in Go; emit JSON booleans.Using
rl.auto_apply = 1yields numeric 0/1 in JSON. Go’s json.Unmarshal will not decode numbers intobooland will fail the entire ratelimits decode (you currently ignore the error → array stays empty). Emit true/false JSON values instead.- 'auto_apply', rl.auto_apply = 1 + 'auto_apply', IF(rl.auto_apply = 1, JSON_EXTRACT('true', '$'), JSON_EXTRACT('false', '$'))This ensures
auto_applyis a proper JSON boolean, allowing clean unmarshal into a Go bool.⛔ Skipped due to learnings
Learnt from: Flo4604 PR: unkeyed/unkey#3785 File: go/pkg/db/querier_generated.go:387-403 Timestamp: 2025-08-14T18:31:49.604Z Learning: In MySQL's JSON_OBJECT function, boolean expressions like `rl.auto_apply = 1` automatically convert to proper JSON boolean values (true/false), not numeric values (0/1). This means Go's json.Unmarshal can correctly handle these fields when unmarshalling into bool types without any conversion issues.go/pkg/db/querier_generated.go (1)
1266-1357: Interface addition is consistent and non-breaking.
ListLiveKeysByKeyAuthIDsignature and parameter/row types align with the generated implementation. Safe additive change to theQuerierinterface.go/pkg/db/key_data.go (1)
100-112: Graceful JSON decode is fine.Ignoring decode errors and defaulting to empty slices prevents a single malformed record from breaking the list. Intentional and acceptable here.
go/apps/api/routes/v2_apis_list_keys/handler.go (2)
202-208: Pagination logic is correct for >= cursor with limit+1.You set nextCursor to the extra row and then slice to the requested size. This avoids boundary duplication across pages.
245-253: Correct use of ToKeyData with batched row type.Passing the generated row directly to
db.ToKeyDataavoids invalid cross-type conversions. Good fix.go/pkg/db/key_list_live_by_auth_id.sql_generated.go (1)
113-146: Generated types match SQL selection; scan order looks correct.Row struct fields and scan order align with the SELECT list. Once you adjust
auto_applyto emit booleans in the SQL file, regeneration will propagate here.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go/apps/api/routes/v2_apis_delete_api/cache_validation_test.go (1)
18-76: Optional: adopt table-driven test styleNot required here, but adopting a table-driven style keeps tests consistent with repo guidelines and scales if you add more cases (e.g., different deletion scenarios).
♻️ Duplicate comments (3)
go/apps/api/routes/v2_apis_list_keys/handler.go (3)
159-177: Remove unnecessary nolint:gosec on Limit cast (OpenAPI clamps limit)OpenAPI validation clamps limit; int32(limit+1) is safe. Keep the codebase lint-clean.
Apply:
- Limit: int32(limit + 1), // nolint:gosec + Limit: int32(limit + 1),
252-260: Avoid exhaustruct: replace partial composites with assignmentsopenapi.KeyResponseData and nested structs likely trigger exhaustruct lints. Use zero-value declarations and field assignments to set only needed fields.
Apply:
-func (h *Handler) buildKeyResponseData(keyData *db.KeyData, plaintext string) (openapi.KeyResponseData, error) { - response := openapi.KeyResponseData{ - CreatedAt: keyData.Key.CreatedAtM, - Enabled: keyData.Key.Enabled, - KeyId: keyData.Key.ID, - Start: keyData.Key.Start, - } +func (h *Handler) buildKeyResponseData(keyData *db.KeyData, plaintext string) (openapi.KeyResponseData, error) { + var response openapi.KeyResponseData + response.CreatedAt = keyData.Key.CreatedAtM + response.Enabled = keyData.Key.Enabled + response.KeyId = keyData.Key.ID + response.Start = keyData.Key.Start @@ - if keyData.Key.RemainingRequests.Valid { - response.Credits = &openapi.KeyCreditsData{ - Remaining: nullable.NewNullableWithValue(int64(keyData.Key.RemainingRequests.Int32)), - } + if keyData.Key.RemainingRequests.Valid { + response.Credits = &openapi.KeyCreditsData{} + response.Credits.Remaining = nullable.NewNullableWithValue(int64(keyData.Key.RemainingRequests.Int32)) @@ - response.Credits.Refill = &openapi.KeyCreditsRefill{ - Amount: int64(keyData.Key.RefillAmount.Int32), - Interval: interval, - RefillDay: refillDay, - } + response.Credits.Refill = &openapi.KeyCreditsRefill{} + response.Credits.Refill.Amount = int64(keyData.Key.RefillAmount.Int32) + response.Credits.Refill.Interval = interval + response.Credits.Refill.RefillDay = refillDay } }Also applies to: 278-298
187-193: Fix pagination duplicate risk with >= cursor by dropping boundary rowWith k.id >= ? and limit+1 fetches, the next page will include the boundary item as the first row. Drop it when continuing with a cursor before computing hasMore/nextCursor.
Apply:
- // Handle pagination - hasMore := len(keyResults) > limit - var nextCursor *string - if hasMore { - nextCursor = ptr.P(keyResults[len(keyResults)-1].ID) - keyResults = keyResults[:limit] - } + // Handle pagination (>= semantics): drop boundary row when continuing + 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] + }Also applies to: 159-165
📜 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 (14)
go/apps/api/routes/register.go(1 hunks)go/apps/api/routes/v2_apis_delete_api/cache_validation_test.go(2 hunks)go/apps/api/routes/v2_apis_delete_api/handler.go(2 hunks)go/apps/api/routes/v2_apis_list_keys/200_test.go(1 hunks)go/apps/api/routes/v2_apis_list_keys/400_test.go(1 hunks)go/apps/api/routes/v2_apis_list_keys/401_test.go(1 hunks)go/apps/api/routes/v2_apis_list_keys/403_test.go(1 hunks)go/apps/api/routes/v2_apis_list_keys/404_test.go(2 hunks)go/apps/api/routes/v2_apis_list_keys/412_test.go(1 hunks)go/apps/api/routes/v2_apis_list_keys/handler.go(4 hunks)go/internal/services/caches/caches.go(3 hunks)go/pkg/db/api_find_live_by_id.sql_generated.go(1 hunks)go/pkg/db/querier_generated.go(2 hunks)go/pkg/db/queries/api_find_live_by_id.sql(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/api_find_live_by_id.sql_generated.gogo/apps/api/routes/register.gogo/apps/api/routes/v2_apis_list_keys/401_test.gogo/apps/api/routes/v2_apis_list_keys/400_test.gogo/apps/api/routes/v2_apis_list_keys/403_test.gogo/apps/api/routes/v2_apis_list_keys/404_test.gogo/apps/api/routes/v2_apis_list_keys/200_test.gogo/apps/api/routes/v2_apis_delete_api/handler.gogo/apps/api/routes/v2_apis_delete_api/cache_validation_test.gogo/apps/api/routes/v2_apis_list_keys/412_test.gogo/pkg/db/querier_generated.gogo/internal/services/caches/caches.gogo/apps/api/routes/v2_apis_list_keys/handler.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/api_find_live_by_id.sql_generated.gogo/apps/api/routes/register.gogo/apps/api/routes/v2_apis_list_keys/401_test.gogo/apps/api/routes/v2_apis_list_keys/400_test.gogo/apps/api/routes/v2_apis_list_keys/403_test.gogo/apps/api/routes/v2_apis_list_keys/404_test.gogo/apps/api/routes/v2_apis_list_keys/200_test.gogo/apps/api/routes/v2_apis_delete_api/handler.gogo/apps/api/routes/v2_apis_delete_api/cache_validation_test.gogo/apps/api/routes/v2_apis_list_keys/412_test.gogo/pkg/db/querier_generated.gogo/internal/services/caches/caches.gogo/apps/api/routes/v2_apis_list_keys/handler.go
**/*_test.go
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*_test.go: Use table-driven tests in Go
Organize Go integration tests with real dependencies
Organize Go tests by HTTP status codes
Files:
go/apps/api/routes/v2_apis_list_keys/401_test.gogo/apps/api/routes/v2_apis_list_keys/400_test.gogo/apps/api/routes/v2_apis_list_keys/403_test.gogo/apps/api/routes/v2_apis_list_keys/404_test.gogo/apps/api/routes/v2_apis_list_keys/200_test.gogo/apps/api/routes/v2_apis_delete_api/cache_validation_test.gogo/apps/api/routes/v2_apis_list_keys/412_test.go
🧠 Learnings (5)
📚 Learning: 2025-07-30T10:46:56.453Z
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.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-08-14T16:26:55.283Z
Learnt from: imeyer
PR: unkeyed/unkey#3785
File: go/pkg/db/key_data.go:54-69
Timestamp: 2025-08-14T16:26:55.283Z
Learning: In the Unkey codebase, readability always trumps code optimization for line count. Struct literals with many fields should be formatted with one field per line for better readability, maintainability, and git diff clarity.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-07-15T14:25:05.608Z
Learnt from: chronark
PR: unkeyed/unkey#3560
File: go/apps/api/routes/v2_keys_create_key/handler.go:353-466
Timestamp: 2025-07-15T14:25:05.608Z
Learning: In the Unkey codebase, input validation for API endpoints is handled at the OpenAPI schema layer, which validates request fields like permission slugs (pattern: "^[a-zA-Z0-9_]+$", length: 1-100 characters) before requests reach the handler code. This validation occurs during the zen.BindBody call in handlers.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-03-19T09:25:59.751Z
Learnt from: Flo4604
PR: unkeyed/unkey#2955
File: go/apps/api/routes/v2_identities_create_identity/handler.go:162-202
Timestamp: 2025-03-19T09:25:59.751Z
Learning: In the Unkey codebase, input validation for API endpoints is primarily handled through OpenAPI schema validation, which occurs before requests reach the handler code. For example, in the identities.createIdentity endpoint, minimum values for ratelimit duration and limit are defined in the OpenAPI schema rather than duplicating these checks in the handler.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-08-19T09:42:40.919Z
Learnt from: Flo4604
PR: unkeyed/unkey#3800
File: go/internal/services/keys/validation.go:45-52
Timestamp: 2025-08-19T09:42:40.919Z
Learning: In go/internal/services/keys/validation.go, keys with unlimited usage (RemainingRequests.Valid = false) have an early return that bypasses the usage limiter entirely. The usage limiter is only called for keys with finite remaining request counts.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
🧬 Code Graph Analysis (13)
go/pkg/db/api_find_live_by_id.sql_generated.go (2)
go/pkg/db/types/null_string.go (1)
NullString(10-10)go/pkg/db/models_generated.go (1)
NullApisAuthType(35-38)
go/apps/api/routes/register.go (1)
go/internal/services/caches/caches.go (1)
Caches(15-27)
go/apps/api/routes/v2_apis_list_keys/401_test.go (1)
go/internal/services/caches/caches.go (1)
Caches(15-27)
go/apps/api/routes/v2_apis_list_keys/400_test.go (1)
go/internal/services/caches/caches.go (1)
Caches(15-27)
go/apps/api/routes/v2_apis_list_keys/403_test.go (1)
go/internal/services/caches/caches.go (1)
Caches(15-27)
go/apps/api/routes/v2_apis_list_keys/404_test.go (3)
go/internal/services/caches/caches.go (1)
Caches(15-27)go/pkg/testutil/http.go (1)
CallRoute(259-293)go/apps/api/openapi/gen.go (1)
NotFoundErrorResponse(270-276)
go/apps/api/routes/v2_apis_list_keys/200_test.go (1)
go/internal/services/caches/caches.go (1)
Caches(15-27)
go/apps/api/routes/v2_apis_delete_api/handler.go (2)
go/apps/api/openapi/gen.go (2)
V2ApisDeleteApiRequestBody(490-496)V2ApisDeleteApiResponseBody(499-505)go/internal/services/caches/caches.go (1)
Caches(15-27)
go/apps/api/routes/v2_apis_delete_api/cache_validation_test.go (2)
go/internal/services/caches/caches.go (1)
Caches(15-27)go/pkg/db/api_find_live_by_id.sql_generated.go (1)
FindLiveApiByIDRow(22-34)
go/apps/api/routes/v2_apis_list_keys/412_test.go (1)
go/internal/services/caches/caches.go (1)
Caches(15-27)
go/pkg/db/querier_generated.go (2)
go/pkg/db/api_find_live_by_id.sql_generated.go (1)
FindLiveApiByIDRow(22-34)go/pkg/db/key_list_live_by_auth_id.sql_generated.go (2)
ListLiveKeysByKeyAuthIDParams(105-111)ListLiveKeysByKeyAuthIDRow(113-146)
go/internal/services/caches/caches.go (4)
go/pkg/cache/interface.go (1)
Cache(7-32)go/pkg/db/api_find_live_by_id.sql_generated.go (1)
FindLiveApiByIDRow(22-34)go/pkg/cache/cache.go (2)
New(55-99)Config(33-50)go/pkg/cache/middleware/tracing.go (1)
WithTracing(16-18)
go/apps/api/routes/v2_apis_list_keys/handler.go (6)
go/pkg/db/api_find_live_by_id.sql_generated.go (1)
FindLiveApiByIDRow(22-34)go/pkg/db/handle_err_no_rows.go (1)
IsNotFound(8-10)go/pkg/cache/interface.go (1)
Null(42-42)go/pkg/ptr/deref.go (1)
SafeDeref(35-44)go/pkg/db/key_list_live_by_auth_id.sql_generated.go (1)
ListLiveKeysByKeyAuthIDParams(105-111)go/pkg/db/key_data.go (2)
ToKeyData(29-46)KeyData(9-21)
⏰ 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 (16)
go/pkg/db/queries/api_find_live_by_id.sql (1)
1-7: Query shape looks correct for “live” semantics (inner join + deleted_at_m filters).Good use of
sqlc.embed(ka)and ensuring both API and key_auth are not deleted. This should support the LiveApiByID cache well.go/internal/services/caches/caches.go (1)
92-99: Confirm cache invalidation on update endpointsWe only see
LiveApiByID.SetNullin the delete handler—no invalidation in any update routes. With a 24 h stale TTL, updates will serve stale data unless explicitly removed. Please manually verify that:
- All mutation handlers (e.g. API update/patch) call
h.Caches.LiveApiByID.Remove(...)orSetNull(...)after persisting changes.- If an update endpoint is missing or named differently, ensure its handler includes the cache invalidation.
go/apps/api/routes/v2_apis_list_keys/403_test.go (1)
24-29: Wiring ApiCache to LiveApiByID looks correct.Dependency injection aligns with the new handler contract and cache type.
go/apps/api/routes/v2_apis_list_keys/412_test.go (1)
25-30: Good: tests now pass the LiveApiByID cache to the handler.Keeps test setup consistent with the production wiring.
go/apps/api/routes/v2_apis_list_keys/401_test.go (1)
17-22: LGTM: ApiCache dependency added to test route.Matches the handler’s new field and the cache type db.FindLiveApiByIDRow.
go/apps/api/routes/v2_apis_list_keys/400_test.go (1)
18-22: LGTM: ApiCache wiring added to the test harness route initializationUsing h.Caches.LiveApiByID here keeps tests aligned with the handler’s new dependency on the live API cache.
go/apps/api/routes/register.go (1)
256-261: Verify all v2_apis_list_keys.Handler initializations include ApiCacheI ran a repository-wide search for
&v2_apis_list_keys.Handler{and only found the one ingo/apps/api/routes/register.go. No other handler instantiations were detected—and thus none appear to be missing the newApiCachefield.Please double-check any tests or other wiring that might directly construct this handler to ensure they also pass:
- Any unit/integration tests importing
v2_apis_list_keys.Handler- Other route or middleware registrations outside of
register.goOnce you’ve confirmed there are no remaining initializations without
ApiCache: svc.Caches.LiveApiByID, this change is good to merge.go/apps/api/routes/v2_apis_list_keys/200_test.go (1)
28-33: LGTM: Tests aligned with ApiCache dependencyInjecting h.Caches.LiveApiByID ensures the test exercises the cached API lookup path consistently with production wiring.
go/apps/api/routes/v2_apis_delete_api/handler.go (1)
160-160: Correct: Invalidate LiveApiByID after deletionSetting a negative cache entry on delete prevents stale reads and avoids an extra DB hit. This aligns with the new LiveApiByID cache semantics.
go/apps/api/routes/v2_apis_list_keys/404_test.go (2)
25-30: LGTM: ApiCache wiring added to the test harness route initializationConsistent with handler expectations; mirrors production route wiring.
172-180: Confirm 404 vs 412 semantics for APIs without KeyAuthThe expectation was changed from 412 (precondition) to 404. Returning 404 can be preferable to avoid information leakage, but please confirm this aligns with API contract and any existing client expectations/docs.
If this is the intended contract change, consider adding/changing OpenAPI docs and a short note in the changelog. Do you want me to draft the doc snippet?
go/apps/api/routes/v2_apis_delete_api/cache_validation_test.go (2)
49-51: LGTM: Migrated to LiveApiByID cache with correct row typeSWR callback and query use FindLiveApiByID consistently; cache value type aligns with caches.Caches.LiveApiByID.
74-75: LGTM: Cache invalidation assertionPost-delete check uses LiveApiByID.Get and expects cache.Null. This verifies eviction correctly.
go/pkg/db/api_find_live_by_id.sql_generated.go (1)
44-70: LGTM: Generated scanner matches SQL column order; nested KeyAuth scan looks correctNo issues spotted. The “live” constraints are enforced at query level.
go/apps/api/routes/v2_apis_list_keys/handler.go (2)
112-117: LGTM: 404 masking for non-existent/wrong-workspace APIUsing cache.Null and workspace mismatch masked as 404 avoids information leakage. Good security posture.
Also applies to: 119-125
151-156: LGTM: Decrypt precondition checkEarly exit when StoreEncryptedKeys is false is correct and avoids unnecessary Vault calls.
Flo4604
left a comment
Flo4604
left a comment
There was a problem hiding this comment.
Added some comments otherwise lgtm
|
@Flo4604 we are all set |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go/internal/services/caches/caches.go (1)
65-67: Fix example usage: field name and Cache.Get signatureMinor doc polish: the example references a non-existent field and wrong Get signature. Prefer this:
-// key, err := caches.KeyByHash.Get(ctx, "some-hash") +// value, hit := caches.VerificationKeyByHash.Get(ctx, "some-hash") +// if hit == cache.Miss { +// // fetch from origin... +// }
♻️ Duplicate comments (4)
go/pkg/db/querier_generated.go (1)
1276-1367: Live keys list: pagination and deterministic JSON aggregates look sound; check DB indexes
- Pagination pattern (id >= cursor with limit+1 and handler dropping the sentinel) is aligned with repo conventions.
- ORDER BY in the roles/permissions subselects helps stabilize the JSON arrays coming from the DB.
For load, consider/verify indexes:
- keys(key_auth_id, workspace_id, id, deleted_at_m)
- keys_roles(key_id), roles_permissions(role_id), keys_permissions(key_id)
- ratelimits(key_id), ratelimits(identity_id)
These align with the join/filter clauses used by the aggregates.
go/apps/api/routes/v2_apis_list_keys/handler.go (3)
167-177: Use non-null KeyAuth ID to avoid NullString pitfalls; drop unnecessary nolintUse the embedded KeyAuth.ID from the cached API (guaranteed non-null via JOIN) instead of api.KeyAuthID.String. Also, OpenAPI caps limit<=100, so nolint:gosec is unnecessary.
Apply:
db.ListLiveKeysByKeyAuthIDParams{ - KeyAuthID: api.KeyAuthID.String, + KeyAuthID: api.KeyAuth.ID, WorkspaceID: auth.AuthorizedWorkspaceID, IDCursor: cursor, Identity: identityFilter, - Limit: int32(limit + 1), // nolint:gosec + Limit: int32(limit + 1), },
366-373: Deterministic ratelimits ordering (key and identity)Sort by Name then Id for stable outputs.
if len(keyRatelimits) > 0 { + sort.Slice(keyRatelimits, func(i, j int) bool { + if keyRatelimits[i].Name == keyRatelimits[j].Name { + return keyRatelimits[i].Id < keyRatelimits[j].Id + } + return keyRatelimits[i].Name < keyRatelimits[j].Name + }) response.Ratelimits = &keyRatelimits } - if len(identityRatelimits) > 0 { + if len(identityRatelimits) > 0 { + sort.Slice(identityRatelimits, func(i, j int) bool { + if identityRatelimits[i].Name == identityRatelimits[j].Name { + return identityRatelimits[i].Id < identityRatelimits[j].Id + } + return identityRatelimits[i].Name < identityRatelimits[j].Name + }) response.Identity.Ratelimits = &identityRatelimits }Note: add the “sort” import as in the previous comment.
208-214: Nit: pre-size plaintext mapReduce rehashing by pre-sizing to expected entries.
- plaintextMap := make(map[string]string) + plaintextMap := make(map[string]string, len(keyResults))
📜 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 (5)
go/apps/api/routes/v2_apis_list_keys/handler.go(4 hunks)go/internal/services/caches/caches.go(3 hunks)go/pkg/db/api_find_live_by_id.sql_generated.go(1 hunks)go/pkg/db/querier_generated.go(2 hunks)go/pkg/db/queries/api_find_live_by_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/api_find_live_by_id.sql_generated.gogo/internal/services/caches/caches.gogo/pkg/db/querier_generated.gogo/apps/api/routes/v2_apis_list_keys/handler.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/api_find_live_by_id.sql_generated.gogo/internal/services/caches/caches.gogo/pkg/db/querier_generated.gogo/apps/api/routes/v2_apis_list_keys/handler.go
🧠 Learnings (11)
📚 Learning: 2025-07-30T10:46:56.453Z
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.
Applied to files:
go/pkg/db/querier_generated.gogo/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-02-21T11:15:16.185Z
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.
Applied to files:
go/pkg/db/querier_generated.go
📚 Learning: 2025-08-14T16:26:55.283Z
Learnt from: imeyer
PR: unkeyed/unkey#3785
File: go/pkg/db/key_data.go:54-69
Timestamp: 2025-08-14T16:26:55.283Z
Learning: In the Unkey codebase, readability always trumps code optimization for line count. Struct literals with many fields should be formatted with one field per line for better readability, maintainability, and git diff clarity.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-07-15T14:25:05.608Z
Learnt from: chronark
PR: unkeyed/unkey#3560
File: go/apps/api/routes/v2_keys_create_key/handler.go:353-466
Timestamp: 2025-07-15T14:25:05.608Z
Learning: In the Unkey codebase, input validation for API endpoints is handled at the OpenAPI schema layer, which validates request fields like permission slugs (pattern: "^[a-zA-Z0-9_]+$", length: 1-100 characters) before requests reach the handler code. This validation occurs during the zen.BindBody call in handlers.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-03-19T09:25:59.751Z
Learnt from: Flo4604
PR: unkeyed/unkey#2955
File: go/apps/api/routes/v2_identities_create_identity/handler.go:162-202
Timestamp: 2025-03-19T09:25:59.751Z
Learning: In the Unkey codebase, input validation for API endpoints is primarily handled through OpenAPI schema validation, which occurs before requests reach the handler code. For example, in the identities.createIdentity endpoint, minimum values for ratelimit duration and limit are defined in the OpenAPI schema rather than duplicating these checks in the handler.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-08-19T09:42:40.919Z
Learnt from: Flo4604
PR: unkeyed/unkey#3800
File: go/internal/services/keys/validation.go:45-52
Timestamp: 2025-08-19T09:42:40.919Z
Learning: In go/internal/services/keys/validation.go, keys with unlimited usage (RemainingRequests.Valid = false) have an early return that bypasses the usage limiter entirely. The usage limiter is only called for keys with finite remaining request counts.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-08-20T14:21:42.382Z
Learnt from: Flo4604
PR: unkeyed/unkey#3809
File: go/apps/api/routes/v2_apis_list_keys/handler.go:92-95
Timestamp: 2025-08-20T14:21:42.382Z
Learning: In the Unkey codebase, the `revalidateKeysCache` flag in API requests is specifically intended for key caching, not API caching. Keys are not currently cached, so this flag can be safely ignored in API cache operations like ApiCache.SWR calls. The flag would only become relevant if/when key caching is implemented in the future.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-08-08T15:10:46.436Z
Learnt from: imeyer
PR: unkeyed/unkey#3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: For repo unkeyed/unkey and PR review workflows: When imeyer comments "issue" on a thread, automatically create a thorough GitHub issue (sections: Summary, Impact, Where, Repro/Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and the specific comment, and assign the issue to imeyer.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-08-08T15:10:46.436Z
Learnt from: imeyer
PR: unkeyed/unkey#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.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-08-08T16:10:00.224Z
Learnt from: imeyer
PR: unkeyed/unkey#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.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
📚 Learning: 2025-08-08T14:59:52.283Z
Learnt from: imeyer
PR: unkeyed/unkey#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.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.go
🧬 Code Graph Analysis (4)
go/pkg/db/api_find_live_by_id.sql_generated.go (2)
go/pkg/db/types/null_string.go (1)
NullString(10-10)go/pkg/db/models_generated.go (1)
NullApisAuthType(35-38)
go/internal/services/caches/caches.go (4)
go/pkg/cache/interface.go (1)
Cache(7-32)go/pkg/db/api_find_live_by_id.sql_generated.go (1)
FindLiveApiByIDRow(23-35)go/pkg/cache/cache.go (2)
New(55-99)Config(33-50)go/pkg/cache/middleware/tracing.go (1)
WithTracing(16-18)
go/pkg/db/querier_generated.go (2)
go/pkg/db/api_find_live_by_id.sql_generated.go (1)
FindLiveApiByIDRow(23-35)go/pkg/db/key_list_live_by_auth_id.sql_generated.go (2)
ListLiveKeysByKeyAuthIDParams(105-111)ListLiveKeysByKeyAuthIDRow(113-146)
go/apps/api/routes/v2_apis_list_keys/handler.go (10)
go/pkg/db/api_find_live_by_id.sql_generated.go (1)
FindLiveApiByIDRow(23-35)go/pkg/db/handle_err_no_rows.go (1)
IsNotFound(8-10)go/pkg/fault/wrap.go (3)
Wrap(25-67)Internal(75-89)Public(97-111)go/pkg/db/models_generated.go (5)
Api(583-594)KeyAuth(751-762)Identity(714-723)EncryptedKey(694-701)Key(725-749)go/pkg/cache/interface.go (1)
Null(42-42)go/internal/services/caches/caches.go (1)
New(67-109)go/pkg/ptr/deref.go (1)
SafeDeref(35-44)go/pkg/db/key_list_live_by_auth_id.sql_generated.go (1)
ListLiveKeysByKeyAuthIDParams(105-111)go/apps/api/openapi/gen.go (6)
Identity(141-153)Meta(259-262)KeyResponseData(196-230)KeyCreditsData(170-176)KeyCreditsRefill(179-190)RatelimitResponse(400-415)go/pkg/db/key_data.go (2)
ToKeyData(29-46)KeyData(9-21)
🪛 golangci-lint (2.2.2)
go/apps/api/routes/v2_apis_list_keys/handler.go
253-253: (*Handler).buildKeyResponseData - result 1 (error) is always nil
(unparam)
⏰ 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 Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
go/pkg/db/queries/api_find_live_by_id.sql (1)
1-9: LGTM: “Live API” query with explicit LIMIT 1 and soft-delete guardsThe query correctly joins key_auth, enforces both apis and key_auth not-deleted, and uses LIMIT 1 to make :one semantics explicit. This is exactly what we want for the LiveApi cache.
go/pkg/db/querier_generated.go (1)
326-335: LGTM: FindLiveApiByID added to Querier with correct “live” constraintsInterface surface and doc-comment mirror the SQL. This integrates cleanly with the LiveApiByID cache.
go/internal/services/caches/caches.go (2)
24-26: LGTM: Cache type and comment now match FindLiveApiByIDRowThe field’s doc matches the value type and purpose.
92-99: LGTM: LiveApiByID cache wiring and resource nameFresh/stale windows and resource label look good; naming follows Go initialism conventions (ID).
go/pkg/db/api_find_live_by_id.sql_generated.go (2)
13-21: LGTM: SQL constant matches query file; explicit column list aids stable scanningExplicit columns (rather than apis.*) in generated code minimize accidental scan-order breakage. LIMIT 1 included. Good.
46-72: Scan order and nested KeyAuth mapping look correctScan targets align with the SELECT list; QueryRowContext uses ctx. This should propagate sql.ErrNoRows for IsNotFound checks upstream.
go/apps/api/routes/v2_apis_list_keys/handler.go (6)
92-95: LGTM: ApiCache.SWR for live API fetch with null-hit handlingGood use of the LiveApiByID cache and DefaultFindFirstOp, including cache-null handling to avoid 200/empty responses on 404 cases.
119-125: Mask cross-workspace access with 404Correctly returns Data.Api.NotFound on workspace mismatches, preventing information leaks about API existence across tenants.
151-156: Correct decrypt precondition: require StoreEncryptedKeysGood guard that avoids misleading decrypt attempts when encryption isn’t enabled for the API.
187-193: LGTM: limit+1 pagination with cursor = last row IDYou compute hasMore, set nextCursor to the last row’s ID, then trim to limit. This matches the repo’s >=-cursor convention.
307-313: Good: treat identity meta JSON “null” as absentGating on identityMeta != nil avoids pointer-to-nil maps in the response. This matches prior guidance.
375-382: Good: treat key meta JSON “null” as absentSame as identity meta; avoiding pointer-to-nil improves JSON output semantics.
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (08/25/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
3 participants
What does this PR do?
This PR removes n+1 queries from
/v2/apis.listKeys/Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Refactor
Behavior Changes