perf: fix n+1 #4484
perf: fix n+1 #4484
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe changes refactor how identities and their ratelimits are fetched and processed. The SQL query now aggregates ratelimits as a JSON field per identity record instead of requiring separate lookups. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
go/apps/api/routes/v2_identities_list_identities/handler.go(2 hunks)go/pkg/db/identity_list.sql_generated.go(4 hunks)go/pkg/db/querier_generated.go(1 hunks)go/pkg/db/queries/identity_list.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: chronark
Repo: unkeyed/unkey PR: 2294
File: apps/api/src/pkg/keys/service.ts:268-271
Timestamp: 2024-10-20T07:05:55.471Z
Learning: In `apps/api/src/pkg/keys/service.ts`, `ratelimitAsync` is a table relation, not a column selection. When querying, ensure that table relations are included appropriately, not as columns.
📚 Learning: 2025-07-16T15:38:53.491Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3606
File: go/pkg/db/replica.go:8-11
Timestamp: 2025-07-16T15:38:53.491Z
Learning: For debugging database replica usage in go/pkg/db/replica.go, it's acceptable to mark QueryRowContext operations as "success" even though SQL errors only surface during row.Scan() calls. The timing metrics are the primary concern for debugging replica performance patterns.
Applied to files:
go/apps/api/routes/v2_identities_list_identities/handler.go
📚 Learning: 2025-07-02T11:51:58.572Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3420
File: go/pkg/hydra/store/gorm/gorm.go:486-498
Timestamp: 2025-07-02T11:51:58.572Z
Learning: The Hydra package (go/pkg/hydra) is planned to be migrated from GORM to sqlc for database operations, which explains why raw SQL queries are acceptable in the current implementation.
Applied to files:
go/apps/api/routes/v2_identities_list_identities/handler.go
📚 Learning: 2025-10-30T15:10:52.743Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Applied to files:
go/pkg/db/queries/identity_list.sql
🧬 Code graph analysis (2)
go/pkg/db/querier_generated.go (2)
go/pkg/db/interface.go (1)
DBTX(29-34)go/pkg/db/identity_list.sql_generated.go (2)
ListIdentitiesParams(45-50)ListIdentitiesRow(52-62)
go/pkg/db/identity_list.sql_generated.go (3)
apps/dashboard/lib/trpc/routers/api/keys/query-api-keys/schema.ts (1)
Ratelimits(33-33)go/pkg/db/queries.go (1)
Queries(3-3)go/pkg/db/interface.go (1)
DBTX(29-34)
⏰ 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 Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Lint Go Code / Lint
- GitHub Check: Build / Build
🔇 Additional comments (6)
go/pkg/db/querier_generated.go (1)
1580-1609: LGTM! Generated interface correctly reflects the optimized query.The updated
ListIdentitiessignature now returns[]ListIdentitiesRowwhich includes the JSON-aggregated ratelimits field, eliminating the N+1 query pattern.go/apps/api/routes/v2_identities_list_identities/handler.go (2)
104-123: LGTM! Graceful error handling for ratelimits unmarshalling.Logging the error and continuing with an empty ratelimits slice is appropriate for a list endpoint - partial data is preferable to failing the entire request. The nil-safe iteration at line 115 handles the error case correctly.
125-140: Clean identity construction with consistent error handling.The metadata unmarshalling follows the same resilient pattern as ratelimits. The direct construction of
openapi.Identityis cleaner than the previous intermediate object approach.go/pkg/db/queries/identity_list.sql (1)
1-30: Good N+1 optimization using correlated JSON subquery.The query correctly uses
COALESCEwithJSON_ARRAY()to handle cases where an identity has no ratelimits, avoiding NULL values in the result set.go/pkg/db/identity_list.sql_generated.go (2)
52-62: Generated struct correctly captures the new row shape.The
Ratelimitsfield asinterface{}is the expected type for JSON columns scanned via database/sql, and the handler appropriately usesdb.UnmarshalNullableJSONToto deserialize it.
95-131: Generated query function correctly implements the optimized query.The function properly scans all columns including the JSON ratelimits field and follows standard error handling patterns.
What does this PR do?
Optimizes the identity listing endpoint by fetching ratelimits in a single SQL query instead of making separate queries for each identity. This change:
ListIdentitiesSQL query to include ratelimits as a JSON array usingJSON_ARRAYAGGandCOALESCEFixes #4148
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin main