From d197164fc023b0ec928e7e77513207e17f411d4a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 12 Aug 2022 19:02:58 +0100 Subject: [PATCH 1/8] Make types immutable --- synapse/push/bulk_push_rule_evaluator.py | 16 +++++++++-- synapse/push/push_rule_evaluator.py | 27 +++++++++++++------ .../databases/main/event_push_actions.py | 22 +++++++++++---- synapse/storage/databases/main/push_rule.py | 18 ++++++++++--- 4 files changed, 65 insertions(+), 18 deletions(-) diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index 713dcf69504b..be4405041cc0 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -15,7 +15,19 @@ import itertools import logging -from typing import TYPE_CHECKING, Any, Dict, Iterable, List, Optional, Set, Tuple, Union +from typing import ( + TYPE_CHECKING, + Any, + Collection, + Dict, + Iterable, + List, + Mapping, + Optional, + Set, + Tuple, + Union, +) from prometheus_client import Counter @@ -254,7 +266,7 @@ async def action_for_event_by_user( count_as_unread = _should_count_as_unread(event, context) rules_by_user = await self._get_rules_for_event(event) - actions_by_user: Dict[str, List[Union[dict, str]]] = {} + actions_by_user: Dict[str, Collection[Union[Mapping, str]]] = {} room_member_count = await self.store.get_number_joined_users_in_room( event.room_id diff --git a/synapse/push/push_rule_evaluator.py b/synapse/push/push_rule_evaluator.py index 2e8a017add34..3c5632cd9153 100644 --- a/synapse/push/push_rule_evaluator.py +++ b/synapse/push/push_rule_evaluator.py @@ -15,7 +15,18 @@ import logging import re -from typing import Any, Dict, List, Mapping, Optional, Pattern, Set, Tuple, Union +from typing import ( + Any, + Dict, + List, + Mapping, + Optional, + Pattern, + Sequence, + Set, + Tuple, + Union, +) from matrix_common.regex import glob_to_regex, to_word_pattern @@ -32,14 +43,14 @@ def _room_member_count( - ev: EventBase, condition: Dict[str, Any], room_member_count: int + ev: EventBase, condition: Mapping[str, Any], room_member_count: int ) -> bool: return _test_ineq_condition(condition, room_member_count) def _sender_notification_permission( ev: EventBase, - condition: Dict[str, Any], + condition: Mapping[str, Any], sender_power_level: int, power_levels: Dict[str, Union[int, Dict[str, int]]], ) -> bool: @@ -54,7 +65,7 @@ def _sender_notification_permission( return sender_power_level >= room_notif_level -def _test_ineq_condition(condition: Dict[str, Any], number: int) -> bool: +def _test_ineq_condition(condition: Mapping[str, Any], number: int) -> bool: if "is" not in condition: return False m = INEQUALITY_EXPR.match(condition["is"]) @@ -137,7 +148,7 @@ def __init__( self._condition_cache: Dict[str, bool] = {} def check_conditions( - self, conditions: List[dict], uid: str, display_name: Optional[str] + self, conditions: Sequence[Mapping], uid: str, display_name: Optional[str] ) -> bool: """ Returns true if a user's conditions/user ID/display name match the event. @@ -169,7 +180,7 @@ def check_conditions( return True def matches( - self, condition: Dict[str, Any], user_id: str, display_name: Optional[str] + self, condition: Mapping[str, Any], user_id: str, display_name: Optional[str] ) -> bool: """ Returns true if a user's condition/user ID/display name match the event. @@ -204,7 +215,7 @@ def matches( # endpoint with an unknown kind, see _rule_tuple_from_request_object. return True - def _event_match(self, condition: dict, user_id: str) -> bool: + def _event_match(self, condition: Mapping, user_id: str) -> bool: """ Check an "event_match" push rule condition. @@ -269,7 +280,7 @@ def _contains_display_name(self, display_name: Optional[str]) -> bool: return bool(r.search(body)) - def _relation_match(self, condition: dict, user_id: str) -> bool: + def _relation_match(self, condition: Mapping, user_id: str) -> bool: """ Check an "relation_match" push rule condition. diff --git a/synapse/storage/databases/main/event_push_actions.py b/synapse/storage/databases/main/event_push_actions.py index 161aad0f89bd..f319e0ba3a29 100644 --- a/synapse/storage/databases/main/event_push_actions.py +++ b/synapse/storage/databases/main/event_push_actions.py @@ -74,7 +74,17 @@ """ import logging -from typing import TYPE_CHECKING, Dict, List, Optional, Tuple, Union, cast +from typing import ( + TYPE_CHECKING, + Collection, + Dict, + List, + Mapping, + Optional, + Tuple, + Union, + cast, +) import attr @@ -154,7 +164,9 @@ class NotifCounts: highlight_count: int = 0 -def _serialize_action(actions: List[Union[dict, str]], is_highlight: bool) -> str: +def _serialize_action( + actions: Collection[Union[Mapping, str]], is_highlight: bool +) -> str: """Custom serializer for actions. This allows us to "compress" common actions. We use the fact that most users have the same actions for notifs (and for @@ -733,7 +745,7 @@ def _get_if_maybe_push_in_range_for_user_txn(txn: LoggingTransaction) -> bool: async def add_push_actions_to_staging( self, event_id: str, - user_id_actions: Dict[str, List[Union[dict, str]]], + user_id_actions: Dict[str, Collection[Union[Mapping, str]]], count_as_unread: bool, ) -> None: """Add the push actions for the event to the push action staging area. @@ -750,7 +762,7 @@ async def add_push_actions_to_staging( # This is a helper function for generating the necessary tuple that # can be used to insert into the `event_push_actions_staging` table. def _gen_entry( - user_id: str, actions: List[Union[dict, str]] + user_id: str, actions: Collection[Union[Mapping, str]] ) -> Tuple[str, str, str, int, int, int]: is_highlight = 1 if _action_has_highlight(actions) else 0 notif = 1 if "notify" in actions else 0 @@ -1397,7 +1409,7 @@ def f( ] -def _action_has_highlight(actions: List[Union[dict, str]]) -> bool: +def _action_has_highlight(actions: Collection[Union[Mapping, str]]) -> bool: for action in actions: if not isinstance(action, dict): continue diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 768f95d16cf0..5f7348c4f2fe 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -14,7 +14,19 @@ # limitations under the License. import abc import logging -from typing import TYPE_CHECKING, Collection, Dict, List, Optional, Tuple, Union, cast +from typing import ( + TYPE_CHECKING, + Any, + Collection, + Dict, + List, + Mapping, + Optional, + Sequence, + Tuple, + Union, + cast, +) from synapse.api.errors import StoreError from synapse.config.homeserver import ExperimentalConfig @@ -345,8 +357,8 @@ async def add_push_rule( user_id: str, rule_id: str, priority_class: int, - conditions: List[Dict[str, str]], - actions: List[Union[JsonDict, str]], + conditions: Sequence[Mapping[str, str]], + actions: Sequence[Union[Mapping[str, Any], str]], before: Optional[str] = None, after: Optional[str] = None, ) -> None: From 236c9a0df99f3a2253531e83c3f99372378bfe97 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 12 Aug 2022 19:04:50 +0100 Subject: [PATCH 2/8] Make PushRule and co. structures --- synapse/push/baserules.py | 837 +++++++++++--------- synapse/push/bulk_push_rule_evaluator.py | 23 +- synapse/push/clientformat.py | 63 +- synapse/storage/databases/main/push_rule.py | 102 +-- tests/handlers/test_deactivate_account.py | 33 +- 5 files changed, 576 insertions(+), 482 deletions(-) diff --git a/synapse/push/baserules.py b/synapse/push/baserules.py index 6c0cc5a6ce82..d4cba802a18e 100644 --- a/synapse/push/baserules.py +++ b/synapse/push/baserules.py @@ -14,433 +14,544 @@ # See the License for the specific language governing permissions and # limitations under the License. -import copy -from typing import Any, Dict, List +import itertools +from typing import Dict, Iterator, List, Mapping, Sequence, Tuple, Union -from synapse.push.rulekinds import PRIORITY_CLASS_INVERSE_MAP, PRIORITY_CLASS_MAP +import attr +from synapse.config.experimental import ExperimentalConfig +from synapse.push.rulekinds import PRIORITY_CLASS_MAP -def list_with_base_rules(rawrules: List[Dict[str, Any]]) -> List[Dict[str, Any]]: - """Combine the list of rules set by the user with the default push rules - Args: - rawrules: The rules the user has modified or set. +@attr.s(auto_attribs=True, slots=True, frozen=True) +class PushRule: + rule_id: str + priority_class: int + conditions: Sequence[Mapping[str, str]] + actions: Sequence[Union[str, Mapping]] + default: bool = False + default_enabled: bool = True - Returns: - A new list with the rules set by the user combined with the defaults. + +@attr.s(auto_attribs=True, slots=True, frozen=True, weakref_slot=False) +class PushRules: + """A collection of push rules for an account. + + Can be iterated over, producing push rules in priority order. """ - ruleslist = [] - # Grab the base rules that the user has modified. - # The modified base rules have a priority_class of -1. - modified_base_rules = {r["rule_id"]: r for r in rawrules if r["priority_class"] < 0} + # A mapping from rule ID to push rule that overrides a base rule. These will + # be returned instead of the base rule. + overriden_base_rules: Dict[str, PushRule] = attr.Factory(dict) + + # The following stores the custom push rules at each priority class. + # + # We keep these separate (rather than combining into one big list) to avoid + # copying the base rules around all the time. + override: List[PushRule] = attr.Factory(list) + content: List[PushRule] = attr.Factory(list) + room: List[PushRule] = attr.Factory(list) + sender: List[PushRule] = attr.Factory(list) + underride: List[PushRule] = attr.Factory(list) + + def __iter__(self) -> Iterator[PushRule]: + # When iterating over the push rules we need to return the base rules + # interspersed at the correct spots. + for rule in itertools.chain( + BASE_PREPEND_OVERRIDE_RULES, + self.override, + BASE_APPEND_OVERRIDE_RULES, + self.content, + BASE_APPEND_CONTENT_RULES, + self.room, + self.sender, + self.underride, + BASE_APPEND_UNDERRIDE_RULES, + ): + # Check if a base rule has been overriden by a custom rule. If so + # return that instead. + override_rule = self.overriden_base_rules.get(rule.rule_id) + if override_rule: + yield override_rule + else: + yield rule + + def __len__(self) -> int: + return ( + len(self.overriden_base_rules) + + len(self.override) + + len(self.content) + + len(self.room) + + len(self.sender) + + len(self.underride) + ) - # Remove the modified base rules from the list, They'll be added back - # in the default positions in the list. - rawrules = [r for r in rawrules if r["priority_class"] >= 0] - # shove the server default rules for each kind onto the end of each - current_prio_class = list(PRIORITY_CLASS_INVERSE_MAP)[-1] +@attr.s(auto_attribs=True, slots=True, frozen=True, weakref_slot=False) +class FilteredPushRules: + """A wrapper around `PushRules` that filters out disabled experimental push + rules, and includes the "enabled" state for each rule when iterated over. + """ - ruleslist.extend( - make_base_prepend_rules( - PRIORITY_CLASS_INVERSE_MAP[current_prio_class], modified_base_rules - ) - ) + push_rules: PushRules + enabled_map: Dict[str, bool] + experimental_config: ExperimentalConfig - for r in rawrules: - if r["priority_class"] < current_prio_class: - while r["priority_class"] < current_prio_class: - ruleslist.extend( - make_base_append_rules( - PRIORITY_CLASS_INVERSE_MAP[current_prio_class], - modified_base_rules, - ) - ) - current_prio_class -= 1 - if current_prio_class > 0: - ruleslist.extend( - make_base_prepend_rules( - PRIORITY_CLASS_INVERSE_MAP[current_prio_class], - modified_base_rules, - ) - ) - - ruleslist.append(r) - - while current_prio_class > 0: - ruleslist.extend( - make_base_append_rules( - PRIORITY_CLASS_INVERSE_MAP[current_prio_class], modified_base_rules - ) - ) - current_prio_class -= 1 - if current_prio_class > 0: - ruleslist.extend( - make_base_prepend_rules( - PRIORITY_CLASS_INVERSE_MAP[current_prio_class], modified_base_rules - ) - ) + def __iter__(self) -> Iterator[Tuple[PushRule, bool]]: + for rule in self.push_rules: + if not _is_experimental_rule_enabled( + rule.rule_id, self.experimental_config + ): + continue - return ruleslist + enabled = self.enabled_map.get(rule.rule_id, rule.default_enabled) + yield rule, enabled -def make_base_append_rules( - kind: str, modified_base_rules: Dict[str, Dict[str, Any]] -) -> List[Dict[str, Any]]: - rules = [] + def __len__(self) -> int: + return len(self.push_rules) - if kind == "override": - rules = BASE_APPEND_OVERRIDE_RULES - elif kind == "underride": - rules = BASE_APPEND_UNDERRIDE_RULES - elif kind == "content": - rules = BASE_APPEND_CONTENT_RULES - # Copy the rules before modifying them - rules = copy.deepcopy(rules) - for r in rules: - # Only modify the actions, keep the conditions the same. - assert isinstance(r["rule_id"], str) - modified = modified_base_rules.get(r["rule_id"]) - if modified: - r["actions"] = modified["actions"] +DEFAULT_EMPTY_PUSH_RULES = PushRules() - return rules +def compile_push_rules(rawrules: List[PushRule]) -> PushRules: + """Given a set of custom push rules return a `PushRules` instance (which + includes the base rules). + """ + + if not rawrules: + # Fast path to avoid allocating empty lists when there are no custom + # rules for the user. + return DEFAULT_EMPTY_PUSH_RULES -def make_base_prepend_rules( - kind: str, - modified_base_rules: Dict[str, Dict[str, Any]], -) -> List[Dict[str, Any]]: - rules = [] + rules = PushRules() - if kind == "override": - rules = BASE_PREPEND_OVERRIDE_RULES + for rule in rawrules: + # We need to decide which bucket each custom push rule goes into. - # Copy the rules before modifying them - rules = copy.deepcopy(rules) - for r in rules: - # Only modify the actions, keep the conditions the same. - assert isinstance(r["rule_id"], str) - modified = modified_base_rules.get(r["rule_id"]) - if modified: - r["actions"] = modified["actions"] + # If it has the same ID as a base rule then it overrides that... + overriden_base_rule = BASE_RULES_BY_ID.get(rule.rule_id) + if overriden_base_rule: + rules.overriden_base_rules[rule.rule_id] = attr.evolve( + overriden_base_rule, actions=rule.actions + ) + continue + + # ... otherwise it gets added to the appropriate priority class bucket + collection: List[PushRule] + if rule.priority_class == 5: + collection = rules.override + elif rule.priority_class == 4: + collection = rules.content + elif rule.priority_class == 3: + collection = rules.room + elif rule.priority_class == 2: + collection = rules.sender + elif rule.priority_class == 1: + collection = rules.underride + else: + raise Exception(f"Unknown priority class: {rule.priority_class}") + + collection.append(rule) return rules -# We have to annotate these types, otherwise mypy infers them as -# `List[Dict[str, Sequence[Collection[str]]]]`. -BASE_APPEND_CONTENT_RULES: List[Dict[str, Any]] = [ - { - "rule_id": "global/content/.m.rule.contains_user_name", - "conditions": [ - { - "kind": "event_match", - "key": "content.body", - # Match the localpart of the requester's MXID. - "pattern_type": "user_localpart", - } - ], - "actions": [ - "notify", - {"set_tweak": "sound", "value": "default"}, - {"set_tweak": "highlight"}, - ], - } +def _is_experimental_rule_enabled( + rule_id: str, experimental_config: ExperimentalConfig +) -> bool: + """Used by `_load_rules` to filter out experimental rules when they + have not been enabled. + """ + if ( + rule_id == "global/override/.org.matrix.msc3786.rule.room.server_acl" + and not experimental_config.msc3786_enabled + ): + return False + if ( + rule_id == "global/underride/.org.matrix.msc3772.thread_reply" + and not experimental_config.msc3772_enabled + ): + return False + return True + + +BASE_APPEND_CONTENT_RULES = [ + PushRule( + default=True, + priority_class=PRIORITY_CLASS_MAP["content"], + **{ + "rule_id": "global/content/.m.rule.contains_user_name", + "conditions": [ + { + "kind": "event_match", + "key": "content.body", + # Match the localpart of the requester's MXID. + "pattern_type": "user_localpart", + } + ], + "actions": [ + "notify", + {"set_tweak": "sound", "value": "default"}, + {"set_tweak": "highlight"}, + ], + }, + ) ] -BASE_PREPEND_OVERRIDE_RULES: List[Dict[str, Any]] = [ - { - "rule_id": "global/override/.m.rule.master", - "enabled": False, - "conditions": [], - "actions": ["dont_notify"], - } +BASE_PREPEND_OVERRIDE_RULES = [ + PushRule( + default=True, + priority_class=PRIORITY_CLASS_MAP["override"], + **{ + "rule_id": "global/override/.m.rule.master", + "enabled": False, + "conditions": [], + "actions": ["dont_notify"], + }, + ) ] -BASE_APPEND_OVERRIDE_RULES: List[Dict[str, Any]] = [ - { - "rule_id": "global/override/.m.rule.suppress_notices", - "conditions": [ - { - "kind": "event_match", - "key": "content.msgtype", - "pattern": "m.notice", - "_cache_key": "_suppress_notices", - } - ], - "actions": ["dont_notify"], - }, +BASE_APPEND_OVERRIDE_RULES = [ + PushRule( + default=True, + priority_class=PRIORITY_CLASS_MAP["override"], + **{ + "rule_id": "global/override/.m.rule.suppress_notices", + "conditions": [ + { + "kind": "event_match", + "key": "content.msgtype", + "pattern": "m.notice", + "_cache_key": "_suppress_notices", + } + ], + "actions": ["dont_notify"], + }, + ), # NB. .m.rule.invite_for_me must be higher prio than .m.rule.member_event # otherwise invites will be matched by .m.rule.member_event - { - "rule_id": "global/override/.m.rule.invite_for_me", - "conditions": [ - { - "kind": "event_match", - "key": "type", - "pattern": "m.room.member", - "_cache_key": "_member", - }, - { - "kind": "event_match", - "key": "content.membership", - "pattern": "invite", - "_cache_key": "_invite_member", - }, - # Match the requester's MXID. - {"kind": "event_match", "key": "state_key", "pattern_type": "user_id"}, - ], - "actions": [ - "notify", - {"set_tweak": "sound", "value": "default"}, - {"set_tweak": "highlight", "value": False}, - ], - }, + PushRule( + default=True, + priority_class=PRIORITY_CLASS_MAP["override"], + **{ + "rule_id": "global/override/.m.rule.invite_for_me", + "conditions": [ + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.member", + "_cache_key": "_member", + }, + { + "kind": "event_match", + "key": "content.membership", + "pattern": "invite", + "_cache_key": "_invite_member", + }, + # Match the requester's MXID. + {"kind": "event_match", "key": "state_key", "pattern_type": "user_id"}, + ], + "actions": [ + "notify", + {"set_tweak": "sound", "value": "default"}, + {"set_tweak": "highlight", "value": False}, + ], + }, + ), # Will we sometimes want to know about people joining and leaving? # Perhaps: if so, this could be expanded upon. Seems the most usual case # is that we don't though. We add this override rule so that even if # the room rule is set to notify, we don't get notifications about # join/leave/avatar/displayname events. # See also: https://matrix.org/jira/browse/SYN-607 - { - "rule_id": "global/override/.m.rule.member_event", - "conditions": [ - { - "kind": "event_match", - "key": "type", - "pattern": "m.room.member", - "_cache_key": "_member", - } - ], - "actions": ["dont_notify"], - }, + PushRule( + default=True, + priority_class=PRIORITY_CLASS_MAP["override"], + **{ + "rule_id": "global/override/.m.rule.member_event", + "conditions": [ + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.member", + "_cache_key": "_member", + } + ], + "actions": ["dont_notify"], + }, + ), # This was changed from underride to override so it's closer in priority # to the content rules where the user name highlight rule lives. This # way a room rule is lower priority than both but a custom override rule # is higher priority than both. - { - "rule_id": "global/override/.m.rule.contains_display_name", - "conditions": [{"kind": "contains_display_name"}], - "actions": [ - "notify", - {"set_tweak": "sound", "value": "default"}, - {"set_tweak": "highlight"}, - ], - }, - { - "rule_id": "global/override/.m.rule.roomnotif", - "conditions": [ - { - "kind": "event_match", - "key": "content.body", - "pattern": "@room", - "_cache_key": "_roomnotif_content", - }, - { - "kind": "sender_notification_permission", - "key": "room", - "_cache_key": "_roomnotif_pl", - }, - ], - "actions": ["notify", {"set_tweak": "highlight", "value": True}], - }, - { - "rule_id": "global/override/.m.rule.tombstone", - "conditions": [ - { - "kind": "event_match", - "key": "type", - "pattern": "m.room.tombstone", - "_cache_key": "_tombstone", - }, - { - "kind": "event_match", - "key": "state_key", - "pattern": "", - "_cache_key": "_tombstone_statekey", - }, - ], - "actions": ["notify", {"set_tweak": "highlight", "value": True}], - }, - { - "rule_id": "global/override/.m.rule.reaction", - "conditions": [ - { - "kind": "event_match", - "key": "type", - "pattern": "m.reaction", - "_cache_key": "_reaction", - } - ], - "actions": ["dont_notify"], - }, + PushRule( + default=True, + priority_class=PRIORITY_CLASS_MAP["override"], + **{ + "rule_id": "global/override/.m.rule.contains_display_name", + "conditions": [{"kind": "contains_display_name"}], + "actions": [ + "notify", + {"set_tweak": "sound", "value": "default"}, + {"set_tweak": "highlight"}, + ], + }, + ), + PushRule( + default=True, + priority_class=PRIORITY_CLASS_MAP["override"], + **{ + "rule_id": "global/override/.m.rule.roomnotif", + "conditions": [ + { + "kind": "event_match", + "key": "content.body", + "pattern": "@room", + "_cache_key": "_roomnotif_content", + }, + { + "kind": "sender_notification_permission", + "key": "room", + "_cache_key": "_roomnotif_pl", + }, + ], + "actions": ["notify", {"set_tweak": "highlight", "value": True}], + }, + ), + PushRule( + default=True, + priority_class=PRIORITY_CLASS_MAP["override"], + **{ + "rule_id": "global/override/.m.rule.tombstone", + "conditions": [ + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.tombstone", + "_cache_key": "_tombstone", + }, + { + "kind": "event_match", + "key": "state_key", + "pattern": "", + "_cache_key": "_tombstone_statekey", + }, + ], + "actions": ["notify", {"set_tweak": "highlight", "value": True}], + }, + ), + PushRule( + default=True, + priority_class=PRIORITY_CLASS_MAP["override"], + **{ + "rule_id": "global/override/.m.rule.reaction", + "conditions": [ + { + "kind": "event_match", + "key": "type", + "pattern": "m.reaction", + "_cache_key": "_reaction", + } + ], + "actions": ["dont_notify"], + }, + ), # XXX: This is an experimental rule that is only enabled if msc3786_enabled # is enabled, if it is not the rule gets filtered out in _load_rules() in # PushRulesWorkerStore - { - "rule_id": "global/override/.org.matrix.msc3786.rule.room.server_acl", - "conditions": [ - { - "kind": "event_match", - "key": "type", - "pattern": "m.room.server_acl", - "_cache_key": "_room_server_acl", - }, - { - "kind": "event_match", - "key": "state_key", - "pattern": "", - "_cache_key": "_room_server_acl_state_key", - }, - ], - "actions": [], - }, + PushRule( + default=True, + priority_class=PRIORITY_CLASS_MAP["override"], + **{ + "rule_id": "global/override/.org.matrix.msc3786.rule.room.server_acl", + "conditions": [ + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.server_acl", + "_cache_key": "_room_server_acl", + }, + { + "kind": "event_match", + "key": "state_key", + "pattern": "", + "_cache_key": "_room_server_acl_state_key", + }, + ], + "actions": [], + }, + ), ] -BASE_APPEND_UNDERRIDE_RULES: List[Dict[str, Any]] = [ - { - "rule_id": "global/underride/.m.rule.call", - "conditions": [ - { - "kind": "event_match", - "key": "type", - "pattern": "m.call.invite", - "_cache_key": "_call", - } - ], - "actions": [ - "notify", - {"set_tweak": "sound", "value": "ring"}, - {"set_tweak": "highlight", "value": False}, - ], - }, +BASE_APPEND_UNDERRIDE_RULES = [ + PushRule( + default=True, + priority_class=PRIORITY_CLASS_MAP["underride"], + **{ + "rule_id": "global/underride/.m.rule.call", + "conditions": [ + { + "kind": "event_match", + "key": "type", + "pattern": "m.call.invite", + "_cache_key": "_call", + } + ], + "actions": [ + "notify", + {"set_tweak": "sound", "value": "ring"}, + {"set_tweak": "highlight", "value": False}, + ], + }, + ), # XXX: once m.direct is standardised everywhere, we should use it to detect # a DM from the user's perspective rather than this heuristic. - { - "rule_id": "global/underride/.m.rule.room_one_to_one", - "conditions": [ - {"kind": "room_member_count", "is": "2", "_cache_key": "member_count"}, - { - "kind": "event_match", - "key": "type", - "pattern": "m.room.message", - "_cache_key": "_message", - }, - ], - "actions": [ - "notify", - {"set_tweak": "sound", "value": "default"}, - {"set_tweak": "highlight", "value": False}, - ], - }, + PushRule( + default=True, + priority_class=PRIORITY_CLASS_MAP["underride"], + **{ + "rule_id": "global/underride/.m.rule.room_one_to_one", + "conditions": [ + {"kind": "room_member_count", "is": "2", "_cache_key": "member_count"}, + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.message", + "_cache_key": "_message", + }, + ], + "actions": [ + "notify", + {"set_tweak": "sound", "value": "default"}, + {"set_tweak": "highlight", "value": False}, + ], + }, + ), # XXX: this is going to fire for events which aren't m.room.messages # but are encrypted (e.g. m.call.*)... - { - "rule_id": "global/underride/.m.rule.encrypted_room_one_to_one", - "conditions": [ - {"kind": "room_member_count", "is": "2", "_cache_key": "member_count"}, - { - "kind": "event_match", - "key": "type", - "pattern": "m.room.encrypted", - "_cache_key": "_encrypted", - }, - ], - "actions": [ - "notify", - {"set_tweak": "sound", "value": "default"}, - {"set_tweak": "highlight", "value": False}, - ], - }, - { - "rule_id": "global/underride/.org.matrix.msc3772.thread_reply", - "conditions": [ - { - "kind": "org.matrix.msc3772.relation_match", - "rel_type": "m.thread", - # Match the requester's MXID. - "sender_type": "user_id", - } - ], - "actions": ["notify", {"set_tweak": "highlight", "value": False}], - }, - { - "rule_id": "global/underride/.m.rule.message", - "conditions": [ - { - "kind": "event_match", - "key": "type", - "pattern": "m.room.message", - "_cache_key": "_message", - } - ], - "actions": ["notify", {"set_tweak": "highlight", "value": False}], - }, + PushRule( + default=True, + priority_class=PRIORITY_CLASS_MAP["underride"], + **{ + "rule_id": "global/underride/.m.rule.encrypted_room_one_to_one", + "conditions": [ + {"kind": "room_member_count", "is": "2", "_cache_key": "member_count"}, + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.encrypted", + "_cache_key": "_encrypted", + }, + ], + "actions": [ + "notify", + {"set_tweak": "sound", "value": "default"}, + {"set_tweak": "highlight", "value": False}, + ], + }, + ), + PushRule( + default=True, + priority_class=PRIORITY_CLASS_MAP["underride"], + **{ + "rule_id": "global/underride/.org.matrix.msc3772.thread_reply", + "conditions": [ + { + "kind": "org.matrix.msc3772.relation_match", + "rel_type": "m.thread", + # Match the requester's MXID. + "sender_type": "user_id", + } + ], + "actions": ["notify", {"set_tweak": "highlight", "value": False}], + }, + ), + PushRule( + default=True, + priority_class=PRIORITY_CLASS_MAP["underride"], + **{ + "rule_id": "global/underride/.m.rule.message", + "conditions": [ + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.message", + "_cache_key": "_message", + } + ], + "actions": ["notify", {"set_tweak": "highlight", "value": False}], + }, + ), # XXX: this is going to fire for events which aren't m.room.messages # but are encrypted (e.g. m.call.*)... - { - "rule_id": "global/underride/.m.rule.encrypted", - "conditions": [ - { - "kind": "event_match", - "key": "type", - "pattern": "m.room.encrypted", - "_cache_key": "_encrypted", - } - ], - "actions": ["notify", {"set_tweak": "highlight", "value": False}], - }, - { - "rule_id": "global/underride/.im.vector.jitsi", - "conditions": [ - { - "kind": "event_match", - "key": "type", - "pattern": "im.vector.modular.widgets", - "_cache_key": "_type_modular_widgets", - }, - { - "kind": "event_match", - "key": "content.type", - "pattern": "jitsi", - "_cache_key": "_content_type_jitsi", - }, - { - "kind": "event_match", - "key": "state_key", - "pattern": "*", - "_cache_key": "_is_state_event", - }, - ], - "actions": ["notify", {"set_tweak": "highlight", "value": False}], - }, + PushRule( + default=True, + priority_class=PRIORITY_CLASS_MAP["underride"], + **{ + "rule_id": "global/underride/.m.rule.encrypted", + "conditions": [ + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.encrypted", + "_cache_key": "_encrypted", + } + ], + "actions": ["notify", {"set_tweak": "highlight", "value": False}], + }, + ), + PushRule( + default=True, + priority_class=PRIORITY_CLASS_MAP["underride"], + **{ + "rule_id": "global/underride/.im.vector.jitsi", + "conditions": [ + { + "kind": "event_match", + "key": "type", + "pattern": "im.vector.modular.widgets", + "_cache_key": "_type_modular_widgets", + }, + { + "kind": "event_match", + "key": "content.type", + "pattern": "jitsi", + "_cache_key": "_content_type_jitsi", + }, + { + "kind": "event_match", + "key": "state_key", + "pattern": "*", + "_cache_key": "_is_state_event", + }, + ], + "actions": ["notify", {"set_tweak": "highlight", "value": False}], + }, + ), ] BASE_RULE_IDS = set() +BASE_RULES_BY_ID: Dict[str, PushRule] = {} + for r in BASE_APPEND_CONTENT_RULES: - r["priority_class"] = PRIORITY_CLASS_MAP["content"] - r["default"] = True - BASE_RULE_IDS.add(r["rule_id"]) + BASE_RULE_IDS.add(r.rule_id) + BASE_RULES_BY_ID[r.rule_id] = r for r in BASE_PREPEND_OVERRIDE_RULES: - r["priority_class"] = PRIORITY_CLASS_MAP["override"] - r["default"] = True - BASE_RULE_IDS.add(r["rule_id"]) + BASE_RULE_IDS.add(r.rule_id) + BASE_RULES_BY_ID[r.rule_id] = r for r in BASE_APPEND_OVERRIDE_RULES: - r["priority_class"] = PRIORITY_CLASS_MAP["override"] - r["default"] = True - BASE_RULE_IDS.add(r["rule_id"]) + BASE_RULE_IDS.add(r.rule_id) + BASE_RULES_BY_ID[r.rule_id] = r for r in BASE_APPEND_UNDERRIDE_RULES: - r["priority_class"] = PRIORITY_CLASS_MAP["underride"] - r["default"] = True - BASE_RULE_IDS.add(r["rule_id"]) + BASE_RULE_IDS.add(r.rule_id) + BASE_RULES_BY_ID[r.rule_id] = r diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index be4405041cc0..ccd512be54b9 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -17,7 +17,6 @@ import logging from typing import ( TYPE_CHECKING, - Any, Collection, Dict, Iterable, @@ -42,6 +41,7 @@ from synapse.util.metrics import measure_func from synapse.visibility import filter_event_for_clients_with_state +from .baserules import FilteredPushRules, PushRule from .push_rule_evaluator import PushRuleEvaluatorForEvent if TYPE_CHECKING: @@ -124,7 +124,7 @@ def __init__(self, hs: "HomeServer"): async def _get_rules_for_event( self, event: EventBase, - ) -> Dict[str, List[Dict[str, Any]]]: + ) -> Dict[str, FilteredPushRules]: """Get the push rules for all users who may need to be notified about the event. @@ -198,7 +198,7 @@ async def _get_power_levels_and_sender_level( return pl_event.content if pl_event else {}, sender_level async def _get_mutual_relations( - self, event: EventBase, rules: Iterable[Dict[str, Any]] + self, event: EventBase, rules: Iterable[Tuple[PushRule, bool]] ) -> Dict[str, Set[Tuple[str, str]]]: """ Fetch event metadata for events which related to the same event as the given event. @@ -228,12 +228,11 @@ async def _get_mutual_relations( # Pre-filter to figure out which relation types are interesting. rel_types = set() - for rule in rules: - # Skip disabled rules. - if "enabled" in rule and not rule["enabled"]: + for rule, enabled in rules: + if not enabled: continue - for condition in rule["conditions"]: + for condition in rule.conditions: if condition["kind"] != "org.matrix.msc3772.relation_match": continue @@ -329,15 +328,13 @@ async def action_for_event_by_user( # current user, it'll be added to the dict later. actions_by_user[uid] = [] - for rule in rules: - if "enabled" in rule and not rule["enabled"]: + for rule, enabled in rules: + if not enabled: continue - matches = evaluator.check_conditions( - rule["conditions"], uid, display_name - ) + matches = evaluator.check_conditions(rule.conditions, uid, display_name) if matches: - actions = [x for x in rule["actions"] if x != "dont_notify"] + actions = [x for x in rule.actions if x != "dont_notify"] if actions and "notify" in actions: # Push rules say we should notify the user of this event actions_by_user[uid] = actions diff --git a/synapse/push/clientformat.py b/synapse/push/clientformat.py index 5117ef6854f1..67b8efe00055 100644 --- a/synapse/push/clientformat.py +++ b/synapse/push/clientformat.py @@ -18,16 +18,15 @@ from synapse.push.rulekinds import PRIORITY_CLASS_INVERSE_MAP, PRIORITY_CLASS_MAP from synapse.types import UserID +from .baserules import FilteredPushRules, PushRule + def format_push_rules_for_user( - user: UserID, ruleslist: List + user: UserID, ruleslist: FilteredPushRules ) -> Dict[str, Dict[str, list]]: """Converts a list of rawrules and a enabled map into nested dictionaries to match the Matrix client-server format for push rules""" - # We're going to be mutating this a lot, so do a deep copy - ruleslist = copy.deepcopy(ruleslist) - rules: Dict[str, Dict[str, List[Dict[str, Any]]]] = { "global": {}, "device": {}, @@ -35,11 +34,25 @@ def format_push_rules_for_user( rules["global"] = _add_empty_priority_class_arrays(rules["global"]) - for r in ruleslist: - template_name = _priority_class_to_template_name(r["priority_class"]) + for r, enabled in ruleslist: + template_name = _priority_class_to_template_name(r.priority_class) + + rulearray = rules["global"][template_name] + + template_rule = _rule_to_template(r) + if not template_rule: + continue + + rulearray.append(template_rule) + + template_rule["enabled"] = enabled + + if "conditions" not in template_rule: + continue # Remove internal stuff. - for c in r["conditions"]: + template_rule["conditions"] = copy.deepcopy(template_rule["conditions"]) + for c in template_rule["conditions"]: c.pop("_cache_key", None) pattern_type = c.pop("pattern_type", None) @@ -52,16 +65,6 @@ def format_push_rules_for_user( if sender_type == "user_id": c["sender"] = user.to_string() - rulearray = rules["global"][template_name] - - template_rule = _rule_to_template(r) - if template_rule: - if "enabled" in r: - template_rule["enabled"] = r["enabled"] - else: - template_rule["enabled"] = True - rulearray.append(template_rule) - return rules @@ -71,24 +74,24 @@ def _add_empty_priority_class_arrays(d: Dict[str, list]) -> Dict[str, list]: return d -def _rule_to_template(rule: Dict[str, Any]) -> Optional[Dict[str, Any]]: - unscoped_rule_id = None - if "rule_id" in rule: - unscoped_rule_id = _rule_id_from_namespaced(rule["rule_id"]) +def _rule_to_template(rule: PushRule) -> Optional[Dict[str, Any]]: + templaterule: Dict[str, Any] + + unscoped_rule_id = _rule_id_from_namespaced(rule.rule_id) - template_name = _priority_class_to_template_name(rule["priority_class"]) + template_name = _priority_class_to_template_name(rule.priority_class) if template_name in ["override", "underride"]: - templaterule = {k: rule[k] for k in ["conditions", "actions"]} + templaterule = {"conditions": rule.conditions, "actions": rule.actions} elif template_name in ["sender", "room"]: - templaterule = {"actions": rule["actions"]} - unscoped_rule_id = rule["conditions"][0]["pattern"] + templaterule = {"actions": rule.actions} + unscoped_rule_id = rule.conditions[0]["pattern"] elif template_name == "content": - if len(rule["conditions"]) != 1: + if len(rule.conditions) != 1: return None - thecond = rule["conditions"][0] + thecond = rule.conditions[0] if "pattern" not in thecond: return None - templaterule = {"actions": rule["actions"]} + templaterule = {"actions": rule.actions} templaterule["pattern"] = thecond["pattern"] else: # This should not be reached unless this function is not kept in sync @@ -97,8 +100,8 @@ def _rule_to_template(rule: Dict[str, Any]) -> Optional[Dict[str, Any]]: if unscoped_rule_id: templaterule["rule_id"] = unscoped_rule_id - if "default" in rule: - templaterule["default"] = rule["default"] + if rule.default: + templaterule["default"] = True return templaterule diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 5f7348c4f2fe..66964c5851eb 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -30,7 +30,7 @@ from synapse.api.errors import StoreError from synapse.config.homeserver import ExperimentalConfig -from synapse.push.baserules import list_with_base_rules +from synapse.push.baserules import FilteredPushRules, PushRule, compile_push_rules from synapse.replication.slave.storage._slaved_id_tracker import SlavedIdTracker from synapse.storage._base import SQLBaseStore, db_to_json from synapse.storage.database import ( @@ -62,60 +62,31 @@ logger = logging.getLogger(__name__) -def _is_experimental_rule_enabled( - rule_id: str, experimental_config: ExperimentalConfig -) -> bool: - """Used by `_load_rules` to filter out experimental rules when they - have not been enabled. - """ - if ( - rule_id == "global/override/.org.matrix.msc3786.rule.room.server_acl" - and not experimental_config.msc3786_enabled - ): - return False - if ( - rule_id == "global/underride/.org.matrix.msc3772.thread_reply" - and not experimental_config.msc3772_enabled - ): - return False - return True - - def _load_rules( rawrules: List[JsonDict], enabled_map: Dict[str, bool], experimental_config: ExperimentalConfig, -) -> List[JsonDict]: +) -> FilteredPushRules: + """Take the DB rows returned from the DB and convert them into a full + `FilteredPushRules` object. + """ + ruleslist = [] for rawrule in rawrules: - rule = dict(rawrule) - rule["conditions"] = db_to_json(rawrule["conditions"]) - rule["actions"] = db_to_json(rawrule["actions"]) - rule["default"] = False - ruleslist.append(rule) - - # We're going to be mutating this a lot, so copy it. We also filter out - # any experimental default push rules that aren't enabled. - rules = [ - rule - for rule in list_with_base_rules(ruleslist) - if _is_experimental_rule_enabled(rule["rule_id"], experimental_config) - ] - - for i, rule in enumerate(rules): - rule_id = rule["rule_id"] + ruleslist.append( + PushRule( + rule_id=rawrule["rule_id"], + priority_class=rawrule["priority_class"], + conditions=db_to_json(rawrule["conditions"]), + actions=db_to_json(rawrule["actions"]), + ) + ) - if rule_id not in enabled_map: - continue - if rule.get("enabled", True) == bool(enabled_map[rule_id]): - continue + push_rules = compile_push_rules(ruleslist) - # Rules are cached across users. - rule = dict(rule) - rule["enabled"] = bool(enabled_map[rule_id]) - rules[i] = rule + filtered_rules = FilteredPushRules(push_rules, enabled_map, experimental_config) - return rules + return filtered_rules # The ABCMeta metaclass ensures that it cannot be instantiated without @@ -174,7 +145,7 @@ def get_max_push_rules_stream_id(self) -> int: raise NotImplementedError() @cached(max_entries=5000) - async def get_push_rules_for_user(self, user_id: str) -> List[JsonDict]: + async def get_push_rules_for_user(self, user_id: str) -> FilteredPushRules: rows = await self.db_pool.simple_select_list( table="push_rules", keyvalues={"user_name": user_id}, @@ -228,11 +199,11 @@ def have_push_rules_changed_txn(txn: LoggingTransaction) -> bool: @cachedList(cached_method_name="get_push_rules_for_user", list_name="user_ids") async def bulk_get_push_rules( self, user_ids: Collection[str] - ) -> Dict[str, List[JsonDict]]: + ) -> Dict[str, FilteredPushRules]: if not user_ids: return {} - results: Dict[str, List[JsonDict]] = {user_id: [] for user_id in user_ids} + raw_rules: Dict[str, List[JsonDict]] = {user_id: [] for user_id in user_ids} rows = await self.db_pool.simple_select_many_batch( table="push_rules", @@ -246,11 +217,13 @@ async def bulk_get_push_rules( rows.sort(key=lambda row: (-int(row["priority_class"]), -int(row["priority"]))) for row in rows: - results.setdefault(row["user_name"], []).append(row) + raw_rules.setdefault(row["user_name"], []).append(row) enabled_map_by_user = await self.bulk_get_push_rules_enabled(user_ids) - for user_id, rules in results.items(): + results: Dict[str, FilteredPushRules] = {} + + for user_id, rules in raw_rules.items(): results[user_id] = _load_rules( rules, enabled_map_by_user.get(user_id, {}), self.hs.config.experimental ) @@ -829,7 +802,7 @@ def get_max_push_rules_stream_id(self) -> int: return self._push_rules_stream_id_gen.get_current_token() async def copy_push_rule_from_room_to_room( - self, new_room_id: str, user_id: str, rule: dict + self, new_room_id: str, user_id: str, rule: PushRule ) -> None: """Copy a single push rule from one room to another for a specific user. @@ -839,21 +812,27 @@ async def copy_push_rule_from_room_to_room( rule: A push rule. """ # Create new rule id - rule_id_scope = "/".join(rule["rule_id"].split("/")[:-1]) + rule_id_scope = "/".join(rule.rule_id.split("/")[:-1]) new_rule_id = rule_id_scope + "/" + new_room_id + new_conditions = [] + # Change room id in each condition - for condition in rule.get("conditions", []): + for condition in rule.conditions: + new_condition = condition if condition.get("key") == "room_id": - condition["pattern"] = new_room_id + new_condition = dict(condition) + new_condition["pattern"] = new_room_id + + new_conditions.append(condition) # Add the rule for the new room await self.add_push_rule( user_id=user_id, rule_id=new_rule_id, - priority_class=rule["priority_class"], - conditions=rule["conditions"], - actions=rule["actions"], + priority_class=rule.priority_class, + conditions=new_conditions, + actions=rule.actions, ) async def copy_push_rules_from_room_to_room_for_user( @@ -871,8 +850,11 @@ async def copy_push_rules_from_room_to_room_for_user( user_push_rules = await self.get_push_rules_for_user(user_id) # Get rules relating to the old room and copy them to the new room - for rule in user_push_rules: - conditions = rule.get("conditions", []) + for rule, enabled in user_push_rules: + if not enabled: + continue + + conditions = rule.conditions if any( (c.get("key") == "room_id" and c.get("pattern") == old_room_id) for c in conditions diff --git a/tests/handlers/test_deactivate_account.py b/tests/handlers/test_deactivate_account.py index ff9f2e8edb16..82baa8f1542a 100644 --- a/tests/handlers/test_deactivate_account.py +++ b/tests/handlers/test_deactivate_account.py @@ -11,11 +11,11 @@ # 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, Dict from twisted.test.proto_helpers import MemoryReactor from synapse.api.constants import AccountDataTypes +from synapse.push.baserules import PushRule from synapse.push.rulekinds import PRIORITY_CLASS_MAP from synapse.rest import admin from synapse.rest.client import account, login @@ -130,12 +130,12 @@ def test_room_account_data_deleted_upon_deactivation(self) -> None: ), ) - def _is_custom_rule(self, push_rule: Dict[str, Any]) -> bool: + def _is_custom_rule(self, push_rule: PushRule) -> bool: """ Default rules start with a dot: such as .m.rule and .im.vector. This function returns true iff a rule is custom (not default). """ - return "/." not in push_rule["rule_id"] + return "/." not in push_rule.rule_id def test_push_rules_deleted_upon_account_deactivation(self) -> None: """ @@ -157,22 +157,21 @@ def test_push_rules_deleted_upon_account_deactivation(self) -> None: ) # Test the rule exists - push_rules = self.get_success(self._store.get_push_rules_for_user(self.user)) + filtered_push_rules = self.get_success( + self._store.get_push_rules_for_user(self.user) + ) # Filter out default rules; we don't care - push_rules = list(filter(self._is_custom_rule, push_rules)) + push_rules = [r for r, _ in filtered_push_rules if self._is_custom_rule(r)] # Check our rule made it self.assertEqual( push_rules, [ - { - "user_name": "@user:test", - "rule_id": "personal.override.rule1", - "priority_class": 5, - "priority": 0, - "conditions": [], - "actions": [], - "default": False, - } + PushRule( + rule_id="personal.override.rule1", + priority_class=5, + conditions=[], + actions=[], + ) ], push_rules, ) @@ -180,9 +179,11 @@ def test_push_rules_deleted_upon_account_deactivation(self) -> None: # Request the deactivation of our account self._deactivate_my_account() - push_rules = self.get_success(self._store.get_push_rules_for_user(self.user)) + filtered_push_rules = self.get_success( + self._store.get_push_rules_for_user(self.user) + ) # Filter out default rules; we don't care - push_rules = list(filter(self._is_custom_rule, push_rules)) + push_rules = [r for r, _ in filtered_push_rules if self._is_custom_rule(r)] # Check our rule no longer exists self.assertEqual(push_rules, [], push_rules) From 80ea239a23d1eef531ebea5db549aa4e46f07615 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 12 Aug 2022 18:44:57 +0100 Subject: [PATCH 3/8] Fix up base rules --- synapse/push/baserules.py | 474 ++++++++++++++++++-------------------- 1 file changed, 220 insertions(+), 254 deletions(-) diff --git a/synapse/push/baserules.py b/synapse/push/baserules.py index d4cba802a18e..70016ea2d960 100644 --- a/synapse/push/baserules.py +++ b/synapse/push/baserules.py @@ -181,22 +181,20 @@ def _is_experimental_rule_enabled( PushRule( default=True, priority_class=PRIORITY_CLASS_MAP["content"], - **{ - "rule_id": "global/content/.m.rule.contains_user_name", - "conditions": [ - { - "kind": "event_match", - "key": "content.body", - # Match the localpart of the requester's MXID. - "pattern_type": "user_localpart", - } - ], - "actions": [ - "notify", - {"set_tweak": "sound", "value": "default"}, - {"set_tweak": "highlight"}, - ], - }, + rule_id="global/content/.m.rule.contains_user_name", + conditions=[ + { + "kind": "event_match", + "key": "content.body", + # Match the localpart of the requester's MXID. + "pattern_type": "user_localpart", + } + ], + actions=[ + "notify", + {"set_tweak": "sound", "value": "default"}, + {"set_tweak": "highlight"}, + ], ) ] @@ -205,12 +203,10 @@ def _is_experimental_rule_enabled( PushRule( default=True, priority_class=PRIORITY_CLASS_MAP["override"], - **{ - "rule_id": "global/override/.m.rule.master", - "enabled": False, - "conditions": [], - "actions": ["dont_notify"], - }, + rule_id="global/override/.m.rule.master", + default_enabled=False, + conditions=[], + actions=["dont_notify"], ) ] @@ -219,48 +215,44 @@ def _is_experimental_rule_enabled( PushRule( default=True, priority_class=PRIORITY_CLASS_MAP["override"], - **{ - "rule_id": "global/override/.m.rule.suppress_notices", - "conditions": [ - { - "kind": "event_match", - "key": "content.msgtype", - "pattern": "m.notice", - "_cache_key": "_suppress_notices", - } - ], - "actions": ["dont_notify"], - }, + rule_id="global/override/.m.rule.suppress_notices", + conditions=[ + { + "kind": "event_match", + "key": "content.msgtype", + "pattern": "m.notice", + "_cache_key": "_suppress_notices", + } + ], + actions=["dont_notify"], ), # NB. .m.rule.invite_for_me must be higher prio than .m.rule.member_event # otherwise invites will be matched by .m.rule.member_event PushRule( default=True, priority_class=PRIORITY_CLASS_MAP["override"], - **{ - "rule_id": "global/override/.m.rule.invite_for_me", - "conditions": [ - { - "kind": "event_match", - "key": "type", - "pattern": "m.room.member", - "_cache_key": "_member", - }, - { - "kind": "event_match", - "key": "content.membership", - "pattern": "invite", - "_cache_key": "_invite_member", - }, - # Match the requester's MXID. - {"kind": "event_match", "key": "state_key", "pattern_type": "user_id"}, - ], - "actions": [ - "notify", - {"set_tweak": "sound", "value": "default"}, - {"set_tweak": "highlight", "value": False}, - ], - }, + rule_id="global/override/.m.rule.invite_for_me", + conditions=[ + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.member", + "_cache_key": "_member", + }, + { + "kind": "event_match", + "key": "content.membership", + "pattern": "invite", + "_cache_key": "_invite_member", + }, + # Match the requester's MXID. + {"kind": "event_match", "key": "state_key", "pattern_type": "user_id"}, + ], + actions=[ + "notify", + {"set_tweak": "sound", "value": "default"}, + {"set_tweak": "highlight", "value": False}, + ], ), # Will we sometimes want to know about people joining and leaving? # Perhaps: if so, this could be expanded upon. Seems the most usual case @@ -271,18 +263,16 @@ def _is_experimental_rule_enabled( PushRule( default=True, priority_class=PRIORITY_CLASS_MAP["override"], - **{ - "rule_id": "global/override/.m.rule.member_event", - "conditions": [ - { - "kind": "event_match", - "key": "type", - "pattern": "m.room.member", - "_cache_key": "_member", - } - ], - "actions": ["dont_notify"], - }, + rule_id="global/override/.m.rule.member_event", + conditions=[ + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.member", + "_cache_key": "_member", + } + ], + actions=["dont_notify"], ), # This was changed from underride to override so it's closer in priority # to the content rules where the user name highlight rule lives. This @@ -291,74 +281,66 @@ def _is_experimental_rule_enabled( PushRule( default=True, priority_class=PRIORITY_CLASS_MAP["override"], - **{ - "rule_id": "global/override/.m.rule.contains_display_name", - "conditions": [{"kind": "contains_display_name"}], - "actions": [ - "notify", - {"set_tweak": "sound", "value": "default"}, - {"set_tweak": "highlight"}, - ], - }, + rule_id="global/override/.m.rule.contains_display_name", + conditions=[{"kind": "contains_display_name"}], + actions=[ + "notify", + {"set_tweak": "sound", "value": "default"}, + {"set_tweak": "highlight"}, + ], ), PushRule( default=True, priority_class=PRIORITY_CLASS_MAP["override"], - **{ - "rule_id": "global/override/.m.rule.roomnotif", - "conditions": [ - { - "kind": "event_match", - "key": "content.body", - "pattern": "@room", - "_cache_key": "_roomnotif_content", - }, - { - "kind": "sender_notification_permission", - "key": "room", - "_cache_key": "_roomnotif_pl", - }, - ], - "actions": ["notify", {"set_tweak": "highlight", "value": True}], - }, + rule_id="global/override/.m.rule.roomnotif", + conditions=[ + { + "kind": "event_match", + "key": "content.body", + "pattern": "@room", + "_cache_key": "_roomnotif_content", + }, + { + "kind": "sender_notification_permission", + "key": "room", + "_cache_key": "_roomnotif_pl", + }, + ], + actions=["notify", {"set_tweak": "highlight", "value": True}], ), PushRule( default=True, priority_class=PRIORITY_CLASS_MAP["override"], - **{ - "rule_id": "global/override/.m.rule.tombstone", - "conditions": [ - { - "kind": "event_match", - "key": "type", - "pattern": "m.room.tombstone", - "_cache_key": "_tombstone", - }, - { - "kind": "event_match", - "key": "state_key", - "pattern": "", - "_cache_key": "_tombstone_statekey", - }, - ], - "actions": ["notify", {"set_tweak": "highlight", "value": True}], - }, + rule_id="global/override/.m.rule.tombstone", + conditions=[ + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.tombstone", + "_cache_key": "_tombstone", + }, + { + "kind": "event_match", + "key": "state_key", + "pattern": "", + "_cache_key": "_tombstone_statekey", + }, + ], + actions=["notify", {"set_tweak": "highlight", "value": True}], ), PushRule( default=True, priority_class=PRIORITY_CLASS_MAP["override"], - **{ - "rule_id": "global/override/.m.rule.reaction", - "conditions": [ - { - "kind": "event_match", - "key": "type", - "pattern": "m.reaction", - "_cache_key": "_reaction", - } - ], - "actions": ["dont_notify"], - }, + rule_id="global/override/.m.rule.reaction", + conditions=[ + { + "kind": "event_match", + "key": "type", + "pattern": "m.reaction", + "_cache_key": "_reaction", + } + ], + actions=["dont_notify"], ), # XXX: This is an experimental rule that is only enabled if msc3786_enabled # is enabled, if it is not the rule gets filtered out in _load_rules() in @@ -366,24 +348,22 @@ def _is_experimental_rule_enabled( PushRule( default=True, priority_class=PRIORITY_CLASS_MAP["override"], - **{ - "rule_id": "global/override/.org.matrix.msc3786.rule.room.server_acl", - "conditions": [ - { - "kind": "event_match", - "key": "type", - "pattern": "m.room.server_acl", - "_cache_key": "_room_server_acl", - }, - { - "kind": "event_match", - "key": "state_key", - "pattern": "", - "_cache_key": "_room_server_acl_state_key", - }, - ], - "actions": [], - }, + rule_id="global/override/.org.matrix.msc3786.rule.room.server_acl", + conditions=[ + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.server_acl", + "_cache_key": "_room_server_acl", + }, + { + "kind": "event_match", + "key": "state_key", + "pattern": "", + "_cache_key": "_room_server_acl_state_key", + }, + ], + actions=[], ), ] @@ -392,146 +372,132 @@ def _is_experimental_rule_enabled( PushRule( default=True, priority_class=PRIORITY_CLASS_MAP["underride"], - **{ - "rule_id": "global/underride/.m.rule.call", - "conditions": [ - { - "kind": "event_match", - "key": "type", - "pattern": "m.call.invite", - "_cache_key": "_call", - } - ], - "actions": [ - "notify", - {"set_tweak": "sound", "value": "ring"}, - {"set_tweak": "highlight", "value": False}, - ], - }, + rule_id="global/underride/.m.rule.call", + conditions=[ + { + "kind": "event_match", + "key": "type", + "pattern": "m.call.invite", + "_cache_key": "_call", + } + ], + actions=[ + "notify", + {"set_tweak": "sound", "value": "ring"}, + {"set_tweak": "highlight", "value": False}, + ], ), # XXX: once m.direct is standardised everywhere, we should use it to detect # a DM from the user's perspective rather than this heuristic. PushRule( default=True, priority_class=PRIORITY_CLASS_MAP["underride"], - **{ - "rule_id": "global/underride/.m.rule.room_one_to_one", - "conditions": [ - {"kind": "room_member_count", "is": "2", "_cache_key": "member_count"}, - { - "kind": "event_match", - "key": "type", - "pattern": "m.room.message", - "_cache_key": "_message", - }, - ], - "actions": [ - "notify", - {"set_tweak": "sound", "value": "default"}, - {"set_tweak": "highlight", "value": False}, - ], - }, + rule_id="global/underride/.m.rule.room_one_to_one", + conditions=[ + {"kind": "room_member_count", "is": "2", "_cache_key": "member_count"}, + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.message", + "_cache_key": "_message", + }, + ], + actions=[ + "notify", + {"set_tweak": "sound", "value": "default"}, + {"set_tweak": "highlight", "value": False}, + ], ), # XXX: this is going to fire for events which aren't m.room.messages # but are encrypted (e.g. m.call.*)... PushRule( default=True, priority_class=PRIORITY_CLASS_MAP["underride"], - **{ - "rule_id": "global/underride/.m.rule.encrypted_room_one_to_one", - "conditions": [ - {"kind": "room_member_count", "is": "2", "_cache_key": "member_count"}, - { - "kind": "event_match", - "key": "type", - "pattern": "m.room.encrypted", - "_cache_key": "_encrypted", - }, - ], - "actions": [ - "notify", - {"set_tweak": "sound", "value": "default"}, - {"set_tweak": "highlight", "value": False}, - ], - }, + rule_id="global/underride/.m.rule.encrypted_room_one_to_one", + conditions=[ + {"kind": "room_member_count", "is": "2", "_cache_key": "member_count"}, + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.encrypted", + "_cache_key": "_encrypted", + }, + ], + actions=[ + "notify", + {"set_tweak": "sound", "value": "default"}, + {"set_tweak": "highlight", "value": False}, + ], ), PushRule( default=True, priority_class=PRIORITY_CLASS_MAP["underride"], - **{ - "rule_id": "global/underride/.org.matrix.msc3772.thread_reply", - "conditions": [ - { - "kind": "org.matrix.msc3772.relation_match", - "rel_type": "m.thread", - # Match the requester's MXID. - "sender_type": "user_id", - } - ], - "actions": ["notify", {"set_tweak": "highlight", "value": False}], - }, + rule_id="global/underride/.org.matrix.msc3772.thread_reply", + conditions=[ + { + "kind": "org.matrix.msc3772.relation_match", + "rel_type": "m.thread", + # Match the requester's MXID. + "sender_type": "user_id", + } + ], + actions=["notify", {"set_tweak": "highlight", "value": False}], ), PushRule( default=True, priority_class=PRIORITY_CLASS_MAP["underride"], - **{ - "rule_id": "global/underride/.m.rule.message", - "conditions": [ - { - "kind": "event_match", - "key": "type", - "pattern": "m.room.message", - "_cache_key": "_message", - } - ], - "actions": ["notify", {"set_tweak": "highlight", "value": False}], - }, + rule_id="global/underride/.m.rule.message", + conditions=[ + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.message", + "_cache_key": "_message", + } + ], + actions=["notify", {"set_tweak": "highlight", "value": False}], ), # XXX: this is going to fire for events which aren't m.room.messages # but are encrypted (e.g. m.call.*)... PushRule( default=True, priority_class=PRIORITY_CLASS_MAP["underride"], - **{ - "rule_id": "global/underride/.m.rule.encrypted", - "conditions": [ - { - "kind": "event_match", - "key": "type", - "pattern": "m.room.encrypted", - "_cache_key": "_encrypted", - } - ], - "actions": ["notify", {"set_tweak": "highlight", "value": False}], - }, + rule_id="global/underride/.m.rule.encrypted", + conditions=[ + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.encrypted", + "_cache_key": "_encrypted", + } + ], + actions=["notify", {"set_tweak": "highlight", "value": False}], ), PushRule( default=True, priority_class=PRIORITY_CLASS_MAP["underride"], - **{ - "rule_id": "global/underride/.im.vector.jitsi", - "conditions": [ - { - "kind": "event_match", - "key": "type", - "pattern": "im.vector.modular.widgets", - "_cache_key": "_type_modular_widgets", - }, - { - "kind": "event_match", - "key": "content.type", - "pattern": "jitsi", - "_cache_key": "_content_type_jitsi", - }, - { - "kind": "event_match", - "key": "state_key", - "pattern": "*", - "_cache_key": "_is_state_event", - }, - ], - "actions": ["notify", {"set_tweak": "highlight", "value": False}], - }, + rule_id="global/underride/.im.vector.jitsi", + conditions=[ + { + "kind": "event_match", + "key": "type", + "pattern": "im.vector.modular.widgets", + "_cache_key": "_type_modular_widgets", + }, + { + "kind": "event_match", + "key": "content.type", + "pattern": "jitsi", + "_cache_key": "_content_type_jitsi", + }, + { + "kind": "event_match", + "key": "state_key", + "pattern": "*", + "_cache_key": "_is_state_event", + }, + ], + actions=["notify", {"set_tweak": "highlight", "value": False}], ), ] From 992467180626fbca49554afcec8fb99dd9a94fe2 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 12 Aug 2022 19:22:39 +0100 Subject: [PATCH 4/8] Newsfile --- changelog.d/13522.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/13522.misc diff --git a/changelog.d/13522.misc b/changelog.d/13522.misc new file mode 100644 index 000000000000..0a8827205d7b --- /dev/null +++ b/changelog.d/13522.misc @@ -0,0 +1 @@ +Improve performance of sending messages in rooms with thousands of local users. From 97a3d1835e1b3787ce1e53c3c4bc07bfc75e0380 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 15 Aug 2022 10:23:53 +0100 Subject: [PATCH 5/8] Comments --- synapse/push/baserules.py | 46 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/synapse/push/baserules.py b/synapse/push/baserules.py index 70016ea2d960..bd443e5eb6d8 100644 --- a/synapse/push/baserules.py +++ b/synapse/push/baserules.py @@ -14,6 +14,40 @@ # See the License for the specific language governing permissions and # limitations under the License. +""" +Push rules is the system used to determine which events trigger a push (and a +bump in notification counts). + +This consists of a list of "push rules" for each user, where a push rule is a +pair of "conditions" and "actions". When a user receives an event Synapse +iterates over the list of push rules until it finds one where all the conditions +match the event, at which point "actions" describe the outcome (e.g. notify, +highlight, etc). + +Push rules are split up into 5 different "kinds" (aka "priority classes"), which +are run in order: + 1. Override — highest priority rules, e.g. always ignore notices + 2. Content — content specific rules, e.g. @ notifications + 3. Room — per room rules, e.g. enable/disable notifications for all messages + in a room + 4. Sender — per sender rules, e.g. never notify for messages from a given + user + 5. Underride — the lowest priority "default" rules, e.g. notify for every + message. + +The set of "base rules" are the list of rules that every user has by default. A +user can modify their copy of the push rules in one of three ways: + + 1. Adding a new push rule of a certain kind + 2. Changing the actions of a base rule + 3. Enabling/disabling a base rule. + +The base rules are split into whether they come before or after a particular +kind, so the order of push rule evaluation would be: base rules for before +"override" kind, user defined "override" rules, base rules after "override" +kind, etc, etc. +""" + import itertools from typing import Dict, Iterator, List, Mapping, Sequence, Tuple, Union @@ -25,6 +59,18 @@ @attr.s(auto_attribs=True, slots=True, frozen=True) class PushRule: + """A push rule + + Attributes: + rule_id: a unique ID for this rule + priority_class: what "kind" of push rule this is (see + `PRIORITY_CLASS_MAP` for mapping between int and kind) + conditions: the sequence of conditions that all need to match + actions: the actions to apply if all conditions are met + default: is this a base rule? + default_enabled: is this enabled by default? + """ + rule_id: str priority_class: int conditions: Sequence[Mapping[str, str]] From 067f36c894022bdc41b29edc9776ac428acb9fd2 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 16 Aug 2022 08:56:24 +0100 Subject: [PATCH 6/8] Apply suggestions from code review Co-authored-by: Patrick Cloke --- synapse/push/baserules.py | 2 +- synapse/storage/databases/main/push_rule.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/push/baserules.py b/synapse/push/baserules.py index bd443e5eb6d8..1f25c3bda844 100644 --- a/synapse/push/baserules.py +++ b/synapse/push/baserules.py @@ -207,7 +207,7 @@ def compile_push_rules(rawrules: List[PushRule]) -> PushRules: def _is_experimental_rule_enabled( rule_id: str, experimental_config: ExperimentalConfig ) -> bool: - """Used by `_load_rules` to filter out experimental rules when they + """Used by `FilteredPushRules` to filter out experimental rules when they have not been enabled. """ if ( diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 66964c5851eb..796076d45792 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -824,7 +824,7 @@ async def copy_push_rule_from_room_to_room( new_condition = dict(condition) new_condition["pattern"] = new_room_id - new_conditions.append(condition) + new_conditions.append(new_condition) # Add the rule for the new room await self.add_push_rule( From 386ffe54df6f0e6a2a64a40d62fcb3d952b1d497 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 16 Aug 2022 09:17:11 +0100 Subject: [PATCH 7/8] Comments --- synapse/push/baserules.py | 3 +++ synapse/push/clientformat.py | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/synapse/push/baserules.py b/synapse/push/baserules.py index 1f25c3bda844..c3e072033ce1 100644 --- a/synapse/push/baserules.py +++ b/synapse/push/baserules.py @@ -123,6 +123,9 @@ def __iter__(self) -> Iterator[PushRule]: yield rule def __len__(self) -> int: + # The length is mostly used by caches to get a sense of "size" / amount + # of memory this object is using, so we only count the number of custom + # rules. return ( len(self.overriden_base_rules) + len(self.override) diff --git a/synapse/push/clientformat.py b/synapse/push/clientformat.py index 67b8efe00055..73618d9234bf 100644 --- a/synapse/push/clientformat.py +++ b/synapse/push/clientformat.py @@ -48,6 +48,11 @@ def format_push_rules_for_user( template_rule["enabled"] = enabled if "conditions" not in template_rule: + # Not all formatted rules have explicit conditions, e.g. "room" + # rules omit them as they can be derived from the kind and rule ID. + # + # If the formatted rule has no conditions then we can skip the + # formatting of conditions. continue # Remove internal stuff. From 43e58e7499c4244d51727bb42931b46c2407fd62 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 16 Aug 2022 09:19:20 +0100 Subject: [PATCH 8/8] List comprehension --- synapse/storage/databases/main/push_rule.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 796076d45792..255620f9968a 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -71,16 +71,15 @@ def _load_rules( `FilteredPushRules` object. """ - ruleslist = [] - for rawrule in rawrules: - ruleslist.append( - PushRule( - rule_id=rawrule["rule_id"], - priority_class=rawrule["priority_class"], - conditions=db_to_json(rawrule["conditions"]), - actions=db_to_json(rawrule["actions"]), - ) + ruleslist = [ + PushRule( + rule_id=rawrule["rule_id"], + priority_class=rawrule["priority_class"], + conditions=db_to_json(rawrule["conditions"]), + actions=db_to_json(rawrule["actions"]), ) + for rawrule in rawrules + ] push_rules = compile_push_rules(ruleslist)