From dd1fbb2aed59d1554ec3266064b4bc251c023079 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 30 Jul 2020 11:05:49 -0400 Subject: [PATCH 1/5] Stop references Deferreds in comments. --- .../server_notices/consent_server_notices.py | 12 +++--- .../resource_limits_server_notices.py | 39 +++++++++---------- .../server_notices/server_notices_manager.py | 2 +- .../server_notices/server_notices_sender.py | 8 ++-- .../worker_server_notices_sender.py | 17 +++----- 5 files changed, 34 insertions(+), 44 deletions(-) diff --git a/synapse/server_notices/consent_server_notices.py b/synapse/server_notices/consent_server_notices.py index 3bfc8d7278ec..089cfef0b3d4 100644 --- a/synapse/server_notices/consent_server_notices.py +++ b/synapse/server_notices/consent_server_notices.py @@ -13,6 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging +from typing import Any from synapse.api.errors import SynapseError from synapse.api.urls import ConsentURIBuilder @@ -55,14 +56,11 @@ def __init__(self, hs): self._consent_uri_builder = ConsentURIBuilder(hs.config) - async def maybe_send_server_notice_to_user(self, user_id): + async def maybe_send_server_notice_to_user(self, user_id: str) -> None: """Check if we need to send a notice to this user, and does so if so Args: - user_id (str): user to check - - Returns: - Deferred + user_id: user to check """ if self._server_notice_content is None: # not enabled @@ -105,7 +103,7 @@ async def maybe_send_server_notice_to_user(self, user_id): self._users_in_progress.remove(user_id) -def copy_with_str_subst(x, substitutions): +def copy_with_str_subst(x: Any, substitutions: Any) -> Any: """Deep-copy a structure, carrying out string substitions on any strings Args: @@ -121,7 +119,7 @@ def copy_with_str_subst(x, substitutions): if isinstance(x, dict): return {k: copy_with_str_subst(v, substitutions) for (k, v) in x.items()} if isinstance(x, (list, tuple)): - return [copy_with_str_subst(y) for y in x] + return [copy_with_str_subst(y, substitutions) for y in x] # assume it's uninterested and can be shallow-copied. return x diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py index 4404ceff930d..e739760e0bf8 100644 --- a/synapse/server_notices/resource_limits_server_notices.py +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -13,6 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging +from typing import List, Tuple from synapse.api.constants import ( EventTypes, @@ -52,7 +53,7 @@ def __init__(self, hs): and not hs.config.hs_disabled ) - async def maybe_send_server_notice_to_user(self, user_id): + async def maybe_send_server_notice_to_user(self, user_id: str) -> None: """Check if we need to send a notice to this user, this will be true in two cases. 1. The server has reached its limit does not reflect this @@ -60,10 +61,7 @@ async def maybe_send_server_notice_to_user(self, user_id): actually the server is fine Args: - user_id (str): user to check - - Returns: - Deferred + user_id: user to check """ if not self._enabled: return @@ -120,14 +118,16 @@ async def maybe_send_server_notice_to_user(self, user_id): except SynapseError as e: logger.error("Error sending resource limits server notice: %s", e) - async def _remove_limit_block_notification(self, user_id, ref_events): + async def _remove_limit_block_notification( + self, user_id: str, ref_events: List[str] + ) -> None: """Utility method to remove limit block notifications from the server notices room. Args: - user_id (str): user to notify - ref_events (list[str]): The event_ids of pinned events that are unrelated to - limit blocking and need to be preserved. + user_id: user to notify + ref_events: The event_ids of pinned events that are unrelated to + limit blocking and need to be preserved. """ content = {"pinned": ref_events} await self._server_notices_manager.send_notice( @@ -135,16 +135,16 @@ async def _remove_limit_block_notification(self, user_id, ref_events): ) async def _apply_limit_block_notification( - self, user_id, event_body, event_limit_type - ): + self, user_id: str, event_body: str, event_limit_type: str + ) -> None: """Utility method to apply limit block notifications in the server notices room. Args: - user_id (str): user to notify - event_body(str): The human readable text that describes the block. - event_limit_type(str): Specifies the type of block e.g. monthly active user - limit has been exceeded. + user_id: user to notify + event_body: The human readable text that describes the block. + event_limit_type: Specifies the type of block e.g. monthly active user + limit has been exceeded. """ content = { "body": event_body, @@ -162,7 +162,7 @@ async def _apply_limit_block_notification( user_id, content, EventTypes.Pinned, "" ) - async def _check_and_set_tags(self, user_id, room_id): + async def _check_and_set_tags(self, user_id: str, room_id: str) -> None: """ Since server notices rooms were originally not with tags, important to check that tags have been set correctly @@ -182,17 +182,16 @@ async def _check_and_set_tags(self, user_id, room_id): ) self._notifier.on_new_event("account_data_key", max_id, users=[user_id]) - async def _is_room_currently_blocked(self, room_id): + async def _is_room_currently_blocked(self, room_id: str) -> Tuple[bool, List[str]]: """ Determines if the room is currently blocked Args: - room_id(str): The room id of the server notices room + room_id: The room id of the server notices room Returns: - Deferred[Tuple[bool, List]]: bool: Is the room currently blocked - list: The list of pinned events that are unrelated to limit blocking + list: The list of pinned event IDs that are unrelated to limit blocking This list can be used as a convenience in the case where the block is to be lifted and the remaining pinned event references need to be preserved diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py index bf2454c01cd8..0099bfb81c8a 100644 --- a/synapse/server_notices/server_notices_manager.py +++ b/synapse/server_notices/server_notices_manager.py @@ -63,7 +63,7 @@ async def send_notice( is_state_event(bool): Is the event a state event Returns: - Deferred[FrozenEvent] + FrozenEvent """ room_id = await self.get_or_create_notice_room_for_user(user_id) await self.maybe_invite_user_to_room(user_id, room_id) diff --git a/synapse/server_notices/server_notices_sender.py b/synapse/server_notices/server_notices_sender.py index be74e866415f..63c80fabc408 100644 --- a/synapse/server_notices/server_notices_sender.py +++ b/synapse/server_notices/server_notices_sender.py @@ -34,20 +34,20 @@ def __init__(self, hs): ResourceLimitsServerNotices(hs), ) - async def on_user_syncing(self, user_id): + async def on_user_syncing(self, user_id: str) -> None: """Called when the user performs a sync operation. Args: - user_id (str): mxid of user who synced + user_id: mxid of user who synced """ for sn in self._server_notices: await sn.maybe_send_server_notice_to_user(user_id) - async def on_user_ip(self, user_id): + async def on_user_ip(self, user_id: str) -> None: """Called on the master when a worker process saw a client request. Args: - user_id (str): mxid + user_id: mxid """ # The synchrotrons use a stubbed version of ServerNoticesSender, so # we check for notices to send to the user in on_user_ip as well as diff --git a/synapse/server_notices/worker_server_notices_sender.py b/synapse/server_notices/worker_server_notices_sender.py index 245ec7c64ff4..e9390b19da86 100644 --- a/synapse/server_notices/worker_server_notices_sender.py +++ b/synapse/server_notices/worker_server_notices_sender.py @@ -12,7 +12,6 @@ # 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 twisted.internet import defer class WorkerServerNoticesSender(object): @@ -24,24 +23,18 @@ def __init__(self, hs): hs (synapse.server.HomeServer): """ - def on_user_syncing(self, user_id): + async def on_user_syncing(self, user_id: str) -> None: """Called when the user performs a sync operation. Args: - user_id (str): mxid of user who synced - - Returns: - Deferred + user_id: mxid of user who synced """ - return defer.succeed(None) + return None - def on_user_ip(self, user_id): + async def on_user_ip(self, user_id: str) -> None: """Called on the master when a worker process saw a client request. Args: - user_id (str): mxid - - Returns: - Deferred + user_id: mxid """ raise AssertionError("on_user_ip unexpectedly called on worker") From 4a3f15deaca4259915af4413dfb2acf91fdc9a98 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 30 Jul 2020 11:15:17 -0400 Subject: [PATCH 2/5] Add to tox and fix a few more issues. --- synapse/server_notices/resource_limits_server_notices.py | 6 +++--- synapse/server_notices/server_notices_sender.py | 4 +++- tox.ini | 1 + 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py index e739760e0bf8..b31c3a2029ec 100644 --- a/synapse/server_notices/resource_limits_server_notices.py +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -13,7 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging -from typing import List, Tuple +from typing import List, Optional, Tuple from synapse.api.constants import ( EventTypes, @@ -135,7 +135,7 @@ async def _remove_limit_block_notification( ) async def _apply_limit_block_notification( - self, user_id: str, event_body: str, event_limit_type: str + self, user_id: str, event_body: str, event_limit_type: Optional[str] ) -> None: """Utility method to apply limit block notifications in the server notices room. @@ -206,7 +206,7 @@ async def _is_room_currently_blocked(self, room_id: str) -> Tuple[bool, List[str # The user has yet to join the server notices room pass - referenced_events = [] + referenced_events = [] # type: List[str] if pinned_state_event is not None: referenced_events = list(pinned_state_event.content.get("pinned", [])) diff --git a/synapse/server_notices/server_notices_sender.py b/synapse/server_notices/server_notices_sender.py index 63c80fabc408..a754f75db4f9 100644 --- a/synapse/server_notices/server_notices_sender.py +++ b/synapse/server_notices/server_notices_sender.py @@ -12,6 +12,8 @@ # 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 Iterable, Union + from synapse.server_notices.consent_server_notices import ConsentServerNotices from synapse.server_notices.resource_limits_server_notices import ( ResourceLimitsServerNotices, @@ -32,7 +34,7 @@ def __init__(self, hs): self._server_notices = ( ConsentServerNotices(hs), ResourceLimitsServerNotices(hs), - ) + ) # type: Iterable[Union[ConsentServerNotices, ResourceLimitsServerNotices]] async def on_user_syncing(self, user_id: str) -> None: """Called when the user performs a sync operation. diff --git a/tox.ini b/tox.ini index a394f6eadcd6..2b1db0f7f780 100644 --- a/tox.ini +++ b/tox.ini @@ -202,6 +202,7 @@ commands = mypy \ synapse/push/push_rule_evaluator.py \ synapse/replication \ synapse/rest \ + synapse/server_notices \ synapse/spam_checker_api \ synapse/storage/data_stores/main/ui_auth.py \ synapse/storage/database.py \ From a9d97d81b4dc262e0669b72b8c5fb7b706b6ad35 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 30 Jul 2020 11:16:49 -0400 Subject: [PATCH 3/5] Add changelog. --- changelog.d/7996.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/7996.bugfix diff --git a/changelog.d/7996.bugfix b/changelog.d/7996.bugfix new file mode 100644 index 000000000000..1e51f2055829 --- /dev/null +++ b/changelog.d/7996.bugfix @@ -0,0 +1 @@ +Fix various comments and minor discrepencies in server notices code. From 699eb3740ee88025a5ff9ff8cbda8b07748e8a6b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 31 Jul 2020 07:17:38 -0400 Subject: [PATCH 4/5] Add more typing info. --- .../server_notices/server_notices_manager.py | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py index 0099bfb81c8a..ed96aa857110 100644 --- a/synapse/server_notices/server_notices_manager.py +++ b/synapse/server_notices/server_notices_manager.py @@ -13,8 +13,10 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging +from typing import Optional from synapse.api.constants import EventTypes, Membership, RoomCreationPreset +from synapse.events import EventBase from synapse.types import UserID, create_requester from synapse.util.caches.descriptors import cached @@ -50,20 +52,21 @@ def is_enabled(self): return self._config.server_notices_mxid is not None async def send_notice( - self, user_id, event_content, type=EventTypes.Message, state_key=None - ): + self, + user_id: str, + event_content: dict, + type: str = EventTypes.Message, + state_key: Optional[bool] = None, + ) -> EventBase: """Send a notice to the given user Creates the server notices room, if none exists. Args: - user_id (str): mxid of user to send event to. - event_content (dict): content of event to send - type(EventTypes): type of event - is_state_event(bool): Is the event a state event - - Returns: - FrozenEvent + user_id: mxid of user to send event to. + event_content: content of event to send + type: type of event + is_state_event: Is the event a state event """ room_id = await self.get_or_create_notice_room_for_user(user_id) await self.maybe_invite_user_to_room(user_id, room_id) @@ -89,17 +92,17 @@ async def send_notice( return event @cached() - async def get_or_create_notice_room_for_user(self, user_id): + async def get_or_create_notice_room_for_user(self, user_id: str) -> str: """Get the room for notices for a given user If we have not yet created a notice room for this user, create it, but don't invite the user to it. Args: - user_id (str): complete user id for the user we want a room for + user_id: complete user id for the user we want a room for Returns: - str: room id of notice room. + room id of notice room. """ if not self.is_enabled(): raise Exception("Server notices not enabled") @@ -163,7 +166,7 @@ async def get_or_create_notice_room_for_user(self, user_id): logger.info("Created server notices room %s for %s", room_id, user_id) return room_id - async def maybe_invite_user_to_room(self, user_id: str, room_id: str): + async def maybe_invite_user_to_room(self, user_id: str, room_id: str) -> None: """Invite the given user to the given server room, unless the user has already joined or been invited to it. From 052a474a54d2797e23ffe758ae6d547bf2ae5d68 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 31 Jul 2020 15:40:12 -0400 Subject: [PATCH 5/5] Ignore mypy on a line. --- synapse/server_notices/resource_limits_server_notices.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py index b31c3a2029ec..c2faef6eabe5 100644 --- a/synapse/server_notices/resource_limits_server_notices.py +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -13,7 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging -from typing import List, Optional, Tuple +from typing import List, Tuple from synapse.api.constants import ( EventTypes, @@ -113,7 +113,7 @@ async def maybe_send_server_notice_to_user(self, user_id: str) -> None: elif not currently_blocked and limit_msg: # Room is not notifying of a block, when it ought to be. await self._apply_limit_block_notification( - user_id, limit_msg, limit_type + user_id, limit_msg, limit_type # type: ignore ) except SynapseError as e: logger.error("Error sending resource limits server notice: %s", e) @@ -135,7 +135,7 @@ async def _remove_limit_block_notification( ) async def _apply_limit_block_notification( - self, user_id: str, event_body: str, event_limit_type: Optional[str] + self, user_id: str, event_body: str, event_limit_type: str ) -> None: """Utility method to apply limit block notifications in the server notices room.