feature(request-context): implement request-scoped memoization for optimized DB reads#5990
feature(request-context): implement request-scoped memoization for optimized DB reads#5990victorvhs017 wants to merge 5 commits intomainfrom
Conversation
…ized DB reads - Introduced `RequestMemoizer` to cache results of database calls within a single HTTP request, reducing redundant reads. - Updated various services to utilize memoization for permission checks and data retrieval, enhancing performance. - Added new utility functions for generating memoization keys to ensure consistency across the application. - Refactored request context handling to incorporate memoization seamlessly, improving overall request processing efficiency.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0c0b7521b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…nd improve memoization - Updated permission service to include actorOrgId in memoization keys for more accurate permission checks. - Refactored permission retrieval logic to utilize memoization, reducing redundant database calls. - Improved error handling for project ownership and action type validation to ensure proper access control. - Enhanced request context handling to support new memoization strategies, optimizing performance for permission-related operations.
…ect permissions - Introduced a new variable to determine the actor type for project permissions, enhancing clarity and maintainability. - Updated error messages to reflect the new actor type variable, ensuring consistent feedback for permission-related issues.
akhilmhdh
left a comment
There was a problem hiding this comment.
Overall looking good! Some questions
Pending application testing
| conditionsMatcher | ||
| } | ||
|
|
||
| const permissionData = await permissionDAL.getPermission({ |
There was a problem hiding this comment.
Is permission a big hitter? Generally i feel like this only called once per endpoint
There was a problem hiding this comment.
My reasoning is - permission is a big object, so storing it in context may not be desirable
There was a problem hiding this comment.
I did some search and we have this pattern mostly on secret v3 endpoints and secrets overview, which would be enough reason to use it. But, I also find it good to have as a safeguard, because it's used a lot and we may miss a duplicated call here and there.
We can track the memory usage after the change, if the footprint rises too much, we remove it from here and fix the places we know directly
|
Application testing looking good. Just need to check on the above comments |
…textKey enum - Updated all instances of requestContextKeys to use the new RequestContextKey enum for improved type safety and consistency across the codebase. - Enhanced readability and maintainability by standardizing the way request context keys are accessed throughout the application.
- Changed the error message in the permission service to use a more descriptive key for the ForbiddenRequestError, enhancing clarity in error reporting when project ownership is violated.
Context
Adds request-scoped memoization so repeated read-only work in a single HTTP request (permissions, project/user/identity lookups) hits the database once instead of many times—especially on batch secret paths.
Before: Each call path could repeat
getProjectPermission/getOrgPermission,projectDAL.findById, and actor lookups independently within the same request.After:
RequestMemoizer(getOrSetwith in-flight deduplication) stored on@fastify/request-contextasmemoizer(wired inapp.tsper existing pattern in the branch).requestMemoize(key, fetcher)which uses the memoizer when present and falls back to a direct fetch when there is no request context (e.g. workers).src/lib/request-context/memo-keys.tsand request-context string keys inrequest-context-keys.ts(kept in sync withRequestContextDatainfastify.d.ts).getOrgPermissionwrapped withrequestMemoize;getProjectPermissionusesmemoizer.getOrSetfor full-function memoization (key includesactorOrgId) and defersrequestContext.setforprojectDetails/identityPermissionMetadatauntil after the memoized load completes. DAL reads userequestMemoizeforprojectFindById,userFindById, andidentityFindById.requestContext.get("…")/setstring literals withrequestContextKeysin touched files (logger, audit log, SAML/OIDC telemetry, assume-privilege router, permission service).backend/CLAUDE.md(Request-Scoped Memoization section).request-memoizer.test.tsfor caching, in-flight coalescing, error behavior, and cross-request isolation.Steps to verify the change
debugagainst the dev DB) while exercising a hot path that previously repeated lookups (e.g. batch secret read/list for one project).backend/src/db/instance.tsprojectId, same auth) and compare number ofSELECTstatements (or log lines) before vs after—project load, permission resolution, and actor lookups should collapse to a single round-trip per distinct key within one request.For example:
Type
Checklist
type(scope): short description(scope is optional, e.g.,fix: prevent crash on syncorfix(api): handle null response).Suggested title:
perf(backend): request-scoped memoization for permissions and project lookupsbackend/CLAUDE.mdupdated for memoization