diff --git a/reviewboard/accounts/models.py b/reviewboard/accounts/models.py index bfe5d9169eb200b3f91cd187d553e6e579fc2ca9..3136e8e72a1834828d47438a32d4816402b8d4df 100644 --- a/reviewboard/accounts/models.py +++ b/reviewboard/accounts/models.py @@ -535,7 +535,7 @@ def _get_profile(self, cached_only=False, create_if_missing=True, newly-created. Returns: - Profile or tuple. + Profile or tuple: The user's profile. If ``return_is_new`` is ``True``, then this will instead return @@ -548,18 +548,38 @@ def _get_profile(self, cached_only=False, create_if_missing=True, """ # Note that we use the same cache variable that a select_related() call # would use, ensuring that we benefit from Django's caching when possible. - profile = getattr(self, '_profile_set_cache', None) + profile = getattr(self, '_profile', None) + profile_was_none = (profile is None) is_new = False - if profile is None and not cached_only: - if create_if_missing: - profile, is_new = Profile.objects.get_or_create(user=self) - else: - # This may raise Profile.DoesNotExist. - profile = Profile.objects.get(user=self) - + if profile is None: + # Check if this was pre-fetched. + try: + profile = list(self._prefetched_objects_cache['profile_set'])[0] + except (AttributeError, IndexError, KeyError): + pass + + if profile is None: + # Check if it's in the field cache (select_related): + try: + field = self._meta.get_field('profile') + profile = field.get_cached_value(self) + except KeyError: + pass + + if profile is None and not cached_only: + # We may need to create or fetch this. + if create_if_missing: + profile, is_new = Profile.objects.get_or_create(user=self) + else: + # This may raise Profile.DoesNotExist. + profile = Profile.objects.get(user=self) + + if profile_was_none and profile is not None: + # We didn't have this cached before, but we have a profile now. + # Cache it on the user and set the user on the profile. profile.user = self - self._profile_set_cache = profile + self._profile = profile # While modern versions of Review Board set this to an empty dictionary, # old versions would initialize this to None. Since we don't want to litter diff --git a/reviewboard/accounts/tests/test_user.py b/reviewboard/accounts/tests/test_user.py index 04e538c95e345fb231bed4e6aceb7cfa7bf476a2..18bee73cf3d7daa718bb44fdc17dca82c5463fd4 100644 --- a/reviewboard/accounts/tests/test_user.py +++ b/reviewboard/accounts/tests/test_user.py @@ -3,6 +3,7 @@ from django.contrib.auth.models import AnonymousUser, User from djblets.testing.decorators import add_fixtures +from reviewboard.accounts.models import Profile from reviewboard.site.models import LocalSite from reviewboard.testing import TestCase @@ -12,6 +13,151 @@ class UserTests(TestCase): fixtures = ['test_users'] + def test_get_profile(self): + """Testing User.get_profile""" + user = self.create_user(username='test1') + profile = Profile.objects.create(user=user) + + # 1 query: + # + # 1. Fetch profile + with self.assertNumQueries(1): + new_profile = user.get_profile() + + self.assertEqual(new_profile, profile) + self.assertIsNot(new_profile, profile) + self.assertIs(new_profile.user, user) + + # A second call should hit cache. + with self.assertNumQueries(0): + self.assertIs(user.get_profile(), new_profile) + + def test_get_profile_with_no_profile(self): + """Testing User.get_profile with no existing profile""" + user = self.create_user(username='test1') + + # 4 queries: + # + # 1. Attempt to fetch profile + # 2. Create savepoint + # 3. Create Profile + # 4. Release savepoint + with self.assertNumQueries(4): + new_profile = user.get_profile() + + self.assertIs(new_profile.user, user) + self.assertIsNotNone(new_profile.pk) + + # A second call should hit cache. + with self.assertNumQueries(0): + self.assertIs(user.get_profile(), new_profile) + + def test_get_profile_with_prefetch_related(self): + """Testing User.get_profile with prefetch_related""" + user = self.create_user(username='test1') + Profile.objects.create(user=user) + + # Now re-fetch. + # + # 2 queries: + # + # 1. Fetch users + # 2. Fetch profiles for fetched user IDs + with self.assertNumQueries(2): + user = list( + User.objects + .filter(username='test1') + .prefetch_related('profile_set') + )[0] + + with self.assertNumQueries(0): + profile = user.get_profile() + + self.assertIs(profile.user, user) + self.assertIsNotNone(profile.pk) + + def test_get_profile_with_select_related(self): + """Testing User.get_profile with select_related""" + user = self.create_user(username='test1') + Profile.objects.create(user=user) + + # Now re-fetch. + # + # 1 query: + # + # 1. Fetch users + profiles + with self.assertNumQueries(1): + user = list( + User.objects + .filter(username='test1') + .select_related('profile') + )[0] + + with self.assertNumQueries(0): + profile = user.get_profile() + + self.assertIs(profile.user, user) + self.assertIsNotNone(profile.pk) + + def test_get_profile_with_no_profile_and_create_if_missing_false(self): + """Testing User.get_profile with no existing profile and + create_if_missing=False + """ + user = self.create_user(username='test1') + + # 1 query: + # + # 1. Attempt to fetch profile + with self.assertNumQueries(1): + with self.assertRaises(Profile.DoesNotExist): + user.get_profile(create_if_missing=False) + + def test_get_profile_with_cached_only_and_in_cache(self): + """Testing User.get_profile with cached_only=True and profile already + in object cache + """ + user = self.create_user(username='test1') + profile = user.get_profile() + + with self.assertNumQueries(0): + new_profile = user.get_profile(cached_only=True) + + self.assertIs(new_profile, profile) + + def test_get_profile_with_cached_only_and_not_in_cache(self): + """Testing User.get_profile with cached_only=True and profile not + in object cache + """ + user = self.create_user(username='test1') + Profile.objects.create(user=user) + + with self.assertNumQueries(0): + self.assertIsNone(user.get_profile(cached_only=True)) + + def test_get_profile_with_return_is_new_and_new(self): + """Testing User.get_profile with return_is_new=True and profile is new + """ + user = self.create_user(username='test1') + result = user.get_profile(return_is_new=True) + + self.assertIsInstance(result, tuple) + self.assertEqual(len(result), 2) + self.assertIsInstance(result[0], Profile) + self.assertTrue(result[1]) + + def test_get_profile_with_return_is_new_and_not_new(self): + """Testing User.get_profile with return_is_new=True and profile is new + """ + user = self.create_user(username='test1') + Profile.objects.create(user=user) + + result = user.get_profile(return_is_new=True) + + self.assertIsInstance(result, tuple) + self.assertEqual(len(result), 2) + self.assertIsInstance(result[0], Profile) + self.assertFalse(result[1]) + def test_is_profile_visible_with_public(self): """Testing User.is_profile_visible with public profiles""" user1 = User.objects.get(username='admin') diff --git a/reviewboard/datagrids/tests.py b/reviewboard/datagrids/tests.py index c4cdea7862760323c3f7a59e3ee26653950126b0..d658975ef1f9040c117770828a7d5b99d35032d4 100644 --- a/reviewboard/datagrids/tests.py +++ b/reviewboard/datagrids/tests.py @@ -584,7 +584,15 @@ class UsersDataGridTests(BaseViewTestCase): self.client.login(username='doc', password='doc') - with self.assertNumQueries(10): + # 6 queries: + # + # 1. Fetch logged-in user + # 2. Fetch logged-in user's profile + # 3. Set profile's sort_submitter_columns and submitter_columns + # 4. Fetch total number of users for datagrid + # 5. Fetch IDs of users for datagrid + # 6. Fetch users + profiles from IDs + with self.assertNumQueries(6): response = self.client.get('/users/?columns=fullname') self.assertEqual(response.status_code, 200) @@ -607,7 +615,12 @@ class UsersDataGridTests(BaseViewTestCase): self.client.logout() - with self.assertNumQueries(7): + # 3 queries: + # + # 1. Fetch total number of users for datagrid + # 2. Fetch IDs of users for datagrid + # 3. Fetch users + profiles from IDs + with self.assertNumQueries(3): response = self.client.get('/users/?columns=fullname') self.assertEqual(response.status_code, 200) @@ -622,12 +635,26 @@ class UsersDataGridTests(BaseViewTestCase): row['cells'][0]) def test_profile_not_exists(self): - """Testing UsersDataGrid when a profile does exist""" + """Testing UsersDataGrid when a profile does not exist""" Profile.objects.all().update(is_private=True) self.client.login(username='doc', password='doc') - with self.assertNumQueries(14): + # 11 queries: + # + # 1. Fetch logged-in user + # 2. Fetch logged-in user's profile + # 3. Set profile's sort_submitter_columns and submitter_columns + # 4. Fetch total number of users for datagrid + # 5. Fetch IDs of users for datagrid + # 6. Fetch users + profiles from IDs + # 7. Fetch users that logged-in user is an admin for on Local Site + # (for private profile access) + # 8. Attempt to fetch missing profile for user ID 3 (dopey) + # 9. Create savepoint + # 10. Create profile for user ID 3 (dopey) + # 11. Release savepoint + with self.assertNumQueries(11): response = self.client.get('/users/?columns=fullname') self.assertEqual(response.status_code, 200) @@ -658,7 +685,17 @@ class UsersDataGridTests(BaseViewTestCase): self.client.login(username='doc', password='doc') - with self.assertNumQueries(11): + # 7 queries: + # + # 1. Fetch logged-in user + # 2. Fetch logged-in user's profile + # 3. Set profile's sort_submitter_columns and submitter_columns + # 4. Fetch total number of users for datagrid + # 5. Fetch IDs of users for datagrid + # 6. Fetch users + profiles from IDs + # 7. Fetch users that logged-in user is an admin for on Local Site + # (for private profile access) + with self.assertNumQueries(7): response = self.client.get('/users/?columns=fullname') self.assertEqual(response.status_code, 200) @@ -690,7 +727,12 @@ class UsersDataGridTests(BaseViewTestCase): Profile.objects.all().update(is_private=True) self.client.logout() - with self.assertNumQueries(7): + # 3 queries: + # + # 1. Fetch total number of users for datagrid + # 2. Fetch IDs of users for datagrid + # 3. Fetch users + profiles from IDs + with self.assertNumQueries(3): response = self.client.get('/users/?columns=fullname') self.assertEqual(response.status_code, 200) @@ -712,7 +754,15 @@ class UsersDataGridTests(BaseViewTestCase): Profile.objects.all().update(is_private=True) self.client.login(username='admin', password='admin') - with self.assertNumQueries(10): + # 6 queries: + # + # 1. Fetch logged-in user + # 2. Fetch logged-in user's profile + # 3. Set profile's sort_submitter_columns and submitter_columns + # 4. Fetch total number of users for datagrid + # 5. Fetch IDs of users for datagrid + # 6. Fetch users + profiles from IDs + with self.assertNumQueries(6): response = self.client.get('/users/?columns=fullname') self.assertEqual(response.status_code, 200) @@ -736,7 +786,17 @@ class UsersDataGridTests(BaseViewTestCase): Profile.objects.all().update(is_private=True) self.client.login(username='doc', password='doc') - with self.assertNumQueries(11): + # 7 queries: + # + # 1. Fetch logged-in user + # 2. Fetch logged-in user's profile + # 3. Set profile's sort_submitter_columns and submitter_columns + # 4. Fetch total number of users for datagrid + # 5. Fetch IDs of users for datagrid + # 6. Fetch users + profiles from IDs + # 7. Fetch users that logged-in user is an admin for on Local Site + # (for private profile access) + with self.assertNumQueries(7): response = self.client.get('/users/?columns=fullname') self.assertEqual(response.status_code, 200) @@ -768,7 +828,19 @@ class UsersDataGridTests(BaseViewTestCase): Profile.objects.all().update(is_private=True) self.client.login(username='doc', password='doc') - with self.assertNumQueries(11): + # 9 queries: + # + # 1. Fetch logged-in user + # 2. Fetch logged-in user's profile + # 3. Fetch LocalSite + # 4. Check LocalSite membership + # 5. Set profile's sort_submitter_columns and submitter_columns + # 6. Fetch total number of users for datagrid + # 7. Fetch IDs of users for datagrid + # 8. Fetch users + profiles from IDs + # 9. Fetch users that logged-in user is an admin for on Local Site + # (for private profile access) + with self.assertNumQueries(9): response = self.client.get( '/s/local-site-2/users/?columns=fullname')