Skip to content
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

Auth._loadRoles should not query the same role twice. #1366

Merged
merged 1 commit into from Apr 5, 2016
Merged

Auth._loadRoles should not query the same role twice. #1366

merged 1 commit into from Apr 5, 2016

Conversation

blacha
Copy link
Contributor

@blacha blacha commented Apr 5, 2016

This fixes bug #1365 By not triggering a _getAllRoleNamesForId for roleID's that have already been queried.

@flovilmart
Copy link
Contributor

Very cool!

@flovilmart
Copy link
Contributor

What happens when new roles are created and added into the hierarchy or removed from the hierarchy?

@codecov-io
Copy link

Current coverage is 93.11%

Merging #1366 into master will increase coverage by +0.01% as of 1fe12b7

@@            master   #1366   diff @@
======================================
  Files           84      84       
  Stmts         5411    5415     +4
  Branches       997     999     +2
  Methods          0       0       
======================================
+ Hit           5038    5042     +4
  Partial          9       9       
  Missed         364     364       

Review entire Coverage Diff as of 1fe12b7

Powered by Codecov. Updated on successful CI builds.

@blacha
Copy link
Contributor Author

blacha commented Apr 5, 2016

Its on a per Auth._loadRoles caching, so each new request will still do the full new query.

It is just trying to limit the same query from happening multiple times in the Auth._loadRoles

@flovilmart
Copy link
Contributor

Alright! LGTM!

@flovilmart flovilmart merged commit 18906f1 into parse-community:master Apr 5, 2016
@blacha
Copy link
Contributor Author

blacha commented Apr 5, 2016

It would probably be a good idea to sometime in the future cache the results of these _loadRoles on a per user basis, in my application right now I can get upwards of 1000 _Role queries in a single query. Not too sure how Parse.com did it, but it was much faster on there than it is on ParseServer

@flovilmart
Copy link
Contributor

Caching is quite a challenge in a distributed environment. But we definitely should look into it.

@flovilmart
Copy link
Contributor

That being said, the Cache class could support easily role hierarchies, and we could invalidated the same way we invalidate users. If that's something you wanna tackle, feel free to get going.

@blacha
Copy link
Contributor Author

blacha commented Apr 5, 2016

This commit actually solves most of our performance problems with Roles in our production environment :)

It would be tricky to trigger cache invalidations on multiple VM's when a one VM changes a Role or User.

Looking of the invalidation code you have already for the User, you would run into this problem when you have more than one instance of the ParseServer.

Do you have anywhere where devs on this project talk (Gitter/IRC/Slack/?), as talking more about this here isnt really ideal.

@flovilmart
Copy link
Contributor

Shoot me a mail.

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.

None yet

4 participants