feat: v2/identitiesDeleteIdentity#3151
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@Flo4604 is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis change introduces a new API endpoint Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Router
participant DeleteIdentityHandler
participant DB
participant AuditLogService
participant PermissionsService
Client->>API_Router: POST /v2/identities.deleteIdentity (with identityId or externalId)
API_Router->>DeleteIdentityHandler: Route request
DeleteIdentityHandler->>PermissionsService: Check delete_identity permission
alt Permission OK
DeleteIdentityHandler->>DB: Find identity by id or externalId (not deleted)
alt Identity found
DeleteIdentityHandler->>DB: Begin transaction
DeleteIdentityHandler->>DB: Soft delete identity (set deleted=true)
DeleteIdentityHandler->>AuditLogService: Write audit log(s)
DeleteIdentityHandler->>DB: Commit transaction
DeleteIdentityHandler-->>API_Router: 200 OK, empty body
else Not found
DeleteIdentityHandler-->>API_Router: 404 Not Found
end
else Permission denied
DeleteIdentityHandler-->>API_Router: 403 Forbidden
end
API_Router-->>Client: Response
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (29)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
Flo4604
requested review from
MichaelUnkey,
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
go/apps/api/routes/v2_identities_delete_identity/401_test.go (1)
14-38: Unauthorized test case is properly implementedGood job implementing a test for unauthorized access. The test correctly verifies that the endpoint returns a 401 status code when an invalid authorization token is provided.
Consider adding additional test cases for other unauthorized scenarios like missing tokens or expired tokens if they aren't covered by authentication middleware tests.
go/apps/api/routes/v2_identities_delete_identity/404_test.go (1)
55-55: Use require.NoError instead of require.Nil for errors.For better readability and more descriptive failure messages, prefer using require.NoError() over require.Nil() when checking for nil errors.
-require.Nil(t, err) +require.NoError(t, err)go/apps/api/routes/v2_identities_delete_identity/403_test.go (1)
44-67: Test function name doesn't match expected status code.The test name "delete identity from other workspace" is under TestWorkspacePermissions but it's actually testing for a 404 Not Found response, not a 403 Forbidden as might be expected from the function name.
Either move this test to TestNotFound in 404_test.go or consider renaming the test function to better reflect its purpose.
go/apps/api/routes/v2_identities_delete_identity/200_test.go (2)
124-148: Duplicate test name but different test cases.There are two test cases with the same name "delete identity via identityID", but they test different functionality - one uses identity ID and the other uses external ID.
Rename the second test case to accurately reflect what it's testing:
-t.Run("delete identity via identityID", func(t *testing.T) { +t.Run("delete identity via externalId", func(t *testing.T) {
64-165: Consider adding audit log verification.The tests verify identity and ratelimit deletion, but don't check if audit logs are properly created during the delete operation.
Consider adding verification that audit log entries are created when identities are deleted, ensuring the audit trail is properly maintained.
go/apps/api/routes/v2_identities_delete_identity/handler.go (1)
118-124: Avoid a second allocation when collecting ratelimit IDsYou already know
len(ratelimits); initialise the slice with that capacity to prevent an extra growth step:-ids := make([]string, 0) -for _, ratelimit := range ratelimits { - ids = append(ids, ratelimit.ID) -} +ids := make([]string, len(ratelimits)) +for i, rl := range ratelimits { + ids[i] = rl.ID +}This is a tiny optimisation, but it’s essentially free to make.
go/apps/api/openapi/gen.go (1)
455-511: (Minor) Double marshal/unmarshal incurs avoidable overhead
MarshalJSONfirst convertst.unionto bytes, then re‑parses those
bytes into theobjectmap, only to marshal the full map again at the
end. For larger payloads this allocates three times:
t.union.MarshalJSON()json.Unmarshal(..)intoobjectjson.Marshal(object)for the final result.Because this file is generated, changing it manually is not ideal, but
upstreamoapi-codegencould inline step 1 by directly unmarshalling
json.RawMessage(t.union)intoobject, saving one marshal + one
allocation.No action required for correctness; flagging purely for possible
generator improvement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (55)
go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes1_limit100_duration1000_load0_90_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes1_limit100_duration1000_load10_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes1_limit100_duration1000_load2_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes1_limit100_duration3600000_load0_90_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes1_limit100_duration3600000_load10_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes1_limit100_duration3600000_load2_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes1_limit100_duration60000_load0_90_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes1_limit100_duration60000_load10_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes1_limit100_duration60000_load2_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes1_limit5_duration1000_load0_90_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes1_limit5_duration1000_load10_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes1_limit5_duration1000_load2_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes1_limit5_duration3600000_load0_90_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes1_limit5_duration3600000_load10_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes1_limit5_duration3600000_load2_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes1_limit5_duration60000_load0_90_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes1_limit5_duration60000_load10_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes1_limit5_duration60000_load2_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes3_limit100_duration1000_load0_90_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes3_limit100_duration1000_load10_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes3_limit100_duration1000_load2_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes3_limit100_duration3600000_load0_90_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes3_limit100_duration3600000_load10_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes3_limit100_duration3600000_load2_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes3_limit100_duration60000_load0_90_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes3_limit100_duration60000_load10_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes3_limit100_duration60000_load2_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes3_limit5_duration1000_load0_90_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes3_limit5_duration1000_load10_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes3_limit5_duration1000_load2_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes3_limit5_duration3600000_load0_90_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes3_limit5_duration3600000_load10_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes3_limit5_duration3600000_load2_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes3_limit5_duration60000_load0_90_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes3_limit5_duration60000_load10_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes3_limit5_duration60000_load2_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes9_limit100_duration1000_load0_90_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes9_limit100_duration1000_load10_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes9_limit100_duration1000_load2_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes9_limit100_duration3600000_load0_90_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes9_limit100_duration3600000_load10_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes9_limit100_duration3600000_load2_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes9_limit100_duration60000_load0_90_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes9_limit100_duration60000_load10_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes9_limit100_duration60000_load2_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes9_limit5_duration1000_load0_90_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes9_limit5_duration1000_load10_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes9_limit5_duration1000_load2_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes9_limit5_duration3600000_load0_90_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes9_limit5_duration3600000_load10_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes9_limit5_duration3600000_load2_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes9_limit5_duration60000_load0_90_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes9_limit5_duration60000_load10_00_windows10/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_ratelimiting/generated/ratelimit_nodes9_limit5_duration60000_load2_00_windows10/generated_test.gois excluded by!**/generated/**go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (63)
go/apps/api/openapi/gen.go(4 hunks)go/apps/api/openapi/openapi.json(3 hunks)go/apps/api/routes/register.go(2 hunks)go/apps/api/routes/v2_apis_create_api/401_test.go(0 hunks)go/apps/api/routes/v2_identities_delete_identity/200_test.go(1 hunks)go/apps/api/routes/v2_identities_delete_identity/400_test.go(1 hunks)go/apps/api/routes/v2_identities_delete_identity/401_test.go(1 hunks)go/apps/api/routes/v2_identities_delete_identity/403_test.go(1 hunks)go/apps/api/routes/v2_identities_delete_identity/404_test.go(1 hunks)go/apps/api/routes/v2_identities_delete_identity/handler.go(1 hunks)go/apps/api/routes/v2_ratelimit_get_override/handler.go(2 hunks)go/go.mod(2 hunks)go/internal/services/auditlogs/insert.go(1 hunks)go/pkg/codes/constants_gen.go(4 hunks)go/pkg/db/api_find_by_id.sql_generated.go(1 hunks)go/pkg/db/api_insert.sql_generated.go(1 hunks)go/pkg/db/audit_log_bucket_id_find_by_workspace_and_name.sql_generated.go(0 hunks)go/pkg/db/audit_log_find_target_by_id.sql_generated.go(1 hunks)go/pkg/db/audit_log_insert.sql_generated.go(1 hunks)go/pkg/db/audit_log_target_insert.sql_generated.go(1 hunks)go/pkg/db/identity_delete.sql_generated.go(1 hunks)go/pkg/db/identity_find_by_external_id.sql_generated.go(1 hunks)go/pkg/db/identity_find_by_id.sql_generated.go(1 hunks)go/pkg/db/identity_find_ratelimits_by_id.sql_generated.go(1 hunks)go/pkg/db/identity_insert.sql_generated.go(1 hunks)go/pkg/db/identity_insert_ratelimit.sql_generated.go(1 hunks)go/pkg/db/key_find_by_hash.sql_generated.go(1 hunks)go/pkg/db/key_find_by_id.sql_generated.go(1 hunks)go/pkg/db/key_find_for_verification.sql_generated.go(1 hunks)go/pkg/db/key_insert.sql_generated.go(1 hunks)go/pkg/db/keyring_find_by_id.sql_generated.go(1 hunks)go/pkg/db/keyring_insert.sql_generated.go(1 hunks)go/pkg/db/models_generated.go(1 hunks)go/pkg/db/permission_find_by_workspace_and_name.sql_generated.go(1 hunks)go/pkg/db/permission_insert.sql_generated.go(1 hunks)go/pkg/db/permission_insert_key_permission.sql_generated.go(1 hunks)go/pkg/db/permissions_by_key_id.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_by_external_id.sql(1 hunks)go/pkg/db/queries/identity_find_by_id.sql(1 hunks)go/pkg/db/queries/ratelimit_delete_many.sql(1 hunks)go/pkg/db/ratelimit_delete_many.sql_generated.go(1 hunks)go/pkg/db/ratelimit_namespace_delete.sql_generated.go(1 hunks)go/pkg/db/ratelimit_namespace_find_by_id.sql_generated.go(1 hunks)go/pkg/db/ratelimit_namespace_find_by_name.sql_generated.go(1 hunks)go/pkg/db/ratelimit_namespace_insert.sql_generated.go(1 hunks)go/pkg/db/ratelimit_namespace_soft_delete.sql_generated.go(1 hunks)go/pkg/db/ratelimit_override_find_by_id.sql_generated.go(1 hunks)go/pkg/db/ratelimit_override_find_by_identifier.sql_generated.go(1 hunks)go/pkg/db/ratelimit_override_find_matches.sql_generated.go(1 hunks)go/pkg/db/ratelimit_override_insert.sql_generated.go(1 hunks)go/pkg/db/ratelimit_override_list_by_namespace_id.sql_generated.go(1 hunks)go/pkg/db/ratelimit_override_soft_delete.sql_generated.go(1 hunks)go/pkg/db/ratelimit_override_update.sql_generated.go(1 hunks)go/pkg/db/workspace_find_by_id.sql_generated.go(1 hunks)go/pkg/db/workspace_hard_delete.sql_generated.go(1 hunks)go/pkg/db/workspace_insert.sql_generated.go(1 hunks)go/pkg/db/workspace_soft_delete.sql_generated.go(1 hunks)go/pkg/db/workspace_update_enabled.sql_generated.go(1 hunks)go/pkg/db/workspace_update_plan.sql_generated.go(1 hunks)go/pkg/db/workspaces_list.sql_generated.go(1 hunks)internal/db/src/schema/identity.ts(2 hunks)
💤 Files with no reviewable changes (2)
- go/apps/api/routes/v2_apis_create_api/401_test.go
- go/pkg/db/audit_log_bucket_id_find_by_workspace_and_name.sql_generated.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
go/pkg/codes/constants_gen.go (1)
go/pkg/urn/urn.go (1)
URN(12-19)
go/apps/api/routes/v2_ratelimit_get_override/handler.go (3)
go/pkg/fault/wrap.go (2)
Wrap(20-37)WithDesc(75-91)go/pkg/fault/code.go (1)
WithCode(37-51)go/pkg/codes/unkey_application.go (1)
App(45-59)
🔇 Additional comments (79)
go/pkg/db/keyring_insert.sql_generated.go (1)
3-3: Generated code version bump is correct
The sqlc tool version in the header comment has been updated to v1.28.0. As this is a non-functional change in a generated file, no further review is required.go/pkg/db/ratelimit_override_update.sql_generated.go (1)
3-3: Skip review for generated file.
This update only bumps the sqlc version comment from v1.27.0 to v1.28.0 in an auto‑generated file. No manual changes are needed.go/pkg/db/permission_insert_key_permission.sql_generated.go (1)
3-3: Approved: Updated sqlc codegen version.This generated file’s header comment has been bumped from
sqlc v1.27.0tosqlc v1.28.0. There are no functional or logical changes, and the SQL and method implementation remain unchanged.go/pkg/db/workspace_soft_delete.sql_generated.go (1)
3-3: Bump sqlc tool version in generated code.The version comment has been updated from
sqlc v1.27.0tosqlc v1.28.0. This is a non-functional change reflecting the upgraded code generation tool.go/pkg/db/ratelimit_override_list_by_namespace_id.sql_generated.go (1)
3-3: Update code generation tool version comment
The auto‐generated header has been bumped fromsqlc v1.27.0tosqlc v1.28.0. This change is expected when regenerating with the latestsqlcand has no impact on functionality.go/pkg/db/ratelimit_override_find_by_identifier.sql_generated.go (1)
3-3: Auto-generated version upgrade
The sqlc generation version comment has been updated from v1.27.0 to v1.28.0. There are no functional changes to the query or code.go/pkg/db/key_find_for_verification.sql_generated.go (1)
3-3: Upgrade sqlc generator version
The code generation header has been updated fromsqlc v1.27.0tosqlc v1.28.0. This is a non-functional change and aligns with other regenerated files in the PR.go/pkg/db/ratelimit_override_find_matches.sql_generated.go (1)
3-3: Generated code version bump is correct.This file is auto-generated by sqlc, and the updated version comment to
v1.28.0matches the other generated artifacts in this PR. No manual changes needed.go/pkg/db/keyring_find_by_id.sql_generated.go (1)
3-3: Approved: bump SQLC generator version only
The comment was updated tosqlc v1.28.0without any functional changes to the query or method.go/pkg/db/workspace_update_enabled.sql_generated.go (1)
3-3: Approved: bump SQLC generator version only
The version comment now readssqlc v1.28.0. No modifications to the SQL or Go code logic.go/pkg/db/ratelimit_override_insert.sql_generated.go (1)
3-3: Approved: bump SQLC generator version only
Only the tool version comment was updated tosqlc v1.28.0. The SQL and generated code remain unchanged.go/pkg/db/ratelimit_namespace_insert.sql_generated.go (1)
3-3: Approved: bump SQLC generator version only
The code generator version comment has been refreshed tosqlc v1.28.0, with no impact on functionality.go/pkg/db/ratelimit_override_find_by_id.sql_generated.go (1)
3-3: Approved: bump SQLC generator version only
This update only reflects the upgrade tosqlc v1.28.0in the generated header. Everything else is unchanged.go/pkg/db/ratelimit_override_soft_delete.sql_generated.go (1)
3-3: Bump sqlc code generation version
The version comment was updated fromsqlc v1.27.0tosqlc v1.28.0. This is expected in generated code and has no impact on functionality.go/internal/services/auditlogs/insert.go (1)
60-60: Remove extraneous blank line
The blank line after thejson.Marshal(l.ActorMeta)error check was removed for concise formatting. This whitespace-only change does not affect control flow or error handling.go/pkg/db/models_generated.go (1)
3-3: Update sqlc tool version comment
The generated models file’s header now reflectssqlc v1.28.0instead of v1.27.0. No functional code was altered.go/pkg/db/ratelimit_namespace_find_by_id.sql_generated.go (1)
3-3: Upgrade sqlc version annotation
The code generation version was bumped tosqlc v1.28.0. There are no changes to the SQL query or its implementation.go/pkg/db/api_find_by_id.sql_generated.go (1)
3-3: Refresh sqlc version header
The generated file comment now indicatessqlc v1.28.0. This version bump is standard for regeneration and does not alter behavior.go/pkg/db/workspace_hard_delete.sql_generated.go (1)
3-3: Generated code version bump only
This change updates the sqlc tool version comment to v1.28.0. There are no functional or structural changes in this generated file.go/pkg/db/identity_insert.sql_generated.go (1)
3-3: Generated code version bump only
This update adjusts the sqlc version comment to v1.28.0. No logic, query, or signature changes were introduced.go/pkg/db/identity_insert_ratelimit.sql_generated.go (1)
3-3: Generated code version bump only
The only modification is the sqlc version comment upgrade to v1.28.0. No other code was altered.go/pkg/db/ratelimit_namespace_delete.sql_generated.go (1)
3-3: Generated code version bump only
This file’s change is solely the sqlc version comment update to v1.28.0. Functionality remains unchanged.go/pkg/db/permission_find_by_workspace_and_name.sql_generated.go (1)
3-3: Generated code version bump only
Only the sqlc version comment was updated to v1.28.0; the query, types, and methods are intact.go/pkg/db/permission_insert.sql_generated.go (1)
3-3: SQLC tool version bump is expected
The version comment has been updated fromsqlc v1.27.0tosqlc v1.28.0, matching the regeneration of all SQLC‐generated files in this PR. No functional or signature changes were introduced.go/pkg/db/ratelimit_namespace_soft_delete.sql_generated.go (1)
3-3: SQLC tool version bump is expected
The version comment has been updated fromsqlc v1.27.0tosqlc v1.28.0, aligning with the rest of the regenerated SQLC files. No logic or API surface changes occurred.go/pkg/db/ratelimit_namespace_find_by_name.sql_generated.go (1)
3-3: SQLC tool version bump is expected
The only change is the version comment update tosqlc v1.28.0. The SQL query and generated code remain unchanged.go/pkg/db/workspace_insert.sql_generated.go (1)
3-3: SQLC tool version bump is expected
Updated the codegen comment tosqlc v1.28.0with no modifications to the query or function signature.go/pkg/db/audit_log_find_target_by_id.sql_generated.go (1)
3-3: SQLC tool version bump is expected
The only change is bumping the version comment tosqlc v1.28.0. All query logic and types remain identical.go/pkg/db/key_find_by_hash.sql_generated.go (1)
3-3: Update sqlc Code Generation Version
The generated sqlc version comment has been bumped tosqlc v1.28.0, aligning with the other DB query files. No functional changes detected.go/pkg/db/audit_log_target_insert.sql_generated.go (1)
3-3: Bump sqlc Tool Version
The sqlc version in the generated comment is updated tov1.28.0. This is a non-functional, consistent version bump.go/pkg/db/workspace_update_plan.sql_generated.go (1)
3-3: sqlc Version Upgrade
The code generation header now reflectssqlc v1.28.0, matching the rest of the generated DB code. No logic changes.go/pkg/db/api_insert.sql_generated.go (1)
3-3: Consistent sqlc Version Update
Updated the code generator version tosqlc v1.28.0. No alterations to SQL or function behavior.go/pkg/db/key_find_by_id.sql_generated.go (1)
3-3: Upgrade sqlc Version Comment
The sqlc comment is nowv1.28.0for consistency; there are no functional changes.go/pkg/db/workspaces_list.sql_generated.go (1)
3-3: Generated code version bump only
This file is auto‑generated by sqlc; the change solely updates the version comment fromv1.27.0tov1.28.0. No functional or behavioral modifications were introduced.go/pkg/db/identity_find_ratelimits_by_id.sql_generated.go (1)
3-3: Generated code version bump only
Only the sqlc version marker was updated tov1.28.0. The SQL query, data structures, and method logic remain unchanged.go/pkg/db/permissions_by_key_id.sql_generated.go (1)
3-3: Generated code version bump only
This update refreshes the sqlc tool version comment tov1.28.0. There are no modifications to the SQL, parameters, or method implementation.go/pkg/db/key_insert.sql_generated.go (1)
3-3: Generated code version bump only
The only change is the sqlc version comment upgrade tov1.28.0; theInsertKeyquery and its parameters are unchanged.go/pkg/db/workspace_find_by_id.sql_generated.go (1)
3-3: Generated code version bump only
The sqlc version marker has been updated tov1.28.0. No alterations were made to the SQL statement or generated Go code logic.go/pkg/db/audit_log_insert.sql_generated.go (1)
3-3: Approve: Updated sqlc code generation version
The sqlc version comment has been bumped from the previous version tov1.28.0, aligning with the upgraded sqlc code generation across the PR.go/go.mod (2)
13-13: Approve: Added github.com/oapi-codegen/runtime v1.1.1 dependency
This dependency is required for the new polymorphic JSON handling in the generated OpenAPI models.
48-48: Approve: Added indirect github.com/apapsch/go-jsonmerge/v2 v2.0.0 dependency
This indirect module supports JSON merge functionality used in the new API model logic.go/pkg/db/queries/ratelimit_delete_many.sql (1)
1-2: Approve: Introduce DeleteManyRatelimitsByIDs exec query
Adds a batch delete for rate limits viasqlc.slice(ids), enabling cleanup of associated rate limits in the identity deletion flow.go/pkg/db/queries/identity_delete.sql (1)
1-2: Approve: Introduce DeleteIdentity exec query
Defines theDeleteIdentitySQL exec query to remove an identity record by its ID within the transaction.go/pkg/db/queries/identity_find_by_external_id.sql (1)
1-2: Approve: Introduce FindIdentityByExternalID query
Adds a select query to retrieve an identity byworkspace_idandexternal_id, supporting the delete-by-external-ID endpoint.go/pkg/codes/constants_gen.go (2)
2-2: Generate timestamp updated.The timestamp has been updated to reflect the latest code generation time.
11-13: Comment alignment improved.The indentation has been removed from section header comments, aligning them to the left margin for better readability and consistency.
Also applies to: 15-15, 24-24, 36-38, 40-40, 45-45, 50-50, 55-55, 60-60, 65-65, 70-70, 75-75, 80-80, 87-87, 92-94, 96-96, 103-103, 110-110
go/pkg/db/queries/identity_find_by_id.sql (1)
2-2: Column order adjusted in SQL query.The query now includes the
idcolumn as the first selected field and movesmetato the end. This change likely aligns with the updated structure in the generated Go code for better consistency.go/apps/api/routes/register.go (2)
13-13: New import added for identity deletion handler.The import for the new
v2IdentitiesDeleteIdentitypackage is correctly added.
107-117: New route registered for identity deletion.The route registration for the new
/v2/identities.deleteIdentityendpoint is properly implemented with the standard default middlewares and required services (Logger, Database, Keys, Permissions, and Auditlogs).go/apps/api/routes/v2_ratelimit_get_override/handler.go (3)
53-56: Improved error handling for namespace lookup failures.Error handling has been enhanced by wrapping database errors with more specific error codes and descriptive messages. This change adds the
ServiceUnavailableerror code and provides both internal and user-facing error descriptions.
108-111: Improved error handling for override lookup failures.Similar to the namespace lookup error handling, this change enhances the override lookup error handling with specific error codes and descriptive messages, providing better context for troubleshooting and user feedback.
113-113: Added empty line for better code readability.A blank line has been added to improve code readability and maintain consistent spacing in the codebase.
internal/db/src/schema/identity.ts (1)
22-25: Index optimization looks good!The index has been improved by changing the order from
external_id_workspace_id_idxtoworkspace_id_external_id_idxwhich is a better approach for multi-tenant systems. This ordering optimizes queries that filter by workspaceId first (which is the typical access pattern), and then by externalId.go/pkg/db/identity_delete.sql_generated.go (1)
12-22: Implementation looks correct for identity deletionThe generated SQL query and implementation follow standard patterns for deletion operations. This will be used in the identity deletion endpoint to remove identities by their ID.
go/pkg/db/ratelimit_delete_many.sql_generated.go (1)
20-33: Bulk deletion implementation is secure and efficientThe implementation for deleting multiple rate limits is well-structured:
- Dynamically constructs the SQL query based on the number of IDs
- Safely handles empty ID slices
- Properly parameterizes the query to prevent SQL injection
This will efficiently delete associated rate limits when deleting an identity.
go/pkg/db/identity_find_by_external_id.sql_generated.go (1)
1-37: LGTM: Generated code for finding identity by external ID looks correct.The SQL query and Go code for finding an identity by external ID are correctly implemented. The function takes appropriate parameters (workspace_id and external_id) and returns the full Identity struct.
go/pkg/db/identity_find_by_id.sql_generated.go (1)
13-29: LGTM: Updated method signature properly returns full Identity struct.The changes appropriately update the query to select all identity fields including the ID and modify the method signature to return the full Identity struct instead of a specialized row type. This aligns with the newly added FindIdentityByExternalID method for consistency.
go/apps/api/routes/v2_identities_delete_identity/400_test.go (4)
36-49: LGTM: Well-structured test for missing both identity identifiers.The test correctly verifies the 400 response when neither externalId nor identityId is provided, with appropriate assertions on the error response structure.
51-64: LGTM: Properly testing empty external ID validation.The test verifies that an empty string for externalId is rejected with a 400 response.
81-91: LGTM: Missing authorization header test case is comprehensive.The test correctly verifies the 400 response when the authorization header is missing.
93-103: LGTM: Malformed authorization header test case is comprehensive.The test correctly verifies the 400 response when the authorization header is malformed.
go/apps/api/openapi/openapi.json (3)
572-600: LGTM: Well-defined request and response schemas for the delete identity endpoint.The schemas for delete identity request and response are well-defined:
- Request schema properly uses
anyOfto require either externalId or identityId- Both fields have appropriate min length validation
- Descriptions clearly explain the purpose of each field
- Empty response schema follows the pattern for delete operations
675-681: LGTM: Formatting improvement for required fields array.The reformatting of the required fields array for RatelimitOverride improves readability without changing functionality.
1190-1273: LGTM: Well-defined API endpoint for delete identity operation.The API endpoint definition is comprehensive with:
- Proper security requirements using the rootKey scheme
- Correct request and response body references
- All appropriate error response status codes (400, 401, 403, 404, 500)
- Consistent response content type for errors (application/problem+json)
- Path and operation ID following the established naming patterns
go/pkg/db/querier_generated.go (4)
13-16: LGTM! Clear and descriptive DB interface method for identity deletion.The SQL query documentation and interface method signature accurately represent the identity deletion functionality.
17-20: LGTM! Well-designed batch deletion method for ratelimits.The method appropriately handles deletion of multiple ratelimits by their IDs, which will be useful for cleaning up associated resources when deleting identities.
38-41: LGTM! New method for finding identity by external ID is correctly implemented.This method allows for polymorphic identity lookup, supporting the ability to delete identities by external ID.
42-45: LGTM! Updated return type for FindIdentityByID.The method now returns an Identity type instead of FindIdentityByIDRow, providing consistency with the new FindIdentityByExternalID method.
go/apps/api/routes/v2_identities_delete_identity/404_test.go (3)
18-30: LGTM! Test setup and handler registration looks good.The test harness and route registration provide a solid foundation for testing the API endpoint behavior.
44-67: LGTM! Good workspace isolation test case.This test properly verifies that attempts to delete identities across workspace boundaries return the appropriate 404 status code, maintaining proper isolation.
69-80: LGTM! Good test for non-existent identity.This test case correctly verifies that attempting to delete a non-existent identity returns a 404 status code.
go/apps/api/routes/v2_identities_delete_identity/403_test.go (2)
18-30: LGTM! Test setup and handler registration looks good.The test harness and route registration provide a solid foundation for testing the API endpoint behavior.
31-42: LGTM! Good test for insufficient permissions.This test case properly verifies that attempts to delete identities without the required permissions return a 403 Forbidden status code.
go/apps/api/routes/v2_identities_delete_identity/200_test.go (5)
19-24: LGTM! Good struct definition for test data.This Identity struct provides a clear and concise representation of the test data structure.
25-62: LGTM! Well-structured helper function.The newIdentity helper function is well-documented and properly handles the creation of test identities with configurable ratelimits.
85-96: LGTM! Good unit test for direct database deletion.This test effectively verifies the basic identity deletion functionality at the database level.
99-122: LGTM! Comprehensive test for ratelimit cleanup.This test thoroughly verifies that both identities and their associated ratelimits can be properly deleted from the database.
150-164: LGTM! Comprehensive test for API deletion with ratelimits.This test thoroughly verifies that both identities and their associated ratelimits are properly deleted when using the API endpoint.
go/apps/api/openapi/gen.go (1)
6-11: Dependency looks fine, but verify go.mod alignmentThe new
github.com/oapi-codegen/runtimeimport is required for the JSON‑merge helpers that follow. As long asgo.modalready pins the exact version that was used during generation, nothing else is needed here.If you have not run
go mod tidyafter code‑generation, please do so to ensure the indirect dependency graph is clean.
| }) | ||
|
|
||
| // Create a identity via DB and delete it again | ||
| t.Run("delete identity ratelimits via db", func(t *testing.T) { |
There was a problem hiding this comment.
this test doesn't really fit here, I think
we're testing the api route in this package, not db functionality.
the test is good though, should we move it into a different package?
There was a problem hiding this comment.
Yeah one package for db tests make sense to me
|
This test from |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
go/apps/api/routes/v2_identities_delete_identity/handler.go (3)
34-226:⚠️ Potential issueCross-workspace lookup by raw identity ID may leak information.
When using
FindIdentityByID, the query isn't scoped to the workspace ID, which could potentially leak information across workspaces and bypass DB-level tenant isolation.Although you check for workspace mismatch after retrieval (lines 98-103), it's better to implement a workspace-scoped query at the database level:
- return db.Query.FindIdentityByID(ctx, svc.DB.RO(), db.FindIdentityByIDParams{ - ID: *req.IdentityId, - Deleted: false, - }) + // Create a query that includes workspace_id filter + identity, err := db.Query.FindIdentityByIDAndWorkspace(ctx, svc.DB.RO(), db.FindIdentityByIDAndWorkspaceParams{ + ID: *req.IdentityId, + WorkspaceID: workspaceID, + Deleted: false, + })Consider adding
FindIdentityByIDAndWorkspacequery to ensure proper multi-tenancy at the database level.
261-280:⚠️ Potential issueRead operations happen outside the running transaction.
The
getIdentityfunction usessvc.DB.RO()instead of the transaction handle, which breaks transaction isolation and could lead to race conditions or inconsistent reads.Refactor to pass the transaction handle to
getIdentity:-func getIdentity(ctx context.Context, svc Services, req Request, workspaceID string) (db.Identity, error) { +func getIdentity(ctx context.Context, dbHandle db.DBTX, req Request, workspaceID string) (db.Identity, error) { switch { case req.IdentityId != nil: - return db.Query.FindIdentityByID(ctx, svc.DB.RO(), db.FindIdentityByIDParams{ + return db.Query.FindIdentityByID(ctx, dbHandle, db.FindIdentityByIDParams{ ID: *req.IdentityId, Deleted: false, }) case req.ExternalId != nil: - return db.Query.FindIdentityByExternalID(ctx, svc.DB.RO(), db.FindIdentityByExternalIDParams{ + return db.Query.FindIdentityByExternalID(ctx, dbHandle, db.FindIdentityByExternalIDParams{ WorkspaceID: workspaceID, ExternalID: *req.ExternalId, Deleted: false, }) } // ... }Then in the handler:
- identity, err := getIdentity(ctx, svc, req, auth.AuthorizedWorkspaceID) + // Get identity inside transaction for consistency + identity, err := getIdentity(ctx, tx, req, auth.AuthorizedWorkspaceID)
170-176: 🛠️ Refactor suggestionRead for ratelimits should also occur within the transaction.
Similar to the
getIdentityissue, reading ratelimits should occur within the same transaction for consistency.- ratelimits, err := db.Query.FindRatelimitsByIdentityID(ctx, tx, sql.NullString{String: identity.ID, Valid: true}) + ratelimits, err := db.Query.FindRatelimitsByIdentityID(ctx, tx, sql.NullString{String: identity.ID, Valid: true})Ensure this uses
txconsistently (it already appears to be using tx here, which is good - just confirming this is correct).go/apps/api/openapi/gen.go (1)
233-244:⚠️ Potential issueExactly‑one identifier rule still unenforced
Both
ExternalIdandIdentityIdare optional pointers; callers can send zero or both fields, making the target of the delete ambiguous. This is the same issue raised earlier. Runtime guard or aoneOfschema constraint is required.if (req.ExternalId == nil) == (req.IdentityId == nil) { return badRequest("exactly one of externalId or identityId must be provided") }Better: encode this in the OpenAPI spec with
oneOf: - required: ["externalId"] - required: ["identityId"]so codegen enforces it automatically.
Would you like a spec patch PR snippet?
🧹 Nitpick comments (6)
go/apps/api/routes/v2_identities_delete_identity/handler.go (3)
138-143: Code duplication in error handling.The error handling for the second soft delete operation duplicates the logic from the first attempt.
Consider extracting this to a helper function:
+ func handleSoftDeleteError(err error) error { + if err != nil && !db.IsDuplicateKeyError(err) { + return fault.Wrap(err, + fault.WithCode(codes.App.Internal.ServiceUnavailable.URN()), + fault.WithDesc("database failed to soft delete identity", "Failed to delete Identity."), + ) + } + return nil + } err = db.Query.SoftDeleteIdentity(ctx, tx, identity.ID) - if err != nil && !db.IsDuplicateKeyError(err) { - return fault.Wrap(err, - fault.WithCode(codes.App.Internal.ServiceUnavailable.URN()), - fault.WithDesc("database failed to soft delete identity", "Failed to delete Identity."), - ) - } + if err := handleSoftDeleteError(err); err != nil { + return err + } // If we hit a duplicate key error, we know that we have an identity that was already soft deleted // so we can hard delete the "old" deleted version if err != nil && db.IsDuplicateKeyError(err) { // error handling for duplicate key... // Re-apply the soft delete operation err = db.Query.SoftDeleteIdentity(ctx, tx, identity.ID) - if err != nil && !db.IsDuplicateKeyError(err) { - return fault.Wrap(err, - fault.WithCode(codes.App.Internal.ServiceUnavailable.URN()), - fault.WithDesc("database failed to soft delete identity", "Failed to delete Identity."), - ) - } + if err := handleSoftDeleteError(err); err != nil { + return err + } }
146-207: Consider factoring out audit log creation.The audit log creation logic is quite verbose and could be refactored for better maintainability, especially for the ratelimit audit logs.
Consider creating helper functions to construct the audit logs:
+ func createIdentityDeleteAuditLog(identity db.Identity, auth keys.AuthVerificationResult, s *zen.Session) auditlog.AuditLog { + return auditlog.AuditLog{ + WorkspaceID: auth.AuthorizedWorkspaceID, + Event: auditlog.IdentityDeleteEvent, + Display: fmt.Sprintf("Deleted identity %s.", identity.ID), + Bucket: auditlogs.DEFAULT_BUCKET, + ActorID: auth.KeyID, + ActorType: auditlog.RootKeyActor, + ActorMeta: nil, + ActorName: "root key", + RemoteIP: s.Location(), + UserAgent: s.UserAgent(), + Resources: []auditlog.AuditLogResource{ + { + ID: identity.ID, + Meta: nil, + Type: auditlog.IdentityResourceType, + DisplayName: identity.ExternalID, + Name: identity.ExternalID, + }, + }, + } + } + + func createRatelimitDeleteAuditLog(identity db.Identity, rl db.FindRatelimitsByIdentityIDRow, + auth keys.AuthVerificationResult, s *zen.Session) auditlog.AuditLog { + return auditlog.AuditLog{ + // similar construction for ratelimit audit log + } + } - auditLogs := []auditlog.AuditLog{ - { - WorkspaceID: auth.AuthorizedWorkspaceID, - Event: auditlog.IdentityDeleteEvent, - Display: fmt.Sprintf("Deleted identity %s.", identity.ID), - // ... rest of the audit log construction - }, - } + auditLogs := []auditlog.AuditLog{ + createIdentityDeleteAuditLog(identity, auth, s), + } // ... ratelimit fetching logic for _, rl := range ratelimits { - auditLogs = append(auditLogs, auditlog.AuditLog{ - // ... verbose audit log construction - }) + auditLogs = append(auditLogs, createRatelimitDeleteAuditLog(identity, rl, auth, s)) }
229-259: Potential for a simpler database operation for deleteOldIdentity.The
deleteOldIdentityfunction has sequential reads and deletes that could potentially be combined into fewer database operations.Consider creating a more atomic database operation:
-- A single stored procedure or transaction could handle this more efficiently DELETE FROM ratelimits WHERE identity_id = ( SELECT id FROM identities WHERE workspace_id = ? AND external_id = ? AND deleted = true ); DELETE FROM identities WHERE workspace_id = ? AND external_id = ? AND deleted = true;This would reduce the number of database roundtrips and potentially improve performance.
go/apps/api/openapi/gen.go (2)
221-225: Pointer‑of‑slice adds unnecessary indirection
*[]Ratelimitmeans “a pointer to a slice”. The zero value (nil) of a slice already represents “not provided”, so an extra pointer complicates usage (nil,&[]nil, empty slice) and JSON encoding.- Ratelimits *[]Ratelimit `json:"ratelimits,omitempty"` + Ratelimits []Ratelimit `json:"ratelimits,omitempty"`The generator allows this tweak via
x-oapi-codegen-skip-prune. Removing the double indirection improves ergonomics without changing wire format.
455-512: Marshal/Unmarshal silently preserve unknown union fieldsCurrent implementation copies the raw
unionobject intoobjectand then blindly re‑marshals. Ifunioncontains keys that collide with the explicit fields (externalId,identityId) the explicit fields win – good – but you also resurrect any unknown keys, effectively allowing clients to smuggle arbitrary JSON that will survive round‑trips.If this is unintended, filter
objectto an allow‑list before re‑marshalling or dropunionhandling altogether once the one‑of rule is enforced.go/pkg/db/schema.sql (1)
221-236: Unique index may clash across workspaces
ratelimits.unique_name_idxis(name, key_id, identity_id)but omitsworkspace_id. Two workspaces could not share the same global limit name attached to the same kind of resource even though they are isolated elsewhere.Confirm whether the uniqueness scope should include
workspace_id; if yes:UNIQUE KEY unique_name_idx (workspace_id, name, key_id, identity_id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
go/apps/api/openapi/gen.go(6 hunks)go/apps/api/openapi/openapi.json(4 hunks)go/apps/api/routes/v2_identities_create_identity/200_test.go(7 hunks)go/apps/api/routes/v2_identities_create_identity/400_test.go(3 hunks)go/apps/api/routes/v2_identities_create_identity/handler.go(1 hunks)go/apps/api/routes/v2_identities_delete_identity/200_test.go(1 hunks)go/apps/api/routes/v2_identities_delete_identity/handler.go(1 hunks)go/pkg/codes/constants_gen.go(1 hunks)go/pkg/db/handle_err_duplicate_key.go(1 hunks)go/pkg/db/identity_find_by_external_id.sql_generated.go(1 hunks)go/pkg/db/identity_find_by_id.sql_generated.go(1 hunks)go/pkg/db/identity_soft_delete.sql_generated.go(1 hunks)go/pkg/db/key_find_by_id.sql_generated.go(4 hunks)go/pkg/db/key_find_for_verification.sql_generated.go(4 hunks)go/pkg/db/models_generated.go(2 hunks)go/pkg/db/querier_generated.go(6 hunks)go/pkg/db/queries/identity_find_by_external_id.sql(1 hunks)go/pkg/db/queries/identity_find_by_id.sql(1 hunks)go/pkg/db/queries/identity_soft_delete.sql(1 hunks)go/pkg/db/queries/ratelimit_delete_by_identity_id.sql(1 hunks)go/pkg/db/ratelimit_delete_by_identity_id.sql_generated.go(1 hunks)go/pkg/db/schema.sql(1 hunks)internal/db/src/schema/identity.ts(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- go/pkg/codes/constants_gen.go
- go/pkg/db/queries/ratelimit_delete_by_identity_id.sql
- go/pkg/db/ratelimit_delete_by_identity_id.sql_generated.go
- go/pkg/db/identity_soft_delete.sql_generated.go
🚧 Files skipped from review as they are similar to previous changes (9)
- go/pkg/db/queries/identity_find_by_id.sql
- go/pkg/db/key_find_by_id.sql_generated.go
- go/pkg/db/key_find_for_verification.sql_generated.go
- go/pkg/db/models_generated.go
- internal/db/src/schema/identity.ts
- go/apps/api/routes/v2_identities_delete_identity/200_test.go
- go/apps/api/openapi/openapi.json
- go/pkg/db/identity_find_by_id.sql_generated.go
- go/pkg/db/querier_generated.go
🧰 Additional context used
🧠 Learnings (1)
go/apps/api/openapi/gen.go (1)
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 destructive operations like deletion in the Unkey API, oneOf validation is preferred over anyOf to enforce explicit targeting and prevent ambiguity.
🧬 Code Graph Analysis (4)
go/apps/api/routes/v2_identities_create_identity/handler.go (1)
go/pkg/db/handle_err_duplicate_key.go (1)
IsDuplicateKeyError(7-13)
go/apps/api/routes/v2_identities_create_identity/200_test.go (3)
go/pkg/db/identity_find_by_id.sql_generated.go (1)
FindIdentityByIDParams(16-19)go/pkg/db/models_generated.go (1)
Ratelimit(381-391)go/apps/api/openapi/gen.go (1)
Ratelimit(129-138)
go/apps/api/routes/v2_identities_create_identity/400_test.go (3)
go/apps/api/routes/v2_identities_create_identity/handler.go (1)
MAX_META_LENGTH_MB(40-40)go/pkg/ptr/pointer.go (1)
P(49-51)go/apps/api/openapi/gen.go (1)
Ratelimit(129-138)
go/apps/api/openapi/gen.go (2)
go/pkg/db/models_generated.go (1)
Ratelimit(381-391)packages/ratelimit/src/ratelimit.ts (1)
Ratelimit(121-214)
🔇 Additional comments (11)
go/apps/api/routes/v2_identities_create_identity/handler.go (1)
129-129: Improved error handling via abstractionThe code now uses a centralized utility function to check for duplicate key errors, which decouples the handler from MySQL-specific error types and makes the code more maintainable.
go/pkg/db/queries/identity_soft_delete.sql (1)
1-2: LGTM: Well-structured soft delete queryThe SQL query is correctly implemented to perform a soft delete operation by setting the
deletedflag to 1 for a specific identity ID, using a parameterized query for safety against SQL injection.go/pkg/db/handle_err_duplicate_key.go (1)
7-13: Well-implemented utility function with minor considerationsThis utility function effectively centralizes the logic for detecting MySQL duplicate key errors, improving code maintainability.
A few considerations for future enhancements:
- The type assertion could panic if
erris nil (though it would typically be called with non-nil errors)- The function is MySQL-specific, which is appropriate for the current database but creates a coupling to the MySQL driver
go/apps/api/routes/v2_identities_create_identity/400_test.go (3)
13-13: Good addition of ptr package.The addition of the ptr package makes sense for the subsequent code changes that use the ptr.P helper function.
83-87: Modern type usage and improved code style.Great changes:
- Modernizing to use
map[string]anyinstead ofmap[string]interface{}- Using range-based loop with
ptr.Pfor creating pointers
107-113: Updated Ratelimit type reference.Correctly updated from
[]openapi.V2Ratelimitto[]openapi.Ratelimit, aligning with the API model update that replaced the V2Ratelimit type.go/apps/api/routes/v2_identities_create_identity/200_test.go (2)
55-58: Updated FindIdentityByID calls with soft-delete support.All calls to
FindIdentityByIDhave been properly updated to use the new parameter struct withDeleted: falseto filter out soft-deleted identities.Also applies to: 77-80, 113-116, 136-139, 176-179
125-125: Modernized type usage in test cases.Good updates:
- Changed map types from
map[string]interface{}tomap[string]any- Updated ratelimit slice type from
[]openapi.V2Ratelimitto[]openapi.RatelimitAlso applies to: 143-143, 153-164
go/pkg/db/identity_find_by_external_id.sql_generated.go (3)
12-14: Well-structured query for finding identities by external ID.The SQL query correctly includes filters for workspace ID, external ID, and deleted status, which supports proper multi-tenancy and soft-deletion functionality.
16-20: Comprehensive parameter struct for identity lookups.The parameter struct includes all necessary fields for the query, including the new
Deletedflag that supports soft deletion.
25-39: Well-implemented database query function.The function follows the standard pattern for executing queries and properly handles error conditions.
| -- name: FindIdentityByExternalID :one | ||
| SELECT id, external_id, workspace_id, environment, created_at, updated_at, meta, deleted FROM identities WHERE workspace_id = ? AND external_id = ? AND deleted = ?; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify parameter style consistency across SQL queries
The query correctly retrieves an identity by its external ID with workspace isolation and deletion status, but uses positional parameters (?) while other queries like identity_soft_delete.sql use named parameters (sqlc.arg('id')). Consider standardizing the parameter style across all SQL queries for consistency.
🏁 Script executed:
#!/bin/bash
# Check parameter style consistency across identity-related SQL queries
echo "Checking for SQL parameter styles in identity queries..."
grep -r --include="identity_*.sql" "sqlc.arg" go/pkg/db/queries/
echo "---"
grep -r --include="identity_*.sql" "WHERE.*?" go/pkg/db/queries/Length of output: 2133
Standardize SQLC Parameter Usage in Identity Queries
Consistently use named parameters via sqlc.arg(...) instead of ? placeholders across all identity-related queries:
• go/pkg/db/queries/identity_find_by_external_id.sql
• go/pkg/db/queries/identity_find_by_id.sql
Suggested diffs:
--- a/go/pkg/db/queries/identity_find_by_external_id.sql
+++ b/go/pkg/db/queries/identity_find_by_external_id.sql
@@ -1,2 +1,2 @@
-- name: FindIdentityByExternalID :one
-SELECT id, external_id, workspace_id, environment, created_at, updated_at, meta, deleted
- FROM identities
- WHERE workspace_id = ? AND external_id = ? AND deleted = ?;
+SELECT id, external_id, workspace_id, environment, created_at, updated_at, meta, deleted
+ FROM identities
+ WHERE workspace_id = sqlc.arg('workspace_id')
+ AND external_id = sqlc.arg('external_id')
+ AND deleted = sqlc.arg('deleted');--- a/go/pkg/db/queries/identity_find_by_id.sql
+++ b/go/pkg/db/queries/identity_find_by_id.sql
@@ -1,2 +1,2 @@
-- name: FindIdentityByID :one
-SELECT id, external_id, workspace_id, environment, created_at, updated_at, meta, deleted
- FROM identities
- WHERE id = sqlc.arg(id) AND deleted = ?;
+SELECT id, external_id, workspace_id, environment, created_at, updated_at, meta, deleted
+ FROM identities
+ WHERE id = sqlc.arg('id')
+ AND deleted = sqlc.arg('deleted');This aligns these queries with the rest of the codebase’s SQLC parameter style.
Committable suggestion skipped: line range outside the PR's diff.
| err = db.Query.SoftDeleteIdentity(ctx, tx, identity.ID) | ||
| if err != nil && !db.IsDuplicateKeyError(err) { | ||
| return fault.Wrap(err, | ||
| fault.WithCode(codes.App.Internal.ServiceUnavailable.URN()), | ||
| fault.WithDesc("database failed to soft delete identity", "Failed to delete Identity."), | ||
| ) | ||
| } | ||
|
|
||
| // If we hit a duplicate key error, we know that we have an identity that was already soft deleted | ||
| // so we can hard delete the "old" deleted version | ||
| if err != nil && db.IsDuplicateKeyError(err) { | ||
| err = deleteOldIdentity(ctx, tx, auth.AuthorizedWorkspaceID, identity.ExternalID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Re-apply the soft delete operation | ||
| err = db.Query.SoftDeleteIdentity(ctx, tx, identity.ID) | ||
| if err != nil && !db.IsDuplicateKeyError(err) { | ||
| return fault.Wrap(err, | ||
| fault.WithCode(codes.App.Internal.ServiceUnavailable.URN()), | ||
| fault.WithDesc("database failed to soft delete identity", "Failed to delete Identity."), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential race condition in duplicate key error handling.
The duplicate key error handling logic performs multiple database operations across different statements, which could lead to race conditions if multiple clients are attempting to delete the same identity simultaneously.
Consider enhancing the transaction isolation level or implementing row-level locking to prevent race conditions:
+ // Add an explicit lock on the external ID to prevent race conditions
+ err = db.Query.LockIdentityByExternalID(ctx, tx, db.LockIdentityByExternalIDParams{
+ WorkspaceID: auth.AuthorizedWorkspaceID,
+ ExternalID: identity.ExternalID,
+ })
+ if err != nil {
+ return fault.Wrap(err,
+ fault.WithCode(codes.App.Internal.ServiceUnavailable.URN()),
+ fault.WithDesc("unable to lock identity", "Failed to lock identity for deletion."),
+ )
+ }
err = db.Query.SoftDeleteIdentity(ctx, tx, identity.ID)
if err != nil && !db.IsDuplicateKeyError(err) {
// error handling...
}Alternatively, implement a more atomic deletion operation at the database level that handles this duplicate key case internally.
Committable suggestion skipped: line range outside the PR's diff.
| // Ratelimit defines model for Ratelimit. | ||
| type Ratelimit struct { | ||
| // Duration The duration for each ratelimit window in milliseconds. | ||
| Duration int64 `json:"duration"` | ||
|
|
||
| // Limit How many requests may pass within a given window before requests are rejected. | ||
| Limit int64 `json:"limit"` | ||
|
|
||
| // Name The name of this limit. You will need to use this again when verifying a key. | ||
| Name string `json:"name"` | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Keep JSON & DB numeric widths in sync
Ratelimit.Limit and Ratelimit.Duration are declared as int64, while the DB definition (go/pkg/db/models_generated.go, ratelimits.limit / ratelimits.duration) uses int32 / bigint.
If the API ever returns a value > 2³¹‑1 the insert/update will overflow and be truncated or fail. Consider:
- Duration int64 `json:"duration"`
- Limit int64 `json:"limit"`
+ Duration int64 `json:"duration"` // DB: bigint – OK
+ Limit int32 `json:"limit"` // DB: int32 – prevent overflow mismatchesor enlarge the DB columns to BIGINT to match the API.
Both sides should agree on width to avoid silent data corruption.
I’ll verify the DB model’s field types to confirm whether Limit and Duration align with the OpenAPI struct:
🏁 Script executed:
#!/bin/bash
set -e
echo "1) Locate the generated models file"
fd models_generated.go
echo "2) Inspect the Ratelimit struct in DB models"
rg -n "type Ratelimit" -A5 go/pkg/db/models_generated.go
echo "3) Check the individual field types"
rg -n "Limit" -n go/pkg/db/models_generated.go
rg -n "Duration" -n go/pkg/db/models_generated.goLength of output: 1866
Align numeric widths between API and DB for Ratelimit
In go/apps/api/openapi/gen.go (lines 128–138), the Ratelimit model declares both fields as int64, but in go/pkg/db/models_generated.go the DB struct uses int32 for limit and int64 (BIGINT) for duration. To avoid overflow or silent truncation, update the API model to match the DB types:
// Ratelimit defines model for Ratelimit.
type Ratelimit struct {
// Duration The duration for each ratelimit window in milliseconds.
- Duration int64 `json:"duration"`
+ Duration int64 `json:"duration"` // DB: BIGINT – OK
// Limit How many requests may pass within a given window before requests are rejected.
- Limit int64 `json:"limit"`
+ Limit int32 `json:"limit"` // DB: INTAlternatively, if you need higher limits, migrate the DB limit column to BIGINT. Both sides must agree on bit‑width to prevent data corruption.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Ratelimit defines model for Ratelimit. | |
| type Ratelimit struct { | |
| // Duration The duration for each ratelimit window in milliseconds. | |
| Duration int64 `json:"duration"` | |
| // Limit How many requests may pass within a given window before requests are rejected. | |
| Limit int64 `json:"limit"` | |
| // Name The name of this limit. You will need to use this again when verifying a key. | |
| Name string `json:"name"` | |
| } | |
| // Ratelimit defines model for Ratelimit. | |
| type Ratelimit struct { | |
| // Duration The duration for each ratelimit window in milliseconds. | |
| Duration int64 `json:"duration"` // DB: BIGINT – OK | |
| // Limit How many requests may pass within a given window before requests are rejected. | |
| Limit int32 `json:"limit"` // DB: INT | |
| // Name The name of this limit. You will need to use this again when verifying a key. | |
| Name string `json:"name"` | |
| } |
2 participants
What does this PR do?
Fixes #3152
Also fixes the index for identities
Type of change
How should this be tested?
Tests
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores