Skip to content

Commit

Permalink
fix caching issue with prefetch_perms
Browse files Browse the repository at this point in the history
  • Loading branch information
@pgeez @brianmay
pgeez authored and brianmay committed May 14, 2016
1 parent 12cf58b commit a2e2216
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 37 deletions.
14 changes: 8 additions & 6 deletions guardian/core.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,12 +204,11 @@ def prefetch_perms(self, objects):

group_model = get_group_obj_perms_model(model)

group_filters = {}
if self.user:
fieldname = 'group__%s' % (
User.groups.field.related_query_name(),
)
group_filters.update({fieldname: self.user})
group_filters = {fieldname: self.user}
else:
group_filters = {'group': self.group}

Expand Down Expand Up @@ -249,15 +248,18 @@ def prefetch_perms(self, objects):
*(group_model.objects.filter(**group_filters).select_related('permission'),)
)

# initialize entry in '_obj_perms_cache' for all prefetched objects
for obj in objects:
key = self.get_local_cache_key(obj)
if key not in self._obj_perms_cache:
self._obj_perms_cache[key] = []

for perm in perms:
if type(perm).objects.is_generic():
key = (ctype.id, perm.object_pk)
else:
key = (ctype.id, force_text(perm.content_object_id))

if key in self._obj_perms_cache:
self._obj_perms_cache[key].append(perm.permission.codename)
else:
self._obj_perms_cache[key] = [perm.permission.codename]
self._obj_perms_cache[key].append(perm.permission.codename)

return True
97 changes: 66 additions & 31 deletions guardian/testapp/tests/test_core.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,18 +190,23 @@ def test_prefetch_user_perms(self):
from django.db import connection

ContentType.objects.clear_cache()
new_group = Group.objects.create(name='new-group')
group1 = Group.objects.create(name='group1')
group2 = Group.objects.create(name='group2')
user = User.objects.create(username='active_user', is_active=True)
assign_perm("change_group", user, self.group)
assign_perm("change_group", user, new_group)
assign_perm("change_group", user, group1)
checker = ObjectPermissionChecker(user)

# Prefetch permissions
self.assertTrue(checker.prefetch_perms([self.group, new_group]))
prefetched_objects = [self.group, group1, group2]
self.assertTrue(checker.prefetch_perms(prefetched_objects))
query_count = len(connection.queries)

# Checking cache is filled
self.assertEqual(len(checker._obj_perms_cache), 2)
self.assertEqual(
len(checker._obj_perms_cache),
len(prefetched_objects)
)

# Checking shouldn't spawn any queries
checker.has_perm("change_group", self.group)
Expand All @@ -213,9 +218,14 @@ def test_prefetch_user_perms(self):
self.assertEqual(len(connection.queries), query_count)

# Checking for same model but other instance shouldn't spawn any queries
checker.has_perm("change_group", new_group)
checker.has_perm("change_group", group1)
self.assertEqual(len(connection.queries), query_count)

# Checking for same model but other instance shouldn't spawn any queries
# Even though User doesn't have perms on Group2, we still should
# not hit DB
self.assertFalse(checker.has_perm("change_group", group2))
self.assertEqual(len(connection.queries), query_count)
finally:
settings.DEBUG = False

Expand All @@ -225,18 +235,22 @@ def test_prefetch_superuser_perms(self):
from django.db import connection

ContentType.objects.clear_cache()
new_group = Group.objects.create(name='new-group')
group1 = Group.objects.create(name='group1')
user = User.objects.create(username='active_superuser',
is_superuser=True, is_active=True)
assign_perm("change_group", user, self.group)
checker = ObjectPermissionChecker(user)

# Prefetch permissions
self.assertTrue(checker.prefetch_perms([self.group, new_group]))
prefetched_objects = [self.group, group1]
self.assertTrue(checker.prefetch_perms(prefetched_objects))
query_count = len(connection.queries)

# Checking cache is filled
self.assertEqual(len(checker._obj_perms_cache), 2)
self.assertEqual(
len(checker._obj_perms_cache),
len(prefetched_objects)
)

# Checking shouldn't spawn any queries
checker.has_perm("change_group", self.group)
Expand All @@ -248,9 +262,8 @@ def test_prefetch_superuser_perms(self):
self.assertEqual(len(connection.queries), query_count)

# Checking for same model but other instance shouldn't spawn any queries
checker.has_perm("change_group", new_group)
checker.has_perm("change_group", group1)
self.assertEqual(len(connection.queries), query_count)

finally:
settings.DEBUG = False

Expand All @@ -260,17 +273,23 @@ def test_prefetch_group_perms(self):
from django.db import connection

ContentType.objects.clear_cache()
new_group = Group.objects.create(name='new-group')
assign_perm("change_group", new_group, self.group)
assign_perm("change_group", new_group, new_group)
checker = ObjectPermissionChecker(new_group)
group1 = Group.objects.create(name='group1')
group2 = Group.objects.create(name='group2')
assign_perm("change_group", group1, self.group)
assign_perm("change_group", group1, group1)
checker = ObjectPermissionChecker(group1)

# Prefetch permissions
self.assertTrue(checker.prefetch_perms([self.group, new_group]))
prefetched_objects = [self.group, group1, group2]
self.assertTrue(checker.prefetch_perms(prefetched_objects))

query_count = len(connection.queries)

# Checking cache is filled
self.assertEqual(len(checker._obj_perms_cache), 2)
self.assertEqual(
len(checker._obj_perms_cache),
len(prefetched_objects)
)

# Checking shouldn't spawn any queries
checker.has_perm("change_group", self.group)
Expand All @@ -282,9 +301,14 @@ def test_prefetch_group_perms(self):
self.assertEqual(len(connection.queries), query_count)

# Checking for same model but other instance shouldn't spawn any queries
checker.has_perm("change_group", new_group)
checker.has_perm("change_group", group1)
self.assertEqual(len(connection.queries), query_count)

# Checking for same model but other instance shouldn't spawn any queries
# Even though User doesn't have perms on Group2, we still should
# not hit DB
self.assertFalse(checker.has_perm("change_group", group2))
self.assertEqual(len(connection.queries), query_count)
finally:
settings.DEBUG = False

Expand All @@ -297,17 +321,18 @@ def test_prefetch_user_perms_direct_rel(self):
user = User.objects.create(username='active_user', is_active=True)
projects = \
[Project.objects.create(name='Project%s' % i)
for i in range(2)]
for project in projects:
assign_perm("change_project", user, project)
for i in range(3)]
assign_perm("change_project", user, projects[0])
assign_perm("change_project", user, projects[1])

checker = ObjectPermissionChecker(user)

# Prefetch permissions
self.assertTrue(checker.prefetch_perms(projects))
query_count = len(connection.queries)

# Checking cache is filled
self.assertEqual(len(checker._obj_perms_cache), 2)
self.assertEqual(len(checker._obj_perms_cache), len(projects))

# Checking shouldn't spawn any queries
checker.has_perm("change_project", projects[0])
Expand All @@ -323,6 +348,11 @@ def test_prefetch_user_perms_direct_rel(self):
checker.has_perm("change_project", projects[1])
self.assertEqual(len(connection.queries), query_count)

# Checking for same model but other instance shouldn't spawn any queries
# Even though User doesn't have perms on projects[2], we still
# should not hit DB
self.assertFalse(checker.has_perm("change_project", projects[2]))
self.assertEqual(len(connection.queries), query_count)
finally:
settings.DEBUG = False

Expand All @@ -337,16 +367,16 @@ def test_prefetch_superuser_perms_direct_rel(self):
projects = \
[Project.objects.create(name='Project%s' % i)
for i in range(2)]
for project in projects:
assign_perm("change_project", user, project)
assign_perm("change_project", user, projects[0])

checker = ObjectPermissionChecker(user)

# Prefetch permissions
self.assertTrue(checker.prefetch_perms(projects))
query_count = len(connection.queries)

# Checking cache is filled
self.assertEqual(len(checker._obj_perms_cache), 2)
self.assertEqual(len(checker._obj_perms_cache), len(projects))

# Checking shouldn't spawn any queries
checker.has_perm("change_project", projects[0])
Expand All @@ -361,7 +391,6 @@ def test_prefetch_superuser_perms_direct_rel(self):
# queries
checker.has_perm("change_project", projects[1])
self.assertEqual(len(connection.queries), query_count)

finally:
settings.DEBUG = False

Expand All @@ -371,20 +400,21 @@ def test_prefetch_group_perms_direct_rel(self):
from django.db import connection

ContentType.objects.clear_cache()
new_group = Group.objects.create(name='new-group')
group = Group.objects.create(name='new-group')
projects = \
[Project.objects.create(name='Project%s' % i)
for i in range(2)]
for project in projects:
assign_perm("change_project", new_group, project)
checker = ObjectPermissionChecker(new_group)
for i in range(3)]
assign_perm("change_project", group, projects[0])
assign_perm("change_project", group, projects[1])

checker = ObjectPermissionChecker(group)

# Prefetch permissions
self.assertTrue(checker.prefetch_perms(projects))
query_count = len(connection.queries)

# Checking cache is filled
self.assertEqual(len(checker._obj_perms_cache), 2)
self.assertEqual(len(checker._obj_perms_cache), len(projects))

# Checking shouldn't spawn any queries
checker.has_perm("change_project", projects[0])
Expand All @@ -400,5 +430,10 @@ def test_prefetch_group_perms_direct_rel(self):
checker.has_perm("change_project", projects[1])
self.assertEqual(len(connection.queries), query_count)

# Checking for same model but other instance shouldn't spawn any queries
# Even though User doesn't have perms on projects[2], we still
# should not hit DB
self.assertFalse(checker.has_perm("change_project", projects[2]))
self.assertEqual(len(connection.queries), query_count)
finally:
settings.DEBUG = False

0 comments on commit a2e2216

Please sign in to comment.