Update intentional mentions (MSC3952) to depend on exact_event_match (

MSC3758). (#15037) This replaces the specific `is_room_mention` push rule condition used in MSC3952 with the generic `exact_event_match` push rule condition from MSC3758. No functionality changes due to this.
matrix-org · Feb 16, 2023 · 979f237 · 979f237
1 parent d1efc47
commit 979f237
Show file tree

Hide file tree

Showing 10 changed files with 26 additions and 59 deletions.
diff --git a/changelog.d/15037.misc b/changelog.d/15037.misc
@@ -0,0 +1 @@
+Update [MSC3952](https://github.com/matrix-org/matrix-spec-proposals/pull/3952) support based on changes to the MSC.
diff --git a/rust/benches/evaluator.rs b/rust/benches/evaluator.rs
@@ -45,7 +45,6 @@ fn bench_match_exact(b: &mut Bencher) {
         flattened_keys,
         false,
         BTreeSet::new(),
-        false,
         10,
         Some(0),
         Default::default(),
@@ -95,7 +94,6 @@ fn bench_match_word(b: &mut Bencher) {
         flattened_keys,
         false,
         BTreeSet::new(),
-        false,
         10,
         Some(0),
         Default::default(),
@@ -145,7 +143,6 @@ fn bench_match_word_miss(b: &mut Bencher) {
         flattened_keys,
         false,
         BTreeSet::new(),
-        false,
         10,
         Some(0),
         Default::default(),
@@ -195,7 +192,6 @@ fn bench_eval_message(b: &mut Bencher) {
         flattened_keys,
         false,
         BTreeSet::new(),
-        false,
         10,
         Some(0),
         Default::default(),

diff --git a/rust/src/push/base_rules.rs b/rust/src/push/base_rules.rs
@@ -21,13 +21,13 @@ use lazy_static::lazy_static;
 use serde_json::Value;
 
 use super::KnownCondition;
-use crate::push::Action;
 use crate::push::Condition;
 use crate::push::EventMatchCondition;
 use crate::push::PushRule;
 use crate::push::RelatedEventMatchCondition;
 use crate::push::SetTweak;
 use crate::push::TweakValue;
+use crate::push::{Action, ExactEventMatchCondition, SimpleJsonValue};
 
 const HIGHLIGHT_ACTION: Action = Action::SetTweak(SetTweak {
     set_tweak: Cow::Borrowed("highlight"),
@@ -168,7 +168,10 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[
         rule_id: Cow::Borrowed(".org.matrix.msc3952.is_room_mention"),
         priority_class: 5,
         conditions: Cow::Borrowed(&[
-            Condition::Known(KnownCondition::IsRoomMention),
+            Condition::Known(KnownCondition::ExactEventMatch(ExactEventMatchCondition {
+                key: Cow::Borrowed("content.org.matrix.msc3952.mentions.room"),
+                value: Cow::Borrowed(&SimpleJsonValue::Bool(true)),
+            })),
             Condition::Known(KnownCondition::SenderNotificationPermission {
                 key: Cow::Borrowed("room"),
             }),

diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs
@@ -73,8 +73,6 @@ pub struct PushRuleEvaluator {
     has_mentions: bool,
     /// The user mentions that were part of the message.
     user_mentions: BTreeSet<String>,
-    /// True if the message is a room message.
-    room_mention: bool,
 
     /// The number of users in the room.
     room_member_count: u64,
@@ -116,7 +114,6 @@ impl PushRuleEvaluator {
         flattened_keys: BTreeMap<String, JsonValue>,
         has_mentions: bool,
         user_mentions: BTreeSet<String>,
-        room_mention: bool,
         room_member_count: u64,
         sender_power_level: Option<i64>,
         notification_power_levels: BTreeMap<String, i64>,
@@ -137,7 +134,6 @@ impl PushRuleEvaluator {
             body,
             has_mentions,
             user_mentions,
-            room_mention,
             room_member_count,
             notification_power_levels,
             sender_power_level,
@@ -279,7 +275,6 @@ impl PushRuleEvaluator {
                     false
                 }
             }
-            KnownCondition::IsRoomMention => self.room_mention,
             KnownCondition::ContainsDisplayName => {
                 if let Some(dn) = display_name {
                     if !dn.is_empty() {
@@ -529,7 +524,6 @@ fn push_rule_evaluator() {
         flattened_keys,
         false,
         BTreeSet::new(),
-        false,
         10,
         Some(0),
         BTreeMap::new(),
@@ -562,7 +556,6 @@ fn test_requires_room_version_supports_condition() {
         flattened_keys,
         false,
         BTreeSet::new(),
-        false,
         10,
         Some(0),
         BTreeMap::new(),

diff --git a/rust/src/push/mod.rs b/rust/src/push/mod.rs
@@ -336,8 +336,6 @@ pub enum KnownCondition {
     ExactEventPropertyContains(ExactEventMatchCondition),
     #[serde(rename = "org.matrix.msc3952.is_user_mention")]
     IsUserMention,
-    #[serde(rename = "org.matrix.msc3952.is_room_mention")]
-    IsRoomMention,
     ContainsDisplayName,
     RoomMemberCount {
         #[serde(skip_serializing_if = "Option::is_none")]
@@ -667,17 +665,6 @@ fn test_deserialize_unstable_msc3952_user_condition() {
     ));
 }
 
-#[test]
-fn test_deserialize_unstable_msc3952_room_condition() {
-    let json = r#"{"kind":"org.matrix.msc3952.is_room_mention"}"#;
-
-    let condition: Condition = serde_json::from_str(json).unwrap();
-    assert!(matches!(
-        condition,
-        Condition::Known(KnownCondition::IsRoomMention)
-    ));
-}
-
 #[test]
 fn test_deserialize_custom_condition() {
     let json = r#"{"kind":"custom_tag"}"#;

diff --git a/stubs/synapse/synapse_rust/push.pyi b/stubs/synapse/synapse_rust/push.pyi
@@ -59,7 +59,6 @@ class PushRuleEvaluator:
         flattened_keys: Mapping[str, JsonValue],
         has_mentions: bool,
         user_mentions: Set[str],
-        room_mention: bool,
         room_member_count: int,
         sender_power_level: Optional[int],
         notification_power_levels: Mapping[str, int],

diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py
@@ -179,9 +179,10 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
             "msc3783_escape_event_match_key", False
         )
 
-        # MSC3952: Intentional mentions
-        self.msc3952_intentional_mentions = experimental.get(
-            "msc3952_intentional_mentions", False
+        # MSC3952: Intentional mentions, this depends on MSC3758.
+        self.msc3952_intentional_mentions = (
+            experimental.get("msc3952_intentional_mentions", False)
+            and self.msc3758_exact_event_match
         )
 
         # MSC3959: Do not generate notifications for edits.

diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py
@@ -400,7 +400,6 @@ async def _action_for_event_by_user(
         mentions = event.content.get(EventContentFields.MSC3952_MENTIONS)
         has_mentions = self._intentional_mentions_enabled and isinstance(mentions, dict)
         user_mentions: Set[str] = set()
-        room_mention = False
         if has_mentions:
             # mypy seems to have lost the type even though it must be a dict here.
             assert isinstance(mentions, dict)
@@ -410,8 +409,6 @@ async def _action_for_event_by_user(
                 user_mentions = set(
                     filter(lambda item: isinstance(item, str), user_mentions_raw)
                 )
-            # Room mention is only true if the value is exactly true.
-            room_mention = mentions.get("room") is True
 
         evaluator = PushRuleEvaluator(
             _flatten_dict(
@@ -420,7 +417,6 @@ async def _action_for_event_by_user(
             ),
             has_mentions,
             user_mentions,
-            room_mention,
             room_member_count,
             sender_power_level,
             notification_levels,

diff --git a/tests/push/test_bulk_push_rule_evaluator.py b/tests/push/test_bulk_push_rule_evaluator.py
@@ -227,7 +227,14 @@ def _create_and_process(
         )
         return len(result) > 0
 
-    @override_config({"experimental_features": {"msc3952_intentional_mentions": True}})
+    @override_config(
+        {
+            "experimental_features": {
+                "msc3758_exact_event_match": True,
+                "msc3952_intentional_mentions": True,
+            }
+        }
+    )
     def test_user_mentions(self) -> None:
         """Test the behavior of an event which includes invalid user mentions."""
         bulk_evaluator = BulkPushRuleEvaluator(self.hs)
@@ -323,7 +330,14 @@ def test_user_mentions(self) -> None:
             )
         )
 
-    @override_config({"experimental_features": {"msc3952_intentional_mentions": True}})
+    @override_config(
+        {
+            "experimental_features": {
+                "msc3758_exact_event_match": True,
+                "msc3952_intentional_mentions": True,
+            }
+        }
+    )
     def test_room_mentions(self) -> None:
         """Test the behavior of an event which includes invalid room mentions."""
         bulk_evaluator = BulkPushRuleEvaluator(self.hs)

diff --git a/tests/push/test_push_rule_evaluator.py b/tests/push/test_push_rule_evaluator.py
@@ -149,7 +149,6 @@ def _get_evaluator(
         *,
         has_mentions: bool = False,
         user_mentions: Optional[Set[str]] = None,
-        room_mention: bool = False,
         related_events: Optional[JsonDict] = None,
     ) -> PushRuleEvaluator:
         event = FrozenEvent(
@@ -170,7 +169,6 @@ def _get_evaluator(
             _flatten_dict(event),
             has_mentions,
             user_mentions or set(),
-            room_mention,
             room_member_count,
             sender_power_level,
             cast(Dict[str, int], power_levels.get("notifications", {})),
@@ -232,27 +230,6 @@ def test_user_mentions(self) -> None:
         # Note that invalid data is tested at tests.push.test_bulk_push_rule_evaluator.TestBulkPushRuleEvaluator.test_mentions
         # since the BulkPushRuleEvaluator is what handles data sanitisation.
 
-    def test_room_mentions(self) -> None:
-        """Check for room mentions."""
-        condition = {"kind": "org.matrix.msc3952.is_room_mention"}
-
-        # No room mention shouldn't match.
-        evaluator = self._get_evaluator({}, has_mentions=True)
-        self.assertFalse(evaluator.matches(condition, None, None))
-
-        # Room mention should match.
-        evaluator = self._get_evaluator({}, has_mentions=True, room_mention=True)
-        self.assertTrue(evaluator.matches(condition, None, None))
-
-        # A room mention and user mention is valid.
-        evaluator = self._get_evaluator(
-            {}, has_mentions=True, user_mentions={"@another:test"}, room_mention=True
-        )
-        self.assertTrue(evaluator.matches(condition, None, None))
-
-        # Note that invalid data is tested at tests.push.test_bulk_push_rule_evaluator.TestBulkPushRuleEvaluator.test_mentions
-        # since the BulkPushRuleEvaluator is what handles data sanitisation.
-
     def _assert_matches(
         self, condition: JsonDict, content: JsonMapping, msg: Optional[str] = None
     ) -> None:
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Update [MSC3952](https://github.com/matrix-org/matrix-spec-proposals/pull/3952) support based on changes to the MSC.