From ec2c2802d1756b99521931bef61390dff3871608 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Thu, 20 Sep 2012 10:54:49 -0400 Subject: [PATCH 01/46] Changing object references from object.id to object.pk --- authority/permissions.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index 434ae2b..e0fe854 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -41,6 +41,7 @@ class BasePermission(object): super(BasePermission, self).__init__(*args, **kwargs) def has_user_perms(self, perm, obj, approved, check_groups=True): + print 'in has_user_perms' if self.user: if self.user.is_superuser: return True @@ -48,7 +49,7 @@ class BasePermission(object): return False # check if a Permission object exists for the given params return Permission.objects.user_permissions(self.user, perm, obj, - approved, check_groups).filter(object_id=obj.id) + approved, check_groups).filter(object_id=obj.pk) return False def has_group_perms(self, perm, obj, approved): @@ -58,7 +59,7 @@ class BasePermission(object): if self.group: perms = Permission.objects.group_permissions(self.group, perm, obj, approved) - return perms.filter(object_id=obj.id) + return perms.filter(object_id=obj.pk) return False def has_perm(self, perm, obj, check_groups=True, approved=True): From a67c7b43bde3f68359c3b7814d0a5d0ab70e29bb Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Thu, 20 Sep 2012 10:56:37 -0400 Subject: [PATCH 02/46] Fixed Q object query that triggers a deepcopy bug in Python < 2.7 when a backend stores an Ellipsis object on the user --- authority/managers.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/authority/managers.py b/authority/managers.py index 218158a..557dfba 100644 --- a/authority/managers.py +++ b/authority/managers.py @@ -19,8 +19,11 @@ class PermissionManager(models.Manager): perms = self.get_for_model(obj) if not check_groups: return perms.select_related('user', 'creator').filter(user=user) + + # Hacking user to user__pk to workaround deepcopy bug: http://bugs.python.org/issue2460 + # Which is triggered by django's deepcopy which backports that fix in Django 1.2 return perms.select_related('user', 'user__groups', 'creator').filter( - Q(user=user) | Q(group__in=user.groups.all())) + Q(user__pk=user.pk) | Q(group__in=user.groups.all())) def user_permissions(self, user, perm, obj, approved=True, check_groups=True): return self.for_user(user, obj, check_groups).filter(codename=perm, @@ -51,4 +54,4 @@ class PermissionManager(models.Manager): perms = self.user_permissions(user, perm, obj).filter(object_id=obj.id) perms.delete() - \ No newline at end of file + From d41bb7147064d158374ccce251fc36343732f22e Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Thu, 20 Sep 2012 11:00:01 -0400 Subject: [PATCH 03/46] flake8 --- authority/managers.py | 26 +++++++++++------- authority/permissions.py | 57 ++++++++++++++++++++++++++-------------- 2 files changed, 55 insertions(+), 28 deletions(-) diff --git a/authority/managers.py b/authority/managers.py index 557dfba..28aa7a7 100644 --- a/authority/managers.py +++ b/authority/managers.py @@ -2,6 +2,7 @@ from django.db import models from django.db.models import Q from django.contrib.contenttypes.models import ContentType + class PermissionManager(models.Manager): def get_content_type(self, obj): @@ -13,28 +14,37 @@ class PermissionManager(models.Manager): def for_object(self, obj, approved=True): return self.get_for_model(obj).select_related( 'user', 'creator', 'group', 'content_type' - ).filter(object_id=obj.id,approved=approved) + ).filter(object_id=obj.id, approved=approved) def for_user(self, user, obj, check_groups=True): perms = self.get_for_model(obj) if not check_groups: return perms.select_related('user', 'creator').filter(user=user) - # Hacking user to user__pk to workaround deepcopy bug: http://bugs.python.org/issue2460 - # Which is triggered by django's deepcopy which backports that fix in Django 1.2 + # Hacking user to user__pk to workaround deepcopy bug: + # http://bugs.python.org/issue2460 + # Which is triggered by django's deepcopy which backports that fix in + # Django 1.2 return perms.select_related('user', 'user__groups', 'creator').filter( Q(user__pk=user.pk) | Q(group__in=user.groups.all())) - def user_permissions(self, user, perm, obj, approved=True, check_groups=True): - return self.for_user(user, obj, check_groups).filter(codename=perm, - approved=approved) + def user_permissions( + self, user, perm, obj, approved=True, check_groups=True): + return self.for_user( + user, + obj, + check_groups, + ).filter( + codename=perm, + approved=approved, + ) def group_permissions(self, group, perm, obj, approved=True): """ Get objects that have Group perm permission on """ return self.get_for_model(obj).select_related( - 'user', 'group', 'creator').filter(group=group, codename=perm, + 'user', 'group', 'creator').filter(group=group, codename=perm, approved=approved) def delete_objects_permissions(self, obj): @@ -53,5 +63,3 @@ class PermissionManager(models.Manager): return perms = self.user_permissions(user, perm, obj).filter(object_id=obj.id) perms.delete() - - diff --git a/authority/permissions.py b/authority/permissions.py index e0fe854..d77e71b 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -1,5 +1,3 @@ -import copy - from django.db.models.base import Model, ModelBase from django.template.defaultfilters import slugify from django.contrib.auth.models import Permission as DjangoPermission @@ -8,6 +6,7 @@ from django.contrib.contenttypes.models import ContentType from authority.exceptions import NotAModel, UnsavedModelInstance from authority.models import Permission + class PermissionMetaclass(type): """ Used to generate the default set of permission checks "add", "change" and @@ -25,6 +24,7 @@ class PermissionMetaclass(type): new_class.checks = [check.lower() for check in new_class.checks] return new_class + class BasePermission(object): """ Base Permission class to be used to define app permissions. @@ -98,19 +98,26 @@ class BasePermission(object): perms = perms or self.has_perm(perm, obj) return perms - def get_django_codename(self, check, model_or_instance, generic=False, without_left=False): + def get_django_codename( + self, check, model_or_instance, generic=False, without_left=False): if without_left: perm = check else: perm = '%s.%s' % (model_or_instance._meta.app_label, check.lower()) if generic: - perm = '%s_%s' % (perm, model_or_instance._meta.object_name.lower()) + perm = '%s_%s' % ( + perm, + model_or_instance._meta.object_name.lower(), + ) return perm def get_codename(self, check, model_or_instance, generic=False): perm = '%s.%s' % (self.label, check.lower()) if generic: - perm = '%s_%s' % (perm, model_or_instance._meta.object_name.lower()) + perm = '%s_%s' % ( + perm, + model_or_instance._meta.object_name.lower(), + ) return perm def assign(self, check=None, content_object=None, generic=False): @@ -151,26 +158,37 @@ class BasePermission(object): for check in checks: if isinstance(content_object, Model): # make an authority per object permission - codename = self.get_codename(check, content_object, generic) + codename = self.get_codename( + check, + content_object, + generic, + ) try: perm = Permission.objects.get( - user = self.user, - codename = codename, - approved = True, - content_type = content_type, - object_id = content_object.pk) + user=self.user, + codename=codename, + approved=True, + content_type=content_type, + object_id=content_object.pk, + ) except Permission.DoesNotExist: perm = Permission.objects.create( - user = self.user, - content_object = content_object, - codename = codename, - approved = True) + user=self.user, + content_object=content_object, + codename=codename, + approved=True, + ) result.append(perm) elif isinstance(content_object, ModelBase): # make a Django permission - codename = self.get_django_codename(check, content_object, generic, without_left=True) + codename = self.get_django_codename( + check, + content_object, + generic, + without_left=True, + ) try: perm = DjangoPermission.objects.get(codename=codename) except DjangoPermission.DoesNotExist: @@ -178,9 +196,10 @@ class BasePermission(object): if '_' in name: name = name[0:name.find('_')] perm = DjangoPermission( - name = name, - codename = codename, - content_type = content_type) + name=name, + codename=codename, + content_type=content_type, + ) perm.save() self.user.user_permissions.add(perm) result.append(perm) From ba0d47ac0b025f785460d98b051fd7f15bad31a9 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Thu, 20 Sep 2012 11:37:44 -0400 Subject: [PATCH 04/46] Built a pre-cached permission dict to reduce the number of queries. --- authority/permissions.py | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index d77e71b..36d165d 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -40,16 +40,44 @@ class BasePermission(object): self.group = group super(BasePermission, self).__init__(*args, **kwargs) + self._permission_cache_filled = False + self._cached_permissions = {} + + def _get_permissions(self): + perms = Permission.objects.filter( + user=self.user, + ) + # Pre cache all the permission in a dictionary. + permissions = {} + for perm in perms: + # Not currently handling groups. + permissions[(perm.object_id, perm.codename, perm.approved)] = perm + return permissions + + @property + def cached_permissions(self): + if self._permission_cache_filled: + return self._cached_permissions + self._cached_permissions = self._get_permissions() + + self._permission_cache_filled = True + return self._cached_permissions + def has_user_perms(self, perm, obj, approved, check_groups=True): - print 'in has_user_perms' if self.user: if self.user.is_superuser: return True if not self.user.is_active: return False # check if a Permission object exists for the given params - return Permission.objects.user_permissions(self.user, perm, obj, - approved, check_groups).filter(object_id=obj.pk) + perm = self.cached_permissions.get(( + obj.pk, + perm, + approved, + )) + if perm: + return True + return False return False def has_group_perms(self, perm, obj, approved): From 5cdf9f01b036b9185133c8e63a6fac9e12c1e1c9 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Thu, 20 Sep 2012 11:55:38 -0400 Subject: [PATCH 05/46] flake8 --- authority/tests.py | 75 +++++++++++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 31 deletions(-) diff --git a/authority/tests.py b/authority/tests.py index 45350a6..84b27fc 100644 --- a/authority/tests.py +++ b/authority/tests.py @@ -7,11 +7,13 @@ from authority import permissions from authority.models import Permission from authority.exceptions import NotAModel, UnsavedModelInstance + class UserPermission(permissions.BasePermission): checks = ('browse',) label = 'user_permission' authority.register(User, UserPermission) + class BehaviourTest(TestCase): """ self.user will be given: @@ -21,7 +23,7 @@ class BehaviourTest(TestCase): This permissions are given in the test case and not in the fixture, for later reference. """ - fixtures = ['tests.json',] + fixtures = ['tests.json'] def setUp(self): self.user = User.objects.get(username='jezdez') @@ -53,39 +55,44 @@ class BehaviourTest(TestCase): self.assertFalse(self.check.delete_user()) self.assertTrue(self.check.delete_user(self.user)) + class AssignBehaviourTest(TestCase): - """ - self.user will be given: - - permission add_user (test_add), - - permission delete_user for him (test_delete), - - all existing codenames permissions: a/b/c/d (test_all), - """ - fixtures = ['tests.json',] + """ + self.user will be given: + - permission add_user (test_add), + - permission delete_user for him (test_delete), + - all existing codenames permissions: a/b/c/d (test_all), + """ + fixtures = ['tests.json'] - def setUp(self): - self.user = User.objects.get(username='jezdez') - self.check = UserPermission(self.user) + def setUp(self): + self.user = User.objects.get(username='jezdez') + self.check = UserPermission(self.user) - def test_add(self): - result = self.check.assign(check='add_user') + def test_add(self): + result = self.check.assign(check='add_user') - self.assertTrue(isinstance(result[0], DjangoPermission)) - self.assertTrue(self.check.add_user()) + self.assertTrue(isinstance(result[0], DjangoPermission)) + self.assertTrue(self.check.add_user()) - def test_delete(self): - result = self.check.assign(content_object=self.user, check='delete_user') + def test_delete(self): + result = self.check.assign( + content_object=self.user, + check='delete_user', + ) - self.assertTrue(isinstance(result[0], Permission)) - self.assertFalse(self.check.delete_user()) - self.assertTrue(self.check.delete_user(self.user)) + self.assertTrue(isinstance(result[0], Permission)) + self.assertFalse(self.check.delete_user()) + self.assertTrue(self.check.delete_user(self.user)) + + def test_all(self): + result = self.check.assign(content_object=self.user) + self.assertTrue(isinstance(result, list)) + self.assertTrue(self.check.browse_user(self.user)) + self.assertTrue(self.check.delete_user(self.user)) + self.assertTrue(self.check.add_user(self.user)) + self.assertTrue(self.check.change_user(self.user)) - def test_all(self): - result = self.check.assign(content_object=self.user) - self.assertTrue(isinstance(result, list)) - self.assertTrue(self.check.browse_user(self.user)) - self.assertTrue(self.check.delete_user(self.user)) - self.assertTrue(self.check.add_user(self.user)) - self.assertTrue(self.check.change_user(self.user)) class GenericAssignBehaviourTest(TestCase): """ @@ -93,7 +100,7 @@ class GenericAssignBehaviourTest(TestCase): - permission add (test_add), - permission delete for him (test_delete), """ - fixtures = ['tests.json',] + fixtures = ['tests.json'] def setUp(self): self.user = User.objects.get(username='jezdez') @@ -106,17 +113,23 @@ class GenericAssignBehaviourTest(TestCase): self.assertTrue(self.check.add_user()) def test_delete(self): - result = self.check.assign(content_object=self.user, check='delete', generic=True) + result = self.check.assign( + content_object=self.user, + check='delete', + generic=True, + ) self.assertTrue(isinstance(result[0], Permission)) self.assertFalse(self.check.delete_user()) self.assertTrue(self.check.delete_user(self.user)) + class AssignExceptionsTest(TestCase): """ - Tests that exceptions are thrown if assign() was called with inconsistent arguments. + Tests that exceptions are thrown if assign() was called with inconsistent + arguments. """ - fixtures = ['tests.json',] + fixtures = ['tests.json'] def setUp(self): self.user = User.objects.get(username='jezdez') From 73cdf40ab232fe9d42960764bf99cfaf729d8956 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Thu, 20 Sep 2012 12:03:12 -0400 Subject: [PATCH 06/46] Added a performance test --- authority/tests.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/authority/tests.py b/authority/tests.py index 84b27fc..63a7110 100644 --- a/authority/tests.py +++ b/authority/tests.py @@ -148,3 +148,30 @@ class AssignExceptionsTest(TestCase): except NotAModel: return True self.fail() + + +class PerformanceTest(TestCase): + """ + Tests that permission are actually cached and that the number of queries + stays constant. + """ + fixtures = ['tests.json'] + + def setUp(self): + self.user = User.objects.get(username='jezdez') + self.check = UserPermission(self.user) + + def test_has_user_perms(self): + # Show that when calling has_user_perms multiple times no additional + # queries are done. + + # Make sure the has_user_perms check does not get short-circuited. + assert not self.user.is_superuser + assert self.user.is_active + + # Regardless of how many times has_user_perms is called, the number of + # queries is the same. + with self.assertNumQueries(1): + self.check.has_user_perms('foo', self.user, True, True) + self.check.has_user_perms('foo', self.user, True, True) + self.check.has_user_perms('foo', self.user, True, True) From e49523dace30721ebc04c45c4ea50915d441ded1 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Thu, 20 Sep 2012 14:52:01 -0400 Subject: [PATCH 07/46] prepended 'base' to all new cache stuff in the base permission class --- authority/permissions.py | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index 36d165d..617165b 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -40,10 +40,10 @@ class BasePermission(object): self.group = group super(BasePermission, self).__init__(*args, **kwargs) - self._permission_cache_filled = False - self._cached_permissions = {} + self._base_permission_cache_filled = False + self._base_cached_permissions = {} - def _get_permissions(self): + def _get_base_permissions(self): perms = Permission.objects.filter( user=self.user, ) @@ -55,13 +55,13 @@ class BasePermission(object): return permissions @property - def cached_permissions(self): - if self._permission_cache_filled: - return self._cached_permissions - self._cached_permissions = self._get_permissions() + def base_cached_permissions(self): + if self._base_permission_cache_filled: + return self._base_cached_permissions + self._base_cached_permissions = self._get_base_permissions() - self._permission_cache_filled = True - return self._cached_permissions + self._base_permission_cache_filled = True + return self._base_cached_permissions def has_user_perms(self, perm, obj, approved, check_groups=True): if self.user: @@ -70,12 +70,14 @@ class BasePermission(object): if not self.user.is_active: return False # check if a Permission object exists for the given params - perm = self.cached_permissions.get(( + cached_permissions = self.base_cached_permissions + cached_perm = cached_permissions.get(( obj.pk, perm, approved, )) - if perm: + + if cached_perm: return True return False return False From 3e403abb1cb85e1cc87387b2c8ed8891251ac414 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Thu, 20 Sep 2012 15:19:06 -0400 Subject: [PATCH 08/46] removed base prefix --- authority/permissions.py | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index 617165b..0b93d23 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -40,10 +40,10 @@ class BasePermission(object): self.group = group super(BasePermission, self).__init__(*args, **kwargs) - self._base_permission_cache_filled = False - self._base_cached_permissions = {} + self._permission_cache_filled = False + self._cached_permissions = {} - def _get_base_permissions(self): + def _get_permissions(self): perms = Permission.objects.filter( user=self.user, ) @@ -55,13 +55,16 @@ class BasePermission(object): return permissions @property - def base_cached_permissions(self): - if self._base_permission_cache_filled: - return self._base_cached_permissions - self._base_cached_permissions = self._get_base_permissions() + def cached_permissions(self): + if self._permission_cache_filled: + return self._cached_permissions + self._cached_permissions = self._get_permissions() - self._base_permission_cache_filled = True - return self._base_cached_permissions + self._permission_cache_filled = True + return self._cached_permissions + + def invalidate_cache(self): + self._permission_cache_filled = False def has_user_perms(self, perm, obj, approved, check_groups=True): if self.user: @@ -70,7 +73,7 @@ class BasePermission(object): if not self.user.is_active: return False # check if a Permission object exists for the given params - cached_permissions = self.base_cached_permissions + cached_permissions = self.cached_permissions cached_perm = cached_permissions.get(( obj.pk, perm, From 9fe2ad5a0d0478a0e6fb2c4fb4c816787f690e51 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Mon, 24 Sep 2012 11:55:19 -0400 Subject: [PATCH 09/46] refs #1: added a git ignore file --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .gitignore diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..6592d0b --- /dev/null +++ b/.gitignore @@ -0,0 +1,3 @@ +*.pyc +*.egg-info +*.sql From c091a91412fe583d208cbda4bfa9c2204c0e4815 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Mon, 24 Sep 2012 12:09:25 -0400 Subject: [PATCH 10/46] refs #2: added docstrings and comments --- authority/permissions.py | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/authority/permissions.py b/authority/permissions.py index 0b93d23..bbd2f18 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -40,30 +40,49 @@ class BasePermission(object): self.group = group super(BasePermission, self).__init__(*args, **kwargs) + # Define variables needed for smart cache. self._permission_cache_filled = False self._cached_permissions = {} def _get_permissions(self): + """ + Return a dictionary representation of the Permission objects that are + related to ``self.user``. + """ perms = Permission.objects.filter( user=self.user, ) # Pre cache all the permission in a dictionary. permissions = {} for perm in perms: - # Not currently handling groups. + # TODO Not currently handling groups. permissions[(perm.object_id, perm.codename, perm.approved)] = perm return permissions @property def cached_permissions(self): + """ + cached_permissions will generate the cache in a lazy fashion. + """ + + # Check to see if the cache has been primed. if self._permission_cache_filled: return self._cached_permissions + + # Prime the cache. self._cached_permissions = self._get_permissions() self._permission_cache_filled = True return self._cached_permissions def invalidate_cache(self): + """ + In the event that the Permission table is changed during the use of a + permission the Permission cache will need to be invalidated and + regenerated. By calling this method the invalidation will occur, and + the next time the cached_permissions is used the cache will be + re-primed. + """ self._permission_cache_filled = False def has_user_perms(self, perm, obj, approved, check_groups=True): From ca3963ea7b129176e1d76f3acacc5ea4a9bb1f1b Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Mon, 24 Sep 2012 12:09:59 -0400 Subject: [PATCH 11/46] refs #2: properly tested invalidate_cache --- authority/tests.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/authority/tests.py b/authority/tests.py index 63a7110..b9baa79 100644 --- a/authority/tests.py +++ b/authority/tests.py @@ -175,3 +175,15 @@ class PerformanceTest(TestCase): self.check.has_user_perms('foo', self.user, True, True) self.check.has_user_perms('foo', self.user, True, True) self.check.has_user_perms('foo', self.user, True, True) + + def test_invalidate_cache(self): + # Show that calling invalidate_cache will cause extra queries. + with self.assertNumQueries(2): + self.check.has_user_perms('foo', self.user, True, True) + + # Invalidate the cache to show that a query will be generated when + # checking perms again. + self.check.invalidate_cache() + + # One query to re generate the cache. + self.check.has_user_perms('foo', self.user, True, True) From ccd3417c99964c8322173b7df44e2991c2e0c15d Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Mon, 24 Sep 2012 12:36:30 -0400 Subject: [PATCH 12/46] refs #3: added a unit test showing that groups are broken --- authority/tests.py | 65 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 64 insertions(+), 1 deletion(-) diff --git a/authority/tests.py b/authority/tests.py index 63a7110..b9b7ec8 100644 --- a/authority/tests.py +++ b/authority/tests.py @@ -1,5 +1,5 @@ from django.test import TestCase -from django.contrib.auth.models import User +from django.contrib.auth.models import User, Group from django.contrib.auth.models import Permission as DjangoPermission import authority @@ -175,3 +175,66 @@ class PerformanceTest(TestCase): self.check.has_user_perms('foo', self.user, True, True) self.check.has_user_perms('foo', self.user, True, True) self.check.has_user_perms('foo', self.user, True, True) + + +class ExpectedBehaviourTestCase(TestCase): + """ + Tests that peg expected behaviour + """ + fixtures = ['tests.json'] + + def setUp(self): + self.user = User.objects.get(username='jezdez') + self.check = UserPermission(self.user) + + def _old_permission_check(self): + # This is what the old, pre-cache system would check to see if a user + # had a given permission. + return Permission.objects.user_permissions( + self.user, + 'foo', + self.user, + approved=True, + check_groups=True, + ) + + def test_has_user_perms_with_groups(self): + perms = self._old_permission_check() + self.assertEqual([], list(perms)) + + # Use the new cached user perms to show that the user does not have the + # perms. + can_foo_with_group = self.check.has_user_perms( + 'foo', + self.user, + approved=True, + check_groups=True, + ) + self.assertFalse(can_foo_with_group) + + # Create a group that the user is a part of. + group = Group.objects.create() + group.user_set.add(self.user) + + # Create a permission with just that group. + perm = Permission.objects.create( + content_type=Permission.objects.get_content_type(User), + object_id=self.user.pk, + codename='foo', + group=group, + approved=True, + ) + + # Old permission check + perms = self._old_permission_check() + self.assertEqual([perm], list(perms)) + + # Invalidate the cache. + self.check.invalidate_cache() + can_foo_with_group = self.check.has_user_perms( + 'foo', + self.user, + approved=True, + check_groups=True, + ) + self.assertTrue(can_foo_with_group) From 03bd5c9445a64b599aaf2762060f4f4ffe07dd98 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Mon, 24 Sep 2012 13:34:35 -0400 Subject: [PATCH 13/46] refs #3: made a distinction for the cache that it was a group free cache --- authority/permissions.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index 0b93d23..fe3f897 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -40,10 +40,10 @@ class BasePermission(object): self.group = group super(BasePermission, self).__init__(*args, **kwargs) - self._permission_cache_filled = False - self._cached_permissions = {} + self._permission_cache_filled_no_groups = False + self._cached_permissions_no_groups = {} - def _get_permissions(self): + def _get_permissions_no_groups(self): perms = Permission.objects.filter( user=self.user, ) @@ -55,16 +55,16 @@ class BasePermission(object): return permissions @property - def cached_permissions(self): - if self._permission_cache_filled: - return self._cached_permissions - self._cached_permissions = self._get_permissions() + def cached_permissions_no_groups(self): + if self._permission_cache_filled_no_groups: + return self._cached_permissions_no_groups + self._cached_permissions_no_groups = self._get_permissions_no_groups() - self._permission_cache_filled = True - return self._cached_permissions + self._permission_cache_filled_no_groups = True + return self._cached_permissions_no_groups def invalidate_cache(self): - self._permission_cache_filled = False + self._permission_cache_filled_no_groups = False def has_user_perms(self, perm, obj, approved, check_groups=True): if self.user: @@ -73,8 +73,8 @@ class BasePermission(object): if not self.user.is_active: return False # check if a Permission object exists for the given params - cached_permissions = self.cached_permissions - cached_perm = cached_permissions.get(( + cached_permissions_no_groups = self.cached_permissions_no_groups + cached_perm = cached_permissions_no_groups.get(( obj.pk, perm, approved, From c94b9c3930f71d95083bcd1b81dcc31606106436 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Mon, 24 Sep 2012 13:43:54 -0400 Subject: [PATCH 14/46] refs #3: Added a cache for group related permissions. --- authority/permissions.py | 39 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index fe3f897..cedfff3 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -1,3 +1,4 @@ +from django.db.models import Q from django.db.models.base import Model, ModelBase from django.template.defaultfilters import slugify from django.contrib.auth.models import Permission as DjangoPermission @@ -43,6 +44,9 @@ class BasePermission(object): self._permission_cache_filled_no_groups = False self._cached_permissions_no_groups = {} + self._permission_cache_filled_with_groups = False + self._cached_permissions_with_groups = {} + def _get_permissions_no_groups(self): perms = Permission.objects.filter( user=self.user, @@ -54,6 +58,17 @@ class BasePermission(object): permissions[(perm.object_id, perm.codename, perm.approved)] = perm return permissions + def _get_permissions_with_groups(self): + perms = Permission.objects.filter( + Q(user__pk=self.user.pk) | Q(group__in=self.user.groups.all()), + ) + # Pre cache all the permission in a dictionary. + permissions = {} + for perm in perms: + # Not currently handling groups. + permissions[(perm.object_id, perm.codename, perm.approved)] = perm + return permissions + @property def cached_permissions_no_groups(self): if self._permission_cache_filled_no_groups: @@ -63,8 +78,19 @@ class BasePermission(object): self._permission_cache_filled_no_groups = True return self._cached_permissions_no_groups + @property + def cached_permissions_with_groups(self): + if self._permission_cache_filled_with_groups: + return self._cached_permissions_with_groups + self._cached_permissions_with_groups = \ + self._get_permissions_with_groups() + + self._permission_cache_filled_with_groups = True + return self._cached_permissions_with_groups + def invalidate_cache(self): self._permission_cache_filled_no_groups = False + self._permission_cache_filled_with_groups = False def has_user_perms(self, perm, obj, approved, check_groups=True): if self.user: @@ -72,9 +98,16 @@ class BasePermission(object): return True if not self.user.is_active: return False - # check if a Permission object exists for the given params - cached_permissions_no_groups = self.cached_permissions_no_groups - cached_perm = cached_permissions_no_groups.get(( + + # The permissions cache is different if groups need to be checked + # as well. + if check_groups: + cached_permissions = self.cached_permissions_with_groups + else: + cached_permissions = self.cached_permissions_no_groups + + # Check to see if the permission is in the cache. + cached_perm = cached_permissions.get(( obj.pk, perm, approved, From d27314ce272a5799623b1916f0452fe95a1289cd Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Mon, 24 Sep 2012 13:57:12 -0400 Subject: [PATCH 15/46] added docs/build/* to the .gitignore --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 6592d0b..96d3622 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ *.pyc *.egg-info *.sql +docs/build/* From 079b1a1595db2eb640636bda3a2eccb5f03d506b Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Mon, 24 Sep 2012 14:20:00 -0400 Subject: [PATCH 16/46] refs #4: made a base test case for smart caching --- authority/tests.py | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/authority/tests.py b/authority/tests.py index 9ab0266..388f213 100644 --- a/authority/tests.py +++ b/authority/tests.py @@ -150,10 +150,9 @@ class AssignExceptionsTest(TestCase): self.fail() -class PerformanceTest(TestCase): +class SmartCacheingTestCase(TestCase): """ - Tests that permission are actually cached and that the number of queries - stays constant. + The base test case for all tests that have to do with smart caching. """ fixtures = ['tests.json'] @@ -161,6 +160,24 @@ class PerformanceTest(TestCase): self.user = User.objects.get(username='jezdez') self.check = UserPermission(self.user) + def _old_permission_check(self): + # This is what the old, pre-cache system would check to see if a user + # had a given permission. + return Permission.objects.user_permissions( + self.user, + 'foo', + self.user, + approved=True, + check_groups=True, + ) + + +class PerformanceTest(SmartCacheingTestCase): + """ + Tests that permission are actually cached and that the number of queries + stays constant. + """ + def test_has_user_perms(self): # Show that when calling has_user_perms multiple times no additional # queries are done. @@ -189,26 +206,10 @@ class PerformanceTest(TestCase): self.check.has_user_perms('foo', self.user, True, True) -class ExpectedBehaviourTestCase(TestCase): +class ExpectedBehaviourTestCase(SmartCacheingTestCase): """ Tests that peg expected behaviour """ - fixtures = ['tests.json'] - - def setUp(self): - self.user = User.objects.get(username='jezdez') - self.check = UserPermission(self.user) - - def _old_permission_check(self): - # This is what the old, pre-cache system would check to see if a user - # had a given permission. - return Permission.objects.user_permissions( - self.user, - 'foo', - self.user, - approved=True, - check_groups=True, - ) def test_has_user_perms_with_groups(self): perms = self._old_permission_check() From 7744fac9f34c0371ba59f56c7269cddfa381c2de Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Mon, 24 Sep 2012 14:28:57 -0400 Subject: [PATCH 17/46] refs #4: The smart cache is now only being done if the correct settings is set. --- authority/permissions.py | 48 ++++++++++++++++++++++++++-------------- authority/tests.py | 7 ++++-- 2 files changed, 36 insertions(+), 19 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index 06a79c3..19eef2b 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -1,8 +1,9 @@ +from django.conf import settings +from django.contrib.auth.models import Permission as DjangoPermission +from django.contrib.contenttypes.models import ContentType from django.db.models import Q from django.db.models.base import Model, ModelBase from django.template.defaultfilters import slugify -from django.contrib.auth.models import Permission as DjangoPermission -from django.contrib.contenttypes.models import ContentType from authority.exceptions import NotAModel, UnsavedModelInstance from authority.models import Permission @@ -123,23 +124,36 @@ class BasePermission(object): if not self.user.is_active: return False - # The permissions cache is different if groups need to be checked - # as well. - if check_groups: - cached_permissions = self.cached_permissions_with_groups + # AUTHORITY_USE_SMART_CACHE defaults to False to maintain backwards + # compatibility. + if getattr(settings, 'AUTHORITY_USE_SMART_CACHE', False): + # The permissions cache is different if groups need to be + # checked as well. + if check_groups: + cached_permissions = self.cached_permissions_with_groups + else: + cached_permissions = self.cached_permissions_no_groups + + # Check to see if the permission is in the cache. + cached_perm = cached_permissions.get(( + obj.pk, + perm, + approved, + )) + if cached_perm: + return True + return False else: - cached_permissions = self.cached_permissions_no_groups + return Permission.objects.user_permissions( + self.user, + perm, + obj, + approved, + check_groups, + ).filter( + object_id=obj.id, + ).exists() - # Check to see if the permission is in the cache. - cached_perm = cached_permissions.get(( - obj.pk, - perm, - approved, - )) - - if cached_perm: - return True - return False return False def has_group_perms(self, perm, obj, approved): diff --git a/authority/tests.py b/authority/tests.py index 388f213..ae2b6c4 100644 --- a/authority/tests.py +++ b/authority/tests.py @@ -1,6 +1,7 @@ -from django.test import TestCase -from django.contrib.auth.models import User, Group +from django.conf import settings from django.contrib.auth.models import Permission as DjangoPermission +from django.contrib.auth.models import User, Group +from django.test import TestCase import authority from authority import permissions @@ -160,6 +161,8 @@ class SmartCacheingTestCase(TestCase): self.user = User.objects.get(username='jezdez') self.check = UserPermission(self.user) + settings.AUTHORITY_USE_SMART_CACHE = True + def _old_permission_check(self): # This is what the old, pre-cache system would check to see if a user # had a given permission. From 23814d6e72394b3b312954f751f6bdb2d973ac7b Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Mon, 24 Sep 2012 14:33:51 -0400 Subject: [PATCH 18/46] refs #4: added an update note, and added authors --- AUTHORS | 5 ++++- README | 8 ++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/AUTHORS b/AUTHORS index 56d0532..8192db5 100644 --- a/AUTHORS +++ b/AUTHORS @@ -1,4 +1,7 @@ Jannis Leidel Martin Mahner Diego Búrigo Zacarão -James Pic \ No newline at end of file +James Pic +Wes Winham +Kyle Gibson +Jason Ward diff --git a/README b/README index 3ff0ede..0040d13 100644 --- a/README +++ b/README @@ -52,6 +52,14 @@ html version using the setup.py:: Changelog: ========== +0.5dev (2012-09-24): +----------------- + +* It is now possible to minimize the number of queries when using + django-authority by caching the results of the Permission query. This can be + done by adding ``AUTHORITY_USE_SMART_CACHE = True`` to your settings.py + + 0.4 (2010-01-15): ----------------- From 77915643d3eb454b0f8d23e6f1333b156a003d6c Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Mon, 24 Sep 2012 15:23:07 -0400 Subject: [PATCH 19/46] refs #3: changed the name of the test case --- authority/tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authority/tests.py b/authority/tests.py index 9ab0266..c60a151 100644 --- a/authority/tests.py +++ b/authority/tests.py @@ -189,7 +189,7 @@ class PerformanceTest(TestCase): self.check.has_user_perms('foo', self.user, True, True) -class ExpectedBehaviourTestCase(TestCase): +class GroupPermissionCacheTestCase(TestCase): """ Tests that peg expected behaviour """ From cb7b25797272c0b36c4080e5cf7350274153d07c Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Mon, 24 Sep 2012 15:27:32 -0400 Subject: [PATCH 20/46] refs #3: name changes --- authority/permissions.py | 44 ++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index 06a79c3..05156f9 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -42,13 +42,13 @@ class BasePermission(object): super(BasePermission, self).__init__(*args, **kwargs) # Define variables needed for smart cache. - self._permission_cache_filled_no_groups = False - self._cached_permissions_no_groups = {} + self._user_permissions_cache_filled = False + self._cached_user_permissions = {} - self._permission_cache_filled_with_groups = False - self._cached_permissions_with_groups = {} + self._group_permissions_cache_filled = False + self._cached_group_permissions = {} - def _get_permissions_no_groups(self): + def _get_cached_user_permissions(self): """ Return a dictionary representation of the Permission objects that are related to ``self.user``, excluding group interactions. @@ -62,7 +62,7 @@ class BasePermission(object): permissions[(perm.object_id, perm.codename, perm.approved)] = perm return permissions - def _get_permissions_with_groups(self): + def _get_cached_group_permissions(self): """ Return a dictionary representation of the Permission objects that are related to ``self.user``, including groups interactions. @@ -77,33 +77,33 @@ class BasePermission(object): return permissions @property - def cached_permissions_no_groups(self): + def cached_user_permissions(self): """ cached_permissions will generate the cache in a lazy fashion. """ # Check to see if the cache has been primed. - if self._permission_cache_filled_no_groups: - return self._cached_permissions_no_groups + if self._user_permissions_cache_filled: + return self._cached_user_permissions # Prime the cache. - self._cached_permissions_no_groups = self._get_permissions_no_groups() - self._permission_cache_filled_no_groups = True - return self._cached_permissions_no_groups + self._cached_user_permissions = self._get_cached_user_permissions() + self._user_permissions_cache_filled = True + return self._cached_user_permissions @property - def cached_permissions_with_groups(self): + def cached_group_permissions(self): """ cached_permissions will generate the cache in a lazy fashion. """ # Check to see if the cache has been primed. - if self._permission_cache_filled_with_groups: - return self._cached_permissions_with_groups + if self._group_permissions_cache_filled: + return self._cached_group_permissions # Prime the cache. - self._cached_permissions_with_groups = \ - self._get_permissions_with_groups() - self._permission_cache_filled_with_groups = True - return self._cached_permissions_with_groups + self._cached_group_permissions = \ + self._get_cached_group_permissions() + self._group_permissions_cache_filled = True + return self._cached_group_permissions def invalidate_cache(self): """ @@ -114,7 +114,7 @@ class BasePermission(object): re-primed. """ self._permission_cache_filled_no_groups = False - self._permission_cache_filled_with_groups = False + self._group_permissions_cache_filled = False def has_user_perms(self, perm, obj, approved, check_groups=True): if self.user: @@ -126,9 +126,9 @@ class BasePermission(object): # The permissions cache is different if groups need to be checked # as well. if check_groups: - cached_permissions = self.cached_permissions_with_groups + cached_permissions = self.cached_group_permissions else: - cached_permissions = self.cached_permissions_no_groups + cached_permissions = self.cached_user_permissions # Check to see if the permission is in the cache. cached_perm = cached_permissions.get(( From ddd4a9c35a06f2ca7e4755caa0b2cec9c825106f Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Mon, 24 Sep 2012 15:45:42 -0400 Subject: [PATCH 21/46] refs #3: split up user and group permissions completely. --- authority/permissions.py | 52 +++++++++++++++++++++++++--------------- authority/tests.py | 19 ++++++++------- 2 files changed, 43 insertions(+), 28 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index 05156f9..b23a9df 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -1,4 +1,3 @@ -from django.db.models import Q from django.db.models.base import Model, ModelBase from django.template.defaultfilters import slugify from django.contrib.auth.models import Permission as DjangoPermission @@ -59,7 +58,12 @@ class BasePermission(object): # Pre cache all the permission in a dictionary. permissions = {} for perm in perms: - permissions[(perm.object_id, perm.codename, perm.approved)] = perm + permissions[( + perm.object_id, + perm.content_type.pk, + perm.codename, + perm.approved, + )] = perm return permissions def _get_cached_group_permissions(self): @@ -68,12 +72,17 @@ class BasePermission(object): related to ``self.user``, including groups interactions. """ perms = Permission.objects.filter( - Q(user__pk=self.user.pk) | Q(group__in=self.user.groups.all()), + group__in=self.user.groups.all(), ) # Pre cache all the permission in a dictionary. permissions = {} for perm in perms: - permissions[(perm.object_id, perm.codename, perm.approved)] = perm + permissions[( + perm.object_id, + perm.content_type.pk, + perm.codename, + perm.approved, + )] = perm return permissions @property @@ -105,7 +114,7 @@ class BasePermission(object): self._group_permissions_cache_filled = True return self._cached_group_permissions - def invalidate_cache(self): + def invalidate_permissions_cache(self): """ In the event that the Permission table is changed during the use of a permission the Permission cache will need to be invalidated and @@ -113,7 +122,7 @@ class BasePermission(object): the next time the cached_permissions is used the cache will be re-primed. """ - self._permission_cache_filled_no_groups = False + self._user_permissions_cache_filled = False self._group_permissions_cache_filled = False def has_user_perms(self, perm, obj, approved, check_groups=True): @@ -123,22 +132,27 @@ class BasePermission(object): if not self.user.is_active: return False - # The permissions cache is different if groups need to be checked - # as well. - if check_groups: - cached_permissions = self.cached_group_permissions - else: - cached_permissions = self.cached_user_permissions + def _user_has_perms(cached_perms): + # Check to see if the permission is in the cache. + return cached_perms.get(( + obj.pk, + Permission.objects.get_content_type(obj).pk, + perm, + approved, + )) + # Check the permissions on the user. + cached_user_permissions = self.cached_user_permissions # Check to see if the permission is in the cache. - cached_perm = cached_permissions.get(( - obj.pk, - perm, - approved, - )) - - if cached_perm: + if _user_has_perms(cached_user_permissions): return True + + # Optionally check group permissions + if check_groups: + cached_group_permissions = self.cached_group_permissions + if _user_has_perms(cached_group_permissions): + return True + return False return False diff --git a/authority/tests.py b/authority/tests.py index c60a151..1b8b3ac 100644 --- a/authority/tests.py +++ b/authority/tests.py @@ -172,21 +172,22 @@ class PerformanceTest(TestCase): # Regardless of how many times has_user_perms is called, the number of # queries is the same. with self.assertNumQueries(1): - self.check.has_user_perms('foo', self.user, True, True) - self.check.has_user_perms('foo', self.user, True, True) - self.check.has_user_perms('foo', self.user, True, True) + self.check.has_user_perms('foo', self.user, True, False) + self.check.has_user_perms('foo', self.user, True, False) + self.check.has_user_perms('foo', self.user, True, False) - def test_invalidate_cache(self): - # Show that calling invalidate_cache will cause extra queries. + def test_invalidate_permissions_cache(self): + # Show that calling invalidate_permissions_cache will cause extra + # queries. with self.assertNumQueries(2): - self.check.has_user_perms('foo', self.user, True, True) + self.check.has_user_perms('foo', self.user, True, False) # Invalidate the cache to show that a query will be generated when # checking perms again. - self.check.invalidate_cache() + self.check.invalidate_permissions_cache() # One query to re generate the cache. - self.check.has_user_perms('foo', self.user, True, True) + self.check.has_user_perms('foo', self.user, True, False) class GroupPermissionCacheTestCase(TestCase): @@ -242,7 +243,7 @@ class GroupPermissionCacheTestCase(TestCase): self.assertEqual([perm], list(perms)) # Invalidate the cache. - self.check.invalidate_cache() + self.check.invalidate_permissions_cache() can_foo_with_group = self.check.has_user_perms( 'foo', self.user, From a519da2fdd0ce1e660abcd8e0c6f7e7b614916ae Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Mon, 24 Sep 2012 16:39:33 -0400 Subject: [PATCH 22/46] refs #3: stopped storing the whole perm object in the cache, all we care at this point is if the perm existed in that specific way. --- authority/permissions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index b23a9df..0dc1467 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -63,7 +63,7 @@ class BasePermission(object): perm.content_type.pk, perm.codename, perm.approved, - )] = perm + )] = True return permissions def _get_cached_group_permissions(self): @@ -82,7 +82,7 @@ class BasePermission(object): perm.content_type.pk, perm.codename, perm.approved, - )] = perm + )] = True return permissions @property From 8f1d2d6a442091dc9d3f75fd135cc82c1049c6eb Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Mon, 24 Sep 2012 17:17:32 -0400 Subject: [PATCH 23/46] refs #3: moved the cached permissions onto the user object (just like django does it) --- authority/permissions.py | 42 ++++++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index 0dc1467..c2d1039 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -40,13 +40,6 @@ class BasePermission(object): self.group = group super(BasePermission, self).__init__(*args, **kwargs) - # Define variables needed for smart cache. - self._user_permissions_cache_filled = False - self._cached_user_permissions = {} - - self._group_permissions_cache_filled = False - self._cached_group_permissions = {} - def _get_cached_user_permissions(self): """ Return a dictionary representation of the Permission objects that are @@ -91,13 +84,19 @@ class BasePermission(object): cached_permissions will generate the cache in a lazy fashion. """ # Check to see if the cache has been primed. - if self._user_permissions_cache_filled: - return self._cached_user_permissions + cache_filled = getattr( + self.user, + '_user_permissions_cache_filled', + False, + ) + if cache_filled: + return self.user._cached_user_permissions # Prime the cache. - self._cached_user_permissions = self._get_cached_user_permissions() - self._user_permissions_cache_filled = True - return self._cached_user_permissions + self.user._cached_user_permissions = \ + self._get_cached_user_permissions() + self.user._user_permissions_cache_filled = True + return self.user._cached_user_permissions @property def cached_group_permissions(self): @@ -105,14 +104,19 @@ class BasePermission(object): cached_permissions will generate the cache in a lazy fashion. """ # Check to see if the cache has been primed. - if self._group_permissions_cache_filled: - return self._cached_group_permissions + cache_filled = getattr( + self.user, + '_group_permissions_cache_filled', + False, + ) + if cache_filled: + return self.user._cached_group_permissions # Prime the cache. - self._cached_group_permissions = \ + self.user._cached_group_permissions = \ self._get_cached_group_permissions() - self._group_permissions_cache_filled = True - return self._cached_group_permissions + self.user._group_permissions_cache_filled = True + return self.user._cached_group_permissions def invalidate_permissions_cache(self): """ @@ -122,8 +126,8 @@ class BasePermission(object): the next time the cached_permissions is used the cache will be re-primed. """ - self._user_permissions_cache_filled = False - self._group_permissions_cache_filled = False + self.user._user_permissions_cache_filled = False + self.user._group_permissions_cache_filled = False def has_user_perms(self, perm, obj, approved, check_groups=True): if self.user: From 485731f3cc3d5bc79d060ba8d65233554d785033 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Mon, 24 Sep 2012 17:20:37 -0400 Subject: [PATCH 24/46] refs #3: name change, we are more like Django now --- authority/permissions.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index c2d1039..0338644 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -90,13 +90,15 @@ class BasePermission(object): False, ) if cache_filled: - return self.user._cached_user_permissions + # Don't really like the name for this, but this matches how Django + # does it. + return self.user._authority_perm_cache # Prime the cache. - self.user._cached_user_permissions = \ + self.user._authority_perm_cache = \ self._get_cached_user_permissions() self.user._user_permissions_cache_filled = True - return self.user._cached_user_permissions + return self.user._authority_perm_cache @property def cached_group_permissions(self): @@ -110,13 +112,13 @@ class BasePermission(object): False, ) if cache_filled: - return self.user._cached_group_permissions + return self.user._authroity_group_perm_cache # Prime the cache. - self.user._cached_group_permissions = \ + self.user._authroity_group_perm_cache = \ self._get_cached_group_permissions() self.user._group_permissions_cache_filled = True - return self.user._cached_group_permissions + return self.user._authroity_group_perm_cache def invalidate_permissions_cache(self): """ From 1818f5885a498af2be1370b1f9b076b4e8f0cd01 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Tue, 25 Sep 2012 12:34:56 -0400 Subject: [PATCH 25/46] refs #3: prime both user and group cache at the same time, reduced the number of queries by 1 --- authority/permissions.py | 55 ++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index 0338644..c499de6 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -1,3 +1,4 @@ +from django.db.models import Q from django.db.models.base import Model, ModelBase from django.template.defaultfilters import slugify from django.contrib.auth.models import Permission as DjangoPermission @@ -40,23 +41,40 @@ class BasePermission(object): self.group = group super(BasePermission, self).__init__(*args, **kwargs) + def _get_cached_permissions(self): + """ + Set up both the user and group caches. + """ + perms = Permission.objects.filter( + Q(user__pk=self.user.pk) | Q(group__in=self.user.groups.all()), + ).select_related( + 'group', + ) + user_permissions = {} + group_permissions = {} + for perm in perms: + if perm.user == self.user: + user_permissions[( + perm.object_id, + perm.content_type.pk, + perm.codename, + perm.approved, + )] = True + if perm.group in self.user.groups.all(): + group_permissions[( + perm.object_id, + perm.content_type.pk, + perm.codename, + perm.approved, + )] = True + return user_permissions, group_permissions + def _get_cached_user_permissions(self): """ Return a dictionary representation of the Permission objects that are related to ``self.user``, excluding group interactions. """ - perms = Permission.objects.filter( - user=self.user, - ) - # Pre cache all the permission in a dictionary. - permissions = {} - for perm in perms: - permissions[( - perm.object_id, - perm.content_type.pk, - perm.codename, - perm.approved, - )] = True + permissions, _ = self._get_cached_permissions() return permissions def _get_cached_group_permissions(self): @@ -64,18 +82,7 @@ class BasePermission(object): Return a dictionary representation of the Permission objects that are related to ``self.user``, including groups interactions. """ - perms = Permission.objects.filter( - group__in=self.user.groups.all(), - ) - # Pre cache all the permission in a dictionary. - permissions = {} - for perm in perms: - permissions[( - perm.object_id, - perm.content_type.pk, - perm.codename, - perm.approved, - )] = True + _, permissions = self._get_cached_permissions() return permissions @property From 667ef532c8bee82492ab89694c3c8b5be87d9659 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Tue, 25 Sep 2012 12:49:16 -0400 Subject: [PATCH 26/46] refs #3: added some select related and kept the query count down --- authority/permissions.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index c499de6..f669f19 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -53,17 +53,17 @@ class BasePermission(object): user_permissions = {} group_permissions = {} for perm in perms: - if perm.user == self.user: + if perm.user_id == self.user.pk: user_permissions[( perm.object_id, - perm.content_type.pk, + perm.content_type_id, perm.codename, perm.approved, )] = True - if perm.group in self.user.groups.all(): + if perm.group and self.user in perm.group.user_set.all(): group_permissions[( perm.object_id, - perm.content_type.pk, + perm.content_type_id, perm.codename, perm.approved, )] = True From 289e9f0bdff55e0bc1272f6fcc9de662c97733c6 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Tue, 25 Sep 2012 13:58:37 -0400 Subject: [PATCH 27/46] refs #4: default to using the smart cache --- authority/permissions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index 68764f6..c94afbd 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -143,7 +143,7 @@ class BasePermission(object): def use_smart_cache(self): # AUTHORITY_USE_SMART_CACHE defaults to False to maintain backwards # compatibility. - return getattr(settings, 'AUTHORITY_USE_SMART_CACHE', False) + return getattr(settings, 'AUTHORITY_USE_SMART_CACHE', True) def has_user_perms(self, perm, obj, approved, check_groups=True): if self.user: @@ -183,7 +183,7 @@ class BasePermission(object): approved, check_groups, ).filter( - object_id=obj.id, + object_id=obj.pk, ).exists() return False From 6ff9c0f1966dbf58514a75299db65b629218059b Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Tue, 25 Sep 2012 16:08:09 -0400 Subject: [PATCH 28/46] refs #6: updated the tests so that there was a permission that did not have a user --- authority/tests.py | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/authority/tests.py b/authority/tests.py index aa9b33c..46406bd 100644 --- a/authority/tests.py +++ b/authority/tests.py @@ -15,6 +15,12 @@ class UserPermission(permissions.BasePermission): authority.register(User, UserPermission) +class GroupPermission(permissions.BasePermission): + checks = ('browse',) + label = 'group_permission' +authority.register(Group, GroupPermission) + + class BehaviourTest(TestCase): """ self.user will be given: @@ -158,9 +164,18 @@ class SmartCacheingTestCase(TestCase): fixtures = ['tests.json'] def setUp(self): + # Create a user. self.user = User.objects.get(username='jezdez') - self.check = UserPermission(self.user) + # Create a group. + self.group = Group.objects.create() + self.group.user_set.add(self.user) + + # Make the checks + self.user_check = UserPermission(user=self.user) + self.group_check = GroupPermission(group=self.group) + + # Ensure we are using the smart cache. settings.AUTHORITY_USE_SMART_CACHE = True def _old_permission_check(self): @@ -192,22 +207,22 @@ class PerformanceTest(SmartCacheingTestCase): # Regardless of how many times has_user_perms is called, the number of # queries is the same. with self.assertNumQueries(1): - self.check.has_user_perms('foo', self.user, True, False) - self.check.has_user_perms('foo', self.user, True, False) - self.check.has_user_perms('foo', self.user, True, False) + self.user_check.has_user_perms('foo', self.user, True, False) + self.user_check.has_user_perms('foo', self.user, True, False) + self.user_check.has_user_perms('foo', self.user, True, False) def test_invalidate_permissions_cache(self): # Show that calling invalidate_permissions_cache will cause extra # queries. with self.assertNumQueries(2): - self.check.has_user_perms('foo', self.user, True, False) + self.user_check.has_user_perms('foo', self.user, True, False) # Invalidate the cache to show that a query will be generated when # checking perms again. - self.check.invalidate_permissions_cache() + self.user_check.invalidate_permissions_cache() # One query to re generate the cache. - self.check.has_user_perms('foo', self.user, True, False) + self.user_check.has_user_perms('foo', self.user, True, False) class GroupPermissionCacheTestCase(SmartCacheingTestCase): @@ -221,7 +236,7 @@ class GroupPermissionCacheTestCase(SmartCacheingTestCase): # Use the new cached user perms to show that the user does not have the # perms. - can_foo_with_group = self.check.has_user_perms( + can_foo_with_group = self.user_check.has_user_perms( 'foo', self.user, approved=True, @@ -229,16 +244,12 @@ class GroupPermissionCacheTestCase(SmartCacheingTestCase): ) self.assertFalse(can_foo_with_group) - # Create a group that the user is a part of. - group = Group.objects.create() - group.user_set.add(self.user) - # Create a permission with just that group. perm = Permission.objects.create( content_type=Permission.objects.get_content_type(User), object_id=self.user.pk, codename='foo', - group=group, + group=self.group, approved=True, ) @@ -247,8 +258,8 @@ class GroupPermissionCacheTestCase(SmartCacheingTestCase): self.assertEqual([perm], list(perms)) # Invalidate the cache. - self.check.invalidate_permissions_cache() - can_foo_with_group = self.check.has_user_perms( + self.user_check.invalidate_permissions_cache() + can_foo_with_group = self.user_check.has_user_perms( 'foo', self.user, approved=True, From de45b9535e2404b3a1a504650719eb08575e6ad4 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Tue, 25 Sep 2012 16:43:05 -0400 Subject: [PATCH 29/46] refs #6: no longer relying on self.user being set --- authority/permissions.py | 14 +++++++++++--- authority/tests.py | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index c94afbd..266222b 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -46,6 +46,8 @@ class BasePermission(object): """ Set up both the user and group caches. """ + if not self.user: + return {}, {} perms = Permission.objects.filter( Q(user__pk=self.user.pk) | Q(group__in=self.user.groups.all()), ).select_related( @@ -92,6 +94,8 @@ class BasePermission(object): cached_permissions will generate the cache in a lazy fashion. """ # Check to see if the cache has been primed. + if not self.user: + return {} cache_filled = getattr( self.user, '_user_permissions_cache_filled', @@ -114,6 +118,8 @@ class BasePermission(object): cached_permissions will generate the cache in a lazy fashion. """ # Check to see if the cache has been primed. + if not self.user: + return {} cache_filled = getattr( self.user, '_group_permissions_cache_filled', @@ -136,14 +142,16 @@ class BasePermission(object): the next time the cached_permissions is used the cache will be re-primed. """ - self.user._user_permissions_cache_filled = False - self.user._group_permissions_cache_filled = False + if self.user: + self.user._user_permissions_cache_filled = False + self.user._group_permissions_cache_filled = False @property def use_smart_cache(self): # AUTHORITY_USE_SMART_CACHE defaults to False to maintain backwards # compatibility. - return getattr(settings, 'AUTHORITY_USE_SMART_CACHE', True) + use_smart_cache = getattr(settings, 'AUTHORITY_USE_SMART_CACHE', True) + return self.user and use_smart_cache def has_user_perms(self, perm, obj, approved, check_groups=True): if self.user: diff --git a/authority/tests.py b/authority/tests.py index 46406bd..cf5d2c1 100644 --- a/authority/tests.py +++ b/authority/tests.py @@ -266,3 +266,38 @@ class GroupPermissionCacheTestCase(SmartCacheingTestCase): check_groups=True, ) self.assertTrue(can_foo_with_group) + + def test_has_group_perms_no_user(self): + # Make sure calling has_user_perms on a permission that does not have a + # user does not throw any errors. + can_foo_with_group = self.group_check.has_group_perms( + 'foo', + self.user, + approved=True, + ) + self.assertFalse(can_foo_with_group) + + self.assertEqual(self.group_check.cached_user_permissions, {}) + self.assertEqual(self.group_check.cached_group_permissions, {}) + + # Create a permission with just that group. + Permission.objects.create( + content_type=Permission.objects.get_content_type(User), + object_id=self.user.pk, + codename='foo', + group=self.group, + approved=True, + ) + + # Invalidate the cache. + self.group_check.invalidate_permissions_cache() + + can_foo_with_group = self.group_check.has_group_perms( + 'foo', + self.user, + approved=True, + ) + self.assertTrue(can_foo_with_group) + + self.assertEqual(self.group_check.cached_user_permissions, {}) + self.assertEqual(self.group_check.cached_group_permissions, {}) From 263144831f62934625c3dd0ba60a85696a78c088 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Wed, 26 Sep 2012 11:09:15 -0400 Subject: [PATCH 30/46] refs #4: removed old comment --- authority/permissions.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index c94afbd..1788b8f 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -141,8 +141,6 @@ class BasePermission(object): @property def use_smart_cache(self): - # AUTHORITY_USE_SMART_CACHE defaults to False to maintain backwards - # compatibility. return getattr(settings, 'AUTHORITY_USE_SMART_CACHE', True) def has_user_perms(self, perm, obj, approved, check_groups=True): From 1d8f74306553a19d070e6271dfa3b23ddd7c18be Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Wed, 26 Sep 2012 13:38:14 -0400 Subject: [PATCH 31/46] refs #3: typo --- authority/permissions.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index f669f19..13c8fd6 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -119,13 +119,13 @@ class BasePermission(object): False, ) if cache_filled: - return self.user._authroity_group_perm_cache + return self.user._authority_group_perm_cache # Prime the cache. - self.user._authroity_group_perm_cache = \ + self.user._authority_group_perm_cache = \ self._get_cached_group_permissions() self.user._group_permissions_cache_filled = True - return self.user._authroity_group_perm_cache + return self.user._authority_group_perm_cache def invalidate_permissions_cache(self): """ From 513a1ad99aeb7a5702747c6d36e2eafbc16fcded Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Wed, 26 Sep 2012 13:41:48 -0400 Subject: [PATCH 32/46] refs #3: small refactor --- authority/permissions.py | 54 ++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index 13c8fd6..d6f6d63 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -139,34 +139,34 @@ class BasePermission(object): self.user._group_permissions_cache_filled = False def has_user_perms(self, perm, obj, approved, check_groups=True): - if self.user: - if self.user.is_superuser: - return True - if not self.user.is_active: - return False - - def _user_has_perms(cached_perms): - # Check to see if the permission is in the cache. - return cached_perms.get(( - obj.pk, - Permission.objects.get_content_type(obj).pk, - perm, - approved, - )) - # Check the permissions on the user. - cached_user_permissions = self.cached_user_permissions - - # Check to see if the permission is in the cache. - if _user_has_perms(cached_user_permissions): - return True - - # Optionally check group permissions - if check_groups: - cached_group_permissions = self.cached_group_permissions - if _user_has_perms(cached_group_permissions): - return True - + if not self.user: return False + if self.user.is_superuser: + return True + if not self.user.is_active: + return False + + def _user_has_perms(cached_perms): + # Check to see if the permission is in the cache. + return cached_perms.get(( + obj.pk, + Permission.objects.get_content_type(obj).pk, + perm, + approved, + )) + # Check the permissions on the user. + cached_user_permissions = self.cached_user_permissions + + # Check to see if the permission is in the cache. + if _user_has_perms(cached_user_permissions): + return True + + # Optionally check group permissions + if check_groups: + cached_group_permissions = self.cached_group_permissions + if _user_has_perms(cached_group_permissions): + return True + return False def has_group_perms(self, perm, obj, approved): From 8b62ad97d200c473965c1f7b5464d672cd915d7c Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Wed, 26 Sep 2012 13:47:16 -0400 Subject: [PATCH 33/46] refs #3: name changes to be more django --- authority/permissions.py | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index d6f6d63..be4ecd7 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -41,7 +41,7 @@ class BasePermission(object): self.group = group super(BasePermission, self).__init__(*args, **kwargs) - def _get_cached_permissions(self): + def _get_cached_perms(self): """ Set up both the user and group caches. """ @@ -69,31 +69,31 @@ class BasePermission(object): )] = True return user_permissions, group_permissions - def _get_cached_user_permissions(self): + def _get_perm_cache(self): """ Return a dictionary representation of the Permission objects that are related to ``self.user``, excluding group interactions. """ - permissions, _ = self._get_cached_permissions() + permissions, _ = self._get_cached_perms() return permissions - def _get_cached_group_permissions(self): + def _get_group_perm_cache(self): """ Return a dictionary representation of the Permission objects that are related to ``self.user``, including groups interactions. """ - _, permissions = self._get_cached_permissions() + _, permissions = self._get_cached_perms() return permissions @property - def cached_user_permissions(self): + def perm_cache(self): """ cached_permissions will generate the cache in a lazy fashion. """ # Check to see if the cache has been primed. cache_filled = getattr( self.user, - '_user_permissions_cache_filled', + '_authority_perm_cached_filled', False, ) if cache_filled: @@ -103,19 +103,19 @@ class BasePermission(object): # Prime the cache. self.user._authority_perm_cache = \ - self._get_cached_user_permissions() - self.user._user_permissions_cache_filled = True + self._get_perm_cache() + self.user._authority_perm_cached_filled = True return self.user._authority_perm_cache @property - def cached_group_permissions(self): + def group_perm_cache(self): """ cached_permissions will generate the cache in a lazy fashion. """ # Check to see if the cache has been primed. cache_filled = getattr( self.user, - '_group_permissions_cache_filled', + '_authority_group_perm_cached_filled', False, ) if cache_filled: @@ -123,8 +123,8 @@ class BasePermission(object): # Prime the cache. self.user._authority_group_perm_cache = \ - self._get_cached_group_permissions() - self.user._group_permissions_cache_filled = True + self._get_group_perm_cache() + self.user._authority_group_perm_cached_filled = True return self.user._authority_group_perm_cache def invalidate_permissions_cache(self): @@ -135,8 +135,8 @@ class BasePermission(object): the next time the cached_permissions is used the cache will be re-primed. """ - self.user._user_permissions_cache_filled = False - self.user._group_permissions_cache_filled = False + self.user._authority_perm_cached_filled = False + self.user._authority_group_perm_cached_filled = False def has_user_perms(self, perm, obj, approved, check_groups=True): if not self.user: @@ -155,16 +155,16 @@ class BasePermission(object): approved, )) # Check the permissions on the user. - cached_user_permissions = self.cached_user_permissions + perm_cache = self.perm_cache # Check to see if the permission is in the cache. - if _user_has_perms(cached_user_permissions): + if _user_has_perms(perm_cache): return True # Optionally check group permissions if check_groups: - cached_group_permissions = self.cached_group_permissions - if _user_has_perms(cached_group_permissions): + group_perm_cache = self.group_perm_cache + if _user_has_perms(group_perm_cache): return True return False From cb2d7c02804ae74c9cc9e216c7b3cdffac42afe4 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Wed, 26 Sep 2012 13:58:40 -0400 Subject: [PATCH 34/46] refs #4: typo --- authority/tests.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/authority/tests.py b/authority/tests.py index aa9b33c..e3c64a1 100644 --- a/authority/tests.py +++ b/authority/tests.py @@ -151,7 +151,7 @@ class AssignExceptionsTest(TestCase): self.fail() -class SmartCacheingTestCase(TestCase): +class SmartCachingTestCase(TestCase): """ The base test case for all tests that have to do with smart caching. """ @@ -175,7 +175,7 @@ class SmartCacheingTestCase(TestCase): ) -class PerformanceTest(SmartCacheingTestCase): +class PerformanceTest(SmartCachingTestCase): """ Tests that permission are actually cached and that the number of queries stays constant. @@ -210,7 +210,7 @@ class PerformanceTest(SmartCacheingTestCase): self.check.has_user_perms('foo', self.user, True, False) -class GroupPermissionCacheTestCase(SmartCacheingTestCase): +class GroupPermissionCacheTestCase(SmartCachingTestCase): """ Tests that peg expected behaviour """ From f6bba30407c006f7c1a72195b7d8c17cbf5ff604 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Wed, 26 Sep 2012 15:31:09 -0400 Subject: [PATCH 35/46] refs #6: name change --- authority/tests.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/authority/tests.py b/authority/tests.py index 19394d4..d1a6cee 100644 --- a/authority/tests.py +++ b/authority/tests.py @@ -21,8 +21,11 @@ class GroupPermission(permissions.BasePermission): authority.register(Group, GroupPermission) -class BehaviourTest(TestCase): +class DjangoPermissionChecksTestCase(TestCase): """ + Django permission objects have certain methods that are always present, + test those here. + self.user will be given: - django permission add_user (test_add) - authority to delete_user which is him (test_delete) From 2d71ba46b12b93e9f25ae5c354dcca451144cad1 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Wed, 26 Sep 2012 16:00:30 -0400 Subject: [PATCH 36/46] refs #7: wrote a test to show the expected number of queries needed for has_user_perms --- authority/tests.py | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/authority/tests.py b/authority/tests.py index d1a6cee..8db8659 100644 --- a/authority/tests.py +++ b/authority/tests.py @@ -227,6 +227,39 @@ class PerformanceTest(SmartCachingTestCase): # One query to re generate the cache. self.user_check.has_user_perms('foo', self.user, True, False) + def test_has_user_perms_check_group(self): + # Create a permission with just a group. + Permission.objects.create( + content_type=Permission.objects.get_content_type(User), + object_id=self.user.pk, + codename='foo', + group=self.group, + approved=True, + ) + + # Check the number of queries. + with self.assertNumQueries(2): + self.user_check.has_user_perms('foo', self.user, True, True) + + # Create a second group. + new_group = Group.objects.create(name='new_group') + new_group.user_set.add(self.user) + + # Create a permission object for it. + Permission.objects.create( + content_type=Permission.objects.get_content_type(User), + object_id=self.user.pk, + codename='foo', + group=new_group, + approved=True, + ) + + self.user_check.invalidate_permissions_cache() + + # Make sure it is the same number of queries. + with self.assertNumQueries(2): + self.user_check.has_user_perms('foo', self.user, True, True) + class GroupPermissionCacheTestCase(SmartCachingTestCase): """ From 7c899a245de0c405bf7eddad6277f59d65704a15 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Wed, 26 Sep 2012 16:01:01 -0400 Subject: [PATCH 37/46] refs #7: doing the correct number of queries now --- authority/permissions.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index 590fe5a..d4667da 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -50,8 +50,6 @@ class BasePermission(object): return {}, {} perms = Permission.objects.filter( Q(user__pk=self.user.pk) | Q(group__in=self.user.groups.all()), - ).select_related( - 'group', ) user_permissions = {} group_permissions = {} @@ -63,7 +61,10 @@ class BasePermission(object): perm.codename, perm.approved, )] = True - if perm.group and self.user in perm.group.user_set.all(): + # If the user has the permission do for something, but perm.user != + # self.user then by definition that permission came from the + # group. + else: group_permissions[( perm.object_id, perm.content_type_id, @@ -162,11 +163,13 @@ class BasePermission(object): return False if self.use_smart_cache: + content_type_pk = Permission.objects.get_content_type(obj).pk + def _user_has_perms(cached_perms): # Check to see if the permission is in the cache. return cached_perms.get(( obj.pk, - Permission.objects.get_content_type(obj).pk, + content_type_pk, perm, approved, )) From 82bc7ee89e51d6af58cbb6bfd07ebd47aeae9b41 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Wed, 26 Sep 2012 16:10:20 -0400 Subject: [PATCH 38/46] refs #8: updated the tests to the correct expected number of queries --- authority/tests.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/authority/tests.py b/authority/tests.py index 8db8659..d9fc36a 100644 --- a/authority/tests.py +++ b/authority/tests.py @@ -214,6 +214,12 @@ class PerformanceTest(SmartCachingTestCase): self.user_check.has_user_perms('foo', self.user, True, False) self.user_check.has_user_perms('foo', self.user, True, False) + 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. + with self.assertNumQueries(1): + self.user_check.has_user_perms('foo', self.user, True, True) + def test_invalidate_permissions_cache(self): # Show that calling invalidate_permissions_cache will cause extra # queries. @@ -227,7 +233,7 @@ class PerformanceTest(SmartCachingTestCase): # One query to re generate the cache. self.user_check.has_user_perms('foo', self.user, True, False) - def test_has_user_perms_check_group(self): + def test_has_user_perms_check_group_multiple(self): # Create a permission with just a group. Permission.objects.create( content_type=Permission.objects.get_content_type(User), @@ -238,7 +244,7 @@ class PerformanceTest(SmartCachingTestCase): ) # Check the number of queries. - with self.assertNumQueries(2): + with self.assertNumQueries(1): self.user_check.has_user_perms('foo', self.user, True, True) # Create a second group. @@ -257,7 +263,7 @@ class PerformanceTest(SmartCachingTestCase): self.user_check.invalidate_permissions_cache() # Make sure it is the same number of queries. - with self.assertNumQueries(2): + with self.assertNumQueries(1): self.user_check.has_user_perms('foo', self.user, True, True) From 8d62a86e96f01b3d7b3f50dc1f3eb3be03494ec3 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Wed, 26 Sep 2012 16:10:45 -0400 Subject: [PATCH 39/46] refs #8: did a smarter job of building the cache without redoing work --- authority/permissions.py | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index d4667da..4137ea7 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -73,21 +73,15 @@ class BasePermission(object): )] = True return user_permissions, group_permissions - def _get_perm_cache(self): + def _authority_prime_perm_caches(self): """ Return a dictionary representation of the Permission objects that are related to ``self.user``, excluding group interactions. """ - permissions, _ = self._get_cached_perms() - return permissions - - def _get_group_perm_cache(self): - """ - Return a dictionary representation of the Permission objects that are - related to ``self.user``, including groups interactions. - """ - _, permissions = self._get_cached_perms() - return permissions + perm_cache, group_perm_cache = self._get_cached_perms() + self.user._authority_perm_cache = perm_cache + self.user._authority_group_perm_cache = group_perm_cache + self.user._authority_perm_cached_filled = True @property def perm_cache(self): @@ -108,9 +102,7 @@ class BasePermission(object): return self.user._authority_perm_cache # Prime the cache. - self.user._authority_perm_cache = \ - self._get_perm_cache() - self.user._authority_perm_cached_filled = True + self._authority_prime_perm_caches() return self.user._authority_perm_cache @property @@ -123,16 +115,14 @@ class BasePermission(object): return {} cache_filled = getattr( self.user, - '_authority_group_perm_cached_filled', + '_authority_perm_cached_filled', False, ) if cache_filled: return self.user._authority_group_perm_cache # Prime the cache. - self.user._authority_group_perm_cache = \ - self._get_group_perm_cache() - self.user._authority_group_perm_cached_filled = True + self._authority_prime_perm_caches() return self.user._authority_group_perm_cache def invalidate_permissions_cache(self): @@ -145,7 +135,6 @@ class BasePermission(object): """ if self.user: self.user._authority_perm_cached_filled = False - self.user._authority_group_perm_cached_filled = False @property def use_smart_cache(self): From 6326631756c238c489f2f48d13eb19678a51a57b Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Wed, 26 Sep 2012 16:27:33 -0400 Subject: [PATCH 40/46] refs #8: language cleanup, code cleanup --- authority/permissions.py | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index 4137ea7..6bf84e6 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -75,13 +75,13 @@ class BasePermission(object): def _authority_prime_perm_caches(self): """ - Return a dictionary representation of the Permission objects that are - related to ``self.user``, excluding group interactions. + Prime both the user and group caches and put them on the ``self.user``. + In addition add a cache filled flag on ``self.user``. """ perm_cache, group_perm_cache = self._get_cached_perms() self.user._authority_perm_cache = perm_cache self.user._authority_group_perm_cache = group_perm_cache - self.user._authority_perm_cached_filled = True + self.user._authority_perm_cache_filled = True @property def perm_cache(self): @@ -93,7 +93,7 @@ class BasePermission(object): return {} cache_filled = getattr( self.user, - '_authority_perm_cached_filled', + '_authority_perm_cache_filled', False, ) if cache_filled: @@ -115,7 +115,7 @@ class BasePermission(object): return {} cache_filled = getattr( self.user, - '_authority_perm_cached_filled', + '_authority_perm_cache_filled', False, ) if cache_filled: @@ -134,7 +134,7 @@ class BasePermission(object): re-primed. """ if self.user: - self.user._authority_perm_cached_filled = False + self.user._authority_perm_cache_filled = False @property def use_smart_cache(self): @@ -162,20 +162,14 @@ class BasePermission(object): perm, approved, )) - # Check the permissions on the user. - perm_cache = self.perm_cache # Check to see if the permission is in the cache. - if _user_has_perms(perm_cache): - return True + has_perm = _user_has_perms(self.perm_cache) # Optionally check group permissions if check_groups: - group_perm_cache = self.group_perm_cache - if _user_has_perms(group_perm_cache): - return True - - return False + has_perm = has_perm or _user_has_perms(self.group_perm_cache) + return has_perm else: return Permission.objects.user_permissions( self.user, From 342f3d53110daecb87aa1b661efb332594af2160 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Wed, 26 Sep 2012 17:17:27 -0400 Subject: [PATCH 41/46] refs #8: name changes and used kwargs in one of the tests --- authority/permissions.py | 14 +++++++------- authority/tests.py | 15 ++++++++++----- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index 6bf84e6..a31a2e8 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -73,7 +73,7 @@ class BasePermission(object): )] = True return user_permissions, group_permissions - def _authority_prime_perm_caches(self): + def _prime_perm_caches(self): """ Prime both the user and group caches and put them on the ``self.user``. In addition add a cache filled flag on ``self.user``. @@ -84,7 +84,7 @@ class BasePermission(object): self.user._authority_perm_cache_filled = True @property - def perm_cache(self): + def _perm_cache(self): """ cached_permissions will generate the cache in a lazy fashion. """ @@ -102,11 +102,11 @@ class BasePermission(object): return self.user._authority_perm_cache # Prime the cache. - self._authority_prime_perm_caches() + self._prime_perm_caches() return self.user._authority_perm_cache @property - def group_perm_cache(self): + def _group_perm_cache(self): """ cached_permissions will generate the cache in a lazy fashion. """ @@ -122,7 +122,7 @@ class BasePermission(object): return self.user._authority_group_perm_cache # Prime the cache. - self._authority_prime_perm_caches() + self._prime_perm_caches() return self.user._authority_group_perm_cache def invalidate_permissions_cache(self): @@ -164,11 +164,11 @@ class BasePermission(object): )) # Check to see if the permission is in the cache. - has_perm = _user_has_perms(self.perm_cache) + has_perm = _user_has_perms(self._perm_cache) # Optionally check group permissions if check_groups: - has_perm = has_perm or _user_has_perms(self.group_perm_cache) + has_perm = has_perm or _user_has_perms(self._group_perm_cache) return has_perm else: return Permission.objects.user_permissions( diff --git a/authority/tests.py b/authority/tests.py index d9fc36a..79d8d46 100644 --- a/authority/tests.py +++ b/authority/tests.py @@ -218,7 +218,12 @@ class PerformanceTest(SmartCachingTestCase): # Regardless of the number groups permissions, it should only take one # query to check both users and groups. with self.assertNumQueries(1): - self.user_check.has_user_perms('foo', self.user, True, True) + self.user_check.has_user_perms( + 'foo', + self.user, + approved=True, + check_groups=True, + ) def test_invalidate_permissions_cache(self): # Show that calling invalidate_permissions_cache will cause extra @@ -319,8 +324,8 @@ class GroupPermissionCacheTestCase(SmartCachingTestCase): ) self.assertFalse(can_foo_with_group) - self.assertEqual(self.group_check.perm_cache, {}) - self.assertEqual(self.group_check.group_perm_cache, {}) + self.assertEqual(self.group_check._perm_cache, {}) + self.assertEqual(self.group_check._group_perm_cache, {}) # Create a permission with just that group. Permission.objects.create( @@ -341,5 +346,5 @@ class GroupPermissionCacheTestCase(SmartCachingTestCase): ) self.assertTrue(can_foo_with_group) - self.assertEqual(self.group_check.perm_cache, {}) - self.assertEqual(self.group_check.group_perm_cache, {}) + self.assertEqual(self.group_check._perm_cache, {}) + self.assertEqual(self.group_check._group_perm_cache, {}) From 6d6c91dfb69d5a2a4c13f5872e70774e914921c1 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Wed, 26 Sep 2012 17:23:30 -0400 Subject: [PATCH 42/46] refs #8: refactor --- authority/permissions.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/authority/permissions.py b/authority/permissions.py index a31a2e8..62869d4 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -164,24 +164,24 @@ class BasePermission(object): )) # Check to see if the permission is in the cache. - has_perm = _user_has_perms(self._perm_cache) + if _user_has_perms(self._perm_cache): + return True # Optionally check group permissions if check_groups: - has_perm = has_perm or _user_has_perms(self._group_perm_cache) - return has_perm - else: - return Permission.objects.user_permissions( - self.user, - perm, - obj, - approved, - check_groups, - ).filter( - object_id=obj.pk, - ).exists() + return _user_has_perms(self._group_perm_cache) + return False - return False + # Actually hit the DB, no smart cache used. + return Permission.objects.user_permissions( + self.user, + perm, + obj, + approved, + check_groups, + ).filter( + object_id=obj.pk, + ).exists() def has_group_perms(self, perm, obj, approved): """ From 95a7dc5e927eb94c9321fb01ec75e1c3078ad54e Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Wed, 26 Sep 2012 17:44:01 -0400 Subject: [PATCH 43/46] refs #9: added a line in the docs showing how to disable the use of the smart cache --- docs/configuration.txt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/configuration.txt b/docs/configuration.txt index bcf734a..c0aa19f 100644 --- a/docs/configuration.txt +++ b/docs/configuration.txt @@ -32,6 +32,11 @@ context processors:: 'django.core.context_processors.request', ) +django-authority defaults to using a smart cache when checking permissions. +This can be disabled by adding the following line to ``settings.py``:: + + AUTHORITY_USE_SMART_CACHE = False + urls.py ======= From e45d774a763ae0303444b40e0d8344d978020f67 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Wed, 26 Sep 2012 17:44:22 -0400 Subject: [PATCH 44/46] refs #9: added a section to show how cache invalidation works --- docs/tips_tricks.txt | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/docs/tips_tricks.txt b/docs/tips_tricks.txt index 6f77da7..ff5ae71 100644 --- a/docs/tips_tricks.txt +++ b/docs/tips_tricks.txt @@ -31,3 +31,38 @@ Within a permission class, you can refer to Django's basic permissions:: # ... authority.register(Flatpage, FlagpagePermisson) + +If the ``Permission`` table changes during the lifespan of a django-authority +permission instance and the smart cache is being used, you will need to call +invalidate_permissions_cache in order to see that changes:: + + class UserPermission(permission.BasePermission): + label = 'user_permission' + checks = ('do_foo',) + authority.register(User, UserPermission) + + user_permission = UserPermission(user) + + # can_foo is False here since the permission has not yet been added. + can_foo = user_permission.has_user_perms('foo', user) + + Permission.objects.create( + content_type=Permission.objects.get_content_type(User), + object_id=user.pk, + codename='foo', + user=user, + approved=True, + ) + + # can_foo is still False because the permission cache has not been + invalidated yet. + can_foo = user_permission.has_user_perms('foo', user) + + user_permission.invalidate_permissions_cache() + + # can_foo is now True + can_foo = user_permission.has_user_perms('foo', user) + +This is particularly useful if you are using the permission instances during a +request, where it is unlikely that the state of the ``Permission`` table will +change. From 6c1692ae6362f3ed7bdf5cdbc9122f1654fd2ed9 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Wed, 26 Sep 2012 17:49:19 -0400 Subject: [PATCH 45/46] refs #9: bumped the version in the docs --- docs/conf.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/conf.py b/docs/conf.py index 1137de2..84cd272 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -46,10 +46,10 @@ copyright = u'2009, the django-authority team' # built documents. # # The short X.Y version. -version = '0.4' +version = '0.5' # The full version, including alpha/beta/rc tags. -release = '0.4dev' +release = '0.5dev' # The language for content autogenerated by Sphinx. Refer to documentation # for a list of supported languages. From 483cb629c72bad11ae99793c18e5d2c684c504ba Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Thu, 27 Sep 2012 17:17:59 -0400 Subject: [PATCH 46/46] refs #9: added a line in the basic overview about the smart cache --- docs/index.txt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/index.txt b/docs/index.txt index 6f9fda9..baa244c 100644 --- a/docs/index.txt +++ b/docs/index.txt @@ -23,6 +23,11 @@ This application provides three abilities: voodoo-code to Django's ``contrib.auth`` system, it keeps your existing permission system intact! +django-authority uses a cache that is stored on the user object to help improve +performance. However, if the ``Permission`` table changes the cache will need +to be invalidated. More information about this can be found in the tips and +tricks section. + .. warning:: We have just started with the documentation and it's far from being perfect. If you find glitches, errors or just have feedback, please contact the team: :ref:`support`.