Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
|
📝 WalkthroughWalkthroughThis update introduces a major expansion and refactoring of the backend API, adding new v2 endpoints for APIs, identities, keys, permissions, and roles, along with comprehensive handler implementations and integration tests. It also updates configuration files, code generation settings, and documentation, including a migration guide from v1 to v2. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Server
participant DB
participant Vault
participant AuditLogs
Client->>API_Server: POST /v2/identities.createIdentity
API_Server->>DB: Begin transaction
API_Server->>DB: Insert identity (with metadata/ratelimits)
API_Server->>AuditLogs: Insert audit log(s)
API_Server->>DB: Commit transaction
API_Server-->>Client: 200 OK (identityId, meta)
Client->>API_Server: POST /v2/keys.createKey
API_Server->>DB: Begin transaction
API_Server->>DB: Insert key, assign permissions/roles
API_Server->>AuditLogs: Insert audit log(s)
API_Server->>DB: Commit transaction
API_Server-->>Client: 200 OK (keyId, key, meta)
Client->>API_Server: POST /v2/permissions.createPermission
API_Server->>DB: Begin transaction
API_Server->>DB: Insert permission
API_Server->>AuditLogs: Insert audit log
API_Server->>DB: Commit transaction
API_Server-->>Client: 200 OK (permissionId, meta)
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
chronark
requested review from
MichaelUnkey,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
go/apps/api/routes/v2_apis_list_keys/200_test.go (1)
136-140: Still swallowing thejson.Marshalerror – propagate it.The past review already pointed this out; the code still ignores the
errorreturned fromjson.Marshal, masking potential failures that would make the test mis-behave in subtle ways.- if keyData.meta != nil { - metaBytes, _ = json.Marshal(keyData.meta) - } + if keyData.meta != nil { + var err error + metaBytes, err = json.Marshal(keyData.meta) + require.NoError(t, err) + }go/apps/api/routes/v2_keys_remove_permissions/200_test.go (1)
56-77: Same “keyString[:4]” length assumption as in roles testSee earlier comment – slice-bounds panic risk if key generator ever shortens.
go/apps/api/routes/v2_keys_add_permissions/handler.go (1)
51-65: 🛠️ Refactor suggestionAdd specific API permission check for consistency.
Similar to the remove permissions handler, this should also check permissions for the specific API that the key belongs to.
After retrieving the key, add the specific API permission check:
err = svc.Permissions.Check( ctx, auth.KeyID, rbac.Or( rbac.T(rbac.Tuple{ ResourceType: rbac.Api, ResourceID: "*", Action: rbac.UpdateKey, }), + rbac.T(rbac.Tuple{ + ResourceType: rbac.Api, + ResourceID: key.KeyringID, // or appropriate API ID field + Action: rbac.UpdateKey, + }), ), )
🧹 Nitpick comments (33)
go/apps/api/routes/v2_keys_create_key/401_test.go (1)
94-106: Using an ad-hoc UID prefix may hide a broken check
uid.New("test_nonexistent")creates a value that may never pass server-side UID validation (if prefixes are whitelisted). In that case the test is exercising format errors, not the “key doesn’t exist” branch you intend.Generate a syntactically valid key ID for the correct prefix and make sure it is absent in the DB instead:
nonexistentKey := uid.New(uid.KeyPrefix) // valid shape, but not insertedgo/apps/api/routes/v2_apis_create_api/400_test.go (2)
17-19: Missingt.Parallel()All other v2 route tests run in parallel. Add it here for consistent execution time:
func TestCreateApi_BadRequest(t *testing.T) { t.Parallel() h := testutil.NewHarness(t)
86-110: Malformed-JSON test bypasses the typed helper – keep the surface constant
CallRawis used once while every other test relies onCallRoute. Diverging helpers complicate future refactors and hide coverage gaps (no request/response schema check). Consider feeding the invalid body throughCallRoutewith[]byteoverload or extendCallRouteto accept raw bodies so all tests share the same path.go/apps/api/routes/v2_keys_add_permissions/401_test.go (2)
20-33: Run sub-tests in parallelAdd
t.Parallel()at the beginning for consistency with the rest of the suite and to surface hidden data-races sooner.
128-162: Four nearly identical header-validation tests – extract helperThe blocks only differ in header value & expected status. Factor a small table-driven helper to reduce 40-ish duplicated lines and improve maintainability.
go/apps/api/routes/v2_keys_add_roles/403_test.go (1)
34-104: Heavy DB fixture duplicationEvery sub-test re-implements the same keyring/key/role creation boilerplate. This inflates the file (~450 lines) and slows CI. Extract helpers like
createKey(ctx, wsID)/insertRole(ctx, wsID, name)and reuse.go/apps/api/routes/v2_keys_add_roles/200_test.go (2)
19-33: Addt.Parallel()and reuse harnessParallelism helps but watch out for shared harness state as noted in earlier file.
448-539: Mixed ID & name – clarify precedence in specThe test asserts that ID wins over name when both supplied. Make sure this rule is documented in the public API; otherwise future devs may “fix” the handler and break clients.
go/apps/api/routes/v2_apis_list_keys/200_test.go (1)
20-24: Run the top-level test in parallel.All sub-tests already execute sequentially; declaring the parent with
t.Parallel()lets the Go test runner schedule this (lengthy) test alongside other packages, reducing total CI time without side-effects (the harness appears to provide an isolated DB per test).func TestSuccess(t *testing.T) { - ctx := context.Background() + t.Parallel() + ctx := context.Background()go/apps/api/routes/v2_keys_remove_permissions/401_test.go (1)
20-24: Addt.Parallel()for faster test suites.func TestAuthenticationErrors(t *testing.T) { - ctx := context.Background() + t.Parallel() + ctx := context.Background()This file performs only local DB operations through the harness, so running in parallel is safe and improves overall throughput.
go/apps/api/routes/v2_keys_remove_permissions/403_test.go (1)
20-24: Enable parallel execution.Same rationale as in the
401_test.gofile – the harness is isolated, so marking the test witht.Parallel()speeds up CI.func TestAuthorizationErrors(t *testing.T) { - ctx := context.Background() + t.Parallel() + ctx := context.Background()go/apps/api/routes/v2_keys_add_roles/400_test.go (1)
20-24: Kick off the validation-error suite in parallel.func TestValidationErrors(t *testing.T) { - ctx := context.Background() + t.Parallel() + ctx := context.Background()Given that the test only touches its own harness instance, parallelism is safe and trims execution time.
go/apps/api/routes/v2_keys_create_key/404_test.go (1)
63-89: Consider implementing proper test data setupTwo tests appear to be incomplete:
- The cross-workspace test (lines 63-89) mentions difficulty in creating an API for testing
- The deleted API test (lines 104-116) is noted as a placeholder
Consider adding helper methods to properly set up test data (APIs, soft-deleted APIs) to make these tests more realistic.
Would you like help implementing helper methods to create and soft-delete test APIs for these scenarios?
Also applies to: 104-116
go/apps/api/routes/v2_apis_get_api/200_test.go (2)
135-168: The “IP-whitelist” scenario is not actually exercising IP-whitelist logicThe test label suggests that the handler should behave differently when an API has an IP whitelist, yet the inserted record has no whitelist fields set and no request comes with a specific
X-Forwarded-For/ remote IP.
If the intent is to assert whitelist handling, insert the relevant whitelist rows (or JSON), or rename the sub-test to avoid confusion.
67-133: Heavy repetition – consider a small helper to cut boilerplateEvery sub-test repeats the root-key creation, header construction, route call, and response assertions. A tiny helper such as:
func callGetAPI(t *testing.T, perms []string, apiID string) testutil.TestResponse[handler.Response] { ... }would shrink the file and highlight the differences per case.
go/apps/api/routes/v2_keys_create_key/README.md (2)
185-194: Add a language identifier to fenced code blockMarkdown-lint (MD040) flags this block; adding a language (
text,bash, etc.) improves syntax highlighting and avoids the lint error.-``` +```text v2_keys_create_key/ ├── handler.go ...<details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 185-185: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> --- `75-83`: **Minor duplicated word in heading** `handling ### 400 Bad Request` renders with a doubled space / repeated word (“handling”). Removing the duplication keeps the doc crisp. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [duplication] ~76-~76: Possible typo: you repeated a word. Context: ...dling ### 400 Bad Request - Missing or invalid `apiId` - Invalid `byteLength` (must be 16-255) - Invalid... (ENGLISH_WORD_REPEAT_RULE) </details> </details> </blockquote></details> <details> <summary>go/apps/api/routes/v2_keys_remove_roles/200_test.go (3)</summary><blockquote> `57-66`: **Possible panic if a future change shortens generated keys** `Start: keyString[:4]` assumes `keyString` length ≥ 4. Today `uid.New("")` yields a long string, but if the generator ever returns a shorter token this will panic. Guard length or derive `Start` from the SHA-256 hash to decouple from generator changes. ```diff -start := keyString[:4] +start := keyString +if len(start) > 4 { + start = start[:4] +}
43-165: Audit-log assertion could miss false negativesThe loop sets
foundDisconnectEventbut does not fail fast if the wanted event is missing; the finalrequire.Truecovers it but the intermediaterequire.Containsis only checked when the event is found.
Usingassert.Containsinside the branch (or hoisting the check after the loop) avoids a noop assertion path.
168-335: Large block of duplicated setup code – extract helpersKeyring + key + role/permission creation is repeated three times.
Extracting helpers (e.g.,makeKey(t, h)/makeRole(t, h, name)) would trim ~150 lines, speed up reading, and ease future changes.go/apps/api/routes/v2_keys_remove_permissions/200_test.go (2)
80-146: Inefficient audit-log searchIterating every audit-log row per test is O(n). If audit-log volume grows, querying directly for the expected event (
WHERE event = ... LIMIT 1) would speed tests and remove the manual loop.
234-338: Sub-tests produce identical setup / teardown codeThe six sub-tests repeat ~80 lines each. Helpers (factory functions) or table-driven tests would shrink file size dramatically without losing clarity.
go/apps/api/routes/v2_identities_create_identity/200_test.go (2)
321-328: JSON equality done viaMarshalon a*mappointer
json.Marshal(nestedMeta)works, but dereferencing once (*nestedMeta) makes the intent clearer and avoids extra indirection shouldnilever be passed.-expectedJSON, _ := json.Marshal(nestedMeta) +expectedJSON, _ := json.Marshal(*nestedMeta)
430-478: Very large single test function – consider splitting file
TestCreateIdentitySuccessfullynow spans ~480 lines and many scenarios.
Breaking it into separate*_test.gofiles per concern (metadata, ratelimits, edge-cases) or using table-driven sub-tests would improve readability and IDE navigation.go/apps/api/routes/v2_keys_add_roles/handler.go (2)
258-265: Simplify the sorting function using strings.Compare.The current sorting implementation is correct but can be simplified.
-slices.SortFunc(finalRoles, func(a, b db.Role) int { - if a.Name < b.Name { - return -1 - } else if a.Name > b.Name { - return 1 - } - return 0 -}) +slices.SortFunc(finalRoles, func(a, b db.Role) int { + return strings.Compare(a.Name, b.Name) +})Note: You'll need to import the
stringspackage.
249-249: Use read-only connection for reading final roles.After the transaction is committed, use a read-only connection for better resource utilization.
-finalRoles, err := db.Query.ListRolesByKeyID(ctx, svc.DB.RW(), req.KeyId) +finalRoles, err := db.Query.ListRolesByKeyID(ctx, svc.DB.RO(), req.KeyId)go/apps/api/routes/v2_keys_add_permissions/400_test.go (1)
45-77: Extract key creation logic into a helper function to reduce duplication.The keyring and key creation logic is duplicated across tests. Consider creating a helper function to improve maintainability.
Add a helper function at the package level:
func createTestKey(t *testing.T, h *testutil.Harness, workspaceID string) string { ctx := context.Background() keyAuthID := uid.New(uid.KeyAuthPrefix) err := db.Query.InsertKeyring(ctx, h.DB.RW(), db.InsertKeyringParams{ ID: keyAuthID, WorkspaceID: workspaceID, StoreEncryptedKeys: false, DefaultPrefix: sql.NullString{Valid: true, String: "test"}, DefaultBytes: sql.NullInt32{Valid: true, Int32: 16}, CreatedAtM: time.Now().UnixMilli(), }) require.NoError(t, err) keyID := uid.New(uid.KeyPrefix) keyString := "test_" + uid.New("") err = db.Query.InsertKey(ctx, h.DB.RW(), db.InsertKeyParams{ ID: keyID, KeyringID: keyAuthID, Hash: hash.Sha256(keyString), Start: keyString[:4], WorkspaceID: workspaceID, ForWorkspaceID: sql.NullString{Valid: false}, Name: sql.NullString{Valid: true, String: "Test Key"}, CreatedAtM: time.Now().UnixMilli(), Enabled: true, IdentityID: sql.NullString{Valid: false}, Meta: sql.NullString{Valid: false}, Expires: sql.NullTime{Valid: false}, RemainingRequests: sql.NullInt32{Valid: false}, RatelimitAsync: sql.NullBool{Valid: false}, RatelimitLimit: sql.NullInt32{Valid: false}, RatelimitDuration: sql.NullInt64{Valid: false}, Environment: sql.NullString{Valid: false}, }) require.NoError(t, err) return keyID }Then use it in tests:
-// Create a valid key for testing -keyAuthID := uid.New(uid.KeyAuthPrefix) -err := db.Query.InsertKeyring(ctx, h.DB.RW(), db.InsertKeyringParams{ - // ... all the parameters -}) -require.NoError(t, err) - -validKeyID := uid.New(uid.KeyPrefix) -// ... rest of key creation +validKeyID := createTestKey(t, h, workspace.ID)go/apps/api/routes/v2_keys_add_permissions/404_test.go (1)
20-369: Excellent security test coverage with workspace isolation scenarios.The tests thoroughly validate workspace boundaries which is critical for multi-tenant security:
- Permission from different workspace (lines 208-287)
- Key from different workspace (lines 289-368)
- Proper 404 responses prevent information leakage
Consider using the same helper function suggested in
400_test.goto reduce the key creation duplication here as well.go/apps/api/routes/v2_keys_add_roles/404_test.go (1)
47-47: Use proper ID generation for non-existent key ID.Instead of hardcoding the key ID, use
uid.New()to generate a properly formatted ID that doesn't exist in the database.-KeyId: "key_nonexistent123456789", // Non-existent key ID +KeyId: uid.New(uid.KeyPrefix), // Non-existent key ID with proper formatgo/apps/api/routes/v2_apis_list_keys/handler.go (1)
36-39: TODO: Implement caching strategy for improved performance.The comment correctly identifies that caching should be implemented to match the TypeScript v1 implementation's performance characteristics.
Would you like me to create an issue to track the implementation of the caching strategy with the
revalidateKeysCacheparameter?go/apps/api/routes/v2_keys_create_key/403_test.go (3)
92-96: Prefer symbolic HTTP status constant over magic numberHard-coding the literal
403obscures intent and makes future refactors (e.g., switching to a custom status) error-prone. Replace with the semantic constanthttp.StatusForbidden.- require.Equal(t, 403, res.Status) + require.Equal(t, http.StatusForbidden, res.Status)Also applies to: 107-111, 122-126, 138-142, 153-157, 168-172
113-142: Two sub-tests appear functionally identical → consolidate or differentiate“permission for different API” and “permission for specific API but requesting different API” both:
- Grant
api.<otherApiID>.create_key- Call the same target API (
apiID)- Expect 403
Unless a subtle scenario distinction is missing, the duplication adds runtime cost without coverage gain. Either merge them or modify one to cover a genuinely different edge case (e.g., wildcard permission
api.*.create_key).
83-170: Extract repetitive header/key setup into a helper to trim boilerplateEach sub-test repeats:
rootKey := h.CreateRootKey(...) headers := http.Header{ "Content-Type": {"application/json"}, "Authorization": {fmt.Sprintf("Bearer %s", rootKey)}, } res := testutil.CallRoute[…](h, route, headers, req)A small helper (e.g.,
call(req, perms ...string)) would:
- Generate the keyed token with given permissions
- Build headers
- Invoke
CallRouteThis shrinks ~60 lines, improves readability, and reduces maintenance cost when the call signature changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (49)
apps/engineering/app/docs/[[...slug]]/page.tsx(1 hunks)go/apps/api/routes/register.go(3 hunks)go/apps/api/routes/v2_apis_create_api/200_test.go(7 hunks)go/apps/api/routes/v2_apis_create_api/400_test.go(2 hunks)go/apps/api/routes/v2_apis_create_api/403_test.go(1 hunks)go/apps/api/routes/v2_apis_create_api/handler.go(5 hunks)go/apps/api/routes/v2_apis_delete_api/200_test.go(1 hunks)go/apps/api/routes/v2_apis_delete_api/412_test.go(1 hunks)go/apps/api/routes/v2_apis_delete_api/cache_validation_test.go(1 hunks)go/apps/api/routes/v2_apis_delete_api/handler.go(1 hunks)go/apps/api/routes/v2_apis_delete_api/special_chars_test.go(1 hunks)go/apps/api/routes/v2_apis_get_api/200_test.go(1 hunks)go/apps/api/routes/v2_apis_get_api/404_test.go(1 hunks)go/apps/api/routes/v2_apis_get_api/handler.go(1 hunks)go/apps/api/routes/v2_apis_list_keys/200_test.go(1 hunks)go/apps/api/routes/v2_apis_list_keys/handler.go(1 hunks)go/apps/api/routes/v2_identities_create_identity/200_test.go(3 hunks)go/apps/api/routes/v2_identities_create_identity/handler.go(5 hunks)go/apps/api/routes/v2_identities_delete_identity/200_test.go(3 hunks)go/apps/api/routes/v2_identities_delete_identity/handler.go(7 hunks)go/apps/api/routes/v2_identities_get_identity/handler.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/handler.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/handler.go(1 hunks)go/apps/api/routes/v2_keys_add_permissions/200_test.go(1 hunks)go/apps/api/routes/v2_keys_add_permissions/400_test.go(1 hunks)go/apps/api/routes/v2_keys_add_permissions/401_test.go(1 hunks)go/apps/api/routes/v2_keys_add_permissions/403_test.go(1 hunks)go/apps/api/routes/v2_keys_add_permissions/404_test.go(1 hunks)go/apps/api/routes/v2_keys_add_permissions/handler.go(1 hunks)go/apps/api/routes/v2_keys_add_roles/200_test.go(1 hunks)go/apps/api/routes/v2_keys_add_roles/400_test.go(1 hunks)go/apps/api/routes/v2_keys_add_roles/401_test.go(1 hunks)go/apps/api/routes/v2_keys_add_roles/403_test.go(1 hunks)go/apps/api/routes/v2_keys_add_roles/404_test.go(1 hunks)go/apps/api/routes/v2_keys_add_roles/handler.go(1 hunks)go/apps/api/routes/v2_keys_create_key/200_test.go(1 hunks)go/apps/api/routes/v2_keys_create_key/400_test.go(1 hunks)go/apps/api/routes/v2_keys_create_key/401_test.go(1 hunks)go/apps/api/routes/v2_keys_create_key/403_test.go(1 hunks)go/apps/api/routes/v2_keys_create_key/404_test.go(1 hunks)go/apps/api/routes/v2_keys_create_key/README.md(1 hunks)go/apps/api/routes/v2_keys_create_key/handler.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/200_test.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/400_test.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/401_test.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/403_test.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/404_test.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/handler.go(1 hunks)go/apps/api/routes/v2_keys_remove_roles/200_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/engineering/app/docs/[[...slug]]/page.tsx
🚧 Files skipped from review as they are similar to previous changes (13)
- go/apps/api/routes/v2_apis_delete_api/special_chars_test.go
- go/apps/api/routes/v2_apis_delete_api/412_test.go
- go/apps/api/routes/v2_apis_create_api/200_test.go
- go/apps/api/routes/v2_apis_get_api/handler.go
- go/apps/api/routes/v2_apis_delete_api/200_test.go
- go/apps/api/routes/v2_identities_create_identity/handler.go
- go/apps/api/routes/v2_apis_create_api/handler.go
- go/apps/api/routes/v2_apis_create_api/403_test.go
- go/apps/api/routes/v2_apis_delete_api/handler.go
- go/apps/api/routes/v2_apis_get_api/404_test.go
- go/apps/api/routes/v2_identities_list_identities/handler.go
- go/apps/api/routes/v2_identities_update_identity/handler.go
- go/apps/api/routes/v2_identities_get_identity/handler.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
go/apps/api/routes/v2_keys_remove_roles/200_test.go (5)
go/pkg/testutil/http.go (2)
NewHarness(53-178)CallRoute(235-269)go/apps/api/routes/v2_keys_remove_roles/handler.go (3)
New(34-285)Request(23-23)Response(24-24)go/pkg/uid/uid.go (3)
KeyAuthPrefix(23-23)KeyPrefix(18-18)TestPrefix(26-26)go/pkg/db/role_insert.sql_generated.go (1)
InsertRoleParams(28-33)go/pkg/db/key_role_insert.sql_generated.go (1)
InsertKeyRoleParams(27-32)
go/apps/api/routes/v2_identities_delete_identity/200_test.go (11)
go/apps/api/routes/v2_identities_delete_identity/handler.go (2)
New(99-282)Response(89-89)go/pkg/testutil/seed/seed.go (2)
New(32-38)Resources(18-22)go/pkg/uid/uid.go (2)
RatelimitPrefix(31-31)IdentityPrefix(30-30)go/pkg/db/identity_find_by_id.sql_generated.go (1)
FindIdentityByIDParams(16-19)go/pkg/assert/equal.go (1)
Equal(17-26)go/pkg/testutil/http.go (1)
CallRoute(235-269)go/pkg/assert/not_nil.go (1)
NotNil(16-25)go/pkg/assert/true.go (1)
True(17-26)go/pkg/db/identity_find_by_external_id.sql_generated.go (1)
FindIdentityByExternalIDParams(16-20)go/pkg/assert/greater_or_equal.go (1)
GreaterOrEqual(19-29)go/pkg/db/models_generated.go (1)
AuditLog(238-254)
🪛 LanguageTool
go/apps/api/routes/v2_keys_create_key/README.md
[duplication] ~76-~76: Possible typo: you repeated a word.
Context: ...dling ### 400 Bad Request - Missing or invalid apiId - Invalid byteLength (must be 16-255) - Invalid...
(ENGLISH_WORD_REPEAT_RULE)
🪛 markdownlint-cli2 (0.17.2)
go/apps/api/routes/v2_keys_create_key/README.md
185-185: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (27)
go/apps/api/routes/v2_keys_create_key/401_test.go (1)
15-28: Parallel test + shared harness may race
t.Parallel()is called at the top-level, yet the same harness instance (with its DB connection) is mutated by all sub-tests. Unlesstestutil.NewHarnessguarantees internal locking or connection pooling that is concurrency-safe, you risk sporadic data-races / “database is closed” panics when the package is tested with-race.- t.Parallel() + // Leave sub-tests parallelised, keep harness serial to avoid DB races + // t.Parallel()Or instantiate an independent harness per sub-test.
go/apps/api/routes/v2_apis_create_api/400_test.go (1)
149-161: Should this be 401 instead of 400?Other endpoints treat a missing
Authorizationheader as “unauthorized” (401). Decide on one convention and update the assertion (or the handler) accordingly.go/apps/api/routes/v2_keys_add_permissions/401_test.go (1)
93-108: Status-code drift with sibling routeHere a missing
Authorizationheader yields 400, while the create-key route tests for 401. Confirm which status the spec mandates and align.go/apps/api/routes/v2_keys_add_roles/200_test.go (1)
334-446: Audit-log count check can race with previous testsBecause previous sub-tests may have inserted audit logs for the same key ID (if reused) the expected count of
connect_role_and_keyevents can be higher than 1/3. Either isolate key IDs per sub-test or filter bycreated_at≥ test start time.go/apps/api/routes/register.go (2)
44-430: Register() is turning into a 400-line god-function – extract per-namespace helpers
Registernow mixes 60+ nearly-identicalsrv.RegisterRouteinvocations.
This hurts readability, increases merge-conflict surface, and makes it easy to forget to attach future middle-wares/services consistently.Suggested direction (pseudo-diff):
-func Register(srv *zen.Server, svc *Services) { - ... - srv.RegisterRoute(defaultMiddlewares, - v2KeysRemoveRoles.New(v2KeysRemoveRoles.Services{ Logger: svc.Logger, DB: svc.Database, … })) -} +func Register(srv *zen.Server, svc *Services) { + defMw := buildDefaultMw(srv, svc) + + registerV2Ratelimit(srv, svc, defMw) + registerV2Identities(srv, svc, defMw) + registerV2Apis(srv, svc, defMw) + registerV2Permissions(srv, svc, defMw) + registerV2Keys(srv, svc, defMw) + + srv.RegisterRoute([]zen.Middleware{ defMw.base... }, reference.New()) +}Each
registerV2*would live in its own file; changes stay local, diff noise disappears, and compile times drop because fewer imports change per edit.
[ suggest_essential_refactor ]
54-61: defaultMiddlewares could be a func to avoid accidental divergenceFive lambdas create one slice; turning this into:
func buildDefaultMw(srv *zen.Server, svc *Services) []zen.Middleware { … }guarantees future routes cannot silently “forget” a middleware when someone re-orders or inserts new items.
[ suggest_optional_refactor ]go/apps/api/routes/v2_keys_add_permissions/403_test.go (2)
20-33: Missingt.Parallel()limits test-suite throughputAdd
t.Parallel()at the top ofTestAuthorizationErrors(and inside each sub-test if the harness is concurrency-safe) to keep CI runtimes under control as the matrix of negative-tests grows.func TestAuthorizationErrors(t *testing.T) { - ctx := context.Background() + t.Parallel() + ctx := context.Background()[ suggest_optional_refactor ]
93-113: Asserting on free-form error strings is brittle
require.Contains(t, res.Body.Error.Detail, "permission")will break the moment wording changes.
Prefer stable signals – e.g., compareres.Body.Error.Code, or expose typed error identifiers.
[ suggest_nitpick ]go/apps/api/routes/v2_apis_delete_api/cache_validation_test.go (2)
20-33: Enable test parallelismSame remark as above – add
t.Parallel()to keep large test suites fast.[ suggest_optional_refactor ]
71-79: Pre-loading cache via SWR duplicates production logicFor tighter coverage, call the public
GET /v2/apis.getApiroute instead of directly touchingCaches.ApiByID.SWR.
This validates that both the handler and the caching layer cooperate correctly.[ suggest_nitpick ]
go/apps/api/routes/v2_keys_create_key/400_test.go (1)
161-172: Hard-coding validation message text couples the test to UX wordingChecking for the full sentence
"Permission 'nonexistent.permission' was not found"makes refactors painful. Assert on an error code or sentinel value instead.[ suggest_nitpick ]
go/apps/api/routes/v2_keys_remove_permissions/404_test.go (2)
20-33: Considert.Parallel()for faster negative-path coverageThese 404 scenarios are DB-heavy; allowing them to run in parallel can cut minutes off the pipeline.
[ suggest_optional_refactor ]
206-214: Potential unique-constraint flake onorg_idAll helper workspaces use static
OrgID: "test_org".
If theorgstable enforces uniqueness, parallel tests (now or future) may race.
Generate a UID forOrgIDas well to stay safe.[ request_verification ]
go/apps/api/routes/v2_keys_add_roles/401_test.go (1)
20-221: LGTM!Comprehensive test coverage for authentication error scenarios. All edge cases are properly tested with appropriate status code assertions.
go/apps/api/routes/v2_identities_delete_identity/handler.go (1)
100-335: LGTM!The refactoring to the new error handling pattern is consistent and well-implemented. The simplified permission checks and updated database query methods align with the broader codebase improvements.
go/apps/api/routes/v2_apis_get_api/200_test.go (1)
170-203: Risk of silently truncating over-long names – double-check DB column width
apiNameis ~170 chars. Make sure theapis.namecolumn (and any unique index) is wide enough (varchar(255)?) or setCHECK/validation to avoid truncation in some SQL drivers (e.g., MySQL will truncate in strict-off mode).
If the column is narrower the equality assertions here will pass in Go but the DB will contain a truncated value, leading to false confidence.go/apps/api/routes/v2_identities_create_identity/200_test.go (1)
94-100:ListIdentityRatelimitsByIDreturns by identity, not workspace – verify semanticsPassing
sql.NullString{String: identityID, Valid: true}is correct, but ensure the DAO indeed filters byidentity_idonly; older queries sometimes requiredworkspace_idtoo.
A mismatch would let rate-limits from other workspaces leak into the result and make the assertion flaky.go/apps/api/routes/v2_keys_add_roles/handler.go (1)
35-287: Well-implemented handler with robust error handling and security checks.The implementation follows best practices:
- Clear, numbered steps for easy understanding
- Proper transaction management with deferred rollback
- Comprehensive workspace validation preventing cross-workspace access
- Detailed audit logging for compliance
- Efficient deduplication of roles to add
go/apps/api/routes/v2_keys_add_permissions/400_test.go (1)
20-305: Comprehensive validation test coverage.The tests thoroughly cover validation scenarios including:
- Missing and empty required fields
- Invalid input formats
- Resource not found cases
- Proper error message assertions
Particularly good is the test for "permission missing both id and name" which validates the mutual exclusivity requirement.
go/apps/api/routes/v2_keys_add_roles/404_test.go (1)
405-627: Excellent edge case coverage for error scenarios.Great test coverage including:
- Multiple roles with partial failure (lines 405-478)
- Deleted key scenario (lines 481-552)
- Deleted role scenario (lines 555-627)
These edge cases are often overlooked but are important for robust error handling.
go/apps/api/routes/v2_keys_create_key/200_test.go (1)
18-164: Well-structured success tests with proper verification.The tests effectively cover:
- Basic key creation with minimal parameters
- Extended key creation with all optional fields
- Proper database state verification
- Response validation including key format with prefix
Good practice verifying the key was actually persisted in the database with expected values.
go/apps/api/routes/v2_keys_remove_permissions/handler.go (2)
104-158: Well-implemented permission resolution logic.The implementation correctly handles both ID and name-based permission references, validates workspace ownership, and provides clear error messages.
159-244: Excellent transaction handling and audit logging implementation.The code correctly:
- Implements idempotent behavior by only removing existing permissions
- Uses database transactions for atomicity
- Creates detailed audit logs for each permission removal
- Handles transaction rollback properly in the defer block
go/apps/api/routes/v2_keys_add_permissions/handler.go (1)
258-279: Good implementation of sorted response data.The code properly sorts permissions alphabetically for consistent API responses and correctly builds the response structure.
go/apps/api/routes/v2_keys_create_key/handler.go (1)
334-358: Comprehensive audit logging for key creation.The audit log properly captures the key creation event with all relevant resources (key and API) and metadata.
go/apps/api/routes/v2_apis_list_keys/handler.go (1)
55-89: Excellent permission validation requiring both API and key read access.The permission check correctly enforces that the caller must have both
ReadAPIandReadKeypermissions, providing proper access control for listing keys.go/apps/api/routes/v2_identities_delete_identity/200_test.go (1)
63-298: Comprehensive test coverage with well-structured test cases.The test suite thoroughly covers:
- Identity deletion by ID and external ID
- Deletion with rate limits
- Permission-based access control (wildcard and specific)
- Audit log creation verification
- Edge case handling for duplicate external IDs
The refactoring to use clearer names (
TestIdentity,createTestIdentity) improves code readability.
| t.Run("no authorization header", func(t *testing.T) { | ||
| headers := http.Header{ | ||
| "Content-Type": {"application/json"}, | ||
| } | ||
|
|
||
| res := testutil.CallRoute[handler.Request, openapi.UnauthorizedErrorResponse](h, route, headers, req) | ||
| require.Equal(t, 401, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| require.Contains(t, res.Body.Error.Detail, "missing") | ||
| }) | ||
|
|
||
| t.Run("empty authorization header", func(t *testing.T) { | ||
| headers := http.Header{ | ||
| "Content-Type": {"application/json"}, | ||
| "Authorization": {""}, | ||
| } | ||
|
|
||
| res := testutil.CallRoute[handler.Request, openapi.UnauthorizedErrorResponse](h, route, headers, req) | ||
| require.Equal(t, 401, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| require.Contains(t, res.Body.Error.Detail, "missing") | ||
| }) | ||
|
|
||
| t.Run("invalid authorization format", func(t *testing.T) { | ||
| headers := http.Header{ | ||
| "Content-Type": {"application/json"}, | ||
| "Authorization": {"InvalidFormat"}, | ||
| } | ||
|
|
||
| res := testutil.CallRoute[handler.Request, openapi.UnauthorizedErrorResponse](h, route, headers, req) | ||
| require.Equal(t, 401, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| require.Contains(t, res.Body.Error.Detail, "malformed") | ||
| }) | ||
|
|
||
| t.Run("missing bearer prefix", func(t *testing.T) { | ||
| headers := http.Header{ | ||
| "Content-Type": {"application/json"}, | ||
| "Authorization": {"sk_invalid_key"}, | ||
| } | ||
|
|
||
| res := testutil.CallRoute[handler.Request, openapi.UnauthorizedErrorResponse](h, route, headers, req) | ||
| require.Equal(t, 401, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| require.Contains(t, res.Body.Error.Detail, "malformed") | ||
| }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Error-code inconsistency vs other v2 routes
For missing or malformed Authorization headers this test expects HTTP 401, while other new routes (v2_keys_add_permissions, v2_apis_create_api) expect HTTP 400. A public API should respond consistently for the same fault class.
Please verify the handler’s spec and align either:
- require.Equal(t, 401, res.Status)
+ require.Equal(t, http.StatusBadRequest, res.Status)(or update sibling routes & docs).
I’ll verify how sibling v2 routes handle missing/malformed auth headers.
🏁 Script executed:
#!/bin/bash
set -e
echo "---- v2_keys_add_permissions auth tests ----"
rg -n "Authorization" go/apps/api/routes/v2_keys_add_permissions
echo
echo "---- v2_keys_add_permissions status checks ----"
rg -n "require.Equal" go/apps/api/routes/v2_keys_add_permissions
echo
echo "---- v2_apis_create_api auth tests ----"
rg -n "Authorization" go/apps/api/routes/v2_apis_create_api
echo
echo "---- v2_apis_create_api status checks ----"
rg -n "require.Equal" go/apps/api/routes/v2_apis_create_apiLength of output: 12978
Align missing/malformed auth header responses to HTTP 400
The other v2 routes return 400 for missing or malformed Authorization headers. Update the four tests in go/apps/api/routes/v2_keys_create_key/401_test.go (lines 35–80) to expect http.StatusBadRequest instead of 401.
Locations to update:
- File: go/apps/api/routes/v2_keys_create_key/401_test.go
• t.Run("no authorization header")
• t.Run("empty authorization header")
• t.Run("invalid authorization format")
• t.Run("missing bearer prefix")
Diff snippet:
- require.Equal(t, 401, res.Status)
+ require.Equal(t, http.StatusBadRequest, res.Status)📝 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.
| t.Run("no authorization header", func(t *testing.T) { | |
| headers := http.Header{ | |
| "Content-Type": {"application/json"}, | |
| } | |
| res := testutil.CallRoute[handler.Request, openapi.UnauthorizedErrorResponse](h, route, headers, req) | |
| require.Equal(t, 401, res.Status) | |
| require.NotNil(t, res.Body) | |
| require.Contains(t, res.Body.Error.Detail, "missing") | |
| }) | |
| t.Run("empty authorization header", func(t *testing.T) { | |
| headers := http.Header{ | |
| "Content-Type": {"application/json"}, | |
| "Authorization": {""}, | |
| } | |
| res := testutil.CallRoute[handler.Request, openapi.UnauthorizedErrorResponse](h, route, headers, req) | |
| require.Equal(t, 401, res.Status) | |
| require.NotNil(t, res.Body) | |
| require.Contains(t, res.Body.Error.Detail, "missing") | |
| }) | |
| t.Run("invalid authorization format", func(t *testing.T) { | |
| headers := http.Header{ | |
| "Content-Type": {"application/json"}, | |
| "Authorization": {"InvalidFormat"}, | |
| } | |
| res := testutil.CallRoute[handler.Request, openapi.UnauthorizedErrorResponse](h, route, headers, req) | |
| require.Equal(t, 401, res.Status) | |
| require.NotNil(t, res.Body) | |
| require.Contains(t, res.Body.Error.Detail, "malformed") | |
| }) | |
| t.Run("missing bearer prefix", func(t *testing.T) { | |
| headers := http.Header{ | |
| "Content-Type": {"application/json"}, | |
| "Authorization": {"sk_invalid_key"}, | |
| } | |
| res := testutil.CallRoute[handler.Request, openapi.UnauthorizedErrorResponse](h, route, headers, req) | |
| require.Equal(t, 401, res.Status) | |
| require.NotNil(t, res.Body) | |
| require.Contains(t, res.Body.Error.Detail, "malformed") | |
| }) | |
| t.Run("no authorization header", func(t *testing.T) { | |
| headers := http.Header{ | |
| "Content-Type": {"application/json"}, | |
| } | |
| res := testutil.CallRoute[handler.Request, openapi.UnauthorizedErrorResponse](h, route, headers, req) | |
| require.Equal(t, http.StatusBadRequest, res.Status) | |
| require.NotNil(t, res.Body) | |
| require.Contains(t, res.Body.Error.Detail, "missing") | |
| }) | |
| t.Run("empty authorization header", func(t *testing.T) { | |
| headers := http.Header{ | |
| "Content-Type": {"application/json"}, | |
| "Authorization": {""}, | |
| } | |
| res := testutil.CallRoute[handler.Request, openapi.UnauthorizedErrorResponse](h, route, headers, req) | |
| require.Equal(t, http.StatusBadRequest, res.Status) | |
| require.NotNil(t, res.Body) | |
| require.Contains(t, res.Body.Error.Detail, "missing") | |
| }) | |
| t.Run("invalid authorization format", func(t *testing.T) { | |
| headers := http.Header{ | |
| "Content-Type": {"application/json"}, | |
| "Authorization": {"InvalidFormat"}, | |
| } | |
| res := testutil.CallRoute[handler.Request, openapi.UnauthorizedErrorResponse](h, route, headers, req) | |
| require.Equal(t, http.StatusBadRequest, res.Status) | |
| require.NotNil(t, res.Body) | |
| require.Contains(t, res.Body.Error.Detail, "malformed") | |
| }) | |
| t.Run("missing bearer prefix", func(t *testing.T) { | |
| headers := http.Header{ | |
| "Content-Type": {"application/json"}, | |
| "Authorization": {"sk_invalid_key"}, | |
| } | |
| res := testutil.CallRoute[handler.Request, openapi.UnauthorizedErrorResponse](h, route, headers, req) | |
| require.Equal(t, http.StatusBadRequest, res.Status) | |
| require.NotNil(t, res.Body) | |
| require.Contains(t, res.Body.Error.Detail, "malformed") | |
| }) |
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_keys_create_key/401_test.go between lines 35 and 80,
the tests for missing or malformed Authorization headers currently expect HTTP
401 status codes, but other v2 routes use HTTP 400 for these cases. Update the
expected status codes in the four test cases ("no authorization header", "empty
authorization header", "invalid authorization format", and "missing bearer
prefix") to http.StatusBadRequest (400) instead of 401 to ensure consistent API
behavior.
| // Test with unexpected fields in request | ||
| t.Run("unexpected fields", func(t *testing.T) { | ||
| // Valid request with additional unexpected fields | ||
| req := struct { | ||
| Name string `json:"name"` | ||
| UnexpectedField bool `json:"unexpectedField"` | ||
| AnotherField int `json:"anotherField"` | ||
| }{ | ||
| Name: "valid-api-name", | ||
| UnexpectedField: true, | ||
| AnotherField: 42, | ||
| } | ||
|
|
||
| res := testutil.CallRoute[struct { | ||
| Name string `json:"name"` | ||
| UnexpectedField bool `json:"unexpectedField"` | ||
| AnotherField int `json:"anotherField"` | ||
| }, handler.Response](h, route, headers, req) | ||
|
|
||
| // Depending on the validation configuration, this might be 200 (if extra fields are ignored) | ||
| // or 400 (if the schema is strict and rejects unknown properties) | ||
| if res.Status == http.StatusBadRequest { | ||
| // If 400, verify the error response | ||
| require.Equal(t, http.StatusBadRequest, res.Status) | ||
| require.Contains(t, res.RawBody, "error") | ||
| } else { | ||
| // If 200, verify it worked despite extra fields (API ignores unknown fields) | ||
| require.Equal(t, http.StatusOK, res.Status) | ||
| require.NotEmpty(t, res.Body.Data.ApiId) | ||
|
|
||
| // Verify the API was created with the correct name | ||
| api, err := db.Query.FindApiByID(context.Background(), h.DB.RO(), res.Body.Data.ApiId) | ||
| require.NoError(t, err) | ||
| require.Equal(t, "valid-api-name", api.Name) | ||
| } | ||
| }) |
There was a problem hiding this comment.
“Unexpected fields” test is non-deterministic
The assertion branch accepts either 200 or 400, which means the test can never fail – it will always satisfy one branch. Lock the expectation to the behaviour defined by the public schema (usually 400 for additional properties):
require.Equal(t, http.StatusBadRequest, res.Status)
require.Contains(t, res.RawBody, `"unexpectedField"`)This keeps the suite informative.
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_apis_create_api/400_test.go between lines 111 and 146,
the test for unexpected fields currently accepts either a 200 or 400 response,
making it non-deterministic and ineffective. Update the test to expect a 400 Bad
Request status explicitly when extra fields are present, and assert that the
response body contains the string "unexpectedField" to confirm the error relates
to the unexpected field. Remove the branch that accepts a 200 status to enforce
strict schema validation.
| // Test case for disabled root key | ||
| t.Run("disabled root key", func(t *testing.T) { | ||
| workspace := h.Resources().UserWorkspace | ||
| rootKey := h.CreateRootKey(workspace.ID, "api.*.update_key") | ||
|
|
||
| // Create a test keyring | ||
| keyAuthID := uid.New(uid.KeyAuthPrefix) | ||
| err := db.Query.InsertKeyring(ctx, h.DB.RW(), db.InsertKeyringParams{ | ||
| ID: keyAuthID, | ||
| WorkspaceID: workspace.ID, | ||
| StoreEncryptedKeys: false, | ||
| DefaultPrefix: sql.NullString{Valid: true, String: "test"}, | ||
| DefaultBytes: sql.NullInt32{Valid: true, Int32: 16}, | ||
| CreatedAtM: time.Now().UnixMilli(), | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // Create a test key | ||
| keyID := uid.New(uid.KeyPrefix) | ||
| keyString := "test_" + uid.New("") | ||
| err = db.Query.InsertKey(ctx, h.DB.RW(), db.InsertKeyParams{ | ||
| ID: keyID, | ||
| KeyringID: keyAuthID, | ||
| Hash: hash.Sha256(keyString), | ||
| Start: keyString[:4], | ||
| WorkspaceID: workspace.ID, | ||
| ForWorkspaceID: sql.NullString{Valid: false}, | ||
| Name: sql.NullString{Valid: true, String: "Test Key"}, | ||
| CreatedAtM: time.Now().UnixMilli(), | ||
| Enabled: true, | ||
| IdentityID: sql.NullString{Valid: false}, | ||
| Meta: sql.NullString{Valid: false}, | ||
| Expires: sql.NullTime{Valid: false}, | ||
| RemainingRequests: sql.NullInt32{Valid: false}, | ||
| RatelimitAsync: sql.NullBool{Valid: false}, | ||
| RatelimitLimit: sql.NullInt32{Valid: false}, | ||
| RatelimitDuration: sql.NullInt64{Valid: false}, | ||
| Environment: sql.NullString{Valid: false}, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // Use invalid root key to simulate disabled key | ||
| rootKey = "invalid_disabled_key" | ||
|
|
||
| req := handler.Request{ | ||
| KeyId: keyID, | ||
| Roles: []struct { | ||
| Id *string `json:"id,omitempty"` | ||
| Name *string `json:"name,omitempty"` | ||
| }{ | ||
| {Id: func() *string { s := "role_123"; return &s }()}, | ||
| }, | ||
| } | ||
|
|
||
| headers := http.Header{ | ||
| "Content-Type": {"application/json"}, | ||
| "Authorization": {fmt.Sprintf("Bearer %s", rootKey)}, | ||
| } | ||
|
|
||
| res := testutil.CallRoute[handler.Request, openapi.UnauthorizedErrorResponse]( | ||
| h, | ||
| route, | ||
| headers, | ||
| req, | ||
| ) | ||
|
|
||
| require.Equal(t, 401, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| require.NotNil(t, res.Body.Error) | ||
| }) |
There was a problem hiding this comment.
“Disabled / expired / deleted” scenarios use invalid tokens instead of real state
Setting rootKey = "invalid_disabled_key" exercises the token-not-found branch, not the disabled branch. Prefer inserting a real root-key row and toggling enabled=false, expires < now, or soft-deleting the workspace to truly cover those code paths.
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_keys_add_roles/403_test.go around lines 177 to 246, the
test simulates a disabled root key by assigning an invalid token string, which
only tests the token-not-found case. To properly test the disabled root key
scenario, insert a real root key record in the database and then update its
state by setting enabled to false, setting an expiration time in the past, or
soft-deleting the workspace. This will ensure the test covers the actual
disabled or expired key code paths.
| // Verify all roles are present and sorted alphabetically | ||
| roleNames := []string{res.Body.Data[0].Name, res.Body.Data[1].Name, res.Body.Data[2].Name} | ||
| require.Equal(t, []string{"admin_multi", "editor_multi", "viewer_multi"}, roleNames) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Order-dependent assertion may be brittle
The handler is not contractually obliged to sort roles alphabetically. If implementation changes (e.g., DB order), this test will fail spuriously. Compare sets instead:
require.ElementsMatch(t,
[]string{"admin_multi", "editor_multi", "viewer_multi"},
roleNames,
)🤖 Prompt for AI Agents
In go/apps/api/routes/v2_keys_add_roles/200_test.go around lines 310 to 313, the
test asserts roleNames in a fixed order which can cause brittle failures if the
order changes. Replace the order-dependent require.Equal assertion with
require.ElementsMatch to compare the slices as sets, ensuring the test passes
regardless of the order of roles.
|
|
||
| t.Run("permission missing both id and name", func(t *testing.T) { | ||
| // Create a permission for valid structure | ||
| permissionID := uid.New(uid.TestPrefix) |
There was a problem hiding this comment.
Use correct prefix for permission IDs
Permission IDs are being created with uid.TestPrefix which appears incorrect. Permission IDs should use a permission-specific prefix.
-permissionID := uid.New(uid.TestPrefix)
+permissionID := uid.New(uid.PermissionPrefix)Also applies to: 214-214, 268-268
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_keys_remove_permissions/400_test.go at lines 180, 214,
and 268, the permission IDs are generated using uid.TestPrefix, which is
incorrect. Update these lines to use the appropriate permission-specific prefix
constant for permission IDs instead of uid.TestPrefix to ensure correct ID
generation.
| require.NoError(t, err) | ||
|
|
||
| // Create a permission | ||
| permissionID := uid.New(uid.TestPrefix) |
There was a problem hiding this comment.
Use correct prefix for permission IDs
Permission IDs are being created with uid.TestPrefix throughout this file. Permission IDs should use a permission-specific prefix for consistency.
-permissionID := uid.New(uid.TestPrefix)
+permissionID := uid.New(uid.PermissionPrefix)Also applies to: 181-181, 267-267, 278-278, 363-363, 454-454, 464-464
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_keys_add_permissions/200_test.go at lines 81, 181, 267,
278, 363, 454, and 464, the permission IDs are created using uid.TestPrefix,
which is incorrect. Replace uid.TestPrefix with the appropriate
permission-specific prefix constant to ensure consistency and correctness in
permission ID generation.
| err = svc.Permissions.Check( | ||
| ctx, | ||
| auth.KeyID, | ||
| rbac.Or( | ||
| rbac.T(rbac.Tuple{ | ||
| ResourceType: rbac.Api, | ||
| ResourceID: "*", | ||
| Action: rbac.UpdateKey, | ||
| }), | ||
| ), | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add specific API permission check for better access control.
The permission check currently only validates wildcard API permissions. For better access control granularity, consider also checking permissions for the specific API that the key belongs to.
After retrieving the key (around line 80), add the specific API permission check:
err = svc.Permissions.Check(
ctx,
auth.KeyID,
rbac.Or(
rbac.T(rbac.Tuple{
ResourceType: rbac.Api,
ResourceID: "*",
Action: rbac.UpdateKey,
}),
+ rbac.T(rbac.Tuple{
+ ResourceType: rbac.Api,
+ ResourceID: key.KeyringID, // or appropriate API ID field
+ Action: rbac.UpdateKey,
+ }),
),
)📝 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.
| err = svc.Permissions.Check( | |
| ctx, | |
| auth.KeyID, | |
| rbac.Or( | |
| rbac.T(rbac.Tuple{ | |
| ResourceType: rbac.Api, | |
| ResourceID: "*", | |
| Action: rbac.UpdateKey, | |
| }), | |
| ), | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| err = svc.Permissions.Check( | |
| ctx, | |
| auth.KeyID, | |
| rbac.Or( | |
| rbac.T(rbac.Tuple{ | |
| ResourceType: rbac.Api, | |
| ResourceID: "*", | |
| Action: rbac.UpdateKey, | |
| }), | |
| rbac.T(rbac.Tuple{ | |
| ResourceType: rbac.Api, | |
| ResourceID: key.KeyringID, // or appropriate API ID field | |
| Action: rbac.UpdateKey, | |
| }), | |
| ), | |
| ) | |
| if err != nil { | |
| return err | |
| } |
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_keys_remove_permissions/handler.go around lines 50 to
63, the current permission check only validates wildcard API permissions, which
lacks granularity. After retrieving the key around line 80, add an additional
permission check for the specific API associated with the key by calling
svc.Permissions.Check with the exact ResourceID of that API. This ensures more
precise access control by verifying permissions not just for all APIs but also
for the specific API the key belongs to.
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
go/apps/api/routes/register.go (1)
62-412: 🛠️ Refactor suggestion
Registeris becoming monolithicAt ~400 LOC the function is difficult to scan and highly error-prone when new routes appear. Two low-risk options:
- Build a
[]zen.Routeslice and iterate once:routes := []zen.Route{ &v2Liveness.Handler{}, &v2RatelimitLimit.Handler{ … }, // … } for _, r := range routes { srv.RegisterRoute(defaultMiddlewares, r) }
- Split by domain – e.g.
registerRatelimitRoutes,registerIdentityRoutes, …Either keeps wiring logic local while regaining readability.
♻️ Duplicate comments (4)
go/apps/api/routes/v2_keys_add_roles/403_test.go (1)
218-234: Still using an “invalid” token instead of a real disabled key
Same concern as previously raised: this only exercises the token-not-found path, not the disabled branch. Insert a real key row and flipenabled=false(or setexpiresin the past) to hit the intended logic.go/apps/api/routes/v2_keys_add_roles/200_test.go (1)
310-312: Order-dependent slice assertion – brittle
Userequire.ElementsMatchto compare sets; the handler isn’t obliged to return roles in a deterministic order.go/apps/api/routes/v2_identities_list_identities/handler.go (1)
104-113:⚠️ Potential issuePerformance issue: N+1 query problem when fetching ratelimits.
The current implementation queries ratelimits for each identity individually in the loop, resulting in N+1 database queries. For the default limit of 100 identities, this would execute 101 queries.
go/apps/api/routes/v2_identities_update_identity/handler.go (1)
84-107:⚠️ Potential issueFix permission check when only externalId is provided.
When only
externalIdis provided, the permission check uses an empty string for the resource ID, which bypasses specific identity permission validation. The identity should be fetched first to get its ID for proper permission checking.
🧹 Nitpick comments (20)
go/apps/api/routes/v2_apis_list_keys/401_test.go (1)
31-71: Consider collapsing repetitive auth-error cases into a table-driven testEach sub-test duplicates header construction & call logic. A simple slice of
{name, header}cases with a loop will cut ~40 lines and make future additions cheaper.go/apps/api/routes/v2_keys_add_roles/200_test.go (1)
424-425: Same ordering issue in idempotent test- require.Equal(t, []string{"admin_idempotent", "editor_idempotent"}, roleNames) + require.ElementsMatch(t, + []string{"admin_idempotent", "editor_idempotent"}, + roleNames, + )go/apps/api/routes/v2_keys_remove_permissions/404_test.go (1)
20-364: Extensive fixture setup duplicated across sub-testsEach case re-creates keyrings, keys, permissions, etc. Extract helpers (
createKey,createPermission) or share common fixtures witht.Helper()to shorten the file and improve maintainability.go/apps/api/routes/v2_keys_remove_permissions/403_test.go (1)
92-111: Avoid duplicated header-construction boilerplateEvery sub-test re-creates the
http.Headermap with identical"Content-Type"and only a different bearer token. Extract a tiny helper to keep tests DRY and easier to read.-headers := http.Header{ - "Content-Type": {"application/json"}, - "Authorization": {fmt.Sprintf("Bearer %s", insufficientRootKey)}, -} +headers := newAuthHeaders(insufficientRootKey)// at top of file (outside any test) func newAuthHeaders(token string) http.Header { return http.Header{ "Content-Type": {"application/json"}, "Authorization": {fmt.Sprintf("Bearer %s", token)}, } }go/apps/api/routes/reference/handler.go (2)
12-16:Loggerfield is dead code
Handler.Loggeris exported but never referenced. Either wire logging inHandle(e.g.h.Logger.Info(...)) or drop the field to avoid confusion.
28-52: Constant-time template avoids repeatedfmt.SprintfallocationsThe HTML never changes at runtime; format-on-every-request allocates a large string each call. Cache it once:
-func (h *Handler) Handle(ctx context.Context, s *zen.Session) error { - html := fmt.Sprintf(` ... `, string(openapi.Spec)) +var referencePage = fmt.Sprintf(` ... `, string(openapi.Spec)) + +func (h *Handler) Handle(_ context.Context, s *zen.Session) error { + html := referencePagego/apps/api/routes/v2_apis_get_api/403_test.go (1)
71-92: Loop variable capture can bite when tests go parallel
tcfrom the outer loop is captured by the closure. If the table is ever switched tot.Parallel()the tests will race. Future-proof by re-binding:for _, tc := range testCases { tc := tc // shadow t.Run(tc.name, func(t *testing.T) { ... }) }go/apps/api/routes/v2_apis_list_keys/403_test.go (2)
36-45: Prefer UID constants for clarity
uid.New("keyauth")anduid.New("api")bypass the typed prefixes used elsewhere. Usinguid.KeyAuthPrefix/uid.APIPrefixcommunicates intent and avoids accidental prefix drift.-keyAuthID := uid.New("keyauth") +keyAuthID := uid.New(uid.KeyAuthPrefix) -apiID := uid.New("api") +apiID := uid.New(uid.APIPrefix)
60-84: Cut boilerplate with a helperThe pattern
rootKey := h.CreateRootKey(... perms ...) headers := newAuthHeaders(rootKey) req := handler.Request{ ApiId: apiID } res := testutil.CallRoute[handler.Request, openapi.ForbiddenErrorResponse](h, route, headers, req)is repeated many times. A small helper would shrink the file and sharpen what each test varies (permissions).
go/apps/api/routes/v2_identities_delete_identity/404_test.go (2)
225-238: Inconsistent error-payload assertionsAll other sub-tests also verify
res.Body.Error.Status, but this last case omits that check and instead assertsres.Statustwice (lines 229 & 234).
Consider adding the missing payload assertion for consistency and removing the duplicate status check:- require.Equal(t, http.StatusNotFound, res.Status) + require.Equal(t, http.StatusNotFound, res.Body.Error.Status)
37-66: Heavy duplication – consider a table-driven sub-test helperEvery sub-test repeats the same request/response plumbing and the same five assertions on the error object. A tiny helper (e.g.
assertIdentityNotFound(t, res)) or a table-driven loop would trim ~150 lines and make future test additions trivial.This is a readability / maintainability refactor; behaviour remains unchanged.
Also applies to: 67-224
go/apps/api/routes/v2_identities_update_identity/200_test.go (1)
41-44: Unnecessary manual transactionYou open a manual transaction (
tx := h.DB.RW().Begin) solely for fixture inserts and thenCommit().
The harness DB already wraps each test in a rollback-on-finish transaction. Dropping the explicitBegin/Commitreduces boilerplate without changing semantics.go/apps/api/routes/v2_apis_get_api/200_test.go (1)
170-238: Test size & duplicationThe file hand-rolls six near-identical “setup API → call route → assert 200” flows. Extracting a helper like
createApiAndAssert200(name string)or moving to a table-driven pattern would halve the LOC and clarify what really differs per case (name variance, unicode, etc.).Also applies to: 240-274
go/apps/api/routes/v2_identities_delete_identity/401_test.go (1)
29-46: Reduce repeated header constructionEvery sub-test re-creates
http.Headerwith identical"Content-Type"and only a varying"Authorization". Consider extracting a small helper (e.g.makeHeaders(token string) http.Header) to DRY up the test and make each case focus solely on what’s being validated.go/apps/api/routes/v2_keys_add_roles/404_test.go (2)
44-68: High duplication – switch to a table-driven testMost of the setup & assertions in the eight sub-tests are identical; only the request payload builder changes. Migrating to a table-driven test with a slice of anonymous structs (name, req, wantDetailContains …) would cut ~300 lines, speed up future maintenance, and make the intent clearer.
406-478: Helper functions for key / role fixturesKeyring, key and role creation logic is reproduced verbatim in multiple cases. Wrapping these in helpers inside the test package (e.g.
createKey(t, wsID) (keyID string)andcreateRole(t, wsID) (roleID string)) removes noise and reduces chances of diverging fixture data.go/apps/api/routes/v2_identities_delete_identity/400_test.go (1)
128-133: Skipped JSON-parsing negative testThe TODO comment highlights missing coverage for raw–body parse errors. With
httptestyou can issue a request containing{ invalid jsondirectly against the mux and keep the same harness. Adding this would close the remaining gap in 400-level validation.go/apps/api/routes/v2_apis_delete_api/handler.go (2)
182-183: Cache failure is silently ignored
h.Caches.ApiByID.SetNullreturns an error but it’s discarded. Logging (or at least a debug statement) would help diagnose cache-invalidation issues in production.
28-37: Add compile-time interface assertionFor early breakage detection:
type Handler struct { @@ Caches caches.Caches } + +// Ensure we still implement zen.Route when changing the struct. +var _ zen.Route = (*Handler)(nil)go/apps/api/routes/v2_apis_list_keys/handler.go (1)
318-330:Intervalis returned as an empty stringOpenAPI names an
intervalfield but the implementation always returns"".
Either populate it from the DB or omit the field to avoid confusing consumers.- k.Credits.Refill = &openapi.KeyCreditsRefill{ - Amount: int(key.Key.RefillAmount.Int32), - Interval: "", + k.Credits.Refill = &openapi.KeyCreditsRefill{ + Amount: int(key.Key.RefillAmount.Int32), + Interval: key.Key.RefillInterval.String, // assuming a column existsIf the column does not exist, consider removing the field from the schema or documenting that the value is always empty.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (84)
go/apps/api/routes/reference/handler.go(2 hunks)go/apps/api/routes/register.go(5 hunks)go/apps/api/routes/v2_apis_create_api/200_test.go(8 hunks)go/apps/api/routes/v2_apis_create_api/400_test.go(2 hunks)go/apps/api/routes/v2_apis_create_api/401_test.go(2 hunks)go/apps/api/routes/v2_apis_create_api/403_test.go(2 hunks)go/apps/api/routes/v2_apis_create_api/handler.go(1 hunks)go/apps/api/routes/v2_apis_delete_api/200_test.go(1 hunks)go/apps/api/routes/v2_apis_delete_api/400_test.go(1 hunks)go/apps/api/routes/v2_apis_delete_api/401_test.go(1 hunks)go/apps/api/routes/v2_apis_delete_api/403_test.go(1 hunks)go/apps/api/routes/v2_apis_delete_api/404_test.go(1 hunks)go/apps/api/routes/v2_apis_delete_api/412_test.go(1 hunks)go/apps/api/routes/v2_apis_delete_api/cache_validation_test.go(1 hunks)go/apps/api/routes/v2_apis_delete_api/handler.go(1 hunks)go/apps/api/routes/v2_apis_delete_api/idempotent_test.go(1 hunks)go/apps/api/routes/v2_apis_delete_api/special_chars_test.go(1 hunks)go/apps/api/routes/v2_apis_get_api/200_test.go(1 hunks)go/apps/api/routes/v2_apis_get_api/400_test.go(1 hunks)go/apps/api/routes/v2_apis_get_api/401_test.go(1 hunks)go/apps/api/routes/v2_apis_get_api/403_test.go(1 hunks)go/apps/api/routes/v2_apis_get_api/404_test.go(1 hunks)go/apps/api/routes/v2_apis_get_api/handler.go(1 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(1 hunks)go/apps/api/routes/v2_apis_list_keys/handler.go(1 hunks)go/apps/api/routes/v2_identities_create_identity/200_test.go(4 hunks)go/apps/api/routes/v2_identities_create_identity/400_test.go(3 hunks)go/apps/api/routes/v2_identities_create_identity/401_test.go(1 hunks)go/apps/api/routes/v2_identities_create_identity/403_test.go(1 hunks)go/apps/api/routes/v2_identities_create_identity/409_test.go(1 hunks)go/apps/api/routes/v2_identities_create_identity/handler.go(2 hunks)go/apps/api/routes/v2_identities_delete_identity/200_test.go(3 hunks)go/apps/api/routes/v2_identities_delete_identity/400_test.go(5 hunks)go/apps/api/routes/v2_identities_delete_identity/401_test.go(2 hunks)go/apps/api/routes/v2_identities_delete_identity/403_test.go(1 hunks)go/apps/api/routes/v2_identities_delete_identity/404_test.go(1 hunks)go/apps/api/routes/v2_identities_delete_identity/handler.go(2 hunks)go/apps/api/routes/v2_identities_get_identity/200_test.go(1 hunks)go/apps/api/routes/v2_identities_get_identity/400_test.go(1 hunks)go/apps/api/routes/v2_identities_get_identity/401_test.go(1 hunks)go/apps/api/routes/v2_identities_get_identity/403_test.go(1 hunks)go/apps/api/routes/v2_identities_get_identity/404_test.go(1 hunks)go/apps/api/routes/v2_identities_get_identity/handler.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/200_test.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/400_test.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/401_test.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/403_test.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/cross_workspace_test.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/handler.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/200_test.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/400_test.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/401_test.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/403_test.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/404_test.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/handler.go(1 hunks)go/apps/api/routes/v2_keys_add_permissions/200_test.go(1 hunks)go/apps/api/routes/v2_keys_add_permissions/400_test.go(1 hunks)go/apps/api/routes/v2_keys_add_permissions/401_test.go(1 hunks)go/apps/api/routes/v2_keys_add_permissions/403_test.go(1 hunks)go/apps/api/routes/v2_keys_add_permissions/404_test.go(1 hunks)go/apps/api/routes/v2_keys_add_permissions/handler.go(1 hunks)go/apps/api/routes/v2_keys_add_roles/200_test.go(1 hunks)go/apps/api/routes/v2_keys_add_roles/400_test.go(1 hunks)go/apps/api/routes/v2_keys_add_roles/401_test.go(1 hunks)go/apps/api/routes/v2_keys_add_roles/403_test.go(1 hunks)go/apps/api/routes/v2_keys_add_roles/404_test.go(1 hunks)go/apps/api/routes/v2_keys_add_roles/handler.go(1 hunks)go/apps/api/routes/v2_keys_create_key/200_test.go(1 hunks)go/apps/api/routes/v2_keys_create_key/400_test.go(1 hunks)go/apps/api/routes/v2_keys_create_key/401_test.go(1 hunks)go/apps/api/routes/v2_keys_create_key/403_test.go(1 hunks)go/apps/api/routes/v2_keys_create_key/404_test.go(1 hunks)go/apps/api/routes/v2_keys_create_key/handler.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/200_test.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/400_test.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/401_test.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/403_test.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/404_test.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/handler.go(1 hunks)go/apps/api/routes/v2_keys_remove_roles/200_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- go/apps/api/routes/v2_identities_create_identity/401_test.go
- go/apps/api/routes/v2_identities_create_identity/403_test.go
- go/apps/api/routes/v2_keys_add_permissions/handler.go
- go/apps/api/routes/v2_apis_get_api/handler.go
🚧 Files skipped from review as they are similar to previous changes (54)
- go/apps/api/routes/v2_apis_create_api/401_test.go
- go/apps/api/routes/v2_keys_create_key/400_test.go
- go/apps/api/routes/v2_keys_create_key/401_test.go
- go/apps/api/routes/v2_apis_create_api/400_test.go
- go/apps/api/routes/v2_keys_remove_permissions/200_test.go
- go/apps/api/routes/v2_identities_get_identity/401_test.go
- go/apps/api/routes/v2_identities_list_identities/401_test.go
- go/apps/api/routes/v2_keys_add_permissions/403_test.go
- go/apps/api/routes/v2_apis_delete_api/401_test.go
- go/apps/api/routes/v2_keys_add_permissions/401_test.go
- go/apps/api/routes/v2_identities_update_identity/401_test.go
- go/apps/api/routes/v2_apis_delete_api/412_test.go
- go/apps/api/routes/v2_identities_update_identity/404_test.go
- go/apps/api/routes/v2_apis_list_keys/200_test.go
- go/apps/api/routes/v2_apis_delete_api/special_chars_test.go
- go/apps/api/routes/v2_apis_delete_api/cache_validation_test.go
- go/apps/api/routes/v2_apis_get_api/401_test.go
- go/apps/api/routes/v2_identities_get_identity/404_test.go
- go/apps/api/routes/v2_apis_create_api/403_test.go
- go/apps/api/routes/v2_identities_update_identity/400_test.go
- go/apps/api/routes/v2_identities_update_identity/403_test.go
- go/apps/api/routes/v2_apis_create_api/200_test.go
- go/apps/api/routes/v2_apis_delete_api/idempotent_test.go
- go/apps/api/routes/v2_apis_delete_api/404_test.go
- go/apps/api/routes/v2_identities_get_identity/403_test.go
- go/apps/api/routes/v2_apis_get_api/404_test.go
- go/apps/api/routes/v2_identities_create_identity/400_test.go
- go/apps/api/routes/v2_keys_remove_permissions/401_test.go
- go/apps/api/routes/v2_keys_create_key/403_test.go
- go/apps/api/routes/v2_identities_get_identity/400_test.go
- go/apps/api/routes/v2_apis_get_api/400_test.go
- go/apps/api/routes/v2_apis_delete_api/400_test.go
- go/apps/api/routes/v2_identities_list_identities/200_test.go
- go/apps/api/routes/v2_keys_add_permissions/404_test.go
- go/apps/api/routes/v2_keys_remove_permissions/400_test.go
- go/apps/api/routes/v2_identities_list_identities/cross_workspace_test.go
- go/apps/api/routes/v2_apis_delete_api/200_test.go
- go/apps/api/routes/v2_keys_remove_roles/200_test.go
- go/apps/api/routes/v2_identities_list_identities/400_test.go
- go/apps/api/routes/v2_apis_delete_api/403_test.go
- go/apps/api/routes/v2_apis_create_api/handler.go
- go/apps/api/routes/v2_keys_add_permissions/400_test.go
- go/apps/api/routes/v2_identities_get_identity/200_test.go
- go/apps/api/routes/v2_apis_list_keys/400_test.go
- go/apps/api/routes/v2_keys_add_permissions/200_test.go
- go/apps/api/routes/v2_keys_create_key/200_test.go
- go/apps/api/routes/v2_identities_get_identity/handler.go
- go/apps/api/routes/v2_identities_delete_identity/handler.go
- go/apps/api/routes/v2_keys_create_key/404_test.go
- go/apps/api/routes/v2_keys_add_roles/400_test.go
- go/apps/api/routes/v2_keys_add_roles/handler.go
- go/apps/api/routes/v2_keys_create_key/handler.go
- go/apps/api/routes/v2_keys_remove_permissions/handler.go
- go/apps/api/routes/v2_apis_list_keys/404_test.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
go/apps/api/routes/v2_identities_create_identity/409_test.go (1)
go/apps/api/routes/v2_identities_create_identity/handler.go (1)
Handler(30-37)
go/apps/api/routes/v2_apis_get_api/200_test.go (10)
go/pkg/testutil/http.go (2)
NewHarness(53-178)CallRoute(235-269)go/apps/api/routes/v2_apis_get_api/handler.go (3)
Handler(22-28)Request(18-18)Response(19-19)go/pkg/uid/uid.go (2)
APIPrefix(20-20)KeyAuthPrefix(23-23)go/pkg/db/api_insert.sql_generated.go (1)
InsertApiParams(33-40)go/pkg/assert/equal.go (1)
Equal(17-26)go/pkg/assert/not_nil.go (1)
NotNil(16-25)go/pkg/db/models_generated.go (3)
NullApisAuthType(33-36)ApisAuthType(14-14)ApisAuthTypeKey(17-17)go/pkg/db/api_update_delete_protection.sql_generated.go (1)
UpdateApiDeleteProtectionParams(19-22)go/pkg/assert/true.go (1)
True(17-26)go/pkg/assert/false.go (1)
False(17-26)
🪛 Gitleaks (8.26.0)
go/apps/api/routes/v2_apis_list_keys/401_test.go
77-77: Found a Stripe Access Token, posing a risk to payment processing services and sensitive financial data.
(stripe-access-token)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Packages / Test ./internal/clickhouse
🔇 Additional comments (9)
go/apps/api/routes/v2_identities_create_identity/409_test.go (1)
18-24: Direct struct literal instantiation looks goodSwitching from
handler.New(...)to an explicit struct literal is perfectly fine here and keeps the test independent of any factory logic. No issues spotted.go/apps/api/routes/v2_keys_remove_permissions/403_test.go (1)
24-30: Unused service fields risk drifting out of sync
Handleris instantiated withoutLogger, diverging from other tests that passLogger. If the handler later starts using the logger, these tests will nil-panic. Recommend supplying the service for consistency:- Logger: h.Logger, + Logger: h.Logger,go/apps/api/routes/v2_identities_list_identities/403_test.go (1)
90-98: Test description mismatches exercised permission scopeThe sub-test is called
"with permission for only specific environment"but the granted permissionidentity.*.read_identityis wildcard, not environment-specific. Rename the test or narrow the permission (e.g.identity.production.read_identity) to avoid misleading readers.go/apps/api/routes/v2_apis_list_keys/403_test.go (1)
225-228: Assert the precise status you expectAllowing either 403 or 404 weakens the contract and can mask regressions. The comment states 404 is the desired masking strategy—assert it explicitly:
- require.True(t, res.Status == 403 || res.Status == 404) + require.Equal(t, 404, res.Status)go/apps/api/routes/v2_apis_get_api/200_test.go (1)
81-106: Wildcard permission expectation may driftYou intentionally expect
403when the key has"*"permission (line 88).
If the permission system later treats*as super-admin (which is common), this test will become a blocker.Please confirm the intended contract; if
*should grant access, adjust the expectation now to avoid future flips.go/apps/api/routes/v2_identities_create_identity/200_test.go (1)
370-387: Potential uniqueness collision across sub-testsThe constant external IDs
"123","test-id_with.special@characters!123"and the Unicode string are each used only once, so they’re fine today. If additional sub-tests reuse them, the unique(workspace_id, external_id)constraint will start failing because the harness shares one DB for all sub-tests in thist.Runchain. You may want to derive the value fromt.Name()oruid.New()to guard against future copy-paste collisions.go/apps/api/routes/v2_identities_delete_identity/400_test.go (1)
96-108: 404 vs 400 for malformed ID – double-check desired contractThe test expects a 404 when an obviously malformed
identityIdis supplied. Returning 400 (“invalid input”) would communicate the problem more accurately and avoid leaking existence information anyway. Confirm this behaviour matches the public API contract; if not, update either handler or test.go/apps/api/routes/v2_identities_delete_identity/403_test.go (1)
18-209: Excellent comprehensive test coverage for authorization scenarios.The test suite thoroughly covers all edge cases for insufficient permissions, including no permissions, wrong permissions, different resource types, specific vs wildcard permissions, read-only permissions, partial matches, and case sensitivity. The consistent error response validation ensures proper error handling across all scenarios.
go/apps/api/routes/v2_identities_delete_identity/200_test.go (1)
63-298: Well-structured test suite with comprehensive coverage.The test suite thoroughly covers successful deletion scenarios including:
- Deletion by both identity ID and external ID
- Soft deletion behavior validation
- Rate limit persistence after deletion
- Different permission scopes (wildcard and specific)
- Audit log verification
- Edge case handling for duplicate keys
The helper function
createTestIdentityis well-designed for test data setup.
| headers := http.Header{ | ||
| "Content-Type": {"application/json"}, | ||
| "Authorization": {"Bearer sk_test_1234567890abcdef1234567890abcdef"}, | ||
| } | ||
|
|
||
| res := testutil.CallRoute[handler.Request, openapi.UnauthorizedErrorResponse]( |
There was a problem hiding this comment.
Hard-coded Stripe-looking secret trips gitleaks
sk_test_1234567890abcdef1234567890abcdef is flagged by gitleaks as a Stripe access token.
Even in tests, this will fail CI / security scanning.
- "Authorization": {"Bearer sk_test_1234567890abcdef1234567890abcdef"},
+ // Intentionally invalid test token – looks nothing like a real Stripe key
+ "Authorization": {"Bearer test_invalid_key_format_1"},📝 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.
| headers := http.Header{ | |
| "Content-Type": {"application/json"}, | |
| "Authorization": {"Bearer sk_test_1234567890abcdef1234567890abcdef"}, | |
| } | |
| res := testutil.CallRoute[handler.Request, openapi.UnauthorizedErrorResponse]( | |
| headers := http.Header{ | |
| "Content-Type": {"application/json"}, | |
| // Intentionally invalid test token – looks nothing like a real Stripe key | |
| "Authorization": {"Bearer test_invalid_key_format_1"}, | |
| } | |
| res := testutil.CallRoute[handler.Request, openapi.UnauthorizedErrorResponse]( |
🧰 Tools
🪛 Gitleaks (8.26.0)
77-77: Found a Stripe Access Token, posing a risk to payment processing services and sensitive financial data.
(stripe-access-token)
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_apis_list_keys/401_test.go around lines 75 to 80, the
test code uses a hard-coded Stripe secret key string which triggers gitleaks
security scanning failures. Replace the hard-coded secret with a non-sensitive
placeholder or mock value that does not resemble a real Stripe key to avoid
CI/security scan issues while preserving test functionality.
| t.Run("update metadata by identityId", func(t *testing.T) { | ||
| newMeta := map[string]interface{}{ | ||
| "name": "Updated User", | ||
| "email": "updated@example.com", | ||
| "plan": "pro", | ||
| "credits": 100, | ||
| } | ||
|
|
||
| req := handler.Request{ | ||
| IdentityId: &identityID, | ||
| Meta: &newMeta, | ||
| } | ||
| res := testutil.CallRoute[handler.Request, handler.Response](h, route, headers, req) | ||
| require.Equal(t, 200, res.Status, "expected 200, sent: %+v, received: %s", req, res.RawBody) | ||
| require.NotNil(t, res.Body) | ||
|
|
||
| // Verify response | ||
| require.Equal(t, identityID, res.Body.Data.Id) | ||
| require.Equal(t, externalID, res.Body.Data.ExternalId) | ||
|
|
||
| // Verify metadata | ||
| require.NotNil(t, res.Body.Data.Meta) | ||
| meta := *res.Body.Data.Meta | ||
| assert.Equal(t, "Updated User", meta["name"]) | ||
| assert.Equal(t, "updated@example.com", meta["email"]) | ||
| assert.Equal(t, "pro", meta["plan"]) | ||
| assert.Equal(t, float64(100), meta["credits"]) | ||
|
|
||
| // Verify ratelimits remain unchanged | ||
| require.NotNil(t, res.Body.Data.Ratelimits) | ||
| require.Len(t, *res.Body.Data.Ratelimits, 2) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sub-tests share and mutate the same identity – order dependency risk
identityID is updated, cleared, and re-populated across multiple sub-tests.
If someone adds t.Parallel() to any case, or re-orders them, tests will start to flicker.
Safest pattern: create fresh fixtures per sub-test (or clone via helper) so cases are isolated and order-independent.
This also avoids hidden coupling between metadata / ratelimit expectations.
Also applies to: 172-231, 233-271, 273-309
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_identities_update_identity/200_test.go around lines 110
to 141, the sub-tests share and mutate the same identityID variable, causing
order dependency and potential test flakiness if tests run in parallel or are
reordered. To fix this, create fresh identityID fixtures or clone the identity
data within each sub-test to ensure isolation and independence. Apply this
pattern consistently to the other indicated line ranges (172-231, 233-271,
273-309) to avoid hidden coupling and maintain reliable tests.
| // Test case for disabled root key | ||
| t.Run("disabled root key", func(t *testing.T) { | ||
| // Use invalid root key to simulate disabled key | ||
| rootKey := "invalid_disabled_key" | ||
|
|
||
| headers := http.Header{ | ||
| "Content-Type": {"application/json"}, | ||
| "Authorization": {fmt.Sprintf("Bearer %s", rootKey)}, | ||
| } | ||
|
|
||
| res := testutil.CallRoute[handler.Request, openapi.UnauthorizedErrorResponse]( | ||
| h, | ||
| route, | ||
| headers, | ||
| req, | ||
| ) | ||
|
|
||
| require.Equal(t, 401, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| require.NotNil(t, res.Body.Error) | ||
| }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
“Disabled” root-key scenario does not actually disable a key
The test feeds an arbitrary string (invalid_disabled_key) instead of inserting an existing key and toggling enabled = false.
This validates “unknown key” rather than “disabled key”. If the handler ever distinguishes those cases the test will give a false sense of coverage.
Recommend inserting a real root key with enabled=false and asserting the dedicated error path.
Let’s inspect both the “disabled root key” and the “valid root key” sections to see how real keys are handled:
🏁 Script executed:
#!/bin/bash
file="go/apps/api/routes/v2_keys_add_roles/401_test.go"
echo "== Disabled root key snippet =="
rg -n "disabled root key" -A5 -B5 "$file" || true
echo
echo "== Valid root key snippet =="
rg -n "valid root key" -A5 -B5 "$file" || trueLength of output: 1189
I’d like to confirm how the handler treats a disabled key versus an unknown key before finalizing. Specifically, we need to see whether the code path for a found-but-disabled key produces a distinct error. Let’s locate the authentication logic in the v2_keys_add_roles handler:
If that points us to the file, we can then pull in the authentication check:
This will let us confirm if there’s a distinct “disabled” branch or if everything falls under “unauthorized.”
🏁 Script executed:
#!/bin/bash
# Find the handler implementation for v2_keys_add_roles
rg -l "func .*AddRoles" -g "go/apps/api/routes/v2_keys_add_roles/*.go"Length of output: 72
🏁 Script executed:
#!/bin/bash
# Suppose the handler is in handler.go; inspect authentication section
sed -n '1,200p' go/apps/api/routes/v2_keys_add_roles/handler.go | sed -n '200,350p'Length of output: 107
I wasn’t able to locate the auth logic for disabled keys—let’s list the route directory and search for “enabled”/“disabled” in its handler to confirm whether a distinct disabled-key path exists:
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Files in v2_keys_add_roles ==="
ls -1 go/apps/api/routes/v2_keys_add_roles
echo && echo "=== Search for key-enabled checks in handler.go ==="
rg -n "enabled" go/apps/api/routes/v2_keys_add_roles/handler.go || true
echo && echo "=== Search for disabled references in handler.go ==="
rg -n "disabled" go/apps/api/routes/v2_keys_add_roles/handler.go || trueLength of output: 590
Simulated “disabled” root-key test doesn’t actually verify a disabled key
The sub-test at go/apps/api/routes/v2_keys_add_roles/401_test.go:200–220 uses the arbitrary string "invalid_disabled_key", which only exercises the “unknown key” path. As soon as you add a distinct “key disabled” error, this will give a false sense of coverage.
• File: go/apps/api/routes/v2_keys_add_roles/401_test.go (lines 200–220)
• Problem: using a non-existent key instead of a real root key with Enabled=false
• Impact: duplicate coverage of “unknown key” rather than a true “disabled” case
Recommended fixes:
- Insert a real root key in your test setup with
Enabled=false(via your test store or helper) and use its value in theAuthorizationheader. - Assert the specific disabled-key error path, e.g.:
require.Equal(t, 401, res.Status)require.Contains(t, res.Body.Error.Detail, "disabled")
If you haven’t implemented disabled-key support yet, either rename/remove this sub-test (to avoid misleading coverage) or add the proper disabled-key logic before revisiting this test.
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_keys_add_roles/401_test.go around lines 200 to 220, the
test for the "disabled root key" uses a fake key string which only tests the
unknown key scenario, not a truly disabled key. To fix this, modify the test
setup to insert a real root key with Enabled set to false, then use that key's
value in the Authorization header. Update the assertions to check for the
specific disabled-key error response, such as confirming the error detail
contains "disabled" along with the 401 status. If disabled-key handling is not
implemented yet, remove or rename this test to avoid misleading coverage.
| err = s.BindBody(&req) | ||
| if err != nil { | ||
| return fault.Wrap(err, | ||
| fault.Internal("invalid request body"), fault.Public("The request body is invalid."), | ||
| ) |
There was a problem hiding this comment.
Missing fault code causes wrong HTTP mapping
fault.Wrap is called without fault.Code(...), so the middleware cannot map the error to a 400 response.
-return fault.Wrap(err,
- fault.Internal("invalid request body"), fault.Public("The request body is invalid."),
-)
+return fault.Wrap(err,
+ fault.Code(codes.App.Application.InvalidInput.URN()),
+ fault.Internal("invalid request body"),
+ fault.Public("The request body is invalid."),
+)Without this, clients may see a 500 instead of a proper BadRequest envelope.
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_apis_delete_api/handler.go around lines 57 to 61, the
error returned by fault.Wrap lacks a fault.Code specifying the error type,
causing the middleware to misclassify the error and respond with HTTP 500
instead of 400. Fix this by adding fault.Code(fault.InvalidArgument) or an
appropriate fault code to the fault.Wrap call so the middleware correctly maps
the error to a BadRequest (400) response.
| limit := ptr.SafeDeref(req.Limit, 100) | ||
| cursor := ptr.SafeDeref(req.Cursor, "") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clamp limit to a sensible maximum
ptr.SafeDeref(req.Limit, 100) trusts user input completely; a very large value can exhaust memory and cause long DB queries.
const defaultLimit = 100
const maxLimit = 500 // tweak to your SLA
limit := ptr.SafeDeref(req.Limit, defaultLimit)
if limit > maxLimit {
limit = maxLimit
}🤖 Prompt for AI Agents
In go/apps/api/routes/v2_apis_list_keys/handler.go around lines 167 to 168, the
limit value derived from user input is not capped, which can lead to excessive
memory use and long database queries. Modify the code to define a maximum limit
constant (e.g., maxLimit = 500) and clamp the limit variable to this maximum
after dereferencing it. This ensures the limit never exceeds the sensible
maximum, protecting system resources.
| // Get permissions for the key | ||
| permissionSlugs, err := db.Query.ListPermissionsByKeyID(ctx, h.DB.RO(), db.ListPermissionsByKeyIDParams{ | ||
| KeyID: k.KeyId, | ||
| }) | ||
| if err != nil { | ||
| return fault.Wrap(err, fault.Code(codes.App.Internal.UnexpectedError.URN()), | ||
| fault.Internal("unable to find permissions for key"), fault.Public("Could not load permissions for key.")) | ||
| } | ||
| k.Permissions = ptr.P(permissionSlugs) | ||
|
|
||
| // Get roles for the key | ||
| roles, err := db.Query.ListRolesByKeyID(ctx, h.DB.RO(), k.KeyId) | ||
| if err != nil { | ||
| return fault.Wrap(err, fault.Code(codes.App.Internal.UnexpectedError.URN()), | ||
| fault.Internal("unable to find roles for key"), fault.Public("Could not load roles for key.")) | ||
| } | ||
|
|
||
| roleNames := make([]string, len(roles)) | ||
| for i, role := range roles { | ||
| roleNames[i] = role.Name | ||
| } | ||
|
|
||
| k.Roles = ptr.P(roleNames) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid N+1 queries for permissions and roles
ListPermissionsByKeyID and ListRolesByKeyID are executed inside the loop, generating up to 2×limit extra round-trips.
Batch fetch once and build a map keyed by key_id, e.g.:
permRows, _ := db.Query.ListPermissionsByKeyIDs(ctx, h.DB.RO(), keyIDs)
roleRows, _ := db.Query.ListRolesByKeyIDs(ctx, h.DB.RO(), keyIDs)
// build maps[keyID][]string …This single optimisation removes O(N) queries and materially improves latency for large pages.
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_apis_list_keys/handler.go around lines 361 to 383, the
code fetches permissions and roles for each key inside a loop, causing N+1 query
issues and performance degradation. Refactor by batching the database calls:
first collect all key IDs, then call ListPermissionsByKeyIDs and
ListRolesByKeyIDs once each to retrieve all permissions and roles in bulk. Build
maps from key ID to permissions and roles, then assign these to each key outside
the loop to eliminate repeated queries and improve latency.
| limit := ptr.SafeDeref(req.Limit, 100) | ||
| cursor := ptr.SafeDeref(req.Cursor, "") | ||
| // List keys | ||
| keys, err := db.Query.ListKeysByKeyAuthID( | ||
| ctx, | ||
| h.DB.RO(), | ||
| db.ListKeysByKeyAuthIDParams{ | ||
| KeyAuthID: api.KeyAuthID.String, | ||
| Limit: int32(limit + 1), | ||
| IDCursor: cursor, | ||
| IdentityID: sql.NullString{Valid: identityId != "", String: identityId}, | ||
| }, |
There was a problem hiding this comment.
Off-by-one pagination when a cursor is supplied
limit+1 is fetched from the DB, but one element (the cursor itself) is later removed.
If there are exactly limit records after the cursor, hasMore is computed as false even though more data exists.
-keys, err := db.Query.ListKeysByKeyAuthID(
+extra := 1
+if cursor != "" {
+ // we drop the cursor later – fetch one more so we can still detect “has more”
+ extra = 2
+}
+keys, err := db.Query.ListKeysByKeyAuthID(
ctx,
h.DB.RO(),
db.ListKeysByKeyAuthIDParams{
KeyAuthID: api.KeyAuthID.String,
- Limit: int32(limit + 1),
+ Limit: int32(limit + extra),
IDCursor: cursor,
IdentityID: sql.NullString{Valid: identityId != "", String: identityId},
},
)Keep the rest of the algorithm unchanged; hasMore will now be correct for both cursor and non-cursor requests.
Also applies to: 255-273
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_apis_list_keys/handler.go around lines 167 to 178, the
code fetches limit+1 records from the database to determine if more data exists,
but later removes one element assuming it is the cursor itself, causing
off-by-one errors in pagination when a cursor is supplied. To fix this, adjust
the logic so that when a cursor is provided, fetch exactly limit records without
adding one, and only add one to the limit when no cursor is supplied. This
preserves the existing algorithm and ensures hasMore is computed correctly for
both cursor and non-cursor requests. Apply the same fix to lines 255-273 as
well.
| var req Request | ||
| err = s.BindBody(&req) | ||
| if err != nil { | ||
| return fault.Wrap(err, | ||
| fault.Internal("invalid request body"), fault.Public("The request body is invalid."), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Return a 400/InvalidArgument instead of 500/Internal for malformed bodies
The request-body parsing failure is a client error, not a server error. Using fault.Internal masks the real cause and returns a 5xx to the caller.
- return fault.Wrap(err,
- fault.Internal("invalid request body"), fault.Public("The request body is invalid."),
- )
+ return fault.Wrap(err,
+ fault.Code(codes.App.InvalidArgument.URN()),
+ fault.Internal("invalid request body"), fault.Public("The request body is invalid."),
+ )📝 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.
| var req Request | |
| err = s.BindBody(&req) | |
| if err != nil { | |
| return fault.Wrap(err, | |
| fault.Internal("invalid request body"), fault.Public("The request body is invalid."), | |
| ) | |
| } | |
| var req Request | |
| err = s.BindBody(&req) | |
| if err != nil { | |
| - return fault.Wrap(err, | |
| - fault.Internal("invalid request body"), fault.Public("The request body is invalid."), | |
| - ) | |
| + return fault.Wrap(err, | |
| + fault.Code(codes.App.InvalidArgument.URN()), | |
| + fault.Internal("invalid request body"), fault.Public("The request body is invalid."), | |
| + ) | |
| } |
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_apis_list_keys/handler.go around lines 56 to 62, the
error handling for a malformed request body currently returns a 500 Internal
error using fault.Internal, which is incorrect. Change the error classification
to fault.InvalidArgument or a similar 400-level client error to properly
indicate the client-side issue. Update the fault.Wrap call to use
fault.InvalidArgument instead of fault.Internal to reflect that the error is due
to an invalid request body.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
go/apps/api/routes/v2_keys_add_roles/403_test.go (1)
248-252: Expired / disabled key path still uses an invalid token stringThis mirrors the earlier feedback: assigning
rootKey := "expired_root_key_12345"exercises the token-not-found branch, not the expired branch you intend to test. To truly hit the expiry logic, insert a real root-key row and setexpires < noworenabled = false.go/apps/api/routes/v2_apis_create_api/400_test.go (1)
138-153: “Unexpected fields” test remains non-deterministicThe test still accepts both 200 and 400, so it can never fail. Please assert one definitive outcome in line with the schema contract (likely
400 BadRequestplus a specific error about the unexpected fields).
This is identical to the feedback already given on a previous commit.go/apps/api/routes/v2_keys_remove_permissions/400_test.go (1)
180-188: Useuid.PermissionPrefixinstead ofuid.TestPrefixfor permission IDs – issue already raisedPrevious reviews flagged this exact problem; the prefix is still wrong.
Using the generic test prefix produces IDs liketest_…, which are invalid for permissions in production code and may bypass validation that key routes rely on.-permissionID := uid.New(uid.TestPrefix) +permissionID := uid.New(uid.PermissionPrefix)Apply the same change to every place where a permission ID is generated in this file (lines indicated above).
Also applies to: 214-224, 268-276
go/apps/api/routes/v2_keys_remove_permissions/200_test.go (1)
81-89: Still using the wrong prefix for permission IDsAll these blocks create permissions with
uid.TestPrefix; production-style permission IDs requireuid.PermissionPrefix. This has been pointed out in prior reviews and remains unfixed.-permissionID := uid.New(uid.TestPrefix) +permissionID := uid.New(uid.PermissionPrefix)Please update every occurrence in the ranges above to avoid silently introducing invalid IDs into tests.
Also applies to: 186-194, 272-292, 465-482, 569-596
go/apps/api/routes/v2_identities_update_identity/200_test.go (1)
110-141: Sub-tests share & mutate the same fixtures – order-dependency riskThe suite re-uses
identityIDand its ratelimits across multiple sub-tests, each of which mutates DB state (e.g. adds/updates/deletes ratelimits, clears metadata).
If someone later tags any case witht.Parallel()or simply re-orders them, tests will flicker or fail because they implicitly rely on side-effects from previous cases.Prefer one of:
- Build fresh fixtures inside each sub-test (or via a helper) so cases are fully isolated, or
- Wrap each sub-test in its own transaction and roll it back, or
- Spin a new
Harnessper sub-test.This mirrors feedback left on earlier commits.
Also applies to: 172-232, 234-252, 254-272, 274-310
go/apps/api/routes/v2_apis_list_keys/200_test.go (1)
137-140:⚠️ Potential issueStill swallowing possible
json.MarshalerrorsThe test continues to ignore the error returned from
json.Marshal, so ifkeyData.metaever contains values that cannot be marshalled (e.g., a channel or a recursive structure) the test will silently proceed with corrupt test-data.- if keyData.meta != nil { - metaBytes, _ = json.Marshal(keyData.meta) - } + if keyData.meta != nil { + var err error + metaBytes, err = json.Marshal(keyData.meta) + require.NoError(t, err) + }go/apps/api/routes/v2_apis_list_keys/handler.go (3)
55-61: Return a 400 instead of 500 for malformed request bodiesThe client sent an invalid JSON body – that’s a
codes.App.InvalidArgument, not an internal server error.- return fault.Wrap(err, - fault.Internal("invalid request body"), fault.Public("The request body is invalid."), - ) + return fault.Wrap(err, + fault.Code(codes.App.InvalidArgument.URN()), + fault.Internal("invalid request body"), fault.Public("The request body is invalid."), + )
358-374: N+1 queries for permissions & roles – batch instead
ListPermissionsByKeyIDandListRolesByKeyIDare called once per key, multiplying DB round-trips by page size. Fetch them in a single call each and build maps keyed bykey_idto populate the response.This will materially improve latency on large pages.
166-175: 🛠️ Refactor suggestionUnbounded
limitand possible pagination off-by-one
ptr.SafeDeref(req.Limit, 100)is fed straight into the query. A malicious caller can ask for an unreasonably large page and hurt DB latency & memory.- You always fetch
limit+1rows, even when a cursor is supplied; if the underlying query already omits the cursor row this loses one real record.const defaultLimit = 100 const maxLimit = 500 limit := ptr.SafeDeref(req.Limit, defaultLimit) if limit > maxLimit { limit = maxLimit } -extra := 1 -if cursor != "" { // when we later drop the cursor itself fetch one extra - extra = 2 -} ... - Limit: int32(limit + 1), + Limit: int32(limit + extra),
🧹 Nitpick comments (11)
go/apps/api/routes/v2_keys_add_roles/403_test.go (3)
20-33: Shared harness reused across all sub-tests – risk of state leakage
h := testutil.NewHarness(t)is created once and then reused by everyt.Run.
Because sub-tests run sequentially but share the same in-memory DB connection, inserts performed in one sub-test remain visible to the next. Future edits (e.g. addingt.Parallel()) or additional assertions could introduce hard-to-reproduce flakiness.Consider isolating state per sub-test:
- h := testutil.NewHarness(t) + // initialise a fresh harness for each scenario to avoid cross-test coupling + newHarness := func(tb testing.TB) *testutil.Harness { + return testutil.NewHarness(tb) + }and invoke
h := newHarness(t)inside everyt.Run.
35-75: Heavy duplication – factor helper for keyring/key setupEach scenario re-creates almost identical keyring + key boilerplate (≈40 LOC).
A small helper (e.g.createTestKey(t, h, wsID) (keyID string)) would:
- Shrink the test by >60 %.
- Surface the intent of each case (permission / workspace / expiry) without noise.
- Reduce maintenance when schema changes.
Also applies to: 105-147, 183-217, 253-287
76-84: Inline anonymousRolesstruct hides intentDeclaring the
Rolesslice with an anonymous struct each time obscures type information and repeats code. Prefer reusing an exported request DTO (if generated by openapi) or define a small helperroleID("role_123")to make the payload clearer and DRY.Also applies to: 149-157, 219-227, 289-297
go/apps/api/routes/v2_apis_create_api/400_test.go (1)
83-85: Swap expected & actual arguments inrequire.Equal
require.Equalexpects the “expected” value first and the “actual” value second.
The current order is reversed, which can make failure messages confusing.- require.Equal(t, res.Body.Error.Title, "Bad Request") + require.Equal(t, "Bad Request", res.Body.Error.Title)go/apps/api/routes/v2_keys_remove_permissions/200_test.go (1)
44-78: Heavy duplication – extract helper builders for keyring/key/permission setupEach sub-test repetitively copies ~30 lines to create a keyring, key, and optionally attach permissions. This balloons the file size and obscures intent.
Recommended light refactor:
func createTestKey(t *testing.T, h *testutil.Harness, wsID string) (keyID string, keyringID string) { keyringID = uid.New(uid.KeyAuthPrefix) require.NoError(t, db.Query.InsertKeyring(ctx, h.DB.RW(), db.InsertKeyringParams{ … })) keyID = uid.New(uid.KeyPrefix) keyString := "test_" + uid.New("") require.NoError(t, db.Query.InsertKey(ctx, h.DB.RW(), db.InsertKeyParams{ … })) return keyID, keyringID }Then each test becomes a few lines:
keyID, _ := createTestKey(t, h, workspace.ID) permID := uid.New(uid.PermissionPrefix) insertPermission(t, h, workspace.ID, permID, "documents.read…")This keeps the behavioural focus of each test crisp and slashes maintenance overhead.
Also applies to: 148-183, 234-269, 340-375, 426-462, 530-565
go/apps/api/routes/v2_identities_get_identity/200_test.go (2)
38-64: Missing rollback/cleanup between identity insertsAll identities are inserted directly into the main RW connection and committed.
Because the sameHarnessinstance is reused for the entire file, data piles up across sub-tests. The current assertions work only because IDs are unique, but the DB will keep growing and may slow large test suites.Consider wrapping each “setup” block in a transaction that you roll back after the sub-test – or create a fresh harness per sub-test.
This keeps the database small and each test hermetic.
225-257: “environment” setup is unassertedYou insert an identity with
Environment: customEnvironment, then explicitly note that the environment is not returned.
If the field is intentionally omitted from the response, no need to perform this setup – it adds noise and slows the test.
If the field should be returned, add an assertion so the test protects the contract.Clarify the intention and adjust accordingly.
go/apps/api/routes/v2_identities_delete_identity/401_test.go (1)
29-45: Repeated error-response assertions – extract helperThe same 6–7 assertions are duplicated in every sub-test (
Status,Type,Title,Detail,RequestId).
Factor them into a small helper, e.g.:func assertError(t *testing.T, res testutil.TestResponse[openapi.BaseErrorResponse], wantStatus int, wantType string) { require.Equal(t, wantStatus, res.Status) require.Equal(t, wantType, res.Body.Error.Type) require.NotEmpty(t, res.Body.Meta.RequestId) }It will shrink each case to three lines and ease future maintenance.
go/apps/api/routes/v2_identities_delete_identity/404_test.go (1)
65-107: Potential data-leak across workspacesThe test correctly verifies that an identity cannot be deleted from another workspace.
However, the follow-upFindIdentityByIDquery does not filter byWorkspaceID; if another test happened to create an identity with the same ID in a different workspace (unlikely but possible with UUID collisions or fixtures), this assertion would pass even though the wrong row is returned.Add
WorkspaceIDto the lookup to harden the check:identity, err := db.Query.FindIdentityByID(ctx, h.DB.RO(), db.FindIdentityByIDParams{ ID: identityId, WorkspaceID: h.Resources().UserWorkspace.ID, Deleted: false, })go/apps/api/routes/v2_identities_delete_identity/403_test.go (1)
31-67: High duplication of permission-failure casesEight sub-tests differ only in the permission string passed to
CreateRootKey.
Consider table-driving the cases:cases := []struct{ name string perms []string }{ {"no perms", nil}, {"wrong perms", []string{"identity.*.create_identity"}}, ... } for _, c := range cases { t.Run(c.name, func(t *testing.T){ ... }) }This cuts ~150 lines, keeps all scenarios, and makes future additions trivial.
MIGRATION_V1_TO_V2.md (1)
274-276: Minor style nit – drop the colon after “for”“Change GET requests to POST for:
getKey, …” violates the RP_COLON rule flagged by LanguageTool. Either delete the colon or insert a noun after “for”.- - Change GET requests to POST for: `getKey`, `getApi`, `listKeys` + - Change GET requests to POST for `getKey`, `getApi`, `listKeys`🧰 Tools
🪛 LanguageTool
[typographical] ~275-~275: Do not use a colon (:) before a series that is introduced by a preposition (‘for’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...t Methods - Change GET requests to POST for:getKey,getApi,listKeys- Move q...(RP_COLON)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
MIGRATION_V1_TO_V2.md(1 hunks)apps/docs/api-reference/errors-v2/unkey/application/protected_resource.mdx(2 hunks)go/.golangci.yaml(1 hunks)go/apps/api/openapi/generate.go(1 hunks)go/apps/api/openapi/openapi.json(0 hunks)go/apps/api/openapi/spec.go(1 hunks)go/apps/api/routes/v2_apis_create_api/200_test.go(10 hunks)go/apps/api/routes/v2_apis_create_api/400_test.go(4 hunks)go/apps/api/routes/v2_apis_create_api/401_test.go(2 hunks)go/apps/api/routes/v2_apis_create_api/403_test.go(5 hunks)go/apps/api/routes/v2_apis_create_api/handler.go(4 hunks)go/apps/api/routes/v2_apis_delete_api/handler.go(1 hunks)go/apps/api/routes/v2_apis_list_keys/200_test.go(1 hunks)go/apps/api/routes/v2_apis_list_keys/handler.go(1 hunks)go/apps/api/routes/v2_identities_create_identity/400_test.go(6 hunks)go/apps/api/routes/v2_identities_create_identity/handler.go(7 hunks)go/apps/api/routes/v2_identities_delete_identity/400_test.go(5 hunks)go/apps/api/routes/v2_identities_delete_identity/401_test.go(2 hunks)go/apps/api/routes/v2_identities_delete_identity/403_test.go(1 hunks)go/apps/api/routes/v2_identities_delete_identity/404_test.go(1 hunks)go/apps/api/routes/v2_identities_delete_identity/handler.go(6 hunks)go/apps/api/routes/v2_identities_get_identity/200_test.go(1 hunks)go/apps/api/routes/v2_identities_get_identity/handler.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/400_test.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/handler.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/200_test.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/handler.go(1 hunks)go/apps/api/routes/v2_keys_add_permissions/handler.go(1 hunks)go/apps/api/routes/v2_keys_add_roles/403_test.go(1 hunks)go/apps/api/routes/v2_keys_add_roles/handler.go(1 hunks)go/apps/api/routes/v2_keys_create_key/400_test.go(1 hunks)go/apps/api/routes/v2_keys_create_key/401_test.go(1 hunks)go/apps/api/routes/v2_keys_create_key/403_test.go(1 hunks)go/apps/api/routes/v2_keys_create_key/404_test.go(1 hunks)go/apps/api/routes/v2_keys_create_key/handler.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/200_test.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/400_test.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/401_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- go/apps/api/openapi/openapi.json
✅ Files skipped from review due to trivial changes (4)
- go/apps/api/openapi/generate.go
- apps/docs/api-reference/errors-v2/unkey/application/protected_resource.mdx
- go/.golangci.yaml
- go/apps/api/openapi/spec.go
🚧 Files skipped from review as they are similar to previous changes (21)
- go/apps/api/routes/v2_apis_create_api/401_test.go
- go/apps/api/routes/v2_keys_create_key/401_test.go
- go/apps/api/routes/v2_apis_create_api/403_test.go
- go/apps/api/routes/v2_keys_create_key/400_test.go
- go/apps/api/routes/v2_keys_create_key/403_test.go
- go/apps/api/routes/v2_apis_create_api/200_test.go
- go/apps/api/routes/v2_keys_remove_permissions/401_test.go
- go/apps/api/routes/v2_keys_create_key/404_test.go
- go/apps/api/routes/v2_identities_list_identities/400_test.go
- go/apps/api/routes/v2_apis_create_api/handler.go
- go/apps/api/routes/v2_identities_get_identity/handler.go
- go/apps/api/routes/v2_apis_delete_api/handler.go
- go/apps/api/routes/v2_identities_delete_identity/400_test.go
- go/apps/api/routes/v2_keys_add_roles/handler.go
- go/apps/api/routes/v2_keys_create_key/handler.go
- go/apps/api/routes/v2_identities_list_identities/handler.go
- go/apps/api/routes/v2_identities_create_identity/400_test.go
- go/apps/api/routes/v2_identities_create_identity/handler.go
- go/apps/api/routes/v2_identities_delete_identity/handler.go
- go/apps/api/routes/v2_identities_update_identity/handler.go
- go/apps/api/routes/v2_keys_add_permissions/handler.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
go/apps/api/routes/v2_identities_delete_identity/401_test.go (5)
go/apps/api/routes/v2_identities_create_identity/handler.go (2)
Handler(29-36)Request(25-25)go/apps/api/routes/v2_identities_delete_identity/handler.go (2)
Handler(91-98)Request(87-87)go/apps/api/routes/register.go (1)
Register(46-433)go/pkg/testutil/http.go (1)
CallRoute(236-270)go/apps/api/openapi/gen.go (4)
BadRequestErrorResponse(123-129)Meta(564-567)UnauthorizedErrorResponse(863-869)NotFoundErrorResponse(570-576)
go/apps/api/routes/v2_identities_update_identity/200_test.go (6)
go/pkg/testutil/http.go (2)
NewHarness(53-179)CallRoute(236-270)go/apps/api/routes/v2_identities_update_identity/handler.go (3)
Handler(29-36)Request(25-25)Response(26-26)go/pkg/uid/uid.go (2)
IdentityPrefix(30-30)RatelimitPrefix(31-31)go/apps/api/openapi/gen.go (1)
Ratelimit(665-698)go/pkg/assert/equal.go (1)
Equal(17-26)go/pkg/assert/not_nil.go (1)
NotNil(16-25)
🪛 LanguageTool
MIGRATION_V1_TO_V2.md
[typographical] ~275-~275: Do not use a colon (:) before a series that is introduced by a preposition (‘for’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...t Methods - Change GET requests to POST for: getKey, getApi, listKeys - Move q...
(RP_COLON)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Build / Build
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Analyze (javascript-typescript)
| t.Run("missing authorization", func(t *testing.T) { | ||
| headers := http.Header{ | ||
| "Content-Type": {"application/json"}, | ||
| // No Authorization header | ||
| } | ||
|
|
||
| req := handler.Request{ | ||
| Name: "test-api", | ||
| } | ||
|
|
||
| res := testutil.CallRoute[handler.Request, handler.Response](h, route, headers, req) | ||
| require.Equal(t, http.StatusBadRequest, res.Status, "expected 400 when authorization header is missing") | ||
| }) |
There was a problem hiding this comment.
Expect 401 Unauthorized when the Authorization header is missing
A request without credentials should yield 401 Unauthorized, not 400 BadRequest.
Keeping the wrong expectation will mask real auth regressions.
- require.Equal(t, http.StatusBadRequest, res.Status, "expected 400 when authorization header is missing")
+ require.Equal(t, http.StatusUnauthorized, res.Status, "expected 401 when authorization header is missing")Consider also validating the error response type (openapi.UnauthorizedErrorResponse) for completeness.
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_apis_create_api/400_test.go around lines 156 to 168,
the test expects a 400 BadRequest status when the Authorization header is
missing, but it should expect a 401 Unauthorized status instead. Update the
assertion to check for http.StatusUnauthorized. Additionally, enhance the test
by validating that the error response type matches
openapi.UnauthorizedErrorResponse to ensure the correct error handling behavior.
| if key.Key.Meta.Valid { | ||
| err = json.Unmarshal([]byte(key.Key.Meta.String), &k.Meta) | ||
| if err != nil { | ||
| h.Logger.Error("unable to unmarshal key metadata", | ||
| "keyId", key.Key.ID, | ||
| "error", err, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent error policy for bad key metadata
For key metadata you only log the unmarshal failure, whereas for identity metadata (lines 350-356) you abort the request with a 500. Choose one policy (preferably non-fatal with a structured warning) and apply it consistently.
- err = json.Unmarshal([]byte(key.Key.Meta.String), &k.Meta)
- if err != nil {
- h.Logger.Error("unable to unmarshal key metadata",
- "keyId", key.Key.ID,
- "error", err,
- )
- }
+ if err := json.Unmarshal([]byte(key.Key.Meta.String), &k.Meta); err != nil {
+ return fault.Wrap(err, fault.Code(codes.App.Internal.UnexpectedError.URN()),
+ fault.Internal("unable to unmarshal key metadata"), fault.Public("Invalid key metadata stored in database."))
+ }📝 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 key.Key.Meta.Valid { | |
| err = json.Unmarshal([]byte(key.Key.Meta.String), &k.Meta) | |
| if err != nil { | |
| h.Logger.Error("unable to unmarshal key metadata", | |
| "keyId", key.Key.ID, | |
| "error", err, | |
| ) | |
| } | |
| } | |
| if key.Key.Meta.Valid { | |
| if err := json.Unmarshal([]byte(key.Key.Meta.String), &k.Meta); err != nil { | |
| return fault.Wrap(err, fault.Code(codes.App.Internal.UnexpectedError.URN()), | |
| fault.Internal("unable to unmarshal key metadata"), fault.Public("Invalid key metadata stored in database.")) | |
| } | |
| } |
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_apis_list_keys/handler.go around lines 299 to 307, the
error handling for unmarshaling key metadata only logs the error without
aborting, while identity metadata errors abort with a 500. To fix this, unify
the error handling by either logging a structured warning without aborting or
aborting with a 500 consistently for both cases. Preferably, modify this block
to log a structured warning and continue processing, matching the non-fatal
approach used elsewhere.
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (6)
go/apps/api/routes/v2_apis_create_api/200_test.go (2)
92-100: 🛠️ Refactor suggestionAdd response-level assertions for stronger contract testing
After the
create basic apicall we only assertStatusandApiId.
Validating returned fields such asName,AuthType, or any flags present on the JSON payload would catch regression bugs where the handler populates the response incorrectly even though the DB write succeeded.Example:
require.Equal(t, 200, res.Status) require.NotEmpty(t, res.Body.Data.ApiId) +require.Equal(t, apiName, res.Body.Data.Name) +require.Equal(t, "key", res.Body.Data.AuthType)
128-148: 🧹 Nitpick (assertive)Sub-tests can run in parallel to reduce wall-clock time
The three loops create independent API resources and rely only on harness isolation. Marking the sub-test with
t.Parallel()improves feedback speed without affecting determinism:t.Run("create multiple apis", func(t *testing.T) { t.Parallel() ... })Ensure the harness/datastore is thread-safe before enabling.
go/apps/api/config.go (1)
56-63: 🛠️ Refactor suggestionAdd validation & guardrails for
VaultMasterKeys
VaultMasterKeysis security-critical. Right nowValidate()ignores it, so an empty slice or malformed key would go unnoticed and the server would panic only when the vault codepath is hit. Please add explicit checks (non-empty, expected length/encoding) and consider redacting the field in any log/printf to avoid accidental leakage.@@ func (c Config) Validate() error { // TLS configuration is validated when it's created from files - // Other validations may be added here in the future + if len(c.VaultMasterKeys) == 0 { + return fmt.Errorf("config: VaultMasterKeys must contain at least one key when Vault integration is enabled") + } + for i, k := range c.VaultMasterKeys { + if len(k) != 44 { // 32 bytes base64-encoded -> 44 chars + return fmt.Errorf("config: VaultMasterKeys[%d] has unexpected length", i) + } + } return nil }Remember to import
fmtat the top of the file.go/apps/api/routes/v2_identities_create_identity/200_test.go (3)
83-91: 🧹 Nitpick (assertive)Nit: favour constant-first multiplication for durations
(time.Minute * 15)is valid but the canonical form is15 * time.Minute, which is easier to scan and mirrors the std-lib style.- Duration: (time.Minute * 15).Milliseconds(), + Duration: (15 * time.Minute).Milliseconds(),
152-164: 🧹 Nitpick (assertive)Minor readability tweak – inline the slice pointer
You build the slice and then take its address.
A simpler pattern is to take the address of the composite literal directly:- identityRateLimits := []openapi.Ratelimit{ … } - req := handler.Request{ - ExternalId: externalTestID, - Ratelimits: &identityRateLimits, - } + req := handler.Request{ + ExternalId: externalTestID, + Ratelimits: &[]openapi.Ratelimit{ … }, + }Same applies to the various
metamaps below. Saves a variable and makes the intent clearer.
195-199: 🧹 Nitpick (assertive)Type-safe comparison for
Limit
rateLimits[idx].Limitisint32, whereasratelimit.Limit(from OpenAPI) is likelyint64.
Casting the DB value on every assertion hides potential sign/overflow issues. Prefer normalising both sides once:dbLimit := int64(rateLimits[idx].Limit) require.Equal(t, dbLimit, ratelimit.Limit)or update the schema/openapi so both carry the same width.
♻️ Duplicate comments (3)
go/apps/api/routes/v2_apis_delete_api/200_test.go (1)
127-135:⚠️ Potential issueMissing non-nullable field
Enabled+ leftover placeholder commentThe
InsertKeyParamsstruct in the DB layer still definesEnabled boolas NOT NULL, yet it is omitted here, and the “Add other required fields…” placeholder survived the last round of review.
Compilation or the INSERT will fail at runtime once the NOT NULL constraint is enforced.err = db.Query.InsertKey(ctx, h.DB.RW(), db.InsertKeyParams{ ID: keyID, KeyringID: keyAuthID, WorkspaceID: h.Resources().UserWorkspace.ID, CreatedAtM: time.Now().UnixMilli(), - // Add other required fields based on your schema + Enabled: true, Hash: hash.Sha256(uid.New(uid.TestPrefix)), Start: "teststart", })go/apps/api/routes/v2_keys_remove_permissions/400_test.go (1)
178-188: Useuid.PermissionPrefixinstead ofuid.TestPrefixfor permission IDsSame issue already raised on a previous commit: permission IDs are still generated with the generic
uid.TestPrefix.
For correctness and to avoid clashes with other entity types, use the dedicated permission prefix.-permissionID := uid.New(uid.TestPrefix) +permissionID := uid.New(uid.PermissionPrefix)Apply the same change to every occurrence in this test file (
nonExistentPermissionID, etc.).Also applies to: 212-223, 264-274
go/apps/api/routes/v2_identities_list_identities/200_test.go (1)
58-62: Avoid generating IDs with an empty prefixUsing
uid.New("")yields un-prefixed IDs, which can be confusing in systems that rely on prefixes for type-safety.
Prefer a dedicated test prefix:- externalID := "test_user_" + uid.New("") + externalID := "test_user_" + uid.New(uid.TestPrefix)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
go/Taskfile.yml(1 hunks)go/apps/api/cancel_test.go(5 hunks)go/apps/api/config.go(1 hunks)go/apps/api/routes/v2_apis_create_api/200_test.go(9 hunks)go/apps/api/routes/v2_apis_delete_api/200_test.go(1 hunks)go/apps/api/routes/v2_identities_create_identity/200_test.go(4 hunks)go/apps/api/routes/v2_identities_get_identity/200_test.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/200_test.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/cross_workspace_test.go(1 hunks)go/apps/api/routes/v2_keys_add_permissions/404_test.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/400_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
go/apps/api/routes/v2_identities_list_identities/cross_workspace_test.go (5)
go/pkg/testutil/http.go (2)
NewHarness(54-183)CallRoute(240-274)go/apps/api/routes/v2_identities_list_identities/handler.go (3)
Handler(25-31)Request(21-21)Response(22-22)go/apps/api/routes/register.go (1)
Register(46-433)go/pkg/uid/uid.go (3)
New(85-115)WorkspacePrefix(22-22)IdentityPrefix(30-30)go/pkg/db/workspace_insert.sql_generated.go (1)
InsertWorkspaceParams(37-42)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
go/apps/api/routes/v2_apis_create_api/200_test.go (2)
26-32: Potential missing dependency:Vaultfield onhandler.Handleris not initialisedThe PR description mentions that
Services(and consequently the handler struct) was extended with aVaultservice.
IfVaultis a non-nil requirement in the new implementation, leaving it unset here will panic at runtime when the code path is exercised (e.g. secrets lookup, encryption).route := &handler.Handler{ Logger: h.Logger, DB: h.DB, Keys: h.Keys, Permissions: h.Permissions, Auditlogs: h.Auditlogs, + Vault: h.Vault, // ensure new dependency is wired }Please double-check the handler signature across the codebase and wire any additional mandatory services to keep the test faithful to production wiring.
159-173: Verify schema limits for very long API namesThe generated name (
my-super-awesome-production-api-for-customer-management-and-analytics) is ~70 chars.
If theapis.namecolumn isVARCHAR(64)(common for slugs) this will silently truncate in some SQL engines. Consider asserting that the stored length matches the input or adding a negative test that expects a 4xx error when the limit is exceeded.go/apps/api/routes/v2_identities_create_identity/200_test.go (2)
24-30: Verify all required handler dependencies are initialisedThe test now instantiates the route via a struct literal instead of a factory/constructor.
That works, but it silently ignores new fields that might be added tohandler.Handlerin the future, leading to nil-pointer panics that tests won’t catch at compile-time.Consider adding a helper such as
newTestHandler(h *testutil.Harness) *handler.Handlerthat sets all fields explicitly (and fails the test if the struct definition changes).
369-387: Good edge-case coverageGreat to see an explicit test for the minimum-length
externalId. Helps prevent regressions around validation logic.
| - cmd: go test -failfast -timeout=60m -shuffle=on -v -json ./... | tparse -all -progress -smallscreen | ||
| silent: true |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Assess performance regression due to removing parallelism in test-full
Removing the -p=10 flag will serialize all integration tests and could significantly increase CI runtime. Consider isolating flaky tests, capping parallelism (e.g., -p=2–5), or grouping tests into stable and unstable suites to preserve throughput while avoiding intermittent failures.
🤖 Prompt for AI Agents
In go/Taskfile.yml around lines 31 to 32, the removal of the `-p=10` flag from
the `go test` command disables parallel test execution, which may cause longer
CI runtimes. To fix this, reintroduce controlled parallelism by adding a `-p`
flag with a moderate value like 2 to 5 to limit concurrency. Alternatively,
separate flaky tests into distinct groups or suites to maintain test throughput
while minimizing intermittent failures.
| test-unit: | ||
| cmds: | ||
| - go test -json -race -failfast -timeout=30m -p=10 ./... | tparse -all -progress -smallscreen | ||
| - go test -json -race -failfast -timeout=30m ./... | tparse -all -progress -smallscreen |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Reevaluate parallelism strategy for unit tests
Disabling concurrency globally may slow down the developer feedback loop. If only specific packages are unstable, target them individually (e.g., -p=1 on those), or use tags/filters instead of turning off parallelism for the entire suite.
🤖 Prompt for AI Agents
In go/Taskfile.yml at line 36, the current test command disables parallelism
globally by using the -failfast flag without specifying parallelism controls,
which may slow down test execution. Adjust the command to enable parallel test
execution by removing or modifying flags that limit concurrency, and if certain
packages are unstable, apply the -p=1 flag or use tags/filters specifically for
those packages instead of disabling parallelism for all tests.
| // Test case for deleting an API without keys | ||
| t.Run("delete api without keys", func(t *testing.T) { | ||
| keyAuthID := uid.New(uid.KeyAuthPrefix) | ||
| err := db.Query.InsertKeyring(ctx, h.DB.RW(), db.InsertKeyringParams{ | ||
| ID: keyAuthID, | ||
| WorkspaceID: h.Resources().UserWorkspace.ID, | ||
| CreatedAtM: time.Now().UnixMilli(), | ||
| DefaultPrefix: sql.NullString{Valid: false, String: ""}, | ||
| DefaultBytes: sql.NullInt32{Valid: false, Int32: 0}, | ||
| StoreEncryptedKeys: false, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| apiID := uid.New(uid.APIPrefix) | ||
| err = db.Query.InsertApi(ctx, h.DB.RW(), db.InsertApiParams{ | ||
| ID: apiID, | ||
| Name: "Test API", | ||
| WorkspaceID: h.Resources().UserWorkspace.ID, | ||
| AuthType: db.NullApisAuthType{Valid: true, ApisAuthType: db.ApisAuthTypeKey}, | ||
| KeyAuthID: sql.NullString{Valid: true, String: keyAuthID}, | ||
| CreatedAtM: time.Now().UnixMilli(), | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // Ensure API exists before deletion | ||
| apiBeforeDelete, err := db.Query.FindApiByID(ctx, h.DB.RO(), apiID) | ||
| require.NoError(t, err) | ||
|
|
||
| require.Equal(t, apiID, apiBeforeDelete.ID) | ||
| require.False(t, apiBeforeDelete.DeletedAtM.Valid) | ||
|
|
||
| // Delete the API | ||
| req := handler.Request{ | ||
| ApiId: apiID, | ||
| } | ||
|
|
||
| res := testutil.CallRoute[handler.Request, handler.Response]( | ||
| h, | ||
| route, | ||
| headers, | ||
| req, | ||
| ) | ||
|
|
||
| require.Equal(t, 200, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| require.NotEmpty(t, res.Body.Meta.RequestId) | ||
|
|
||
| // Verify API is marked as deleted | ||
| apiAfterDelete, err := db.Query.FindApiByID(ctx, h.DB.RO(), apiID) | ||
| require.NoError(t, err) // Should still find it, just marked as deleted | ||
| require.True(t, apiAfterDelete.DeletedAtM.Valid) | ||
| }) | ||
|
|
||
| // Test case for deleting an API with active keys | ||
| t.Run("delete api with active keys", func(t *testing.T) { | ||
| // Create keyring for the API | ||
| keyAuthID := uid.New(uid.KeyAuthPrefix) | ||
| err := db.Query.InsertKeyring(ctx, h.DB.RW(), db.InsertKeyringParams{ | ||
| ID: keyAuthID, | ||
| WorkspaceID: h.Resources().UserWorkspace.ID, | ||
| CreatedAtM: time.Now().UnixMilli(), | ||
| DefaultPrefix: sql.NullString{Valid: false, String: ""}, | ||
| DefaultBytes: sql.NullInt32{Valid: false, Int32: 0}, | ||
| StoreEncryptedKeys: false, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // Create the API | ||
| apiID := uid.New(uid.APIPrefix) | ||
| err = db.Query.InsertApi(ctx, h.DB.RW(), db.InsertApiParams{ | ||
| ID: apiID, | ||
| Name: "Test API With Keys", | ||
| WorkspaceID: h.Resources().UserWorkspace.ID, | ||
| AuthType: db.NullApisAuthType{Valid: true, ApisAuthType: db.ApisAuthTypeKey}, | ||
| KeyAuthID: sql.NullString{Valid: true, String: keyAuthID}, | ||
| CreatedAtM: time.Now().UnixMilli(), | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // Create a key associated with this API | ||
| keyID := uid.New(uid.KeyPrefix) | ||
| err = db.Query.InsertKey(ctx, h.DB.RW(), db.InsertKeyParams{ | ||
| ID: keyID, | ||
| KeyringID: keyAuthID, | ||
| WorkspaceID: h.Resources().UserWorkspace.ID, | ||
| CreatedAtM: time.Now().UnixMilli(), | ||
| // Add other required fields based on your schema | ||
| Hash: hash.Sha256(uid.New(uid.TestPrefix)), | ||
| Start: "teststart", | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // Ensure API exists before deletion | ||
| apiBeforeDelete, err := db.Query.FindApiByID(ctx, h.DB.RO(), apiID) | ||
| require.NoError(t, err) | ||
| require.Equal(t, apiID, apiBeforeDelete.ID) | ||
| require.False(t, apiBeforeDelete.DeletedAtM.Valid) | ||
|
|
||
| // Delete the API | ||
| req := handler.Request{ | ||
| ApiId: apiID, | ||
| } | ||
|
|
||
| res := testutil.CallRoute[handler.Request, handler.Response]( | ||
| h, | ||
| route, | ||
| headers, | ||
| req, | ||
| ) | ||
|
|
||
| require.Equal(t, 200, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| require.NotEmpty(t, res.Body.Meta.RequestId) | ||
|
|
||
| // Verify API is marked as deleted | ||
| apiAfterDelete, err := db.Query.FindApiByID(ctx, h.DB.RO(), apiID) | ||
| require.NoError(t, err) | ||
| require.True(t, apiAfterDelete.DeletedAtM.Valid) | ||
|
|
||
| // Check that the key is still accessible (soft delete doesn't cascade to keys) | ||
| key, err := db.Query.FindKeyByID(ctx, h.DB.RO(), keyID) | ||
| require.NoError(t, err) | ||
| require.Equal(t, keyID, key.ID) | ||
| }) | ||
|
|
||
| // Test case for deleting an API immediately after creation | ||
| t.Run("delete api immediately after creation", func(t *testing.T) { | ||
| // Create keyring for the API | ||
| keyAuthID := uid.New(uid.KeyAuthPrefix) | ||
| err := db.Query.InsertKeyring(ctx, h.DB.RW(), db.InsertKeyringParams{ | ||
| ID: keyAuthID, | ||
| WorkspaceID: h.Resources().UserWorkspace.ID, | ||
| CreatedAtM: time.Now().UnixMilli(), | ||
| DefaultPrefix: sql.NullString{Valid: false, String: ""}, | ||
| DefaultBytes: sql.NullInt32{Valid: false, Int32: 0}, | ||
| StoreEncryptedKeys: false, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // Create the API | ||
| apiID := uid.New(uid.APIPrefix) | ||
| apiName := "Test Immediate Delete API" | ||
| err = db.Query.InsertApi(ctx, h.DB.RW(), db.InsertApiParams{ | ||
| ID: apiID, | ||
| Name: apiName, | ||
| WorkspaceID: h.Resources().UserWorkspace.ID, | ||
| AuthType: db.NullApisAuthType{Valid: true, ApisAuthType: db.ApisAuthTypeKey}, | ||
| KeyAuthID: sql.NullString{Valid: true, String: keyAuthID}, | ||
| CreatedAtM: time.Now().UnixMilli(), | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // Verify the API was created | ||
| apiBeforeDelete, err := db.Query.FindApiByID(ctx, h.DB.RO(), apiID) | ||
| require.NoError(t, err) | ||
| require.Equal(t, apiID, apiBeforeDelete.ID) | ||
| require.Equal(t, apiName, apiBeforeDelete.Name) | ||
| require.False(t, apiBeforeDelete.DeletedAtM.Valid) | ||
|
|
||
| // Immediately delete the API without any delay | ||
| req := handler.Request{ | ||
| ApiId: apiID, | ||
| } | ||
|
|
||
| res := testutil.CallRoute[handler.Request, handler.Response]( | ||
| h, | ||
| route, | ||
| headers, | ||
| req, | ||
| ) | ||
|
|
||
| require.Equal(t, 200, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| require.NotEmpty(t, res.Body.Meta.RequestId) | ||
|
|
||
| // Verify API is marked as deleted | ||
| apiAfterDelete, err := db.Query.FindApiByID(ctx, h.DB.RO(), apiID) | ||
| require.NoError(t, err) | ||
| require.True(t, apiAfterDelete.DeletedAtM.Valid) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
DRY up repetitive test scaffolding
All three sub-tests duplicate the same boilerplate for creating keyrings, APIs, and request execution. Extracting small helpers (e.g. createKeyring, createAPI, callDeleteAPI) would:
• shorten each test by ~25 lines
• make the intent clearer (Arrange-Act-Assert)
• ease future maintenance when schema fields change.
No functional change required, purely readability & maintainability.
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_apis_delete_api/200_test.go between lines 46 and 225,
the three sub-tests contain repetitive code for creating keyrings, APIs, and
executing delete requests. Refactor by extracting helper functions such as
createKeyring to insert a keyring, createAPI to insert an API with parameters,
and callDeleteAPI to perform the delete request and return the response. Replace
the duplicated code in each sub-test with calls to these helpers to reduce
redundancy, improve readability, and simplify future updates without changing
test functionality.
| // Create a valid key for testing | ||
| keyAuthID := uid.New(uid.KeyAuthPrefix) | ||
| err := db.Query.InsertKeyring(ctx, h.DB.RW(), db.InsertKeyringParams{ | ||
| ID: keyAuthID, | ||
| WorkspaceID: workspace.ID, | ||
| StoreEncryptedKeys: false, | ||
| DefaultPrefix: sql.NullString{Valid: true, String: "test"}, | ||
| DefaultBytes: sql.NullInt32{Valid: true, Int32: 16}, | ||
| CreatedAtM: time.Now().UnixMilli(), | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| validKeyID := uid.New(uid.KeyPrefix) | ||
| keyString := "test_" + uid.New("") | ||
| err = db.Query.InsertKey(ctx, h.DB.RW(), db.InsertKeyParams{ | ||
| ID: validKeyID, | ||
| KeyringID: keyAuthID, | ||
| Hash: hash.Sha256(keyString), | ||
| Start: keyString[:4], | ||
| WorkspaceID: workspace.ID, | ||
| ForWorkspaceID: sql.NullString{Valid: false}, | ||
| Name: sql.NullString{Valid: true, String: "Test Key"}, | ||
| CreatedAtM: time.Now().UnixMilli(), | ||
| Enabled: true, | ||
| IdentityID: sql.NullString{Valid: false}, | ||
| Meta: sql.NullString{Valid: false}, | ||
| Expires: sql.NullTime{Valid: false}, | ||
| RemainingRequests: sql.NullInt32{Valid: false}, | ||
| RatelimitAsync: sql.NullBool{Valid: false}, | ||
| RatelimitLimit: sql.NullInt32{Valid: false}, | ||
| RatelimitDuration: sql.NullInt64{Valid: false}, | ||
| Environment: sql.NullString{Valid: false}, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid non-deterministic timestamps in tests
time.Now().UnixMilli() introduces nondeterminism. When tests depend on precise timestamps (e.g., in ordering or TTL logic) this can cause flaky behaviour.
If the timestamp is not semantically important, consider using a fixed value or a helper such as testutil.FixedUnixMilli() (if available) to keep tests deterministic.
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_keys_remove_permissions/400_test.go between lines 44
and 75, replace all instances of time.Now().UnixMilli() with a fixed timestamp
value or a helper function like testutil.FixedUnixMilli() to ensure
deterministic and stable test behavior by avoiding non-deterministic current
timestamps.
| // Test creating an API manually via DB to verify queries | ||
| // This test validates that the underlying database queries work correctly | ||
| // by bypassing the HTTP handler and directly testing the DB operations. | ||
| t.Run("insert api via DB", func(t *testing.T) { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
DB-only test belongs in the DAO/queries package, not the HTTP handler suite
The “insert api via DB” sub-test bypasses the route and directly asserts low-level query behaviour.
Mixing concerns inflates the scope of this test file and makes failures harder to triage (handler vs DAO). Consider moving this block into a dedicated db/ test, or keep it here but mark it clearly as an integration test that touches multiple layers.
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_apis_create_api/200_test.go around lines 42 to 45, the
"insert api via DB" test directly tests database queries instead of the HTTP
handler, mixing concerns in this test file. To fix this, move this sub-test to a
dedicated DAO or queries package test file focused on database operations, or
alternatively, clearly mark it as an integration test here to distinguish it
from pure handler tests.
| t.Run("permission not found by ID", func(t *testing.T) { | ||
| // Create a test keyring | ||
| keyAuthID := uid.New(uid.KeyAuthPrefix) | ||
| err := db.Query.InsertKeyring(ctx, h.DB.RW(), db.InsertKeyringParams{ | ||
| ID: keyAuthID, | ||
| WorkspaceID: workspace.ID, | ||
| StoreEncryptedKeys: false, | ||
| DefaultPrefix: sql.NullString{Valid: true, String: "test"}, | ||
| DefaultBytes: sql.NullInt32{Valid: true, Int32: 16}, | ||
| CreatedAtM: time.Now().UnixMilli(), | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // Create a test key | ||
| keyID := uid.New(uid.KeyPrefix) | ||
| keyString := "test_" + uid.New("") | ||
| err = db.Query.InsertKey(ctx, h.DB.RW(), db.InsertKeyParams{ | ||
| ID: keyID, | ||
| KeyringID: keyAuthID, | ||
| Hash: hash.Sha256(keyString), | ||
| Start: keyString[:4], | ||
| WorkspaceID: workspace.ID, | ||
| ForWorkspaceID: sql.NullString{Valid: false}, | ||
| Name: sql.NullString{Valid: true, String: "Test Key"}, | ||
| CreatedAtM: time.Now().UnixMilli(), | ||
| Enabled: true, | ||
| IdentityID: sql.NullString{Valid: false}, | ||
| Meta: sql.NullString{Valid: false}, | ||
| Expires: sql.NullTime{Valid: false}, | ||
| RemainingRequests: sql.NullInt32{Valid: false}, | ||
| RatelimitAsync: sql.NullBool{Valid: false}, | ||
| RatelimitLimit: sql.NullInt32{Valid: false}, | ||
| RatelimitDuration: sql.NullInt64{Valid: false}, | ||
| Environment: sql.NullString{Valid: false}, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // Use a non-existent permission ID | ||
| nonExistentPermissionID := uid.New(uid.TestPrefix) | ||
|
|
||
| req := handler.Request{ | ||
| KeyId: keyID, | ||
| Permissions: []struct { | ||
| Create *bool `json:"create,omitempty"` | ||
| Id *string `json:"id,omitempty"` | ||
| Slug *string `json:"slug,omitempty"` | ||
| }{ | ||
| {Id: &nonExistentPermissionID}, | ||
| }, | ||
| } | ||
|
|
||
| res := testutil.CallRoute[handler.Request, openapi.NotFoundErrorResponse]( | ||
| h, | ||
| route, | ||
| headers, | ||
| req, | ||
| ) | ||
|
|
||
| require.Equal(t, 404, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| require.Contains(t, res.Body.Error.Detail, "was not found") | ||
| }) | ||
|
|
||
| t.Run("permission not found by name", func(t *testing.T) { | ||
| // Create a test keyring | ||
| keyAuthID := uid.New(uid.KeyAuthPrefix) | ||
| err := db.Query.InsertKeyring(ctx, h.DB.RW(), db.InsertKeyringParams{ | ||
| ID: keyAuthID, | ||
| WorkspaceID: workspace.ID, | ||
| StoreEncryptedKeys: false, | ||
| DefaultPrefix: sql.NullString{Valid: true, String: "test"}, | ||
| DefaultBytes: sql.NullInt32{Valid: true, Int32: 16}, | ||
| CreatedAtM: time.Now().UnixMilli(), | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // Create a test key | ||
| keyID := uid.New(uid.KeyPrefix) | ||
| keyString := "test_" + uid.New("") | ||
| err = db.Query.InsertKey(ctx, h.DB.RW(), db.InsertKeyParams{ | ||
| ID: keyID, | ||
| KeyringID: keyAuthID, | ||
| Hash: hash.Sha256(keyString), | ||
| Start: keyString[:4], | ||
| WorkspaceID: workspace.ID, | ||
| ForWorkspaceID: sql.NullString{Valid: false}, | ||
| Name: sql.NullString{Valid: true, String: "Test Key"}, | ||
| CreatedAtM: time.Now().UnixMilli(), | ||
| Enabled: true, | ||
| IdentityID: sql.NullString{Valid: false}, | ||
| Meta: sql.NullString{Valid: false}, | ||
| Expires: sql.NullTime{Valid: false}, | ||
| RemainingRequests: sql.NullInt32{Valid: false}, | ||
| RatelimitAsync: sql.NullBool{Valid: false}, | ||
| RatelimitLimit: sql.NullInt32{Valid: false}, | ||
| RatelimitDuration: sql.NullInt64{Valid: false}, | ||
| Environment: sql.NullString{Valid: false}, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| nonExistentPermissionSlug := "nonexistent.permission.name" | ||
|
|
||
| req := handler.Request{ | ||
| KeyId: keyID, | ||
| Permissions: []struct { | ||
| Create *bool `json:"create,omitempty"` | ||
| Id *string `json:"id,omitempty"` | ||
| Slug *string `json:"slug,omitempty"` | ||
| }{ | ||
| {Slug: &nonExistentPermissionSlug}, | ||
| }, | ||
| } | ||
|
|
||
| res := testutil.CallRoute[handler.Request, openapi.NotFoundErrorResponse]( | ||
| h, | ||
| route, | ||
| headers, | ||
| req, | ||
| ) | ||
|
|
||
| require.Equal(t, 404, res.Status, "Wanted status code 404, received: %s", res.RawBody) | ||
| require.NotNil(t, res.Body) | ||
| require.Contains(t, res.Body.Error.Detail, "was not found") | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider table-driven tests for the 404 matrix
All five scenarios share the same Arrange-Act-Assert structure. A table-driven loop with per-row setup callbacks would shrink this section dramatically and make it trivial to add new “not found” permutations later.
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_keys_add_permissions/404_test.go around lines 82 to
205, the test cases for "permission not found by ID" and "permission not found
by name" share a similar Arrange-Act-Assert pattern. Refactor these tests into a
single table-driven test by defining a slice of test cases with fields for the
setup parameters and expected outcomes. Use a loop to iterate over the test
cases, running subtests for each scenario, and include per-row setup callbacks
if needed to handle specific arrangements. This will reduce code duplication and
simplify adding future test scenarios.
| t.Run("key not found", func(t *testing.T) { | ||
| // Create a permission that exists | ||
| permissionID := uid.New(uid.TestPrefix) | ||
| err := db.Query.InsertPermission(ctx, h.DB.RW(), db.InsertPermissionParams{ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Run sub-tests in parallel and DRY up repeated fixture code
Each t.Run is fully isolated (new keyrings/keys/permissions) and can safely run with t.Parallel(), cutting test time substantially.
The blocks that create keyrings, keys and permissions are near-identical across sub-tests—extract helpers in the harness (e.g. h.CreateKeyring, h.CreateKey, h.CreatePermission) to reduce >150 duplicated lines and keep the intent of each case crystal-clear.
Also applies to: 82-85, 145-148, 207-211, 288-291
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_keys_add_permissions/404_test.go around lines 44 to 47,
the sub-tests are not running in parallel and contain repeated fixture setup
code for keyrings, keys, and permissions. Modify each t.Run sub-test to call
t.Parallel() to enable concurrent execution and extract the repeated setup code
into helper functions within the test harness, such as h.CreateKeyring,
h.CreateKey, and h.CreatePermission, to eliminate duplication and clarify test
intent. Apply the same changes to the similar code blocks at lines 82-85,
145-148, 207-211, and 288-291.
| tx, err := h.DB.RW().Begin(ctx) | ||
| require.NoError(t, err) | ||
| defer tx.Rollback() | ||
|
|
||
| // First, create the workspace B |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Suppress benign rollback error after Commit()
defer tx.Rollback() fires even after a successful tx.Commit(), producing a harmless sql.ErrTxDone.
Wrap the call to ignore the error:
- defer tx.Rollback()
+ defer func() { _ = tx.Rollback() }() // ignore, tx may already be committed📝 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.
| tx, err := h.DB.RW().Begin(ctx) | |
| require.NoError(t, err) | |
| defer tx.Rollback() | |
| // First, create the workspace B | |
| tx, err := h.DB.RW().Begin(ctx) | |
| require.NoError(t, err) | |
| - defer tx.Rollback() | |
| + defer func() { _ = tx.Rollback() }() // ignore, tx may already be committed | |
| // First, create the workspace B |
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_identities_list_identities/cross_workspace_test.go
around lines 42 to 46, the deferred tx.Rollback() runs even after a successful
tx.Commit(), causing a benign sql.ErrTxDone error. Modify the deferred rollback
call to check the error returned and ignore sql.ErrTxDone, ensuring it only logs
or handles unexpected rollback errors.
| tx, err := h.DB.RW().Begin(ctx) | ||
| require.NoError(t, err) | ||
| defer tx.Rollback() | ||
|
|
||
| err = db.Query.InsertIdentity(ctx, tx, db.InsertIdentityParams{ | ||
| ID: identityWithoutRatelimitsID, | ||
| ExternalID: externalIDWithoutRatelimits, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Silence rollback error after explicit Commit()
After you Commit() the transaction on line 214, the deferred tx.Rollback() will return sql.ErrTxDone.
Ignore it to keep test output clean:
- defer tx.Rollback()
+ defer func() { _ = tx.Rollback() }() // ignore error after commit📝 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.
| tx, err := h.DB.RW().Begin(ctx) | |
| require.NoError(t, err) | |
| defer tx.Rollback() | |
| err = db.Query.InsertIdentity(ctx, tx, db.InsertIdentityParams{ | |
| ID: identityWithoutRatelimitsID, | |
| ExternalID: externalIDWithoutRatelimits, | |
| tx, err := h.DB.RW().Begin(ctx) | |
| require.NoError(t, err) | |
| - defer tx.Rollback() | |
| + defer func() { _ = tx.Rollback() }() // ignore error after commit | |
| err = db.Query.InsertIdentity(ctx, tx, db.InsertIdentityParams{ | |
| ID: identityWithoutRatelimitsID, | |
| ExternalID: externalIDWithoutRatelimits, |
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_identities_get_identity/200_test.go around lines 198 to
204, the deferred tx.Rollback() call after an explicit tx.Commit() can return
sql.ErrTxDone, causing unnecessary error output. Modify the deferred rollback to
check the error returned and ignore it if it is sql.ErrTxDone, ensuring the test
output remains clean.
when reusing containers between tests, we had some colissions that caused test failures. By using unique ids everywhere, we can safely reuse containers.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
go/apps/api/integration/multi_node_ratelimiting/generate_tests/main.go (1)
85-90:os.RemoveAll("../generated")could delete hand-written testsRunning
go generatefrom a different working directory (e.g. via an IDE) rewrites the relative path and may wipe unrelated files.
Safeguards to consider:-if err := os.RemoveAll(baseDir); err != nil { ... } +if _, err := os.Stat(baseDir); err == nil { + // Only delete if the directory was created by this generator. + if err := os.RemoveAll(baseDir); err != nil { panic(err) } +}or move generated code under
internal/generatedand add a sentinel file (DO_NOT_EDIT) to ensure the directory is solely generator-owned.
♻️ Duplicate comments (1)
go/apps/api/integration/multi_node_ratelimiting/generate_tests/main.go (1)
35-41: Same width-overflow caveat applies to the generated test function names – see the previous comment for recommendations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (54)
go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0005_duration000001000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0005_duration000001000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0005_duration000001000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0005_duration000060000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0005_duration000060000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0005_duration000060000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0005_duration003600000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0005_duration003600000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0005_duration003600000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration000001000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration000001000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration000001000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration000060000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration000060000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration000060000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration003600000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration003600000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes01_limit0100_duration003600000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0005_duration000001000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0005_duration000001000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0005_duration000001000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0005_duration000060000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0005_duration000060000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0005_duration000060000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0005_duration003600000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0005_duration003600000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0005_duration003600000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration000001000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration000001000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration000001000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration000060000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration000060000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration000060000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration003600000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration003600000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes03_limit0100_duration003600000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0005_duration000001000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0005_duration000001000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0005_duration000001000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0005_duration000060000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0005_duration000060000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0005_duration000060000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0005_duration003600000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0005_duration003600000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0005_duration003600000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration000001000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration000001000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration000001000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration000060000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration000060000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration000060000_load10_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration003600000_load00_90_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration003600000_load02_00_windows010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes09_limit0100_duration003600000_load10_00_windows010/generated_test.gois excluded by!**/generated/**
📒 Files selected for processing (2)
go/apps/api/integration/harness.go(1 hunks)go/apps/api/integration/multi_node_ratelimiting/generate_tests/main.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (22)
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
go/apps/api/integration/harness.go (1)
45-45: ```shell
#!/bin/bashLocate all invocations of RunOtel, with surrounding context
rg -n --context 2 'RunOtel(' .
Find the RunOtel method signature to see its return type
rg -n --context 2 'func .*RunOtel' .
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| return fmt.Sprintf("ratelimit_nodes%02d_limit%04d_duration%09d_load%s_windows%03d", | ||
| tc.NodeCount, | ||
| tc.Limit, | ||
| tc.Duration, | ||
| strings.ReplaceAll(fmt.Sprintf("%.2f", tc.LoadFactor), ".", "_"), | ||
| strings.ReplaceAll(fmt.Sprintf("%05.2f", tc.LoadFactor), ".", "_"), | ||
| tc.WindowCount, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
❓ Verification inconclusive
Fixed-width specifiers risk identifier collisions / invalid Go identifiers once the matrix grows
%02d, %04d, %09d, and %05.2f work for the current matrix (≤ 9 nodes, ≤ 9999 limit, ≤ 1 h duration, ≤ 99.99 load), but:
- Future test values > 99 nodes, > 9999 limit, or > 999 999 999 ms will silently overflow the field width, producing longer strings and breaking the one-package-per-test directory assumption.
%05.2ftruncates load factors ≥ 100 (e.g.123.4 → 123.40, width 6) so the package name diverges from the test name and may become an invalid identifier after the.→_replacement.
Consider switching to dynamic width formatting or base-36/hex encoding to guarantee stable lengths:
-return fmt.Sprintf("ratelimit_nodes%02d_limit%04d_duration%09d_load%s_windows%03d",
+return fmt.Sprintf("ratelimit_nodes%03d_limit%05d_duration%012d_load%s_windows%03d",or even:
return fmt.Sprintf(
"ratelimit_n%02d_l%x_d%x_lf%s_w%02d",
tc.NodeCount, tc.Limit, tc.Duration,
strings.ReplaceAll(fmt.Sprintf("%.2f", tc.LoadFactor), ".", "_"),
tc.WindowCount,
)This future-proofs the generator and avoids Go identifier issues.
Fixed-width specifiers risk identifier collisions / invalid Go identifiers once the matrix grows
%02d, %04d, %09d, and %05.2f work for the current matrix (≤ 9 nodes, ≤ 9999 limit, ≤ 1 h duration, ≤ 99.99 load), but:
- Future test values > 99 nodes, > 9999 limit, or > 999 999 999 ms will silently overflow the field width, producing longer strings and breaking the one-package-per-test directory assumption.
%05.2ftruncates load factors ≥ 100 (e.g.123.4 → 123.40, width 6) so the package name diverges from the test name and may become an invalid identifier after the.→_replacement.
Consider switching to dynamic width formatting or base-36/hex encoding to guarantee stable lengths:
-return fmt.Sprintf("ratelimit_nodes%02d_limit%04d_duration%09d_load%s_windows%03d",
+return fmt.Sprintf("ratelimit_nodes%03d_limit%05d_duration%012d_load%s_windows%03d",or even:
return fmt.Sprintf(
"ratelimit_n%02d_l%x_d%x_lf%s_w%02d",
tc.NodeCount, tc.Limit, tc.Duration,
strings.ReplaceAll(fmt.Sprintf("%.2f", tc.LoadFactor), ".", "_"),
tc.WindowCount,
)This future-proofs the generator and avoids Go identifier issues.
🤖 Prompt for AI Agents
In go/apps/api/integration/multi_node_ratelimiting/generate_tests/main.go around
lines 24 to 30, the fixed-width format specifiers in the fmt.Sprintf call risk
producing invalid or inconsistent Go identifiers when test values exceed current
limits. To fix this, replace fixed-width decimal formatting with dynamic width
or use hexadecimal/base-36 encoding for numeric fields like NodeCount, Limit,
and Duration. Also, format LoadFactor without fixed width and replace dots with
underscores to maintain valid identifiers. This ensures stable, collision-free
package names regardless of test matrix growth.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
End-users will experience a more secure, flexible, and well-documented API platform with enhanced permission controls and reliable error feedback.