Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

api: Add dataclass for update_message events. #18411

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

api: Add dataclass for update_message events. #18411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
api: Add dataclass for update_message events. 
Different variants of update_message events made it difficult
to keep api changes uniform across them. Created a dataclass for
update_message variant events and refactored to replace Dict with
TypedDict. Fixes #16022.
  • Loading branch information
@MSurfer20
MSurfer20 committed May 13, 2021
commit d7cddad39f3c37250fdf8bb491ef6d109232cc7c
1 change: 1 addition & 0 deletions tools/linter_lib/custom_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
# This has lots of query data embedded, so it's hard
# to fix everything until we migrate the DB to "topic".
"zerver/tests/test_message_fetch.py",
"zerver/lib/actions.py",
}

shebang_rules: List["Rule"] = [
Expand Down
48 changes: 42 additions & 6 deletions zerver/lib/actions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5622,7 +5622,7 @@ def do_update_embedded_data(
content: Optional[str],
rendered_content: Optional[str],
) -> None:
event: Dict[str, Any] = {"type": "update_message", "message_id": message.id}
event: UpdateMessagesEvent = {"type": "update_message", "message_id": message.id}
changed_messages = [message]

ums = UserMessage.objects.filter(message=message.id)
Expand Down Expand Up @@ -5658,6 +5658,38 @@ class DeleteMessagesEvent(TypedDict, total=False):
stream_id: int


class UpdateMessagesEvent(TypedDict, total=False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why total=False here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is something I used as a temporary solution and need to fix(hence the current WIP status). Removing it causes error since message_ids, which is a compulary field is calculated much later in the code and the event TypedDict is created much before.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timabbott Something that I now remembered -- the total=False is necessary because we need to omit the Optional parameters completely rather than including them with None, since the API expects those keys to be absent rather than present with value None, and gives error in such cases.
A downside of doing this is that we won't be able to check for missing parameters in the event.(I followed the example of DeleteMessagesEvent to use total=False)

type: str
user_id: int
message_id: int
message_ids: List[int]

edit_timestamp: Optional[int]
propagate_mode: Optional[str]
orig_content: Optional[str]
orig_rendered_content: Optional[str]
prev_rendered_content_version: Optional[int]
content: Optional[str]
flags: Optional[List[str]]
is_me_message: Optional[bool]
rendered_content: Optional[str]

push_notify_user_ids: Optional[List[int]]
stream_push_user_ids: Optional[List[int]]
stream_email_user_ids: Optional[List[int]]
prior_mention_user_ids: Optional[List[int]]
mention_user_ids: Optional[List[int]]
presence_idle_user_ids: Optional[List[int]]
wildcard_mention_user_ids: Optional[List[int]]

stream_name: Optional[str]
stream_id: Optional[int]
new_stream_id: Optional[int]
orig_subject: Optional[str]
subject: Optional[str]
topic_links: Optional[List[Dict[str, str]]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kinda giant; I wonder if there's a good way to define simpler classes; e.g. have the common elements in a base class, and then subclasses for "has topic edit"? Not sure; in any case, it's an improvement to just have a defined type even if it is giant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, ideally we'd use the class in zerver/tornado_event_queue.py if possible.

Copy link
Member Author

@MSurfer20 MSurfer20 May 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kinda giant; I wonder if there's a good way to define simpler classes; e.g. have the common elements in a base class, and then subclasses for "has topic edit"? Not sure; in any case, it's an improvement to just have a defined type even if it is giant.

I initially tried to do it this way -- but there are total 4 branching ifs - if there is content, if it is a stream message, if the topic is changed or stream is moved, if the topic is changed - giving rise to 16 possible classes. That might make the class structure, as well as the code to use the appropriate class for the TypedDict a little cluttered in my opinion.
Also, in TypedDict, it is not possible to change type of an inherited parameter in the child class, so an approach where we have Optional elements made non-optional in child classes won't work out as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timabbott Would this large number of different classes be desirable? Since this is the only way I think we could remove the total=False if that is desired.



# We use transaction.atomic to support select_for_update in the attachment codepath.
@transaction.atomic
def do_update_message(
Expand Down Expand Up @@ -5688,7 +5720,7 @@ def do_update_message(
timestamp = timezone_now()
target_message.last_edit_time = timestamp

event: Dict[str, Any] = {
event: UpdateMessagesEvent = {
"type": "update_message",
"user_id": user_profile.id,
"edit_timestamp": datetime_to_timestamp(timestamp),
Expand Down Expand Up @@ -5799,7 +5831,8 @@ def do_update_message(
assert stream_being_edited is not None

edit_history_event["prev_stream"] = stream_being_edited.id
event[ORIG_TOPIC] = orig_topic_name
assert ORIG_TOPIC == "orig_subject"
event["orig_subject"] = orig_topic_name
target_message.recipient_id = new_stream.recipient_id

event["new_stream_id"] = new_stream.id
Expand Down Expand Up @@ -5854,9 +5887,12 @@ def do_update_message(
target_message.set_topic_name(topic_name)

# These fields have legacy field names.
event[ORIG_TOPIC] = orig_topic_name
event[TOPIC_NAME] = topic_name
event[TOPIC_LINKS] = topic_links(target_message.sender.realm_id, topic_name)
assert ORIG_TOPIC == "orig_subject"
assert TOPIC_NAME == "subject"
assert TOPIC_LINKS == "topic_links"
event["orig_subject"] = orig_topic_name
event["subject"] = topic_name
event["topic_links"] = topic_links(target_message.sender.realm_id, topic_name)
edit_history_event[LEGACY_PREV_TOPIC] = orig_topic_name

update_edit_history(target_message, timestamp, edit_history_event)
Expand Down