Update MSC3958 support to interact with intentional mentions. (#15992)

* Updates the rule ID.
* Use `event_property_is` instead of `event_match`.

This updates the implementation of MSC3958 to match the latest
text from the MSC.

changelog.d/15992.misc

@@ -0,0 +1 @@
+Update support for [MSC3958](https://github.com/matrix-org/matrix-spec-proposals/pull/3958) to match the latest revision of the MSC.

rust/benches/evaluator.rs

@@ -13,6 +13,9 @@
 // limitations under the License.
 
 #![feature(test)]
+
+use std::borrow::Cow;
+
 use synapse::push::{
     evaluator::PushRuleEvaluator, Condition, EventMatchCondition, FilteredPushRules, JsonValue,
     PushRules, SimpleJsonValue,
@@ -26,15 +29,15 @@ fn bench_match_exact(b: &mut Bencher) {
     let flattened_keys = [
         (
             "type".to_string(),
-            JsonValue::Value(SimpleJsonValue::Str("m.text".to_string())),
+            JsonValue::Value(SimpleJsonValue::Str(Cow::Borrowed("m.text"))),
         ),
         (
             "room_id".to_string(),
-            JsonValue::Value(SimpleJsonValue::Str("!room:server".to_string())),
+            JsonValue::Value(SimpleJsonValue::Str(Cow::Borrowed("!room:server"))),
         ),
         (
             "content.body".to_string(),
-            JsonValue::Value(SimpleJsonValue::Str("test message".to_string())),
+            JsonValue::Value(SimpleJsonValue::Str(Cow::Borrowed("test message"))),
         ),
     ]
     .into_iter()
@@ -71,15 +74,15 @@ fn bench_match_word(b: &mut Bencher) {
     let flattened_keys = [
         (
             "type".to_string(),
-            JsonValue::Value(SimpleJsonValue::Str("m.text".to_string())),
+            JsonValue::Value(SimpleJsonValue::Str(Cow::Borrowed("m.text"))),
         ),
         (
             "room_id".to_string(),
-            JsonValue::Value(SimpleJsonValue::Str("!room:server".to_string())),
+            JsonValue::Value(SimpleJsonValue::Str(Cow::Borrowed("!room:server"))),
         ),
         (
             "content.body".to_string(),
-            JsonValue::Value(SimpleJsonValue::Str("test message".to_string())),
+            JsonValue::Value(SimpleJsonValue::Str(Cow::Borrowed("test message"))),
         ),
     ]
     .into_iter()
@@ -116,15 +119,15 @@ fn bench_match_word_miss(b: &mut Bencher) {
     let flattened_keys = [
         (
             "type".to_string(),
-            JsonValue::Value(SimpleJsonValue::Str("m.text".to_string())),
+            JsonValue::Value(SimpleJsonValue::Str(Cow::Borrowed("m.text"))),
         ),
         (
             "room_id".to_string(),
-            JsonValue::Value(SimpleJsonValue::Str("!room:server".to_string())),
+            JsonValue::Value(SimpleJsonValue::Str(Cow::Borrowed("!room:server"))),
         ),
         (
             "content.body".to_string(),
-            JsonValue::Value(SimpleJsonValue::Str("test message".to_string())),
+            JsonValue::Value(SimpleJsonValue::Str(Cow::Borrowed("test message"))),
         ),
     ]
     .into_iter()
@@ -161,15 +164,15 @@ fn bench_eval_message(b: &mut Bencher) {
     let flattened_keys = [
         (
             "type".to_string(),
-            JsonValue::Value(SimpleJsonValue::Str("m.text".to_string())),
+            JsonValue::Value(SimpleJsonValue::Str(Cow::Borrowed("m.text"))),
         ),
         (
             "room_id".to_string(),
-            JsonValue::Value(SimpleJsonValue::Str("!room:server".to_string())),
+            JsonValue::Value(SimpleJsonValue::Str(Cow::Borrowed("!room:server"))),
         ),
         (
             "content.body".to_string(),
-            JsonValue::Value(SimpleJsonValue::Str("test message".to_string())),
+            JsonValue::Value(SimpleJsonValue::Str(Cow::Borrowed("test message"))),
         ),
     ]
     .into_iter()

rust/src/push/base_rules.rs

@@ -63,22 +63,6 @@ pub const BASE_PREPEND_OVERRIDE_RULES: &[PushRule] = &[PushRule {
 }];
 
 pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[
-    // We don't want to notify on edits. Not only can this be confusing in real
-    // time (2 notifications, one message) but it's especially confusing
-    // if a bridge needs to edit a previously backfilled message.
-    PushRule {
-        rule_id: Cow::Borrowed("global/override/.com.beeper.suppress_edits"),
-        priority_class: 5,
-        conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::EventMatch(
-            EventMatchCondition {
-                key: Cow::Borrowed("content.m\\.relates_to.rel_type"),
-                pattern: Cow::Borrowed("m.replace"),
-            },
-        ))]),
-        actions: Cow::Borrowed(&[]),
-        default: true,
-        default_enabled: true,
-    },
     PushRule {
         rule_id: Cow::Borrowed("global/override/.m.rule.suppress_notices"),
         priority_class: 5,
@@ -146,7 +130,7 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[
         priority_class: 5,
         conditions: Cow::Borrowed(&[Condition::Known(
             KnownCondition::ExactEventPropertyContainsType(EventPropertyIsTypeCondition {
-                key: Cow::Borrowed("content.m\\.mentions.user_ids"),
+                key: Cow::Borrowed(r"content.m\.mentions.user_ids"),
                 value_type: Cow::Borrowed(&EventMatchPatternType::UserId),
             }),
         )]),
@@ -167,8 +151,8 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[
         priority_class: 5,
         conditions: Cow::Borrowed(&[
             Condition::Known(KnownCondition::EventPropertyIs(EventPropertyIsCondition {
-                key: Cow::Borrowed("content.m\\.mentions.room"),
-                value: Cow::Borrowed(&SimpleJsonValue::Bool(true)),
+                key: Cow::Borrowed(r"content.m\.mentions.room"),
+                value: Cow::Owned(SimpleJsonValue::Bool(true)),
             })),
             Condition::Known(KnownCondition::SenderNotificationPermission {
                 key: Cow::Borrowed("room"),
@@ -241,6 +225,21 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[
         default: true,
         default_enabled: true,
     },
+    // We don't want to notify on edits *unless* the edit directly mentions a
+    // user, which is handled above.
+    PushRule {
+        rule_id: Cow::Borrowed("global/override/.org.matrix.msc3958.suppress_edits"),
+        priority_class: 5,
+        conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::EventPropertyIs(
+            EventPropertyIsCondition {
+                key: Cow::Borrowed(r"content.m\.relates_to.rel_type"),
+                value: Cow::Owned(SimpleJsonValue::Str(Cow::Borrowed("m.replace"))),
+            },
+        ))]),
+        actions: Cow::Borrowed(&[]),
+        default: true,
+        default_enabled: true,
+    },
     PushRule {
         rule_id: Cow::Borrowed("global/override/.org.matrix.msc3930.rule.poll_response"),
         priority_class: 5,

rust/src/push/evaluator.rs

@@ -117,7 +117,7 @@ impl PushRuleEvaluator {
         msc3931_enabled: bool,
     ) -> Result<Self, Error> {
         let body = match flattened_keys.get("content.body") {
-            Some(JsonValue::Value(SimpleJsonValue::Str(s))) => s.clone(),
+            Some(JsonValue::Value(SimpleJsonValue::Str(s))) => s.clone().into_owned(),
             _ => String::new(),
         };
 
@@ -313,13 +313,15 @@ impl PushRuleEvaluator {
                 };
 
                 let pattern = match &*exact_event_match.value_type {
-                    EventMatchPatternType::UserId => user_id,
-                    EventMatchPatternType::UserLocalpart => get_localpart_from_id(user_id)?,
+                    EventMatchPatternType::UserId => user_id.to_owned(),
+                    EventMatchPatternType::UserLocalpart => {
+                        get_localpart_from_id(user_id)?.to_owned()
+                    }
                 };
 
                 self.match_event_property_contains(
                     exact_event_match.key.clone(),
-                    Cow::Borrowed(&SimpleJsonValue::Str(pattern.to_string())),
+                    Cow::Borrowed(&SimpleJsonValue::Str(Cow::Owned(pattern))),
                 )?
             }
             KnownCondition::ContainsDisplayName => {
@@ -494,7 +496,7 @@ fn push_rule_evaluator() {
     let mut flattened_keys = BTreeMap::new();
     flattened_keys.insert(
         "content.body".to_string(),
-        JsonValue::Value(SimpleJsonValue::Str("foo bar bob hello".to_string())),
+        JsonValue::Value(SimpleJsonValue::Str(Cow::Borrowed("foo bar bob hello"))),
     );
     let evaluator = PushRuleEvaluator::py_new(
         flattened_keys,
@@ -522,7 +524,7 @@ fn test_requires_room_version_supports_condition() {
     let mut flattened_keys = BTreeMap::new();
     flattened_keys.insert(
         "content.body".to_string(),
-        JsonValue::Value(SimpleJsonValue::Str("foo bar bob hello".to_string())),
+        JsonValue::Value(SimpleJsonValue::Str(Cow::Borrowed("foo bar bob hello"))),
     );
     let flags = vec![RoomVersionFeatures::ExtensibleEvents.as_str().to_string()];
     let evaluator = PushRuleEvaluator::py_new(

rust/src/push/mod.rs

@@ -256,7 +256,7 @@ impl<'de> Deserialize<'de> for Action {
 #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)]
 #[serde(untagged)]
 pub enum SimpleJsonValue {
-    Str(String),
+    Str(Cow<'static, str>),
     Int(i64),
     Bool(bool),
     Null,
@@ -265,7 +265,7 @@ pub enum SimpleJsonValue {
 impl<'source> FromPyObject<'source> for SimpleJsonValue {
     fn extract(ob: &'source PyAny) -> PyResult<Self> {
         if let Ok(s) = <PyString as pyo3::PyTryFrom>::try_from(ob) {
-            Ok(SimpleJsonValue::Str(s.to_string()))
+            Ok(SimpleJsonValue::Str(Cow::Owned(s.to_string())))
         // A bool *is* an int, ensure we try bool first.
         } else if let Ok(b) = <PyBool as pyo3::PyTryFrom>::try_from(ob) {
             Ok(SimpleJsonValue::Bool(b.extract()?))
@@ -585,7 +585,7 @@ impl FilteredPushRules {
                 }
 
                 if !self.msc3958_suppress_edits_enabled
-                    && rule.rule_id == "global/override/.com.beeper.suppress_edits"
+                    && rule.rule_id == "global/override/.org.matrix.msc3958.suppress_edits"
                 {
                     return false;
                 }

tests/push/test_bulk_push_rule_evaluator.py

@@ -409,16 +409,33 @@ def test_suppress_edits(self) -> None:
             )
         )
 
-        # Room mentions from those without power should not notify.
+        # The edit should not cause a notification.
         self.assertFalse(
             self._create_and_process(
                 bulk_evaluator,
                 {
-                    "body": self.alice,
+                    "body": "Test message",
                     "m.relates_to": {
                         "rel_type": RelationTypes.REPLACE,
                         "event_id": event.event_id,
                     },
                 },
             )
         )
+
+        # An edit which is a mention will cause a notification.
+        self.assertTrue(
+            self._create_and_process(
+                bulk_evaluator,
+                {
+                    "body": "Test message",
+                    "m.relates_to": {
+                        "rel_type": RelationTypes.REPLACE,
+                        "event_id": event.event_id,
+                    },
+                    "m.mentions": {
+                        "user_ids": [self.alice],
+                    },
+                },
+            )
+        )