From b742f1241f02ed9b22a67b209ee3dc30cb67fa06 Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Fri, 14 Jul 2023 10:37:29 +0000 Subject: [PATCH] realm emoji: Use a single cache for all lookups. The active realm emoji are just a subset of all your realm emoji, so just use a single cache entry per realm. Cache misses should be very infrequent per realm. If a realm has lots of deactivated realm emoji, then there's a minor expense to deserialize them, but that is gonna be dwarfed by all the other more expensive operations in message-send. I also renamed the two related functions. I erred on the side of using somewhat verbose names, as we don't want folks to confuse the two use cases. Fortunately there are somewhat natural affordances to use one or the other, and mypy helps too. Finally, I use realm_id instead of realm in places where we don't need the full Realm object. --- zerver/actions/realm_emoji.py | 14 ++++++-- zerver/lib/emoji.py | 22 +++++++++--- zerver/lib/events.py | 3 +- zerver/lib/markdown/__init__.py | 10 ++++-- zerver/models.py | 44 ++++++------------------ zerver/tests/test_audit_log.py | 3 +- zerver/tests/test_email_notifications.py | 4 ++- zerver/views/realm_emoji.py | 6 ++-- 8 files changed, 58 insertions(+), 48 deletions(-) diff --git a/zerver/actions/realm_emoji.py b/zerver/actions/realm_emoji.py index a2faf6e8a1..9c67dd316a 100644 --- a/zerver/actions/realm_emoji.py +++ b/zerver/actions/realm_emoji.py @@ -11,7 +11,15 @@ from zerver.lib.emoji import get_emoji_file_name from zerver.lib.exceptions import JsonableError from zerver.lib.pysa import mark_sanitized from zerver.lib.upload import upload_emoji_image -from zerver.models import EmojiInfo, Realm, RealmAuditLog, RealmEmoji, UserProfile, active_user_ids +from zerver.models import ( + EmojiInfo, + Realm, + RealmAuditLog, + RealmEmoji, + UserProfile, + active_user_ids, + get_all_custom_emoji_for_realm, +) from zerver.tornado.django_api import send_event_on_commit @@ -49,7 +57,7 @@ def check_add_realm_emoji( realm_emoji.is_animated = is_animated realm_emoji.save(update_fields=["file_name", "is_animated"]) - realm_emoji_dict = realm.get_emoji() + realm_emoji_dict = get_all_custom_emoji_for_realm(realm.id) RealmAuditLog.objects.create( realm=realm, acting_user=author, @@ -72,7 +80,7 @@ def do_remove_realm_emoji(realm: Realm, name: str, *, acting_user: Optional[User emoji.deactivated = True emoji.save(update_fields=["deactivated"]) - realm_emoji_dict = realm.get_emoji() + realm_emoji_dict = get_all_custom_emoji_for_realm(realm.id) RealmAuditLog.objects.create( realm=realm, acting_user=acting_user, diff --git a/zerver/lib/emoji.py b/zerver/lib/emoji.py index c406250337..56dd59c48f 100644 --- a/zerver/lib/emoji.py +++ b/zerver/lib/emoji.py @@ -9,7 +9,14 @@ from django.utils.translation import gettext as _ from zerver.lib.exceptions import JsonableError from zerver.lib.storage import static_path from zerver.lib.upload import upload_backend -from zerver.models import Reaction, Realm, RealmEmoji, UserProfile +from zerver.models import ( + Reaction, + Realm, + RealmEmoji, + UserProfile, + get_all_custom_emoji_for_realm, + get_name_keyed_dict_for_active_realm_emoji, +) emoji_codes_path = static_path("generated/emoji/emoji_codes.json") if not os.path.exists(emoji_codes_path): # nocoverage @@ -56,10 +63,12 @@ def translate_emoticons(text: str) -> str: def emoji_name_to_emoji_code(realm: Realm, emoji_name: str) -> Tuple[str, str]: - realm_emojis = realm.get_active_emoji() - realm_emoji = realm_emojis.get(emoji_name) + # TODO: Just ask callers for realm_id. Also consider returning a + # tiny dataclass instead of a tuple. + realm_emoji_dict = get_name_keyed_dict_for_active_realm_emoji(realm.id) + realm_emoji = realm_emoji_dict.get(emoji_name) if realm_emoji is not None: - return str(realm_emojis[emoji_name]["id"]), Reaction.REALM_EMOJI + return str(realm_emoji["id"]), Reaction.REALM_EMOJI if emoji_name == "zulip": return emoji_name, Reaction.ZULIP_EXTRA_EMOJI if emoji_name in name_to_codepoint: @@ -71,7 +80,10 @@ def check_emoji_request(realm: Realm, emoji_name: str, emoji_code: str, emoji_ty # For a given realm and emoji type, checks whether an emoji # code is valid for new reactions, or not. if emoji_type == "realm_emoji": - realm_emojis = realm.get_emoji() + # We cache emoji, so this generally avoids a round trip, + # but it does require deserializing a bigger data structure + # than we need. + realm_emojis = get_all_custom_emoji_for_realm(realm.id) realm_emoji = realm_emojis.get(emoji_code) if realm_emoji is None: raise JsonableError(_("Invalid custom emoji.")) diff --git a/zerver/lib/events.py b/zerver/lib/events.py index 448af85a90..3215eaa532 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -67,6 +67,7 @@ from zerver.models import ( UserStatus, UserTopic, custom_profile_fields_for_realm, + get_all_custom_emoji_for_realm, get_default_stream_groups, get_realm_domains, get_realm_playgrounds, @@ -384,7 +385,7 @@ def fetch_initial_state_data( state["realm_domains"] = get_realm_domains(realm) if want("realm_emoji"): - state["realm_emoji"] = realm.get_emoji() + state["realm_emoji"] = get_all_custom_emoji_for_realm(realm.id) if want("realm_linkifiers"): if linkifier_url_template: diff --git a/zerver/lib/markdown/__init__.py b/zerver/lib/markdown/__init__.py index 5e8b750ada..9e93f8b9bd 100644 --- a/zerver/lib/markdown/__init__.py +++ b/zerver/lib/markdown/__init__.py @@ -70,7 +70,13 @@ from zerver.lib.timezone import common_timezones from zerver.lib.types import LinkifierDict from zerver.lib.url_encoding import encode_stream, hash_util_encode from zerver.lib.url_preview.types import UrlEmbedData, UrlOEmbedData -from zerver.models import EmojiInfo, Message, Realm, linkifiers_for_realm +from zerver.models import ( + EmojiInfo, + Message, + Realm, + get_name_keyed_dict_for_active_realm_emoji, + linkifiers_for_realm, +) ReturnT = TypeVar("ReturnT") @@ -2537,7 +2543,7 @@ def do_convert( stream_name_info = mention_data.get_stream_name_map(stream_names) if content_has_emoji_syntax(content): - active_realm_emoji = message_realm.get_active_emoji() + active_realm_emoji = get_name_keyed_dict_for_active_realm_emoji(message_realm.id) else: active_realm_emoji = {} diff --git a/zerver/models.py b/zerver/models.py index 159be1eb52..2af0a1960b 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -239,14 +239,10 @@ def get_recipient_ids( return to, recipient_type_str -def get_realm_emoji_cache_key(realm_id: int) -> str: +def get_all_custom_emoji_for_realm_cache_key(realm_id: int) -> str: return f"realm_emoji:{realm_id}" -def get_active_realm_emoji_cache_key(realm_id: int) -> str: - return f"active_realm_emoji:{realm_id}" - - # This simple call-once caching saves ~500us in auth_enabled_helper, # which is a significant optimization for common_context. Note that # these values cannot change in a running production system, but do @@ -837,16 +833,6 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub ret[realm_authentication_method.name] = True return ret - # `realm` instead of `self` here to make sure the parameters of the cache key - # function matches the original method. - @cache_with_key(lambda realm: get_realm_emoji_cache_key(realm.id), timeout=3600 * 24 * 7) - def get_emoji(realm) -> Dict[str, EmojiInfo]: # noqa: N805 - return get_realm_emoji_uncached(realm.id) - - @cache_with_key(lambda realm: get_active_realm_emoji_cache_key(realm.id), timeout=3600 * 24 * 7) - def get_active_emoji(realm) -> Dict[str, EmojiInfo]: # noqa: N805 - return get_active_realm_emoji_uncached(realm.id) - def get_admin_users_and_bots( self, include_realm_owners: bool = True ) -> QuerySet["UserProfile"]: @@ -1161,7 +1147,7 @@ class RealmEmoji(models.Model): return f"{self.realm.string_id}: {self.id} {self.name} {self.deactivated} {self.file_name}" -def get_realm_emoji_dicts(realm_id: int, only_active_emojis: bool = False) -> Dict[str, EmojiInfo]: +def get_all_custom_emoji_for_realm_uncached(realm_id: int) -> Dict[str, EmojiInfo]: # RealmEmoji objects with file_name=None are still in the process # of being uploaded, and we expect to be cleaned up by a # try/finally block if the upload fails, so it's correct to @@ -1169,8 +1155,6 @@ def get_realm_emoji_dicts(realm_id: int, only_active_emojis: bool = False) -> Di query = RealmEmoji.objects.filter(realm_id=realm_id).exclude( file_name=None, ) - if only_active_emojis: - query = query.filter(deactivated=False) d = {} from zerver.lib.emoji import get_emoji_url @@ -1202,16 +1186,15 @@ def get_realm_emoji_dicts(realm_id: int, only_active_emojis: bool = False) -> Di return d -def get_realm_emoji_uncached(realm_id: int) -> Dict[str, EmojiInfo]: - return get_realm_emoji_dicts(realm_id) +@cache_with_key(get_all_custom_emoji_for_realm_cache_key, timeout=3600 * 24 * 7) +def get_all_custom_emoji_for_realm(realm_id: int) -> Dict[str, EmojiInfo]: + return get_all_custom_emoji_for_realm_uncached(realm_id) -def get_active_realm_emoji_uncached(realm_id: int) -> Dict[str, EmojiInfo]: - realm_emojis = get_realm_emoji_dicts(realm_id, only_active_emojis=True) - d = {} - for emoji_id, emoji_dict in realm_emojis.items(): - d[emoji_dict["name"]] = emoji_dict - return d +def get_name_keyed_dict_for_active_realm_emoji(realm_id: int) -> Dict[str, EmojiInfo]: + # It's important to use the cached version here. + realm_emojis = get_all_custom_emoji_for_realm(realm_id) + return {row["name"]: row for row in realm_emojis.values() if not row["deactivated"]} def flush_realm_emoji(*, instance: RealmEmoji, **kwargs: object) -> None: @@ -1227,13 +1210,8 @@ def flush_realm_emoji(*, instance: RealmEmoji, **kwargs: object) -> None: return realm_id = instance.realm_id cache_set( - get_realm_emoji_cache_key(realm_id), - get_realm_emoji_uncached(realm_id), - timeout=3600 * 24 * 7, - ) - cache_set( - get_active_realm_emoji_cache_key(realm_id), - get_active_realm_emoji_uncached(realm_id), + get_all_custom_emoji_for_realm_cache_key(realm_id), + get_all_custom_emoji_for_realm_uncached(realm_id), timeout=3600 * 24 * 7, ) diff --git a/zerver/tests/test_audit_log.py b/zerver/tests/test_audit_log.py index 9446573f49..70cf29d197 100644 --- a/zerver/tests/test_audit_log.py +++ b/zerver/tests/test_audit_log.py @@ -84,6 +84,7 @@ from zerver.models import ( Subscription, UserGroup, UserProfile, + get_all_custom_emoji_for_realm, get_realm, get_realm_domains, get_realm_playgrounds, @@ -1001,7 +1002,7 @@ class TestRealmAuditLog(ZulipTestCase): def test_realm_emoji_entries(self) -> None: user = self.example_user("iago") - realm_emoji_dict = user.realm.get_emoji() + realm_emoji_dict = get_all_custom_emoji_for_realm(user.realm_id) now = timezone_now() with get_test_image_file("img.png") as img_file: # Because we want to verify the IntegrityError handling diff --git a/zerver/tests/test_email_notifications.py b/zerver/tests/test_email_notifications.py index a4d0862e0d..78f1c9240f 100644 --- a/zerver/tests/test_email_notifications.py +++ b/zerver/tests/test_email_notifications.py @@ -42,6 +42,7 @@ from zerver.models import ( UserMessage, UserProfile, UserTopic, + get_name_keyed_dict_for_active_realm_emoji, get_realm, get_stream, ) @@ -1492,7 +1493,8 @@ class TestMissedMessages(ZulipTestCase): self.example_user("hamlet"), "Extremely personal message with a realm emoji :green_tick:!", ) - realm_emoji_id = realm.get_active_emoji()["green_tick"]["id"] + realm_emoji_dict = get_name_keyed_dict_for_active_realm_emoji(realm.id) + realm_emoji_id = realm_emoji_dict["green_tick"]["id"] realm_emoji_url = ( f"http://zulip.testserver/user_avatars/{realm.id}/emoji/images/{realm_emoji_id}.png" ) diff --git a/zerver/views/realm_emoji.py b/zerver/views/realm_emoji.py index 4d72a0d9f6..e7389a0a2f 100644 --- a/zerver/views/realm_emoji.py +++ b/zerver/views/realm_emoji.py @@ -9,13 +9,15 @@ from zerver.lib.emoji import check_remove_custom_emoji, check_valid_emoji_name, from zerver.lib.exceptions import JsonableError, ResourceNotFoundError from zerver.lib.request import REQ, has_request_variables from zerver.lib.response import json_success -from zerver.models import RealmEmoji, UserProfile +from zerver.models import RealmEmoji, UserProfile, get_all_custom_emoji_for_realm def list_emoji(request: HttpRequest, user_profile: UserProfile) -> HttpResponse: # We don't do any checks here because the list of realm # emoji is public. - return json_success(request, data={"emoji": user_profile.realm.get_emoji()}) + return json_success( + request, data=dict(emoji=get_all_custom_emoji_for_realm(user_profile.realm_id)) + ) @require_member_or_admin