Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

message_edit: Carry the QuerySet through as much as possible. 

Rather than pass around a list of message objects in-memory, we
instead keep the same constructed QuerySet which includes the later
propagated messages (if any), and use that same query to pick out
affected Attachment objects, rather than limiting to the set of ids.
This is not necessarily a win -- the list of message-ids *may* be very
long, and thus the query may be more concise, easier to send to
PostgreSQL, and faster for PostgreSQL to parse.  However, the list of
ids is almost certainly better-indexed.

After processing the move, the QuerySet must be re-defined as a search
of ids (and possibly a very long list of such), since there is no
other way which is guaranteed to correctly single out the moved
messages.  At this point, it is mostly equivalent to the list of
Message objects, and certainly takes no less memory.

(cherry picked from commit eaf58438ec)
This commit is contained in:
Alex Vandiver 2023-09-26 18:44:28 +00:00 committed by Tim Abbott
parent 210c9aaf1c
commit 3e2b295140
4 changed files with 50 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

52
zerver/actions/message_edit.py
View File

@ -5,7 +5,7 @@ from typing import AbstractSet, Any, Dict, Iterable, List, Optional, Set, Tuple
from django.conf import settings
from django.db import transaction
from django.db.models import Q
from django.db.models import Q, QuerySet
from django.utils.timezone import now as timezone_now
from django.utils.translation import gettext as _
from django.utils.translation import gettext_lazy
@ -125,7 +125,7 @@ def maybe_send_resolve_topic_notifications(
stream: Stream,
old_topic: str,
new_topic: str,
changed_messages: List[Message],
changed_messages: QuerySet[Message],
) -> Optional[int]:
"""Returns resolved_topic_message_id if resolve topic notifications were in fact sent."""
# Note that topics will have already been stripped in check_update_message.
@ -158,9 +158,11 @@ def maybe_send_resolve_topic_notifications(
# Compute the users who either sent or reacted to messages that
# were moved via the "resolve topic' action. Only those users
# should be eligible for this message being managed as unread.
affected_participant_ids = {message.sender_id for message in changed_messages} | set(
Reaction.objects.filter(message__in=changed_messages).values_list(
"user_profile_id", flat=True
affected_participant_ids = set(
changed_messages.values_list("sender_id", flat=True).union(
Reaction.objects.filter(message__in=changed_messages).values_list(
"user_profile_id", flat=True
)
)
)
sender = get_system_bot(settings.NOTIFICATION_BOT, user_profile.realm_id)
@ -409,8 +411,6 @@ def do_update_message(
"timestamp": event["edit_timestamp"],
}
changed_messages = [target_message]
realm = user_profile.realm
stream_being_edited = None
@ -631,6 +631,12 @@ def do_update_message(
realm.id, target_stream.recipient_id, target_topic
).exists()
changed_messages = Message.objects.filter(id=target_message.id)
changed_message_ids = [target_message.id]
changed_messages_count = 1
save_changes_for_propagation_mode = lambda: Message.objects.filter(
id=target_message.id
).select_related(*Message.DEFAULT_SELECT_RELATED)
if propagate_mode in ["change_later", "change_all"]:
assert topic_name is not None or new_stream is not None
assert stream_being_edited is not None
@ -647,7 +653,7 @@ def do_update_message(
topic_only_edit_history_event["prev_stream"] = edit_history_event["prev_stream"]
topic_only_edit_history_event["stream"] = edit_history_event["stream"]
messages_list = update_messages_for_topic_edit(
later_messages, save_changes_for_propagation_mode = update_messages_for_topic_edit(
acting_user=user_profile,
edited_message=target_message,
propagate_mode=propagate_mode,
@ -658,12 +664,12 @@ def do_update_message(
edit_history_event=topic_only_edit_history_event,
last_edit_time=timestamp,
)
changed_messages += messages_list
changed_messages |= later_messages
changed_message_ids = list(changed_messages.values_list("id", flat=True))
changed_messages_count = len(changed_message_ids)
if new_stream is not None:
assert stream_being_edited is not None
changed_message_ids = [msg.id for msg in changed_messages]
if gaining_usermessage_user_ids:
ums_to_create = []
for message_id in changed_message_ids:
@ -688,7 +694,7 @@ def do_update_message(
# very expensive, since it's N guest users x M messages.
UserMessage.objects.filter(
user_profile_id__in=[sub.user_profile_id for sub in subs_losing_usermessages],
message_id__in=changed_message_ids,
message_id__in=changed_messages,
).delete()
delete_event: DeleteMessagesEvent = {
@ -703,24 +709,32 @@ def do_update_message(
# Reset the Attachment.is_*_public caches for all messages
# moved to another stream with different access permissions.
if new_stream.invite_only != stream_being_edited.invite_only:
Attachment.objects.filter(messages__in=changed_message_ids).update(
Attachment.objects.filter(messages__in=changed_messages.values("id")).update(
is_realm_public=None,
)
ArchivedAttachment.objects.filter(messages__in=changed_message_ids).update(
ArchivedAttachment.objects.filter(
messages__in=changed_messages.values("id")
).update(
is_realm_public=None,
)
if new_stream.is_web_public != stream_being_edited.is_web_public:
Attachment.objects.filter(messages__in=changed_message_ids).update(
Attachment.objects.filter(messages__in=changed_messages.values("id")).update(
is_web_public=None,
)
ArchivedAttachment.objects.filter(messages__in=changed_message_ids).update(
ArchivedAttachment.objects.filter(
messages__in=changed_messages.values("id")
).update(
is_web_public=None,
)
# This does message.save(update_fields=[...])
save_message_for_edit_use_case(message=target_message)
# This updates any later messages, if any. It returns the
# freshly-fetched-from-the-database changed messages.
changed_messages = save_changes_for_propagation_mode()
realm_id: Optional[int] = None
if stream_being_edited is not None:
realm_id = stream_being_edited.realm_id
@ -993,8 +1007,6 @@ def do_update_message(
if (new_stream is not None or topic_name is not None) and stream_being_edited is not None:
# Notify users that the topic was moved.
changed_messages_count = len(changed_messages)
old_thread_notification_string = None
if send_notification_to_old_thread:
if moved_all_visible_messages:
@ -1034,8 +1046,6 @@ def do_update_message(
new_topic = topic_name if topic_name is not None else orig_topic_name
changed_message_ids = [changed_message.id for changed_message in changed_messages]
# We calculate whether the user moved the entire topic
# using that user's own permissions, which is important to
# avoid leaking information about whether there are
@ -1077,7 +1087,7 @@ def do_update_message(
changed_messages_count,
)
return len(changed_messages)
return changed_messages_count
def check_time_limit_for_change_all_propagate_mode(

6
zerver/lib/message.py
View File

@ -499,7 +499,7 @@ class MessageDict:
@staticmethod
def to_dict_uncached(
messages: List[Message], realm_id: Optional[int] = None
messages: Collection[Message], realm_id: Optional[int] = None
) -> Dict[int, bytes]:
messages_dict = MessageDict.to_dict_uncached_helper(messages, realm_id)
encoded_messages = {msg["id"]: stringify_message_dict(msg) for msg in messages_dict}
@ -507,7 +507,7 @@ class MessageDict:
@staticmethod
def to_dict_uncached_helper(
messages: List[Message], realm_id: Optional[int] = None
messages: Collection[Message], realm_id: Optional[int] = None
) -> List[Dict[str, Any]]:
# Near duplicate of the build_message_dict + get_raw_db_rows
# code path that accepts already fetched Message objects
@ -1827,7 +1827,7 @@ def parse_message_time_limit_setting(
def update_to_dict_cache(
changed_messages: List[Message], realm_id: Optional[int] = None
changed_messages: Collection[Message], realm_id: Optional[int] = None
) -> List[int]:
"""Updates the message as stored in the to_dict cache (for serving
messages)."""

17
zerver/lib/topic.py
View File

@ -1,5 +1,5 @@
from datetime import datetime
from typing import Any, Dict, List, Optional, Set, Tuple
from typing import Any, Callable, Dict, List, Optional, Set, Tuple
import orjson
from django.db import connection
@ -154,7 +154,7 @@ def update_messages_for_topic_edit(
old_stream: Stream,
edit_history_event: EditHistoryEvent,
last_edit_time: datetime,
) -> List[Message]:
) -> Tuple[QuerySet[Message], Callable[[], QuerySet[Message]]]:
# Uses index: zerver_message_realm_recipient_upper_subject
messages = Message.objects.filter(
realm_id=old_stream.realm_id,
@ -222,12 +222,15 @@ def update_messages_for_topic_edit(
# update, and then return a fresh collection -- so we know their
# metadata has been updated for the UPDATE command, and the caller
# can update the remote cache with that.
message_ids = list(messages.values_list("id", flat=True))
messages.update(**update_fields)
message_ids = [edited_message.id, *messages.values_list("id", flat=True)]
return list(
Message.objects.filter(id__in=message_ids).select_related(*Message.DEFAULT_SELECT_RELATED)
)
def propagate() -> QuerySet[Message]:
messages.update(**update_fields)
return Message.objects.filter(id__in=message_ids).select_related(
*Message.DEFAULT_SELECT_RELATED
)
return messages, propagate
def generate_topic_history_from_db_rows(rows: List[Tuple[str, int]]) -> List[Dict[str, Any]]:

12
zerver/tests/test_message_edit.py
View File

@ -1433,7 +1433,7 @@ class EditMessageTest(EditMessageTestCase):
# state + 1/user with a UserTopic row for the events data)
# beyond what is typical were there not UserTopic records to
# update. Ideally, we'd eliminate the per-user component.
with self.assert_database_query_count(24):
with self.assert_database_query_count(26):
check_update_message(
user_profile=hamlet,
message_id=message_id,
@ -1530,7 +1530,7 @@ class EditMessageTest(EditMessageTestCase):
set_topic_visibility_policy(desdemona, muted_topics, UserTopic.VisibilityPolicy.MUTED)
set_topic_visibility_policy(cordelia, muted_topics, UserTopic.VisibilityPolicy.MUTED)
with self.assert_database_query_count(29):
with self.assert_database_query_count(30):
check_update_message(
user_profile=desdemona,
message_id=message_id,
@ -1561,7 +1561,7 @@ class EditMessageTest(EditMessageTestCase):
set_topic_visibility_policy(desdemona, muted_topics, UserTopic.VisibilityPolicy.MUTED)
set_topic_visibility_policy(cordelia, muted_topics, UserTopic.VisibilityPolicy.MUTED)
with self.assert_database_query_count(34):
with self.assert_database_query_count(35):
check_update_message(
user_profile=desdemona,
message_id=message_id,
@ -1594,7 +1594,7 @@ class EditMessageTest(EditMessageTestCase):
set_topic_visibility_policy(desdemona, muted_topics, UserTopic.VisibilityPolicy.MUTED)
set_topic_visibility_policy(cordelia, muted_topics, UserTopic.VisibilityPolicy.MUTED)
with self.assert_database_query_count(29):
with self.assert_database_query_count(30):
check_update_message(
user_profile=desdemona,
message_id=message_id,
@ -1617,7 +1617,7 @@ class EditMessageTest(EditMessageTestCase):
second_message_id = self.send_stream_message(
hamlet, stream_name, topic_name="changed topic name", content="Second message"
)
with self.assert_database_query_count(24):
with self.assert_database_query_count(25):
check_update_message(
user_profile=desdemona,
message_id=second_message_id,
@ -1716,7 +1716,7 @@ class EditMessageTest(EditMessageTestCase):
users_to_be_notified_via_muted_topics_event.append(user_topic.user_profile_id)
change_all_topic_name = "Topic 1 edited"
with self.assert_database_query_count(29):
with self.assert_database_query_count(31):
check_update_message(
user_profile=hamlet,
message_id=message_id,