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 Git ↗︎ 2 Skipped Deployments
|
📝 WalkthroughWalkthroughThis change introduces specific error codes and documentation for handling duplicate role and permission creation in the Unkey system. It updates Go backend logic to use new, more precise error codes for these cases, adds supporting constants and struct fields, and provides detailed documentation for the new errors. The OpenAPI bundle generation is also simplified by removing timestamp headers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant DB
Client->>API: Create Permission/Role (name)
API->>DB: Insert Permission/Role (name)
DB-->>API: Duplicate Key Error
API-->>Client: 409 Conflict with err:unkey:data:permission_already_exists or err:unkey:data:role_already_exists
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsgo/pkg/zen/middleware_errors.go (2)Learnt from: chronark Learnt from: ogzhanolguncu 🧬 Code Graph Analysis (1)go/pkg/zen/middleware_errors.go (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
Graphite Automations"Notify author when CI fails" took an action on this PR • (07/28/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 • (07/28/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
go/pkg/codes/unkey_data.go (1)
106-112: Missing initialization for new Duplicate fields.The
PermissionandRolestructs in theDatavariable are missing initialization for the newDuplicatefields, which means they'll have zero values and won't work as expected.Permission: dataPermission{ + Duplicate: Code{SystemUnkey, CategoryUnkeyData, "permission_already_exists"}, NotFound: Code{SystemUnkey, CategoryUnkeyData, "permission_not_found"}, }, Role: dataRole{ + Duplicate: Code{SystemUnkey, CategoryUnkeyData, "role_already_exists"}, NotFound: Code{SystemUnkey, CategoryUnkeyData, "role_not_found"}, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
apps/docs/errors/unkey/data/permission_already_exists.mdx(1 hunks)apps/docs/errors/unkey/data/role_already_exists.mdx(1 hunks)go/apps/api/openapi/generate_bundle.go(1 hunks)go/apps/api/openapi/openapi-generated.yaml(0 hunks)go/apps/api/routes/v2_permissions_create_permission/handler.go(1 hunks)go/apps/api/routes/v2_permissions_create_role/handler.go(1 hunks)go/pkg/codes/constants_gen.go(1 hunks)go/pkg/codes/unkey_data.go(1 hunks)
💤 Files with no reviewable changes (1)
- go/apps/api/openapi/openapi-generated.yaml
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Flo4604
PR: unkeyed/unkey#3647
File: go/apps/api/openapi/openapi-generated.yaml:3569-3575
Timestamp: 2025-07-22T18:09:41.800Z
Learning: In the Unkey codebase, using non-standard HTTP status code 529 for internal-only endpoints is acceptable and should not be flagged as an issue in future reviews.
Learnt from: Flo4604
PR: unkeyed/unkey#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.
Learnt from: chronark
PR: unkeyed/unkey#3560
File: go/apps/api/routes/v2_keys_create_key/handler.go:468-581
Timestamp: 2025-07-15T14:47:20.490Z
Learning: In the Unkey codebase, role and permission names are validated at the OpenAPI schema layer with strict regex patterns: role names must match "^[a-zA-Z][a-zA-Z0-9_-]*$" (start with letter, followed by letters/numbers/underscores/hyphens) and permission names must match "^[a-zA-Z0-9_]+$" (letters, numbers, underscores only). This validation occurs during zen.BindBody call before handlers run, preventing malicious or improperly formatted names from reaching auto-creation logic.
Learnt from: Flo4604
PR: unkeyed/unkey#2955
File: go/apps/api/routes/v2_identities_create_identity/handler.go:162-202
Timestamp: 2025-03-19T09:25:59.751Z
Learning: In the Unkey codebase, input validation for API endpoints is primarily handled through OpenAPI schema validation, which occurs before requests reach the handler code. For example, in the identities.createIdentity endpoint, minimum values for ratelimit duration and limit are defined in the OpenAPI schema rather than duplicating these checks in the handler.
Learnt from: chronark
PR: unkeyed/unkey#3560
File: go/apps/api/routes/v2_keys_create_key/handler.go:353-466
Timestamp: 2025-07-15T14:25:05.608Z
Learning: In the Unkey codebase, input validation for API endpoints is handled at the OpenAPI schema layer, which validates request fields like permission slugs (pattern: "^[a-zA-Z0-9_]+$", length: 1-100 characters) before requests reach the handler code. This validation occurs during the zen.BindBody call in handlers.
Learnt from: Flo4604
PR: unkeyed/unkey#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.
Learnt from: AkshayBandi027
PR: unkeyed/unkey#2215
File: apps/dashboard/app/(app)/@breadcrumb/authorization/roles/[roleId]/page.tsx:28-29
Timestamp: 2024-10-08T15:33:04.290Z
Learning: In `authorization/roles/[roleId]/update-role.tsx`, the tag `role-${role.id}` is revalidated after updating a role to ensure that the caching mechanism is properly handled for roles.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3324
File: apps/dashboard/app/(app)/authorization/roles/components/table/components/actions/keys-table-action.popover.constants.tsx:17-18
Timestamp: 2025-06-19T11:48:05.070Z
Learning: In the authorization roles refactor, the RoleBasic type uses `roleId` as the property name for the role identifier, not `id`. This is consistent throughout the codebase in apps/dashboard/lib/trpc/routers/authorization/roles/query.ts.
go/apps/api/openapi/generate_bundle.go (1)
Learnt from: Flo4604
PR: #3606
File: go/pkg/prometheus/metrics/database.go:29-30
Timestamp: 2025-07-16T10:06:35.397Z
Learning: In Go packages, variables defined in one file within a package (like latencyBuckets and constLabels in go/pkg/prometheus/metrics/http.go) are accessible from other files in the same package without requiring imports. This is a common pattern for sharing configuration across multiple files within a package.
go/apps/api/routes/v2_permissions_create_permission/handler.go (1)
Learnt from: ogzhanolguncu
PR: #3661
File: go/apps/api/routes/v2_identities_update_identity/handler.go:115-119
Timestamp: 2025-07-28T11:47:43.144Z
Learning: The v2 update identity endpoint (go/apps/api/routes/v2_identities_update_identity/handler.go) intentionally uses ExternalId field instead of the unified Identity field used in other v2 identity endpoints. This is because the update endpoint needs to both find by externalId and potentially update the externalId value, making the specific field name more appropriate than the generic Identity field.
go/apps/api/routes/v2_permissions_create_role/handler.go (4)
Learnt from: ogzhanolguncu
PR: #3661
File: go/apps/api/routes/v2_identities_update_identity/handler.go:115-119
Timestamp: 2025-07-28T11:47:43.144Z
Learning: The v2 update identity endpoint (go/apps/api/routes/v2_identities_update_identity/handler.go) intentionally uses ExternalId field instead of the unified Identity field used in other v2 identity endpoints. This is because the update endpoint needs to both find by externalId and potentially update the externalId value, making the specific field name more appropriate than the generic Identity field.
Learnt from: ogzhanolguncu
PR: #3324
File: apps/dashboard/app/(app)/authorization/roles/components/table/components/actions/keys-table-action.popover.constants.tsx:17-18
Timestamp: 2025-06-19T11:48:05.070Z
Learning: In the authorization roles refactor, the RoleBasic type uses roleId as the property name for the role identifier, not id. This is consistent throughout the codebase in apps/dashboard/lib/trpc/routers/authorization/roles/query.ts.
Learnt from: chronark
PR: #3560
File: go/apps/api/routes/v2_keys_create_key/handler.go:468-581
Timestamp: 2025-07-15T14:47:20.490Z
Learning: In the Unkey codebase, role and permission names are validated at the OpenAPI schema layer with strict regex patterns: role names must match "^[a-zA-Z][a-zA-Z0-9_-]*$" (start with letter, followed by letters/numbers/underscores/hyphens) and permission names must match "^[a-zA-Z0-9_]+$" (letters, numbers, underscores only). This validation occurs during zen.BindBody call before handlers run, preventing malicious or improperly formatted names from reaching auto-creation logic.
Learnt from: AkshayBandi027
PR: #2215
File: apps/dashboard/app/(app)/@breadcrumb/authorization/roles/[roleId]/page.tsx:28-29
Timestamp: 2024-10-08T15:33:04.290Z
Learning: In authorization/roles/[roleId]/update-role.tsx, the tag role-${role.id} is revalidated after updating a role to ensure that the caching mechanism is properly handled for roles.
go/pkg/codes/constants_gen.go (1)
Learnt from: chronark
PR: #3560
File: go/apps/api/routes/v2_keys_create_key/handler.go:468-581
Timestamp: 2025-07-15T14:47:20.490Z
Learning: In the Unkey codebase, role and permission names are validated at the OpenAPI schema layer with strict regex patterns: role names must match "^[a-zA-Z][a-zA-Z0-9_-]*$" (start with letter, followed by letters/numbers/underscores/hyphens) and permission names must match "^[a-zA-Z0-9_]+$" (letters, numbers, underscores only). This validation occurs during zen.BindBody call before handlers run, preventing malicious or improperly formatted names from reaching auto-creation logic.
apps/docs/errors/unkey/data/permission_already_exists.mdx (3)
Learnt from: Flo4604
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.
Learnt from: MichaelUnkey
PR: #3173
File: apps/docs/security/delete-protection.mdx:21-24
Timestamp: 2025-04-22T17:34:04.438Z
Learning: In the Unkey dashboard UI for enabling delete protection, the button/link to initiate the process is labeled "Enable Delete Protection" while the confirmation button is labeled "Enable API Delete Protection". The documentation should maintain these different labels to match the actual UI.
Learnt from: MichaelUnkey
PR: #3173
File: apps/docs/security/delete-protection.mdx:32-36
Timestamp: 2025-04-22T17:33:28.162Z
Learning: In the Unkey dashboard UI for delete protection, the button/link to initiate the process is labeled "Disable Delete Protection" while the confirmation button is labeled "Disable API Delete Protection". The documentation should maintain these different labels to match the actual UI.
apps/docs/errors/unkey/data/role_already_exists.mdx (2)
Learnt from: AkshayBandi027
PR: #2215
File: apps/dashboard/app/(app)/@breadcrumb/authorization/roles/[roleId]/page.tsx:28-29
Timestamp: 2024-10-08T15:33:04.290Z
Learning: In authorization/roles/[roleId]/update-role.tsx, the tag role-${role.id} is revalidated after updating a role to ensure that the caching mechanism is properly handled for roles.
Learnt from: chronark
PR: #3560
File: go/apps/api/routes/v2_keys_create_key/handler.go:468-581
Timestamp: 2025-07-15T14:47:20.490Z
Learning: In the Unkey codebase, role and permission names are validated at the OpenAPI schema layer with strict regex patterns: role names must match "^[a-zA-Z][a-zA-Z0-9_-]*$" (start with letter, followed by letters/numbers/underscores/hyphens) and permission names must match "^[a-zA-Z0-9_]+$" (letters, numbers, underscores only). This validation occurs during zen.BindBody call before handlers run, preventing malicious or improperly formatted names from reaching auto-creation logic.
🧬 Code Graph Analysis (1)
go/pkg/codes/constants_gen.go (1)
go/pkg/urn/urn.go (1)
URN(12-19)
🪛 Gitleaks (8.27.2)
apps/docs/errors/unkey/data/permission_already_exists.mdx
35-37: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
57-59: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
apps/docs/errors/unkey/data/role_already_exists.mdx
35-37: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
57-59: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
⏰ 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). (7)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Packages / Test
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
go/apps/api/openapi/generate_bundle.go (1)
76-81: Good call removing the timestamp – improves deterministic builds.Dropping the
timeimport and the “Generated at” line keeps the bundle content stable across runs, which prevents unnecessary diff noise in CI and release artifacts. The current header string is concise and still conveys provenance.
No further concerns here.go/apps/api/routes/v2_permissions_create_permission/handler.go (1)
84-84: Improved error specificity with dedicated permission duplicate code.The change from a generic duplicate identity error to
codes.Data.Permission.Duplicate.URN()provides better error categorization and aligns with the PR objective of fixing wrong error codes in v2.go/apps/api/routes/v2_permissions_create_role/handler.go (1)
91-91: Consistent error specificity improvement for role duplicates.The change to
codes.Data.Role.Duplicate.URN()maintains consistency with the permission handler changes and provides better error categorization for duplicate role scenarios.go/pkg/codes/unkey_data.go (1)
34-35: Improve comment specificity.The comment "requested identity already exists" is generic and doesn't clearly indicate this is for role duplicates. Consider updating to be more specific.
- // Duplicate indicates the requested identity already exists. + // Duplicate indicates the requested role already exists.⛔ Skipped due to learnings
Learnt from: ogzhanolguncu PR: unkeyed/unkey#3661 File: go/apps/api/routes/v2_identities_update_identity/handler.go:115-119 Timestamp: 2025-07-28T11:47:43.144Z Learning: The v2 update identity endpoint (go/apps/api/routes/v2_identities_update_identity/handler.go) intentionally uses `ExternalId` field instead of the unified `Identity` field used in other v2 identity endpoints. This is because the update endpoint needs to both find by externalId and potentially update the externalId value, making the specific field name more appropriate than the generic `Identity` field.Learnt from: ogzhanolguncu PR: unkeyed/unkey#3324 File: apps/dashboard/app/(app)/authorization/roles/components/table/components/actions/keys-table-action.popover.constants.tsx:17-18 Timestamp: 2025-06-19T11:48:05.070Z Learning: In the authorization roles refactor, the RoleBasic type uses `roleId` as the property name for the role identifier, not `id`. This is consistent throughout the codebase in apps/dashboard/lib/trpc/routers/authorization/roles/query.ts.apps/docs/errors/unkey/data/permission_already_exists.mdx (1)
1-97: Comprehensive and well-structured error documentation.This documentation effectively covers all aspects of the permission duplicate error including:
- Clear error example with proper JSON structure
- Detailed explanation of common scenarios
- Practical solutions and code examples
- Common mistakes to avoid
- Related error references
The curl examples use proper placeholder formatting with "YOUR_API_KEY" which is standard practice for API documentation.
|
yeah not sure why, will fix in a sec dealing with merge conflicts in the other pr atm |
|
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
2 participants
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Documentation
Bug Fixes
Chores