refactor: /v2/getKey and /v2/whoami#3799
Conversation
|
📝 WalkthroughWalkthroughRefactors v2/keys.getKey and v2/keys.whoami to consume consolidated db.ToKeyData, centralizes decryption into a new decryptKey helper with permission and vault checks, consolidates response assembly (meta, identity, permissions, roles, ratelimits, credits/refill), adjusts imports/logging, and adds unit tests for multiple error scenarios including DB failures. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Handler
participant DB
participant Vault
Client->>API: GET /v2/keys.getKey or /v2/keys.whoami (Auth, decrypt flag)
API->>DB: single query -> find live key row
DB-->>API: row
API->>Handler: db.ToKeyData(row) -> keyData
Handler->>Handler: permission checks (read, decrypt) using keyData.Api.ID
alt decrypt requested and key is encrypted
Handler->>Vault: h.decryptKey(ctx, auth, keyData)
Vault-->>Handler: plaintext or error (error logged, call continues)
end
Handler-->>Client: 200 JSON { Meta: {RequestId}, Data: KeyResponseData }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Out-of-scope changes
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
|
I feel like we can make a common for both of these endpoints and move some of the repetitive logic there. Like, |
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: 4
🔭 Outside diff range comments (1)
go/apps/api/routes/v2_keys_get_key/handler.go (1)
28-35: Missing AIDEV comment for Handler structSimilar to the whoami handler, this Handler struct should have appropriate documentation according to the coding guidelines.
Add documentation for the Handler struct:
+// AIDEV-HANDLER: Handles v2/keys.getKey endpoint for retrieving comprehensive key information +// by keyId. Uses pre-fetched key data from FindLiveKeyByID query to assemble the complete +// response without redundant database lookups. Supports optional key decryption. // Handler implements zen.Route interface for the v2 keys.getKey endpoint type Handler struct {
♻️ Duplicate comments (6)
go/apps/api/routes/v2_keys_get_key/handler.go (6)
203-227: Identical helper implementation - needs deduplicationThis setCredits method is identical to the one in v2_keys_whoami/handler.go. This confirms the need for a shared implementation.
229-246: Identical helper implementation - needs deduplicationThis setIdentity method is identical to the one in v2_keys_whoami/handler.go.
248-282: Identical helper implementation - needs deduplicationThis setPermissions method is identical to the one in v2_keys_whoami/handler.go.
284-302: Identical helper implementation - needs deduplicationThis setRoles method is identical to the one in v2_keys_whoami/handler.go.
304-345: Identical helper implementation - needs deduplicationThis setRatelimits method is identical to the one in v2_keys_whoami/handler.go.
347-353: Identical helper implementation - needs deduplicationThis setMeta method is identical to the one in v2_keys_whoami/handler.go.
📜 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_keys_get_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_whoami/handler.go(2 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_keys_whoami/handler.gogo/apps/api/routes/v2_keys_get_key/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_keys_whoami/handler.gogo/apps/api/routes/v2_keys_get_key/handler.go
🧬 Code Graph Analysis (2)
go/apps/api/routes/v2_keys_whoami/handler.go (19)
go/apps/api/routes/v2_keys_get_key/handler.go (2)
Response(25-25)Handler(29-35)go/apps/api/routes/v2_keys_remove_roles/handler.go (2)
Response(24-24)Handler(27-33)go/apps/api/routes/v2_keys_add_roles/handler.go (2)
Response(25-25)Handler(27-33)go/apps/api/routes/v2_permissions_get_role/handler.go (2)
Response(20-20)Handler(23-28)go/apps/api/routes/v2_permissions_list_roles/handler.go (2)
Response(20-20)Handler(23-28)go/apps/api/routes/v2_keys_set_roles/handler.go (2)
Response(25-25)Handler(28-34)go/apps/api/routes/v2_keys_update_key/handler.go (2)
Response(29-29)Handler(32-38)go/apps/api/routes/v2_keys_delete_key/handler.go (2)
Response(25-25)Handler(28-34)go/apps/api/routes/v2_keys_add_permissions/handler.go (2)
Response(26-26)Handler(28-34)go/apps/api/routes/v2_keys_set_permissions/handler.go (2)
Response(26-26)Handler(29-35)go/apps/api/routes/v2_keys_update_credits/handler.go (2)
Response(25-25)Handler(28-34)go/apps/api/routes/v2_keys_remove_permissions/handler.go (2)
Response(22-22)Handler(25-32)go/apps/api/routes/v2_keys_verify_key/handler.go (2)
Response(25-25)Handler(30-36)go/apps/api/routes/v2_identities_list_identities/handler.go (2)
Response(21-21)Handler(24-29)go/apps/api/routes/v2_apis_list_keys/handler.go (2)
Response(25-25)Handler(29-34)go/apps/api/routes/v2_identities_update_identity/handler.go (2)
Response(27-27)Handler(31-37)go/apps/api/openapi/gen.go (8)
Meta(259-262)KeyResponseData(196-230)KeyCreditsData(170-176)Daily(16-16)Monthly(17-17)KeyCreditsRefill(179-190)Identity(141-153)RatelimitResponse(400-415)go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
FindLiveKeyByHashRow(104-140)go/pkg/ptr/pointer.go (1)
P(49-51)
go/apps/api/routes/v2_keys_get_key/handler.go (4)
go/pkg/db/models_generated.go (2)
KeyAuth(751-762)Identity(714-723)go/apps/api/openapi/gen.go (8)
Meta(259-262)KeyResponseData(196-230)KeyCreditsData(170-176)Daily(16-16)Monthly(17-17)KeyCreditsRefill(179-190)Identity(141-153)RatelimitResponse(400-415)go/pkg/db/key_find_live_by_id.sql_generated.go (1)
FindLiveKeyByIDRow(105-141)go/pkg/ptr/pointer.go (1)
P(49-51)
🪛 golangci-lint (2.2.2)
go/apps/api/routes/v2_keys_whoami/handler.go
201-201: Error return value is not checked
(errcheck)
⏰ 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 Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
🔇 Additional comments (5)
go/apps/api/routes/v2_keys_whoami/handler.go (4)
150-174: Well-structured credit refill logicGood implementation of the credit refill interval logic. The approach of deriving the interval type from the presence of RefillDay is clean and avoids redundant storage.
176-193: Excellent elimination of redundant DB callsThis change successfully addresses the PR objective by removing the separate identity lookup. The identity data is now derived directly from the key row fields, which improves performance as intended.
195-229: Effective permission consolidation logicThe approach of merging direct permissions and role permissions into a unified set using a map to eliminate duplicates is efficient and correct.
251-292: Smart ratelimit distribution logicThe logic correctly separates ratelimits between key-specific and identity-specific based on the associated IDs. This maintains proper data organization in the response.
go/apps/api/routes/v2_keys_get_key/handler.go (1)
132-137: Excellent simplification of encryption checkGreat improvement! The change from
keyAuth.StoreEncryptedKeystokey.KeyAuth.StoreEncryptedKeyssuccessfully eliminates the redundant database lookup for key auth information, directly addressing the PR's performance objectives.
|
We do could that yeah since both endpoints are the same except that for one we are looking at a hash and the other searches for an ID |
|
any examples in the repo similar to this? I don't wanna butcher the handlers 😄 |
imeyer
left a comment
imeyer
left a comment
There was a problem hiding this comment.
I think the helper functions should return error, and the functions themselves should return from an error, not just log and have it be implicit.
There's a good opportunity to write a test for the handler too, as it doesn't have any.
|
I don't think so since most of the handles are completely different There is |
|
shit even better 🫡 |
|
@imeyer both handlers have tests? |
|
I'll clean up these first thing on Monday and then we merge 🫡 |
|
There's no Concrete example? We have a test for the first condition, but not the second.. which is also important to know that when there is a database error, we do the right thing (and the user sees the same as well) |
|
Hmm empty key and malformed payloads will be caught by openapi validation, but I see what you mean |
|
yes empty key / malformed payloads are and should be in the 400_test.go as they are catched by the openapi validation |
|
but that's correct for the second path when the db fails, we should add that |
|
Deal. I can add that that and any other missing case |
|
I reverted that |
|
what prefix? |
|
:ty: |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (11)
go/apps/api/routes/v2_keys_get_key/500_test.go (1)
16-16: Name the test and subtest explicitly (endpoint + failure mode)Make reports readable at a glance.
-func TestInternalError(t *testing.T) { +func TestGetKey_InternalServerError_DBClosed(t *testing.T) { @@ - t.Run("database connection closed during request", func(t *testing.T) { + t.Run("db closed yields internal server error", func(t *testing.T) {Also applies to: 32-32
go/apps/api/routes/v2_keys_whoami/500_test.go (2)
14-14: Name the test and subtest explicitly (endpoint + failure mode)Aligns with conventions and improves scanability.
-func TestInternalError(t *testing.T) { +func TestWhoami_InternalServerError_DBClosed(t *testing.T) { @@ - t.Run("database connection closed during request", func(t *testing.T) { + t.Run("db closed yields internal server error", func(t *testing.T) {Also applies to: 31-31
40-44: Fix expected public error message for DB failureThis handler also returns fault.Public("Failed to retrieve Key information.") for DB errors.
- require.Contains(t, res.Body.Error.Detail, "We could not load the requested key") + require.Contains(t, res.Body.Error.Detail, "Failed to retrieve Key information.")go/apps/api/routes/v2_keys_whoami/handler.go (4)
29-36: Add AIDEV doc comment to HandlerPublic types require documentation and important flows should include an AIDEV-* note. Add a concise description and intent.
-// Handler implements zen.Route interface for the v2 keys.whoami endpoint +// Handler implements zen.Route for the v2 keys.whoami endpoint. +// AIDEV-HANDLER: Builds KeyResponseData from a single consolidated DB row via db.ToKeyData, +// avoiding redundant queries and assembling identity, permissions, roles, ratelimits, and meta. type Handler struct {
31-36: Vault dependency appears unusedIf whoami never decrypts, drop Vault from the Handler to reduce surface area; otherwise, document why it’s present.
type Handler struct { Logger logging.Logger DB db.Database Keys keys.KeyService Auditlogs auditlogs.AuditLogService - Vault *vault.Service + // Vault intentionally unused in whoami; reserved for future sensitive flows or kept for constructor parity. + Vault *vault.Service }
109-115: Suppress exhaustruct for compact KeyResponseData literalAvoid noisy lints for intentionally omitted optional fields.
- response := openapi.KeyResponseData{ + response := openapi.KeyResponseData{ //nolint:exhaustruct // optional fields are set conditionally below CreatedAt: keyData.Key.CreatedAtM, Enabled: keyData.Key.Enabled, KeyId: keyData.Key.ID, Start: keyData.Key.Start, Plaintext: nil, }
77-81: Defensive nil-guard after ToKeyDataWhile ToKeyData should return a non-nil value for this row type, guard against future changes to avoid nil deref.
- keyData := db.ToKeyData(key) + keyData := db.ToKeyData(key) + if keyData == nil { + return fault.New("unexpected nil key data", + fault.Code(codes.App.Internal.UnexpectedError.URN()), + fault.Internal("ToKeyData returned nil for FindLiveKeyByHashRow"), + fault.Public("Failed to retrieve Key information."), + ) + }go/apps/api/routes/v2_keys_get_key/handler.go (4)
256-256: Add AIDEV note above decryptKeyThis is a sensitive flow; document intent and non-fatal semantics.
-func (h *Handler) decryptKey(ctx context.Context, auth *keys.KeyVerifier, keyData *db.KeyData) (*string, error) { +// AIDEV-DECRYPT: RBAC-gated decryption; absent encryption or decrypt errors return no plaintext (non-fatal). +func (h *Handler) decryptKey(ctx context.Context, auth *keys.KeyVerifier, keyData *db.KeyData) (*string, error) {
80-81: Defensive nil-guard after ToKeyDataMirror the whoami guard to prevent nil deref if ToKeyData ever changes.
- keyData := db.ToKeyData(key) + keyData := db.ToKeyData(key) + if keyData == nil { + return fault.New("unexpected nil key data", + fault.Code(codes.App.Internal.UnexpectedError.URN()), + fault.Internal("ToKeyData returned nil for FindLiveKeyByIDRow"), + fault.Public("Failed to retrieve Key information."), + ) + }
241-246: Instrument JSON unmarshal failuresConsider incrementing a metric to monitor frequency of meta parse errors.
- if err := json.Unmarshal([]byte(keyData.Key.Meta.String), &meta); err != nil { - h.Logger.Error("failed to unmarshal key meta", "error", err) + if err := json.Unmarshal([]byte(keyData.Key.Meta.String), &meta); err != nil { + h.Logger.Error("failed to unmarshal key meta", "error", err) + // AIDEV-METRICS: increment counter for key meta unmarshal failures (labels: route=get_key, keyId) } else { response.Meta = &meta }
118-124: Suppress exhaustruct for compact KeyResponseData literalKeep the literal concise without tripping the linter.
- response := openapi.KeyResponseData{ + response := openapi.KeyResponseData{ //nolint:exhaustruct // optional fields are set conditionally below CreatedAt: keyData.Key.CreatedAtM, Enabled: keyData.Key.Enabled, KeyId: keyData.Key.ID, Start: keyData.Key.Start, Plaintext: plaintext, }
📜 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 (6)
go/apps/api/routes/v2_keys_get_key/500_test.go(1 hunks)go/apps/api/routes/v2_keys_get_key/handler.go(3 hunks)go/apps/api/routes/v2_keys_whoami/500_test.go(1 hunks)go/apps/api/routes/v2_keys_whoami/handler.go(5 hunks)go/pkg/db/key_data_test.go(1 hunks)go/pkg/zen/auth.go(0 hunks)
💤 Files with no reviewable changes (1)
- go/pkg/zen/auth.go
🧰 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_keys_get_key/500_test.gogo/pkg/db/key_data_test.gogo/apps/api/routes/v2_keys_whoami/handler.gogo/apps/api/routes/v2_keys_get_key/handler.gogo/apps/api/routes/v2_keys_whoami/500_test.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_keys_get_key/500_test.gogo/pkg/db/key_data_test.gogo/apps/api/routes/v2_keys_whoami/500_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_keys_get_key/500_test.gogo/pkg/db/key_data_test.gogo/apps/api/routes/v2_keys_whoami/handler.gogo/apps/api/routes/v2_keys_get_key/handler.gogo/apps/api/routes/v2_keys_whoami/500_test.go
🧠 Learnings (3)
📚 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_keys_get_key/500_test.gogo/pkg/db/key_data_test.gogo/apps/api/routes/v2_keys_whoami/500_test.go
📚 Learning: 2025-08-04T07:44:39.438Z
Learnt from: CR
PR: unkeyed/unkey#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T07:44:39.438Z
Learning: Applies to **/*.go : Use AIDEV-* comments for complex/important code in Go services
Applied to files:
go/apps/api/routes/v2_keys_whoami/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_keys_get_key/handler.go
🧬 Code Graph Analysis (5)
go/apps/api/routes/v2_keys_get_key/500_test.go (5)
go/pkg/testutil/http.go (2)
NewHarness(52-175)CallRoute(257-291)go/apps/api/routes/v2_keys_get_key/handler.go (2)
Handler(30-36)Request(25-25)go/pkg/uid/uid.go (1)
KeyPrefix(16-16)go/pkg/ptr/pointer.go (1)
P(49-51)go/apps/api/openapi/gen.go (1)
InternalServerErrorResponse(161-167)
go/pkg/db/key_data_test.go (3)
go/pkg/db/key_find_live_by_id.sql_generated.go (1)
FindLiveKeyByIDRow(105-141)go/pkg/db/key_data.go (1)
ToKeyData(32-45)go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
FindLiveKeyByHashRow(104-140)
go/apps/api/routes/v2_keys_whoami/handler.go (3)
go/pkg/db/key_data.go (1)
ToKeyData(32-45)go/apps/api/openapi/gen.go (6)
KeyResponseData(196-230)KeyCreditsData(170-176)KeyCreditsRefill(179-190)Identity(141-153)Meta(259-262)RatelimitResponse(400-415)go/pkg/ptr/pointer.go (1)
P(49-51)
go/apps/api/routes/v2_keys_get_key/handler.go (8)
go/pkg/db/key_data.go (2)
ToKeyData(32-45)KeyData(9-24)go/pkg/db/models_generated.go (5)
Key(725-749)Api(583-594)Identity(714-723)KeyAuth(751-762)EncryptedKey(694-701)go/pkg/rbac/permissions.go (4)
ReadKey(64-64)Limit(74-74)Tuple(175-184)DecryptKey(61-61)go/pkg/ptr/deref.go (1)
SafeDeref(35-44)go/apps/api/openapi/gen.go (6)
KeyResponseData(196-230)KeyCreditsData(170-176)KeyCreditsRefill(179-190)Identity(141-153)Meta(259-262)RatelimitResponse(400-415)go/pkg/ptr/pointer.go (1)
P(49-51)go/internal/services/keys/verifier.go (1)
KeyVerifier(32-56)go/pkg/rbac/query.go (2)
Or(64-70)T(84-90)
go/apps/api/routes/v2_keys_whoami/500_test.go (3)
go/pkg/testutil/http.go (2)
NewHarness(52-175)CallRoute(257-291)go/apps/api/routes/v2_keys_whoami/handler.go (2)
Handler(30-36)Request(25-25)go/apps/api/openapi/gen.go (1)
InternalServerErrorResponse(161-167)
🪛 golangci-lint (2.2.2)
go/apps/api/routes/v2_keys_get_key/handler.go
118-118: openapi.KeyResponseData is missing fields Credits, Expires, Identity, Meta, Name, Permissions, Ratelimits, Roles, UpdatedAt
(exhaustruct)
141-141: openapi.KeyCreditsData is missing field Refill
(exhaustruct)
163-163: openapi.Identity is missing fields Meta, Ratelimits
(exhaustruct)
291-291: return both a nil error and an invalid value: use a sentinel error instead
(nilnil)
303-303: return both a nil error and an invalid value: use a sentinel error instead
(nilnil)
⏰ 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 (5)
go/apps/api/routes/v2_keys_whoami/handler.go (2)
169-184: Deterministic permissions ordering — niceSorting slugs ensures stable API output and non-flaky tests.
209-217: Good: nil-safe identity ratelimit mappingThe explicit keyData.Identity != nil guard avoids unexpected panics.
go/apps/api/routes/v2_keys_get_key/handler.go (3)
191-193: Deterministic permissions ordering — goodSorting map keys stabilizes API output and tests.
223-226: Nil-safe identity ratelimit mapping — goodThe added keyData.Identity != nil guard prevents panics.
91-106: Consistency: Hide existence on permission failure?Whoami wraps permission failures as NotFound to avoid leaking key existence. Here you return the raw error from VerifyRootKey (likely 403). Decide whether to align both endpoints for consistency.
If you choose to hide existence, wrap the error as in whoami:
- if err != nil { - return err - } + if err != nil { + return fault.Wrap(err, + fault.Code(codes.Data.Key.NotFound.URN()), + fault.Internal("user doesn't have permissions and we don't want to leak the existence of the key"), + fault.Public("The specified key was not found."), + ) + }
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
go/pkg/db/key_data_test.go (3)
151-201: Nice — positive coverage for ratelimits JSON decoding is in placeThis fills the earlier gap and verifies proper decoding and field types for ratelimits. Good assertions on ID, Name, Limit (int32), Duration (int64), and AutoApply.
37-37: Pointer subtests aren’t exercising the pointer code pathBoth “pointer” cases still pass the value, so you never hit the
*FindLiveKeyBy...branches inToKeyData. Pass&rowto actually cover those switch cases.- result := ToKeyData(row) + result := ToKeyData(&row) @@ - result := ToKeyData(row) + result := ToKeyData(&row)Also applies to: 71-71
11-79: Adopt table-driven tests per repo guidelinesThese subtests are repetitive and ideal for consolidation into a table-driven suite (value vs pointer, ID vs Hash). This aligns with the repository’s guidance to use table-driven tests in Go.
Example sketch:
func TestToKeyData_KeyBasics(t *testing.T) { cases := []struct{ name string row any wantID, wantHash, wantWS string wantEnabled bool }{ {"IDRow value", FindLiveKeyByIDRow{ID: "id1", Hash: "h1", WorkspaceID: "ws1", Enabled: true}, "id1", "h1", "ws1", true}, {"IDRow pointer", &FindLiveKeyByIDRow{ID: "id2", Hash: "h2", WorkspaceID: "ws2"}, "id2", "h2", "ws2", false}, {"HashRow value", FindLiveKeyByHashRow{ID: "id3", Hash: "h3", WorkspaceID: "ws3", Enabled: true}, "id3", "h3", "ws3", true}, {"HashRow pointer", &FindLiveKeyByHashRow{ID: "id4", Hash: "h4", WorkspaceID: "ws4"}, "id4", "h4", "ws4", false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { res := ToKeyData(tc.row) require.NotNil(t, res) require.Equal(t, tc.wantID, res.Key.ID) require.Equal(t, tc.wantHash, res.Key.Hash) require.Equal(t, tc.wantWS, res.Key.WorkspaceID) require.Equal(t, tc.wantEnabled, res.Key.Enabled) }) } }
📜 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 (1)
go/pkg/db/key_data_test.go(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/key_data_test.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/pkg/db/key_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/pkg/db/key_data_test.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/pkg/db/key_data_test.go
📚 Learning: 2025-08-04T07:44:39.438Z
Learnt from: CR
PR: unkeyed/unkey#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T07:44:39.438Z
Learning: Applies to **/*_test.go : Use table-driven tests in Go
Applied to files:
go/pkg/db/key_data_test.go
🧬 Code Graph Analysis (1)
go/pkg/db/key_data_test.go (4)
go/pkg/db/key_find_live_by_id.sql_generated.go (1)
FindLiveKeyByIDRow(105-141)go/pkg/db/key_data.go (1)
ToKeyData(32-45)go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
FindLiveKeyByHashRow(104-140)go/pkg/db/custom_types.go (1)
RatelimitInfo(23-31)
⏰ 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). (5)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
Flo4604
left a comment
Flo4604
left a comment
There was a problem hiding this comment.
Just those small things otherwise lgtm
|
@Flo4604 boss we are ready |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
go/apps/api/routes/v2_keys_whoami/handler.go (2)
46-51: Guard defer on potential nil emit to avoid panicsIf GetRootKey ever returns a nil emit function on error, deferring it will panic. Guard before deferring.
- auth, emit, err := h.Keys.GetRootKey(ctx, s) - defer emit() + auth, emit, err := h.Keys.GetRootKey(ctx, s) + if emit != nil { // defensive: avoid deferring a nil function + defer emit() + } if err != nil { return err }Please confirm GetRootKey guarantees a non-nil emit even on error; if so, this change is optional.
46-46: Add debug log for parity with getKeygetKey logs request start; mirror it here for consistency and easier tracing.
func (h *Handler) Handle(ctx context.Context, s *zen.Session) error { + h.Logger.Debug("handling request", "requestId", s.RequestID(), "path", "/v2/keys.whoami") auth, emit, err := h.Keys.GetRootKey(ctx, s)go/apps/api/routes/v2_keys_get_key/handler.go (3)
29-36: Add AIDEV annotation and clarify Handler docPublic types require docs; also mark this as important code.
-// Handler implements zen.Route interface for the v2 keys.getKey endpoint +// Handler implements the zen.Route for the v2 keys.getKey endpoint and assembles a complete key profile +// from a single DB query (db.ToKeyData), optionally decrypting the key when requested and permitted. +// AIDEV-HANDLER: Critical route; keep parity with whoami and avoid extra DB calls; sensitive decryption flow. type Handler struct {
38-44: Document exported methods (Method, Path, Handle)Add GoDoc comments for exported methods per guidelines.
+// Method implements zen.Route.Method for the getKey route. func (h *Handler) Method() string { return "POST" } +// Path implements zen.Route.Path for the getKey route. func (h *Handler) Path() string { return "/v2/keys.getKey" } +// Handle fetches the key by ID, performs RBAC checks, optional decryption, and returns KeyResponseData. func (h *Handler) Handle(ctx context.Context, s *zen.Session) error {Also applies to: 46-46
50-55: Guard defer on potential nil emit to avoid panicsDefensively guard the deferred emit.
auth, emit, err := h.Keys.GetRootKey(ctx, s) - defer emit() + if emit != nil { // defensive: avoid deferring a nil function + defer emit() + } if err != nil { return err }Please confirm GetRootKey always returns a non-nil emit even when err != nil.
♻️ Duplicate comments (7)
go/apps/api/routes/v2_keys_whoami/handler.go (4)
77-79: Defensive guard: ToKeyData nil checkWhile ToKeyData should return non-nil for FindLiveKeyByHashRow, guard against future changes to avoid panics when dereferencing keyData.
- keyData := db.ToKeyData(key) + keyData := db.ToKeyData(key) + if keyData == nil { + return fault.New("unexpected nil key data", + fault.Code(codes.App.Internal.UnexpectedError.URN()), + fault.Internal("ToKeyData returned nil for FindLiveKeyByHashRow"), + fault.Public("Failed to retrieve Key information."), + ) + }
29-36: Add missing AIDEV annotation and expand doc for public HandlerPublic types must be documented per guidelines. Add an AIDEV-* note to flag this as important code.
-// Handler implements zen.Route interface for the v2 keys.whoami endpoint +// Handler implements the zen.Route for the v2 keys.whoami endpoint and assembles a complete key profile +// from a single DB query (db.ToKeyData) without extra lookups. +// AIDEV-HANDLER: Important route in the critical path; keep behavior consistent with getKey and avoid extra DB calls. type Handler struct {
31-36: Clarify unused Vault dependency or remove itVault isn’t used in whoami. Either remove it or document why it exists to avoid confusion.
type Handler struct { Logger logging.Logger DB db.Database Keys keys.KeyService Auditlogs auditlogs.AuditLogService - Vault *vault.Service + // Vault is intentionally unused in whoami; kept for parity with getKey/decryption flows and potential future needs. + Vault *vault.Service }
109-115: Silence exhaustruct lints on partial literalsStruct literals intentionally omit optional fields. Add nolint markers to avoid noisy lints.
- response := openapi.KeyResponseData{ + response := openapi.KeyResponseData{ //nolint:exhaustruct // optional fields are set conditionally below CreatedAt: keyData.Key.CreatedAtM, Enabled: keyData.Key.Enabled, KeyId: keyData.Key.ID, Start: keyData.Key.Start, Plaintext: nil, } @@ - response.Credits = &openapi.KeyCreditsData{ + response.Credits = &openapi.KeyCreditsData{ //nolint:exhaustruct // Refill set conditionally Remaining: nullable.NewNullableWithValue(int64(keyData.Key.RemainingRequests.Int32)), } @@ - response.Identity = &openapi.Identity{ + response.Identity = &openapi.Identity{ //nolint:exhaustruct // optional fields added conditionally below Id: keyData.Identity.ID, ExternalId: keyData.Identity.ExternalID, }Also applies to: 132-134, 154-157
go/apps/api/routes/v2_keys_get_key/handler.go (3)
118-124: Silence exhaustruct lints on partial literalsAdd nolint markers where optional fields are set conditionally.
- response := openapi.KeyResponseData{ + response := openapi.KeyResponseData{ //nolint:exhaustruct // optional fields set conditionally below CreatedAt: keyData.Key.CreatedAtM, Enabled: keyData.Key.Enabled, KeyId: keyData.Key.ID, Start: keyData.Key.Start, Plaintext: plaintext, } @@ - response.Credits = &openapi.KeyCreditsData{ + response.Credits = &openapi.KeyCreditsData{ //nolint:exhaustruct // Refill set conditionally Remaining: nullable.NewNullableWithValue(int64(keyData.Key.RemainingRequests.Int32)), } @@ - response.Identity = &openapi.Identity{ + response.Identity = &openapi.Identity{ //nolint:exhaustruct // optional fields set conditionally below Id: keyData.Identity.ID, ExternalId: keyData.Identity.ExternalID, }Also applies to: 141-143, 163-166
80-82: Defensive guard: ToKeyData nil checkAvoid potential panics on future changes by guarding keyData before use.
- keyData := db.ToKeyData(key) + keyData := db.ToKeyData(key) + if keyData == nil { + return fault.New("unexpected nil key data", + fault.Code(codes.App.Internal.UnexpectedError.URN()), + fault.Internal("ToKeyData returned nil for FindLiveKeyByIDRow"), + fault.Public("Failed to retrieve Key information."), + ) + }
256-307: Document sensitive decryption flow and silence nilnil lintsThis path is security-sensitive with intentional (nil, nil) returns. Add an AIDEV comment and suppress nilnil lints.
-func (h *Handler) decryptKey(ctx context.Context, auth *keys.KeyVerifier, keyData *db.KeyData) (*string, error) { +// AIDEV-DECRYPT: RBAC-gated decryption; if encryption is not configured or decryption fails, return no plaintext +// without failing the request. Errors are logged; callers proceed with plaintext=nil. +func (h *Handler) decryptKey(ctx context.Context, auth *keys.KeyVerifier, keyData *db.KeyData) (*string, error) { @@ - if !keyData.EncryptedKey.Valid || !keyData.EncryptionKeyID.Valid { - return nil, nil + if !keyData.EncryptedKey.Valid || !keyData.EncryptionKeyID.Valid { + return nil, nil //nolint:nilnil // no plaintext available and not an error } @@ - if err != nil { + if err != nil { h.Logger.Error("failed to decrypt key", "keyId", keyData.Key.ID, "error", err, ) - return nil, nil // Return nil instead of failing the entire request + return nil, nil //nolint:nilnil // decryption failed; proceed without plaintext }
📜 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_keys_get_key/handler.go(3 hunks)go/apps/api/routes/v2_keys_whoami/handler.go(5 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_keys_get_key/handler.gogo/apps/api/routes/v2_keys_whoami/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_keys_get_key/handler.gogo/apps/api/routes/v2_keys_whoami/handler.go
🧠 Learnings (3)
📚 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_keys_get_key/handler.go
📚 Learning: 2025-08-20T11:41:36.683Z
Learnt from: Flo4604
PR: unkeyed/unkey#3810
File: go/apps/api/routes/v2_ratelimit_limit/handler.go:54-56
Timestamp: 2025-08-20T11:41:36.683Z
Learning: In the Unkey codebase, the X-Unkey-Metrics: disabled header is used by the v1 API when replaying ratelimit requests to the v2 API (go/apps/api/routes/v2_ratelimit_limit/handler.go) to prevent double billing/logging to ClickHouse. This is a legitimate internal service-to-service communication pattern for API migration scenarios, not intended for external client usage.
Applied to files:
go/apps/api/routes/v2_keys_get_key/handler.go
📚 Learning: 2025-08-04T07:44:39.438Z
Learnt from: CR
PR: unkeyed/unkey#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T07:44:39.438Z
Learning: Applies to **/*.go : Use AIDEV-* comments for complex/important code in Go services
Applied to files:
go/apps/api/routes/v2_keys_whoami/handler.go
🧬 Code Graph Analysis (2)
go/apps/api/routes/v2_keys_get_key/handler.go (7)
go/pkg/db/key_data.go (2)
ToKeyData(32-45)KeyData(9-24)go/pkg/db/models_generated.go (5)
Key(725-749)Api(583-594)Identity(714-723)KeyAuth(751-762)EncryptedKey(694-701)go/pkg/rbac/permissions.go (4)
ReadKey(64-64)Limit(74-74)Tuple(175-184)DecryptKey(61-61)go/pkg/ptr/deref.go (1)
SafeDeref(35-44)go/apps/api/openapi/gen.go (5)
KeyResponseData(196-230)KeyCreditsData(170-176)Identity(141-153)Meta(259-262)RatelimitResponse(400-415)go/internal/services/keys/options.go (1)
WithPermissions(47-52)go/pkg/rbac/query.go (2)
Or(64-70)T(84-90)
go/apps/api/routes/v2_keys_whoami/handler.go (3)
go/pkg/db/key_data.go (1)
ToKeyData(32-45)go/apps/api/openapi/gen.go (6)
KeyResponseData(196-230)KeyCreditsData(170-176)KeyCreditsRefill(179-190)Identity(141-153)Meta(259-262)RatelimitResponse(400-415)go/pkg/ptr/pointer.go (1)
P(49-51)
🪛 golangci-lint (2.2.2)
go/apps/api/routes/v2_keys_get_key/handler.go
118-118: openapi.KeyResponseData is missing fields Credits, Expires, Identity, Meta, Name, Permissions, Ratelimits, Roles, UpdatedAt
(exhaustruct)
141-141: openapi.KeyCreditsData is missing field Refill
(exhaustruct)
163-163: openapi.Identity is missing fields Meta, Ratelimits
(exhaustruct)
291-291: return both a nil error and an invalid value: use a sentinel error instead
(nilnil)
303-303: return both a nil error and an invalid value: use a sentinel error instead
(nilnil)
⏰ 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: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
go/apps/api/routes/v2_keys_whoami/handler.go (1)
169-184: Good: deterministic permissions orderingSorting the permissions slugs avoids flaky responses and tests. Nice.
go/apps/api/routes/v2_keys_get_key/handler.go (2)
104-106: Verify permission-error semantics (leak prevention)whoami returns NotFound on permission failure to avoid leaking existence; getKey returns the raw error. Confirm the intended behavior and align both routes if needed.
Would you like to wrap the permission error here with a NotFound fault (same as whoami) to avoid existence leaks?
178-193: Good: deterministic permissions orderingSorting permission slugs gives stable API output. Nice.
| response := openapi.KeyResponseData{ | ||
| CreatedAt: keyData.Key.CreatedAtM, | ||
| Enabled: keyData.Key.Enabled, | ||
| KeyId: keyData.Key.ID, | ||
| Start: keyData.Key.Start, | ||
| Plaintext: plaintext, | ||
| } | ||
|
|
||
| if key.Name.Valid { | ||
| k.Name = ptr.P(key.Name.String) | ||
| // Set optional fields | ||
| if keyData.Key.Name.Valid { | ||
| response.Name = ptr.P(keyData.Key.Name.String) | ||
| } | ||
|
|
||
| if key.UpdatedAtM.Valid { | ||
| k.UpdatedAt = ptr.P(key.UpdatedAtM.Int64) | ||
| if keyData.Key.UpdatedAtM.Valid { | ||
| response.UpdatedAt = ptr.P(keyData.Key.UpdatedAtM.Int64) | ||
| } | ||
|
|
||
| if key.Expires.Valid { | ||
| k.Expires = ptr.P(key.Expires.Time.UnixMilli()) | ||
| if keyData.Key.Expires.Valid { | ||
| response.Expires = ptr.P(keyData.Key.Expires.Time.UnixMilli()) | ||
| } | ||
|
|
||
| if key.RemainingRequests.Valid { | ||
| k.Credits = &openapi.KeyCreditsData{ | ||
| Remaining: nullable.NewNullableWithValue(int64(key.RemainingRequests.Int32)), | ||
| Refill: nil, | ||
| // Set credits | ||
| if keyData.Key.RemainingRequests.Valid { | ||
| response.Credits = &openapi.KeyCreditsData{ | ||
| Remaining: nullable.NewNullableWithValue(int64(keyData.Key.RemainingRequests.Int32)), | ||
| } | ||
|
|
||
| if key.RefillAmount.Valid { | ||
| if keyData.Key.RefillAmount.Valid { | ||
| var refillDay *int | ||
| interval := openapi.Daily | ||
| if key.RefillDay.Valid { | ||
| if keyData.Key.RefillDay.Valid { | ||
| interval = openapi.Monthly | ||
| refillDay = ptr.P(int(key.RefillDay.Int16)) | ||
| refillDay = ptr.P(int(keyData.Key.RefillDay.Int16)) | ||
| } | ||
|
|
||
| k.Credits.Refill = &openapi.KeyCreditsRefill{ | ||
| Amount: int64(key.RefillAmount.Int32), | ||
| response.Credits.Refill = &openapi.KeyCreditsRefill{ | ||
| Amount: int64(keyData.Key.RefillAmount.Int32), | ||
| Interval: interval, | ||
| RefillDay: refillDay, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if key.IdentityID.Valid { | ||
| identity, idErr := db.Query.FindIdentity(ctx, h.DB.RO(), db.FindIdentityParams{ | ||
| Identity: key.IdentityID.String, | ||
| Deleted: false, | ||
| WorkspaceID: auth.AuthorizedWorkspaceID, | ||
| }) | ||
| if idErr != nil { | ||
| if db.IsNotFound(idErr) { | ||
| return fault.New("identity not found for key", | ||
| fault.Code(codes.Data.Identity.NotFound.URN()), | ||
| fault.Internal("identity not found"), | ||
| fault.Public("The requested identity does not exist or has been deleted."), | ||
| ) | ||
| // Set identity | ||
| if keyData.Identity != nil { | ||
| response.Identity = &openapi.Identity{ | ||
| Id: keyData.Identity.ID, | ||
| ExternalId: keyData.Identity.ExternalID, | ||
| } | ||
|
|
||
| if len(keyData.Identity.Meta) > 0 { | ||
| var identityMeta map[string]any | ||
| if err := json.Unmarshal(keyData.Identity.Meta, &identityMeta); err != nil { | ||
| h.Logger.Error("failed to unmarshal identity meta", "error", err) | ||
| } else { | ||
| response.Identity.Meta = &identityMeta | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return fault.Wrap(idErr, | ||
| fault.Code(codes.App.Internal.ServiceUnavailable.URN()), | ||
| fault.Internal("database error"), | ||
| fault.Public("Failed to retrieve Identity information."), | ||
| ) | ||
| // Set permissions, combine direct + role permissions | ||
| permissionSlugs := make(map[string]struct{}) | ||
| for _, p := range keyData.Permissions { | ||
| permissionSlugs[p.Slug] = struct{}{} | ||
| } | ||
| for _, p := range keyData.RolePermissions { | ||
| permissionSlugs[p.Slug] = struct{}{} | ||
| } | ||
| if len(permissionSlugs) > 0 { | ||
| slugs := make([]string, 0, len(permissionSlugs)) | ||
| for slug := range permissionSlugs { | ||
| slugs = append(slugs, slug) | ||
| } | ||
| sort.Strings(slugs) | ||
| response.Permissions = &slugs | ||
| } | ||
|
|
||
| k.Identity = &openapi.Identity{ | ||
| Id: identity.ID, | ||
| ExternalId: identity.ExternalID, | ||
| Meta: nil, | ||
| Ratelimits: nil, | ||
| // Set roles | ||
| if len(keyData.Roles) > 0 { | ||
| roleNames := make([]string, len(keyData.Roles)) | ||
| for i, role := range keyData.Roles { | ||
| roleNames[i] = role.Name | ||
| } | ||
| response.Roles = &roleNames | ||
| } | ||
|
|
||
| if len(identity.Meta) > 0 { | ||
| err = json.Unmarshal(identity.Meta, &k.Identity.Meta) | ||
| if err != nil { | ||
| return fault.Wrap(err, fault.Code(codes.App.Internal.UnexpectedError.URN()), | ||
| fault.Internal("unable to unmarshal identity meta"), | ||
| fault.Public("We encountered an error while trying to unmarshal the identity meta data."), | ||
| ) | ||
| // Set ratelimits | ||
| if len(keyData.Ratelimits) > 0 { | ||
| var keyRatelimits []openapi.RatelimitResponse |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider extracting shared response builder to remove duplication with whoami
The response assembly (credits, identity, permissions, roles, ratelimits, meta) duplicates whoami. A shared builder (e.g., internal/keyresponse.Builder) would reduce maintenance cost.
I can draft a minimal Builder with BuildFromLiveKeyByID/BuildFromLiveKeyByHash and port the setter logic there if you want.
🧰 Tools
🪛 golangci-lint (2.2.2)
118-118: openapi.KeyResponseData is missing fields Credits, Expires, Identity, Meta, Name, Permissions, Ratelimits, Roles, UpdatedAt
(exhaustruct)
141-141: openapi.KeyCreditsData is missing field Refill
(exhaustruct)
163-163: openapi.Identity is missing fields Meta, Ratelimits
(exhaustruct)
| if len(keyData.Identity.Meta) > 0 { | ||
| var identityMeta map[string]any | ||
| if err := json.Unmarshal(keyData.Identity.Meta, &identityMeta); err != nil { | ||
| h.Logger.Error("failed to unmarshal identity meta", "error", err) | ||
| } else { | ||
| response.Identity.Meta = &identityMeta | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Instrument JSON unmarshal failures for observability
Add metric increments alongside logs to track data-quality issues.
if len(keyData.Identity.Meta) > 0 {
var identityMeta map[string]any
if err := json.Unmarshal(keyData.Identity.Meta, &identityMeta); err != nil {
h.Logger.Error("failed to unmarshal identity meta", "error", err)
+ // AIDEV-METRICS: increment counter for identity meta unmarshal failures (route=getKey, keyId=keyData.Key.ID)
} else {
response.Identity.Meta = &identityMeta
}
}
@@
if keyData.Key.Meta.Valid {
var meta map[string]any
if err := json.Unmarshal([]byte(keyData.Key.Meta.String), &meta); err != nil {
h.Logger.Error("failed to unmarshal key meta", "error", err)
+ // AIDEV-METRICS: increment counter for key meta unmarshal failures (route=getKey, keyId=keyData.Key.ID)
} else {
response.Meta = &meta
}
}Also applies to: 239-246
| if len(identityRatelimits) > 0 { | ||
| k.Identity.Ratelimits = ptr.P(identityRatelimits) | ||
| response.Identity.Ratelimits = &identityRatelimits | ||
| } |
There was a problem hiding this comment.
Avoid potential nil dereference when assigning identity ratelimits
Guard against response.Identity being nil.
- if len(identityRatelimits) > 0 {
- response.Identity.Ratelimits = &identityRatelimits
- }
+ if len(identityRatelimits) > 0 && response.Identity != nil {
+ response.Identity.Ratelimits = &identityRatelimits
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(identityRatelimits) > 0 { | |
| k.Identity.Ratelimits = ptr.P(identityRatelimits) | |
| response.Identity.Ratelimits = &identityRatelimits | |
| } | |
| if len(identityRatelimits) > 0 && response.Identity != nil { | |
| response.Identity.Ratelimits = &identityRatelimits | |
| } |
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_keys_get_key/handler.go around lines 233-235, the code
assigns a pointer to identityRatelimits to response.Identity.Ratelimits without
ensuring response.Identity is non-nil; add a nil-check and initialize
response.Identity before assigning (e.g., if response.Identity == nil {
response.Identity = &models.Identity{} } ), then set
response.Identity.Ratelimits = &identityRatelimits so you avoid a nil
dereference.
| func (h *Handler) Method() string { | ||
| return "POST" | ||
| } | ||
|
|
||
| // Path returns the URL path pattern this route matches | ||
| func (h *Handler) Path() string { | ||
| return "/v2/keys.whoami" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document exported methods (Method, Path, Handle)
Exported methods should have GoDoc comments.
+// Method implements zen.Route.Method for the whoami route.
func (h *Handler) Method() string {
return "POST"
}
+// Path implements zen.Route.Path for the whoami route.
func (h *Handler) Path() string {
return "/v2/keys.whoami"
}
+// Handle resolves key identity/profile from a single query and returns KeyResponseData.
func (h *Handler) Handle(ctx context.Context, s *zen.Session) error {Also applies to: 46-46
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_keys_whoami/handler.go around lines 38-44 (and also
line 46), the exported methods Method, Path and Handle lack GoDoc comments; add
concise Go-style comments immediately above each exported method (e.g., "//
Method returns the HTTP method for the handler."), describing what each method
does and any important behavior or parameters, ensuring the comments start with
the method name to satisfy GoDoc conventions.
| if len(keyData.Identity.Meta) > 0 { | ||
| var identityMeta map[string]any | ||
| if err := json.Unmarshal(keyData.Identity.Meta, &identityMeta); err != nil { | ||
| h.Logger.Error("failed to unmarshal identity meta", "error", err) | ||
| } else { | ||
| response.Identity.Meta = &identityMeta | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Instrument JSON unmarshal failures for observability
Log lines exist; add a metric counter to track frequency for data quality monitoring.
if len(keyData.Identity.Meta) > 0 {
var identityMeta map[string]any
if err := json.Unmarshal(keyData.Identity.Meta, &identityMeta); err != nil {
h.Logger.Error("failed to unmarshal identity meta", "error", err)
+ // AIDEV-METRICS: increment counter for identity meta unmarshal failures (route=whoami, keyId=keyData.Key.ID)
} else {
response.Identity.Meta = &identityMeta
}
}
@@
if keyData.Key.Meta.Valid {
var meta map[string]any
if err := json.Unmarshal([]byte(keyData.Key.Meta.String), &meta); err != nil {
h.Logger.Error("failed to unmarshal key meta", "error", err)
+ // AIDEV-METRICS: increment counter for key meta unmarshal failures (route=whoami, keyId=keyData.Key.ID)
} else {
response.Meta = &meta
}
}Also applies to: 227-235
| if len(keyRatelimits) > 0 { | ||
| response.Ratelimits = &keyRatelimits | ||
| } | ||
| if len(identityRatelimits) > 0 { | ||
| response.Identity.Ratelimits = &identityRatelimits | ||
| } |
There was a problem hiding this comment.
Avoid potential nil dereference when assigning identity ratelimits
response.Identity may be nil; guard before writing to it. Even if the SQL currently guarantees no identity ratelimits without an identity, this makes the code resilient to future changes.
- if len(identityRatelimits) > 0 {
- response.Identity.Ratelimits = &identityRatelimits
- }
+ if len(identityRatelimits) > 0 && response.Identity != nil {
+ response.Identity.Ratelimits = &identityRatelimits
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(keyRatelimits) > 0 { | |
| response.Ratelimits = &keyRatelimits | |
| } | |
| if len(identityRatelimits) > 0 { | |
| response.Identity.Ratelimits = &identityRatelimits | |
| } | |
| if len(keyRatelimits) > 0 { | |
| response.Ratelimits = &keyRatelimits | |
| } | |
| if len(identityRatelimits) > 0 && response.Identity != nil { | |
| response.Identity.Ratelimits = &identityRatelimits | |
| } |
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_keys_whoami/handler.go around lines 219 to 224,
response.Identity may be nil before you assign to response.Identity.Ratelimits;
guard against a nil dereference by checking if response.Identity == nil and
initializing it (e.g. response.Identity = &<appropriate struct>{}) before
setting response.Identity.Ratelimits = &identityRatelimits, and keep the
existing conditional that only assigns when len(identityRatelimits) > 0.
4 participants
What does this PR do?
/v2/whoamiand/v2/getKeyBearerinauth.go. It should be case insensitive, but in our code it was sensitive toBearerandbearer.Fixes #3793
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
./docker compose -f ./deployment/docker-compose.yaml up apiv2 apiv2_lb redis clickhouse mysql s3 -d --buildhttps://www.diffchecker.com/text-compare/Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit