From 48d417000eb98862bd9d3f8991f215fcf12ff02b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 12 May 2022 08:25:22 -0400 Subject: [PATCH 01/16] Add the new default push rule. --- changelog.d/12740.feature | 1 + synapse/push/baserules.py | 14 ++++++++++++++ 2 files changed, 15 insertions(+) create mode 100644 changelog.d/12740.feature diff --git a/changelog.d/12740.feature b/changelog.d/12740.feature new file mode 100644 index 000000000000..e674c31ae8aa --- /dev/null +++ b/changelog.d/12740.feature @@ -0,0 +1 @@ +Experimental support for [MSC3772](https://github.com/matrix-org/matrix-spec-proposals/pull/3772): Push rule for mutually related events. diff --git a/synapse/push/baserules.py b/synapse/push/baserules.py index a17b35a605fb..4c7278b5a112 100644 --- a/synapse/push/baserules.py +++ b/synapse/push/baserules.py @@ -139,6 +139,7 @@ def make_base_prepend_rules( { "kind": "event_match", "key": "content.body", + # Match the localpart of the requester's MXID. "pattern_type": "user_localpart", } ], @@ -191,6 +192,7 @@ def make_base_prepend_rules( "pattern": "invite", "_cache_key": "_invite_member", }, + # Match the requester's MXID. {"kind": "event_match", "key": "state_key", "pattern_type": "user_id"}, ], "actions": [ @@ -350,6 +352,18 @@ def make_base_prepend_rules( {"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": [ From d21989048693a205eaa99765445940e6194e89e2 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 12 May 2022 08:35:46 -0400 Subject: [PATCH 02/16] Pass relations information to PushRuleEvaluatorForEvent. --- synapse/push/bulk_push_rule_evaluator.py | 4 ++- synapse/push/push_rule_evaluator.py | 4 ++- synapse/storage/databases/main/relations.py | 37 ++++++++++++++++++++- tests/push/test_push_rule_evaluator.py | 2 +- 4 files changed, 43 insertions(+), 4 deletions(-) diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index 4ac2c546bf2a..a59161ffd730 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -211,8 +211,10 @@ async def action_for_event_by_user( sender_power_level, ) = await self._get_power_levels_and_sender_level(event, context) + relations = await self.store.get_mutual_event_relations(event) + evaluator = PushRuleEvaluatorForEvent( - event, len(room_members), sender_power_level, power_levels + event, len(room_members), sender_power_level, power_levels, relations ) # If the event is not a state event check if any users ignore the sender. diff --git a/synapse/push/push_rule_evaluator.py b/synapse/push/push_rule_evaluator.py index 54db6b5612a3..f1db646dfd6f 100644 --- a/synapse/push/push_rule_evaluator.py +++ b/synapse/push/push_rule_evaluator.py @@ -15,7 +15,7 @@ import logging import re -from typing import Any, Dict, List, Mapping, Optional, Pattern, Tuple, Union +from typing import Any, Dict, List, Mapping, Optional, Pattern, Set, Tuple, Union from matrix_common.regex import glob_to_regex, to_word_pattern @@ -120,11 +120,13 @@ def __init__( room_member_count: int, sender_power_level: int, power_levels: Dict[str, Union[int, Dict[str, int]]], + relations: Set[Tuple[str, str, str]], ): self._event = event self._room_member_count = room_member_count self._sender_power_level = sender_power_level self._power_levels = power_levels + self._relations = relations # Maps strings of e.g. 'content.body' -> event["content"]["body"] self._value_cache = _flatten_dict(event) diff --git a/synapse/storage/databases/main/relations.py b/synapse/storage/databases/main/relations.py index 484976ca6b0b..7e327b2822d7 100644 --- a/synapse/storage/databases/main/relations.py +++ b/synapse/storage/databases/main/relations.py @@ -29,7 +29,7 @@ import attr from synapse.api.constants import RelationTypes -from synapse.events import EventBase +from synapse.events import EventBase, relation_from_event from synapse.storage._base import SQLBaseStore from synapse.storage.database import LoggingTransaction, make_in_list_sql_clause from synapse.storage.databases.main.stream import generate_pagination_where_clause @@ -765,6 +765,41 @@ def _get_if_user_has_annotated_event(txn: LoggingTransaction) -> bool: "get_if_user_has_annotated_event", _get_if_user_has_annotated_event ) + async def get_mutual_event_relations( + self, event: EventBase + ) -> Set[Tuple[str, str, str]]: + """ + Fetch event meta data for events which related to the same event as the given event. + + If the given event has no relation information, returns an empty set. + + Args: + event: The event to fetch relations for. + + Returns: + A set of tuples of: + The relation type + The sender + The event type + """ + relation = relation_from_event(event) + if not relation: + return set() + + sql = """ + SELECT relation_type, sender, type FROM event_relations + INNER JOIN events USING (event_id) + WHERE relates_to_id = ? + """ + + def _get_event_relations(txn: LoggingTransaction) -> Set[Tuple[str, str, str]]: + txn.execute(sql, (relation.parent_id,)) # type: ignore[union-attr] + return set(cast(List[Tuple[str, str, str]], txn.fetchall())) + + return await self.db_pool.runInteraction( + "get_event_relations", _get_event_relations + ) + class RelationsStore(RelationsWorkerStore): pass diff --git a/tests/push/test_push_rule_evaluator.py b/tests/push/test_push_rule_evaluator.py index 5dba1870762e..363e9fa2c4d9 100644 --- a/tests/push/test_push_rule_evaluator.py +++ b/tests/push/test_push_rule_evaluator.py @@ -42,7 +42,7 @@ def _get_evaluator(self, content: JsonDict) -> PushRuleEvaluatorForEvent: sender_power_level = 0 power_levels: Dict[str, Union[int, Dict[str, int]]] = {} return PushRuleEvaluatorForEvent( - event, room_member_count, sender_power_level, power_levels + event, room_member_count, sender_power_level, power_levels, set() ) def test_display_name(self) -> None: From 001982e8d4da779023e9240a7c581550b88bee0f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 12 May 2022 11:19:53 -0400 Subject: [PATCH 03/16] Support relation_match in push rules. --- synapse/push/clientformat.py | 4 ++ synapse/push/push_rule_evaluator.py | 39 ++++++++++++++++ tests/push/test_push_rule_evaluator.py | 63 ++++++++++++++++++++++++-- 3 files changed, 103 insertions(+), 3 deletions(-) diff --git a/synapse/push/clientformat.py b/synapse/push/clientformat.py index 63b22d50aea9..5117ef6854f1 100644 --- a/synapse/push/clientformat.py +++ b/synapse/push/clientformat.py @@ -48,6 +48,10 @@ def format_push_rules_for_user( elif pattern_type == "user_localpart": c["pattern"] = user.localpart + sender_type = c.pop("sender_type", None) + if sender_type == "user_id": + c["sender"] = user.to_string() + rulearray = rules["global"][template_name] template_rule = _rule_to_template(r) diff --git a/synapse/push/push_rule_evaluator.py b/synapse/push/push_rule_evaluator.py index f1db646dfd6f..18516fae7ec5 100644 --- a/synapse/push/push_rule_evaluator.py +++ b/synapse/push/push_rule_evaluator.py @@ -190,6 +190,8 @@ def matches( return _sender_notification_permission( self._event, condition, self._sender_power_level, self._power_levels ) + elif condition["kind"] == "org.matrix.msc3772.relation_match": + return self._relation_match(condition, user_id) else: return True @@ -258,6 +260,43 @@ 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: + """ + Check an "relation_match" push rule condition. + + Args: + condition: The "event_match" push rule condition to match. + user_id: The user's MXID. + + Returns: + True if the condition matches the event, False otherwise. + """ + rel_type_pattern = condition.get("rel_type") + sender_pattern = condition.get("sender") + if sender_pattern is None: + sender_type = condition.get("sender_type") + if sender_type == "user_id": + sender_pattern = user_id + type_pattern = condition.get("type") + + if not rel_type_pattern and not sender_pattern and not type_pattern: + logger.warning("relation_match condition with nothing to match") + return False + + # If any other relations matches, return True. + for relation in self._relations: + if rel_type_pattern and not _glob_matches(rel_type_pattern, relation[0]): + continue + if sender_pattern and not _glob_matches(sender_pattern, relation[1]): + continue + if type_pattern and not _glob_matches(type_pattern, relation[2]): + continue + # All values must have matched. + return True + + # No relations matched. + return False + # Caches (string, is_glob, word_boundary) -> regex for push. See _glob_matches regex_cache: LruCache[Tuple[str, bool, bool], Pattern] = LruCache( diff --git a/tests/push/test_push_rule_evaluator.py b/tests/push/test_push_rule_evaluator.py index 363e9fa2c4d9..8980836bd1a7 100644 --- a/tests/push/test_push_rule_evaluator.py +++ b/tests/push/test_push_rule_evaluator.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -from typing import Dict, Optional, Union +from typing import Dict, Optional, Set, Tuple, Union import frozendict @@ -26,7 +26,9 @@ class PushRuleEvaluatorTestCase(unittest.TestCase): - def _get_evaluator(self, content: JsonDict) -> PushRuleEvaluatorForEvent: + def _get_evaluator( + self, content: JsonDict, relations: Optional[Set[Tuple[str, str, str]]] = None + ) -> PushRuleEvaluatorForEvent: event = FrozenEvent( { "event_id": "$event_id", @@ -42,7 +44,11 @@ def _get_evaluator(self, content: JsonDict) -> PushRuleEvaluatorForEvent: sender_power_level = 0 power_levels: Dict[str, Union[int, Dict[str, int]]] = {} return PushRuleEvaluatorForEvent( - event, room_member_count, sender_power_level, power_levels, set() + event, + room_member_count, + sender_power_level, + power_levels, + relations or set(), ) def test_display_name(self) -> None: @@ -276,3 +282,54 @@ def test_tweaks_for_actions(self) -> None: push_rule_evaluator.tweaks_for_actions(actions), {"sound": "default", "highlight": True}, ) + + def test_relation_match(self) -> None: + """Test the relation_match push rule kind.""" + evaluator = self._get_evaluator( + {}, {("m.annotation", "@user:test", "m.reaction")} + ) + + # Check just relation type. + condition = { + "kind": "org.matrix.msc3772.relation_match", + "rel_type": "m.annotation", + } + self.assertTrue(evaluator.matches(condition, "@user:test", "foo")) + + # Check relation type and sender. + condition = { + "kind": "org.matrix.msc3772.relation_match", + "rel_type": "m.annotation", + "sender": "@user:test", + } + self.assertTrue(evaluator.matches(condition, "@user:test", "foo")) + condition = { + "kind": "org.matrix.msc3772.relation_match", + "rel_type": "m.annotation", + "sender": "@other:test", + } + self.assertFalse(evaluator.matches(condition, "@user:test", "foo")) + + # Check relation type and event type. + condition = { + "kind": "org.matrix.msc3772.relation_match", + "rel_type": "m.annotation", + "type": "m.reaction", + } + self.assertTrue(evaluator.matches(condition, "@user:test", "foo")) + + # Check just sender. + condition = { + "kind": "org.matrix.msc3772.relation_match", + "sender": "@user:test", + } + self.assertTrue(evaluator.matches(condition, "@user:test", "foo")) + condition = { + "kind": "org.matrix.msc3772.relation_match", + "sender": "@other:test", + } + self.assertFalse(evaluator.matches(condition, "@user:test", "foo")) + + # Check glob. + condition = {"kind": "org.matrix.msc3772.relation_match", "sender": "@*:test"} + self.assertTrue(evaluator.matches(condition, "@user:test", "foo")) From 50898372524023122c059f4b2f5b0a7c09833940 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 16 May 2022 08:21:11 -0400 Subject: [PATCH 04/16] Add an experimental configuration flag. --- synapse/config/experimental.py | 3 +++ synapse/push/bulk_push_rule_evaluator.py | 10 +++++++++- synapse/push/push_rule_evaluator.py | 7 ++++++- synapse/storage/databases/main/push_rule.py | 5 +++++ tests/push/test_push_rule_evaluator.py | 16 +++++++++++++++- 5 files changed, 38 insertions(+), 3 deletions(-) diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index b20d949689ec..cc417e2fbf9b 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -84,3 +84,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: # MSC3786 (Add a default push rule to ignore m.room.server_acl events) self.msc3786_enabled: bool = experimental.get("msc3786_enabled", False) + + # MSC3772: A push rule for mutual relations. + self.msc3772_enabled: bool = experimental.get("msc3772_enabled", False) diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index a59161ffd730..5122d7fe1061 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -120,6 +120,9 @@ def __init__(self, hs: "HomeServer"): resizable=False, ) + # Whether to support MSC3772 is supported. + self._relations_match_enabled = self.hs.config.experimental.msc3772_enabled + async def _get_rules_for_event( self, event: EventBase, context: EventContext ) -> Dict[str, List[Dict[str, Any]]]: @@ -214,7 +217,12 @@ async def action_for_event_by_user( relations = await self.store.get_mutual_event_relations(event) evaluator = PushRuleEvaluatorForEvent( - event, len(room_members), sender_power_level, power_levels, relations + event, + len(room_members), + sender_power_level, + power_levels, + relations, + self._relations_match_enabled, ) # If the event is not a state event check if any users ignore the sender. diff --git a/synapse/push/push_rule_evaluator.py b/synapse/push/push_rule_evaluator.py index 18516fae7ec5..182c4f6cf1f4 100644 --- a/synapse/push/push_rule_evaluator.py +++ b/synapse/push/push_rule_evaluator.py @@ -121,12 +121,14 @@ def __init__( sender_power_level: int, power_levels: Dict[str, Union[int, Dict[str, int]]], relations: Set[Tuple[str, str, str]], + relations_match_enabled: bool, ): self._event = event self._room_member_count = room_member_count self._sender_power_level = sender_power_level self._power_levels = power_levels self._relations = relations + self._relations_match_enabled = relations_match_enabled # Maps strings of e.g. 'content.body' -> event["content"]["body"] self._value_cache = _flatten_dict(event) @@ -190,7 +192,10 @@ def matches( return _sender_notification_permission( self._event, condition, self._sender_power_level, self._power_levels ) - elif condition["kind"] == "org.matrix.msc3772.relation_match": + elif ( + condition["kind"] == "org.matrix.msc3772.relation_match" + and self._relations_match_enabled + ): return self._relation_match(condition, user_id) else: return True diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 4ed913e24879..708cc84da279 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -54,6 +54,11 @@ def _is_experimental_rule_enabled( 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 diff --git a/tests/push/test_push_rule_evaluator.py b/tests/push/test_push_rule_evaluator.py index 8980836bd1a7..d25dc49a3b1a 100644 --- a/tests/push/test_push_rule_evaluator.py +++ b/tests/push/test_push_rule_evaluator.py @@ -27,7 +27,10 @@ class PushRuleEvaluatorTestCase(unittest.TestCase): def _get_evaluator( - self, content: JsonDict, relations: Optional[Set[Tuple[str, str, str]]] = None + self, + content: JsonDict, + relations: Optional[Set[Tuple[str, str, str]]] = None, + relations_match_enabled: bool = False, ) -> PushRuleEvaluatorForEvent: event = FrozenEvent( { @@ -49,6 +52,7 @@ def _get_evaluator( sender_power_level, power_levels, relations or set(), + relations_match_enabled, ) def test_display_name(self) -> None: @@ -285,9 +289,19 @@ def test_tweaks_for_actions(self) -> None: def test_relation_match(self) -> None: """Test the relation_match push rule kind.""" + + # Check if the experimental feature is disabled. evaluator = self._get_evaluator( {}, {("m.annotation", "@user:test", "m.reaction")} ) + condition = {"kind": "relation_match"} + # Oddly, an unknown condition always matches. + self.assertTrue(evaluator.matches(condition, "@user:test", "foo")) + + # A push rule evaluator with the experimental rule enabled. + evaluator = self._get_evaluator( + {}, {("m.annotation", "@user:test", "m.reaction")}, True + ) # Check just relation type. condition = { From 92855b37d799f90b7ba69df151d2060084847784 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 18 May 2022 10:28:05 -0400 Subject: [PATCH 05/16] Add DISTINCT clause. Co-authored-by: Erik Johnston --- synapse/storage/databases/main/relations.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/relations.py b/synapse/storage/databases/main/relations.py index 7e327b2822d7..4dcdc26804d8 100644 --- a/synapse/storage/databases/main/relations.py +++ b/synapse/storage/databases/main/relations.py @@ -787,7 +787,7 @@ async def get_mutual_event_relations( return set() sql = """ - SELECT relation_type, sender, type FROM event_relations + SELECT DISTINCT relation_type, sender, type FROM event_relations INNER JOIN events USING (event_id) WHERE relates_to_id = ? """ From 330a7355af088e88abb53e591d9968926be8037c Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 18 May 2022 10:44:06 -0400 Subject: [PATCH 06/16] Adds a XXX comment. --- synapse/push/push_rule_evaluator.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/synapse/push/push_rule_evaluator.py b/synapse/push/push_rule_evaluator.py index 182c4f6cf1f4..a3e157fffcae 100644 --- a/synapse/push/push_rule_evaluator.py +++ b/synapse/push/push_rule_evaluator.py @@ -198,6 +198,10 @@ def matches( ): return self._relation_match(condition, user_id) else: + # XXX This looks incorrect -- we have reached an unknown condition + # kind and are unconditionally returning that it matches. Note + # that it seems possible to provide a condition to the /pushrules + # endpoint with an unknown kind, see _rule_tuple_from_request_object. return True def _event_match(self, condition: dict, user_id: str) -> bool: From 149d20beebe1d584f1564ff5330953a69978925f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 18 May 2022 10:44:16 -0400 Subject: [PATCH 07/16] Only fetch relations if experimental feature is enabled. --- synapse/push/bulk_push_rule_evaluator.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index 5122d7fe1061..fd027d5292c3 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -214,7 +214,11 @@ async def action_for_event_by_user( sender_power_level, ) = await self._get_power_levels_and_sender_level(event, context) - relations = await self.store.get_mutual_event_relations(event) + # If the experimental feature is not enabled, skip fetching relations. + if self._relations_match_enabled: + relations = await self.store.get_mutual_event_relations(event) + else: + relations = set() evaluator = PushRuleEvaluatorForEvent( event, From 057139d22d27a3cf997f636a01f95014cf592c13 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 18 May 2022 10:56:44 -0400 Subject: [PATCH 08/16] Move relation parsing logic into caller. --- synapse/push/bulk_push_rule_evaluator.py | 8 +++++++- synapse/storage/databases/main/relations.py | 12 ++++-------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index fd027d5292c3..700c0c1b4e69 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -216,7 +216,13 @@ async def action_for_event_by_user( # If the experimental feature is not enabled, skip fetching relations. if self._relations_match_enabled: - relations = await self.store.get_mutual_event_relations(event) + relation = relation_from_event(event) + if relation: + relations = await self.store.get_mutual_event_relations( + relation.parent_id + ) + else: + relations = set() else: relations = set() diff --git a/synapse/storage/databases/main/relations.py b/synapse/storage/databases/main/relations.py index 4dcdc26804d8..5b10a1b6d724 100644 --- a/synapse/storage/databases/main/relations.py +++ b/synapse/storage/databases/main/relations.py @@ -29,7 +29,7 @@ import attr from synapse.api.constants import RelationTypes -from synapse.events import EventBase, relation_from_event +from synapse.events import EventBase from synapse.storage._base import SQLBaseStore from synapse.storage.database import LoggingTransaction, make_in_list_sql_clause from synapse.storage.databases.main.stream import generate_pagination_where_clause @@ -766,7 +766,7 @@ def _get_if_user_has_annotated_event(txn: LoggingTransaction) -> bool: ) async def get_mutual_event_relations( - self, event: EventBase + self, event_id: str ) -> Set[Tuple[str, str, str]]: """ Fetch event meta data for events which related to the same event as the given event. @@ -774,7 +774,7 @@ async def get_mutual_event_relations( If the given event has no relation information, returns an empty set. Args: - event: The event to fetch relations for. + event_id: The event ID which is targeted by relations. Returns: A set of tuples of: @@ -782,10 +782,6 @@ async def get_mutual_event_relations( The sender The event type """ - relation = relation_from_event(event) - if not relation: - return set() - sql = """ SELECT DISTINCT relation_type, sender, type FROM event_relations INNER JOIN events USING (event_id) @@ -793,7 +789,7 @@ async def get_mutual_event_relations( """ def _get_event_relations(txn: LoggingTransaction) -> Set[Tuple[str, str, str]]: - txn.execute(sql, (relation.parent_id,)) # type: ignore[union-attr] + txn.execute(sql, (event_id,)) return set(cast(List[Tuple[str, str, str]], txn.fetchall())) return await self.db_pool.runInteraction( From 33b90612a7580f8581c1977ca645fa7793ec3889 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 18 May 2022 11:01:25 -0400 Subject: [PATCH 09/16] Add a cache to get_mutual_event_relations. --- synapse/storage/databases/main/relations.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/storage/databases/main/relations.py b/synapse/storage/databases/main/relations.py index 5b10a1b6d724..71f6cf706ee3 100644 --- a/synapse/storage/databases/main/relations.py +++ b/synapse/storage/databases/main/relations.py @@ -765,6 +765,7 @@ def _get_if_user_has_annotated_event(txn: LoggingTransaction) -> bool: "get_if_user_has_annotated_event", _get_if_user_has_annotated_event ) + @cached(iterable=True) async def get_mutual_event_relations( self, event_id: str ) -> Set[Tuple[str, str, str]]: From 32de3c5960020e4818611722fc713a25e7c13220 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 18 May 2022 11:01:40 -0400 Subject: [PATCH 10/16] Fix typo. --- synapse/storage/databases/main/relations.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/relations.py b/synapse/storage/databases/main/relations.py index 71f6cf706ee3..b46abbb0e849 100644 --- a/synapse/storage/databases/main/relations.py +++ b/synapse/storage/databases/main/relations.py @@ -770,7 +770,7 @@ async def get_mutual_event_relations( self, event_id: str ) -> Set[Tuple[str, str, str]]: """ - Fetch event meta data for events which related to the same event as the given event. + Fetch event metadata for events which related to the same event as the given event. If the given event has no relation information, returns an empty set. From 3456524bbbdfc831c230618b54acf0d9ec8ccdf0 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 18 May 2022 15:42:05 -0400 Subject: [PATCH 11/16] Properly invalidate the cache. --- synapse/storage/databases/main/events.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index 42d484dc98d9..f7975f033515 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -1830,6 +1830,10 @@ def _handle_event_relations( self.store.get_aggregation_groups_for_event.invalidate, (relation.parent_id,), ) + txn.call_after( + self.store.get_mutual_event_relations.invalidate, + (relation.parent_id,), + ) if relation.rel_type == RelationTypes.REPLACE: txn.call_after( @@ -2006,6 +2010,9 @@ def _handle_redact_relations( self.store._invalidate_cache_and_stream( txn, self.store.get_thread_participated, (redacted_relates_to,) ) + self.store._invalidate_cache_and_stream( + txn, self.store.get_mutual_event_relations, (redacted_relates_to,) + ) self.db_pool.simple_delete_txn( txn, table="event_relations", keyvalues={"event_id": redacted_event_id} From 885121761e27125e78752981bec6cfda4630f0de Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 20 May 2022 10:19:16 -0400 Subject: [PATCH 12/16] Require the rel_type field. --- synapse/push/push_rule_evaluator.py | 12 ++++++------ tests/push/test_push_rule_evaluator.py | 19 +++++++++++++------ 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/synapse/push/push_rule_evaluator.py b/synapse/push/push_rule_evaluator.py index a3e157fffcae..75706154d67d 100644 --- a/synapse/push/push_rule_evaluator.py +++ b/synapse/push/push_rule_evaluator.py @@ -280,7 +280,11 @@ def _relation_match(self, condition: dict, user_id: str) -> bool: Returns: True if the condition matches the event, False otherwise. """ - rel_type_pattern = condition.get("rel_type") + rel_type = condition.get("rel_type") + if not rel_type: + logger.warning("relation_match condition missing rel_type") + return False + sender_pattern = condition.get("sender") if sender_pattern is None: sender_type = condition.get("sender_type") @@ -288,13 +292,9 @@ def _relation_match(self, condition: dict, user_id: str) -> bool: sender_pattern = user_id type_pattern = condition.get("type") - if not rel_type_pattern and not sender_pattern and not type_pattern: - logger.warning("relation_match condition with nothing to match") - return False - # If any other relations matches, return True. for relation in self._relations: - if rel_type_pattern and not _glob_matches(rel_type_pattern, relation[0]): + if rel_type != relation[0]: continue if sender_pattern and not _glob_matches(sender_pattern, relation[1]): continue diff --git a/tests/push/test_push_rule_evaluator.py b/tests/push/test_push_rule_evaluator.py index d25dc49a3b1a..bed82b9613ce 100644 --- a/tests/push/test_push_rule_evaluator.py +++ b/tests/push/test_push_rule_evaluator.py @@ -332,18 +332,25 @@ def test_relation_match(self) -> None: } self.assertTrue(evaluator.matches(condition, "@user:test", "foo")) - # Check just sender. + # Check just sender, this fails since rel_type is required. condition = { "kind": "org.matrix.msc3772.relation_match", "sender": "@user:test", } - self.assertTrue(evaluator.matches(condition, "@user:test", "foo")) + self.assertFalse(evaluator.matches(condition, "@user:test", "foo")) + + # Check sender glob. condition = { "kind": "org.matrix.msc3772.relation_match", - "sender": "@other:test", + "rel_type": "m.annotation", + "sender": "@*:test", } - self.assertFalse(evaluator.matches(condition, "@user:test", "foo")) + self.assertTrue(evaluator.matches(condition, "@user:test", "foo")) - # Check glob. - condition = {"kind": "org.matrix.msc3772.relation_match", "sender": "@*:test"} + # Check event type glob. + condition = { + "kind": "org.matrix.msc3772.relation_match", + "rel_type": "m.annotation", + "event_type": "*.reaction", + } self.assertTrue(evaluator.matches(condition, "@user:test", "foo")) From 820ca7fc2e6d797bf8f2b9c214dfe26d2219a7d0 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 20 May 2022 14:18:18 -0400 Subject: [PATCH 13/16] Rework to pull less information from the database. --- synapse/push/bulk_push_rule_evaluator.py | 32 ++++++++++++---- synapse/push/push_rule_evaluator.py | 10 ++--- synapse/storage/databases/main/events.py | 6 ++- synapse/storage/databases/main/relations.py | 41 +++++++++++++++------ tests/push/test_push_rule_evaluator.py | 6 +-- 5 files changed, 66 insertions(+), 29 deletions(-) diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index 700c0c1b4e69..96d93aa43894 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -13,6 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import itertools import logging from typing import TYPE_CHECKING, Any, Dict, List, Optional, Set, Tuple, Union @@ -215,16 +216,33 @@ async def action_for_event_by_user( ) = await self._get_power_levels_and_sender_level(event, context) # If the experimental feature is not enabled, skip fetching relations. + relations = {} if self._relations_match_enabled: + # If the event does not have a relation, then cannot have any mutual + # relations. relation = relation_from_event(event) if relation: - relations = await self.store.get_mutual_event_relations( - relation.parent_id - ) - else: - relations = set() - else: - relations = set() + # Pre-filter to figure out which relation types are interesting. + rel_types = set() + for rule in itertools.chain(*rules_by_user.values()): + # Skip disabled rules. + if not rule.get("enabled"): + continue + + for condition in rule["conditions"]: + if condition["kind"] != "org.matrix.msc3772.relation_match": + continue + + # rel_type is required. + rel_type = condition.get("rel_type") + if rel_type: + rel_types.add(rel_type) + + # If any valid rules were found, fetch the mutual relations. + if rel_types: + relations = await self.store.get_mutual_event_relations( + relation.parent_id, rel_types + ) evaluator = PushRuleEvaluatorForEvent( event, diff --git a/synapse/push/push_rule_evaluator.py b/synapse/push/push_rule_evaluator.py index 75706154d67d..2e8a017add34 100644 --- a/synapse/push/push_rule_evaluator.py +++ b/synapse/push/push_rule_evaluator.py @@ -120,7 +120,7 @@ def __init__( room_member_count: int, sender_power_level: int, power_levels: Dict[str, Union[int, Dict[str, int]]], - relations: Set[Tuple[str, str, str]], + relations: Dict[str, Set[Tuple[str, str]]], relations_match_enabled: bool, ): self._event = event @@ -293,12 +293,10 @@ def _relation_match(self, condition: dict, user_id: str) -> bool: type_pattern = condition.get("type") # If any other relations matches, return True. - for relation in self._relations: - if rel_type != relation[0]: + for sender, event_type in self._relations.get(rel_type, ()): + if sender_pattern and not _glob_matches(sender_pattern, sender): continue - if sender_pattern and not _glob_matches(sender_pattern, relation[1]): - continue - if type_pattern and not _glob_matches(type_pattern, relation[2]): + if type_pattern and not _glob_matches(type_pattern, event_type): continue # All values must have matched. return True diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index e2098767afd6..17e35cf63e68 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -1829,7 +1829,7 @@ def _handle_event_relations( (relation.parent_id,), ) txn.call_after( - self.store.get_mutual_event_relations.invalidate, + self.store.get_mutual_event_relations_for_rel_type.invalidate, (relation.parent_id,), ) @@ -2009,7 +2009,9 @@ def _handle_redact_relations( txn, self.store.get_thread_participated, (redacted_relates_to,) ) self.store._invalidate_cache_and_stream( - txn, self.store.get_mutual_event_relations, (redacted_relates_to,) + txn, + self.store.get_mutual_event_relations_for_rel_type, + (redacted_relates_to,), ) self.db_pool.simple_delete_txn( diff --git a/synapse/storage/databases/main/relations.py b/synapse/storage/databases/main/relations.py index cf194af75535..df6a0f484660 100644 --- a/synapse/storage/databases/main/relations.py +++ b/synapse/storage/databases/main/relations.py @@ -13,6 +13,7 @@ # limitations under the License. import logging +from collections import defaultdict from typing import ( Collection, Dict, @@ -768,9 +769,18 @@ def _get_if_user_has_annotated_event(txn: LoggingTransaction) -> bool: ) @cached(iterable=True) + async def get_mutual_event_relations_for_rel_type( + self, event_id: str, relation_type: str + ) -> Set[Tuple[str, str]]: + raise NotImplementedError() + + @cachedList( + cached_method_name="get_mutual_event_relations_for_rel_type", + list_name="relation_types", + ) async def get_mutual_event_relations( - self, event_id: str - ) -> Set[Tuple[str, str, str]]: + self, event_id: str, relation_types: Collection[str] + ) -> Dict[str, Set[Tuple[str, str]]]: """ Fetch event metadata for events which related to the same event as the given event. @@ -780,20 +790,29 @@ async def get_mutual_event_relations( event_id: The event ID which is targeted by relations. Returns: - A set of tuples of: - The relation type - The sender - The event type + A dictionary of relation type to: + A set of tuples of: + The sender + The event type """ - sql = """ + rel_type_sql, rel_type_args = make_in_list_sql_clause( + self.database_engine, "rel_type", relation_types + ) + + sql = f""" SELECT DISTINCT relation_type, sender, type FROM event_relations INNER JOIN events USING (event_id) - WHERE relates_to_id = ? + WHERE relates_to_id = ? AND {rel_type_sql} """ - def _get_event_relations(txn: LoggingTransaction) -> Set[Tuple[str, str, str]]: - txn.execute(sql, (event_id,)) - return set(cast(List[Tuple[str, str, str]], txn.fetchall())) + def _get_event_relations( + txn: LoggingTransaction, + ) -> Dict[str, Set[Tuple[str, str]]]: + txn.execute(sql, [event_id] + rel_type_args) + result = defaultdict(set) + for rel_type, sender, type in txn.fetchall(): + result[rel_type].add((sender, type)) + return result return await self.db_pool.runInteraction( "get_event_relations", _get_event_relations diff --git a/tests/push/test_push_rule_evaluator.py b/tests/push/test_push_rule_evaluator.py index bed82b9613ce..9b623d0033cd 100644 --- a/tests/push/test_push_rule_evaluator.py +++ b/tests/push/test_push_rule_evaluator.py @@ -29,7 +29,7 @@ class PushRuleEvaluatorTestCase(unittest.TestCase): def _get_evaluator( self, content: JsonDict, - relations: Optional[Set[Tuple[str, str, str]]] = None, + relations: Optional[Dict[str, Set[Tuple[str, str]]]] = None, relations_match_enabled: bool = False, ) -> PushRuleEvaluatorForEvent: event = FrozenEvent( @@ -292,7 +292,7 @@ def test_relation_match(self) -> None: # Check if the experimental feature is disabled. evaluator = self._get_evaluator( - {}, {("m.annotation", "@user:test", "m.reaction")} + {}, {"m.annotation": {("@user:test", "m.reaction")}} ) condition = {"kind": "relation_match"} # Oddly, an unknown condition always matches. @@ -300,7 +300,7 @@ def test_relation_match(self) -> None: # A push rule evaluator with the experimental rule enabled. evaluator = self._get_evaluator( - {}, {("m.annotation", "@user:test", "m.reaction")}, True + {}, {"m.annotation": {("@user:test", "m.reaction")}}, True ) # Check just relation type. From 1e9fc9b6dfd7a250ac22f190184c9d2ff3d6ccf7 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 23 May 2022 12:49:48 -0400 Subject: [PATCH 14/16] Fix bugs. --- synapse/push/bulk_push_rule_evaluator.py | 2 +- synapse/storage/databases/main/relations.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index 96d93aa43894..546cd3c5bead 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -226,7 +226,7 @@ async def action_for_event_by_user( rel_types = set() for rule in itertools.chain(*rules_by_user.values()): # Skip disabled rules. - if not rule.get("enabled"): + if "enabled" in rule and not rule["enabled"]: continue for condition in rule["conditions"]: diff --git a/synapse/storage/databases/main/relations.py b/synapse/storage/databases/main/relations.py index df6a0f484660..c8570558fdc6 100644 --- a/synapse/storage/databases/main/relations.py +++ b/synapse/storage/databases/main/relations.py @@ -796,7 +796,7 @@ async def get_mutual_event_relations( The event type """ rel_type_sql, rel_type_args = make_in_list_sql_clause( - self.database_engine, "rel_type", relation_types + self.database_engine, "relation_type", relation_types ) sql = f""" From 69d7daadee3a43ba73c2f1d32fa4f0ca54949ced Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 24 May 2022 08:58:31 -0400 Subject: [PATCH 15/16] Fixup comments. --- synapse/storage/databases/main/relations.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/relations.py b/synapse/storage/databases/main/relations.py index c8570558fdc6..3b1b2ce6cb8d 100644 --- a/synapse/storage/databases/main/relations.py +++ b/synapse/storage/databases/main/relations.py @@ -784,10 +784,11 @@ async def get_mutual_event_relations( """ Fetch event metadata for events which related to the same event as the given event. - If the given event has no relation information, returns an empty set. + If the given event has no relation information, returns an empty dictionary. Args: event_id: The event ID which is targeted by relations. + relation_types: The relation types to check for mutual relations. Returns: A dictionary of relation type to: From 5a7566aab3c77f95e67d483af1a446b7642442d5 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 24 May 2022 08:58:44 -0400 Subject: [PATCH 16/16] Move relation calculation out to a separate method. --- synapse/push/bulk_push_rule_evaluator.py | 87 ++++++++++++++++-------- 1 file changed, 58 insertions(+), 29 deletions(-) diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index 2a08f0d39a3d..1a8e7ef3dc46 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -15,7 +15,7 @@ import itertools import logging -from typing import TYPE_CHECKING, Any, Dict, List, Optional, Set, Tuple, Union +from typing import TYPE_CHECKING, Any, Dict, Iterable, List, Optional, Set, Tuple, Union import attr from prometheus_client import Counter @@ -196,6 +196,60 @@ 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]] + ) -> Dict[str, Set[Tuple[str, str]]]: + """ + Fetch event metadata for events which related to the same event as the given event. + + If the given event has no relation information, returns an empty dictionary. + + Args: + event_id: The event ID which is targeted by relations. + rules: The push rules which will be processed for this event. + + Returns: + A dictionary of relation type to: + A set of tuples of: + The sender + The event type + """ + + # If the experimental feature is not enabled, skip fetching relations. + if not self._relations_match_enabled: + return {} + + # If the event does not have a relation, then cannot have any mutual + # relations. + relation = relation_from_event(event) + if not relation: + return {} + + # 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"]: + continue + + for condition in rule["conditions"]: + if condition["kind"] != "org.matrix.msc3772.relation_match": + continue + + # rel_type is required. + rel_type = condition.get("rel_type") + if rel_type: + rel_types.add(rel_type) + + # If no valid rules were found, no mutual relations. + if not rel_types: + return {} + + # If any valid rules were found, fetch the mutual relations. + return await self.store.get_mutual_event_relations( + relation.parent_id, rel_types + ) + @measure_func("action_for_event_by_user") async def action_for_event_by_user( self, event: EventBase, context: EventContext @@ -220,34 +274,9 @@ async def action_for_event_by_user( sender_power_level, ) = await self._get_power_levels_and_sender_level(event, context) - # If the experimental feature is not enabled, skip fetching relations. - relations = {} - if self._relations_match_enabled: - # If the event does not have a relation, then cannot have any mutual - # relations. - relation = relation_from_event(event) - if relation: - # Pre-filter to figure out which relation types are interesting. - rel_types = set() - for rule in itertools.chain(*rules_by_user.values()): - # Skip disabled rules. - if "enabled" in rule and not rule["enabled"]: - continue - - for condition in rule["conditions"]: - if condition["kind"] != "org.matrix.msc3772.relation_match": - continue - - # rel_type is required. - rel_type = condition.get("rel_type") - if rel_type: - rel_types.add(rel_type) - - # If any valid rules were found, fetch the mutual relations. - if rel_types: - relations = await self.store.get_mutual_event_relations( - relation.parent_id, rel_types - ) + relations = await self._get_mutual_relations( + event, itertools.chain(*rules_by_user.values()) + ) evaluator = PushRuleEvaluatorForEvent( event,