Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Improved push typing #11409

Merged
merged 11 commits into from
Nov 30, 2021
Merged

Improved push typing #11409

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
56b921e
improve type checking for push
Bubu Nov 22, 2021
e9991bf
add changelog
Bubu Nov 22, 2021
fb1fbc1
isort
Bubu Nov 22, 2021
08f12e4
lint
Bubu Nov 22, 2021
d404a4c
use TypedDict from typing_extensions
Bubu Nov 22, 2021
073bdb8
isort again
Bubu Nov 22, 2021
ec27056
properly type fields pulled from event json
Bubu Nov 23, 2021
6f8c5d5
Update synapse/push/httppusher.py
Bubu Nov 23, 2021
c8387bd
add docstrings for EmailReason
Bubu Nov 23, 2021
388803d
document all the push types
Bubu Nov 23, 2021
2904f49
lint
Bubu Nov 23, 2021
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
1 change: 1 addition & 0 deletions changelog.d/11409.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Improve internal types in push code.
10 changes: 7 additions & 3 deletions synapse/push/emailpusher.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
from synapse.metrics.background_process_metrics import run_as_background_process
from synapse.push import Pusher, PusherConfig, PusherConfigException, ThrottleParams
from synapse.push.mailer import Mailer
from synapse.push.push_types import EmailReason
from synapse.storage.databases.main.event_push_actions import EmailPushAction
from synapse.util.threepids import validate_email

if TYPE_CHECKING:
Expand Down Expand Up @@ -190,7 +192,7 @@ async def _unsafe_process(self) -> None:
# we then consider all previously outstanding notifications
# to be delivered.

reason = {
reason: EmailReason = {
"room_id": push_action["room_id"],
"now": self.clock.time_msec(),
"received_at": received_at,
Expand Down Expand Up @@ -275,7 +277,7 @@ def room_ready_to_notify_at(self, room_id: str) -> int:
return may_send_at

async def sent_notif_update_throttle(
self, room_id: str, notified_push_action: dict
self, room_id: str, notified_push_action: EmailPushAction
) -> None:
# We have sent a notification, so update the throttle accordingly.
# If the event that triggered the notif happened more than
Expand Down Expand Up @@ -315,7 +317,9 @@ async def sent_notif_update_throttle(
self.pusher_id, room_id, self.throttle_params[room_id]
)

async def send_notification(self, push_actions: List[dict], reason: dict) -> None:
async def send_notification(
self, push_actions: List[EmailPushAction], reason: EmailReason
) -> None:
logger.info("Sending notif email for user %r", self.user_id)

await self.mailer.send_notification_mail(
Expand Down
3 changes: 2 additions & 1 deletion synapse/push/httppusher.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
from synapse.metrics.background_process_metrics import run_as_background_process
from synapse.push import Pusher, PusherConfig, PusherConfigException

from ..storage.databases.main.event_push_actions import HttpPushAction
Bubu marked this conversation as resolved.
Show resolved Hide resolved
from . import push_rule_evaluator, push_tools

if TYPE_CHECKING:
Expand Down Expand Up @@ -273,7 +274,7 @@ async def _unsafe_process(self) -> None:
)
break

async def _process_one(self, push_action: dict) -> bool:
async def _process_one(self, push_action: HttpPushAction) -> bool:
if "notify" not in push_action["actions"]:
return True

Expand Down
66 changes: 38 additions & 28 deletions synapse/push/mailer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

import logging
import urllib.parse
from typing import TYPE_CHECKING, Any, Dict, Iterable, List, Optional, TypeVar
from typing import TYPE_CHECKING, Dict, Iterable, List, Optional, TypeVar

import bleach
import jinja2
Expand All @@ -28,6 +28,14 @@
descriptor_from_member_events,
name_from_member_event,
)
from synapse.push.push_types import (
EmailReason,
MessageVars,
NotifVars,
RoomVars,
TemplateVars,
)
from synapse.storage.databases.main.event_push_actions import EmailPushAction
from synapse.storage.state import StateFilter
from synapse.types import StateMap, UserID
from synapse.util.async_helpers import concurrently_execute
Expand Down Expand Up @@ -135,7 +143,7 @@ async def send_password_reset_mail(
% urllib.parse.urlencode(params)
)

template_vars = {"link": link}
template_vars: TemplateVars = {"link": link}

await self.send_email(
email_address,
Expand Down Expand Up @@ -165,7 +173,7 @@ async def send_registration_mail(
% urllib.parse.urlencode(params)
)

template_vars = {"link": link}
template_vars: TemplateVars = {"link": link}

await self.send_email(
email_address,
Expand Down Expand Up @@ -196,7 +204,7 @@ async def send_add_threepid_mail(
% urllib.parse.urlencode(params)
)

template_vars = {"link": link}
template_vars: TemplateVars = {"link": link}

await self.send_email(
email_address,
Expand All @@ -210,8 +218,8 @@ async def send_notification_mail(
app_id: str,
user_id: str,
email_address: str,
push_actions: Iterable[Dict[str, Any]],
reason: Dict[str, Any],
push_actions: Iterable[EmailPushAction],
reason: EmailReason,
) -> None:
"""
Send email regarding a user's room notifications
Expand All @@ -230,7 +238,7 @@ async def send_notification_mail(
[pa["event_id"] for pa in push_actions]
)

notifs_by_room: Dict[str, List[Dict[str, Any]]] = {}
notifs_by_room: Dict[str, List[EmailPushAction]] = {}
for pa in push_actions:
notifs_by_room.setdefault(pa["room_id"], []).append(pa)

Expand Down Expand Up @@ -258,7 +266,7 @@ async def _fetch_room_state(room_id: str) -> None:
# actually sort our so-called rooms_in_order list, most recent room first
rooms_in_order.sort(key=lambda r: -(notifs_by_room[r][-1]["received_ts"] or 0))

rooms: List[Dict[str, Any]] = []
rooms: List[RoomVars] = []

for r in rooms_in_order:
roomvars = await self._get_room_vars(
Expand Down Expand Up @@ -289,7 +297,7 @@ async def _fetch_room_state(room_id: str) -> None:
notifs_by_room, state_by_room, notif_events, reason
)

template_vars = {
template_vars: TemplateVars = {
"user_display_name": user_display_name,
"unsubscribe_link": self._make_unsubscribe_link(
user_id, app_id, email_address
Expand All @@ -302,10 +310,10 @@ async def _fetch_room_state(room_id: str) -> None:
await self.send_email(email_address, summary_text, template_vars)

async def send_email(
self, email_address: str, subject: str, extra_template_vars: Dict[str, Any]
self, email_address: str, subject: str, extra_template_vars: TemplateVars
) -> None:
"""Send an email with the given information and template text"""
template_vars = {
template_vars: TemplateVars = {
"app_name": self.app_name,
"server_name": self.hs.config.server.server_name,
}
Expand All @@ -327,10 +335,10 @@ async def _get_room_vars(
self,
room_id: str,
user_id: str,
notifs: Iterable[Dict[str, Any]],
notifs: Iterable[EmailPushAction],
notif_events: Dict[str, EventBase],
room_state_ids: StateMap[str],
) -> Dict[str, Any]:
) -> RoomVars:
"""
Generate the variables for notifications on a per-room basis.

Expand All @@ -356,7 +364,7 @@ async def _get_room_vars(

room_name = await calculate_room_name(self.store, room_state_ids, user_id)

room_vars: Dict[str, Any] = {
room_vars: RoomVars = {
"title": room_name,
"hash": string_ordinal_total(room_id), # See sender avatar hash
"notifs": [],
Expand Down Expand Up @@ -417,11 +425,11 @@ async def _get_room_avatar(

async def _get_notif_vars(
self,
notif: Dict[str, Any],
notif: EmailPushAction,
user_id: str,
notif_event: EventBase,
room_state_ids: StateMap[str],
) -> Dict[str, Any]:
) -> NotifVars:
"""
Generate the variables for a single notification.

Expand All @@ -442,7 +450,7 @@ async def _get_notif_vars(
after_limit=CONTEXT_AFTER,
)

ret = {
ret: NotifVars = {
"link": self._make_notif_link(notif),
"ts": notif["received_ts"],
"messages": [],
Expand All @@ -461,8 +469,8 @@ async def _get_notif_vars(
return ret

async def _get_message_vars(
self, notif: Dict[str, Any], event: EventBase, room_state_ids: StateMap[str]
) -> Optional[Dict[str, Any]]:
self, notif: EmailPushAction, event: EventBase, room_state_ids: StateMap[str]
) -> Optional[MessageVars]:
"""
Generate the variables for a single event, if possible.

Expand Down Expand Up @@ -494,7 +502,9 @@ async def _get_message_vars(

if sender_state_event:
sender_name = name_from_member_event(sender_state_event)
sender_avatar_url = sender_state_event.content.get("avatar_url")
sender_avatar_url: Optional[str] = sender_state_event.content.get(
"avatar_url"
)
else:
# No state could be found, fallback to the MXID.
sender_name = event.sender
Expand All @@ -504,7 +514,7 @@ async def _get_message_vars(
# sender_hash % the number of default images to choose from
sender_hash = string_ordinal_total(event.sender)

ret = {
ret: MessageVars = {
"event_type": event.type,
"is_historical": event.event_id != notif["event_id"],
"id": event.event_id,
Expand Down Expand Up @@ -533,7 +543,7 @@ async def _get_message_vars(
return ret

def _add_text_message_vars(
self, messagevars: Dict[str, Any], event: EventBase
self, messagevars: MessageVars, event: EventBase
) -> None:
"""
Potentially add a sanitised message body to the message variables.
Expand All @@ -555,7 +565,7 @@ def _add_text_message_vars(
messagevars["body_text_html"] = safe_text(body)

def _add_image_message_vars(
self, messagevars: Dict[str, Any], event: EventBase
self, messagevars: MessageVars, event: EventBase
) -> None:
"""
Potentially add an image URL to the message variables.
Expand All @@ -570,7 +580,7 @@ def _add_image_message_vars(
async def _make_summary_text_single_room(
self,
room_id: str,
notifs: List[Dict[str, Any]],
notifs: List[EmailPushAction],
room_state_ids: StateMap[str],
notif_events: Dict[str, EventBase],
user_id: str,
Expand Down Expand Up @@ -685,10 +695,10 @@ async def _make_summary_text_single_room(

async def _make_summary_text(
self,
notifs_by_room: Dict[str, List[Dict[str, Any]]],
notifs_by_room: Dict[str, List[EmailPushAction]],
room_state_ids: Dict[str, StateMap[str]],
notif_events: Dict[str, EventBase],
reason: Dict[str, Any],
reason: EmailReason,
) -> str:
"""
Make a summary text for the email when multiple rooms have notifications.
Expand Down Expand Up @@ -718,7 +728,7 @@ async def _make_summary_text(
async def _make_summary_text_from_member_events(
self,
room_id: str,
notifs: List[Dict[str, Any]],
notifs: List[EmailPushAction],
room_state_ids: StateMap[str],
notif_events: Dict[str, EventBase],
) -> str:
Expand Down Expand Up @@ -805,7 +815,7 @@ def _make_room_link(self, room_id: str) -> str:
base_url = "https://matrix.to/#"
return "%s/%s" % (base_url, room_id)

def _make_notif_link(self, notif: Dict[str, str]) -> str:
def _make_notif_link(self, notif: EmailPushAction) -> str:
"""
Generate a link to open an event in the web client.

Expand Down
67 changes: 67 additions & 0 deletions synapse/push/push_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
# Copyright 2021 The Matrix.org Foundation C.I.C.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
from typing import Any, List, Optional

from typing_extensions import TypedDict


class EmailReason(TypedDict, total=False):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The total=False isn't great in my opinion, but it's currently necessary as many code path iteratively construct these dicts. Maybe reworking that could be done as a next step?

Copy link
Member

Choose a reason for hiding this comment

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

Can we keep them as normal dicts while we're building them, and then cast? Or just use local variables and then build it at the end? e.g.:

let room_id = ...
...
let room_name = ...
....
reason: EmailReason  = { ... }

Not sure if that works

Copy link
Contributor

Choose a reason for hiding this comment

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

That would require changing how these functions iteratively build the struct, while i think that's a good idea in general, i don't know how possible that is, certainly as i remember some of them building these up across multiple functions, passing intermediate results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I also believe that's the case. I'll take another look and see if I can resolve at least some easy cases here.

Copy link
Contributor Author

@Bubu Bubu Nov 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I looked again and with the current code structure doing anything like this doesn't really make sense as for the reasons @ShadowJonathan mentioned.

The thing we could do is creating None value fields instead of structures with missing fields (so adding a bunch of Optionals instead of total=False). I currently have no opinion if this is better or worse.

Anything else would require more restructuring of the code, which I think is better done seperately?

Edit: another idea would be segmenting these types into more granular ones like "EmailReason" and EmailReasonWithRoomName. I'm not convinced by this either. really.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for looking into it. I think restructuring this may be the right answer eventually but you're right that it's not for here and now.

room_id: str
room_name: Optional[str]
now: int
received_at: int
delay_before_mail_ms: int
last_sent_ts: int
throttle_ms: int


class NotifVars(TypedDict):
Bubu marked this conversation as resolved.
Show resolved Hide resolved
link: str
ts: Optional[int]
messages: List


class RoomVars(TypedDict):
title: Optional[str]
hash: int
notifs: List
invite: bool
link: str
avatar_url: Optional[str]


class MessageVars(TypedDict, total=False):
event_type: str
is_historical: bool
id: str
ts: int
sender_name: str
sender_avatar_url: Optional[str]
sender_hash: int
msgtype: Optional[Any]
Bubu marked this conversation as resolved.
Show resolved Hide resolved
body_text_html: str
body_text_plain: str
image_url: str
format: Optional[Any]


class TemplateVars(TypedDict, total=False):
app_name: str
server_name: str
link: str
user_display_name: str
unsubscribe_link: str
summary_text: str
rooms: List
reason: EmailReason
Loading