From 1847086044d484bb336874ac375b5468d7a1c021 Mon Sep 17 00:00:00 2001 From: bedo Date: Thu, 17 Oct 2024 12:25:00 +0300 Subject: [PATCH] subscription: Remove unnecessary select_related fields and clean up. Removes the unnecessary fields from bulk_access_users_by_email and bulk_access_users_id, while also removing duplication of these lists of fields. "base_bulk_get_user_queryset", used when fetching a user other than the acting user. "base_get_user_queryset", used when fetching the acting user, prefetching more fields. There remains some inconsistency in the models.py functions that may merit further investigation. --- zerver/lib/users.py | 34 ++++++-------------------- zerver/models/users.py | 55 ++++++++++++++++++++---------------------- 2 files changed, 33 insertions(+), 56 deletions(-) diff --git a/zerver/lib/users.py b/zerver/lib/users.py index 082dac8750..98b3986064 100644 --- a/zerver/lib/users.py +++ b/zerver/lib/users.py @@ -45,6 +45,8 @@ from zerver.models.realms import BotCreationPolicyEnum, get_fake_email_domain, r from zerver.models.users import ( active_non_guest_user_ids, active_user_ids, + base_bulk_get_user_queryset, + base_get_user_queryset, get_realm_user_dicts, get_user_by_id_in_realm_including_cross_realm, get_user_profile_by_id_in_realm, @@ -405,12 +407,7 @@ def access_user_by_email( # ineffective. # # Notably, we use the same select_related as access_user_by_id. - target = UserProfile.objects.select_related( - "realm", - "realm__can_access_all_users_group", - "realm__can_access_all_users_group__named_user_group", - "bot_owner", - ).get( + target = base_get_user_queryset().get( delivery_email__iexact=email.strip(), realm=user_profile.realm, email_address_visibility__in=allowed_email_address_visibility_values, @@ -433,19 +430,11 @@ def bulk_access_users_by_email( # `email__iexact__in=emails` is not supported by Django. target_emails_upper = [email.strip().upper() for email in emails] users = ( - UserProfile.objects.annotate(email_upper=Upper("email")) - .select_related( - "realm", - "realm__can_access_all_users_group", - "realm__can_access_all_users_group__named_user_group", - "realm__direct_message_initiator_group", - "realm__direct_message_initiator_group__named_user_group", - "realm__direct_message_permission_group", - "realm__direct_message_permission_group__named_user_group", - "bot_owner", - ) + base_bulk_get_user_queryset() + .annotate(email_upper=Upper("email")) .filter(email_upper__in=target_emails_upper, realm=acting_user.realm) ) + valid_emails_upper = {user_profile.email_upper for user_profile in users} all_users_exist = all(email in valid_emails_upper for email in target_emails_upper) @@ -466,16 +455,7 @@ def bulk_access_users_by_id( allow_bots: bool = False, for_admin: bool, ) -> set[UserProfile]: - users = UserProfile.objects.select_related( - "realm", - "realm__can_access_all_users_group", - "realm__can_access_all_users_group__named_user_group", - "realm__direct_message_initiator_group", - "realm__direct_message_initiator_group__named_user_group", - "realm__direct_message_permission_group", - "realm__direct_message_permission_group__named_user_group", - "bot_owner", - ).filter(id__in=user_ids, realm=acting_user.realm) + users = base_bulk_get_user_queryset().filter(id__in=user_ids, realm=acting_user.realm) valid_user_ids = {user_profile.id for user_profile in users} all_users_exist = all(user_id in valid_user_ids for user_id in user_ids) diff --git a/zerver/models/users.py b/zerver/models/users.py index 2f56bf2a82..443acc968d 100644 --- a/zerver/models/users.py +++ b/zerver/models/users.py @@ -951,8 +951,23 @@ def remote_user_to_email(remote_user: str) -> str: post_save.connect(flush_user_profile, sender=UserProfile) -@cache_with_key(user_profile_by_id_cache_key, timeout=3600 * 24 * 7) -def get_user_profile_by_id(user_profile_id: int) -> UserProfile: +def base_bulk_get_user_queryset() -> QuerySet[UserProfile]: + """Base select_related options for UserProfile for general user; + prefetches can_access_all_users_group, which is often necessary + for calculations of where events should be sent. + """ + return UserProfile.objects.select_related( + "realm", + "realm__can_access_all_users_group", + "realm__can_access_all_users_group__named_user_group", + ) + + +def base_get_user_queryset() -> QuerySet[UserProfile]: + """Base select_related options for fetching a single user object. + In contrast with base_bulk_get_user_queryset, additionally fetches + fields that are relevant for this user sending a message. + """ return UserProfile.objects.select_related( "realm", "realm__can_access_all_users_group", @@ -962,7 +977,12 @@ def get_user_profile_by_id(user_profile_id: int) -> UserProfile: "realm__direct_message_permission_group", "realm__direct_message_permission_group__named_user_group", "bot_owner", - ).get(id=user_profile_id) + ) + + +@cache_with_key(user_profile_by_id_cache_key, timeout=3600 * 24 * 7) +def get_user_profile_by_id(user_profile_id: int) -> UserProfile: + return base_get_user_queryset().get(id=user_profile_id) def get_user_profile_by_email(email: str) -> UserProfile: @@ -1007,16 +1027,7 @@ def get_user_by_delivery_email(email: str, realm: "Realm") -> UserProfile: EMAIL_ADDRESS_VISIBILITY_ADMINS security model. Use get_user in those code paths. """ - return UserProfile.objects.select_related( - "realm", - "realm__can_access_all_users_group", - "realm__can_access_all_users_group__named_user_group", - "realm__direct_message_initiator_group", - "realm__direct_message_initiator_group__named_user_group", - "realm__direct_message_permission_group", - "realm__direct_message_permission_group__named_user_group", - "bot_owner", - ).get(delivery_email__iexact=email.strip(), realm=realm) + return base_get_user_queryset().get(delivery_email__iexact=email.strip(), realm=realm) def get_users_by_delivery_email(emails: set[str], realm: "Realm") -> QuerySet[UserProfile]: @@ -1051,16 +1062,7 @@ def get_user(email: str, realm: "Realm") -> UserProfile: EMAIL_ADDRESS_VISIBILITY_ADMINS. In those code paths, use get_user_by_delivery_email. """ - return UserProfile.objects.select_related( - "realm", - "realm__can_access_all_users_group", - "realm__can_access_all_users_group__named_user_group", - "realm__direct_message_initiator_group", - "realm__direct_message_initiator_group__named_user_group", - "realm__direct_message_permission_group", - "realm__direct_message_permission_group__named_user_group", - "bot_owner", - ).get(email__iexact=email.strip(), realm=realm) + return base_get_user_queryset().get(email__iexact=email.strip(), realm=realm) def get_active_user(email: str, realm: "Realm") -> UserProfile: @@ -1073,12 +1075,7 @@ def get_active_user(email: str, realm: "Realm") -> UserProfile: def get_user_profile_by_id_in_realm(uid: int, realm: "Realm") -> UserProfile: - return UserProfile.objects.select_related( - "realm", - "realm__can_access_all_users_group", - "realm__can_access_all_users_group__named_user_group", - "bot_owner", - ).get(id=uid, realm=realm) + return base_bulk_get_user_queryset().get(id=uid, realm=realm) def get_active_user_profile_by_id_in_realm(uid: int, realm: "Realm") -> UserProfile: