diff --git a/authority/permissions.py b/authority/permissions.py index d4667da..62869d4 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -73,24 +73,18 @@ class BasePermission(object): )] = True return user_permissions, group_permissions - def _get_perm_cache(self): + def _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``. """ - 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_cache_filled = True @property - def perm_cache(self): + def _perm_cache(self): """ cached_permissions will generate the cache in a lazy fashion. """ @@ -99,7 +93,7 @@ class BasePermission(object): return {} cache_filled = getattr( self.user, - '_authority_perm_cached_filled', + '_authority_perm_cache_filled', False, ) if cache_filled: @@ -108,13 +102,11 @@ 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._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. """ @@ -123,16 +115,14 @@ class BasePermission(object): return {} cache_filled = getattr( self.user, - '_authority_group_perm_cached_filled', + '_authority_perm_cache_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._prime_perm_caches() return self.user._authority_group_perm_cache def invalidate_permissions_cache(self): @@ -144,8 +134,7 @@ class BasePermission(object): re-primed. """ if self.user: - self.user._authority_perm_cached_filled = False - self.user._authority_group_perm_cached_filled = False + self.user._authority_perm_cache_filled = False @property def use_smart_cache(self): @@ -173,32 +162,26 @@ 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): + if _user_has_perms(self._perm_cache): return True # Optionally check group permissions if check_groups: - group_perm_cache = self.group_perm_cache - if _user_has_perms(group_perm_cache): - return True - + return _user_has_perms(self._group_perm_cache) return False - else: - return Permission.objects.user_permissions( - self.user, - perm, - obj, - approved, - check_groups, - ).filter( - object_id=obj.pk, - ).exists() - 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): """ diff --git a/authority/tests.py b/authority/tests.py index 8db8659..79d8d46 100644 --- a/authority/tests.py +++ b/authority/tests.py @@ -214,6 +214,17 @@ 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, + approved=True, + check_groups=True, + ) + def test_invalidate_permissions_cache(self): # Show that calling invalidate_permissions_cache will cause extra # queries. @@ -227,7 +238,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 +249,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 +268,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) @@ -313,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( @@ -335,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, {})