From 82bc7ee89e51d6af58cbb6bfd07ebd47aeae9b41 Mon Sep 17 00:00:00 2001 From: Jason Ward Date: Wed, 26 Sep 2012 16:10:20 -0400 Subject: [PATCH 1/5] 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 2/5] 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 3/5] 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 4/5] 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 5/5] 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): """