refactor: update identity param for v2#3661
Conversation
ogzhanolguncu
requested review from
Flo4604,
chronark,
imeyer and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
📝 Walkthrough## Walkthrough
This change updates the API, handler logic, database queries, and tests to unify identity lookup and deletion by replacing separate external and internal identity identifiers with a single generic `identity` field. The database queries are refactored to accept workspace-scoped parameters and identify records by either internal ID or external ID. OpenAPI schemas and request bodies are updated accordingly.
## Changes
| File(s) | Change Summary |
|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------|
| **OpenAPI schema and request bodies**<br>`go/apps/api/openapi/gen.go`<br>`go/apps/api/openapi/openapi-generated.yaml`<br>`go/apps/api/openapi/spec/paths/v2/identities/deleteIdentity/V2IdentitiesDeleteIdentityRequestBody.yaml`<br>`go/apps/api/openapi/spec/paths/v2/identities/getIdentity/V2IdentitiesGetIdentityRequestBody.yaml` | Replace `externalId` with `identity` in request bodies and OpenAPI schemas; update descriptions, constraints, and required fields. |
| **Handlers refactoring and query updates**<br>`go/apps/api/routes/v2_identities_delete_identity/handler.go`<br>`go/apps/api/routes/v2_identities_get_identity/handler.go`<br>`go/apps/api/routes/v2_identities_update_identity/handler.go`<br>`go/apps/api/routes/v2_keys_create_key/handler.go`<br>`go/apps/api/routes/v2_keys_get_key/handler.go`<br>`go/apps/api/routes/v2_keys_update_key/handler.go`<br>`go/apps/api/routes/v2_apis_list_keys/handler.go`<br>`go/apps/api/routes/v2_keys_whoami/handler.go` | Refactor handlers to use unified `FindIdentity` query with workspace-scoped parameters; update soft delete logic; streamline deletion helper; adjust type alias formatting. |
| **Test code updates**<br>`go/apps/api/routes/v2_identities_create_identity/200_test.go`<br>`go/apps/api/routes/v2_identities_delete_identity/200_test.go`<br>`go/apps/api/routes/v2_identities_delete_identity/400_test.go`<br>`go/apps/api/routes/v2_identities_delete_identity/401_test.go`<br>`go/apps/api/routes/v2_identities_delete_identity/403_test.go`<br>`go/apps/api/routes/v2_identities_delete_identity/404_test.go`<br>`go/apps/api/routes/v2_identities_get_identity/200_test.go`<br>`go/apps/api/routes/v2_identities_get_identity/400_test.go`<br>`go/apps/api/routes/v2_identities_get_identity/401_test.go`<br>`go/apps/api/routes/v2_identities_get_identity/403_test.go`<br>`go/apps/api/routes/v2_identities_get_identity/404_test.go`<br>`go/apps/api/routes/v2_identities_list_identities/200_test.go`<br>`go/apps/api/routes/v2_keys_update_key/200_test.go`<br>`go/apps/api/routes/v2_keys_update_key/three_state_test.go` | Update tests to use `identity` field; replace calls to separate identity queries with unified `FindIdentity`; add test for retrieval by internal ID; update soft delete calls. |
| **New and updated SQL queries and generated code**<br>`go/pkg/db/queries/identity_find.sql`<br>`go/pkg/db/identity_find.sql_generated.go` | Add new SQL query and generated code for `FindIdentity` supporting workspace-scoped lookup by internal or external ID. |
| **Updated delete queries and generated code**<br>`go/pkg/db/queries/identity_delete.sql`<br>`go/pkg/db/identity_delete.sql_generated.go` | Update delete query and generated code to accept workspace and identity parameters; delete by either internal or external ID. |
| **Updated soft delete queries and generated code**<br>`go/pkg/db/queries/identity_soft_delete.sql`<br>`go/pkg/db/identity_soft_delete.sql_generated.go` | Update soft delete query and generated code to accept workspace and identity parameters; soft delete by either internal or external ID. |
| **Database interface updates**<br>`go/pkg/db/querier_generated.go` | Update interface: remove separate internal/external ID methods; add unified `FindIdentity`; update delete and soft delete signatures; add new method for deleting old identities with ratelimits. |
| **Removed obsolete queries and generated code**<br>`go/pkg/db/queries/identity_find_by_external_id.sql`<br>`go/pkg/db/identity_find_by_external_id.sql_generated.go`<br>`go/pkg/db/queries/identity_find_by_id.sql`<br>`go/pkg/db/identity_find_by_id.sql_generated.go` | Remove obsolete separate queries and generated code for internal and external ID lookups. |
| **Code style and comment formatting**<br>`go/pkg/codes/constants_gen.go` | Remove indentation from comment blocks for consistency; no functional changes. |
| **Database schema update**<br>`go/pkg/db/schema.sql` | Add composite index on `identities(workspace_id, id, deleted)` to optimize new queries. |
| **New SQL query and generated code for deleting old identities with ratelimits**<br>`go/pkg/db/identity_delete_old_with_ratelimits.sql_generated.go`<br>`go/pkg/db/queries/identity_delete_old_with_ratelimits.sql` | Add new query and generated code to delete old soft-deleted identities and their associated ratelimits by workspace and identity. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Client
participant API_Handler
participant DB
Client->>API_Handler: Request (identity: externalId or identityId)
API_Handler->>DB: FindIdentity(workspaceId, identity, deleted=false)
DB-->>API_Handler: Identity record or error
alt Found
API_Handler-->>Client: Success response
else Not found
API_Handler-->>Client: Not found error
endsequenceDiagram
participant Client
participant API_Handler
participant DB
Client->>API_Handler: Delete Identity (identity: externalId or identityId)
API_Handler->>DB: SoftDeleteIdentity(workspaceId, identity)
DB-->>API_Handler: Success or error
alt Success
API_Handler-->>Client: 200 OK
else Error
API_Handler-->>Client: Error response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested labels
Suggested reviewers
Learnt from: ogzhanolguncu Learnt from: ogzhanolguncu Learnt from: chronark Learnt from: Flo4604 Learnt from: ogzhanolguncu Learnt from: ogzhanolguncu Learnt from: ogzhanolguncu Learnt from: ogzhanolguncu Learnt from: ogzhanolguncu Learnt from: ogzhanolguncu |
|
Thank you for following the naming conventions for pull request titles! 🙏 |
|
bro what are these large PRs??? : |
|
Nice try |
|
😄 |
|
🤣 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (37)
go/apps/api/openapi/gen.go(2 hunks)go/apps/api/openapi/openapi-generated.yaml(3 hunks)go/apps/api/openapi/spec/paths/v2/identities/deleteIdentity/V2IdentitiesDeleteIdentityRequestBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/identities/getIdentity/V2IdentitiesGetIdentityRequestBody.yaml(1 hunks)go/apps/api/routes/v2_apis_list_keys/handler.go(2 hunks)go/apps/api/routes/v2_identities_create_identity/200_test.go(10 hunks)go/apps/api/routes/v2_identities_delete_identity/200_test.go(6 hunks)go/apps/api/routes/v2_identities_delete_identity/400_test.go(4 hunks)go/apps/api/routes/v2_identities_delete_identity/401_test.go(8 hunks)go/apps/api/routes/v2_identities_delete_identity/403_test.go(7 hunks)go/apps/api/routes/v2_identities_delete_identity/404_test.go(4 hunks)go/apps/api/routes/v2_identities_delete_identity/handler.go(4 hunks)go/apps/api/routes/v2_identities_get_identity/200_test.go(7 hunks)go/apps/api/routes/v2_identities_get_identity/400_test.go(4 hunks)go/apps/api/routes/v2_identities_get_identity/401_test.go(1 hunks)go/apps/api/routes/v2_identities_get_identity/403_test.go(2 hunks)go/apps/api/routes/v2_identities_get_identity/404_test.go(2 hunks)go/apps/api/routes/v2_identities_get_identity/handler.go(3 hunks)go/apps/api/routes/v2_identities_list_identities/200_test.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/handler.go(2 hunks)go/apps/api/routes/v2_keys_create_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_get_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_update_key/200_test.go(2 hunks)go/apps/api/routes/v2_keys_update_key/handler.go(3 hunks)go/apps/api/routes/v2_keys_update_key/three_state_test.go(3 hunks)go/pkg/codes/constants_gen.go(3 hunks)go/pkg/db/identity_delete.sql_generated.go(1 hunks)go/pkg/db/identity_find.sql_generated.go(1 hunks)go/pkg/db/identity_find_by_external_id.sql_generated.go(0 hunks)go/pkg/db/identity_find_by_id.sql_generated.go(0 hunks)go/pkg/db/identity_soft_delete.sql_generated.go(1 hunks)go/pkg/db/querier_generated.go(3 hunks)go/pkg/db/queries/identity_delete.sql(1 hunks)go/pkg/db/queries/identity_find.sql(1 hunks)go/pkg/db/queries/identity_find_by_external_id.sql(0 hunks)go/pkg/db/queries/identity_find_by_id.sql(0 hunks)go/pkg/db/queries/identity_soft_delete.sql(1 hunks)
💤 Files with no reviewable changes (4)
- go/pkg/db/queries/identity_find_by_id.sql
- go/pkg/db/queries/identity_find_by_external_id.sql
- go/pkg/db/identity_find_by_external_id.sql_generated.go
- go/pkg/db/identity_find_by_id.sql_generated.go
🧰 Additional context used
🧬 Code Graph Analysis (19)
go/apps/api/routes/v2_identities_get_identity/401_test.go (1)
go/apps/api/openapi/gen.go (1)
Identity(142-151)
go/apps/api/routes/v2_identities_delete_identity/400_test.go (3)
go/apps/api/routes/v2_identities_delete_identity/handler.go (1)
Request(22-22)go/apps/api/openapi/gen.go (1)
Identity(142-151)go/pkg/db/models_generated.go (1)
Identity(655-664)
go/pkg/codes/constants_gen.go (1)
go/pkg/urn/urn.go (1)
URN(12-19)
go/apps/api/routes/v2_identities_delete_identity/404_test.go (5)
go/apps/api/routes/v2_identities_delete_identity/handler.go (1)
Request(22-22)go/apps/api/openapi/gen.go (1)
Identity(142-151)go/pkg/db/models_generated.go (1)
Identity(655-664)go/pkg/db/identity_soft_delete.sql_generated.go (1)
SoftDeleteIdentityParams(19-22)go/pkg/testutil/seed/seed.go (1)
Resources(20-25)
go/apps/api/routes/v2_identities_list_identities/200_test.go (1)
go/pkg/db/identity_soft_delete.sql_generated.go (1)
SoftDeleteIdentityParams(19-22)
go/apps/api/routes/v2_identities_get_identity/handler.go (3)
go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(20-24)go/apps/api/openapi/gen.go (1)
Identity(142-151)go/pkg/db/models_generated.go (1)
Identity(655-664)
go/apps/api/routes/v2_identities_delete_identity/401_test.go (3)
go/apps/api/routes/v2_identities_delete_identity/handler.go (1)
Request(22-22)go/apps/api/openapi/gen.go (1)
Identity(142-151)go/pkg/db/models_generated.go (1)
Identity(655-664)
go/apps/api/routes/v2_identities_get_identity/403_test.go (1)
go/apps/api/openapi/gen.go (1)
Identity(142-151)
go/apps/api/routes/v2_identities_get_identity/400_test.go (4)
go/apps/api/openapi/gen.go (2)
Meta(333-336)Identity(142-151)go/apps/api/routes/v2_identities_get_identity/handler.go (1)
Request(19-19)go/pkg/db/models_generated.go (1)
Identity(655-664)go/pkg/uid/uid.go (1)
TestPrefix(24-24)
go/apps/api/routes/v2_keys_create_key/handler.go (3)
go/apps/api/openapi/gen.go (3)
V2KeysCreateKeyRequestBody(958-1032)V2KeysCreateKeyResponseBody(1035-1040)Identity(142-151)go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(20-24)go/pkg/db/models_generated.go (1)
Identity(655-664)
go/apps/api/routes/v2_apis_list_keys/handler.go (3)
go/apps/api/openapi/gen.go (2)
V2ApisListKeysRequestBody(622-654)Identity(142-151)go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(20-24)go/pkg/db/models_generated.go (1)
Identity(655-664)
go/apps/api/routes/v2_keys_update_key/handler.go (2)
go/apps/api/openapi/gen.go (3)
V2KeysUpdateKeyRequestBody(1444-1491)V2KeysUpdateKeyResponseBody(1494-1500)Identity(142-151)go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(20-24)
go/apps/api/routes/v2_identities_update_identity/handler.go (3)
go/apps/api/openapi/gen.go (3)
V2IdentitiesUpdateIdentityRequestBody(779-796)V2IdentitiesUpdateIdentityResponseBody(799-804)Identity(142-151)go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(20-24)go/pkg/db/models_generated.go (1)
Identity(655-664)
go/apps/api/routes/v2_keys_get_key/handler.go (3)
go/apps/api/openapi/gen.go (3)
V2KeysGetKeyRequestBody(1080-1101)V2KeysGetKeyResponseBody(1110-1115)Identity(142-151)go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(20-24)go/pkg/db/models_generated.go (1)
Identity(655-664)
go/apps/api/routes/v2_identities_create_identity/200_test.go (3)
go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(20-24)go/pkg/db/models_generated.go (1)
Identity(655-664)go/pkg/testutil/seed/seed.go (1)
Resources(20-25)
go/apps/api/routes/v2_identities_delete_identity/handler.go (8)
go/apps/api/openapi/gen.go (3)
V2IdentitiesDeleteIdentityRequestBody(717-720)V2IdentitiesDeleteIdentityResponseBody(723-726)Identity(142-151)go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(20-24)go/pkg/db/models_generated.go (1)
Identity(655-664)go/pkg/db/identity_soft_delete.sql_generated.go (1)
SoftDeleteIdentityParams(19-22)go/pkg/db/handle_err_duplicate_key.go (1)
IsDuplicateKeyError(7-13)go/pkg/db/handle_err_no_rows.go (1)
IsNotFound(8-10)go/pkg/fault/wrap.go (3)
Wrap(25-67)Internal(75-89)Public(97-111)go/pkg/db/identity_delete.sql_generated.go (1)
DeleteIdentityParams(18-21)
go/apps/api/openapi/gen.go (1)
go/pkg/db/models_generated.go (1)
Identity(655-664)
go/pkg/db/identity_find.sql_generated.go (1)
go/pkg/hydra/store/db.go (1)
DBTX(8-13)
go/pkg/db/querier_generated.go (5)
go/pkg/hydra/store/db.go (1)
DBTX(8-13)go/pkg/db/identity_delete.sql_generated.go (1)
DeleteIdentityParams(18-21)go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(20-24)go/pkg/db/models_generated.go (1)
Identity(655-664)go/pkg/db/identity_soft_delete.sql_generated.go (1)
SoftDeleteIdentityParams(19-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
🔇 Additional comments (59)
go/pkg/codes/constants_gen.go (1)
9-126: Comment formatting changes look good.The alignment of comment blocks to the left margin improves readability. Since this is a generated file (as indicated by the header comment), these formatting changes likely result from updates to the code generator, which is appropriate.
go/apps/api/routes/v2_identities_get_identity/401_test.go (1)
24-26: Field rename aligns with the refactor objectives.The change from
ExternalIdtoIdentityis consistent with the PR's goal to unify identity lookup using a single field that can accept either external ID or internal ID.go/apps/api/routes/v2_identities_list_identities/200_test.go (1)
200-203: Parameter struct usage properly implements workspace scoping.The change to use
db.SoftDeleteIdentityParamswith bothIdentityandWorkspaceIDfields correctly implements workspace scoping for the soft delete operation, which aligns with the refactor's goal to unify identity operations.go/apps/api/routes/v2_identities_get_identity/404_test.go (3)
37-37: Field rename is consistent with the refactor.The change from
ExternalIdtoIdentityaligns with the unified identity lookup approach.
70-73: Proper workspace scoping implementation.The use of
db.SoftDeleteIdentityParamswith bothIdentityandWorkspaceIDcorrectly implements workspace scoping for the soft delete operation.
81-81: Consistent field usage in test scenario.The
Identityfield usage is consistent with the refactor's unified approach for identity lookup.go/apps/api/routes/v2_identities_get_identity/400_test.go (2)
30-30: Good update to test description.The test name change from "missing externalID" to "missing identity" correctly reflects the unified identity parameter that can accept both external IDs and internal identity IDs.
62-62: Field name changes are correct.The changes from
ExternalIdtoIdentityare consistent with the refactor to unify identity parameters. The use ofuid.New(uid.TestPrefix)generates appropriate test data for the unified identity field.Also applies to: 76-76
go/pkg/db/queries/identity_delete.sql (1)
2-4: Excellent addition of workspace scoping and flexible identity matching.The updated query improves both security and flexibility:
- Security: The
workspace_idconstraint prevents accidental cross-workspace deletions- Flexibility: The OR condition
(id = sqlc.arg(identity) OR external_id = sqlc.arg(identity))allows deletion by either internal ID or external ID using a single parameter- Consistency: Aligns with the broader refactor to unify identity parameters
The use of
sqlc.arg()parameters prevents SQL injection, and the workspace scoping ensures proper data isolation.go/apps/api/routes/v2_identities_get_identity/handler.go (2)
63-67: Excellent refactor to unified identity lookup.The change from
FindIdentityByExternalIDtoFindIdentitywith the new parameter structure improves the API's flexibility:
- Unified lookup: Single method can find identities by either internal ID or external ID
- Workspace scoping: Maintains security by scoping to
auth.AuthorizedWorkspaceID- Active records:
Deleted: falseensures only active identities are returned- Consistent naming:
req.Identityaligns with the unified parameter approachThe error handling logic remains unchanged, ensuring backward compatibility in error responses.
119-119: Good modernization to useanytype alias.The change from
map[string]interface{}tomap[string]anyfollows modern Go idioms. Theanyalias (introduced in Go 1.18) is more readable and is the preferred way to express the empty interface type in contemporary Go code.Also applies to: 128-128
go/apps/api/routes/v2_identities_get_identity/403_test.go (1)
75-75: Field rename is consistent with the refactor.The change from
ExternalIdtoIdentitycorrectly reflects the unified identity parameter that can accept both external IDs and internal identity IDs. The test logic remains valid for checking permission enforcement.go/apps/api/routes/v2_identities_delete_identity/403_test.go (1)
34-34: LGTM! Field name change aligns with the unified identity parameter refactor.The consistent replacement of
ExternalIdwithIdentityin all test request structs correctly reflects the API refactoring to use a single field that accepts either internal or external identity IDs.Also applies to: 53-53, 72-72, 91-91, 110-110, 132-134, 153-153
go/apps/api/routes/v2_identities_delete_identity/401_test.go (1)
33-33: LGTM! Consistent field renaming across all test cases.The replacement of
ExternalIdwithIdentityin request structs is consistent with the unified identity parameter refactor and maintains test coverage for all authorization scenarios.Also applies to: 51-53, 71-71, 89-91, 109-109, 127-127, 150-150, 168-168
go/apps/api/routes/v2_identities_get_identity/200_test.go (2)
76-76: LGTM! Field name changes align with the unified identity parameter.The consistent replacement of
ExternalIdwithIdentityin request structs correctly reflects the API refactoring.Also applies to: 119-119, 154-154, 241-241, 321-321, 369-369
378-392: Excellent addition! New test validates querying by internal identity ID.This new test case effectively demonstrates and validates the core functionality of the refactor - the ability to query identities using the internal identity ID in addition to the external ID. The test follows good practices by creating an identity and then retrieving it using the returned internal ID.
go/apps/api/routes/v2_identities_delete_identity/404_test.go (2)
37-37: LGTM! Consistent field renaming across test cases.The replacement of
ExternalIdwithIdentityin request structs aligns with the unified identity parameter refactor.Also applies to: 73-73, 107-107, 121-121
100-103: LGTM! Database method updated to use workspace-scoped parameters.The change from a single identity ID parameter to the
SoftDeleteIdentityParamsstruct with bothIdentityandWorkspaceIDfields improves security by ensuring workspace scoping and aligns with the database layer refactoring.go/apps/api/routes/v2_identities_update_identity/handler.go (3)
24-27: LGTM! Clean type declaration grouping.The grouping of type declarations is a good formatting improvement that enhances readability.
115-119: LGTM! Database query updated to use unified identity lookup method.The change from
FindIdentityByExternalIDtoFindIdentitywith the new parameter struct correctly implements the unified identity lookup approach with workspace scoping. The new method provides better security by ensuring workspace isolation.
115-119: No field naming inconsistency to address
Both the create and update identity request bodies inopenapi/gen.goconsistently useexternalIdrather thanidentity. No changes are required to the update endpoint’s field name.Likely an incorrect or invalid review comment.
go/apps/api/routes/v2_apis_list_keys/handler.go (2)
23-26: LGTM! Clean type alias definitions.The type alias definitions improve code readability and maintainability.
179-181: Excellent refactor to unified identity lookup.The change from
FindIdentityByExternalIDtoFindIdentitywith structured parameters improves flexibility by allowing queries by either internal ID or external ID while maintaining proper workspace scoping. The logic correctly handles the case where identity is not found by returning an empty result.go/apps/api/openapi/spec/paths/v2/identities/getIdentity/V2IdentitiesGetIdentityRequestBody.yaml (1)
3-10: Excellent API design improvement for flexible identity lookup.The changes enhance the API's usability:
- Renaming
externalIdtoidentitymakes the field more intuitive and flexible- The updated description clearly communicates that both external IDs and internal identity IDs are supported
- The increased
minLengthconstraint (3 characters) is reasonable for preventing very short identifiers- This aligns well with the broader refactor to unify identity handling across the codebase
go/apps/api/routes/v2_keys_update_key/three_state_test.go (2)
271-273: LGTM! Updated to use unified identity lookup with workspace scoping.The test correctly updates to use the new
FindIdentitymethod with structured parameters that include workspace scoping. This improves security by ensuring identity lookups are properly scoped to the workspace context.Also applies to: 318-320
347-352: Good use of modern Go type alias.The change from
map[string]interface{}tomap[string]anyuses the more concise type alias introduced in Go 1.18+, improving code readability.go/apps/api/openapi/spec/paths/v2/identities/deleteIdentity/V2IdentitiesDeleteIdentityRequestBody.yaml (1)
3-10: Excellent consistency with the getIdentity API design.The changes mirror the improvements made to the getIdentity endpoint:
- The
identityfield name is more intuitive thanexternalId- The description clearly explains support for both external IDs and internal identity IDs
- The
minLength: 3constraint prevents very short identifiers- This maintains API consistency across identity-related endpoints
go/pkg/db/queries/identity_soft_delete.sql (1)
2-5: Excellent SQL design for unified identity handling with workspace scoping.The updated query improves both security and flexibility:
- Workspace scoping: The
workspace_idfilter ensures operations are properly isolated by workspace- Unified identity matching: Using the same parameter to match either
idORexternal_idelegantly supports the flexible identity lookup- Clean SQL pattern: The OR condition is efficient and maintains query performance
This aligns perfectly with the broader refactor to unify identity handling across the codebase.
go/apps/api/routes/v2_keys_update_key/200_test.go (2)
109-109: LGTM: Modern Go type alias usageGood use of the
anytype alias instead ofinterface{}for improved readability.
137-139: LGTM: Correctly updated to use unified identity lookupThe change from
FindIdentityByIDtoFindIdentityproperly aligns with the refactor to unify identity lookups. The test correctly uses the internal identity ID and workspace scoping.go/pkg/db/identity_find.sql_generated.go (3)
12-18: LGTM: Well-designed unified identity queryThe SQL query correctly implements the unified identity lookup with proper workspace scoping and the ability to match by either external ID or internal ID using a single parameter.
20-24: LGTM: Clean parameter structureThe
FindIdentityParamsstruct appropriately captures all necessary parameters for the unified identity lookup with proper database tags.
33-52: LGTM: Correct method implementationThe generated
FindIdentitymethod properly implements the unified lookup logic. The double usage ofarg.Identity(lines 36-37) is correct since the SQL query needs the same value for both external_id and id comparisons in the OR condition.go/apps/api/openapi/gen.go (2)
718-719: LGTM! Field rename improves API clarity.The rename from
ExternalIdtoIdentitywith the updated comment clearly communicates that this field now accepts both external IDs and internal identity IDs, supporting the unified identity lookup approach. The JSON tag remains consistent.
730-731: LGTM! Consistent field rename across identity operations.The
Identityfield rename matches the pattern used in the delete request body, maintaining consistency across identity operations. The updated comment correctly explains that both external IDs and internal identity IDs are accepted.go/apps/api/routes/v2_keys_create_key/handler.go (2)
29-32: Clean formatting improvement.Grouping the type aliases into a single type block improves code organization and readability.
208-212: Good refactor to unified identity lookup with workspace scoping.The change from
FindIdentityByExternalIDtoFindIdentityis part of a beneficial refactor that:
- Adds workspace scoping for improved security
- Unifies identity lookup to handle both internal and external IDs
- Standardizes parameter structures across the codebase
go/apps/api/routes/v2_keys_get_key/handler.go (2)
25-28: Clean formatting improvement.Consistent with other files in this PR, grouping type aliases improves code organization.
256-260: Consistent refactor to unified identity lookup.The update from
FindIdentityByIDtoFindIdentitymaintains the same functionality while adding workspace scoping and preparing for the unified identity parameter approach.go/pkg/db/identity_delete.sql_generated.go (1)
12-31: Well-designed database layer update for unified identity deletion.The changes improve both security and flexibility:
- Security: Workspace scoping prevents unauthorized cross-workspace deletions
- Flexibility: Single method handles deletion by either internal ID or external ID
- Type safety: Structured parameters replace positional string arguments
The implementation correctly passes the
Identityparameter twice to match bothidandexternal_idfields in the SQL WHERE clause.go/apps/api/routes/v2_keys_update_key/handler.go (3)
25-28: Consistent formatting improvement.Type alias grouping maintains consistency across the codebase.
158-162: Consistent refactor to unified identity lookup.The update adds workspace scoping and standardizes the parameter structure while maintaining the same functionality for finding existing identities during key updates.
193-197: Proper refactor of fallback identity lookup.The error handling path correctly uses the same unified
FindIdentitymethod, maintaining consistent behavior when dealing with duplicate identity creation scenarios.go/pkg/db/identity_soft_delete.sql_generated.go (1)
12-33: Consistent database layer improvement for soft deletion.The soft delete operation benefits from the same improvements as hard deletion:
- Security: Workspace scoping prevents unauthorized access
- Flexibility: Unified identity parameter handles both ID types
- Consistency: Aligns with the broader refactoring pattern
The SQL logic correctly preserves the soft delete behavior (setting
deleted = 1) while adding enhanced filtering capabilities.go/apps/api/routes/v2_identities_create_identity/200_test.go (3)
52-56: LGTM! Unified identity lookup implementation is correct.The migration from
FindIdentityByIDtoFindIdentityproperly demonstrates the new unified approach. The test correctly uses the internalidentityIDin theIdentityfield with proper workspace scoping.
111-115: LGTM! External ID lookup works correctly with unified method.The test properly demonstrates that the unified
FindIdentitymethod can handle external IDs in theIdentityfield. The workspace scoping and deletion filtering are correctly applied.
133-137: Excellent test coverage for the unified identity lookup.All test cases consistently use the new
FindIdentitymethod with theFindIdentityParamsstruct. The tests properly validate that external IDs work seamlessly with the unified approach, and workspace scoping is correctly applied throughout.Also applies to: 174-178, 233-237, 309-313, 350-354, 380-384, 420-424
go/apps/api/routes/v2_identities_delete_identity/handler.go (4)
21-24: LGTM! Type aliases updated correctly for unified identity field.The request and response types now properly reference the updated OpenAPI definitions that use the
Identityfield instead ofExternalId, supporting both internal and external ID lookups.
73-77: LGTM! Identity lookup correctly implemented with workspace scoping.The unified
FindIdentitymethod properly uses thereq.Identityfield with appropriate workspace scoping and deletion filtering. This allows the endpoint to accept either internal or external IDs seamlessly.
100-103: LGTM! Soft delete operations properly use unified parameter struct.The
SoftDeleteIdentitycalls correctly use the new parameter struct with workspace scoping and the identity field from the request. The retry logic maintains the same parameter usage for consistency.Also applies to: 114-117
206-229: LGTM! Hard delete function updated correctly with improved error handling.The
deleteOldIdentityfunction properly uses the newDeleteIdentitymethod with the parameter struct including workspace scoping. The error handling correctly ignoresNotFounderrors for both rate limit and identity deletion, which is appropriate for cleanup operations.go/apps/api/openapi/openapi-generated.yaml (2)
1-5: Avoid direct edits to generated bundleThis file is regenerated by
generate_bundle.go; any manual tweaks will disappear on the next run. Please ensure the underlying OpenAPI YAML fragments are updated instead.
416-424: Re-evaluateminLength: 3constraintSome integrations may legitimately use identifiers shorter than three characters (e.g.
id1). Either lower the limit (e.g.minLength: 1) or document clearly why a minimum of three is required.go/apps/api/routes/v2_identities_delete_identity/200_test.go (2)
85-89: LGTM: Unified identity lookup implementation looks correct.The refactoring from
FindIdentityByExternalIDtoFindIdentityis implemented correctly with proper workspace scoping and the genericIdentityparameter.
94-94: LGTM: Request field rename aligns with API unification.The field rename from
ExternalIdtoIdentityis consistent with the broader refactoring to support both internal and external identity references.go/pkg/db/querier_generated.go (4)
23-28: LGTM: Well-designed unified deletion interface.The
DeleteIdentitymethod now properly enforces workspace scoping and supports both internal and external identity references through a single parameter. The SQL query correctly uses OR logic to match either ID type.
155-162: LGTM: Excellent consolidation of identity lookup methods.The unified
FindIdentitymethod successfully replaces the separateFindIdentityByExternalIDandFindIdentityByIDmethods while adding proper workspace scoping. The SQL query design using(external_id = ? OR id = ?)elegantly handles both identity types.
1140-1146: LGTM: Consistent soft delete implementation.The
SoftDeleteIdentitymethod follows the same unified pattern as the other identity operations, with proper workspace scoping and support for both identity types.
155-162: ✅ Parameter binding order is correctThe generated code in go/pkg/db/identity_find.sql_generated.go passes parameters as:
- arg.WorkspaceID
- arg.Identity
- arg.Identity
- arg.Deleted
This matches the SQL placeholders (
?,?,?,?) in
WHERE workspace_id = ? AND (external_id = ? OR id = ?) AND deleted = ?. No changes required.
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
go/apps/api/routes/v2_identities_delete_identity/handler.go (1)
206-229: Improved deleteOldIdentity implementationThe refactored function is more efficient by removing redundant lookups and properly handling not-found errors. The workspace-scoped deletion ensures proper tenant isolation.
Consider extracting the error checking pattern to reduce duplication:
func deleteOldIdentity(ctx context.Context, tx db.DBTX, workspaceID, identityID string) error { + handleNotFound := func(err error) error { + if err != nil && !db.IsNotFound(err) { + return err + } + return nil + } + err := db.Query.DeleteManyRatelimitsByIdentityID(ctx, tx, sql.NullString{String: identityID, Valid: true}) - if err != nil && !db.IsNotFound(err) { + if err = handleNotFound(err); err != nil { return fault.Wrap(err, fault.Code(codes.App.Internal.ServiceUnavailable.URN()), fault.Internal("database failed to delete identity ratelimits"), fault.Public("Failed to delete Identity ratelimits."), ) } err = db.Query.DeleteIdentity(ctx, tx, db.DeleteIdentityParams{ WorkspaceID: workspaceID, Identity: identityID, }) - if err != nil && !db.IsNotFound(err) { + if err = handleNotFound(err); err != nil { return fault.Wrap(err, fault.Code(codes.App.Internal.ServiceUnavailable.URN()), fault.Internal("database failed to delete identity"), fault.Public("Failed to delete Identity."), ) } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (37)
go/apps/api/openapi/gen.go(2 hunks)go/apps/api/openapi/openapi-generated.yaml(3 hunks)go/apps/api/openapi/spec/paths/v2/identities/deleteIdentity/V2IdentitiesDeleteIdentityRequestBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/identities/getIdentity/V2IdentitiesGetIdentityRequestBody.yaml(1 hunks)go/apps/api/routes/v2_apis_list_keys/handler.go(2 hunks)go/apps/api/routes/v2_identities_create_identity/200_test.go(10 hunks)go/apps/api/routes/v2_identities_delete_identity/200_test.go(6 hunks)go/apps/api/routes/v2_identities_delete_identity/400_test.go(4 hunks)go/apps/api/routes/v2_identities_delete_identity/401_test.go(8 hunks)go/apps/api/routes/v2_identities_delete_identity/403_test.go(7 hunks)go/apps/api/routes/v2_identities_delete_identity/404_test.go(4 hunks)go/apps/api/routes/v2_identities_delete_identity/handler.go(4 hunks)go/apps/api/routes/v2_identities_get_identity/200_test.go(7 hunks)go/apps/api/routes/v2_identities_get_identity/400_test.go(4 hunks)go/apps/api/routes/v2_identities_get_identity/401_test.go(1 hunks)go/apps/api/routes/v2_identities_get_identity/403_test.go(2 hunks)go/apps/api/routes/v2_identities_get_identity/404_test.go(2 hunks)go/apps/api/routes/v2_identities_get_identity/handler.go(3 hunks)go/apps/api/routes/v2_identities_list_identities/200_test.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/handler.go(2 hunks)go/apps/api/routes/v2_keys_create_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_get_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_update_key/200_test.go(2 hunks)go/apps/api/routes/v2_keys_update_key/handler.go(3 hunks)go/apps/api/routes/v2_keys_update_key/three_state_test.go(3 hunks)go/pkg/codes/constants_gen.go(3 hunks)go/pkg/db/identity_delete.sql_generated.go(1 hunks)go/pkg/db/identity_find.sql_generated.go(1 hunks)go/pkg/db/identity_find_by_external_id.sql_generated.go(0 hunks)go/pkg/db/identity_find_by_id.sql_generated.go(0 hunks)go/pkg/db/identity_soft_delete.sql_generated.go(1 hunks)go/pkg/db/querier_generated.go(3 hunks)go/pkg/db/queries/identity_delete.sql(1 hunks)go/pkg/db/queries/identity_find.sql(1 hunks)go/pkg/db/queries/identity_find_by_external_id.sql(0 hunks)go/pkg/db/queries/identity_find_by_id.sql(0 hunks)go/pkg/db/queries/identity_soft_delete.sql(1 hunks)
💤 Files with no reviewable changes (4)
- go/pkg/db/queries/identity_find_by_external_id.sql
- go/pkg/db/queries/identity_find_by_id.sql
- go/pkg/db/identity_find_by_id.sql_generated.go
- go/pkg/db/identity_find_by_external_id.sql_generated.go
🧰 Additional context used
🧬 Code Graph Analysis (15)
go/pkg/codes/constants_gen.go (1)
go/pkg/urn/urn.go (1)
URN(12-19)
go/apps/api/routes/v2_identities_delete_identity/400_test.go (3)
go/apps/api/routes/v2_identities_delete_identity/handler.go (1)
Request(22-22)go/apps/api/openapi/gen.go (1)
Identity(142-151)go/pkg/db/models_generated.go (1)
Identity(655-664)
go/apps/api/routes/v2_identities_get_identity/403_test.go (1)
go/apps/api/openapi/gen.go (1)
Identity(142-151)
go/apps/api/routes/v2_identities_get_identity/401_test.go (1)
go/apps/api/openapi/gen.go (1)
Identity(142-151)
go/apps/api/routes/v2_identities_get_identity/handler.go (3)
go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(20-24)go/apps/api/openapi/gen.go (1)
Identity(142-151)go/pkg/db/models_generated.go (1)
Identity(655-664)
go/apps/api/routes/v2_identities_list_identities/200_test.go (1)
go/pkg/db/identity_soft_delete.sql_generated.go (1)
SoftDeleteIdentityParams(19-22)
go/apps/api/routes/v2_keys_create_key/handler.go (3)
go/apps/api/openapi/gen.go (3)
V2KeysCreateKeyRequestBody(958-1032)V2KeysCreateKeyResponseBody(1035-1040)Identity(142-151)go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(20-24)go/pkg/db/models_generated.go (1)
Identity(655-664)
go/apps/api/routes/v2_apis_list_keys/handler.go (3)
go/apps/api/openapi/gen.go (2)
V2ApisListKeysRequestBody(622-654)Identity(142-151)go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(20-24)go/pkg/db/models_generated.go (1)
Identity(655-664)
go/pkg/db/identity_soft_delete.sql_generated.go (2)
go/pkg/db/models_generated.go (1)
Identity(655-664)go/pkg/hydra/store/db.go (1)
DBTX(8-13)
go/pkg/db/identity_find.sql_generated.go (1)
go/pkg/hydra/store/db.go (1)
DBTX(8-13)
go/apps/api/openapi/gen.go (1)
go/pkg/db/models_generated.go (1)
Identity(655-664)
go/apps/api/routes/v2_identities_update_identity/handler.go (3)
go/apps/api/openapi/gen.go (3)
V2IdentitiesUpdateIdentityRequestBody(779-796)V2IdentitiesUpdateIdentityResponseBody(799-804)Identity(142-151)go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(20-24)go/pkg/db/models_generated.go (1)
Identity(655-664)
go/apps/api/routes/v2_identities_delete_identity/handler.go (8)
go/apps/api/openapi/gen.go (3)
V2IdentitiesDeleteIdentityRequestBody(717-720)V2IdentitiesDeleteIdentityResponseBody(723-726)Identity(142-151)go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(20-24)go/pkg/db/models_generated.go (1)
Identity(655-664)go/pkg/db/identity_soft_delete.sql_generated.go (1)
SoftDeleteIdentityParams(19-22)go/pkg/db/handle_err_duplicate_key.go (1)
IsDuplicateKeyError(7-13)go/pkg/db/handle_err_no_rows.go (1)
IsNotFound(8-10)go/pkg/fault/wrap.go (3)
Wrap(25-67)Internal(75-89)Public(97-111)go/pkg/db/identity_delete.sql_generated.go (1)
DeleteIdentityParams(18-21)
go/pkg/db/querier_generated.go (5)
go/pkg/hydra/store/db.go (1)
DBTX(8-13)go/pkg/db/identity_delete.sql_generated.go (1)
DeleteIdentityParams(18-21)go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(20-24)go/pkg/db/models_generated.go (1)
Identity(655-664)go/pkg/db/identity_soft_delete.sql_generated.go (1)
SoftDeleteIdentityParams(19-22)
go/apps/api/routes/v2_keys_get_key/handler.go (3)
go/apps/api/openapi/gen.go (3)
V2KeysGetKeyRequestBody(1080-1101)V2KeysGetKeyResponseBody(1110-1115)Identity(142-151)go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(20-24)go/pkg/db/models_generated.go (1)
Identity(655-664)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
🔇 Additional comments (61)
go/apps/api/routes/v2_identities_get_identity/401_test.go (1)
25-25: LGTM! Field rename aligns with the unified identity parameter approach.The change from
ExternalIdtoIdentityis consistent with the broader refactor to support both external and internal identity IDs through a single parameter.go/apps/api/routes/v2_identities_get_identity/handler.go (2)
63-67: Excellent refactor to unified identity lookup.The change from
FindIdentityByExternalIDtoFindIdentitywith the new parameter structure successfully implements the core objective of supporting both external and internal identity IDs through a single parameter. The workspace scoping is properly maintained for security.
119-119: Good modernization of Go type usage.The change from
map[string]interface{}tomap[string]anyis a nice modernization that takes advantage of Go 1.18+ type alias forinterface{}.Also applies to: 128-128
go/apps/api/routes/v2_identities_get_identity/404_test.go (2)
37-37: LGTM! Field renames are consistent with the unified identity parameter approach.The changes from
ExternalIdtoIdentityalign with the refactor to support both external and internal identity IDs through a single parameter.Also applies to: 81-81
70-73: Good improvement to workspace-scoped identity deletion.The change to use a parameter struct for
SoftDeleteIdentitywith explicitWorkspaceIDandIdentityfields improves security by ensuring workspace scoping and supports the unified identity parameter approach.go/pkg/db/queries/identity_find.sql (1)
1-6: Excellent unified identity lookup query design.This SQL query cleanly supports lookups by either
external_idoridwith properworkspace_idscoping anddeletedfiltering.Please verify that your database schema includes indexes to optimize this OR condition, for example:
- Composite index on
(workspace_id, external_id, deleted)- Composite index on
(workspace_id, id, deleted)If these indexes are defined in your migrations or elsewhere in the schema, confirm they’re applied to avoid performance issues.
go/apps/api/routes/v2_identities_get_identity/403_test.go (1)
75-75: LGTM! Consistent field rename aligns with unified identity parameter.The changes correctly update the request struct to use the new
Identityfield instead ofExternalId, maintaining the same test logic while supporting the unified identity lookup approach.Also applies to: 93-93
go/apps/api/routes/v2_identities_delete_identity/403_test.go (1)
34-34: LGTM! Comprehensive field rename maintains test consistency.All test cases correctly update from
ExternalIdtoIdentityfield, maintaining the original test logic while supporting the unified identity parameter approach. The multi-line struct format on lines 132-134 also improves readability.Also applies to: 53-53, 72-72, 91-91, 110-110, 132-134, 153-153
go/apps/api/routes/v2_identities_list_identities/200_test.go (1)
200-203: LGTM! Updated to use workspace-scoped parameter struct.The change correctly updates the soft delete operation to use the new
db.SoftDeleteIdentityParamsstruct, which includes bothIdentityandWorkspaceIDfields. This ensures proper workspace scoping and aligns with the unified identity parameter approach.go/apps/api/routes/v2_identities_update_identity/handler.go (1)
24-27: LGTM! Cleaner type declaration format.The grouped type declaration improves code organization and readability.
go/apps/api/routes/v2_identities_delete_identity/401_test.go (1)
33-33: LGTM! Consistent field updates across all unauthorized test cases.All test cases correctly update from
ExternalIdtoIdentityfield, maintaining the original authentication and authorization test logic. The multi-line struct formats also improve code readability.Also applies to: 51-53, 71-71, 89-91, 109-109, 127-127, 150-150, 168-168
go/apps/api/openapi/spec/paths/v2/identities/deleteIdentity/V2IdentitiesDeleteIdentityRequestBody.yaml (2)
3-6: LGTM! Proper unification of identity field.The schema correctly updates the field name from
externalIdtoidentityand provides clear documentation that it can accept either the external ID or internal identity ID. The constraints and requirements are properly maintained.
10-10: Required field updated correctly.The required field array is properly updated to reflect the new
identityfield name.go/pkg/db/queries/identity_soft_delete.sql (1)
2-5: Excellent unified identity lookup with proper workspace scoping.The SQL query correctly implements the unified identity approach by:
- Adding workspace scoping for security (
workspace_id = sqlc.arg('workspace_id'))- Supporting both internal and external ID lookup with a single parameter (
id = sqlc.arg('identity') OR external_id = sqlc.arg('identity'))This ensures proper isolation between workspaces while enabling flexible identity identification.
go/apps/api/routes/v2_keys_update_key/three_state_test.go (3)
271-274: Correct migration to unified identity query.The test properly updates from
FindIdentityByIDtoFindIdentitywith the new parameter structure that includes workspace scoping. This ensures the test uses the same unified identity lookup approach as the production code.
318-321: Consistent use of unified identity query.Second instance correctly updated to use the new
FindIdentitymethod with proper workspace scoping.
347-352: Good modernization of Go type alias.The change from
map[string]interface{}tomap[string]anyis a nice modernization using Go 1.18+ type alias. This makes the code more readable while maintaining the same functionality.go/apps/api/routes/v2_identities_delete_identity/404_test.go (4)
37-37: Correct update to unified identity field.Request structure properly updated from
ExternalIdtoIdentityfield, consistent with the API refactor.
73-73: Consistent field naming across test scenarios.Another instance correctly updated to use the
Identityfield.
100-103: Proper migration to new database query structure.The
SoftDeleteIdentitycall correctly updated to use the new parameter structure withIdentityandWorkspaceIDfields, ensuring proper workspace scoping in the test.
107-107: Remaining instances updated consistently.All other test cases properly updated to use the
Identityfield, maintaining consistency across the test file.Also applies to: 121-121
go/apps/api/routes/v2_identities_get_identity/200_test.go (3)
43-43: LGTM: Metadata type declaration is correct.The
map[string]anytype is appropriate for handling flexible JSON metadata.
76-76: LGTM: Field name changes are consistent with the API refactoring.The changes from
ExternalIdtoIdentityacross all test cases align with the unified identity parameter approach, where the API now accepts either external IDs or internal identity IDs through a single field.Also applies to: 119-119, 154-154, 241-241, 321-321, 369-369
378-392: Excellent test coverage for the new unified identity parameter.This test case properly validates that identities can be retrieved using their internal ID, which is a key part of the API refactoring. The test logic correctly:
- Creates an identity and captures the internal ID
- Uses the internal ID to retrieve the identity via the
Identityfield- Verifies the response matches the expected external ID
This provides good coverage for the new functionality.
go/apps/api/routes/v2_identities_get_identity/400_test.go (3)
30-30: LGTM: Test name updated to reflect the new field name.The test case name change from "missing externalId" to "missing identity" is consistent with the API field name change.
39-39: LGTM: Error location path correctly updated.The validation error location path is properly updated to match the new field name in the OpenAPI schema.
47-47: LGTM: Request field names updated consistently.The field name changes from
ExternalIdtoIdentityin the test request objects are consistent with the API refactoring.Also applies to: 62-62, 76-76
go/apps/api/routes/v2_keys_update_key/200_test.go (2)
109-109: LGTM: Modern Go type alias usage.Using
map[string]anyinstead ofmap[string]interface{}follows modern Go conventions whereanyis the preferred alias forinterface{}.
137-140: LGTM: Database query updated to use unified identity lookup.The change from
FindIdentityByIDtoFindIdentitywith the new parameter struct is consistent with the database refactoring. The parameters are correctly set:
Identityuses the key's internal identity IDWorkspaceIDprovides proper workspace scopingThis maintains the same functionality while using the new unified query interface.
go/apps/api/routes/v2_keys_create_key/handler.go (2)
29-32: LGTM: Improved code organization.Grouping the type aliases in a single declaration block improves readability and follows Go style conventions.
208-211: LGTM: Database query updated to use unified identity lookup.The change from
FindIdentityByExternalIDtoFindIdentityis consistent with the database refactoring. The new parameter struct correctly includes:
WorkspaceIDfor proper workspace scopingIdentitywith the external ID valueDeleted: falseto filter out deleted identitiesThis maintains the same functionality while using the new unified query interface.
go/apps/api/openapi/spec/paths/v2/identities/getIdentity/V2IdentitiesGetIdentityRequestBody.yaml (4)
3-3: LGTM: Property name updated to reflect unified identity parameter.The change from
externalIdtoidentityaligns with the API refactoring to support both external and internal identity identifiers.
5-5: LGTM: Improved validation with minimum length requirement.Increasing the
minLengthfrom 1 to 3 characters provides better validation for identity values, as very short identifiers are likely to be invalid.
6-6: LGTM: Clear and comprehensive field description.The updated description clearly explains that the
identityfield can accept either an external ID or internal identity ID, which is essential for API users to understand the new unified parameter.
10-10: LGTM: Required field updated consistently.The required field is correctly updated to match the new property name
identity.go/pkg/db/queries/identity_delete.sql (1)
2-4: LGTM! Secure implementation of unified identity deletion.The SQL query correctly implements workspace scoping and uses proper parameter binding to prevent SQL injection. The OR condition allows flexible matching of either internal ID or external ID using the same parameter value.
go/pkg/db/identity_delete.sql_generated.go (2)
18-21: Parameter struct correctly implements unified identity lookup.The struct properly defines the required fields for workspace-scoped identity operations.
28-30: Function correctly passes identity parameter twice.The implementation properly passes
arg.Identitytwice to match both theidandexternal_idplaceholders in the SQL query, enabling flexible identity matching.go/pkg/db/identity_soft_delete.sql_generated.go (2)
19-22: Parameter struct correctly mirrors delete operation structure.Consistent implementation of the unified identity parameter approach for soft delete operations.
30-32: Soft delete implementation correctly handles identity parameter.The function properly passes the identity parameter twice to match both
idandexternal_idconditions in the UPDATE query.go/apps/api/routes/v2_keys_update_key/handler.go (3)
25-28: Code formatting improved with grouped type declarations.Clean formatting change that improves readability without altering functionality.
158-162: Identity lookup correctly updated to use unified query.The refactored call properly includes workspace scoping and uses the new
FindIdentityParamsstructure. TheIdentityfield correctly receives the external ID value.
193-197: Consistent implementation of unified identity lookup in error path.The fallback identity lookup in the duplicate key error handling correctly uses the same unified approach with proper workspace scoping.
go/apps/api/routes/v2_keys_get_key/handler.go (2)
25-28: Type declarations properly grouped for improved readability.Consistent formatting improvement that enhances code organization.
256-260: Identity lookup correctly implements workspace-scoped unified query.The refactored call demonstrates the flexibility of the unified approach by using an internal identity ID while maintaining proper workspace scoping and security. The parameter structure correctly maps to the new
FindIdentitymethod.go/apps/api/openapi/gen.go (2)
718-719: LGTM! Well-designed field consolidation.The replacement of
ExternalIdwith the more flexibleIdentityfield is a good design improvement. The comment clearly explains that it accepts either external IDs or internal identity IDs, which provides better API flexibility for consumers.
730-731: Excellent consistency across identity operations.The
Identityfield implementation here is identical to the delete operation, maintaining consistent API patterns. This consistency will improve the developer experience when working with identity operations.go/pkg/db/identity_find.sql_generated.go (3)
1-4: Generated file - no manual edits allowedThis is an auto-generated file. Any manual changes will be overwritten on the next generation.
12-24: Well-designed unified identity queryThe SQL query correctly implements a unified lookup that can match either external or internal IDs within a workspace scope. The parameter structure properly encapsulates the query inputs.
26-52: Correct implementation of unified identity lookupThe method properly executes the query with duplicate identity parameters to support the OR condition, correctly scans all columns, and follows standard database query patterns.
go/apps/api/routes/v2_identities_create_identity/200_test.go (3)
52-56: Correct migration to unified identity queryThe test properly uses the new
FindIdentitymethod with workspace scoping and the internal identity ID.
111-118: Validates external ID lookup functionalityThese test cases correctly demonstrate that the unified
FindIdentitymethod can query by external ID, confirming the OR condition in the SQL query works as intended.Also applies to: 133-140, 174-181
233-240: Consistent application of unified identity query across all testsAll test cases have been properly updated to use the new
FindIdentitymethod with appropriate workspace scoping, maintaining test coverage for both internal and external ID lookups.Also applies to: 309-316, 350-358, 380-387, 420-426
go/apps/api/routes/v2_identities_delete_identity/200_test.go (1)
94-94: Correct API request field migrationThe request structure properly uses the new
Identityfield, maintaining functionality while supporting the unified identity parameter approach.Also applies to: 129-129, 158-158, 176-176, 202-202, 219-219
go/apps/api/routes/v2_identities_delete_identity/handler.go (3)
21-24: Correct type alias updatesThe type aliases properly reference the updated OpenAPI schema for the unified identity parameter approach.
73-77: Proper workspace-scoped identity lookupThe identity lookup correctly uses the unified
FindIdentitymethod with workspace scoping and the generic identity parameter from the request.
100-103: Correct soft delete parameter updatesThe soft delete operations properly use the new parameter structure with workspace scoping and the generic identity parameter, maintaining the duplicate key retry logic.
Also applies to: 114-117
go/apps/api/openapi/openapi-generated.yaml (1)
435-443: Verify stricterminLength: 3will not break existing clientsBoth request bodies now require an
identitywithminLength: 3. If any existing external IDs / internal IDs are shorter (e.g. “id”, “42”), requests will begin failing with 400 responses once the spec is enforced.Please confirm this constraint matches production data, or reduce the limit / add a migration notice. A quick grep over prod data (or tests) should surface shorter IDs.
go/pkg/db/querier_generated.go (3)
25-28: LGTM: Unified identity deletion with workspace scopingThe
DeleteIdentitymethod correctly implements the unified approach by accepting either internal or external IDs through a singleIdentityparameter while properly scoping to the workspace for security.
155-162: LGTM: Unified identity lookup with comprehensive filteringThe new
FindIdentitymethod successfully consolidates the previous separateFindIdentityByExternalIDandFindIdentityByIDmethods into a single, more flexible interface. The implementation correctly includes workspace scoping, supports both ID types through the OR condition, and maintains proper deleted flag filtering.
142-146: LGTM: Unified identity soft deletion with workspace scopingThe
SoftDeleteIdentitymethod correctly implements the unified approach consistent with the other identity operations. It properly supports both ID types through the OR condition and maintains workspace scoping for security.
chronark
left a comment
chronark
left a comment
There was a problem hiding this comment.
code rabbit caught most things
|
I’ll check first thing on Monday 🫡 🫡 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
go/apps/api/routes/v2_identities_delete_identity/400_test.go (1)
89-92: Update variable name for consistency.The field change is correct, but the variable name
longExternalIDshould be updated to reflect the unified identity parameter semantics.- longExternalID := "test_" + string(make([]byte, 1000)) - req := handler.Request{Identity: longExternalID} + longIdentity := "test_" + string(make([]byte, 1000)) + req := handler.Request{Identity: longIdentity}go/apps/api/routes/v2_identities_get_identity/403_test.go (1)
91-93: Update comment to reflect the unified identity parameter.The field change is correct, but the comment should be updated to reflect that the
Identityfield now accepts both external IDs and internal identity IDs.- // Try to use identity when only having permission for specific identity IDs + // Try to use identity when only having permission for specific identity IDsgo/apps/api/openapi/gen.go (1)
730-732: Same naming collision applies hereThe
Identityfield in the get-request struct shadows the package-levelIdentitytype for readers. See previous comment for rationale and suggested alternatives.go/apps/api/openapi/openapi-generated.yaml (2)
1-3: Timestamp still causes noisy diffsThe non-deterministic “Generated at” line will flip on every regeneration and keep polluting the change log. Please strip or stabilise it in the generator/post-step.
416-423: Duplication ofidentityschema – extract a shared componentThe
identityproperty (type, minLength, description) is now repeated in both Delete and Get request bodies. Define it once undercomponents/schemas/IdentityParamand$refit here to keep the spec DRY and avoid future drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
go/apps/api/openapi/gen.go(2 hunks)go/apps/api/openapi/openapi-generated.yaml(3 hunks)go/apps/api/openapi/spec/paths/v2/identities/deleteIdentity/V2IdentitiesDeleteIdentityRequestBody.yaml(1 hunks)go/apps/api/routes/v2_identities_delete_identity/200_test.go(7 hunks)go/apps/api/routes/v2_identities_delete_identity/400_test.go(4 hunks)go/apps/api/routes/v2_identities_get_identity/400_test.go(4 hunks)go/apps/api/routes/v2_identities_get_identity/403_test.go(2 hunks)go/pkg/db/schema.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2825
File: apps/dashboard/app/(app)/logs-v2/hooks/use-bookmarked-filters.ts:0-0
Timestamp: 2025-01-30T20:51:44.359Z
Learning: The user (ogzhanolguncu) prefers to handle refactoring suggestions in separate PRs to maintain focus in the current PR.
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.134Z
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: Flo4604
PR: unkeyed/unkey#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.
Learnt from: chronark
PR: unkeyed/unkey#2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In `apps/api/src/routes/v1_keys_updateKey.ts`, the code intentionally handles `externalId` and `ownerId` separately for clarity. The `ownerId` field will be removed in the future, simplifying the code.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#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.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3315
File: apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/general-setup.tsx:40-50
Timestamp: 2025-06-19T13:01:55.338Z
Learning: In the create-key form's GeneralSetup component, the Controller is intentionally bound to "identityId" as the primary field while "externalId" is set explicitly via setValue. The ExternalIdField component has been designed to handle this pattern where it receives identityId as its value prop but manages both identityId and externalId through its onChange callback.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3297
File: apps/dashboard/lib/trpc/routers/authorization/roles/query.ts:210-323
Timestamp: 2025-06-04T20:13:12.060Z
Learning: The user ogzhanolguncu prefers explicit, duplicated code over abstracted helper functions when it improves readability, even if it means some duplication in filter building functions in the authorization roles query module.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3480
File: apps/dashboard/app/new-2/hooks/use-workspace-step.tsx:47-79
Timestamp: 2025-07-09T11:35:51.724Z
Learning: In the Unkey codebase, ogzhanolguncu prefers to keep invariant checks that throw errors for cases that shouldn't happen in normal operation (like null workspace ID checks), rather than adding graceful error handling code for edge cases that would only occur if someone tampers with the actual flow.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3375
File: apps/dashboard/app/(app)/settings/root-keys/components/table/hooks/use-root-keys-list-query.ts:0-0
Timestamp: 2025-06-25T20:32:10.471Z
Learning: In the Unkey codebase, ogzhanolguncu prefers strict validation with fail-fast error handling. When validation errors occur that shouldn't happen in normal operation (like invalid operators), throwing errors to crash the page is preferred over graceful error handling or console logging.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3499
File: apps/dashboard/app/new/hooks/use-workspace-step.tsx:19-26
Timestamp: 2025-07-11T13:00:05.416Z
Learning: In the Unkey codebase, ogzhanolguncu prefers to keep commented code for planned future features (like slug-based workspaces) rather than removing it, as it serves as a reference for upcoming implementation.
go/pkg/db/schema.sql (2)
Learnt from: ogzhanolguncu
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.
Learnt from: ogzhanolguncu
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.
go/apps/api/routes/v2_identities_delete_identity/400_test.go (7)
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: Flo4604
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.
Learnt from: chronark
PR: #2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In apps/api/src/routes/v1_keys_updateKey.ts, the code intentionally handles externalId and ownerId separately for clarity. The ownerId field will be removed in the future, simplifying the code.
Learnt from: chronark
PR: #2126
File: apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts:36-36
Timestamp: 2024-11-13T19:06:36.786Z
Learning: In the rate limit test files (e.g., apps/api/src/routes/v1_ratelimit_getOverride.happy.test.ts), URL parameters like namespaceId and identifier do not need to be URL-encoded in the test code because the values used are always considered safe within the test environment.
Learnt from: CR
PR: unkeyed/unkey#0
File: go/deploy/CLAUDE.md:0-0
Timestamp: 2025-07-21T18:05:58.236Z
Learning: Applies to go/deploy/**/*.{go,js,ts,tsx,py,sh,md,txt,json,yaml,yml,ini,env,conf,html,css,scss,xml,c,h,cpp,java,rb,rs,php,pl,sql} : Use AIDEV-NOTE:, AIDEV-TODO:, AIDEV-BUSINESS_RULE:, or AIDEV-QUESTION: (all-caps prefix) as anchor comments aimed at AI and developers.
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: ogzhanolguncu
PR: #3315
File: apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/general-setup.tsx:40-50
Timestamp: 2025-06-19T13:01:55.338Z
Learning: In the create-key form's GeneralSetup component, the Controller is intentionally bound to "identityId" as the primary field while "externalId" is set explicitly via setValue. The ExternalIdField component has been designed to handle this pattern where it receives identityId as its value prop but manages both identityId and externalId through its onChange callback.
go/apps/api/routes/v2_identities_get_identity/403_test.go (5)
Learnt from: ogzhanolguncu
PR: #3661
File: go/apps/api/routes/v2_identities_update_identity/handler.go:115-119
Timestamp: 2025-07-28T11:47:43.134Z
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: Flo4604
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.
Learnt from: chronark
PR: #2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In apps/api/src/routes/v1_keys_updateKey.ts, the code intentionally handles externalId and ownerId separately for clarity. The ownerId field will be removed in the future, simplifying the code.
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: ogzhanolguncu
PR: #3315
File: apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/general-setup.tsx:40-50
Timestamp: 2025-06-19T13:01:55.338Z
Learning: In the create-key form's GeneralSetup component, the Controller is intentionally bound to "identityId" as the primary field while "externalId" is set explicitly via setValue. The ExternalIdField component has been designed to handle this pattern where it receives identityId as its value prop but manages both identityId and externalId through its onChange callback.
go/apps/api/openapi/spec/paths/v2/identities/deleteIdentity/V2IdentitiesDeleteIdentityRequestBody.yaml (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: Flo4604
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.
Learnt from: chronark
PR: #2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In apps/api/src/routes/v1_keys_updateKey.ts, the code intentionally handles externalId and ownerId separately for clarity. The ownerId field will be removed in the future, simplifying the code.
Learnt from: ogzhanolguncu
PR: #3315
File: apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/general-setup.tsx:40-50
Timestamp: 2025-06-19T13:01:55.338Z
Learning: In the create-key form's GeneralSetup component, the Controller is intentionally bound to "identityId" as the primary field while "externalId" is set explicitly via setValue. The ExternalIdField component has been designed to handle this pattern where it receives identityId as its value prop but manages both identityId and externalId through its onChange callback.
go/apps/api/routes/v2_identities_get_identity/400_test.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: Flo4604
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.
Learnt from: chronark
PR: #2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In apps/api/src/routes/v1_keys_updateKey.ts, the code intentionally handles externalId and ownerId separately for clarity. The ownerId field will be removed in the future, simplifying the code.
Learnt from: Flo4604
PR: #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.
go/apps/api/routes/v2_identities_delete_identity/200_test.go (7)
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: Flo4604
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.
Learnt from: chronark
PR: #2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In apps/api/src/routes/v1_keys_updateKey.ts, the code intentionally handles externalId and ownerId separately for clarity. The ownerId field will be removed in the future, simplifying the code.
Learnt from: ogzhanolguncu
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.
Learnt from: ogzhanolguncu
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.
Learnt from: chronark
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.
Learnt from: chronark
PR: #3617
File: go/apps/api/openapi/openapi.yaml:3309-3312
Timestamp: 2025-07-16T17:51:57.297Z
Learning: In the Unkey API OpenAPI schema, the permissions query regex for the verifyKey endpoint intentionally allows all whitespace characters (including tabs and newlines) via \s. Do not flag this as an error in future reviews.
go/apps/api/openapi/gen.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: Flo4604
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.
Learnt from: chronark
PR: #2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In apps/api/src/routes/v1_keys_updateKey.ts, the code intentionally handles externalId and ownerId separately for clarity. The ownerId field will be removed in the future, simplifying the code.
Learnt from: ogzhanolguncu
PR: #3315
File: apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/general-setup.tsx:40-50
Timestamp: 2025-06-19T13:01:55.338Z
Learning: In the create-key form's GeneralSetup component, the Controller is intentionally bound to "identityId" as the primary field while "externalId" is set explicitly via setValue. The ExternalIdField component has been designed to handle this pattern where it receives identityId as its value prop but manages both identityId and externalId through its onChange callback.
go/apps/api/openapi/openapi-generated.yaml (13)
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: Flo4604
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.
Learnt from: chronark
PR: #2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In apps/api/src/routes/v1_keys_updateKey.ts, the code intentionally handles externalId and ownerId separately for clarity. The ownerId field will be removed in the future, simplifying the code.
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: Flo4604
PR: #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: ogzhanolguncu
PR: #3315
File: apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/general-setup.tsx:40-50
Timestamp: 2025-06-19T13:01:55.338Z
Learning: In the create-key form's GeneralSetup component, the Controller is intentionally bound to "identityId" as the primary field while "externalId" is set explicitly via setValue. The ExternalIdField component has been designed to handle this pattern where it receives identityId as its value prop but manages both identityId and externalId through its onChange callback.
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.
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: chronark
PR: #2146
File: apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts:80-80
Timestamp: 2024-10-04T17:27:08.666Z
Learning: Ensure that audit log descriptions accurately reflect the action being performed, such as updating the defaultPrefix, and avoid incorrect references like 'name' when not applicable.
Learnt from: CR
PR: unkeyed/unkey#0
File: go/deploy/CLAUDE.md:0-0
Timestamp: 2025-07-21T18:05:58.236Z
Learning: Applies to go/deploy/**/*.{go,js,ts,tsx,py,sh,md,txt,json,yaml,yml,ini,env,conf,html,css,scss,xml,c,h,cpp,java,rb,rs,php,pl,sql} : Use AIDEV-NOTE:, AIDEV-TODO:, AIDEV-BUSINESS_RULE:, or AIDEV-QUESTION: (all-caps prefix) as anchor comments aimed at AI and developers.
Learnt from: CR
PR: unkeyed/unkey#0
File: go/deploy/CLAUDE.md:0-0
Timestamp: 2025-07-21T18:05:58.236Z
Learning: Applies to go/deploy/**/*.{go,js,ts,tsx,py,sh,md,txt,json,yaml,yml,ini,env,conf,html,css,scss,xml,c,h,cpp,java,rb,rs,php,pl,sql} : Do not remove AIDEV-*s without explicit human instruction.
Learnt from: CR
PR: unkeyed/unkey#0
File: go/deploy/CLAUDE.md:0-0
Timestamp: 2025-07-21T18:05:58.236Z
Learning: Applies to go/deploy/**/*.{go,js,ts,tsx,py,sh,md,txt,json,yaml,yml,ini,env,conf,html,css,scss,xml,c,h,cpp,java,rb,rs,php,pl,sql} : Make sure to add relevant anchor comments whenever a file or piece of code is too complex, very important, confusing, or could have a bug.
Learnt from: CR
PR: unkeyed/unkey#0
File: go/deploy/CLAUDE.md:0-0
Timestamp: 2025-07-21T18:05:58.236Z
Learning: Applies to go/deploy/deploy/{assetmanagerd,billaged,builderd,metald}/**/*.go : When a service's *.go code changes significantly, increase the patch-level version number.
🧬 Code Graph Analysis (4)
go/apps/api/routes/v2_identities_delete_identity/400_test.go (3)
go/apps/api/routes/v2_identities_delete_identity/handler.go (1)
Request(22-22)go/apps/api/openapi/gen.go (1)
Identity(142-151)go/pkg/db/models_generated.go (1)
Identity(655-664)
go/apps/api/routes/v2_identities_get_identity/403_test.go (2)
go/apps/api/openapi/gen.go (1)
Identity(142-151)go/apps/api/routes/v2_identities_get_identity/handler.go (1)
Request(19-19)
go/apps/api/routes/v2_identities_delete_identity/200_test.go (4)
go/pkg/db/identity_find.sql_generated.go (1)
FindIdentityParams(20-24)go/pkg/testutil/seed/seed.go (1)
Resources(20-25)go/pkg/db/models_generated.go (1)
Identity(655-664)go/pkg/testutil/http.go (1)
CallRoute(257-291)
go/apps/api/openapi/gen.go (1)
go/pkg/db/models_generated.go (1)
Identity(655-664)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Agent Local / test_agent_local
🔇 Additional comments (21)
go/pkg/db/schema.sql (1)
433-433: LGTM! Index supports the unified identity lookup pattern.This composite index is well-designed to support the new unified
FindIdentityquery that replaces the separateFindIdentityByIDandFindIdentityByExternalIDqueries. The index coversworkspace_id,id, anddeletedcolumns in the optimal order, providing efficient lookups when querying identities by internal ID within a workspace while considering the deletion status.This complements the existing
workspace_id_external_id_deleted_idxunique constraint for external ID lookups, ensuring both identity lookup paths are properly indexed.go/apps/api/routes/v2_identities_delete_identity/400_test.go (3)
47-48: LGTM! Field rename is consistent with the unified identity parameter.The change from
ExternalIdtoIdentitycorrectly reflects the API refactoring that allows the identity field to accept both external IDs and internal identity IDs.
61-62: LGTM! Test description and field usage correctly updated.The test name and field usage have been appropriately updated to reflect the unified identity parameter semantics.
75-77: LGTM! Test description and comment properly updated.The test name and comment have been correctly updated to reflect that the
Identityfield now accepts both external IDs and internal identity IDs.go/apps/api/routes/v2_identities_get_identity/403_test.go (1)
75-75: LGTM! Field rename is consistent with the unified identity parameter.The change from
ExternalIdtoIdentitycorrectly reflects the refactoring that allows querying by either external ID or internal identity ID.go/apps/api/openapi/spec/paths/v2/identities/deleteIdentity/V2IdentitiesDeleteIdentityRequestBody.yaml (2)
3-6: LGTM! Property rename and description improve API clarity.The change from
externalIdtoidentityunifies the identity parameter semantics, and the updated description clearly explains that the field accepts both external IDs and internal identity IDs. This provides better flexibility for API consumers.
8-10: LGTM! Required property and schema constraints correctly updated.The repositioning of
additionalProperties: falseand the update of the required property fromexternalIdtoidentitymaintain schema correctness and consistency.go/apps/api/routes/v2_identities_get_identity/400_test.go (5)
30-30: LGTM! Test description correctly updated.The test name has been appropriately changed from "missing externalId" to "missing identity" to reflect the unified identity parameter.
39-39: LGTM! Error validation path correctly updated.The assertion has been properly updated to check for the new error location
/properties/identity/minLengthinstead of the previous/properties/externalId/minLength.
45-47: LGTM! Test description and field usage correctly updated.Both the test name and the field usage have been appropriately updated to reflect the unified identity parameter semantics.
62-62: LGTM! Field rename is consistent.The change from
ExternalIdtoIdentitymaintains consistency with the unified identity parameter refactoring.
76-76: LGTM! Field rename is consistent.The change from
ExternalIdtoIdentitycorrectly reflects the API refactoring that allows the identity field to accept both external IDs and internal identity IDs.go/apps/api/routes/v2_identities_delete_identity/200_test.go (8)
40-40: LGTM: Modern Go range syntax.Good use of the range-over-int syntax introduced in Go 1.22, which is more idiomatic for simple counting loops.
85-94: LGTM: Consistent unified identity parameter usage.The changes correctly implement the unified identity approach by:
- Using the new
Identityfield in request structs instead ofExternalId- Properly calling
FindIdentitywithFindIdentityParamsincluding workspace scoping- Maintaining consistent use of external ID for lookups in this test case
101-113: LGTM: Proper soft deletion verification.The FindIdentity calls correctly verify the soft deletion behavior by checking both the removal from active queries (
Deleted: false) and persistence for audit purposes (Deleted: true).
129-140: LGTM: Testing unified identity parameter behavior.The test correctly validates that deletion by external ID (in the request) properly affects the corresponding internal identity (in the verification), demonstrating the unified identity parameter functionality.
159-170: LGTM: Consistent wildcard permission testing.The test maintains the same pattern of deleting by external ID and verifying by internal ID, properly testing the unified identity parameter with wildcard permissions.
178-178: LGTM: Consistent request parameter usage.Correct use of the unified
Identityfield in the request struct.
204-230: LGTM: Proper duplicate key error handling test.The test correctly implements the duplicate key scenario by deleting by external ID and verifying the new identity by internal ID, ensuring proper edge case handling.
234-238: LGTM: Correct hard deletion verification.The test properly verifies that the original identity was hard deleted (not found even with
Deleted: true) as expected in the duplicate key error handling scenario.go/apps/api/openapi/openapi-generated.yaml (1)
435-443: Breaking change risk:minLengthraised from 1 → 3Clients that previously sent a 1- or 2-character
externalIdwill now be rejected with 422. Was this increase intentional? If not strictly required, keepminLength: 1to maintain backward compatibility.- minLength: 3 + minLength: 1
|
other than that we can merge it |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
go/pkg/codes/constants_gen.go (1)
9-130: Do not hand-edit generated code – update the generator insteadThis file carries the
"// Code generated … DO NOT EDIT."header, yet this PR manually removes indentation from comment blocks throughout the file. Any futurego generate(or CI) run will overwrite these changes, making the effort transient and adding merge noise.Move the desired formatting logic into the code generator source and regenerate the file, then commit the result.
go/apps/api/openapi/openapi-generated.yaml (1)
409-416:identityproperty duplicated → extract shared schema
The exact sameidentityproperty block (type, minLength, description) now lives in both Get- and Delete-request bodies. Define it once undercomponents/schemas/IdentityParamand$refit from both places to avoid future drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
go/apps/api/openapi/openapi-generated.yaml(2 hunks)go/apps/api/routes/v2_keys_whoami/handler.go(1 hunks)go/pkg/codes/constants_gen.go(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2825
File: apps/dashboard/app/(app)/logs-v2/hooks/use-bookmarked-filters.ts:0-0
Timestamp: 2025-01-30T20:51:44.359Z
Learning: The user (ogzhanolguncu) prefers to handle refactoring suggestions in separate PRs to maintain focus in the current PR.
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: Flo4604
PR: unkeyed/unkey#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.
Learnt from: chronark
PR: unkeyed/unkey#2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In `apps/api/src/routes/v1_keys_updateKey.ts`, the code intentionally handles `externalId` and `ownerId` separately for clarity. The `ownerId` field will be removed in the future, simplifying the code.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#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.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3315
File: apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/general-setup.tsx:40-50
Timestamp: 2025-06-19T13:01:55.338Z
Learning: In the create-key form's GeneralSetup component, the Controller is intentionally bound to "identityId" as the primary field while "externalId" is set explicitly via setValue. The ExternalIdField component has been designed to handle this pattern where it receives identityId as its value prop but manages both identityId and externalId through its onChange callback.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3297
File: apps/dashboard/lib/trpc/routers/authorization/roles/query.ts:210-323
Timestamp: 2025-06-04T20:13:12.060Z
Learning: The user ogzhanolguncu prefers explicit, duplicated code over abstracted helper functions when it improves readability, even if it means some duplication in filter building functions in the authorization roles query module.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3480
File: apps/dashboard/app/new-2/hooks/use-workspace-step.tsx:47-79
Timestamp: 2025-07-09T11:35:51.724Z
Learning: In the Unkey codebase, ogzhanolguncu prefers to keep invariant checks that throw errors for cases that shouldn't happen in normal operation (like null workspace ID checks), rather than adding graceful error handling code for edge cases that would only occur if someone tampers with the actual flow.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3375
File: apps/dashboard/app/(app)/settings/root-keys/components/table/hooks/use-root-keys-list-query.ts:0-0
Timestamp: 2025-06-25T20:32:10.471Z
Learning: In the Unkey codebase, ogzhanolguncu prefers strict validation with fail-fast error handling. When validation errors occur that shouldn't happen in normal operation (like invalid operators), throwing errors to crash the page is preferred over graceful error handling or console logging.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3499
File: apps/dashboard/app/new/hooks/use-workspace-step.tsx:19-26
Timestamp: 2025-07-11T13:00:05.416Z
Learning: In the Unkey codebase, ogzhanolguncu prefers to keep commented code for planned future features (like slug-based workspaces) rather than removing it, as it serves as a reference for upcoming implementation.
go/pkg/codes/constants_gen.go (5)
Learnt from: CR
PR: unkeyed/unkey#0
File: go/deploy/CLAUDE.md:0-0
Timestamp: 2025-07-21T18:05:58.236Z
Learning: Applies to go/deploy/**/*.{go,js,ts,tsx,py,sh,md,txt,json,yaml,yml,ini,env,conf,html,css,scss,xml,c,h,cpp,java,rb,rs,php,pl,sql} : Make sure to add relevant anchor comments whenever a file or piece of code is too complex, very important, confusing, or could have a bug.
Learnt from: CR
PR: unkeyed/unkey#0
File: go/deploy/CLAUDE.md:0-0
Timestamp: 2025-07-21T18:05:58.236Z
Learning: Applies to go/deploy/deploy/{assetmanagerd,billaged,builderd,metald}/**/*.go : When a service's *.go code changes significantly, increase the patch-level version number.
Learnt from: CR
PR: unkeyed/unkey#0
File: go/deploy/CLAUDE.md:0-0
Timestamp: 2025-07-21T18:05:58.236Z
Learning: Applies to go/deploy/**/*.{go,js,ts,tsx,py,sh,md,txt,json,yaml,yml,ini,env,conf,html,css,scss,xml,c,h,cpp,java,rb,rs,php,pl,sql} : Update relevant anchors when modifying associated code.
Learnt from: CR
PR: unkeyed/unkey#0
File: go/deploy/CLAUDE.md:0-0
Timestamp: 2025-07-21T18:05:58.236Z
Learning: Applies to go/deploy/**/*.{go,js,ts,tsx,py,sh,md,txt,json,yaml,yml,ini,env,conf,html,css,scss,xml,c,h,cpp,java,rb,rs,php,pl,sql} : Use AIDEV-NOTE:, AIDEV-TODO:, AIDEV-BUSINESS_RULE:, or AIDEV-QUESTION: (all-caps prefix) as anchor comments aimed at AI and developers.
Learnt from: CR
PR: unkeyed/unkey#0
File: go/deploy/CLAUDE.md:0-0
Timestamp: 2025-07-21T18:05:58.236Z
Learning: Applies to go/deploy/**/*.{go,js,ts,tsx,py,sh,md,txt,json,yaml,yml,ini,env,conf,html,css,scss,xml,c,h,cpp,java,rb,rs,php,pl,sql} : Do not remove AIDEV-*s without explicit human instruction.
go/apps/api/routes/v2_keys_whoami/handler.go (5)
Learnt from: ogzhanolguncu
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.
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: #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.
Learnt from: chronark
PR: #2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In apps/api/src/routes/v1_keys_updateKey.ts, the code intentionally handles externalId and ownerId separately for clarity. The ownerId field will be removed in the future, simplifying the code.
Learnt from: Flo4604
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.
go/apps/api/openapi/openapi-generated.yaml (13)
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: Flo4604
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.
Learnt from: chronark
PR: #2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In apps/api/src/routes/v1_keys_updateKey.ts, the code intentionally handles externalId and ownerId separately for clarity. The ownerId field will be removed in the future, simplifying the code.
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: Flo4604
PR: #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: 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.
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: chronark
PR: #2146
File: apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts:80-80
Timestamp: 2024-10-04T17:27:08.666Z
Learning: Ensure that audit log descriptions accurately reflect the action being performed, such as updating the defaultPrefix, and avoid incorrect references like 'name' when not applicable.
Learnt from: CR
PR: unkeyed/unkey#0
File: go/deploy/CLAUDE.md:0-0
Timestamp: 2025-07-21T18:05:58.236Z
Learning: Applies to go/deploy/**/*.{go,js,ts,tsx,py,sh,md,txt,json,yaml,yml,ini,env,conf,html,css,scss,xml,c,h,cpp,java,rb,rs,php,pl,sql} : Use AIDEV-NOTE:, AIDEV-TODO:, AIDEV-BUSINESS_RULE:, or AIDEV-QUESTION: (all-caps prefix) as anchor comments aimed at AI and developers.
Learnt from: CR
PR: unkeyed/unkey#0
File: go/deploy/CLAUDE.md:0-0
Timestamp: 2025-07-21T18:05:58.236Z
Learning: Applies to go/deploy/**/*.{go,js,ts,tsx,py,sh,md,txt,json,yaml,yml,ini,env,conf,html,css,scss,xml,c,h,cpp,java,rb,rs,php,pl,sql} : Do not remove AIDEV-*s without explicit human instruction.
Learnt from: CR
PR: unkeyed/unkey#0
File: go/deploy/CLAUDE.md:0-0
Timestamp: 2025-07-21T18:05:58.236Z
Learning: Applies to go/deploy/**/*.{go,js,ts,tsx,py,sh,md,txt,json,yaml,yml,ini,env,conf,html,css,scss,xml,c,h,cpp,java,rb,rs,php,pl,sql} : Make sure to add relevant anchor comments whenever a file or piece of code is too complex, very important, confusing, or could have a bug.
Learnt from: CR
PR: unkeyed/unkey#0
File: go/deploy/CLAUDE.md:0-0
Timestamp: 2025-07-21T18:05:58.236Z
Learning: Applies to go/deploy/deploy/{assetmanagerd,billaged,builderd,metald}/**/*.go : When a service's *.go code changes significantly, increase the patch-level version number.
Learnt from: ogzhanolguncu
PR: #3315
File: apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/components/general-setup.tsx:40-50
Timestamp: 2025-06-19T13:01:55.338Z
Learning: In the create-key form's GeneralSetup component, the Controller is intentionally bound to "identityId" as the primary field while "externalId" is set explicitly via setValue. The ExternalIdField component has been designed to handle this pattern where it receives identityId as its value prop but manages both identityId and externalId through its onChange callback.
🧬 Code Graph Analysis (1)
go/pkg/codes/constants_gen.go (1)
go/pkg/urn/urn.go (1)
URN(12-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
🔇 Additional comments (1)
go/apps/api/routes/v2_keys_whoami/handler.go (1)
165-165: LGTM! Improved security with workspace scopingThe change from
FindIdentityByIDto the unifiedFindIdentitymethod properly scopes the identity lookup to the authorized workspace (auth.AuthorizedWorkspaceID), preventing potential cross-workspace access issues. This aligns with the broader refactor to consolidate identity query methods while enhancing security.
ogzhanolguncu
force-pushed
the
update-identity-param-for-v2
branch
from
d29a7dc to
3ca7a64
Compare
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (07/29/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 participants
What does this PR do?
This PR refactors
api/routes/v2_identities_get_identityendpoint to make it queryable by bothidentityIdandexternalIdusing a single field.Type of change
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
identityand accepts both types of identifiers.Bug Fixes
Refactor
Tests
identityparameter and added coverage for using internal identity IDs.Documentation
identityfield supports both internal and external identifiers.