-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
fix(api): N+1 db query in GroupSerializerBase._get_permalink #29717
fix(api): N+1 db query in GroupSerializerBase._get_permalink #29717
Conversation
The `_get_permalink` method was being invoked per object, and each time it is invoked, it checks if the user is authorized to get the permalink. This change moves the authorization check to `get_attr` to avoid this N+1 db query.
or is_valid_sentryapp | ||
or (user.is_authenticated and user.get_orgs().filter(id=obj.organization.id).exists()) | ||
): | ||
return user.is_authenticated and user.get_orgs().filter(id=organization_id).exists() |
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.
Previously, this was filtering on obj.organization.id
but as per the comment here, it should always be the case that the organizations are the same on all the objs.
is_valid_sentryapp = SentryAppInstallationToken.objects.has_organization_access( | ||
request.auth, obj.organization | ||
) | ||
if SentryAppInstallationToken.objects.has_organization_access( |
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 condition can be simplified to:
def _is_authorized(self, user: User, organization_id: int) -> bool:
# If user is not logged in and member of the organization,
# do not return the permalink which contains private information i.e. org name.
request = env.request
return (
request
and (
(is_active_superuser(request) and request.user == user)
or (
# If user is a sentry_app then it's a proxy user meaning we can't do a org lookup via `get_orgs()`
# because the user isn't an org member. Instead we can use the auth token and the installation
# it's associated with to find out what organization the token has access to.
getattr(request.user, "is_sentry_app", False)
and isinstance(request.auth, ApiToken)
and SentryAppInstallationToken.objects.has_organization_access(
request.auth, organization_id
)
)
)
) or (user.is_authenticated and user.get_orgs().filter(id=organization_id).exists())
This is definitely overkill but I think you could merge up the has_organization_access()
call.
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.
When you say to merge up has_organization_access
, do you mean to lift its contents inline into this function?
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.
Thanks for fixing this. Could you post a link to some discover graphs from before/after once this merges? Interested to see how much difference it makes.
The
_get_permalink
method was being invoked per object, and each time it isinvoked, it checks if the user is authorized to get the permalink. This change
moves the authorization check to
get_attr
to avoid this N+1 db query.