Skip to content

Commit

Permalink
Merge pull request #9 from jezdez/issue_9
Browse files Browse the repository at this point in the history
_get_user_cached_perms is doing a dependent sub-query
  • Loading branch information
jlward committed Jul 3, 2013
2 parents 50fd7f5 + 9d868c7 commit a1f5a84
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 8 deletions.
6 changes: 6 additions & 0 deletions README
Expand Up @@ -52,6 +52,12 @@ html version using the setup.py::
Changelog:
==========

0.7 (2013-07-03):
-----------------

* No longer doing dependent sub-queries. It will be faster to do two small
queries instead of one with a dependent sub-query in the general case.

0.6 (2013-06-13):
-----------------

Expand Down
6 changes: 5 additions & 1 deletion authority/permissions.py
Expand Up @@ -48,8 +48,12 @@ def _get_user_cached_perms(self):
"""
if not self.user:
return {}, {}
group_pks = set(self.user.groups.values_list(
'pk',
flat=True,
))
perms = Permission.objects.filter(
Q(user__pk=self.user.pk) | Q(group__in=self.user.groups.all()),
Q(user__pk=self.user.pk) | Q(group__pk__in=group_pks),
)
user_permissions = {}
group_permissions = {}
Expand Down
13 changes: 6 additions & 7 deletions authority/tests.py
@@ -1,6 +1,5 @@
from django import VERSION
from django.conf import settings
from django.contrib import auth
from django.contrib.auth.models import Permission as DjangoPermission
from django.contrib.auth.models import Group
from django.db.models import Q
Expand Down Expand Up @@ -77,7 +76,7 @@ def test_delete(self):
# test
self.assertFalse(self.check.delete_user())
self.assertTrue(self.check.delete_user(self.user))


class AssignBehaviourTest(TestCase):
"""
Expand Down Expand Up @@ -236,7 +235,7 @@ def test_has_user_perms(self):
# Regardless of how many times has_user_perms is called, the number of
# queries is the same.
# Content type and permissions (2 queries)
with self.assertNumQueries(2):
with self.assertNumQueries(3):
for _ in range(5):
# Need to assert it so the query actually gets executed.
assert not self.user_check.has_user_perms(
Expand All @@ -259,7 +258,7 @@ def test_has_user_perms_check_group(self):
# Regardless of the number groups permissions, it should only take one
# query to check both users and groups.
# Content type and permissions (2 queries)
with self.assertNumQueries(2):
with self.assertNumQueries(3):
self.user_check.has_user_perms(
'foo',
self.user,
Expand All @@ -273,7 +272,7 @@ def test_invalidate_user_permissions_cache(self):
# For each time invalidate_permissions_cache gets called, you
# will need to do one query to get content type and one to get
# the permissions.
with self.assertNumQueries(4):
with self.assertNumQueries(6):
for _ in range(5):
assert not self.user_check.has_user_perms(
'foo',
Expand Down Expand Up @@ -335,7 +334,7 @@ def test_has_user_perms_check_group_multiple(self):
# gets created.

# Check the number of queries.
with self.assertNumQueries(1):
with self.assertNumQueries(2):
assert self.user_check.has_user_perms('foo', self.user, True, True)

# Create a second group.
Expand All @@ -354,7 +353,7 @@ def test_has_user_perms_check_group_multiple(self):
self.user_check.invalidate_permissions_cache()

# Make sure it is the same number of queries.
with self.assertNumQueries(1):
with self.assertNumQueries(2):
assert self.user_check.has_user_perms('foo', self.user, True, True)


Expand Down

0 comments on commit a1f5a84

Please sign in to comment.