From 51cef01c2921c9c35369f00052c1cba0c0ac35d2 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Sun, 17 Aug 2025 03:25:23 +0800 Subject: [PATCH] message: Use .is_channel_message column instead of is_stream_message(). This avoids a potential unnecessary message.recipient fetch required by is_stream_message(). is_stream_message() methods precedes the addition of the denormalized is_channel_message column and is now unnecessary. In practice, we usually fetch Message objects with `.recipient` already, so I don't expect any notable performance impact here - but it's still a useful change to make. --- zerver/actions/message_delete.py | 2 +- zerver/actions/message_edit.py | 10 +++++----- zerver/actions/message_flags.py | 2 +- zerver/actions/message_send.py | 10 +++++----- zerver/actions/uploads.py | 8 +++++++- zerver/lib/attachments.py | 4 ++-- zerver/lib/email_notifications.py | 2 +- zerver/lib/message.py | 6 +++--- zerver/lib/message_report.py | 2 +- zerver/lib/push_notifications.py | 4 ++-- zerver/lib/reminders.py | 2 +- zerver/lib/test_classes.py | 1 + zerver/models/messages.py | 10 ---------- zerver/models/scheduled_jobs.py | 2 +- zerver/tests/test_message_delete.py | 2 +- zerver/tests/test_message_fetch.py | 10 +++++----- zerver/tests/test_retention.py | 2 +- zerver/views/message_edit.py | 6 +++--- 18 files changed, 41 insertions(+), 44 deletions(-) diff --git a/zerver/actions/message_delete.py b/zerver/actions/message_delete.py index 44c2ffa930..0e51cf05f2 100644 --- a/zerver/actions/message_delete.py +++ b/zerver/actions/message_delete.py @@ -111,7 +111,7 @@ def do_delete_messages( ) stream_by_recipient_id = {} for message in messages: - if message.is_stream_message(): + if message.is_channel_message: recipient_id = message.recipient_id # topics are case-insensitive. topic_name = message.topic_name().lower() diff --git a/zerver/actions/message_edit.py b/zerver/actions/message_edit.py index e07ad9bf95..a534ace4b9 100644 --- a/zerver/actions/message_edit.py +++ b/zerver/actions/message_edit.py @@ -126,7 +126,7 @@ def validate_message_edit_payload( if topic_name is None and content is None and stream_id is None: raise JsonableError(_("Nothing to change")) - if not message.is_stream_message(): + if not message.is_channel_message: if stream_id is not None: raise JsonableError(_("Direct messages cannot be moved to channels.")) if topic_name is not None: @@ -1454,7 +1454,7 @@ def build_message_edit_request( content = "(deleted)" new_content = normalize_body(content) - if not message.is_stream_message(): + if not message.is_channel_message: # We have already validated that at least one of content, topic, or stream # must be modified, and for DMs, only the content can be edited. return DirectMessageEditRequest( @@ -1633,12 +1633,12 @@ def check_update_message( ) links_for_embed |= rendering_result.links_for_preview - if message.is_stream_message() and rendering_result.mentions_stream_wildcard: + if message.is_channel_message and rendering_result.mentions_stream_wildcard: stream = access_stream_by_id(user_profile, message.recipient.type_id)[0] if not stream_wildcard_mention_allowed(message.sender, stream, message.realm): raise StreamWildcardMentionNotAllowedError - if message.is_stream_message() and rendering_result.mentions_topic_wildcard: + if message.is_channel_message and rendering_result.mentions_topic_wildcard: topic_participant_count = len( participants_for_topic(message.realm.id, message.recipient.id, message.topic_name()) ) @@ -1653,7 +1653,7 @@ def check_update_message( if isinstance(message_edit_request, StreamMessageEditRequest): if message_edit_request.is_stream_edited: - assert message.is_stream_message() + assert message.is_channel_message if not can_move_messages_out_of_channel(user_profile, message_edit_request.orig_stream): raise JsonableError(_("You don't have permission to move this message")) diff --git a/zerver/actions/message_flags.py b/zerver/actions/message_flags.py index 3af444dbf3..c23ac7b3ac 100644 --- a/zerver/actions/message_flags.py +++ b/zerver/actions/message_flags.py @@ -212,7 +212,7 @@ def do_update_mobile_push_notification( # in a sent notification if a message was edited to mention a # group rather than a user (or vice versa), though it is likely # not worth the effort to do such a change. - if not message.is_stream_message(): + if not message.is_channel_message: return remove_notify_users = prior_mention_user_ids - mentions_user_ids - stream_push_user_ids diff --git a/zerver/actions/message_send.py b/zerver/actions/message_send.py index 290554f344..61355e35d2 100644 --- a/zerver/actions/message_send.py +++ b/zerver/actions/message_send.py @@ -608,7 +608,7 @@ def build_message_send_dict( message_sender=message.sender, ) - if message.is_stream_message(): + if message.is_channel_message: stream_id = message.recipient.type_id stream_topic: StreamTopicTarget | None = StreamTopicTarget( stream_id=stream_id, @@ -745,7 +745,7 @@ def create_user_messages( # These properties on the Message are set via # render_message_markdown by code in the Markdown inline patterns ids_with_alert_words = rendering_result.user_ids_with_alert_words - is_stream_message = message.is_stream_message() + is_stream_message = message.is_channel_message base_flags = 0 if rendering_result.mentions_stream_wildcard: @@ -968,7 +968,7 @@ def do_send_messages( # * Adding links to the embed_links queue for open graph processing. for send_request in send_message_requests: realm_id: int | None = None - if send_request.message.is_stream_message(): + if send_request.message.is_channel_message: if send_request.stream is None: stream_id = send_request.message.recipient.type_id send_request.stream = Stream.objects.get(id=stream_id) @@ -1172,7 +1172,7 @@ def do_send_messages( realm_host=send_request.realm.host, ) - if send_request.message.is_stream_message(): + if send_request.message.is_channel_message: # Note: This is where authorization for single-stream # get_updates happens! We only attach stream data to the # notify new_message request if it's a public stream, @@ -1237,7 +1237,7 @@ def do_send_messages( # Check if this is a 1:1 DM between a user and the Welcome Bot, # in which case we may want to send an automated response. - if not send_request.message.is_stream_message() and len(send_request.active_user_ids) == 2: + if not send_request.message.is_channel_message and len(send_request.active_user_ids) == 2: welcome_bot_id = get_system_bot(settings.WELCOME_BOT, send_request.realm.id).id if ( welcome_bot_id in send_request.active_user_ids diff --git a/zerver/actions/uploads.py b/zerver/actions/uploads.py index cf1a77945d..a83bbb4d6e 100644 --- a/zerver/actions/uploads.py +++ b/zerver/actions/uploads.py @@ -43,7 +43,13 @@ def do_claim_attachments( user_profile = message.sender is_message_realm_public = False is_message_web_public = False - if message.is_stream_message(): + if isinstance(message, Message): + is_channel_message = message.is_channel_message + else: + assert isinstance(message, ScheduledMessage) + is_channel_message = message.is_channel_message() + + if is_channel_message: stream = Stream.objects.get(id=message.recipient.type_id) is_message_realm_public = stream.is_public() is_message_web_public = stream.is_web_public diff --git a/zerver/lib/attachments.py b/zerver/lib/attachments.py index 1f3fa5c079..537cdf6b9a 100644 --- a/zerver/lib/attachments.py +++ b/zerver/lib/attachments.py @@ -140,7 +140,7 @@ def validate_attachment_request( user_profile=user_profile, message__in=messages ).select_related("message", "message__recipient") for um in usermessage_rows: - if not um.message.is_stream_message(): + if not um.message.is_channel_message: # If the attachment was sent in a direct message or group direct # message then anyone who received that message can access it. return True, attachment @@ -164,7 +164,7 @@ def validate_attachment_request( message_channel_ids = set() for message in messages: - if message.is_stream_message(): + if message.is_channel_message: message_channel_ids.add(message.recipient.type_id) if len(message_channel_ids) == 0: diff --git a/zerver/lib/email_notifications.py b/zerver/lib/email_notifications.py index 74573f393f..b941ce1a40 100644 --- a/zerver/lib/email_notifications.py +++ b/zerver/lib/email_notifications.py @@ -685,7 +685,7 @@ def handle_missedmessage_emails( for msg_list in messages_by_bucket.values(): msg = min(msg_list, key=lambda msg: msg.date_sent) - if msg.is_stream_message() and UserMessage.has_any_mentions(user_profile_id, msg.id): + if msg.is_channel_message and UserMessage.has_any_mentions(user_profile_id, msg.id): context_messages = get_context_for_message(msg) filtered_context_messages = bulk_access_messages( user_profile, context_messages, is_modifying_message=False diff --git a/zerver/lib/message.py b/zerver/lib/message.py index 46f2771f3c..ff8155a8cf 100644 --- a/zerver/lib/message.py +++ b/zerver/lib/message.py @@ -465,7 +465,7 @@ def access_web_public_message( except Message.DoesNotExist: raise MissingAuthenticationError - if not message.is_stream_message(): + if not message.is_channel_message: raise MissingAuthenticationError queryset = get_web_public_streams_queryset(realm) @@ -615,7 +615,7 @@ def event_recipient_ids_for_action_on_messages( return set(usermessages.values_list("user_profile_id", flat=True)) sample_message = messages[0] - if not sample_message.is_stream_message(): + if not sample_message.is_channel_message: # For DM, event is sent to users who actually received the message. return get_user_ids_having_usermessage_row_for_messages(message_ids) @@ -1720,7 +1720,7 @@ def should_change_visibility_policy( def set_visibility_policy_possible(user_profile: UserProfile, message: Message) -> bool: """If the user can set a visibility policy.""" - if not message.is_stream_message(): + if not message.is_channel_message: return False if user_profile.is_bot: diff --git a/zerver/lib/message_report.py b/zerver/lib/message_report.py index 37a37e6bed..ffb51bbedd 100644 --- a/zerver/lib/message_report.py +++ b/zerver/lib/message_report.py @@ -59,7 +59,7 @@ def send_message_report( last_user_mention=last_user_mention, ) else: - assert reported_message.is_stream_message() is True + assert reported_message.is_channel_message is True topic_name = reported_message.topic_name() channel_id = reported_message.recipient.type_id channel_name = reported_message.recipient.label() diff --git a/zerver/lib/push_notifications.py b/zerver/lib/push_notifications.py index 8648644c08..1aac367121 100644 --- a/zerver/lib/push_notifications.py +++ b/zerver/lib/push_notifications.py @@ -1053,7 +1053,7 @@ def get_apns_alert_title(message: Message, language: str) -> str: assert isinstance(recipients, list) if len(recipients) > 2: return ", ".join(sorted(r["full_name"] for r in recipients)) - elif message.is_stream_message(): + elif message.is_channel_message: stream_name = get_message_stream_name_from_database(message) topic_display_name = get_topic_display_name(message.topic_name(), language) return f"#{stream_name} > {topic_display_name}" @@ -1712,7 +1712,7 @@ def handle_push_notification(user_profile_id: int, missed_message: dict[str, Any user_profile, {trigger}, mentioned_user_group_members_count ) - if message.is_stream_message(): + if message.is_channel_message: # This will almost always be True. The corner case where you # can be receiving a message from a user you cannot access # involves your being a guest user whose access is restricted diff --git a/zerver/lib/reminders.py b/zerver/lib/reminders.py index 78c952519e..f1cbccb20b 100644 --- a/zerver/lib/reminders.py +++ b/zerver/lib/reminders.py @@ -32,7 +32,7 @@ def get_reminder_formatted_content( if note: note = normalize_note_text(note) - if message.is_stream_message(): + if message.is_channel_message: # We don't need to check access here since we already have the message # whose access has already been checked by the caller. stream = Stream.objects.get( diff --git a/zerver/lib/test_classes.py b/zerver/lib/test_classes.py index a637d8b21b..31f193ccb4 100644 --- a/zerver/lib/test_classes.py +++ b/zerver/lib/test_classes.py @@ -2775,6 +2775,7 @@ class PushNotificationTestCase(BouncerTestCase): rendered_content="This is test content", date_sent=timezone_now(), sending_client=self.sending_client, + is_channel_message=type == Recipient.STREAM, ) message.set_topic_name("Test topic") message.save() diff --git a/zerver/models/messages.py b/zerver/models/messages.py index 25f44ad122..dd3d9906ca 100644 --- a/zerver/models/messages.py +++ b/zerver/models/messages.py @@ -283,16 +283,6 @@ class Message(AbstractMessage): def set_topic_name(self, topic_name: str) -> None: self.subject = topic_name - def is_stream_message(self) -> bool: - """ - Find out whether a message is a stream message by - looking up its recipient.type. TODO: Make this - an easier operation by denormalizing the message - type onto Message, either explicitly (message.type) - or implicitly (message.stream_id is not None). - """ - return self.recipient.type == Recipient.STREAM - def get_realm(self) -> Realm: return self.realm diff --git a/zerver/models/scheduled_jobs.py b/zerver/models/scheduled_jobs.py index e56eafb80e..1e3183df88 100644 --- a/zerver/models/scheduled_jobs.py +++ b/zerver/models/scheduled_jobs.py @@ -225,7 +225,7 @@ class ScheduledMessage(models.Model): def set_topic_name(self, topic_name: str) -> None: self.subject = topic_name - def is_stream_message(self) -> bool: + def is_channel_message(self) -> bool: return self.recipient.type == Recipient.STREAM def to_dict(self) -> APIScheduledStreamMessageDict | APIScheduledDirectMessageDict: diff --git a/zerver/tests/test_message_delete.py b/zerver/tests/test_message_delete.py index 6cb08389bd..5803bffab6 100644 --- a/zerver/tests/test_message_delete.py +++ b/zerver/tests/test_message_delete.py @@ -914,7 +914,7 @@ class DeleteMessageTest(ZulipTestCase): self.assertEqual(stream.first_message_id, message_ids[1]) all_messages = Message.objects.filter(id__in=message_ids) - with self.assert_database_query_count(27): + with self.assert_database_query_count(25): do_delete_messages(realm, all_messages, acting_user=None) stream = get_stream(stream_name, realm) self.assertEqual(stream.first_message_id, None) diff --git a/zerver/tests/test_message_fetch.py b/zerver/tests/test_message_fetch.py index fbec54fb65..06697be469 100644 --- a/zerver/tests/test_message_fetch.py +++ b/zerver/tests/test_message_fetch.py @@ -2433,7 +2433,7 @@ class GetOldMessagesTest(ZulipTestCase): self.assertFalse(aaron.is_active) personals = [ - m for m in get_user_messages(self.example_user("hamlet")) if not m.is_stream_message() + m for m in get_user_messages(self.example_user("hamlet")) if not m.is_channel_message ] for personal in personals: emails = dr_emails(get_display_recipient(personal.recipient)) @@ -2748,7 +2748,7 @@ class GetOldMessagesTest(ZulipTestCase): self.send_personal_message(hamlet, hamlet) messages = get_user_messages(hamlet) - channel_messages = [msg for msg in messages if msg.is_stream_message()] + channel_messages = [msg for msg in messages if msg.is_channel_message] self.assertGreater(len(messages), len(channel_messages)) self.assert_length(channel_messages, num_messages_per_channel * len(channel_names)) @@ -2804,7 +2804,7 @@ class GetOldMessagesTest(ZulipTestCase): ) messages = get_user_messages(self.mit_user("starnine")) - channel_messages = [msg for msg in messages if msg.is_stream_message()] + channel_messages = [msg for msg in messages if msg.is_channel_message] self.assert_length(result["messages"], 2) for i, message in enumerate(result["messages"]): @@ -2835,7 +2835,7 @@ class GetOldMessagesTest(ZulipTestCase): ) messages = get_user_messages(mit_user_profile) - channel_messages = [msg for msg in messages if msg.is_stream_message()] + channel_messages = [msg for msg in messages if msg.is_channel_message] self.assert_length(result["messages"], 5) for i, message in enumerate(result["messages"]): self.assertEqual(message["type"], "stream") @@ -2869,7 +2869,7 @@ class GetOldMessagesTest(ZulipTestCase): ) messages = get_user_messages(mit_user_profile) - channel_messages = [msg for msg in messages if msg.is_stream_message()] + channel_messages = [msg for msg in messages if msg.is_channel_message] self.assert_length(result["messages"], 7) for i, message in enumerate(result["messages"]): self.assertEqual(message["type"], "stream") diff --git a/zerver/tests/test_retention.py b/zerver/tests/test_retention.py index 6be28fb3c0..06ecd19cbb 100644 --- a/zerver/tests/test_retention.py +++ b/zerver/tests/test_retention.py @@ -1151,7 +1151,7 @@ class TestDoDeleteMessages(ZulipTestCase): message_ids = [self.send_stream_message(cordelia, "Verona", str(i)) for i in range(10)] messages = Message.objects.filter(id__in=message_ids) - with self.assert_database_query_count(32): + with self.assert_database_query_count(23): do_delete_messages(realm, messages, acting_user=None) self.assertFalse(Message.objects.filter(id__in=message_ids).exists()) diff --git a/zerver/views/message_edit.py b/zerver/views/message_edit.py index 1186bc3b5f..b28982ba37 100644 --- a/zerver/views/message_edit.py +++ b/zerver/views/message_edit.py @@ -46,7 +46,7 @@ def fill_edit_history_entries( """ prev_content = message.content prev_rendered_content = message.rendered_content - is_channel_message = message.is_stream_message() + is_channel_message = message.is_channel_message if is_channel_message: prev_topic_name = maybe_rename_empty_topic_to_general_chat( message.topic_name(), is_channel_message, allow_empty_topic_name @@ -185,7 +185,7 @@ def validate_can_delete_message(user_profile: UserProfile, message: Message) -> return stream: Stream | None = None - if message.is_stream_message(): + if message.is_channel_message: stream = get_stream_by_id_in_realm(message.recipient.type_id, user_profile.realm) if can_delete_any_message_in_channel(user_profile, stream): return @@ -195,7 +195,7 @@ def validate_can_delete_message(user_profile: UserProfile, message: Message) -> raise JsonableError(_("You don't have permission to delete this message")) if not user_profile.can_delete_own_message(): - if not message.is_stream_message(): + if not message.is_channel_message: raise JsonableError(_("You don't have permission to delete this message")) assert stream is not None