-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
feat(performance): use $graphLookup for auth #6556
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
base: alpha
Are you sure you want to change the base?
feat(performance): use $graphLookup for auth #6556
Conversation
It seems like husky added a number of formatting changes on the staged files |
Codecov Report
@@ Coverage Diff @@
## master #6556 +/- ##
==========================================
- Coverage 94.01% 93.80% -0.22%
==========================================
Files 169 169
Lines 11850 11876 +26
==========================================
- Hits 11141 11140 -1
- Misses 709 736 +27
Continue to review full report at Codecov.
|
@omairvaiyani nice idea. It looks very promising. Let me know if you need any help! |
@davimacedo Thanks! We've battle-tested this on our production app ~25-40k heavy MAUs. No issues at all so far. The two things stopping this PR from merge are:
Any thoughts on getting around this? |
Hi all, any updates on this one, we'd love to upgrade from ParseServer 2.x but this is blocking us. ParseServer 2.x had a bug where it only returned the first 100 roles which is fixed in later versions but this makes ParseServer unusably slow for our product because we have some users that are in thousands of roles. We'd like to help get this over the line, can @omairvaiyani tell me more about what leads to the lack of testability? |
@TylerBrock I believe that the version of Another issue making this PR a headache is the formatting changes that auto-ran on my commits, blurring the diff and failing the coverage checks. |
@omairvaiyani I took a stab at rewriting the commits in this PR on top of master to remove all of the auto-formatting noise. Does https://github.com/Hustle/parse-server/commits/omairvaiyani/improve-auth-efficiency look like it captures your changes as expected? |
|
@abubkr-hago would you want to review this PR and try to bring it over the finish line? |
At a high-level, this PR (work in progress) replaces the current recursive roles/auth query with a single aggregation pipeline to list all roles within the user's graph in one request. For applications at scale, this (at first set of benchmarks) appears to suggest a >10x reduction in request times before the cache kicks in.
After this discussion I have managed to land a big performance improvement. Each request by an authenticated user that results in a database query must be authorised, which currently means loading all the roles that the user is in, and loading the roles that yield to these, one level at a time.
Currently, for every new layer of roles added to an application, the request grows by 1. Depending on your setup, this can be quite a slow process. We've seen this take upwards of 5s on some of our heavy users with a significant amount of roles. There are certainly some application and schema design improvements that developers could make to reduce the total number or indeed levels of roles, however, we have a clear vector to optimise a very central part of this codebase.
Rough benchmarks:
I ran these against my own application about 10 times for each of the following cases
Num roles: 5
Current: ~100-160ms
New: ~25-30ms
Num roles: 50
Current: ~200-500ms
New: ~40-60ms
Num roles: 1000
Current: ~900-4400ms
New: ~65-120ms
Help needed
The test suite uses
mongo-runner
which appears to have some shortcomings when dealing with$graphLookup
, hence the anaemic test suite in this PR. Any suggestions here would be helpful. The current test-suite (Auth.spec.js
) does not fully cover the role look up anyways.Aggregation pipeline
Work in progress:
DatabaseController...classExists
problemCredit to @noahsilas for paving the blocks for this