Skip to content

perf: fix n+1#4484

Merged
Flo4604 merged 2 commits intomainfrom
perf/list-identeties
Dec 8, 2025
Merged

perf: fix n+1#4484
Flo4604 merged 2 commits intomainfrom
perf/list-identeties

Conversation

@Flo4604
Copy link
Member

@Flo4604 Flo4604 commented Dec 5, 2025

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:

  1. Updates the ListIdentities SQL query to include ratelimits as a JSON array using JSON_ARRAYAGG and COALESCE
  2. Modifies the handler to unmarshal the ratelimits from the JSON response rather than making additional database queries
  3. Simplifies the identity response construction with a more direct approach

Fixes #4148

Type of change

  • Enhancement (small improvements)
  • Chore (refactoring code, technical debt, workflow improvements)

How should this be tested?

  • Test listing identities with the API endpoint and verify that ratelimits are correctly included
  • Verify performance improvement by comparing response times before and after the change
  • Test with identities that have multiple ratelimits to ensure all are properly included

Checklist

Required

  • Filled out the "How to test" section in this PR
  • Read Contributing Guide
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand areas
  • Ran pnpm build
  • Ran pnpm fmt
  • Ran make fmt on /go directory
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues

@changeset-bot
Copy link

changeset-bot bot commented Dec 5, 2025

⚠️ No Changeset found

Latest commit: 65e0b47

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Dec 5, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
dashboard Ready Ready Preview Comment Dec 8, 2025 8:58am
1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
engineering Ignored Ignored Preview Dec 8, 2025 8:58am

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 5, 2025

📝 Walkthrough

Walkthrough

The 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 ListIdentitiesRow type captures this structure, and the API handler deserializes JSON ratelimits in-place rather than performing additional queries.

Changes

Cohort / File(s) Summary
Database query and codegen
go/pkg/db/queries/identity_list.sql, go/pkg/db/identity_list.sql_generated.go, go/pkg/db/querier_generated.go
Changed SQL query from wildcard SELECT to explicit column projection with a JSON-aggregated ratelimits subquery per identity. Introduced new public type ListIdentitiesRow and updated ListIdentities function to return []ListIdentitiesRow instead of []Identity. Updated Querier interface signature accordingly.
API handler
go/apps/api/routes/v2_identities_list_identities/handler.go
Replaced per-identity ratelimits retrieval from SQL queries with JSON unmarshalling from identity.Ratelimits. Unmarshals identity metadata from JSON; logs errors on failure but continues. Removes intermediate object creation and simplifies error handling for ratelimits.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify the SQL aggregation logic for ratelimits (COALESCE subquery correctness)
  • Confirm JSON unmarshalling in handler properly handles edge cases and preserves backward compatibility
  • Validate that all callers of ListIdentities have been updated to work with the new ListIdentitiesRow type
  • Check that error logging in the handler doesn't mask critical deserialization failures

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'perf: fix n+1' clearly refers to the main optimization work of eliminating N+1 database queries for ratelimits in the identity listing endpoint.
Description check ✅ Passed The description provides comprehensive details about the optimization, the changes made, type of change, testing instructions, and all required checklist items are marked as complete.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch perf/list-identeties

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Member Author

Flo4604 commented Dec 5, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@Flo4604 Flo4604 marked this pull request as ready for review December 5, 2025 11:59
@vercel vercel bot temporarily deployed to Preview – engineering December 5, 2025 11:59 Inactive
@vercel vercel bot temporarily deployed to Preview – dashboard December 5, 2025 11:59 Inactive
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0740117 and 31c10e8.

📒 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 ListIdentities signature now returns []ListIdentitiesRow which 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.Identity is 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 COALESCE with JSON_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 Ratelimits field as interface{} is the expected type for JSON columns scanned via database/sql, and the handler appropriately uses db.UnmarshalNullableJSONTo to 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.

Copy link
Contributor

@imeyer imeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@graphite-app
Copy link

graphite-app bot commented Dec 8, 2025

Jim Carrey Thumbs Up GIF (Added via Giphy)

@graphite-app
Copy link

graphite-app bot commented Dec 8, 2025

Graphite Automations

"Post a GIF when PR approved" took an action on this PR • (12/08/25)

1 gif was posted to this PR based on Andreas Thomas's automation.

@vercel vercel bot temporarily deployed to Preview – dashboard December 8, 2025 08:58 Inactive
@Flo4604 Flo4604 merged commit 93c703b into main Dec 8, 2025
20 checks passed
@Flo4604 Flo4604 deleted the perf/list-identeties branch December 8, 2025 09:21
mcstepp pushed a commit that referenced this pull request Dec 9, 2025
Flo4604 added a commit that referenced this pull request Dec 10, 2025
chronark added a commit that referenced this pull request Dec 19, 2025
* feat: add environment variables db schema and queries

* fix db query

* feat: add SecretsConfig proto for encrypted env vars

* [autofix.ci] apply automated fixes

* feat: dashboard UI for environment variables management

* fix comment and rename file

* fix file export name

* Remove unnecessary comments from add-env-vars

* add toasts for environment variable operations

* [autofix.ci] apply automated fixes

* fix: add try/catch error handling to env var mutations

* unfmt file

* [autofix.ci] apply automated fixes

* feat: decrypt env vars in CTRL workflow before passing to Krane

* feat: inject env vars into pod spec via Krane

* feat: add customer-workload service account for pod isolation

* remove gw from k8s manifest, add agent fix ctrl vault for certs

* seperate master keys too

* add inital webhook stuff

* add generated stuff

* adjust comments

* use otel lgtm stack in k8s too

* fix some rabbit comments

* fix some rabbit comments

* get rid of some unncessary comments

* actually add unkey env cmd gitignores...

* fix golint issues

* Fix/update validation issues status label (#4478)

* fix: update API key status label from 'Potential issues' to 'High Error Rate'

Changed the validation-issues status label to more clearly communicate
that the key is receiving invalid requests, rather than implying the
API or key itself is broken.

Changes:
- Label: 'Potential issues' → 'High Error Rate'
- Tooltip: Updated to clarify that requests are invalid (rate limited,
  unauthorized, etc.) rather than suggesting system issues

Fixes #4474

* chore: apply biome formatting

* fix: update status label to 'Elevated Rejections' per review

---------

Co-authored-by: CodeReaper <148160799+MichaelUnkey@users.noreply.github.com>

* chore: Remove un-used UI components (#4472)

* removed un used components

* updated members refs

---------

Co-authored-by: James P <james@unkey.dev>
Co-authored-by: Andreas Thomas <dev@chronark.com>

* perf: fix n+1 (#4484)

* fix: add 403 error when 0 key verification perms (#4483)

* fix: add 403 error when 0 key verification perms

* cleanup tests

* feat: add environment variables db schema and queries (#4450)

* feat: add environment variables db schema and queries

* fix db query

* feat: add SecretsConfig proto for encrypted env vars (#4451)

* feat: add environment variables db schema and queries

* fix db query

* feat: add SecretsConfig proto for encrypted env vars

* [autofix.ci] apply automated fixes

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: Andreas Thomas <dev@chronark.com>

* feat: add GetPullToken

* feat: dashboard UI for environment variables management (#4452)

* feat: add environment variables db schema and queries

* fix db query

* feat: add SecretsConfig proto for encrypted env vars

* [autofix.ci] apply automated fixes

* feat: dashboard UI for environment variables management

* fix comment and rename file

* fix file export name

* Remove unnecessary comments from add-env-vars

* add toasts for environment variable operations

* [autofix.ci] apply automated fixes

* fix: add try/catch error handling to env var mutations

* unfmt file

* [autofix.ci] apply automated fixes

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: Andreas Thomas <dev@chronark.com>

* feat: decrypt env vars in CTRL workflow before passing to Krane (#4453)

* feat: add environment variables db schema and queries

* fix db query

* feat: add SecretsConfig proto for encrypted env vars

* [autofix.ci] apply automated fixes

* feat: dashboard UI for environment variables management

* fix comment and rename file

* fix file export name

* Remove unnecessary comments from add-env-vars

* add toasts for environment variable operations

* [autofix.ci] apply automated fixes

* fix: add try/catch error handling to env var mutations

* unfmt file

* [autofix.ci] apply automated fixes

* feat: decrypt env vars in CTRL workflow before passing to Krane

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: Andreas Thomas <dev@chronark.com>

* feat: inject env vars into pod spec via Krane (#4454)

* feat: add environment variables db schema and queries

* fix db query

* feat: add SecretsConfig proto for encrypted env vars

* [autofix.ci] apply automated fixes

* feat: dashboard UI for environment variables management

* fix comment and rename file

* fix file export name

* Remove unnecessary comments from add-env-vars

* add toasts for environment variable operations

* [autofix.ci] apply automated fixes

* fix: add try/catch error handling to env var mutations

* unfmt file

* [autofix.ci] apply automated fixes

* feat: decrypt env vars in CTRL workflow before passing to Krane

* feat: inject env vars into pod spec via Krane

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: Andreas Thomas <dev@chronark.com>

* feat: add customer-workload service account for pod isolation (#4455)

* feat: add environment variables db schema and queries

* fix db query

* feat: add SecretsConfig proto for encrypted env vars

* [autofix.ci] apply automated fixes

* feat: dashboard UI for environment variables management

* fix comment and rename file

* fix file export name

* Remove unnecessary comments from add-env-vars

* add toasts for environment variable operations

* [autofix.ci] apply automated fixes

* fix: add try/catch error handling to env var mutations

* unfmt file

* [autofix.ci] apply automated fixes

* feat: decrypt env vars in CTRL workflow before passing to Krane

* feat: inject env vars into pod spec via Krane

* feat: add customer-workload service account for pod isolation

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: Andreas Thomas <dev@chronark.com>

* remove gw from k8s manifest, add agent fix ctrl vault for certs (#4463)

* feat: add environment variables db schema and queries

* fix db query

* feat: add SecretsConfig proto for encrypted env vars

* [autofix.ci] apply automated fixes

* feat: dashboard UI for environment variables management

* fix comment and rename file

* fix file export name

* Remove unnecessary comments from add-env-vars

* add toasts for environment variable operations

* [autofix.ci] apply automated fixes

* fix: add try/catch error handling to env var mutations

* unfmt file

* [autofix.ci] apply automated fixes

* feat: decrypt env vars in CTRL workflow before passing to Krane

* feat: inject env vars into pod spec via Krane

* feat: add customer-workload service account for pod isolation

* remove gw from k8s manifest, add agent fix ctrl vault for certs

* seperate master keys too

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: Andreas Thomas <dev@chronark.com>

* chore: Make Stripe Great Again (#4479)

* fix: Make stripe webhooks more robust

* chore: Move alert to UI (#4485)

* Moved alert to ui and swapped usages

* feat: better env var injection (#4468)

* feat: add environment variables db schema and queries

* fix db query

* feat: add SecretsConfig proto for encrypted env vars

* [autofix.ci] apply automated fixes

* feat: dashboard UI for environment variables management

* fix comment and rename file

* fix file export name

* Remove unnecessary comments from add-env-vars

* add toasts for environment variable operations

* [autofix.ci] apply automated fixes

* fix: add try/catch error handling to env var mutations

* unfmt file

* [autofix.ci] apply automated fixes

* feat: decrypt env vars in CTRL workflow before passing to Krane

* feat: inject env vars into pod spec via Krane

* feat: add customer-workload service account for pod isolation

* remove gw from k8s manifest, add agent fix ctrl vault for certs

* seperate master keys too

* add inital webhook stuff

* add generated stuff

* adjust comments

* use otel lgtm stack in k8s too

* fix some rabbit comments

* fix some rabbit comments

* get rid of some unncessary comments

* actually add unkey env cmd gitignores...

* fix golint issues (#4477)

* [autofix.ci] apply automated fixes

* fix fmt

* linter be happy

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: Andreas Thomas <dev@chronark.com>

* make token pod owned

* feat: add lets encrypt challenges (#4471)

* feat: add lets encrypt challenges

* always disable cname following

* cleanup some code

* cleanup some code

* cleanup some code

* cleanup some code

* cleanup some code

* fix golint issues

* fix golint issues

* fmt

* remove old webhook code

* remove old webhook code

* make build id not optiona

* cleanup

* cleanup

* fmt

* fmt

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: abhay <88815641+theabhayprajapati@users.noreply.github.com>
Co-authored-by: CodeReaper <148160799+MichaelUnkey@users.noreply.github.com>
Co-authored-by: James P <james@unkey.dev>
Co-authored-by: Andreas Thomas <dev@chronark.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

List identeties does n+1

3 participants