-
Notifications
You must be signed in to change notification settings - Fork 3.1k
perf: up to 5000% faster permissions calculation #14631
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📦 esbuild Bundle Analysis for payloadThis analysis was generated by esbuild-bundle-analyzer. 🤖
Largest pathsThese visualization shows top 20 largest paths in the bundle.Meta file: packages/next/meta_index.json, Out file: esbuild/index.js
Meta file: packages/payload/meta_index.json, Out file: esbuild/index.js
Meta file: packages/payload/meta_shared.json, Out file: esbuild/exports/shared.js
Meta file: packages/richtext-lexical/meta_client.json, Out file: esbuild/exports/client_optimized/index.js
Meta file: packages/ui/meta_client.json, Out file: esbuild/exports/client_optimized/index.js
Meta file: packages/ui/meta_shared.json, Out file: esbuild/exports/shared_optimized/index.js
DetailsNext to the size is how much the size has increased or decreased compared with the base branch of this PR.
|
…lel operations (#14652) ## Description Fixes `MongoExpiredSessionError` caused by transaction state mutation leaking between parallel operations in `renderDocument`. ## Problem When `renderDocument` runs `getDocumentPermissions` and `getIsLocked` in parallel via `Promise.all`, they share the same `req` object. When `getDocumentPermissions` calls `docAccessOperation` → `initTransaction()`, it **mutates** `req.transactionID`. This mutation is visible to `getIsLocked`, which then tries to use the same transaction. When the permission check completes and commits its transaction, `getIsLocked` fails with an expired session error. This is because `getIsLocked` may still be trying to use the transactionID it (involuntarily) received from `getDocumentPermissions`, even though `getDocumentPermissions` already closed it. ## Solution Use `isolateObjectProperty(req, 'transactionID')` to give each parallel operation its own isolated transaction state. This creates a Proxy where `transactionID` mutations go to a delegate object instead of the shared parent req. ```ts // Before - shared req, mutations leak await Promise.all([ getDocumentPermissions({ req }), // Mutates req.transactionID getIsLocked({ req }), // Sees the mutation → ERROR ]) ``` ```ts // After - isolated transactionID const reqForPermissions = isolateObjectProperty(req, 'transactionID') const reqForLockCheck = isolateObjectProperty(req, 'transactionID') await Promise.all([ getDocumentPermissions({ req: reqForPermissions }), // Isolated getIsLocked({ req: reqForLockCheck }), // Isolated ]) ``` If parent `req` already has a transaction, it's preserved (no isolation applied). ## Testing This PR does not include tests, as it's very difficult to reliably test for this race condition. I reproduced this issue using this PR: #14631 as it just happened to speed up `getDocumentPermissions` just enough to trigger this race condition more often.
…ss-control-after-save
…ss-control-after-save
… not awaited, leading in error when sanitizePermissions encounters promise
| /** | ||
| * Recursively remove empty objects and false values from an object. | ||
| * | ||
| * @internal - this function may change or be removed in a minor release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not sure this is going to be a pattern we should follow. Not everyone is going to read the jsdocs and I'm not sure we should expect them to for things like this. We should probably be naming our functions differently or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at the point of export we could rename it as something like INTERNAL_
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that is what I was thinking as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it to a new payload/internal export, which should make it explicit and keep the jsdocs + function names clean
| // may cause `shouldNotShowInClientConfigUnlessAuthenticated` to be included in the bundle, | ||
| // even though we're never actually sending it to the client. | ||
| // We'll need to run this test in production to ensure it passes. | ||
| test.skip('should protect field schemas behind authentication', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love that we are removing a test that worked(?) before. Should we remove the entire test for now? What are the next steps here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test assumes no sensitive info leaking on the client. But Next.js intentionally leaks sensitive info during development.
=> the way this test worked was never sustainable. If you're unlucky, adding a button to a page could have caused that hidden field to show up on the client
We should add it back, which is why I'm only skipping it, but that would require setting up prebuild again our our test suites (it's broken right now), which wouldn't be easy and is out of scope for this PR
|
🚀 This is included in version v3.65.0 |
In large projects, calculating the
permissionsobject is one of the slowest parts of Payload. This PR completely rewrites the permission calculation system to make it significantly faster and more reliable.The permissions object is calculated multiple times throughout Payload's lifecycle: for the entire config every time we load or navigate to any admin panel page, on every API endpoint call, and as part of query path validation when calling any Payload operation.
This is done by
getAccessResultswithout any document data, and in large configs can be one of the slowest operations, noticeably slowing down admin panel navigation.After this rewrite, speed improvements in permissions calculation without data range from 5.3% (access-control test suite) to 74% (fields test suite - our largest config). The larger the config, the more noticeable these improvements become. These performance gains apply to both the Payload Admin Panel and API requests.
The
permissionsobject can also be calculated WITH document data, which triggers evaluation ofWhereconditions in collection/global-level access control and may fetch the document to pass to access control functions.This has been heavily optimized with speed improvements for permissions calculation with data ranging between 75% and 5138% (if I craft a collection config that excessively uses aspects this PR improves) in the benchmarks. This optimization affects:
These improvements are most noticeable when navigating to documents in the admin panel.
What's Fixed
In addition to performance improvements, the new function is now cleaner, easier to understand and works more reliably. I specifically wanna call out two issues that were fixed:
Field access control data bug: Added a new e2e test that was previously failing - when saving a document, the data object passed to field-level update access control functions had the wrong shape (or in some cases did not exist at all). This caused fields to show incorrect
readOnlystates after save. The new implementation properly passes document data through all access control checks.Where query validation: The old implementation sometimes skipped validating
Wherequeries for collections whenreq.dataexisted, potentially granting incorrect access. The new version always validates where queries correctly whenfetchData: true.Global Where queries:: Where queries were never executed for globals. Instead, we were just checking if the global existed in general. This PR ensures that
Wherequeries returned from global access control are respected.What's Faster (and Why)
Where query caching: When multiple operations return identical where queries (common pattern), we now cache the result. Instead of 3-4 separate DB calls, we make 1 call and reuse it.
Parallel execution: All
Wherequery evaluations and async access control functions (for both collections and fields) now execute concurrently with maximum parallelism, rather than sequentially.Optimized DB operations: When evaluating returned
Wherequeries, we now use direct databasedb.count()queries instead ofpayload.find()operations. This is much faster (especially on Postgres).Synchronous tree traversal: The entire field permission tree is now built synchronously with all async work collected and executed in parallel at the end, rather than cascading await calls through each nesting level. This eliminates sequential bottlenecks in deeply nested field structures.
Benchmarks
Admin Panel
Before:
Screenshot.2025-11-19.at.00.31.34.mp4
After:
Screenshot.2025-11-19.at.00.33.24.mp4
Branch: https://github.com/payloadcms/payload/tree/fix/update-field-access-control-after-save-benchmarks
Modified Access-Control Test Suite
cd test && pnpm payload run access-control/benchmark-permissions.ts📊 Benchmark 1: getAccessResults (all collections + globals) ────────────────────────────────────────────────────────────────────── ┌─────────┬───────────────────┬─────────┬───────────────────┬──────────┐ │ (index) │ Task Name │ ops/sec │ Average Time (ms) │ Margin │ ├─────────┼───────────────────┼─────────┼───────────────────┼──────────┤ │ 0 │ 'NEW (optimized)' │ '16.91' │ '59203.986' │ '±0.52%' │ │ 1 │ 'OLD (previous)' │ '16.07' │ '62323.953' │ '±0.57%' │ └─────────┴───────────────────┴─────────┴───────────────────┴──────────┘ ⚡ Speedup: +5.3% 🚀 📊 DB Calls per operation (across all collections + globals): NEW: 0.0 total (0.0 data, 0.0 where) OLD: 0.0 total (0.0 data, 0.0 where) 📊 Benchmark 2: docAccessOperation (with fetchData) ────────────────────────────────────────────────────────────────────── Collection: where-cache-same (same where queries) ┌─────────┬────────────────────┬───────────┬───────────────────┬──────────┐ │ (index) │ Task Name │ ops/sec │ Average Time (ms) │ Margin │ ├─────────┼────────────────────┼───────────┼───────────────────┼──────────┤ │ 0 │ 'NEW (with cache)' │ '2272.81' │ '442.319' │ '±0.11%' │ │ 1 │ 'OLD (no cache)' │ '1019.72' │ '983.347' │ '±0.12%' │ └─────────┴────────────────────┴───────────┴───────────────────┴──────────┘ ⚡ Speedup: +122.9% 🚀 📊 DB Calls per operation: NEW: 2.0 total (1.0 data, 1.0 where) OLD: 4.0 total (1.0 data, 3.0 where) 📊 Benchmark 3: docAccessOperation (with data passed) ────────────────────────────────────────────────────────────────────── Collection: where-cache-same (same where queries, no DB fetch) ┌─────────┬────────────────────┬───────────┬───────────────────┬──────────┐ │ (index) │ Task Name │ ops/sec │ Average Time (ms) │ Margin │ ├─────────┼────────────────────┼───────────┼───────────────────┼──────────┤ │ 0 │ 'NEW (with cache)' │ '4945.43' │ '203.252' │ '±0.11%' │ │ 1 │ 'OLD (no cache)' │ '1023.75' │ '979.898' │ '±0.14%' │ └─────────┴────────────────────┴───────────┴───────────────────┴──────────┘ ⚡ Speedup: +383.1% 🚀 📊 DB Calls per operation: NEW: 1.0 total (0.0 data, 1.0 where) OLD: 4.0 total (1.0 data, 3.0 where) 📊 Benchmark 4: docAccessOperation (unique where queries) ────────────────────────────────────────────────────────────────────── Collection: where-cache-unique (unique where queries per operation) ┌─────────┬────────────────────┬───────────┬───────────────────┬──────────┐ │ (index) │ Task Name │ ops/sec │ Average Time (ms) │ Margin │ ├─────────┼────────────────────┼───────────┼───────────────────┼──────────┤ │ 0 │ 'NEW (parallel)' │ '1693.67' │ '599.091' │ '±0.29%' │ │ 1 │ 'OLD (sequential)' │ '970.37' │ '1036.268' │ '±0.20%' │ └─────────┴────────────────────┴───────────┴───────────────────┴──────────┘ ⚡ Speedup: +74.5% 🚀 📊 DB Calls per operation: NEW: 4.0 total (1.0 data, 3.0 where) OLD: 4.0 total (1.0 data, 3.0 where) 📊 Benchmark 5: Complex Collection (async access, nested blocks, field access) ────────────────────────────────────────────────────────────────────── Collection: complex-content (stress test) ┌─────────┬────────────────────┬──────────┬───────────────────┬──────────┐ │ (index) │ Task Name │ ops/sec │ Average Time (ms) │ Margin │ ├─────────┼────────────────────┼──────────┼───────────────────┼──────────┤ │ 0 │ 'NEW (optimized)' │ '133.17' │ '7683.346' │ '±0.83%' │ │ 1 │ 'OLD (sequential)' │ '72.86' │ '13901.616' │ '±0.82%' │ └─────────┴────────────────────┴──────────┴───────────────────┴──────────┘ ⚡ Speedup: +82.8% 🚀 📊 DB Calls per operation: NEW: 2.0 total (1.0 data, 1.0 where) OLD: 2.0 total (1.0 data, 1.0 where) 📊 Benchmark 6: Sync-Heavy Collection (same where, many sync field access) ────────────────────────────────────────────────────────────────────── Collection: sync-heavy (where cache + field access) ┌─────────┬────────────────────┬───────────┬───────────────────┬──────────┐ │ (index) │ Task Name │ ops/sec │ Average Time (ms) │ Margin │ ├─────────┼────────────────────┼───────────┼───────────────────┼──────────┤ │ 0 │ 'NEW (with cache)' │ '3607.72' │ '279.559' │ '±0.15%' │ │ 1 │ 'OLD (no cache)' │ '68.87' │ '15136.557' │ '±1.44%' │ └─────────┴────────────────────┴───────────┴───────────────────┴──────────┘ ⚡ Speedup: +5138.5% 🚀 📊 DB Calls per operation: NEW: 1.0 total (0.0 data, 1.0 where) OLD: 5.0 total (1.0 data, 4.0 where) ══════════════════════════════════════════════════════════════════════ 📈 Summary: ══════════════════════════════════════════════════════════════════════ 1. getAccessResults: (see above) 2. docAccessOperation (with fetchData): (see above) 3. docAccessOperation (with data passed): (see above) 4. docAccessOperation (unique where): (see above) 5. Complex collection (async + nested): (see above) 6. Sync-heavy (where cache + fields): (see above) ══════════════════════════════════════════════════════════════════════Fields Test Suite
cd test && pnpm payload run fields/benchmark-getAccessResults.ts📊 Benchmark: getAccessResults (all fields test collections + globals) ────────────────────────────────────────────────────────────────────── ┌─────────┬───────────────────┬───────────┬───────────────────┬──────────┐ │ (index) │ Task Name │ ops/sec │ Average Time (ms) │ Margin │ ├─────────┼───────────────────┼───────────┼───────────────────┼──────────┤ │ 0 │ 'NEW (optimized)' │ '2104.06' │ '478.382' │ '±0.18%' │ │ 1 │ 'OLD (previous)' │ '1208.14' │ '834.974' │ '±0.21%' │ └─────────┴───────────────────┴───────────┴───────────────────┴──────────┘ ⚡ Speedup: +74.2% 🚀 📊 DB Calls per operation (across all collections + globals): NEW: 0.0 total (0.0 data, 0.0 where) OLD: 0.0 total (0.0 data, 0.0 where)