Skip to content

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

Draft
wants to merge 5 commits into
base: alpha
Choose a base branch
from

Conversation

omairvaiyani
Copy link
Contributor

@omairvaiyani omairvaiyani commented Apr 1, 2020

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

// on _Join:users:_Role
[
    {
      $match: {
        relatedId: this.user.id,
      },
    },
    {
      $graphLookup: {
        from: '_Join:roles:_Role',
        startWith: '$owningId',
        connectFromField: 'owningId',
        connectToField: 'relatedId',
        as: 'childRolePath',
      },
    },
    {
      $facet: {
        directRoles: [
          {
            $lookup: {
              from: '_Role',
              localField: 'owningId',
              foreignField: '_id',
              as: 'Roles',
            },
          },
          {
            $unwind: {
              path: '$Roles',
            },
          },
          {
            $replaceRoot: {
              newRoot: {
                $ifNull: ['$Roles', { $literal: {} }],
              },
            },
          },
          {
            $project: {
              name: 1,
            },
          },
        ],
        childRoles: [
          {
            $lookup: {
              from: '_Role',
              localField: 'childRolePath.owningId',
              foreignField: '_id',
              as: 'Roles',
            },
          },
          {
            $unwind: {
              path: '$Roles',
            },
          },
          {
            $replaceRoot: {
              newRoot: {
                $ifNull: ['$Roles', { $literal: {} }],
              },
            },
          },
          {
            $project: {
              name: 1,
            },
          },
        ],
      },
    },
  ]

Work in progress:

  • Proof of concept
  • Remove legacy functions
  • Test-cases
  • Postgres support or backwards compatibility
  • Proper solution for DatabaseController...classExists problem

Credit to @noahsilas for paving the blocks for this

@omairvaiyani
Copy link
Contributor Author

It seems like husky added a number of formatting changes on the staged files

@codecov
Copy link

codecov bot commented Apr 2, 2020

Codecov Report

Merging #6556 into master will decrease coverage by 0.21%.
The diff coverage is 73.56%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
src/Auth.js 86.85% <50.00%> (-13.15%) ⬇️
src/Controllers/DatabaseController.js 95.19% <100.00%> (ø)
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 92.82% <0.00%> (-0.68%) ⬇️
src/RestWrite.js 93.48% <0.00%> (-0.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d52d35b...d92dc8b. Read the comment docs.

@omairvaiyani omairvaiyani requested a review from davimacedo April 20, 2020 11:06
@omairvaiyani omairvaiyani marked this pull request as draft April 20, 2020 11:06
@davimacedo
Copy link
Member

@omairvaiyani nice idea. It looks very promising. Let me know if you need any help!

@omairvaiyani
Copy link
Contributor Author

@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:

  • diff coverage (due to post-commit formatting issues) <- maybe I can re-submit a separate PR after resolving those?
  • lack of testability for the $graphLookup as the mongodb runner in this repository is unable to properly handle it.

Any thoughts on getting around this?

@TylerBrock
Copy link
Contributor

TylerBrock commented Aug 11, 2020

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?

@omairvaiyani
Copy link
Contributor Author

omairvaiyani commented Aug 12, 2020

@TylerBrock I believe that the version of mongo-runner server used in the test-suite is unable to handle the $graphLookup stage in the pipeline, or at least, that was my observation while submitting this PR. Without it, testing in this manner is quite difficult. In our own private test-suites, we use Docker compose to test against a real Mongo instance. We have been using this pipeline to run our role auth for many months now without an issue.

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.

@noahsilas
Copy link
Contributor

@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?

@mtrezza
Copy link
Member

mtrezza commented Sep 3, 2021

⚠️ Important change for merging PRs from Parse Server 5.0 onwards!

We are planning to release the first beta version of Parse Server 5.0 in October 2021.

If a PR contains a breaking change and is not merged before the beta release of Parse Server 5.0, it cannot be merged until the end of 2022. Instead it has to follow the Deprecation Policy and phase-in breaking changes to be merged during the course of 2022.

One of the most voiced community feedbacks was the demand for predictability in breaking changes to make it easy to upgrade Parse Server. We have made a first step towards this by introducing the Deprecation Policy in February 2021 that assists to phase-in breaking changes, giving developers time to adapt. We will follow-up with the introduction of Release Automation and a branch model that will allow breaking changes only with a new major release, scheduled for the beginning of each calendar year.

We understand that some PRs are a long time in the making and we very much appreciate your contribution. We want to make it easy for PRs that contain a breaking change and were created before the introduction of the Deprecation Policy. These PRs can be merged with a breaking change without being phased-in before the beta release of Parse Server 5.0. We are making this exception because we appreciate that this is a time of transition that requires additional effort from contributors to adapt. We encourage everyone to prepare their PRs until the end of September and account for review time and possible adaptions.

If a PR contains a breaking change and should be merged before the beta release, please mention @parse-community/server-maintenance and we will coordinate with you to merge the PR.

Thanks for your contribution and support during this transition to Parse Server release automation!

@mtrezza
Copy link
Member

mtrezza commented Apr 29, 2025

@abubkr-hago would you want to review this PR and try to bring it over the finish line?

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.

Use $graphLookup for auth
5 participants