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

Templates: optimize permissions check #7277

Merged
merged 2 commits into from Jul 9, 2020
Merged

Templates: optimize permissions check #7277

merged 2 commits into from Jul 9, 2020

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Jul 8, 2020

This has a lot of effect in corporate,
where the check for is_admin is more complex.

This has a lot of effect in corporate,
where the check for is_admin is more complex.
@stsewd stsewd requested a review from a team Jul 8, 2020
@@ -421,6 +427,7 @@ def project_versions(request, project_slug):
'inactive_versions': inactive_versions,
'active_versions': active_versions,
'project': project,
'is_project_admin': AdminPermission.is_admin(request.user, project),
Copy link
Member

@ericholscher ericholscher Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if its better to add some kind of caching to is_admin so we don't hit this over and over? We could even just cache it for like 10 seconds or something, but that might make the code a bit simpler than having an explicit cache in the template?

Copy link
Member Author

@stsewd stsewd Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it's a class method, the cache key would be something like the class, the object id and the user. I'll try and see how that would work.

Copy link
Member Author

@stsewd stsewd Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I did a test. Caching reduces the number of queries to almost the same number, but it adds more overhead in the caching side (more CPU and time), making the requests slow.

Screenshot_2020-07-08 Versions - Read the Docs for Business(1)

Screenshot_2020-07-08 Versions - Read the Docs for Business

I can submit a PR with the cache, so cache + this PR we should reduce more the slowness

Copy link
Member

@ericholscher ericholscher Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yea, good point on all the cache calls -- I think this approach is probably best 👍

@stsewd stsewd requested a review from ericholscher Jul 9, 2020
@stsewd stsewd merged commit 0b128b2 into master Jul 9, 2020
2 checks passed
@stsewd stsewd deleted the optimize-templates branch Jul 9, 2020
{% if request.user|is_admin:project %}
{% if is_project_admin %}
Copy link
Member

@humitos humitos Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference here?

is_project_admin is calling AdminPermission.is_admin which is exactly the same that is_admin filter does.

If we need to use the value multiple times in the template, we can use {% with request.user|is_admin:project as is_project_admin %}.

I don't like too much to change the pattern of using |is_admin filter here, since it's everywhere and already stablished.

Copy link
Member

@ericholscher ericholscher Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@humitos it uses a lot fewer queries with the cache. There were over 800 queries on this page in .com because of the permission checks getting really expensive.

Copy link
Member

@ericholscher ericholscher Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The with statement wouldn't work across templates, so I think this is the correct solution.

Copy link
Member Author

@stsewd stsewd Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would have a big with statement somewhere in a common template, I don't feel that's more readable

Copy link
Member

@humitos humitos Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes to reduce the queries are 💯. I just wanted to try to keep the same pattern we have been using if possible, instead of having a new one only for these files.

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

3 participants