Conversation
Flo4604
requested review from
MichaelUnkey,
chronark,
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughAdds a key migration feature: new POST /v2/keys.migrateKeys endpoint, migration-aware verification and GetMigrated flow, DB schema and sqlc bindings for key_migrations and pending_migration_id, prefixed API key utilities and CLI, new migration_not_found error and docs, route/handler, tests, and OpenAPI updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MigrateAPI as POST /v2/keys.migrateKeys
participant Handler
participant DB
participant KeyService
participant Cache
Note over MigrateAPI,Handler: Bulk migration request (migrationId, apiId, keys[])
Client->>MigrateAPI: POST payload
MigrateAPI->>Handler: validate + auth
Handler->>DB: FindKeyMigrationByID(migrationId)
Handler->>DB: FindKeysByHash (detect existing hashes)
Handler->>DB: FindIdentitiesByExternalId
Handler->>DB: Bulk insert identities/permissions/roles (chunked)
Handler->>DB: InsertKeys (with pending_migration_id)
Handler->>DB: InsertRatelimits, Assign permissions/roles
Handler->>Cache: Evict/prime entries as needed
Handler->>DB: Audit log
Handler->>Client: 200 { migrated: [...], failed: [...] }
sequenceDiagram
participant Client
participant VerifyAPI as POST /v2/keys.verifyKey
participant VerifyHandler
participant KeyService
participant KeyCache
participant MigrationSvc as GetMigrated
Note over VerifyAPI,VerifyHandler: Verification with optional migrationId
Client->>VerifyAPI: POST (key, migrationId?)
VerifyAPI->>KeyService: Get(hash.Sha256(key))
KeyService->>KeyCache: lookup hash
alt Key found
KeyService->>VerifyAPI: return verified key
else Not found and migrationId provided
VerifyHandler->>MigrationSvc: GetMigrated(rawKey, migrationId)
MigrationSvc->>DB: FindKeyMigrationByID -> FindLiveKeyByHash (prefixed lookup)
alt pending_migration matches
MigrationSvc->>DB: UpdateKeyHashAndMigration (set new hash, clear pending)
MigrationSvc->>KeyCache: Evict old hash
MigrationSvc->>VerifyAPI: return migrated key
else
MigrationSvc->>VerifyAPI: return NotFound / migration error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
go/apps/api/routes/v2_keys_update_credits/200_test.go (1)
172-182: Optional: Consider adding a brief comment about hash-based retrieval.While the code is correct, a brief comment explaining that keys are now retrieved by hash (due to the migration feature) could help future maintainers understand the change.
Example:
t.Run("counter cache invalidation after credit update", func(t *testing.T) { // Create a new key with initial credits for this test initialCredits := int32(100) cacheTestKey := h.CreateKey(seed.CreateKeyRequest{ WorkspaceID: workspace.ID, KeySpaceID: api.KeyAuthID.String, Name: &keyName, Remaining: &initialCredits, }) + // Keys are retrieved by hash as part of the migration feature authBefore, _, err := h.Keys.Get(ctx, &zen.Session{}, hash.Sha256(cacheTestKey.Key))go/apps/api/routes/v2_keys_verify_key/migration_test.go (1)
117-124: Clarify assertion message
The assertion compareskey.Startto the first seven characters of the token, but the failure message still says “first 6 chars.” Align the message (or the slice) so future readers don’t second-guess which prefix length is expected.go/apps/api/openapi/openapi-generated.yaml (5)
1431-1435: Align migrationId length and avoid embedding support email in the spec
- Length mismatch: here maxLength is 256, while V2KeysMigrateKeysRequestBody.migrationId uses 255. Make them consistent (recommend 255).
- Prefer moving the support email to docs and keeping the description product-neutral.
Apply:
migrationId: type: string - maxLength: 256 - description: Migrate keys on demand from your previous system. Reach out for migration support at support@unkey.dev + maxLength: 255 + description: Migrate keys on demand from your previous system. example: "m_1234abcd"
931-935: Add apiId pattern to match existing constraintsOther endpoints validate apiId with ^[a-zA-Z0-9_]+$ and indicate it begins with 'api_'. Mirror that here for consistency and better client-side validation.
apiId: type: string minLength: 3 maxLength: 255 - description: The ID of the API that the keys should be inserted into + pattern: "^[a-zA-Z0-9_]+$" + description: | + The ID of the API that the keys should be inserted into. + Must be a valid API ID that begins with 'api_' and exists within your workspace. example: api_123456789
2733-2762: Normalize roles/permissions constraints in migration payloadCurrent constraints are looser than add/set endpoints. Aligning patterns/min lengths prevents invalid role/permission slugs from slipping through during migration.
roles: type: array maxItems: 100 items: - type: string - minLength: 1 - maxLength: 100 + type: string + minLength: 3 + maxLength: 255 + pattern: "^[a-zA-Z][a-zA-Z0-9._-]*$" ... permissions: type: array maxItems: 1000 items: - type: string - minLength: 1 - maxLength: 100 + type: string + minLength: 3 + maxLength: 100 + pattern: "^[a-zA-Z0-9_:\\-\\.\\*]+$"Based on learnings
2819-2831: Include per-hash failure reasons in migrateKeys responseA plain string list loses actionable context (e.g., already exists, invalid hash, provider error). Return objects with a reason/code to enable retries and dashboards.
failed: type: array - description: Hashes that could not be migrated (e.g., already exist in the system) - items: - type: string - description: The hash that failed to migrate - example: sha256_ghi789jkl012 + description: Hashes that could not be migrated with failure reasons + items: + "$ref": "#/components/schemas/V2KeysMigrateKeysFailure"Add this schema under components/schemas:
V2KeysMigrateKeysFailure: type: object required: [hash, reason, code] properties: hash: type: string description: The hash that failed to migrate example: sha256_ghi789jkl012 reason: type: string description: Human-readable reason example: "already exists in target API" code: type: string description: Machine-readable error code enum: [ALREADY_EXISTS, INVALID_HASH, PROVIDER_ERROR, UNKNOWN]
4769-4775: Clarify idempotency/deduping semanticsThe description mentions partial success; explicitly documenting behavior on duplicate hashes improves client UX.
description: | Returns HTTP 200 even on partial success; hashes that could not be migrated are listed under `data.failed`. + + The operation is idempotent per hash: submitting the same hash again will not create duplicates and will surface in `data.failed` with an appropriate reason.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (61)
apps/docs/docs.json(1 hunks)apps/docs/errors/unkey/data/migration_not_found.mdx(1 hunks)go/apps/api/openapi/gen.go(3 hunks)go/apps/api/openapi/openapi-generated.yaml(5 hunks)go/apps/api/openapi/openapi-split.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/createKey/V2KeysCreateKeyRequestBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeysMigration.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeysRequestBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeysResponseBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeysResponseData.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/index.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/verifyKey/V2KeysVerifyKeyRequestBody.yaml(1 hunks)go/apps/api/routes/register.go(2 hunks)go/apps/api/routes/v2_keys_migrate_keys/200_test.go(1 hunks)go/apps/api/routes/v2_keys_migrate_keys/400_test.go(1 hunks)go/apps/api/routes/v2_keys_migrate_keys/401_test.go(1 hunks)go/apps/api/routes/v2_keys_migrate_keys/403_test.go(1 hunks)go/apps/api/routes/v2_keys_migrate_keys/404_test.go(1 hunks)go/apps/api/routes/v2_keys_migrate_keys/handler.go(1 hunks)go/apps/api/routes/v2_keys_update_credits/200_test.go(3 hunks)go/apps/api/routes/v2_keys_update_key/200_test.go(3 hunks)go/apps/api/routes/v2_keys_verify_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_verify_key/migration_test.go(1 hunks)go/apps/api/routes/v2_keys_verify_key/resend_demo_test.go(1 hunks)go/apps/gw/services/auth/auth.go(2 hunks)go/internal/services/keys/get.go(2 hunks)go/internal/services/keys/get_migrated.go(1 hunks)go/internal/services/keys/interface.go(1 hunks)go/pkg/codes/constants_gen.go(1 hunks)go/pkg/codes/unkey_data.go(3 hunks)go/pkg/db/bulk_key_insert.sql_generated.go(3 hunks)go/pkg/db/bulk_key_migration_insert.sql_generated.go(1 hunks)go/pkg/db/identity_find_many_by_external_id.sql_generated.go(1 hunks)go/pkg/db/key_find_by_id.sql_generated.go(2 hunks)go/pkg/db/key_find_for_verification.sql_generated.go(4 hunks)go/pkg/db/key_find_live_by_hash.sql_generated.go(4 hunks)go/pkg/db/key_find_live_by_id.sql_generated.go(4 hunks)go/pkg/db/key_find_many_by_hash.sql_generated.go(1 hunks)go/pkg/db/key_insert.sql_generated.go(5 hunks)go/pkg/db/key_list_by_key_space_id.sql_generated.go(3 hunks)go/pkg/db/key_list_live_by_key_space_id.sql_generated.go(4 hunks)go/pkg/db/key_migration_find_by_id.sql_generated.go(1 hunks)go/pkg/db/key_migration_insert.sql_generated.go(1 hunks)go/pkg/db/key_update_hash_and_migration.sql_generated.go(1 hunks)go/pkg/db/models_generated.go(3 hunks)go/pkg/db/querier_bulk_generated.go(1 hunks)go/pkg/db/querier_generated.go(13 hunks)go/pkg/db/queries/identity_find_many_by_external_id.sql(1 hunks)go/pkg/db/queries/key_find_for_verification.sql(1 hunks)go/pkg/db/queries/key_find_many_by_hash.sql(1 hunks)go/pkg/db/queries/key_insert.sql(2 hunks)go/pkg/db/queries/key_migration_find_by_id.sql(1 hunks)go/pkg/db/queries/key_migration_insert.sql(1 hunks)go/pkg/db/queries/key_update_hash_and_migration.sql(1 hunks)go/pkg/db/schema.sql(3 hunks)go/pkg/prefixedapikey/LICENSE(1 hunks)go/pkg/prefixedapikey/prefixedapikey.go(1 hunks)go/pkg/prefixedapikey/prefixedapikey_test.go(1 hunks)go/pkg/zen/middleware_errors.go(1 hunks)internal/db/src/schema/keys.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (26)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/queries/key_find_for_verification.sql:15-15
Timestamp: 2025-08-27T13:48:54.016Z
Learning: The pending_migration_id field in FindKeyForVerification is only used internally by get_migrated.go for migration logic and doesn't need to be exposed in KeyVerifier struct or API response DTOs since it's an internal implementation detail.
📚 Learning: 2025-07-17T14:24:20.403Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3631
File: go/pkg/db/bulk_keyring_insert.sql.go:23-25
Timestamp: 2025-07-17T14:24:20.403Z
Learning: In go/pkg/db/bulk_keyring_insert.sql.go and similar bulk insert generated files, hardcoded zero values for fields like size_approx and size_last_updated_at are intentional and reflect the original SQL query structure, not missing parameters.
Applied to files:
go/pkg/db/queries/key_migration_insert.sqlgo/pkg/db/queries/key_find_for_verification.sqlgo/pkg/db/bulk_key_insert.sql_generated.gogo/pkg/db/queries/key_insert.sqlgo/pkg/db/querier_bulk_generated.gogo/pkg/db/key_find_for_verification.sql_generated.gogo/pkg/db/key_find_many_by_hash.sql_generated.gogo/pkg/db/schema.sqlgo/pkg/db/key_update_hash_and_migration.sql_generated.gogo/pkg/db/key_insert.sql_generated.gogo/pkg/db/key_migration_insert.sql_generated.gogo/pkg/db/key_list_live_by_key_space_id.sql_generated.gogo/pkg/db/querier_generated.gogo/pkg/db/models_generated.gogo/pkg/db/bulk_key_migration_insert.sql_generated.gogo/pkg/db/key_list_by_key_space_id.sql_generated.gogo/pkg/db/queries/key_update_hash_and_migration.sqlgo/pkg/db/key_find_by_id.sql_generated.gogo/pkg/db/queries/key_find_many_by_hash.sql
📚 Learning: 2025-09-12T08:01:20.792Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3944
File: go/pkg/db/acme_challenge_update_verified_with_expiry.sql_generated.go:31-39
Timestamp: 2025-09-12T08:01:20.792Z
Learning: Do not review or suggest changes to files with sql_generated.go suffix or other files marked as auto-generated (containing "Code generated by" comments), as these are generated by tools like sqlc and changes would be overwritten on regeneration.
Applied to files:
go/pkg/db/queries/key_migration_insert.sqlgo/pkg/db/key_find_many_by_hash.sql_generated.gogo/pkg/db/key_update_hash_and_migration.sql_generated.gogo/pkg/db/bulk_key_migration_insert.sql_generated.go
📚 Learning: 2025-10-30T15:10:52.743Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Applied to files:
go/pkg/db/queries/key_migration_insert.sqlgo/pkg/db/queries/key_find_for_verification.sqlgo/pkg/db/bulk_key_insert.sql_generated.gogo/pkg/db/queries/key_insert.sqlgo/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yamlgo/pkg/db/key_find_for_verification.sql_generated.gogo/pkg/db/schema.sqlgo/pkg/db/queries/key_migration_find_by_id.sqlgo/apps/api/routes/v2_keys_update_credits/200_test.gogo/pkg/db/key_find_live_by_id.sql_generated.gogo/pkg/db/key_insert.sql_generated.gogo/pkg/db/key_list_live_by_key_space_id.sql_generated.gointernal/db/src/schema/keys.tsgo/pkg/db/querier_generated.gogo/apps/api/openapi/gen.gogo/pkg/db/models_generated.gogo/pkg/db/key_list_by_key_space_id.sql_generated.gogo/pkg/db/queries/key_update_hash_and_migration.sqlgo/pkg/db/key_find_by_id.sql_generated.go
📚 Learning: 2025-08-27T13:48:54.016Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/queries/key_find_for_verification.sql:15-15
Timestamp: 2025-08-27T13:48:54.016Z
Learning: The pending_migration_id field in FindKeyForVerification is only used internally by get_migrated.go for migration logic and doesn't need to be exposed in KeyVerifier struct or API response DTOs since it's an internal implementation detail.
Applied to files:
go/pkg/db/queries/key_find_for_verification.sqlgo/pkg/db/bulk_key_insert.sql_generated.gogo/pkg/db/key_find_live_by_hash.sql_generated.gogo/pkg/db/key_find_for_verification.sql_generated.gogo/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeysMigration.yamlgo/internal/services/keys/get_migrated.gogo/pkg/db/queries/key_migration_find_by_id.sqlgo/pkg/db/key_migration_find_by_id.sql_generated.gogo/pkg/db/key_find_live_by_id.sql_generated.gogo/pkg/db/key_insert.sql_generated.gogo/apps/api/openapi/spec/paths/v2/keys/verifyKey/V2KeysVerifyKeyRequestBody.yamlgo/internal/services/keys/interface.gogo/pkg/db/key_list_live_by_key_space_id.sql_generated.gogo/pkg/db/querier_generated.gogo/pkg/codes/unkey_data.gogo/apps/api/routes/v2_keys_verify_key/migration_test.gogo/apps/api/openapi/gen.gogo/pkg/db/models_generated.gogo/apps/api/openapi/openapi-generated.yamlgo/pkg/db/key_list_by_key_space_id.sql_generated.gogo/pkg/db/key_find_by_id.sql_generated.gogo/apps/api/routes/v2_keys_verify_key/handler.go
📚 Learning: 2025-08-27T13:46:00.608Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/key_find_live_by_id.sql_generated.go:15-16
Timestamp: 2025-08-27T13:46:00.608Z
Learning: FindLiveKeyByID doesn't need to select pending_migration_id since it's only used in key management operations that don't care about migration state. The migration flow uses other queries like FindKeyForVerification.
Applied to files:
go/pkg/db/queries/key_find_for_verification.sqlgo/pkg/db/key_find_live_by_hash.sql_generated.gogo/pkg/db/key_find_for_verification.sql_generated.gogo/internal/services/keys/get_migrated.gogo/pkg/db/queries/key_migration_find_by_id.sqlgo/pkg/db/key_migration_find_by_id.sql_generated.gogo/pkg/db/key_find_live_by_id.sql_generated.gogo/pkg/db/key_list_live_by_key_space_id.sql_generated.gogo/pkg/db/querier_generated.gogo/pkg/db/key_list_by_key_space_id.sql_generated.gogo/pkg/db/key_find_by_id.sql_generated.go
📚 Learning: 2025-09-11T14:24:40.988Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3944
File: go/apps/ctrl/services/deployment/deploy_workflow.go:326-334
Timestamp: 2025-09-11T14:24:40.988Z
Learning: The InsertDomains method in the bulk queries uses ON DUPLICATE KEY UPDATE, making it an upsert operation that is idempotent and safe for retries, despite the "Insert" naming convention.
Applied to files:
go/pkg/db/bulk_key_insert.sql_generated.go
📚 Learning: 2025-08-14T16:25:48.167Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Applied to files:
go/apps/api/routes/v2_keys_migrate_keys/400_test.gogo/apps/api/routes/v2_keys_update_key/200_test.gogo/apps/api/routes/v2_keys_update_credits/200_test.gogo/apps/api/routes/v2_keys_migrate_keys/401_test.gogo/apps/api/routes/v2_keys_migrate_keys/404_test.gogo/pkg/prefixedapikey/prefixedapikey_test.gogo/apps/api/routes/v2_keys_migrate_keys/200_test.gogo/apps/api/routes/v2_keys_verify_key/migration_test.gogo/apps/api/routes/v2_keys_migrate_keys/403_test.gogo/apps/api/routes/v2_keys_verify_key/resend_demo_test.go
📚 Learning: 2025-07-03T05:58:10.699Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3421
File: go/apps/api/openapi/openapi.yaml:196-200
Timestamp: 2025-07-03T05:58:10.699Z
Learning: In the Unkey codebase, OpenAPI 3.1 is used, which allows sibling keys (such as `description`) alongside `$ref` in schema objects. Do not flag this as an error in future reviews.
Applied to files:
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yamlgo/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeysRequestBody.yamlgo/apps/api/openapi/openapi-generated.yaml
📚 Learning: 2025-08-29T13:48:43.353Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3884
File: go/apps/api/routes/v2_ratelimit_delete_override/handler.go:218-228
Timestamp: 2025-08-29T13:48:43.353Z
Learning: The unkeyed/unkey codebase has identified a need for cache key normalization (forcing lowercase) to ensure consistent cache hit rates, with a preference for implementing a centralized cache.Key method to handle this standardization.
Applied to files:
go/apps/api/routes/v2_keys_update_key/200_test.go
📚 Learning: 2025-09-15T19:53:28.487Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3952
File: go/proto/ctrl/v1/routing.proto:0-0
Timestamp: 2025-09-15T19:53:28.487Z
Learning: In the Unkey codebase, authentication/authorization errors intentionally return NOT_FOUND error codes instead of distinct auth error codes (like FORBIDDEN or UNAUTHORIZED) for security reasons. This prevents attackers from distinguishing between "resource doesn't exist" and "you don't have permission to access this resource", avoiding information disclosure about workspace existence and access boundaries.
Applied to files:
go/pkg/zen/middleware_errors.gogo/pkg/codes/constants_gen.goapps/docs/errors/unkey/data/migration_not_found.mdxapps/docs/docs.json
📚 Learning: 2025-08-27T13:49:23.825Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/queries/key_find_many_by_hash.sql:1-2
Timestamp: 2025-08-27T13:49:23.825Z
Learning: In the Unkey system, key hashes are globally unique across the entire keys table, not scoped to individual workspaces. This means queries like FindKeysByHash should work globally without workspace filtering.
Applied to files:
go/pkg/db/key_find_live_by_hash.sql_generated.gogo/pkg/db/key_find_live_by_id.sql_generated.gogo/pkg/db/querier_generated.gogo/pkg/db/queries/key_find_many_by_hash.sql
📚 Learning: 2025-09-01T08:29:10.199Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3895
File: go/pkg/db/key_list_live_by_auth_id.sql_generated.go:98-105
Timestamp: 2025-09-01T08:29:10.199Z
Learning: In Unkey's ListLiveKeysByKeyAuthID query, adding ka.workspace_id = k.workspace_id constraint negatively impacts index performance. Workspace validation is handled upstream at the API level before the query is called, making the additional constraint unnecessary.
Applied to files:
go/pkg/db/key_find_live_by_hash.sql_generated.gogo/pkg/db/schema.sqlgo/pkg/db/key_find_live_by_id.sql_generated.gogo/pkg/db/key_list_live_by_key_space_id.sql_generated.gogo/pkg/db/querier_generated.gogo/pkg/db/key_list_by_key_space_id.sql_generated.go
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: In go/apps/api/openapi, oapi-codegen doesn’t support OpenAPI 3.1 union nullability; overlay.yaml must be applied before codegen. The overlay key in oapi-codegen config isn’t supported—use a pre-step (programmatic or CLI) to merge overlay into the bundled spec, then run oapi-codegen.
Applied to files:
go/pkg/codes/constants_gen.gogo/apps/api/openapi/gen.gogo/pkg/db/models_generated.go
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: For go/apps/api/openapi, oapi-codegen is used and does not support OpenAPI 3.1 union types like [T, "null"]; an overlay step is required to downconvert to 3.0-style nullable before code generation.
Applied to files:
go/pkg/codes/constants_gen.gogo/apps/api/openapi/gen.gogo/pkg/db/models_generated.go
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: In go/apps/api/openapi, overlays are required to downconvert 3.1 union nullability to 3.0-style nullable before running oapi-codegen; config.yaml’s output-options.overlay is not recognized by oapi-codegen, so overlays must be applied in a pre-step (programmatic or CLI) prior to codegen.
Applied to files:
go/pkg/codes/constants_gen.gogo/apps/api/openapi/gen.go
📚 Learning: 2025-04-22T14:43:11.724Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/002_ratelimits/006_ratelimits_per_day_v1.sql:1-13
Timestamp: 2025-04-22T14:43:11.724Z
Learning: In the unkey project, the SQL files in clickhouse/schema/databases represent the current production schema and shouldn't be modified directly in PRs. Schema changes require dedicated migration scripts.
Applied to files:
go/pkg/db/schema.sqlinternal/db/src/schema/keys.ts
📚 Learning: 2025-09-15T17:40:51.536Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3973
File: go/pkg/db/schema.sql:307-308
Timestamp: 2025-09-15T17:40:51.536Z
Learning: The environments table in the Unkey codebase is not being used in production yet, so schema changes to it don't require complex migration sequences to handle existing data or concurrent usage.
Applied to files:
go/pkg/db/schema.sqlinternal/db/src/schema/keys.ts
📚 Learning: 2025-08-21T12:37:40.996Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3821
File: apps/dashboard/lib/trpc/routers/key/updateRootKeyPermissions.ts:74-74
Timestamp: 2025-08-21T12:37:40.996Z
Learning: Root keys in Unkey have two workspace fields: `workspaceId` (always set to env().UNKEY_WORKSPACE_ID for the Unkey workspace that owns the key) and `forWorkspaceId` (set to ctx.workspace.id for the user's workspace that the key is for). When querying root keys, the system filters by forWorkspaceId to get keys for the current user's workspace, but the returned rootKey.workspaceId is always the Unkey workspace ID.
Applied to files:
go/pkg/db/key_find_live_by_id.sql_generated.gogo/pkg/db/key_list_live_by_key_space_id.sql_generated.gogo/pkg/db/querier_generated.gogo/pkg/db/key_list_by_key_space_id.sql_generated.go
📚 Learning: 2025-11-10T11:50:46.258Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4292
File: go/apps/api/openapi/openapi-generated.yaml:386-387
Timestamp: 2025-11-10T11:50:46.258Z
Learning: Repo unkeyed/unkey: It’s acceptable for pagination properties (e.g., V2IdentitiesListIdentitiesResponseBody.pagination) to omit x-go-type-skip-optional-pointer and x-go-type-skip-optional-pointer-with-omitzero; do not flag this in future reviews.
Applied to files:
go/apps/api/openapi/spec/paths/v2/keys/verifyKey/V2KeysVerifyKeyRequestBody.yaml
📚 Learning: 2025-08-20T11:41:36.718Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3810
File: go/apps/api/routes/v2_ratelimit_limit/handler.go:54-56
Timestamp: 2025-08-20T11:41:36.718Z
Learning: In the Unkey codebase, the X-Unkey-Metrics: disabled header is used by the v1 API when replaying ratelimit requests to the v2 API (go/apps/api/routes/v2_ratelimit_limit/handler.go) to prevent double billing/logging to ClickHouse. This is a legitimate internal service-to-service communication pattern for API migration scenarios, not intended for external client usage.
Applied to files:
go/apps/api/openapi/spec/paths/v2/keys/verifyKey/V2KeysVerifyKeyRequestBody.yamlgo/apps/api/openapi/openapi-generated.yaml
📚 Learning: 2024-10-04T20:44:38.489Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 2180
File: apps/dashboard/lib/constants/workspace-navigations.tsx:56-118
Timestamp: 2024-10-04T20:44:38.489Z
Learning: When typing the `workspace` parameter in functions like `createWorkspaceNavigation`, prefer importing the `Workspace` type from the database module and picking the necessary keys (e.g., `features`) instead of redefining the interface.
Applied to files:
internal/db/src/schema/keys.ts
📚 Learning: 2025-08-21T15:54:45.198Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3825
File: go/internal/services/usagelimiter/limit.go:38-0
Timestamp: 2025-08-21T15:54:45.198Z
Learning: In go/internal/services/usagelimiter/limit.go, the UpdateKeyCreditsDecrement operation cannot be safely wrapped with db.WithRetry due to the lack of idempotency mechanisms in the current tech stack. Retrying this non-idempotent write operation risks double-charging users if the first attempt commits but the client sees a transient error.
Applied to files:
go/pkg/db/querier_generated.go
📚 Learning: 2025-04-18T20:01:33.812Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3151
File: go/apps/api/openapi/gen.go:221-233
Timestamp: 2025-04-18T20:01:33.812Z
Learning: For identity deletion operations in the Unkey API, identityId takes precedence over externalId when both are provided in the request body.
Applied to files:
go/pkg/db/queries/identity_find_many_by_external_id.sql
📚 Learning: 2025-08-08T15:20:40.288Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:20:40.288Z
Learning: Repo unkeyed/unkey: oapi-codegen v2.4+ (v2.5.0 in use) supports output-options.overlay in go/apps/api/openapi/config.yaml; the generator applies overlay.yaml at codegen time, so no separate pre-step is required if oapi-codegen is invoked with -config=config.yaml.
Applied to files:
go/apps/api/openapi/gen.go
📚 Learning: 2025-07-15T14:59:30.212Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3560
File: go/deploy/metald/internal/database/repository.go:0-0
Timestamp: 2025-07-15T14:59:30.212Z
Learning: go/deploy/metald cannot currently import helpers from go/pkg/db because it is not yet part of the main Go module; avoid suggesting such imports until the modules are unified.
Applied to files:
go/apps/api/routes/v2_keys_verify_key/handler.go
🧬 Code graph analysis (28)
go/apps/api/routes/v2_keys_migrate_keys/400_test.go (4)
go/pkg/testutil/http.go (2)
NewHarness(59-208)CallRoute(385-419)go/apps/api/routes/v2_keys_migrate_keys/handler.go (2)
Handler(38-44)Request(30-30)go/pkg/testutil/seed/seed.go (1)
CreateApiRequest(84-92)go/apps/api/openapi/gen.go (2)
BadRequestErrorResponse(65-71)V2KeysMigrateKeyData(1063-1119)
go/pkg/zen/middleware_errors.go (1)
go/pkg/codes/constants_gen.go (1)
UnkeyDataErrorsMigrationNotFound(93-93)
go/pkg/db/querier_bulk_generated.go (1)
go/pkg/db/key_migration_insert.sql_generated.go (1)
InsertKeyMigrationParams(24-28)
go/pkg/db/key_find_for_verification.sql_generated.go (1)
go/pkg/db/types/null_string.go (1)
NullString(10-10)
go/apps/api/routes/register.go (2)
go/apps/api/routes/v2_keys_create_key/handler.go (1)
Handler(35-41)go/internal/services/caches/caches.go (1)
Caches(21-49)
go/internal/services/keys/get_migrated.go (5)
go/internal/services/keys/verifier.go (1)
KeyVerifier(32-57)go/pkg/db/key_migration_find_by_id.sql_generated.go (1)
FindKeyMigrationByIDParams(22-25)go/internal/services/keys/status.go (1)
StatusNotFound(16-16)go/pkg/db/models_generated.go (2)
KeyMigrationsAlgorithmGithubcomSeamapiPrefixedApiKey(327-327)Key(695-720)go/pkg/db/key_update_hash_and_migration.sql_generated.go (1)
UpdateKeyHashAndMigrationParams(23-29)
go/pkg/db/key_migration_find_by_id.sql_generated.go (1)
go/pkg/db/models_generated.go (1)
KeyMigration(735-739)
go/pkg/db/key_update_hash_and_migration.sql_generated.go (1)
go/pkg/db/types/null_string.go (1)
NullString(10-10)
go/apps/api/routes/v2_keys_migrate_keys/401_test.go (7)
go/pkg/testutil/http.go (2)
NewHarness(59-208)CallRoute(385-419)go/apps/api/routes/v2_keys_migrate_keys/handler.go (2)
Handler(38-44)Request(30-30)go/pkg/testutil/seed/seed.go (1)
CreateApiRequest(84-92)go/pkg/db/key_migration_insert.sql_generated.go (1)
InsertKeyMigrationParams(24-28)go/pkg/db/models_generated.go (1)
KeyMigrationsAlgorithmGithubcomSeamapiPrefixedApiKey(327-327)go/apps/api/openapi/gen.go (2)
V2KeysMigrateKeyData(1063-1119)UnauthorizedErrorResponse(500-506)go/pkg/uid/uid.go (1)
KeyPrefix(20-20)
go/internal/services/keys/get.go (4)
go/internal/services/keys/verifier.go (1)
KeyVerifier(32-57)go/pkg/db/cached_key_data.go (1)
CachedKeyData(5-8)go/pkg/db/key_find_for_verification.sql_generated.go (1)
FindKeyForVerificationRow(93-121)go/pkg/db/retry.go (1)
WithRetryContext(36-46)
go/pkg/db/key_find_live_by_id.sql_generated.go (1)
go/pkg/db/types/null_string.go (1)
NullString(10-10)
go/apps/api/routes/v2_keys_migrate_keys/404_test.go (6)
go/pkg/testutil/http.go (2)
NewHarness(59-208)CallRoute(385-419)go/apps/api/routes/v2_keys_migrate_keys/handler.go (2)
Handler(38-44)Request(30-30)go/pkg/testutil/seed/seed.go (1)
CreateApiRequest(84-92)go/pkg/uid/uid.go (1)
APIPrefix(22-22)go/apps/api/openapi/gen.go (2)
V2KeysMigrateKeyData(1063-1119)NotFoundErrorResponse(290-296)go/pkg/db/api_soft_delete.sql_generated.go (1)
SoftDeleteApiParams(19-22)
go/pkg/prefixedapikey/prefixedapikey_test.go (2)
go/pkg/prefixedapikey/prefixedapikey.go (10)
APIKey(26-31)HashLongToken(40-42)ExtractLongToken(126-132)ExtractShortToken(135-141)GetTokenComponents(157-164)TokenComponents(149-154)CheckAPIKey(167-177)GenerateAPIKeyOptions(18-23)GenerateAPIKey(54-123)ExtractLongTokenHash(144-146)go/pkg/uid/uid.go (1)
KeyPrefix(20-20)
go/pkg/db/key_migration_insert.sql_generated.go (1)
go/pkg/db/models_generated.go (1)
KeyMigrationsAlgorithm(324-324)
go/apps/api/routes/v2_keys_migrate_keys/200_test.go (9)
go/pkg/testutil/http.go (2)
NewHarness(59-208)CallRoute(385-419)go/apps/api/routes/v2_keys_migrate_keys/handler.go (3)
Handler(38-44)Request(30-30)Response(31-31)go/pkg/db/key_migration_insert.sql_generated.go (1)
InsertKeyMigrationParams(24-28)go/pkg/db/models_generated.go (3)
KeyMigrationsAlgorithmGithubcomSeamapiPrefixedApiKey(327-327)Key(695-720)Identity(684-693)go/pkg/prefixedapikey/prefixedapikey.go (2)
GenerateAPIKey(54-123)GenerateAPIKeyOptions(18-23)go/pkg/db/key_data.go (1)
ToKeyData(28-45)go/pkg/db/identity_find_many_by_external_id.sql_generated.go (1)
FindIdentitiesByExternalIdParams(19-23)go/pkg/db/permission_find_by_slugs.sql_generated.go (1)
FindPermissionsBySlugsParams(17-20)go/pkg/db/role_find_by_names.sql_generated.go (1)
FindRolesByNamesParams(17-20)
go/internal/services/keys/interface.go (1)
go/internal/services/keys/verifier.go (1)
KeyVerifier(32-57)
go/pkg/db/querier_generated.go (7)
go/pkg/partition/db/database.go (1)
DBTX(10-10)go/pkg/db/identity_find_many_by_external_id.sql_generated.go (1)
FindIdentitiesByExternalIdParams(19-23)go/pkg/db/models_generated.go (2)
Identity(684-693)KeyMigration(735-739)go/pkg/db/key_migration_find_by_id.sql_generated.go (1)
FindKeyMigrationByIDParams(22-25)go/pkg/db/key_find_many_by_hash.sql_generated.go (1)
FindKeysByHashRow(17-20)go/pkg/db/key_migration_insert.sql_generated.go (1)
InsertKeyMigrationParams(24-28)go/pkg/db/key_update_hash_and_migration.sql_generated.go (1)
UpdateKeyHashAndMigrationParams(23-29)
go/pkg/codes/unkey_data.go (1)
go/pkg/codes/codes.go (2)
SystemUnkey(32-32)CategoryUnkeyData(84-84)
go/apps/api/routes/v2_keys_verify_key/migration_test.go (5)
go/pkg/testutil/http.go (2)
NewHarness(59-208)CallRoute(385-419)go/pkg/db/key_migration_insert.sql_generated.go (1)
InsertKeyMigrationParams(24-28)go/pkg/db/models_generated.go (1)
Key(695-720)go/pkg/prefixedapikey/prefixedapikey.go (2)
GenerateAPIKey(54-123)GenerateAPIKeyOptions(18-23)go/apps/api/openapi/gen.go (2)
V2KeysMigrateKeyData(1063-1119)VALID(43-43)
go/apps/api/openapi/gen.go (1)
go/pkg/codes/unkey_data.go (1)
Data(119-176)
go/pkg/db/bulk_key_migration_insert.sql_generated.go (1)
go/pkg/db/key_migration_insert.sql_generated.go (1)
InsertKeyMigrationParams(24-28)
go/pkg/db/identity_find_many_by_external_id.sql_generated.go (1)
go/pkg/db/models_generated.go (2)
Identity(684-693)Environment(673-682)
go/apps/api/routes/v2_keys_migrate_keys/403_test.go (4)
go/pkg/testutil/http.go (2)
NewHarness(59-208)CallRoute(385-419)go/apps/api/routes/v2_keys_migrate_keys/handler.go (2)
Handler(38-44)Request(30-30)go/pkg/testutil/seed/seed.go (1)
CreateApiRequest(84-92)go/apps/api/openapi/gen.go (2)
V2KeysMigrateKeyData(1063-1119)ForbiddenErrorResponse(138-144)
go/apps/api/routes/v2_keys_migrate_keys/handler.go (19)
go/apps/api/openapi/gen.go (5)
V2KeysMigrateKeysRequestBody(1131-1138)V2KeysMigrateKeysResponseBody(1141-1146)Meta(279-282)V2KeysMigrateKeysMigration(1122-1128)V2KeysMigrateKeysResponseData(1149-1155)go/internal/services/keys/interface.go (1)
KeyService(11-24)go/pkg/cache/scoped_key.go (1)
ScopedKey(49-62)go/pkg/db/api_find_live_by_id.sql_generated.go (1)
FindLiveApiByIDRow(23-35)go/pkg/zen/request_util.go (1)
BindBody(10-23)go/internal/services/caches/op.go (1)
DefaultFindFirstOp(9-22)go/pkg/cache/interface.go (1)
Null(59-59)go/pkg/db/key_migration_find_by_id.sql_generated.go (1)
FindKeyMigrationByIDParams(22-25)go/pkg/db/key_insert.sql_generated.go (1)
InsertKeyParams(53-70)go/pkg/db/key_insert_ratelimit.sql_generated.go (1)
InsertKeyRatelimitParams(39-49)go/pkg/db/identity_insert.sql_generated.go (1)
InsertIdentityParams(31-38)go/pkg/db/key_role_insert.sql_generated.go (1)
InsertKeyRoleParams(27-32)go/pkg/db/key_permission_insert.sql_generated.go (1)
InsertKeyPermissionParams(27-33)go/pkg/db/role_insert.sql_generated.go (1)
InsertRoleParams(30-36)go/pkg/db/permission_insert.sql_generated.go (1)
InsertPermissionParams(33-40)go/pkg/uid/uid.go (5)
KeyPrefix(20-20)IdentityPrefix(32-32)PermissionPrefix(31-31)RolePrefix(27-27)RatelimitPrefix(33-33)go/pkg/db/tx.go (1)
Tx(196-201)go/pkg/auditlog/events.go (3)
AuthConnectPermissionKeyEvent(55-55)AuthConnectRoleKeyEvent(53-53)KeyCreateEvent(20-20)go/pkg/auditlog/target.go (4)
KeyResourceType(11-11)PermissionResourceType(12-12)RoleResourceType(16-16)APIResourceType(7-7)
go/pkg/db/key_list_by_key_space_id.sql_generated.go (1)
go/pkg/db/models_generated.go (1)
Key(695-720)
go/pkg/prefixedapikey/prefixedapikey.go (2)
go/pkg/uid/uid.go (1)
KeyPrefix(20-20)go/pkg/base58/encode.go (1)
Encode(15-58)
go/apps/api/routes/v2_keys_verify_key/resend_demo_test.go (7)
go/pkg/testutil/http.go (2)
NewHarness(59-208)CallRoute(385-419)go/pkg/testutil/seed/seed.go (1)
CreateApiRequest(84-92)go/pkg/db/key_migration_insert.sql_generated.go (1)
InsertKeyMigrationParams(24-28)go/pkg/db/models_generated.go (2)
KeyMigrationsAlgorithmGithubcomSeamapiPrefixedApiKey(327-327)Key(695-720)go/pkg/prefixedapikey/prefixedapikey.go (2)
GenerateAPIKey(54-123)GenerateAPIKeyOptions(18-23)go/pkg/uid/uid.go (1)
KeyPrefix(20-20)go/pkg/db/key_insert.sql_generated.go (1)
InsertKeyParams(53-70)
go/apps/api/routes/v2_keys_verify_key/handler.go (2)
go/pkg/db/models_generated.go (1)
Key(695-720)go/internal/services/keys/status.go (1)
StatusNotFound(16-16)
🪛 Gitleaks (8.29.0)
go/pkg/prefixedapikey/prefixedapikey_test.go
[high] 40-40: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 91-91: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 362-362: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
go/apps/api/routes/v2_keys_verify_key/resend_demo_test.go
[high] 77-77: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 78-78: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 87-87: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 91-91: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test Packages / Test
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml
Show resolved
Hide resolved
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeysRequestBody.yaml
Outdated
Show resolved
Hide resolved
chronark
reviewed
Nov 12, 2025
go/apps/api/openapi/spec/paths/v2/keys/createKey/V2KeysCreateKeyRequestBody.yaml
Show resolved
Hide resolved
chronark
reviewed
Nov 12, 2025
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml
Show resolved
Hide resolved
chronark
reviewed
Nov 12, 2025
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeysRequestBody.yaml
Outdated
Show resolved
Hide resolved
chronark
reviewed
Nov 12, 2025
Co-authored-by: Andreas Thomas <dev@chronark.com>
…teKeysRequestBody.yaml Co-authored-by: Andreas Thomas <dev@chronark.com>
chronark
approved these changes
Nov 12, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
go/apps/api/routes/v2_keys_migrate_keys/200_test.go (1)
63-65: Use explicit nullable constructor and verify RemainingRequests.The credits initialization still uses the empty struct
nullable.Nullable[int64]{}instead of the clearernullable.NewNullNullable[int64]()as suggested in the previous review. Additionally, the test doesn't verify thatRemainingRequestsis correctly set (or not set) in the database after migration.For consistency with other tests (like
TestCreateKeyWithCreditsRemainingNullin create_key tests), change line 64:- Remaining: nullable.Nullable[int64]{}, + Remaining: nullable.NewNullNullable[int64](),Then add verification after line 108 to confirm the unlimited key behavior:
require.False(t, keydata.Key.RemainingRequests.Valid, "Unlimited key should not have RemainingRequests set")
🧹 Nitpick comments (2)
go/pkg/prefixedapikey/prefixedapikey_test.go (2)
7-129: Excellent test coverage with table-driven patterns!The test setup and coverage are comprehensive. The
exampleKeyglobal provides consistent test data, and the use of table-driven tests aligns well with Go best practices.Consider consolidating the initial explicit checks (lines 24-30 and 75-81) into the table-driven test cases to reduce duplication:
func TestExtractLongToken(t *testing.T) { - result := ExtractLongToken(exampleKey.Token) - expected := exampleKey.LongToken - - if result != expected { - t.Errorf("ExtractLongToken() = %v, want %v", result, expected) - } - - // Additional test cases tests := []struct { name string token string expected string }{ + { + name: "exampleKey", + token: exampleKey.Token, + expected: exampleKey.LongToken, + }, { name: "standard token format",Apply similar consolidation to
TestExtractShortToken(lines 75-81) andTestCheckAPIKey(lines 158-164). Based on learnings.
131-156: Consider more idiomatic struct comparison.The individual field assertions work but are verbose. Consider using a struct comparison approach for cleaner code:
func TestGetTokenComponents(t *testing.T) { result := GetTokenComponents(exampleKey.Token) expected := &TokenComponents{ LongToken: exampleKey.LongToken, ShortToken: exampleKey.ShortToken, LongTokenHash: exampleKey.LongTokenHash, Token: exampleKey.Token, } - if result.LongToken != expected.LongToken { - t.Errorf("GetTokenComponents().LongToken = %v, want %v", result.LongToken, expected.LongToken) - } - - if result.ShortToken != expected.ShortToken { - t.Errorf("GetTokenComponents().ShortToken = %v, want %v", result.ShortToken, expected.ShortToken) - } - - if result.LongTokenHash != expected.LongTokenHash { - t.Errorf("GetTokenComponents().LongTokenHash = %v, want %v", result.LongTokenHash, expected.LongTokenHash) - } - - if result.Token != expected.Token { - t.Errorf("GetTokenComponents().Token = %v, want %v", result.Token, expected.Token) - } + if *result != *expected { + t.Errorf("GetTokenComponents() = %+v, want %+v", result, expected) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeysRequestBody.yaml(1 hunks)go/apps/api/routes/v2_keys_migrate_keys/200_test.go(1 hunks)go/apps/api/routes/v2_keys_migrate_keys/404_test.go(1 hunks)go/apps/api/routes/v2_keys_migrate_keys/handler.go(1 hunks)go/internal/services/keys/get.go(2 hunks)go/internal/services/keys/get_migrated.go(1 hunks)go/internal/services/keys/get_test.go(3 hunks)go/pkg/db/models_generated.go(3 hunks)go/pkg/db/schema.sql(3 hunks)go/pkg/prefixedapikey/prefixedapikey.go(1 hunks)go/pkg/prefixedapikey/prefixedapikey_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml
- go/pkg/prefixedapikey/prefixedapikey.go
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/runbook-freshness-check.yaml:157-173
Timestamp: 2025-08-08T14:59:52.283Z
Learning: Repo unkeyed/unkey: When a CI/workflow fix is deferred, imeyer prefers a thorough GitHub issue be opened with sections (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References) and assigned to imeyer, including backlinks to the originating PR and comment.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/job_detect_changes.yaml:111-114
Timestamp: 2025-08-08T16:10:00.224Z
Learning: Repo: unkeyed/unkey — When imeyer writes “make me an issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: perkinsjr
Repo: unkeyed/unkey PR: 3775
File: apps/dashboard/lib/trpc/routers/api/setDefaultBytes.ts:1-1
Timestamp: 2025-08-22T12:49:20.668Z
Learning: The team at unkeyed/unkey is comfortable with keeping TRPC schema imports from app route folders as temporary technical debt during their migration from TRPC to calling their v2 API directly, since this code will be replaced rather than maintained long-term.
📚 Learning: 2025-08-14T16:25:48.167Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Applied to files:
go/internal/services/keys/get_test.gogo/apps/api/routes/v2_keys_migrate_keys/200_test.gogo/pkg/prefixedapikey/prefixedapikey_test.gogo/apps/api/routes/v2_keys_migrate_keys/404_test.gogo/apps/api/routes/v2_keys_migrate_keys/handler.gogo/internal/services/keys/get_migrated.go
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: For go/apps/api/openapi, oapi-codegen is used and does not support OpenAPI 3.1 union types like [T, "null"]; an overlay step is required to downconvert to 3.0-style nullable before code generation.
Applied to files:
go/pkg/db/models_generated.go
📚 Learning: 2025-10-30T15:10:52.743Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Applied to files:
go/pkg/db/models_generated.gogo/pkg/db/schema.sqlgo/apps/api/routes/v2_keys_migrate_keys/200_test.go
📚 Learning: 2025-08-27T13:48:54.016Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/queries/key_find_for_verification.sql:15-15
Timestamp: 2025-08-27T13:48:54.016Z
Learning: The pending_migration_id field in FindKeyForVerification is only used internally by get_migrated.go for migration logic and doesn't need to be exposed in KeyVerifier struct or API response DTOs since it's an internal implementation detail.
Applied to files:
go/pkg/db/models_generated.gogo/internal/services/keys/get_migrated.go
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: In go/apps/api/openapi, oapi-codegen doesn’t support OpenAPI 3.1 union nullability; overlay.yaml must be applied before codegen. The overlay key in oapi-codegen config isn’t supported—use a pre-step (programmatic or CLI) to merge overlay into the bundled spec, then run oapi-codegen.
Applied to files:
go/pkg/db/models_generated.go
📚 Learning: 2025-07-17T14:24:20.403Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3631
File: go/pkg/db/bulk_keyring_insert.sql.go:23-25
Timestamp: 2025-07-17T14:24:20.403Z
Learning: In go/pkg/db/bulk_keyring_insert.sql.go and similar bulk insert generated files, hardcoded zero values for fields like size_approx and size_last_updated_at are intentional and reflect the original SQL query structure, not missing parameters.
Applied to files:
go/pkg/db/models_generated.gogo/pkg/db/schema.sql
📚 Learning: 2025-04-22T14:43:11.724Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/002_ratelimits/006_ratelimits_per_day_v1.sql:1-13
Timestamp: 2025-04-22T14:43:11.724Z
Learning: In the unkey project, the SQL files in clickhouse/schema/databases represent the current production schema and shouldn't be modified directly in PRs. Schema changes require dedicated migration scripts.
Applied to files:
go/pkg/db/schema.sql
📚 Learning: 2025-09-15T17:40:51.536Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3973
File: go/pkg/db/schema.sql:307-308
Timestamp: 2025-09-15T17:40:51.536Z
Learning: The environments table in the Unkey codebase is not being used in production yet, so schema changes to it don't require complex migration sequences to handle existing data or concurrent usage.
Applied to files:
go/pkg/db/schema.sql
📚 Learning: 2025-09-01T08:29:10.199Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3895
File: go/pkg/db/key_list_live_by_auth_id.sql_generated.go:98-105
Timestamp: 2025-09-01T08:29:10.199Z
Learning: In Unkey's ListLiveKeysByKeyAuthID query, adding ka.workspace_id = k.workspace_id constraint negatively impacts index performance. Workspace validation is handled upstream at the API level before the query is called, making the additional constraint unnecessary.
Applied to files:
go/pkg/db/schema.sqlgo/apps/api/routes/v2_keys_migrate_keys/404_test.go
📚 Learning: 2025-08-19T09:46:03.702Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3800
File: go/internal/services/usagelimiter/redis.go:176-186
Timestamp: 2025-08-19T09:46:03.702Z
Learning: Zero-cost requests in the usage limiter should not change the remaining credits but should still return the actual remaining credits for that key, not treat the key as unlimited. A key with 100 credits remaining should still report 100 credits remaining after a zero-cost request.
Applied to files:
go/apps/api/routes/v2_keys_migrate_keys/200_test.go
📚 Learning: 2025-10-21T09:45:47.560Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 4107
File: go/pkg/clickhouse/key_verifications_test.go:20-20
Timestamp: 2025-10-21T09:45:47.560Z
Learning: In ClickHouse tests (e.g., go/pkg/clickhouse/key_verifications_test.go), parallel execution with t.Parallel() is safe when tests use workspaceID-based isolation. Each test generates a unique workspaceID (via uid.New(uid.WorkspacePrefix)), and all database operations are scoped to that workspaceID, providing logical isolation even when tests share a single ClickHouse instance.
Applied to files:
go/apps/api/routes/v2_keys_migrate_keys/404_test.go
📚 Learning: 2025-04-08T09:34:24.576Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 2872
File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39
Timestamp: 2025-04-08T09:34:24.576Z
Learning: In the Unkey dashboard, when making database queries involving workspaces, use `ctx.workspace.id` directly instead of fetching the workspace separately for better performance and security.
Applied to files:
go/apps/api/routes/v2_keys_migrate_keys/404_test.go
📚 Learning: 2025-04-08T09:34:24.576Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 2872
File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39
Timestamp: 2025-04-08T09:34:24.576Z
Learning: When querying or updating namespaces in the Unkey dashboard, always scope the operations to the current workspace using `eq(table.workspaceId, ctx.workspace.id)` to prevent cross-workspace access.
Applied to files:
go/apps/api/routes/v2_keys_migrate_keys/404_test.go
📚 Learning: 2025-07-16T15:38:53.491Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3606
File: go/pkg/db/replica.go:8-11
Timestamp: 2025-07-16T15:38:53.491Z
Learning: For debugging database replica usage in go/pkg/db/replica.go, it's acceptable to mark QueryRowContext operations as "success" even though SQL errors only surface during row.Scan() calls. The timing metrics are the primary concern for debugging replica performance patterns.
Applied to files:
go/apps/api/routes/v2_keys_migrate_keys/handler.go
📚 Learning: 2025-08-27T13:46:00.608Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/key_find_live_by_id.sql_generated.go:15-16
Timestamp: 2025-08-27T13:46:00.608Z
Learning: FindLiveKeyByID doesn't need to select pending_migration_id since it's only used in key management operations that don't care about migration state. The migration flow uses other queries like FindKeyForVerification.
Applied to files:
go/internal/services/keys/get_migrated.go
📚 Learning: 2025-07-03T05:58:10.699Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3421
File: go/apps/api/openapi/openapi.yaml:196-200
Timestamp: 2025-07-03T05:58:10.699Z
Learning: In the Unkey codebase, OpenAPI 3.1 is used, which allows sibling keys (such as `description`) alongside `$ref` in schema objects. Do not flag this as an error in future reviews.
Applied to files:
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeysRequestBody.yaml
🧬 Code graph analysis (6)
go/internal/services/keys/get.go (4)
go/internal/services/keys/verifier.go (1)
KeyVerifier(32-57)go/pkg/db/cached_key_data.go (1)
CachedKeyData(5-8)go/pkg/db/key_find_for_verification.sql_generated.go (1)
FindKeyForVerificationRow(93-121)go/pkg/db/retry.go (1)
WithRetryContext(36-46)
go/apps/api/routes/v2_keys_migrate_keys/200_test.go (12)
go/pkg/testutil/http.go (2)
NewHarness(59-208)CallRoute(385-419)go/apps/api/routes/v2_keys_migrate_keys/handler.go (3)
Handler(38-44)Request(30-30)Response(31-31)go/apps/api/routes/register.go (1)
Register(65-618)go/pkg/testutil/seed/seed.go (1)
CreateApiRequest(84-92)go/pkg/db/key_migration_insert.sql_generated.go (1)
InsertKeyMigrationParams(24-28)go/pkg/db/models_generated.go (3)
KeyMigrationsAlgorithmGithubcomSeamapiPrefixedApiKey(328-328)Key(696-721)Identity(685-694)go/pkg/prefixedapikey/prefixedapikey.go (2)
GenerateAPIKey(58-127)GenerateAPIKeyOptions(22-27)go/apps/api/openapi/gen.go (5)
V2KeysMigrateKeyData(1063-1119)KeyCreditsData(190-196)Meta(279-282)RatelimitRequest(381-417)Identity(161-173)go/pkg/db/key_data.go (1)
ToKeyData(28-45)go/pkg/db/identity_find_many_by_external_id.sql_generated.go (1)
FindIdentitiesByExternalIdParams(19-23)go/pkg/db/permission_find_by_slugs.sql_generated.go (1)
FindPermissionsBySlugsParams(17-20)go/pkg/db/role_find_by_names.sql_generated.go (1)
FindRolesByNamesParams(17-20)
go/pkg/prefixedapikey/prefixedapikey_test.go (1)
go/pkg/prefixedapikey/prefixedapikey.go (10)
APIKey(30-35)HashLongToken(44-46)ExtractLongToken(130-136)ExtractShortToken(139-145)GetTokenComponents(161-168)TokenComponents(153-158)CheckAPIKey(171-181)GenerateAPIKeyOptions(22-27)GenerateAPIKey(58-127)ExtractLongTokenHash(148-150)
go/apps/api/routes/v2_keys_migrate_keys/404_test.go (6)
go/pkg/testutil/http.go (2)
NewHarness(59-208)CallRoute(385-419)go/apps/api/routes/v2_keys_migrate_keys/handler.go (2)
Handler(38-44)Request(30-30)go/pkg/testutil/seed/seed.go (1)
CreateApiRequest(84-92)go/pkg/uid/uid.go (1)
APIPrefix(22-22)go/apps/api/openapi/gen.go (2)
V2KeysMigrateKeyData(1063-1119)NotFoundErrorResponse(290-296)go/pkg/db/api_soft_delete.sql_generated.go (1)
SoftDeleteApiParams(19-22)
go/apps/api/routes/v2_keys_migrate_keys/handler.go (21)
go/apps/api/openapi/gen.go (6)
V2KeysMigrateKeysRequestBody(1131-1138)V2KeysMigrateKeysResponseBody(1141-1146)Meta(279-282)KeyCreditsRefillIntervalMonthly(17-17)V2KeysMigrateKeysMigration(1122-1128)V2KeysMigrateKeysResponseData(1149-1155)go/internal/services/keys/interface.go (1)
KeyService(11-24)go/pkg/cache/scoped_key.go (1)
ScopedKey(49-62)go/pkg/db/api_find_live_by_id.sql_generated.go (1)
FindLiveApiByIDRow(23-35)go/pkg/zen/request_util.go (1)
BindBody(10-23)go/internal/services/caches/op.go (1)
DefaultFindFirstOp(9-22)go/pkg/db/key_migration_find_by_id.sql_generated.go (1)
FindKeyMigrationByIDParams(22-25)go/pkg/db/key_insert.sql_generated.go (1)
InsertKeyParams(53-70)go/pkg/db/key_insert_ratelimit.sql_generated.go (1)
InsertKeyRatelimitParams(39-49)go/pkg/db/identity_insert.sql_generated.go (1)
InsertIdentityParams(31-38)go/pkg/db/key_role_insert.sql_generated.go (1)
InsertKeyRoleParams(27-32)go/pkg/db/key_permission_insert.sql_generated.go (1)
InsertKeyPermissionParams(27-33)go/pkg/db/role_insert.sql_generated.go (1)
InsertRoleParams(30-36)go/pkg/db/permission_insert.sql_generated.go (1)
InsertPermissionParams(33-40)go/pkg/uid/uid.go (5)
KeyPrefix(20-20)IdentityPrefix(32-32)PermissionPrefix(31-31)RolePrefix(27-27)RatelimitPrefix(33-33)go/pkg/db/tx.go (1)
Tx(196-201)go/pkg/db/identity_find_many_by_external_id.sql_generated.go (1)
FindIdentitiesByExternalIdParams(19-23)go/pkg/db/permission_find_by_slugs.sql_generated.go (1)
FindPermissionsBySlugsParams(17-20)go/pkg/db/role_find_by_names.sql_generated.go (1)
FindRolesByNamesParams(17-20)go/pkg/auditlog/events.go (3)
AuthConnectPermissionKeyEvent(55-55)AuthConnectRoleKeyEvent(53-53)KeyCreateEvent(20-20)go/pkg/auditlog/target.go (4)
KeyResourceType(11-11)PermissionResourceType(12-12)RoleResourceType(16-16)APIResourceType(7-7)
go/internal/services/keys/get_migrated.go (5)
go/internal/services/keys/verifier.go (1)
KeyVerifier(32-57)go/pkg/db/key_migration_find_by_id.sql_generated.go (1)
FindKeyMigrationByIDParams(22-25)go/internal/services/keys/status.go (1)
StatusNotFound(16-16)go/pkg/db/models_generated.go (2)
KeyMigrationsAlgorithmGithubcomSeamapiPrefixedApiKey(328-328)Key(696-721)go/pkg/db/key_update_hash_and_migration.sql_generated.go (1)
UpdateKeyHashAndMigrationParams(23-29)
🪛 Gitleaks (8.29.0)
go/pkg/prefixedapikey/prefixedapikey_test.go
[high] 40-40: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 91-91: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 354-354: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (9)
go/pkg/prefixedapikey/prefixedapikey_test.go (1)
158-362: Excellent comprehensive test coverage!The remaining test functions are well-structured:
TestCheckAPIKeycovers valid/invalid cases and edge conditionsTestGenerateAPIKeythoroughly validates structure, hashing, and validationTestGenerateAPIKeyConsistencyensures uniqueness across multiple generationsTestExtractLongTokenHashverifies the helper functionAll tests follow Go best practices with clear assertions and good edge case coverage.
Note: The Gitleaks warnings (lines 40, 91, 354) are false positives. These are test fixture strings (e.g.,
"test_12345678_abcdefghijklmnopqrstuvwx"), not real API keys. No remediation needed.go/internal/services/keys/get_test.go (1)
50-50: LGTM! Error message updates align with API signature change.The error message updates from "rawKey is empty" to "sha256Hash is empty" are consistent with the parameter rename in the Get function signature. The test correctly validates the new error messaging.
Also applies to: 65-65, 86-86
go/internal/services/keys/get.go (1)
61-67: LGTM! Signature change enables migration support.The refactoring to accept
sha256Hashdirectly instead ofrawKeyis necessary for the migration feature, where callers may need to derive hashes using different algorithms. The error message correctly references the new parameter name.go/apps/api/routes/v2_keys_migrate_keys/404_test.go (1)
88-119: LGTM! Cross-workspace isolation test now correct.The previous issue where
otherApiwas created in the wrong workspace has been fixed. Line 94 correctly usesotherWorkspace.ID, properly testing cross-workspace isolation. The test suite comprehensively covers various 404 scenarios.go/internal/services/keys/get_migrated.go (1)
59-71: LGTM! Prefixed key parsing now handles underscores correctly.The previous issue where
strings.Splitwould break keys with underscores in the prefix has been fixed by usingstrings.SplitN(rawKey, "_", 3). This ensuresparts[2]always contains the full long token regardless of underscores in the prefix.go/pkg/db/schema.sql (1)
98-103: LGTM! Schema changes enable migration tracking.The new
key_migrationstable,pending_migration_idcolumn, and corresponding index provide the necessary database structure for tracking migration state. The composite primary key on(id, workspace_id)ensures proper workspace isolation for migration configurations.Also applies to: 129-129, 422-422
go/pkg/db/models_generated.go (1)
324-364: LGTM! Generated code follows established patterns.The new
KeyMigrationsAlgorithmenum, nullable wrapper, updatedKeystruct withPendingMigrationID, and newKeyMigrationstruct are all correctly generated from the schema changes. The code follows the same patterns as other enum types in this file.Also applies to: 696-721, 736-740
go/apps/api/routes/v2_keys_migrate_keys/200_test.go (1)
119-215: Excellent test coverage for entity reuse.This subtest thoroughly validates that identities, permissions, and roles are correctly reused across multiple key migrations instead of creating duplicates. The pre- and post-migration verification ensures both the reuse logic and the integrity constraints work correctly. This is exactly the type of integration test needed for a batch migration feature.
go/apps/api/routes/v2_keys_migrate_keys/handler.go (1)
266-281: LGTM - Transaction isolation fixed.The duplicate hash lookup now correctly uses the transaction handle (
tx), resolving the critical issue from the previous review. All queries within the transaction properly usetx, ensuring proper isolation and preventing race conditions where recently inserted hashes could be missed.
Graphite Automations"Notify author when CI fails" took an action on this PR • (11/12/25)1 teammate was notified to this PR based on Andreas Thomas's automation. "Post a GIF when PR approved" took an action on this PR • (11/13/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
go/pkg/prefixedapikey/cmd/main.go (1)
36-68: LGTM! Solid implementation.The function correctly constructs options from CLI flags, validates the count parameter, and generates the requested keys. Error handling with
%wis appropriate for error wrapping.Optional: Consider checking context cancellation in the loop
For long-running operations (generating many keys), you could add context cancellation checks:
for i := 0; i < count; i++ { + if err := ctx.Err(); err != nil { + return fmt.Errorf("operation cancelled: %w", err) + } key, err := prefixedapikey.GenerateAPIKey(opts)This allows users to interrupt generation with Ctrl+C gracefully.
go/internal/services/keys/get_migrated.go (2)
92-98: Consider adding a clarifying comment for the sha256 case.The immediate
StatusNotFoundreturn is correct—if a key is already sha256-hashed and wasn't found in the initial lookup, it truly doesn't exist and there's nothing to migrate. A brief inline comment explaining this flow would help future maintainers.
111-133: Consider adding structured logging for successful migrations.Currently, only tracing captures the migration flow. Adding a log entry when a key is successfully migrated would improve production observability by capturing the
migrationID, key ID, and algorithm used.Example:
if key.Key.PendingMigrationID.Valid && key.Key.PendingMigrationID.String == migrationID { newHash := hash.Sha256(rawKey) err = db.Query.UpdateKeyHashAndMigration(ctx, s.db.RW(), db.UpdateKeyHashAndMigrationParams{ ID: key.Key.ID, Hash: newHash, Start: start, PendingMigrationID: sql.NullString{Valid: false, String: ""}, UpdatedAtM: sql.NullInt64{Valid: true, Int64: time.Now().UnixMilli()}, }) if err != nil { return nil, log, fault.Wrap( err, fault.Code(codes.App.Internal.UnexpectedError.URN()), fault.Public("We could not update the key hash and migration id"), ) } s.logger.Info(ctx, "key migrated successfully", "keyID", key.Key.ID, "migrationID", migrationID, "algorithm", migration.Algorithm, ) s.keyCache.Remove( ctx, h, newHash, ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml(1 hunks)go/apps/api/routes/register.go(2 hunks)go/internal/services/keys/get_migrated.go(1 hunks)go/pkg/prefixedapikey/cmd/main.go(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/runbook-freshness-check.yaml:157-173
Timestamp: 2025-08-08T14:59:52.283Z
Learning: Repo unkeyed/unkey: When a CI/workflow fix is deferred, imeyer prefers a thorough GitHub issue be opened with sections (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References) and assigned to imeyer, including backlinks to the originating PR and comment.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/job_detect_changes.yaml:111-114
Timestamp: 2025-08-08T16:10:00.224Z
Learning: Repo: unkeyed/unkey — When imeyer writes “make me an issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/queries/key_find_for_verification.sql:15-15
Timestamp: 2025-08-27T13:48:54.016Z
Learning: The pending_migration_id field in FindKeyForVerification is only used internally by get_migrated.go for migration logic and doesn't need to be exposed in KeyVerifier struct or API response DTOs since it's an internal implementation detail.
📚 Learning: 2025-08-27T13:48:54.016Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/queries/key_find_for_verification.sql:15-15
Timestamp: 2025-08-27T13:48:54.016Z
Learning: The pending_migration_id field in FindKeyForVerification is only used internally by get_migrated.go for migration logic and doesn't need to be exposed in KeyVerifier struct or API response DTOs since it's an internal implementation detail.
Applied to files:
go/internal/services/keys/get_migrated.go
📚 Learning: 2025-08-27T13:46:00.608Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/key_find_live_by_id.sql_generated.go:15-16
Timestamp: 2025-08-27T13:46:00.608Z
Learning: FindLiveKeyByID doesn't need to select pending_migration_id since it's only used in key management operations that don't care about migration state. The migration flow uses other queries like FindKeyForVerification.
Applied to files:
go/internal/services/keys/get_migrated.go
📚 Learning: 2025-08-14T16:25:48.167Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Applied to files:
go/internal/services/keys/get_migrated.go
📚 Learning: 2025-08-08T15:20:40.288Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:20:40.288Z
Learning: Repo unkeyed/unkey: oapi-codegen v2.4+ (v2.5.0 in use) supports output-options.overlay in go/apps/api/openapi/config.yaml; the generator applies overlay.yaml at codegen time, so no separate pre-step is required if oapi-codegen is invoked with -config=config.yaml.
Applied to files:
go/pkg/prefixedapikey/cmd/main.go
📚 Learning: 2025-10-30T15:10:52.743Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Applied to files:
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml
📚 Learning: 2025-07-03T05:58:10.699Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3421
File: go/apps/api/openapi/openapi.yaml:196-200
Timestamp: 2025-07-03T05:58:10.699Z
Learning: In the Unkey codebase, OpenAPI 3.1 is used, which allows sibling keys (such as `description`) alongside `$ref` in schema objects. Do not flag this as an error in future reviews.
Applied to files:
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml
🧬 Code graph analysis (3)
go/internal/services/keys/get_migrated.go (5)
go/internal/services/keys/verifier.go (1)
KeyVerifier(32-57)go/pkg/db/key_migration_find_by_id.sql_generated.go (1)
FindKeyMigrationByIDParams(22-25)go/internal/services/keys/status.go (1)
StatusNotFound(16-16)go/pkg/db/models_generated.go (3)
KeyMigrationsAlgorithmGithubcomSeamapiPrefixedApiKey(328-328)KeyMigrationsAlgorithmSha256(327-327)Key(696-721)go/pkg/db/key_update_hash_and_migration.sql_generated.go (1)
UpdateKeyHashAndMigrationParams(23-29)
go/apps/api/routes/register.go (2)
go/apps/api/routes/v2_apis_list_keys/handler.go (1)
Handler(30-36)go/internal/services/caches/caches.go (1)
Caches(21-49)
go/pkg/prefixedapikey/cmd/main.go (3)
go/pkg/cli/command.go (2)
Action(21-21)Exit(325-329)go/pkg/cli/flag.go (5)
Flag(20-27)String(419-453)Required(298-317)Int(538-577)Bool(497-535)go/pkg/prefixedapikey/prefixedapikey.go (3)
GenerateAPIKeyOptions(22-27)APIKey(30-35)GenerateAPIKey(58-127)
🔇 Additional comments (7)
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml (1)
127-149: Previous example feedback has been resolved.The example
basicKeynow includes a completevaluesection with realistic, representative data across the main fields: hash, name, externalId, enabled, meta (with nested structure), permissions, roles, expires, and ratelimits. This resolves the prior feedback about incomplete examples and will enable proper documentation generation.go/apps/api/routes/register.go (2)
47-47: LGTM! Import follows established conventions.The import for the new migration endpoint is correctly placed and follows the existing naming pattern.
407-417: LGTM! Route registration is well-structured.The handler dependencies are appropriate for a migration endpoint:
ApiCachevalidates the target API exists and is liveDBhandles batch key insertionAuditlogstracks the migration operationKeysprovides key service operationsThe absence of
Vaultis correct since migrated keys are already hashed (as indicated in the PR description).go/pkg/prefixedapikey/cmd/main.go (2)
1-34: LGTM! Clean CLI setup.The package structure, imports, and CLI command setup are well-organized. The flag definitions properly use
cli.Required()for the mandatory prefix andcli.Default()for optional parameters. Error handling is appropriate for a CLI application.
70-74: LGTM!Clean JSON output implementation with proper indentation for readability.
go/internal/services/keys/get_migrated.go (2)
63-91: Excellent fix for multi-underscore prefixes.The right-to-left parsing correctly handles prefixes containing underscores. Since
shortTokenandlongTokennever contain underscores (per the format spec), extracting the last two segments as tokens and joining the rest as the prefix is the right approach.
128-132: CacheRemoveAPI call is correct—it accepts variadic hash arguments.The
Removemethod ingo/pkg/cache/cache.goaccepts variadic keys:Remove(ctx context.Context, keys ...K), so passing bothhandnewHashin a single call is the intended usage pattern.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
go/internal/services/keys/get_migrated.go (1)
63-90: Prefixed key parsing still breaks when the long token contains_.
strings.Split(rawKey, "_")still mis-parses valid prefixed keys. For a Seam key likedemo_prod_AB12_OqV_t0ken(the long token is URL-safe base64 and may contain_), the slice becomes["demo","prod","AB12","OqV","t0ken"]. Your logic then treatsshortToken := parts[len(parts)-2]→"OqV"andlongToken := parts[len(parts)-1]→"t0ken", so the migration hashes the wrong value, never finds the key, and the flow returns NOT_FOUND for perfectly valid keys. We need to stop splitting inside the long token. Please limit the split to the first two separators (e.g., use the diff suggested earlier withstrings.SplitN(rawKey, "_", 3)followed by a secondSplitNon the tail, or delegate toprefixedapikey.Parse) so the suffix always retains the entire long token.
🧹 Nitpick comments (1)
go/pkg/prefixedapikey/cmd/main.go (1)
36-50: Add validation for length parameters and consider a count upper bound.While count validation ensures at least 1 key is generated, consider adding:
- Validation for
short-lengthandlong-lengthto ensure they are positive values (e.g.,>= 1).- An upper bound on
count(e.g., 1000 or 10000) to prevent excessive memory usage or long execution times.Apply this diff to add validation:
count := cmd.Int("count") useJSON := cmd.Bool("json") // Validate count if count < 1 { return fmt.Errorf("count must be at least 1") } + if count > 10000 { + return fmt.Errorf("count must not exceed 10000") + } + + // Validate lengths + if opts.ShortTokenLength < 1 { + return fmt.Errorf("short-length must be at least 1") + } + if opts.LongTokenLength < 1 { + return fmt.Errorf("long-length must be at least 1") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml(1 hunks)go/apps/api/routes/register.go(2 hunks)go/internal/services/keys/get_migrated.go(1 hunks)go/pkg/prefixedapikey/cmd/main.go(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/runbook-freshness-check.yaml:157-173
Timestamp: 2025-08-08T14:59:52.283Z
Learning: Repo unkeyed/unkey: When a CI/workflow fix is deferred, imeyer prefers a thorough GitHub issue be opened with sections (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References) and assigned to imeyer, including backlinks to the originating PR and comment.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/job_detect_changes.yaml:111-114
Timestamp: 2025-08-08T16:10:00.224Z
Learning: Repo: unkeyed/unkey — When imeyer writes “make me an issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: perkinsjr
Repo: unkeyed/unkey PR: 3775
File: apps/dashboard/lib/trpc/routers/api/setDefaultBytes.ts:1-1
Timestamp: 2025-08-22T12:49:20.668Z
Learning: The team at unkeyed/unkey is comfortable with keeping TRPC schema imports from app route folders as temporary technical debt during their migration from TRPC to calling their v2 API directly, since this code will be replaced rather than maintained long-term.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/queries/key_find_for_verification.sql:15-15
Timestamp: 2025-08-27T13:48:54.016Z
Learning: The pending_migration_id field in FindKeyForVerification is only used internally by get_migrated.go for migration logic and doesn't need to be exposed in KeyVerifier struct or API response DTOs since it's an internal implementation detail.
📚 Learning: 2025-07-03T05:58:10.699Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3421
File: go/apps/api/openapi/openapi.yaml:196-200
Timestamp: 2025-07-03T05:58:10.699Z
Learning: In the Unkey codebase, OpenAPI 3.1 is used, which allows sibling keys (such as `description`) alongside `$ref` in schema objects. Do not flag this as an error in future reviews.
Applied to files:
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml
📚 Learning: 2025-10-30T15:10:52.743Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Applied to files:
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: In go/apps/api/openapi, oapi-codegen doesn’t support OpenAPI 3.1 union nullability; overlay.yaml must be applied before codegen. The overlay key in oapi-codegen config isn’t supported—use a pre-step (programmatic or CLI) to merge overlay into the bundled spec, then run oapi-codegen.
Applied to files:
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml
📚 Learning: 2025-08-27T13:48:54.016Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/queries/key_find_for_verification.sql:15-15
Timestamp: 2025-08-27T13:48:54.016Z
Learning: The pending_migration_id field in FindKeyForVerification is only used internally by get_migrated.go for migration logic and doesn't need to be exposed in KeyVerifier struct or API response DTOs since it's an internal implementation detail.
Applied to files:
go/internal/services/keys/get_migrated.go
📚 Learning: 2025-08-14T16:25:48.167Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Applied to files:
go/internal/services/keys/get_migrated.go
📚 Learning: 2025-08-27T13:46:00.608Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/key_find_live_by_id.sql_generated.go:15-16
Timestamp: 2025-08-27T13:46:00.608Z
Learning: FindLiveKeyByID doesn't need to select pending_migration_id since it's only used in key management operations that don't care about migration state. The migration flow uses other queries like FindKeyForVerification.
Applied to files:
go/internal/services/keys/get_migrated.go
🧬 Code graph analysis (3)
go/apps/api/routes/register.go (4)
go/apps/api/routes/v2_apis_list_keys/handler.go (1)
Handler(30-36)go/apps/api/routes/v2_keys_create_key/handler.go (1)
Handler(35-41)go/apps/api/routes/v2_keys_get_key/handler.go (1)
Handler(28-34)go/internal/services/caches/caches.go (1)
Caches(21-49)
go/internal/services/keys/get_migrated.go (5)
go/internal/services/keys/verifier.go (1)
KeyVerifier(32-57)go/pkg/db/key_migration_find_by_id.sql_generated.go (1)
FindKeyMigrationByIDParams(22-25)go/internal/services/keys/status.go (1)
StatusNotFound(16-16)go/pkg/db/models_generated.go (3)
KeyMigrationsAlgorithmGithubcomSeamapiPrefixedApiKey(328-328)KeyMigrationsAlgorithmSha256(327-327)Key(696-721)go/pkg/db/key_update_hash_and_migration.sql_generated.go (1)
UpdateKeyHashAndMigrationParams(23-29)
go/pkg/prefixedapikey/cmd/main.go (3)
go/pkg/cli/command.go (2)
Action(21-21)Exit(325-329)go/pkg/cli/flag.go (5)
Flag(20-27)String(419-453)Required(298-317)Int(538-577)Bool(497-535)go/pkg/prefixedapikey/prefixedapikey.go (3)
GenerateAPIKeyOptions(22-27)APIKey(30-35)GenerateAPIKey(58-127)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build / Build
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Packages / Test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
go/apps/api/routes/register.go (1)
407-417: Route registration looks good.The new v2/keys.migrateKeys route is properly registered with appropriate dependencies and middleware. The handler dependencies (Logger, ApiCache, DB, Auditlogs, Keys) are well-suited for a migration endpoint, and the absence of Vault makes sense since migrations work with pre-existing key hashes rather than generating new keys.
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml (1)
131-149: Example value is now complete.The previous review concern about the missing example value has been addressed. The example now includes all relevant fields with realistic values, making the API documentation clear and complete.
go/pkg/prefixedapikey/cmd/main.go (3)
13-28: LGTM! Clear CLI definition with appropriate flag setup.The CLI command structure is well-organized with clear descriptions and appropriate defaults. The required flag on
prefixensures the key requirement is met.
52-60: LGTM! Key generation logic is correct.The loop correctly generates keys and handles errors appropriately with proper error wrapping.
70-74: LGTM! Clean JSON output implementation.Proper use of
json.Encoderwith indentation for readable output.
go/apps/api/openapi/spec/paths/v2/keys/migrateKeys/V2KeysMigrateKeyData.yaml
Show resolved
Hide resolved
ogzhanolguncu
approved these changes
Nov 13, 2025
|
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
go/apps/api/openapi/openapi-generated.yaml (3)
2704-2829: Harden migrate payload item schema (hash length, examples, field patterns)Strengthen constraints and align with create/update schemas for consistency.
Apply:
hash: type: string minLength: 3 - description: The current hash of the key on your side - example: qwerty123 + maxLength: 1024 + description: The current hash of the key in your system (e.g., raw SHA-256 hex or prefixed form like "sha256_<hex>"). + example: sha256_0123abcd externalId: type: string minLength: 1 maxLength: 255 + pattern: "^[a-zA-Z0-9_.-]+$" description: | Links this key to a user or entity in your system using your own identifier. roles: type: array maxItems: 100 items: type: string minLength: 1 maxLength: 100 + pattern: "^[a-zA-Z0-9_:\\-\\.\\*]+$" permissions: type: array maxItems: 1000 items: type: string minLength: 1 maxLength: 100 + pattern: "^[a-zA-Z][a-zA-Z0-9._-]*$"Note: keeping hash pattern flexible to allow multiple algorithms and prefixed forms.
2831-2861: Include structured failure reasons in migrateKeys responseCurrent
failedreturns only hashes; add an optional structuredfailuresarray without breaking the existing field.Apply:
V2KeysMigrateKeysResponseData: type: object required: - migrated - failed properties: migrated: type: array description: Successfully migrated keys with their hash and generated keyId items: "$ref": "#/components/schemas/V2KeysMigrateKeysMigration" failed: type: array description: Hashes that could not be migrated (e.g., already exist in the system) items: type: string description: The hash that failed to migrate example: sha256_ghi789jkl012 + failures: + type: array + description: Detailed failure entries for troubleshooting and retries. + items: + type: object + required: + - hash + - code + properties: + hash: + type: string + code: + type: string + enum: [ALREADY_EXISTS, INVALID_HASH, MIGRATION_NOT_FOUND, INTERNAL_ERROR] + detail: + type: stringThis improves UX and retry logic while preserving backward compatibility.
4868-4927: Clarify idempotency and partial success semantics; align limitsDocument that the operation is idempotent by hash and reference the batch cap; consider adding 207 to examples if you ever expose multi-status in the future (keeping 200 as the contract).
Apply:
description: | - Returns HTTP 200 even on partial success; hashes that could not be migrated are listed under `data.failed`. + Returns HTTP 200 even on partial success; hashes that could not be migrated are listed under `data.failed`. + Idempotent by hash: submitting the same hash multiple times will not create duplicates and will surface under `data.failed`. + Large batches are accepted up to the documented per-request cap (see schema: `keys.maxItems`).No response code change required now; this is doc clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
go/apps/api/openapi/gen.go(3 hunks)go/apps/api/openapi/openapi-generated.yaml(5 hunks)go/apps/api/openapi/openapi-split.yaml(1 hunks)go/apps/api/routes/register.go(2 hunks)go/pkg/db/querier_generated.go(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- go/apps/api/openapi/openapi-split.yaml
- go/apps/api/openapi/gen.go
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/runbook-freshness-check.yaml:157-173
Timestamp: 2025-08-08T14:59:52.283Z
Learning: Repo unkeyed/unkey: When a CI/workflow fix is deferred, imeyer prefers a thorough GitHub issue be opened with sections (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References) and assigned to imeyer, including backlinks to the originating PR and comment.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/job_detect_changes.yaml:111-114
Timestamp: 2025-08-08T16:10:00.224Z
Learning: Repo: unkeyed/unkey — When imeyer writes “make me an issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/queries/key_find_for_verification.sql:15-15
Timestamp: 2025-08-27T13:48:54.016Z
Learning: The pending_migration_id field in FindKeyForVerification is only used internally by get_migrated.go for migration logic and doesn't need to be exposed in KeyVerifier struct or API response DTOs since it's an internal implementation detail.
📚 Learning: 2025-10-30T15:10:52.743Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Applied to files:
go/pkg/db/querier_generated.go
📚 Learning: 2025-08-27T13:48:54.016Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/queries/key_find_for_verification.sql:15-15
Timestamp: 2025-08-27T13:48:54.016Z
Learning: The pending_migration_id field in FindKeyForVerification is only used internally by get_migrated.go for migration logic and doesn't need to be exposed in KeyVerifier struct or API response DTOs since it's an internal implementation detail.
Applied to files:
go/pkg/db/querier_generated.gogo/apps/api/openapi/openapi-generated.yaml
📚 Learning: 2025-08-27T13:46:00.608Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/key_find_live_by_id.sql_generated.go:15-16
Timestamp: 2025-08-27T13:46:00.608Z
Learning: FindLiveKeyByID doesn't need to select pending_migration_id since it's only used in key management operations that don't care about migration state. The migration flow uses other queries like FindKeyForVerification.
Applied to files:
go/pkg/db/querier_generated.go
📚 Learning: 2025-08-27T13:49:23.825Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/queries/key_find_many_by_hash.sql:1-2
Timestamp: 2025-08-27T13:49:23.825Z
Learning: In the Unkey system, key hashes are globally unique across the entire keys table, not scoped to individual workspaces. This means queries like FindKeysByHash should work globally without workspace filtering.
Applied to files:
go/pkg/db/querier_generated.go
📚 Learning: 2025-09-01T08:29:10.199Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3895
File: go/pkg/db/key_list_live_by_auth_id.sql_generated.go:98-105
Timestamp: 2025-09-01T08:29:10.199Z
Learning: In Unkey's ListLiveKeysByKeyAuthID query, adding ka.workspace_id = k.workspace_id constraint negatively impacts index performance. Workspace validation is handled upstream at the API level before the query is called, making the additional constraint unnecessary.
Applied to files:
go/pkg/db/querier_generated.go
📚 Learning: 2025-07-17T14:24:20.403Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3631
File: go/pkg/db/bulk_keyring_insert.sql.go:23-25
Timestamp: 2025-07-17T14:24:20.403Z
Learning: In go/pkg/db/bulk_keyring_insert.sql.go and similar bulk insert generated files, hardcoded zero values for fields like size_approx and size_last_updated_at are intentional and reflect the original SQL query structure, not missing parameters.
Applied to files:
go/pkg/db/querier_generated.go
📚 Learning: 2025-08-21T12:37:40.996Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3821
File: apps/dashboard/lib/trpc/routers/key/updateRootKeyPermissions.ts:74-74
Timestamp: 2025-08-21T12:37:40.996Z
Learning: Root keys in Unkey have two workspace fields: `workspaceId` (always set to env().UNKEY_WORKSPACE_ID for the Unkey workspace that owns the key) and `forWorkspaceId` (set to ctx.workspace.id for the user's workspace that the key is for). When querying root keys, the system filters by forWorkspaceId to get keys for the current user's workspace, but the returned rootKey.workspaceId is always the Unkey workspace ID.
Applied to files:
go/pkg/db/querier_generated.go
📚 Learning: 2025-08-21T15:54:45.198Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3825
File: go/internal/services/usagelimiter/limit.go:38-0
Timestamp: 2025-08-21T15:54:45.198Z
Learning: In go/internal/services/usagelimiter/limit.go, the UpdateKeyCreditsDecrement operation cannot be safely wrapped with db.WithRetry due to the lack of idempotency mechanisms in the current tech stack. Retrying this non-idempotent write operation risks double-charging users if the first attempt commits but the client sees a transient error.
Applied to files:
go/pkg/db/querier_generated.go
📚 Learning: 2025-07-03T05:58:10.699Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3421
File: go/apps/api/openapi/openapi.yaml:196-200
Timestamp: 2025-07-03T05:58:10.699Z
Learning: In the Unkey codebase, OpenAPI 3.1 is used, which allows sibling keys (such as `description`) alongside `$ref` in schema objects. Do not flag this as an error in future reviews.
Applied to files:
go/apps/api/openapi/openapi-generated.yaml
📚 Learning: 2025-08-20T11:41:36.718Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3810
File: go/apps/api/routes/v2_ratelimit_limit/handler.go:54-56
Timestamp: 2025-08-20T11:41:36.718Z
Learning: In the Unkey codebase, the X-Unkey-Metrics: disabled header is used by the v1 API when replaying ratelimit requests to the v2 API (go/apps/api/routes/v2_ratelimit_limit/handler.go) to prevent double billing/logging to ClickHouse. This is a legitimate internal service-to-service communication pattern for API migration scenarios, not intended for external client usage.
Applied to files:
go/apps/api/openapi/openapi-generated.yaml
🧬 Code graph analysis (2)
go/pkg/db/querier_generated.go (6)
go/pkg/db/identity_find_many_by_external_id.sql_generated.go (1)
FindIdentitiesByExternalIdParams(19-23)go/pkg/db/models_generated.go (2)
Identity(685-694)KeyMigration(736-740)go/pkg/db/key_migration_find_by_id.sql_generated.go (1)
FindKeyMigrationByIDParams(22-25)go/pkg/db/key_find_many_by_hash.sql_generated.go (1)
FindKeysByHashRow(17-20)go/pkg/db/key_migration_insert.sql_generated.go (1)
InsertKeyMigrationParams(24-28)go/pkg/db/key_update_hash_and_migration.sql_generated.go (1)
UpdateKeyHashAndMigrationParams(23-29)
go/apps/api/routes/register.go (1)
go/internal/services/caches/caches.go (1)
Caches(21-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
go/apps/api/openapi/openapi-generated.yaml (4)
695-695: Increase to 255 looks goodMax length aligns with other name fields elsewhere. No action.
947-955: Response envelope OKShape matches project conventions (meta + data).
921-946: Based on my verification of the codebase via the script output, I can now assess the review comment. The grep results confirm that theV2KeysMigrateKeysRequestBodyschema in the OpenAPI spec currently lacks:
- Pattern validation for
migrationId- Pattern validation for
apiIdmaxItemsconstraint on thekeysarrayThese are reasonable additions for API specification. Industry practices show batch endpoints should have size limits (1000 is a common, sensible upper bound referenced by multiple APIs). The learnings from the repository confirm that input validation patterns are properly defined at the OpenAPI schema layer.
However, I need to verify one more detail: whether these are new suggestions or if there's a mismatch between the source specs and the generated output.
Tighten request validation and batch size for migrateKeys
Add consistent patterns and batch cap to reduce bad inputs and accidental huge payloads.
Apply:
migrationId: type: string minLength: 3 maxLength: 255 + pattern: "^[a-zA-Z][a-zA-Z0-9._-]*$" description: Identifier of the configured migration provider/strategy to use (e.g., "your_company"). apiId: type: string minLength: 3 maxLength: 255 + pattern: "^[a-zA-Z0-9_]+$" description: The ID of the API that the keys should be inserted into keys: type: array minItems: 1 + maxItems: 1000 items: $ref: "#/components/schemas/V2KeysMigrateKeyData"Rationale: Aligns with existing apiId patterns, prevents unbounded batches, and makes migrationId a slug-like identifier (same semantics used elsewhere).
1431-1436: Inconsistency in migrationId constraints verified; suggested fix requires refinementThe constraint divergence is confirmed across both endpoints:
migrateKeys: minLength: 3, maxLength: 255, requiredverifyKey: maxLength: 256, no minLength, optionalHowever, the suggested pattern constraint
"^[a-zA-Z][a-zA-Z0-9._-]*$"does not exist in the current codebase for either endpoint and cannot be verified. Additionally, prior context suggests verifyKey's migrationId serves a different purpose (internal migration flag vs explicit strategy identifier), which may justify different constraints.Recommendation: Unify constraints by removing the pattern suggestion and instead aligning verifyKey's maxLength to 255 and adding minLength: 3 to match migrateKeys, unless the difference is intentional and should be documented.
migrationId: type: string + minLength: 3 - maxLength: 256 + maxLength: 255 description: Migrate keys on demand from your previous system. Reach out for migration support at support@unkey.dev example: "m_1234abcd"go/apps/api/routes/register.go (1)
424-434: Route wiring looks correct.The migrateKeys handler gets the same default middleware stack plus Logger, ApiCache, DB, Auditlogs, and Keys, so it slots cleanly into the existing v2 key routes and should have everything it needs.
go/pkg/db/querier_generated.go (1)
281-287: New migration queries align with the flow.
FindIdentitiesByExternalId, the key-migration fetch/insert helpers, and theUpdateKeyHashAndMigrationwrite all line up with the migration handler’s needs, and the extrapending_migration_idselects keep the models in sync—looks good.Also applies to: 358-474, 1252-1262, 1951-1960
19 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Superseeds #3841
This adds a new migration endpoint into the v2 api where we batch insert everything.
Also we now support at runtime migration of different key hashing algorithms such as this prefixed key typescript package.
Type of change
How should this be tested?
Unit Tests
Manual Test:
Migrating a key that has a different algo than ours:
Migrating a key that is sha256 encoded already
We don't need the migrationId here! since well hash is already correct.
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated