From c2d0c2773a41183ac2796baef371b44bab9b7ad3 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Sun, 4 Sep 2022 18:40:32 +0100 Subject: [PATCH 01/35] Implement push rules as Rust native types. --- .rustfmt.toml | 1 + changelog.d/13720.misc | 1 + rust/Cargo.toml | 10 +- rust/src/lib.rs | 8 +- rust/src/push/base_rules.rs | 320 ++++++++++++++ rust/src/push/mod.rs | 411 ++++++++++++++++++ .../__init__.pyi} | 0 stubs/synapse/synapse_rust/push.pyi | 30 ++ synapse/push/bulk_push_rule_evaluator.py | 9 +- synapse/push/clientformat.py | 5 +- synapse/storage/databases/main/push_rule.py | 24 +- tests/handlers/test_deactivate_account.py | 27 +- 12 files changed, 815 insertions(+), 31 deletions(-) create mode 100644 .rustfmt.toml create mode 100644 changelog.d/13720.misc create mode 100644 rust/src/push/base_rules.rs create mode 100644 rust/src/push/mod.rs rename stubs/synapse/{synapse_rust.pyi => synapse_rust/__init__.pyi} (100%) create mode 100644 stubs/synapse/synapse_rust/push.pyi diff --git a/.rustfmt.toml b/.rustfmt.toml new file mode 100644 index 000000000000..bf96e7743de0 --- /dev/null +++ b/.rustfmt.toml @@ -0,0 +1 @@ +group_imports = "StdExternalCrate" diff --git a/changelog.d/13720.misc b/changelog.d/13720.misc new file mode 100644 index 000000000000..af5de96b0447 --- /dev/null +++ b/changelog.d/13720.misc @@ -0,0 +1 @@ +Speed up push rule evaluation in rooms with large numbers of local users. diff --git a/rust/Cargo.toml b/rust/Cargo.toml index 0a9760cafcd1..fd4bdfe8e713 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -18,4 +18,12 @@ crate-type = ["cdylib"] name = "synapse.synapse_rust" [dependencies] -pyo3 = { version = "0.16.5", features = ["extension-module", "macros", "abi3", "abi3-py37"] } +anyhow = "1.0.63" +lazy_static = "1.4.0" +log = "0.4.17" +pyo3 = { version = "0.17.1", features = ["extension-module", "macros", "anyhow", "abi3", "abi3-py37"] } +pyo3-log = "0.7.0" +pythonize = "0.17.0" +regex = "1.6.0" +serde = { version = "1.0.144", features = ["derive"] } +serde_json = "1.0.85" diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 142fc2ed93af..2bd76c1aa86b 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -1,5 +1,7 @@ use pyo3::prelude::*; +pub mod push; + /// Formats the sum of two numbers as string. #[pyfunction] #[pyo3(text_signature = "(a, b, /)")] @@ -9,8 +11,12 @@ fn sum_as_string(a: usize, b: usize) -> PyResult { /// The entry point for defining the Python module. #[pymodule] -fn synapse_rust(_py: Python<'_>, m: &PyModule) -> PyResult<()> { +fn synapse_rust(py: Python<'_>, m: &PyModule) -> PyResult<()> { + pyo3_log::init(); + m.add_function(wrap_pyfunction!(sum_as_string, m)?)?; + push::register_module(py, m)?; + Ok(()) } diff --git a/rust/src/push/base_rules.rs b/rust/src/push/base_rules.rs new file mode 100644 index 000000000000..eed3ae51b18c --- /dev/null +++ b/rust/src/push/base_rules.rs @@ -0,0 +1,320 @@ +// Copyright 2022 The Matrix.org Foundation C.I.C. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// 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. + +//! Contains the definitions of the "base" push rules. + +use std::borrow::Cow; +use std::collections::HashMap; + +use lazy_static::lazy_static; +use serde_json::Value; + +use crate::push::Action; +use crate::push::Condition; +use crate::push::EventMatchCondition; +use crate::push::PushRule; +use crate::push::SetTweak; +use crate::push::TweakValue; + +const HIGHLIGHT_ACTION: Action = Action::SetTweak(SetTweak { + set_tweak: Cow::Borrowed("highlight"), + value: None, + other_keys: Value::Null, +}); + +const HIGHLIGHT_FALSE_ACTION: Action = Action::SetTweak(SetTweak { + set_tweak: Cow::Borrowed("highlight"), + value: Some(TweakValue::Other(Value::Bool(false))), + other_keys: Value::Null, +}); + +const SOUND_ACTION: Action = Action::SetTweak(SetTweak { + set_tweak: Cow::Borrowed("sound"), + value: Some(TweakValue::String(Cow::Borrowed("default"))), + other_keys: Value::Null, +}); + +const RING_ACTION: Action = Action::SetTweak(SetTweak { + set_tweak: Cow::Borrowed("sound"), + value: Some(TweakValue::String(Cow::Borrowed("ring"))), + other_keys: Value::Null, +}); + +pub const BASE_PREPEND_OVERRIDE_RULES: &[PushRule] = &[PushRule { + rule_id: Cow::Borrowed("global/override/.m.rule.master"), + priority_class: 5, + conditions: Cow::Borrowed(&[]), + actions: Cow::Borrowed(&[Action::DontNotify]), + default: true, + default_enabled: false, +}]; + +pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[ + PushRule { + rule_id: Cow::Borrowed("global/override/.m.rule.suppress_notices"), + priority_class: 5, + conditions: Cow::Borrowed(&[Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("content.msgtype"), + pattern: Some(Cow::Borrowed("m.notice")), + pattern_type: None, + })]), + actions: Cow::Borrowed(&[Action::DontNotify]), + default: true, + default_enabled: true, + }, + PushRule { + rule_id: Cow::Borrowed("global/override/.m.rule.invite_for_me"), + priority_class: 5, + conditions: Cow::Borrowed(&[ + Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("type"), + pattern: Some(Cow::Borrowed("m.room.member")), + pattern_type: None, + }), + Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("content.membership"), + pattern: Some(Cow::Borrowed("invite")), + pattern_type: None, + }), + Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("state_key"), + pattern: None, + pattern_type: Some(Cow::Borrowed("user_id")), + }), + ]), + actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_FALSE_ACTION, SOUND_ACTION]), + default: true, + default_enabled: true, + }, + PushRule { + rule_id: Cow::Borrowed("global/override/.m.rule.member_event"), + priority_class: 5, + conditions: Cow::Borrowed(&[Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("type"), + pattern: Some(Cow::Borrowed("m.room.member")), + pattern_type: None, + })]), + actions: Cow::Borrowed(&[Action::DontNotify]), + default: true, + default_enabled: true, + }, + PushRule { + rule_id: Cow::Borrowed("global/override/.m.rule.contains_display_name"), + priority_class: 5, + conditions: Cow::Borrowed(&[Condition::ContainsDisplayName]), + actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_ACTION, SOUND_ACTION]), + default: true, + default_enabled: true, + }, + PushRule { + rule_id: Cow::Borrowed("global/override/.m.rule.roomnotif"), + priority_class: 5, + conditions: Cow::Borrowed(&[ + Condition::SenderNotificationPermission { + key: Cow::Borrowed("room"), + }, + Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("content.body"), + pattern: Some(Cow::Borrowed("@room")), + pattern_type: None, + }), + ]), + actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_ACTION, SOUND_ACTION]), + default: true, + default_enabled: true, + }, + PushRule { + rule_id: Cow::Borrowed("global/override/.m.rule.tombstone"), + priority_class: 5, + conditions: Cow::Borrowed(&[ + Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("type"), + pattern: Some(Cow::Borrowed("m.room.tombstone")), + pattern_type: None, + }), + Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("state_key"), + pattern: Some(Cow::Borrowed("")), + pattern_type: None, + }), + ]), + actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_ACTION]), + default: true, + default_enabled: true, + }, + PushRule { + rule_id: Cow::Borrowed("global/override/.m.rule.reaction"), + priority_class: 5, + conditions: Cow::Borrowed(&[Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("type"), + pattern: Some(Cow::Borrowed("m.reaction")), + pattern_type: None, + })]), + actions: Cow::Borrowed(&[Action::DontNotify]), + default: true, + default_enabled: true, + }, + PushRule { + rule_id: Cow::Borrowed("global/override/.org.matrix.msc3786.rule.room.server_acl"), + priority_class: 5, + conditions: Cow::Borrowed(&[ + Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("type"), + pattern: Some(Cow::Borrowed("m.room.server_acl")), + pattern_type: None, + }), + Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("state_key"), + pattern: Some(Cow::Borrowed("")), + pattern_type: None, + }), + ]), + actions: Cow::Borrowed(&[]), + default: true, + default_enabled: true, + }, +]; + +pub const BASE_APPEND_CONTENT_RULES: &[PushRule] = &[PushRule { + rule_id: Cow::Borrowed("global/content/.m.rule.contains_user_name"), + priority_class: 4, + conditions: Cow::Borrowed(&[Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("content.body"), + pattern: None, + pattern_type: Some(Cow::Borrowed("user_localpart")), + })]), + actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_ACTION, SOUND_ACTION]), + default: true, + default_enabled: true, +}]; + +pub const BASE_APPEND_UNDERRIDE_RULES: &[PushRule] = &[ + PushRule { + rule_id: Cow::Borrowed("global/underride/.m.rule.call"), + priority_class: 1, + conditions: Cow::Borrowed(&[Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("type"), + pattern: Some(Cow::Borrowed("m.call.invite")), + pattern_type: None, + })]), + actions: Cow::Borrowed(&[Action::Notify, RING_ACTION, HIGHLIGHT_FALSE_ACTION]), + default: true, + default_enabled: true, + }, + PushRule { + rule_id: Cow::Borrowed("global/underride/.m.rule.room_one_to_one"), + priority_class: 1, + conditions: Cow::Borrowed(&[ + Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("type"), + pattern: Some(Cow::Borrowed("m.room.message")), + pattern_type: None, + }), + Condition::RoomMemberCount { + is: Some(Cow::Borrowed("2")), + }, + ]), + actions: Cow::Borrowed(&[Action::Notify, SOUND_ACTION, HIGHLIGHT_FALSE_ACTION]), + default: true, + default_enabled: true, + }, + PushRule { + rule_id: Cow::Borrowed("global/underride/.m.rule.encrypted_room_one_to_one"), + priority_class: 1, + conditions: Cow::Borrowed(&[ + Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("type"), + pattern: Some(Cow::Borrowed("m.room.encrypted")), + pattern_type: None, + }), + Condition::RoomMemberCount { + is: Some(Cow::Borrowed("2")), + }, + ]), + actions: Cow::Borrowed(&[Action::Notify, SOUND_ACTION, HIGHLIGHT_FALSE_ACTION]), + default: true, + default_enabled: true, + }, + PushRule { + rule_id: Cow::Borrowed("global/underride/.org.matrix.msc3772.thread_reply"), + priority_class: 1, + conditions: Cow::Borrowed(&[Condition::RelationMatch { + rel_type: Cow::Borrowed("m.thread"), + sender: None, + sender_type: Some(Cow::Borrowed("user_id")), + }]), + actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_FALSE_ACTION]), + default: true, + default_enabled: true, + }, + PushRule { + rule_id: Cow::Borrowed("global/underride/.m.rule.message"), + priority_class: 1, + conditions: Cow::Borrowed(&[Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("type"), + pattern: Some(Cow::Borrowed("m.room.message")), + pattern_type: None, + })]), + actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_FALSE_ACTION]), + default: true, + default_enabled: true, + }, + PushRule { + rule_id: Cow::Borrowed("global/underride/.m.rule.encrypted"), + priority_class: 1, + conditions: Cow::Borrowed(&[Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("type"), + pattern: Some(Cow::Borrowed("m.room.encrypted")), + pattern_type: None, + })]), + actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_FALSE_ACTION]), + default: true, + default_enabled: true, + }, + PushRule { + rule_id: Cow::Borrowed("global/underride/.im.vector.jitsi"), + priority_class: 1, + conditions: Cow::Borrowed(&[ + Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("type"), + pattern: Some(Cow::Borrowed("im.vector.modular.widgets")), + pattern_type: None, + }), + Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("content.type"), + pattern: Some(Cow::Borrowed("jitsi")), + pattern_type: None, + }), + Condition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("state_key"), + pattern: Some(Cow::Borrowed("*")), + pattern_type: None, + }), + ]), + actions: Cow::Borrowed(&[HIGHLIGHT_FALSE_ACTION]), + default: true, + default_enabled: true, + }, +]; + +lazy_static! { + pub static ref BASE_RULES_BY_ID: HashMap<&'static str, &'static PushRule> = + BASE_PREPEND_OVERRIDE_RULES + .iter() + .chain(BASE_APPEND_OVERRIDE_RULES.iter()) + .chain(BASE_APPEND_CONTENT_RULES.iter()) + .chain(BASE_APPEND_UNDERRIDE_RULES.iter()) + .map(|rule| { (&*rule.rule_id, rule) }) + .collect(); +} diff --git a/rust/src/push/mod.rs b/rust/src/push/mod.rs new file mode 100644 index 000000000000..f9d3db8182b1 --- /dev/null +++ b/rust/src/push/mod.rs @@ -0,0 +1,411 @@ +// Copyright 2022 The Matrix.org Foundation C.I.C. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// 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. + +//! An implementation of Matrix push rules. +//! +//! The `Cow<_>` type is used extensively within this module to allow creating +//! the base rules as constants (in Rust constants can't require explicit +//! allocation atm). + +use std::borrow::Cow; +use std::collections::{BTreeMap, HashMap}; + +use anyhow::{Context, Error}; +use log::warn; +use pyo3::prelude::*; +use pythonize::pythonize; +use serde::de::Error as _; +use serde::{Deserialize, Serialize}; +use serde_json::Value; + +mod base_rules; + +/// Called when registering modules with python. +pub fn register_module(py: Python<'_>, m: &PyModule) -> PyResult<()> { + let child_module = PyModule::new(py, "push")?; + child_module.add_class::()?; + child_module.add_class::()?; + child_module.add_class::()?; + m.add_submodule(child_module)?; + + // We need to manually add the module to sys.modules to make `from + // synapse.synapse_rust import push` work. + py.import("sys")? + .getattr("modules")? + .set_item("synapse.synapse_rust.push", child_module)?; + + Ok(()) +} + +/// A single push rule for a user. +#[derive(Debug, Clone)] +#[pyclass(frozen)] +pub struct PushRule { + pub rule_id: Cow<'static, str>, + #[pyo3(get)] + pub priority_class: i32, + pub conditions: Cow<'static, [Condition]>, + pub actions: Cow<'static, [Action]>, + #[pyo3(get)] + pub default: bool, + #[pyo3(get)] + pub default_enabled: bool, +} + +#[pymethods] +impl PushRule { + #[staticmethod] + pub fn from_db( + rule_id: String, + priority_class: i32, + conditions: &str, + actions: &str, + ) -> Result { + let conditions = serde_json::from_str(conditions).context("parsing conditions")?; + let actions = serde_json::from_str(actions).context("parsing actions")?; + + Ok(PushRule { + rule_id: Cow::Owned(rule_id), + priority_class, + conditions, + actions, + default: false, + default_enabled: true, + }) + } + + #[getter] + fn rule_id(&self) -> &str { + &self.rule_id + } + + #[getter] + fn actions(&self) -> Vec { + self.actions.clone().into_owned() + } + + #[getter] + fn conditions(&self) -> Vec { + self.conditions.clone().into_owned() + } + + fn __repr__(&self) -> String { + format!( + "", + self.rule_id, self.conditions, self.actions + ) + } +} + +/// The "action" Synapse should perform for a matching push rule. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Action { + DontNotify, + Notify, + Coalesce, + SetTweak(SetTweak), +} + +impl IntoPy for Action { + fn into_py(self, py: Python<'_>) -> PyObject { + pythonize(py, &self).expect("valid action") + } +} + +/// The body of a `SetTweak` push action. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] +pub struct SetTweak { + set_tweak: Cow<'static, str>, + + #[serde(skip_serializing_if = "Option::is_none")] + value: Option, + + // This picks saves any other fields that may have been added as clients. + // These get added when we convert the `Action` to a python object. + #[serde(flatten)] + other_keys: Value, +} + +/// The value of a `set_tweak`. +/// +/// We need this (rather than using `TweakValue` directly) so that we can use +/// `&'static str` in the value when defining the constant base rules. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] +#[serde(untagged)] +pub enum TweakValue { + String(Cow<'static, str>), + Other(Value), +} + +impl Serialize for Action { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + match self { + Action::DontNotify => serializer.serialize_str("dont_notify"), + Action::Notify => serializer.serialize_str("notify"), + Action::Coalesce => serializer.serialize_str("coalesce"), + Action::SetTweak(tweak) => tweak.serialize(serializer), + } + } +} + +/// Simple helper class for deserializing Action from JSON. +#[derive(Deserialize)] +#[serde(untagged)] +enum ActionDeserializeHelper { + Str(String), + SetTweak(SetTweak), +} + +impl<'de> Deserialize<'de> for Action { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let helper: ActionDeserializeHelper = Deserialize::deserialize(deserializer)?; + match helper { + ActionDeserializeHelper::Str(s) => match &*s { + "dont_notify" => Ok(Action::DontNotify), + "notify" => Ok(Action::Notify), + "coalesce" => Ok(Action::Coalesce), + _ => Err(D::Error::custom("unrecognized action")), + }, + ActionDeserializeHelper::SetTweak(set_tweak) => Ok(Action::SetTweak(set_tweak)), + } + } +} + +/// A condition used in push rules to match against an event. +#[derive(Serialize, Deserialize, Debug, Clone)] +#[serde(rename_all = "snake_case")] +#[serde(tag = "kind")] +pub enum Condition { + EventMatch(EventMatchCondition), + ContainsDisplayName, + RoomMemberCount { + #[serde(skip_serializing_if = "Option::is_none")] + is: Option>, + }, + SenderNotificationPermission { + key: Cow<'static, str>, + }, + #[serde(rename = "org.matrix.msc3772.relation_match")] + RelationMatch { + rel_type: Cow<'static, str>, + #[serde(skip_serializing_if = "Option::is_none")] + sender: Option>, + #[serde(skip_serializing_if = "Option::is_none")] + sender_type: Option>, + }, +} + +impl IntoPy for Condition { + fn into_py(self, py: Python<'_>) -> PyObject { + pythonize(py, &self).expect("valid condition") + } +} + +/// The body of a [`Condition::EventMatch`] +#[derive(Serialize, Deserialize, Debug, Clone)] +pub struct EventMatchCondition { + key: Cow<'static, str>, + #[serde(skip_serializing_if = "Option::is_none")] + pattern: Option>, + #[serde(skip_serializing_if = "Option::is_none")] + pattern_type: Option>, +} + +/// The collection of push rules for a user. +#[derive(Debug, Clone, Default)] +#[pyclass(frozen)] +struct PushRules { + /// Custom push rules that override a base rule. + overridden_base_rules: HashMap, PushRule>, + + /// Custom rules that come between the prepend/append override base rules. + override_rules: Vec, + /// Custom rules that come before the base content rules. + content: Vec, + /// Custom rules that come before the base room rules. + room: Vec, + /// Custom rules that come before the base sender rules. + sender: Vec, + /// Custom rules that come before the base underride rules. + underride: Vec, +} + +#[pymethods] +impl PushRules { + #[new] + fn new(rules: Vec) -> PushRules { + let mut push_rules: PushRules = Default::default(); + + for rule in rules { + if let Some(&o) = base_rules::BASE_RULES_BY_ID.get(&*rule.rule_id) { + push_rules.overridden_base_rules.insert( + rule.rule_id.clone(), + PushRule { + actions: rule.actions.clone(), + ..o.clone() + }, + ); + + continue; + } + + match rule.priority_class { + 5 => push_rules.override_rules.push(rule), + 4 => push_rules.content.push(rule), + 3 => push_rules.room.push(rule), + 2 => push_rules.sender.push(rule), + 1 => push_rules.underride.push(rule), + _ => { + warn!( + "Unrecognized priority class for rule {}: {}", + rule.rule_id, rule.priority_class + ); + } + } + } + + push_rules + } + + /// Returns the list of all rules, including base rules, in the order they + /// should be executed in. + fn rules(&self) -> Vec { + self.iter().cloned().collect() + } +} + +impl PushRules { + /// Iterates over all the rules, including base rules, in the order they + /// should be executed in. + pub fn iter(&self) -> impl Iterator { + base_rules::BASE_PREPEND_OVERRIDE_RULES + .iter() + .chain(self.override_rules.iter()) + .chain(base_rules::BASE_APPEND_OVERRIDE_RULES.iter()) + .chain(self.content.iter()) + .chain(base_rules::BASE_APPEND_CONTENT_RULES.iter()) + .chain(self.room.iter()) + .chain(self.sender.iter()) + .chain(self.underride.iter()) + .chain(base_rules::BASE_APPEND_UNDERRIDE_RULES.iter()) + .map(|rule| { + self.overridden_base_rules + .get(&*rule.rule_id) + .unwrap_or(rule) + }) + } +} + +/// A wrapper around `PushRules` that checks the enabled state of rules and +/// filters out disabled experimental rules. +#[derive(Debug, Clone, Default)] +#[pyclass(frozen)] +pub struct FilteredPushRules { + push_rules: PushRules, + enabled_map: BTreeMap, + msc3786_enabled: bool, + msc3772_enabled: bool, +} + +#[pymethods] +impl FilteredPushRules { + #[new] + fn py_new( + push_rules: PushRules, + enabled_map: BTreeMap, + msc3786_enabled: bool, + msc3772_enabled: bool, + ) -> Self { + Self { + push_rules, + enabled_map, + msc3786_enabled, + msc3772_enabled, + } + } + + /// Returns the list of all rules and their enabled state, including base + /// rules, in the order they should be executed in. + fn rules(&self) -> Vec<(PushRule, bool)> { + self.iter().map(|(r, e)| (r.clone(), e)).collect() + } +} + +impl FilteredPushRules { + /// Iterates over all the rules and their enabled state, including base + /// rules, in the order they should be executed in. + fn iter(&self) -> impl Iterator { + self.push_rules + .iter() + .filter(|rule| { + // Ignore disabled experimental push rules + if !self.msc3786_enabled + && rule.rule_id == "global/override/.org.matrix.msc3786.rule.room.server_acl" + { + return false; + } + + if !self.msc3772_enabled + && rule.rule_id == "global/underride/.org.matrix.msc3772.thread_reply" + { + return false; + } + + true + }) + .map(|r| { + let enabled = *self + .enabled_map + .get(&*r.rule_id) + .unwrap_or(&r.default_enabled); + (r, enabled) + }) + } +} + +#[test] +fn test_erialize_condition() { + let condition = Condition::EventMatch(EventMatchCondition { + key: "content.body".into(), + pattern: Some("coffee".into()), + pattern_type: None, + }); + + let json = serde_json::to_string(&condition).unwrap(); + assert_eq!( + json, + r#"{"kind":"event_match","key":"content.body","pattern":"coffee"}"# + ) +} + +#[test] +fn test_deserialize_condition() { + let json = r#"{"kind":"event_match","key":"content.body","pattern":"coffee"}"#; + + let _: Condition = serde_json::from_str(json).unwrap(); +} + +#[test] +fn test_deserialize_action() { + let _: Action = serde_json::from_str(r#""notify""#).unwrap(); + let _: Action = serde_json::from_str(r#""dont_notify""#).unwrap(); + let _: Action = serde_json::from_str(r#""coalesce""#).unwrap(); + let _: Action = serde_json::from_str(r#"{"set_tweak": "highlight"}"#).unwrap(); +} diff --git a/stubs/synapse/synapse_rust.pyi b/stubs/synapse/synapse_rust/__init__.pyi similarity index 100% rename from stubs/synapse/synapse_rust.pyi rename to stubs/synapse/synapse_rust/__init__.pyi diff --git a/stubs/synapse/synapse_rust/push.pyi b/stubs/synapse/synapse_rust/push.pyi new file mode 100644 index 000000000000..06336c8899b2 --- /dev/null +++ b/stubs/synapse/synapse_rust/push.pyi @@ -0,0 +1,30 @@ +from typing import Any, Collection, Dict, Mapping, Sequence, Tuple, Union + +from synapse.types import JsonDict + +class PushRule: + rule_id: str + priority_class: int + conditions: Sequence[Mapping[str, str]] + actions: Sequence[Union[Mapping[str, Any], str]] + default: bool + default_enabled: bool + + @staticmethod + def from_db( + rule_id: str, priority_class: int, conditions: str, actions: str + ) -> "PushRule": ... + +class PushRules: + def __init__(self, rules: Collection[PushRule]): ... + def rules(self) -> Collection[PushRule]: ... + +class FilteredPushRules: + def __init__( + self, + push_rules: PushRules, + enabled_map: Dict[str, bool], + msc3786_enabled: bool, + msc3772_enabled: bool, + ): ... + def rules(self) -> Collection[Tuple[PushRule, bool]]: ... diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index d1caf8a0f7a0..b976508ef3d8 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -34,14 +34,15 @@ from synapse.event_auth import auth_types_for_event, get_user_power_level from synapse.events import EventBase, relation_from_event from synapse.events.snapshot import EventContext +from synapse.push.push_rule_evaluator import _flatten_dict from synapse.state import POWER_KEY from synapse.storage.databases.main.roommember import EventIdMembership from synapse.storage.state import StateFilter +from synapse.synapse_rust.push import FilteredPushRules, PushRule from synapse.util.caches import register_cache 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: @@ -282,9 +283,11 @@ async def action_for_event_by_user( ) = await self._get_power_levels_and_sender_level(event, context) relations = await self._get_mutual_relations( - event, itertools.chain(*rules_by_user.values()) + event, itertools.chain(*(r.rules() for r in rules_by_user.values())) ) + logger.info("Flatten map: %s", _flatten_dict(event)) + logger.info("room_member_count: %s", room_member_count) evaluator = PushRuleEvaluatorForEvent( event, room_member_count, @@ -333,7 +336,7 @@ async def action_for_event_by_user( # current user, it'll be added to the dict later. actions_by_user[uid] = [] - for rule, enabled in rules: + for rule, enabled in rules.rules(): if not enabled: continue diff --git a/synapse/push/clientformat.py b/synapse/push/clientformat.py index 73618d9234bf..ebc13beda1bc 100644 --- a/synapse/push/clientformat.py +++ b/synapse/push/clientformat.py @@ -16,10 +16,9 @@ from typing import Any, Dict, List, Optional from synapse.push.rulekinds import PRIORITY_CLASS_INVERSE_MAP, PRIORITY_CLASS_MAP +from synapse.synapse_rust.push import FilteredPushRules, PushRule from synapse.types import UserID -from .baserules import FilteredPushRules, PushRule - def format_push_rules_for_user( user: UserID, ruleslist: FilteredPushRules @@ -34,7 +33,7 @@ def format_push_rules_for_user( rules["global"] = _add_empty_priority_class_arrays(rules["global"]) - for r, enabled in ruleslist: + for r, enabled in ruleslist.rules(): template_name = _priority_class_to_template_name(r.priority_class) rulearray = rules["global"][template_name] diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 5079edd1e083..9d76e9b87979 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -30,9 +30,8 @@ from synapse.api.errors import StoreError from synapse.config.homeserver import ExperimentalConfig -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._base import SQLBaseStore from synapse.storage.database import ( DatabasePool, LoggingDatabaseConnection, @@ -51,6 +50,7 @@ IdGenerator, StreamIdGenerator, ) +from synapse.synapse_rust.push import FilteredPushRules, PushRule, PushRules from synapse.types import JsonDict from synapse.util import json_encoder from synapse.util.caches.descriptors import cached, cachedList @@ -72,18 +72,26 @@ def _load_rules( """ ruleslist = [ - PushRule( + PushRule.from_db( rule_id=rawrule["rule_id"], priority_class=rawrule["priority_class"], - conditions=db_to_json(rawrule["conditions"]), - actions=db_to_json(rawrule["actions"]), + conditions=rawrule["conditions"], + actions=rawrule["actions"], ) for rawrule in rawrules ] - push_rules = compile_push_rules(ruleslist) + push_rules = PushRules( + ruleslist, + ) - filtered_rules = FilteredPushRules(push_rules, enabled_map, experimental_config) + # TODO: Experimental config + filtered_rules = FilteredPushRules( + push_rules, + enabled_map, + msc3786_enabled=experimental_config.msc3786_enabled, + msc3772_enabled=experimental_config.msc3772_enabled, + ) return filtered_rules @@ -845,7 +853,7 @@ 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, enabled in user_push_rules: + for rule, enabled in user_push_rules.rules(): if not enabled: continue diff --git a/tests/handlers/test_deactivate_account.py b/tests/handlers/test_deactivate_account.py index 7b9b711521d8..bce65fab7d84 100644 --- a/tests/handlers/test_deactivate_account.py +++ b/tests/handlers/test_deactivate_account.py @@ -15,11 +15,11 @@ 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 from synapse.server import HomeServer +from synapse.synapse_rust.push import PushRule from synapse.util import Clock from tests.unittest import HomeserverTestCase @@ -161,20 +161,15 @@ def test_push_rules_deleted_upon_account_deactivation(self) -> None: self._store.get_push_rules_for_user(self.user) ) # Filter out default rules; we don't care - push_rules = [r for r, _ in filtered_push_rules if self._is_custom_rule(r)] + push_rules = [ + r for r, _ in filtered_push_rules.rules() if self._is_custom_rule(r) + ] # Check our rule made it - self.assertEqual( - push_rules, - [ - PushRule( - rule_id="personal.override.rule1", - priority_class=5, - conditions=[], - actions=[], - ) - ], - push_rules, - ) + self.assertEqual(len(push_rules), 1) + self.assertEqual(push_rules[0].rule_id, "personal.override.rule1") + self.assertEqual(push_rules[0].priority_class, 5) + self.assertEqual(push_rules[0].conditions, []) + self.assertEqual(push_rules[0].actions, []) # Request the deactivation of our account self._deactivate_my_account() @@ -183,7 +178,9 @@ def test_push_rules_deleted_upon_account_deactivation(self) -> None: self._store.get_push_rules_for_user(self.user) ) # Filter out default rules; we don't care - push_rules = [r for r, _ in filtered_push_rules if self._is_custom_rule(r)] + push_rules = [ + r for r, _ in filtered_push_rules.rules() if self._is_custom_rule(r) + ] # Check our rule no longer exists self.assertEqual(push_rules, [], push_rules) From dcea8d82b92111853aa04a888b5d6163388aae21 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 9 Sep 2022 15:28:02 +0100 Subject: [PATCH 02/35] Remove python baserules --- rust/src/push/mod.rs | 9 +- stubs/synapse/synapse_rust/push.pyi | 2 + synapse/handlers/push_rules.py | 5 +- synapse/push/baserules.py | 583 ---------------------------- 4 files changed, 14 insertions(+), 585 deletions(-) delete mode 100644 synapse/push/baserules.py diff --git a/rust/src/push/mod.rs b/rust/src/push/mod.rs index f9d3db8182b1..6df5382126c0 100644 --- a/rust/src/push/mod.rs +++ b/rust/src/push/mod.rs @@ -19,7 +19,7 @@ //! allocation atm). use std::borrow::Cow; -use std::collections::{BTreeMap, HashMap}; +use std::collections::{BTreeMap, HashMap, HashSet}; use anyhow::{Context, Error}; use log::warn; @@ -37,6 +37,8 @@ pub fn register_module(py: Python<'_>, m: &PyModule) -> PyResult<()> { child_module.add_class::()?; child_module.add_class::()?; child_module.add_class::()?; + child_module.add_function(wrap_pyfunction!(get_base_rule_ids, m)?)?; + m.add_submodule(child_module)?; // We need to manually add the module to sys.modules to make `from @@ -48,6 +50,11 @@ pub fn register_module(py: Python<'_>, m: &PyModule) -> PyResult<()> { Ok(()) } +#[pyfunction] +fn get_base_rule_ids() -> HashSet<&'static str> { + base_rules::BASE_RULES_BY_ID.keys().copied().collect() +} + /// A single push rule for a user. #[derive(Debug, Clone)] #[pyclass(frozen)] diff --git a/stubs/synapse/synapse_rust/push.pyi b/stubs/synapse/synapse_rust/push.pyi index 06336c8899b2..cb59bf9d8af9 100644 --- a/stubs/synapse/synapse_rust/push.pyi +++ b/stubs/synapse/synapse_rust/push.pyi @@ -28,3 +28,5 @@ class FilteredPushRules: msc3772_enabled: bool, ): ... def rules(self) -> Collection[Tuple[PushRule, bool]]: ... + +def get_base_rule_ids() -> Collection[str]: ... diff --git a/synapse/handlers/push_rules.py b/synapse/handlers/push_rules.py index 2599160bcc00..1219672a59dc 100644 --- a/synapse/handlers/push_rules.py +++ b/synapse/handlers/push_rules.py @@ -16,14 +16,17 @@ import attr from synapse.api.errors import SynapseError, UnrecognizedRequestError -from synapse.push.baserules import BASE_RULE_IDS from synapse.storage.push_rule import RuleNotFoundException +from synapse.synapse_rust.push import get_base_rule_ids from synapse.types import JsonDict if TYPE_CHECKING: from synapse.server import HomeServer +BASE_RULE_IDS = get_base_rule_ids() + + @attr.s(slots=True, frozen=True, auto_attribs=True) class RuleSpec: scope: str diff --git a/synapse/push/baserules.py b/synapse/push/baserules.py deleted file mode 100644 index 440205e80c05..000000000000 --- a/synapse/push/baserules.py +++ /dev/null @@ -1,583 +0,0 @@ -# Copyright 2015, 2016 OpenMarket Ltd -# Copyright 2017 New Vector Ltd -# Copyright 2019 The Matrix.org Foundation C.I.C. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# 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. - -""" -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 -import logging -from typing import Dict, Iterator, List, Mapping, Sequence, Tuple, Union - -import attr - -from synapse.config.experimental import ExperimentalConfig -from synapse.push.rulekinds import PRIORITY_CLASS_MAP - -logger = logging.getLogger(__name__) - - -@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]] - actions: Sequence[Union[str, Mapping]] - default: bool = False - default_enabled: bool = True - - -@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. - """ - - # 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: - # 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) - + len(self.content) - + len(self.room) - + len(self.sender) - + len(self.underride) - ) - - -@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. - """ - - push_rules: PushRules - enabled_map: Dict[str, bool] - experimental_config: ExperimentalConfig - - 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 - - enabled = self.enabled_map.get(rule.rule_id, rule.default_enabled) - - yield rule, enabled - - def __len__(self) -> int: - return len(self.push_rules) - - -DEFAULT_EMPTY_PUSH_RULES = PushRules() - - -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 - - rules = PushRules() - - for rule in rawrules: - # We need to decide which bucket each custom push rule goes into. - - # 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 - elif rule.priority_class <= 0: - logger.info( - "Got rule with priority class less than zero, but doesn't override a base rule: %s", - rule, - ) - continue - else: - # We log and continue here so as not to break event sending - logger.error("Unknown priority class: %", rule.priority_class) - continue - - collection.append(rule) - - return rules - - -def _is_experimental_rule_enabled( - rule_id: str, experimental_config: ExperimentalConfig -) -> bool: - """Used by `FilteredPushRules` 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 = [ - PushRule( - default=True, - priority_class=PRIORITY_CLASS_MAP["override"], - rule_id="global/override/.m.rule.master", - default_enabled=False, - conditions=[], - 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 - 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 - 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. - 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 - 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 = [ - 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. - 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.*)... - 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.*)... - 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: - BASE_RULE_IDS.add(r.rule_id) - BASE_RULES_BY_ID[r.rule_id] = r - -for r in BASE_PREPEND_OVERRIDE_RULES: - BASE_RULE_IDS.add(r.rule_id) - BASE_RULES_BY_ID[r.rule_id] = r - -for r in BASE_APPEND_OVERRIDE_RULES: - BASE_RULE_IDS.add(r.rule_id) - BASE_RULES_BY_ID[r.rule_id] = r - -for r in BASE_APPEND_UNDERRIDE_RULES: - BASE_RULE_IDS.add(r.rule_id) - BASE_RULES_BY_ID[r.rule_id] = r From 62caed98a839626b7eeeadf938915c3772e91065 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 9 Sep 2022 15:49:35 +0100 Subject: [PATCH 03/35] Newsfile --- changelog.d/13768.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/13768.misc diff --git a/changelog.d/13768.misc b/changelog.d/13768.misc new file mode 100644 index 000000000000..28bddb705967 --- /dev/null +++ b/changelog.d/13768.misc @@ -0,0 +1 @@ +Port push rules to using Rust. From 071953f49a0112513a1a2c2c0e20bd5dbf525af7 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 12 Sep 2022 17:54:18 +0100 Subject: [PATCH 04/35] Update rust/src/push/mod.rs Co-authored-by: David Robertson --- rust/src/push/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/src/push/mod.rs b/rust/src/push/mod.rs index 6df5382126c0..2a5fb625244f 100644 --- a/rust/src/push/mod.rs +++ b/rust/src/push/mod.rs @@ -388,7 +388,7 @@ impl FilteredPushRules { } #[test] -fn test_erialize_condition() { +fn test_serialize_condition() { let condition = Condition::EventMatch(EventMatchCondition { key: "content.body".into(), pattern: Some("coffee".into()), From 4d9734950f44d54b31be29d8426820405de21ffd Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 13 Sep 2022 08:13:44 +0100 Subject: [PATCH 05/35] Code review --- rust/src/push/mod.rs | 3 +++ stubs/synapse/synapse_rust/push.pyi | 19 ++++++++++++------- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/rust/src/push/mod.rs b/rust/src/push/mod.rs index 2a5fb625244f..dcbe9aeec0f0 100644 --- a/rust/src/push/mod.rs +++ b/rust/src/push/mod.rs @@ -126,6 +126,9 @@ pub enum Action { impl IntoPy for Action { fn into_py(self, py: Python<'_>) -> PyObject { + // When we pass the `Action` struct to Python we want it to be converted + // to a dict. We use `pythonize`, which converts the struct using the + // `serde` serialization. pythonize(py, &self).expect("valid action") } } diff --git a/stubs/synapse/synapse_rust/push.pyi b/stubs/synapse/synapse_rust/push.pyi index cb59bf9d8af9..93c4e69d424f 100644 --- a/stubs/synapse/synapse_rust/push.pyi +++ b/stubs/synapse/synapse_rust/push.pyi @@ -3,13 +3,18 @@ from typing import Any, Collection, Dict, Mapping, Sequence, Tuple, Union from synapse.types import JsonDict class PushRule: - rule_id: str - priority_class: int - conditions: Sequence[Mapping[str, str]] - actions: Sequence[Union[Mapping[str, Any], str]] - default: bool - default_enabled: bool - + @property + def rule_id(self) -> str: ... + @property + def priority_class(self) -> int: ... + @property + def conditions(self) -> Sequence[Mapping[str, str]]: ... + @property + def actions(self) -> Sequence[Union[Mapping[str, Any], str]]: ... + @property + def default(self) -> bool: ... + @property + def default_enabled(self) -> bool: ... @staticmethod def from_db( rule_id: str, priority_class: int, conditions: str, actions: str From e828b34e4925c949927e7fb505d695a386cd1907 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 19 Sep 2022 10:17:16 +0100 Subject: [PATCH 06/35] Apply suggestions from code review Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- rust/src/push/base_rules.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/src/push/base_rules.rs b/rust/src/push/base_rules.rs index eed3ae51b18c..aaedb2aab10f 100644 --- a/rust/src/push/base_rules.rs +++ b/rust/src/push/base_rules.rs @@ -130,7 +130,7 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[ pattern_type: None, }), ]), - actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_ACTION, SOUND_ACTION]), + actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_ACTION]), default: true, default_enabled: true, }, @@ -302,7 +302,7 @@ pub const BASE_APPEND_UNDERRIDE_RULES: &[PushRule] = &[ pattern_type: None, }), ]), - actions: Cow::Borrowed(&[HIGHLIGHT_FALSE_ACTION]), + actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_FALSE_ACTION]), default: true, default_enabled: true, }, From 47a3dcf90701678c2ead4edbad8832c6700a86c8 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 19 Sep 2022 10:17:55 +0100 Subject: [PATCH 07/35] Apply suggestions from code review Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- rust/src/push/mod.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/rust/src/push/mod.rs b/rust/src/push/mod.rs index dcbe9aeec0f0..848c61ff1b6f 100644 --- a/rust/src/push/mod.rs +++ b/rust/src/push/mod.rs @@ -59,13 +59,19 @@ fn get_base_rule_ids() -> HashSet<&'static str> { #[derive(Debug, Clone)] #[pyclass(frozen)] pub struct PushRule { + /// A unique ID for this rule pub rule_id: Cow<'static, str>, + /// The "kind" of push rule this is (see `PRIORITY_CLASS_MAP` in Python) #[pyo3(get)] pub priority_class: i32, + /// The conditions that must all match for actions to be applied pub conditions: Cow<'static, [Condition]>, + /// The actions to apply if all conditions are met pub actions: Cow<'static, [Action]>, + /// Whether this is a base rule #[pyo3(get)] pub default: bool, + /// Whether this is enabled by default #[pyo3(get)] pub default_enabled: bool, } From 68689fa8f57e721e94d8b6adce3fba1626679307 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 19 Sep 2022 10:18:11 +0100 Subject: [PATCH 08/35] Apply suggestions from code review Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- rust/src/push/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/src/push/mod.rs b/rust/src/push/mod.rs index 848c61ff1b6f..6e0c1034a189 100644 --- a/rust/src/push/mod.rs +++ b/rust/src/push/mod.rs @@ -147,7 +147,7 @@ pub struct SetTweak { #[serde(skip_serializing_if = "Option::is_none")] value: Option, - // This picks saves any other fields that may have been added as clients. + // This picks up any other fields that may have been added by clients. // These get added when we convert the `Action` to a python object. #[serde(flatten)] other_keys: Value, From a109a7756560b4d3442690005804fb1678689c7a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 19 Sep 2022 10:18:54 +0100 Subject: [PATCH 09/35] Remove newsfile --- changelog.d/13720.misc | 1 - 1 file changed, 1 deletion(-) delete mode 100644 changelog.d/13720.misc diff --git a/changelog.d/13720.misc b/changelog.d/13720.misc deleted file mode 100644 index af5de96b0447..000000000000 --- a/changelog.d/13720.misc +++ /dev/null @@ -1 +0,0 @@ -Speed up push rule evaluation in rooms with large numbers of local users. From dbeb21b9dfe5084e14f948dd690b827c447c03c7 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 19 Sep 2022 10:20:03 +0100 Subject: [PATCH 10/35] Remove old todo --- synapse/storage/databases/main/push_rule.py | 1 - 1 file changed, 1 deletion(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 9d76e9b87979..ed17b2e70c04 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -85,7 +85,6 @@ def _load_rules( ruleslist, ) - # TODO: Experimental config filtered_rules = FilteredPushRules( push_rules, enabled_map, From 0e10bb36d3daa05c5139b793e47cc94566d2ad91 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 19 Sep 2022 10:21:15 +0100 Subject: [PATCH 11/35] Swap actions --- rust/src/push/base_rules.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/src/push/base_rules.rs b/rust/src/push/base_rules.rs index aaedb2aab10f..1fd09e5fcff3 100644 --- a/rust/src/push/base_rules.rs +++ b/rust/src/push/base_rules.rs @@ -194,7 +194,7 @@ pub const BASE_APPEND_CONTENT_RULES: &[PushRule] = &[PushRule { pattern: None, pattern_type: Some(Cow::Borrowed("user_localpart")), })]), - actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_ACTION, SOUND_ACTION]), + actions: Cow::Borrowed(&[Action::Notify, SOUND_ACTION, HIGHLIGHT_ACTION]), default: true, default_enabled: true, }]; From 1e0584325d277551817ed50744949860d0077a51 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 19 Sep 2022 10:22:26 +0100 Subject: [PATCH 12/35] Add back module docstring --- rust/src/push/mod.rs | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/rust/src/push/mod.rs b/rust/src/push/mod.rs index 6e0c1034a189..2dff2202c2b3 100644 --- a/rust/src/push/mod.rs +++ b/rust/src/push/mod.rs @@ -17,6 +17,40 @@ //! The `Cow<_>` type is used extensively within this module to allow creating //! the base rules as constants (in Rust constants can't require explicit //! allocation atm). +//! +//! --- +//! +//! 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. use std::borrow::Cow; use std::collections::{BTreeMap, HashMap, HashSet}; From 415bbd691fb10d44634522beaeabe30b69882fb8 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 19 Sep 2022 10:23:58 +0100 Subject: [PATCH 13/35] Remove debug logging --- synapse/push/bulk_push_rule_evaluator.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index f82f15ec4492..dafd4730f246 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -287,8 +287,6 @@ async def action_for_event_by_user( if relation.rel_type == RelationTypes.THREAD: thread_id = relation.parent_id - logger.info("Flatten map: %s", _flatten_dict(event)) - logger.info("room_member_count: %s", room_member_count) evaluator = PushRuleEvaluatorForEvent( event, room_member_count, From 86515a9e54f02cb06659a02e093cdc0e7972ef11 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 19 Sep 2022 10:27:43 +0100 Subject: [PATCH 14/35] Handle unrecognized actions --- rust/src/push/mod.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/rust/src/push/mod.rs b/rust/src/push/mod.rs index 2dff2202c2b3..c8c9c15da7ac 100644 --- a/rust/src/push/mod.rs +++ b/rust/src/push/mod.rs @@ -162,6 +162,9 @@ pub enum Action { Notify, Coalesce, SetTweak(SetTweak), + + // An unrecognized custom action. + Unknown(Value), } impl IntoPy for Action { @@ -208,6 +211,7 @@ impl Serialize for Action { Action::Notify => serializer.serialize_str("notify"), Action::Coalesce => serializer.serialize_str("coalesce"), Action::SetTweak(tweak) => tweak.serialize(serializer), + Action::Unknown(value) => value.serialize(serializer), } } } @@ -218,6 +222,7 @@ impl Serialize for Action { enum ActionDeserializeHelper { Str(String), SetTweak(SetTweak), + Unknown(Value), } impl<'de> Deserialize<'de> for Action { @@ -234,6 +239,7 @@ impl<'de> Deserialize<'de> for Action { _ => Err(D::Error::custom("unrecognized action")), }, ActionDeserializeHelper::SetTweak(set_tweak) => Ok(Action::SetTweak(set_tweak)), + ActionDeserializeHelper::Unknown(value) => Ok(Action::Unknown(value)), } } } @@ -459,3 +465,14 @@ fn test_deserialize_action() { let _: Action = serde_json::from_str(r#""coalesce""#).unwrap(); let _: Action = serde_json::from_str(r#"{"set_tweak": "highlight"}"#).unwrap(); } + +#[test] +fn test_custom_action() { + let json = r#"{"some_custom":"action_fields"}"#; + + let action: Action = serde_json::from_str(json).unwrap(); + assert!(matches!(action, Action::Unknown(_))); + + let new_json = serde_json::to_string(&action).unwrap(); + assert_eq!(json, new_json); +} From b57be51f5a6adcd5fdb2ad9d9dce4d75b296cfdc Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 19 Sep 2022 11:00:31 +0100 Subject: [PATCH 15/35] Correctly handle unknwon conditions --- rust/src/push/base_rules.rs | 157 ++++++++++++++++++++---------------- rust/src/push/mod.rs | 30 ++++++- 2 files changed, 113 insertions(+), 74 deletions(-) diff --git a/rust/src/push/base_rules.rs b/rust/src/push/base_rules.rs index 1fd09e5fcff3..7c62bc48490f 100644 --- a/rust/src/push/base_rules.rs +++ b/rust/src/push/base_rules.rs @@ -20,6 +20,7 @@ use std::collections::HashMap; use lazy_static::lazy_static; use serde_json::Value; +use super::KnownCondition; use crate::push::Action; use crate::push::Condition; use crate::push::EventMatchCondition; @@ -64,11 +65,13 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[ PushRule { rule_id: Cow::Borrowed("global/override/.m.rule.suppress_notices"), priority_class: 5, - conditions: Cow::Borrowed(&[Condition::EventMatch(EventMatchCondition { - key: Cow::Borrowed("content.msgtype"), - pattern: Some(Cow::Borrowed("m.notice")), - pattern_type: None, - })]), + conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::EventMatch( + EventMatchCondition { + key: Cow::Borrowed("content.msgtype"), + pattern: Some(Cow::Borrowed("m.notice")), + pattern_type: None, + }, + ))]), actions: Cow::Borrowed(&[Action::DontNotify]), default: true, default_enabled: true, @@ -77,21 +80,21 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[ rule_id: Cow::Borrowed("global/override/.m.rule.invite_for_me"), priority_class: 5, conditions: Cow::Borrowed(&[ - Condition::EventMatch(EventMatchCondition { + Condition::Known(KnownCondition::EventMatch(EventMatchCondition { key: Cow::Borrowed("type"), pattern: Some(Cow::Borrowed("m.room.member")), pattern_type: None, - }), - Condition::EventMatch(EventMatchCondition { + })), + Condition::Known(KnownCondition::EventMatch(EventMatchCondition { key: Cow::Borrowed("content.membership"), pattern: Some(Cow::Borrowed("invite")), pattern_type: None, - }), - Condition::EventMatch(EventMatchCondition { + })), + Condition::Known(KnownCondition::EventMatch(EventMatchCondition { key: Cow::Borrowed("state_key"), pattern: None, pattern_type: Some(Cow::Borrowed("user_id")), - }), + })), ]), actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_FALSE_ACTION, SOUND_ACTION]), default: true, @@ -100,11 +103,13 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[ PushRule { rule_id: Cow::Borrowed("global/override/.m.rule.member_event"), priority_class: 5, - conditions: Cow::Borrowed(&[Condition::EventMatch(EventMatchCondition { - key: Cow::Borrowed("type"), - pattern: Some(Cow::Borrowed("m.room.member")), - pattern_type: None, - })]), + conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::EventMatch( + EventMatchCondition { + key: Cow::Borrowed("type"), + pattern: Some(Cow::Borrowed("m.room.member")), + pattern_type: None, + }, + ))]), actions: Cow::Borrowed(&[Action::DontNotify]), default: true, default_enabled: true, @@ -112,7 +117,7 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[ PushRule { rule_id: Cow::Borrowed("global/override/.m.rule.contains_display_name"), priority_class: 5, - conditions: Cow::Borrowed(&[Condition::ContainsDisplayName]), + conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::ContainsDisplayName)]), actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_ACTION, SOUND_ACTION]), default: true, default_enabled: true, @@ -121,14 +126,14 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[ rule_id: Cow::Borrowed("global/override/.m.rule.roomnotif"), priority_class: 5, conditions: Cow::Borrowed(&[ - Condition::SenderNotificationPermission { + Condition::Known(KnownCondition::SenderNotificationPermission { key: Cow::Borrowed("room"), - }, - Condition::EventMatch(EventMatchCondition { + }), + Condition::Known(KnownCondition::EventMatch(EventMatchCondition { key: Cow::Borrowed("content.body"), pattern: Some(Cow::Borrowed("@room")), pattern_type: None, - }), + })), ]), actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_ACTION]), default: true, @@ -138,16 +143,16 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[ rule_id: Cow::Borrowed("global/override/.m.rule.tombstone"), priority_class: 5, conditions: Cow::Borrowed(&[ - Condition::EventMatch(EventMatchCondition { + Condition::Known(KnownCondition::EventMatch(EventMatchCondition { key: Cow::Borrowed("type"), pattern: Some(Cow::Borrowed("m.room.tombstone")), pattern_type: None, - }), - Condition::EventMatch(EventMatchCondition { + })), + Condition::Known(KnownCondition::EventMatch(EventMatchCondition { key: Cow::Borrowed("state_key"), pattern: Some(Cow::Borrowed("")), pattern_type: None, - }), + })), ]), actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_ACTION]), default: true, @@ -156,11 +161,13 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[ PushRule { rule_id: Cow::Borrowed("global/override/.m.rule.reaction"), priority_class: 5, - conditions: Cow::Borrowed(&[Condition::EventMatch(EventMatchCondition { - key: Cow::Borrowed("type"), - pattern: Some(Cow::Borrowed("m.reaction")), - pattern_type: None, - })]), + conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::EventMatch( + EventMatchCondition { + key: Cow::Borrowed("type"), + pattern: Some(Cow::Borrowed("m.reaction")), + pattern_type: None, + }, + ))]), actions: Cow::Borrowed(&[Action::DontNotify]), default: true, default_enabled: true, @@ -169,16 +176,16 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[ rule_id: Cow::Borrowed("global/override/.org.matrix.msc3786.rule.room.server_acl"), priority_class: 5, conditions: Cow::Borrowed(&[ - Condition::EventMatch(EventMatchCondition { + Condition::Known(KnownCondition::EventMatch(EventMatchCondition { key: Cow::Borrowed("type"), pattern: Some(Cow::Borrowed("m.room.server_acl")), pattern_type: None, - }), - Condition::EventMatch(EventMatchCondition { + })), + Condition::Known(KnownCondition::EventMatch(EventMatchCondition { key: Cow::Borrowed("state_key"), pattern: Some(Cow::Borrowed("")), pattern_type: None, - }), + })), ]), actions: Cow::Borrowed(&[]), default: true, @@ -189,12 +196,14 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[ pub const BASE_APPEND_CONTENT_RULES: &[PushRule] = &[PushRule { rule_id: Cow::Borrowed("global/content/.m.rule.contains_user_name"), priority_class: 4, - conditions: Cow::Borrowed(&[Condition::EventMatch(EventMatchCondition { - key: Cow::Borrowed("content.body"), - pattern: None, - pattern_type: Some(Cow::Borrowed("user_localpart")), - })]), - actions: Cow::Borrowed(&[Action::Notify, SOUND_ACTION, HIGHLIGHT_ACTION]), + conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::EventMatch( + EventMatchCondition { + key: Cow::Borrowed("content.body"), + pattern: None, + pattern_type: Some(Cow::Borrowed("user_localpart")), + }, + ))]), + actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_ACTION, SOUND_ACTION]), default: true, default_enabled: true, }]; @@ -203,11 +212,13 @@ pub const BASE_APPEND_UNDERRIDE_RULES: &[PushRule] = &[ PushRule { rule_id: Cow::Borrowed("global/underride/.m.rule.call"), priority_class: 1, - conditions: Cow::Borrowed(&[Condition::EventMatch(EventMatchCondition { - key: Cow::Borrowed("type"), - pattern: Some(Cow::Borrowed("m.call.invite")), - pattern_type: None, - })]), + conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::EventMatch( + EventMatchCondition { + key: Cow::Borrowed("type"), + pattern: Some(Cow::Borrowed("m.call.invite")), + pattern_type: None, + }, + ))]), actions: Cow::Borrowed(&[Action::Notify, RING_ACTION, HIGHLIGHT_FALSE_ACTION]), default: true, default_enabled: true, @@ -216,14 +227,14 @@ pub const BASE_APPEND_UNDERRIDE_RULES: &[PushRule] = &[ rule_id: Cow::Borrowed("global/underride/.m.rule.room_one_to_one"), priority_class: 1, conditions: Cow::Borrowed(&[ - Condition::EventMatch(EventMatchCondition { + Condition::Known(KnownCondition::EventMatch(EventMatchCondition { key: Cow::Borrowed("type"), pattern: Some(Cow::Borrowed("m.room.message")), pattern_type: None, - }), - Condition::RoomMemberCount { + })), + Condition::Known(KnownCondition::RoomMemberCount { is: Some(Cow::Borrowed("2")), - }, + }), ]), actions: Cow::Borrowed(&[Action::Notify, SOUND_ACTION, HIGHLIGHT_FALSE_ACTION]), default: true, @@ -233,14 +244,14 @@ pub const BASE_APPEND_UNDERRIDE_RULES: &[PushRule] = &[ rule_id: Cow::Borrowed("global/underride/.m.rule.encrypted_room_one_to_one"), priority_class: 1, conditions: Cow::Borrowed(&[ - Condition::EventMatch(EventMatchCondition { + Condition::Known(KnownCondition::EventMatch(EventMatchCondition { key: Cow::Borrowed("type"), pattern: Some(Cow::Borrowed("m.room.encrypted")), pattern_type: None, - }), - Condition::RoomMemberCount { + })), + Condition::Known(KnownCondition::RoomMemberCount { is: Some(Cow::Borrowed("2")), - }, + }), ]), actions: Cow::Borrowed(&[Action::Notify, SOUND_ACTION, HIGHLIGHT_FALSE_ACTION]), default: true, @@ -249,11 +260,11 @@ pub const BASE_APPEND_UNDERRIDE_RULES: &[PushRule] = &[ PushRule { rule_id: Cow::Borrowed("global/underride/.org.matrix.msc3772.thread_reply"), priority_class: 1, - conditions: Cow::Borrowed(&[Condition::RelationMatch { + conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::RelationMatch { rel_type: Cow::Borrowed("m.thread"), sender: None, sender_type: Some(Cow::Borrowed("user_id")), - }]), + })]), actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_FALSE_ACTION]), default: true, default_enabled: true, @@ -261,11 +272,13 @@ pub const BASE_APPEND_UNDERRIDE_RULES: &[PushRule] = &[ PushRule { rule_id: Cow::Borrowed("global/underride/.m.rule.message"), priority_class: 1, - conditions: Cow::Borrowed(&[Condition::EventMatch(EventMatchCondition { - key: Cow::Borrowed("type"), - pattern: Some(Cow::Borrowed("m.room.message")), - pattern_type: None, - })]), + conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::EventMatch( + EventMatchCondition { + key: Cow::Borrowed("type"), + pattern: Some(Cow::Borrowed("m.room.message")), + pattern_type: None, + }, + ))]), actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_FALSE_ACTION]), default: true, default_enabled: true, @@ -273,11 +286,13 @@ pub const BASE_APPEND_UNDERRIDE_RULES: &[PushRule] = &[ PushRule { rule_id: Cow::Borrowed("global/underride/.m.rule.encrypted"), priority_class: 1, - conditions: Cow::Borrowed(&[Condition::EventMatch(EventMatchCondition { - key: Cow::Borrowed("type"), - pattern: Some(Cow::Borrowed("m.room.encrypted")), - pattern_type: None, - })]), + conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::EventMatch( + EventMatchCondition { + key: Cow::Borrowed("type"), + pattern: Some(Cow::Borrowed("m.room.encrypted")), + pattern_type: None, + }, + ))]), actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_FALSE_ACTION]), default: true, default_enabled: true, @@ -286,21 +301,21 @@ pub const BASE_APPEND_UNDERRIDE_RULES: &[PushRule] = &[ rule_id: Cow::Borrowed("global/underride/.im.vector.jitsi"), priority_class: 1, conditions: Cow::Borrowed(&[ - Condition::EventMatch(EventMatchCondition { + Condition::Known(KnownCondition::EventMatch(EventMatchCondition { key: Cow::Borrowed("type"), pattern: Some(Cow::Borrowed("im.vector.modular.widgets")), pattern_type: None, - }), - Condition::EventMatch(EventMatchCondition { + })), + Condition::Known(KnownCondition::EventMatch(EventMatchCondition { key: Cow::Borrowed("content.type"), pattern: Some(Cow::Borrowed("jitsi")), pattern_type: None, - }), - Condition::EventMatch(EventMatchCondition { + })), + Condition::Known(KnownCondition::EventMatch(EventMatchCondition { key: Cow::Borrowed("state_key"), pattern: Some(Cow::Borrowed("*")), pattern_type: None, - }), + })), ]), actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_FALSE_ACTION]), default: true, diff --git a/rust/src/push/mod.rs b/rust/src/push/mod.rs index c8c9c15da7ac..de6764e7c5c7 100644 --- a/rust/src/push/mod.rs +++ b/rust/src/push/mod.rs @@ -245,10 +245,23 @@ impl<'de> Deserialize<'de> for Action { } /// A condition used in push rules to match against an event. +/// +/// We need this split as `serde` doesn't give us the ability to have a +/// "catchall" variant in tagged enums. +#[derive(Serialize, Deserialize, Debug, Clone)] +#[serde(untagged)] +pub enum Condition { + /// A recognized condition that we can match against + Known(KnownCondition), + /// An unrecognized condition that we ignore. + Unknown(Value), +} + +/// The set of "known" conditions that we can handle. #[derive(Serialize, Deserialize, Debug, Clone)] #[serde(rename_all = "snake_case")] #[serde(tag = "kind")] -pub enum Condition { +pub enum KnownCondition { EventMatch(EventMatchCondition), ContainsDisplayName, RoomMemberCount { @@ -438,11 +451,11 @@ impl FilteredPushRules { #[test] fn test_serialize_condition() { - let condition = Condition::EventMatch(EventMatchCondition { + let condition = Condition::Known(KnownCondition::EventMatch(EventMatchCondition { key: "content.body".into(), pattern: Some("coffee".into()), pattern_type: None, - }); + })); let json = serde_json::to_string(&condition).unwrap(); assert_eq!( @@ -458,6 +471,17 @@ fn test_deserialize_condition() { let _: Condition = serde_json::from_str(json).unwrap(); } +#[test] +fn test_deserialize_custom_condition() { + let json = r#"{"kind":"custom_tag"}"#; + + let condition: Condition = serde_json::from_str(json).unwrap(); + assert!(matches!(condition, Condition::Unknown(_))); + + let new_json = serde_json::to_string(&condition).unwrap(); + assert_eq!(json, new_json); +} + #[test] fn test_deserialize_action() { let _: Action = serde_json::from_str(r#""notify""#).unwrap(); From a691a2dbdce27cbbecada278c384c74132e3b914 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 19 Sep 2022 11:01:08 +0100 Subject: [PATCH 16/35] Lint --- synapse/push/bulk_push_rule_evaluator.py | 1 - 1 file changed, 1 deletion(-) diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index dafd4730f246..404379ef67d3 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -34,7 +34,6 @@ from synapse.event_auth import auth_types_for_event, get_user_power_level from synapse.events import EventBase, relation_from_event from synapse.events.snapshot import EventContext -from synapse.push.push_rule_evaluator import _flatten_dict from synapse.state import POWER_KEY from synapse.storage.databases.main.roommember import EventIdMembership from synapse.storage.state import StateFilter From b7308765a7f4c18d94d7652c599610d948d0d53d Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 19 Sep 2022 12:17:01 +0100 Subject: [PATCH 17/35] Implement push rule evaluation in Rust --- rust/src/push/base_rules.rs | 1 + rust/src/push/evaluator.rs | 336 +++++++++++++++++++++ rust/src/push/mod.rs | 15 +- rust/src/push/utils.rs | 128 ++++++++ stubs/synapse/synapse_rust/push.pyi | 19 +- synapse/push/bulk_push_rule_evaluator.py | 44 +-- synapse/push/httppusher.py | 39 ++- synapse/push/push_rule_evaluator.py | 361 ----------------------- tests/push/test_push_rule_evaluator.py | 17 +- 9 files changed, 569 insertions(+), 391 deletions(-) create mode 100644 rust/src/push/evaluator.rs create mode 100644 rust/src/push/utils.rs delete mode 100644 synapse/push/push_rule_evaluator.py diff --git a/rust/src/push/base_rules.rs b/rust/src/push/base_rules.rs index 7c62bc48490f..bb59676bdea9 100644 --- a/rust/src/push/base_rules.rs +++ b/rust/src/push/base_rules.rs @@ -262,6 +262,7 @@ pub const BASE_APPEND_UNDERRIDE_RULES: &[PushRule] = &[ priority_class: 1, conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::RelationMatch { rel_type: Cow::Borrowed("m.thread"), + event_type_pattern: None, sender: None, sender_type: Some(Cow::Borrowed("user_id")), })]), diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs new file mode 100644 index 000000000000..e6caebc49fd3 --- /dev/null +++ b/rust/src/push/evaluator.rs @@ -0,0 +1,336 @@ +use std::collections::{BTreeMap, BTreeSet}; + +use anyhow::{Context, Error}; +use lazy_static::lazy_static; +use log::warn; +use pyo3::prelude::*; +use regex::Regex; + +use super::{ + utils::{get_localpart_from_id, glob_to_regex, GlobMatchType}, + Action, Condition, EventMatchCondition, FilteredPushRules, KnownCondition, +}; + +lazy_static! { + static ref INEQUALITY_EXPR: Regex = Regex::new(r"^([=<>]*)([0-9]*)$").expect("valid regex"); +} + +/// Allows running a set of push rules against a particular event. +#[pyclass] +pub struct PushRuleEvaluator { + /// A mapping of "flattened" keys to string values in the event, e.g. + /// includes things liek "type" and "content.msgtype". + flattened_keys: BTreeMap, + + /// The "content.body", if any. + body: String, + + /// The number of users in the room. + room_member_count: u64, + + /// The `notifications` section of the current power levels in the room. + notification_power_levels: BTreeMap, + + /// The relations related to the event as a mapping from relation type to + /// set of sender/event type 2-tuples. + relations: BTreeMap>, + + /// Is running "relation" conditions enabled? + relation_match_enabled: bool, + + /// The power level of the sender of the event + sender_power_level: i64, +} + +#[pymethods] +impl PushRuleEvaluator { + /// Create a new `PushRuleEvaluator`. See struct docstring for details. + #[new] + fn py_new( + flattened_keys: BTreeMap, + room_member_count: u64, + sender_power_level: i64, + notification_power_levels: BTreeMap, + relations: BTreeMap>, + relation_match_enabled: bool, + ) -> Result { + let body = flattened_keys + .get("content.body") + .cloned() + .unwrap_or_default(); + + Ok(PushRuleEvaluator { + flattened_keys, + body, + room_member_count, + notification_power_levels, + relations, + relation_match_enabled, + sender_power_level, + }) + } + + /// Run the evaluator with the given push rules, for the given user ID and + /// display name of the user. + /// + /// Passing in None will skip evaluating rules matching user ID and display + /// name. + /// + /// Returns the set of actions, if any, that match (filtering out any + /// `dont_notify` actions). + fn run( + &self, + push_rules: &FilteredPushRules, + user_id: Option<&str>, + display_name: Option<&str>, + ) -> Vec { + 'outer: for (push_rule, enabled) in push_rules.iter() { + if !enabled { + continue; + } + + for condition in push_rule.conditions.iter() { + match self.match_condition(condition, user_id, display_name) { + Ok(true) => {} + Ok(false) => continue 'outer, + Err(err) => { + warn!("Condition match failed {err}"); + continue 'outer; + } + } + } + + let actions = push_rule + .actions + .iter() + // Filter out "dont_notify" actions, as we don't store them. + .filter(|a| **a != Action::DontNotify) + .cloned() + .collect(); + + return actions; + } + + Vec::new() + } + + /// Check if the given condition matches. + fn matches( + &self, + condition: Condition, + user_id: Option<&str>, + display_name: Option<&str>, + ) -> bool { + match self.match_condition(&condition, user_id, display_name) { + Ok(true) => true, + Ok(false) => false, + Err(err) => { + warn!("Condition match failed {err}"); + true + } + } + } +} + +impl PushRuleEvaluator { + /// Match a given `Condition` for a push rule. + pub fn match_condition( + &self, + condition: &Condition, + user_id: Option<&str>, + display_name: Option<&str>, + ) -> Result { + let known_condition = match condition { + Condition::Known(known) => known, + + // 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. + Condition::Unknown(_) => { + return Ok(true); + } + }; + + let result = match known_condition { + KnownCondition::EventMatch(event_match) => { + self.match_event_match(event_match, user_id)? + } + KnownCondition::ContainsDisplayName => { + if let Some(dn) = display_name { + if !dn.is_empty() { + let matcher = glob_to_regex(dn, GlobMatchType::Word)?; + matcher.is_match(&self.body) + } else { + // We specifically ignore empty display names, as otherwise + // they would always match. + false + } + } else { + false + } + } + KnownCondition::RoomMemberCount { is } => { + if let Some(is) = is { + self.match_member_count(is)? + } else { + false + } + } + KnownCondition::SenderNotificationPermission { key } => { + let required_level = self + .notification_power_levels + .get(key.as_ref()) + .copied() + .unwrap_or(50); + + self.sender_power_level >= required_level + } + KnownCondition::RelationMatch { + rel_type, + event_type_pattern, + sender, + sender_type, + } => { + if !self.relation_match_enabled { + return Ok(false); + } + + let relations = if let Some(relations) = self.relations.get(&**rel_type) { + relations + } else { + return Ok(false); + }; + + let sender_pattern = if let Some(sender) = sender { + Some(sender.as_ref()) + } else if let Some(sender_type) = sender_type { + if sender_type == "user_id" { + if let Some(user_id) = user_id { + Some(user_id) + } else { + return Ok(false); + } + } else { + warn!("Unrecognized sender_type: {sender_type}"); + return Ok(false); + } + } else { + None + }; + + let sender_compiled_pattern = if let Some(pattern) = sender_pattern { + Some(glob_to_regex(pattern, GlobMatchType::Whole)?) + } else { + None + }; + + let type_compiled_pattern = if let Some(pattern) = event_type_pattern { + Some(glob_to_regex(pattern, GlobMatchType::Whole)?) + } else { + None + }; + + for (relation_sender, event_type) in relations { + if let Some(pattern) = &sender_compiled_pattern { + if !pattern.is_match(relation_sender) { + continue; + } + } + + if let Some(pattern) = &type_compiled_pattern { + if !pattern.is_match(event_type) { + continue; + } + } + + return Ok(true); + } + + false + } + }; + + Ok(result) + } + + fn match_event_match( + &self, + event_match: &EventMatchCondition, + user_id: Option<&str>, + ) -> Result { + let pattern = if let Some(pattern) = &event_match.pattern { + pattern + } else if let Some(pattern_type) = &event_match.pattern_type { + let user_id = if let Some(user_id) = user_id { + user_id + } else { + return Ok(false); + }; + match &**pattern_type { + "user_id" => user_id, + "user_localpart" => get_localpart_from_id(user_id)?, + _ => return Ok(false), + } + } else { + return Ok(false); + }; + + // For the content.body we match against "words", but for everything + // else we match against the entire value. + let match_type = if event_match.key == "content.body" { + GlobMatchType::Word + } else { + GlobMatchType::Whole + }; + + if let Some(value) = self.flattened_keys.get(&*event_match.key) { + let compiled_pattern = glob_to_regex(pattern, match_type)?; + Ok(compiled_pattern.is_match(value)) + } else { + Ok(false) + } + } + + /// Match the member count against an 'is' condition + fn match_member_count(&self, is: &str) -> Result { + // The 'is' condition can be things like '>2', '==3' or event just '4'. + let captures = INEQUALITY_EXPR.captures(is).context("bad 'is' clause")?; + let ineq = captures.get(1).map(|m| m.as_str()).unwrap_or("=="); + let rhs: u64 = captures + .get(2) + .context("missing number")? + .as_str() + .parse()?; + + let matches = match ineq { + "" | "==" => self.room_member_count == rhs, + "<" => self.room_member_count < rhs, + ">" => self.room_member_count > rhs, + ">=" => self.room_member_count >= rhs, + "<=" => self.room_member_count <= rhs, + _ => false, + }; + + Ok(matches) + } +} + +#[test] +fn push_rule_evaluator() { + let mut flattened_keys = BTreeMap::new(); + flattened_keys.insert("content.body".to_string(), "foo bar bob hello".to_string()); + let evaluator = PushRuleEvaluator::py_new( + flattened_keys, + 10, + 0, + BTreeMap::new(), + BTreeMap::new(), + true, + ) + .unwrap(); + + let result = evaluator.run(&FilteredPushRules::default(), None, Some("bob")); + assert_eq!(result.len(), 3); +} diff --git a/rust/src/push/mod.rs b/rust/src/push/mod.rs index de6764e7c5c7..ff010821e5d1 100644 --- a/rust/src/push/mod.rs +++ b/rust/src/push/mod.rs @@ -58,12 +58,16 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use anyhow::{Context, Error}; use log::warn; use pyo3::prelude::*; -use pythonize::pythonize; +use pythonize::{depythonize, pythonize}; use serde::de::Error as _; use serde::{Deserialize, Serialize}; use serde_json::Value; +use self::evaluator::PushRuleEvaluator; + mod base_rules; +mod evaluator; +mod utils; /// Called when registering modules with python. pub fn register_module(py: Python<'_>, m: &PyModule) -> PyResult<()> { @@ -71,6 +75,7 @@ pub fn register_module(py: Python<'_>, m: &PyModule) -> PyResult<()> { child_module.add_class::()?; child_module.add_class::()?; child_module.add_class::()?; + child_module.add_class::()?; child_module.add_function(wrap_pyfunction!(get_base_rule_ids, m)?)?; m.add_submodule(child_module)?; @@ -274,6 +279,8 @@ pub enum KnownCondition { #[serde(rename = "org.matrix.msc3772.relation_match")] RelationMatch { rel_type: Cow<'static, str>, + #[serde(skip_serializing_if = "Option::is_none", rename = "type")] + event_type_pattern: Option>, #[serde(skip_serializing_if = "Option::is_none")] sender: Option>, #[serde(skip_serializing_if = "Option::is_none")] @@ -287,6 +294,12 @@ impl IntoPy for Condition { } } +impl<'source> FromPyObject<'source> for Condition { + fn extract(ob: &'source PyAny) -> PyResult { + Ok(depythonize(ob)?) + } +} + /// The body of a [`Condition::EventMatch`] #[derive(Serialize, Deserialize, Debug, Clone)] pub struct EventMatchCondition { diff --git a/rust/src/push/utils.rs b/rust/src/push/utils.rs new file mode 100644 index 000000000000..88d82b410a3a --- /dev/null +++ b/rust/src/push/utils.rs @@ -0,0 +1,128 @@ +use anyhow::bail; +use anyhow::Context; +use anyhow::Error; +use lazy_static::lazy_static; +use regex; +use regex::Regex; +use regex::RegexBuilder; + +lazy_static! { + /// Matches runs of "glob" style wild cards + static ref WILDCARD_RUN: Regex = Regex::new(r"([^\?\*]*)([\?\*]*)").expect("valid regex"); +} + +/// Extract the localpart from a Matrix style ID +pub(crate) fn get_localpart_from_id(id: &str) -> Result<&str, Error> { + let (localpart, _) = id + .split_once(':') + .with_context(|| format!("ID does not contain colon: {id}"))?; + + // We need to strip off the first character, which is the ID type. + if localpart.is_empty() { + bail!("Invalid ID {id}"); + } + + Ok(&localpart[1..]) +} + +/// Used by `glob_to_regex` to specify what to match the regex against. +pub(crate) enum GlobMatchType { + /// The generated regex will match against the entire input. + Whole, + /// The generated regex will match against words. + Word, +} + +/// Convert a "glob" style expression to a regex, anchoring either to the entire +/// input or to individual words. +pub(crate) fn glob_to_regex(glob: &str, match_type: GlobMatchType) -> Result { + let mut chunks = Vec::new(); + + // Patterns with wildcards must be simplified to avoid performance cliffs + // - The glob `?**?**?` is equivalent to the glob `???*` + // - The glob `???*` is equivalent to the regex `.{3,}` + for captures in WILDCARD_RUN.captures_iter(glob) { + if let Some(chunk) = captures.get(1) { + chunks.push(regex::escape(chunk.as_str())); + } + + if let Some(wildcards) = captures.get(2) { + if wildcards.as_str() == "" { + continue; + } + + let question_marks = wildcards.as_str().chars().filter(|c| *c == '?').count(); + + if wildcards.as_str().contains('*') { + chunks.push(format!(".{{{question_marks},}}")); + } else { + chunks.push(format!(".{{{question_marks}}}")); + } + } + } + + let joined = chunks.join(""); + + let regex_str = match match_type { + GlobMatchType::Whole => format!(r"\A{joined}\z"), + + // `^|\W` and `\W|$` handle the case where `pattern` starts or ends with a non-word + // character. + GlobMatchType::Word => format!(r"(?:^|\W|\b){joined}(?:\b|\W|$)"), + }; + + Ok(RegexBuilder::new(®ex_str) + .case_insensitive(true) + .build()?) +} + +#[test] +fn test_get_domain_from_id() { + get_localpart_from_id("").unwrap_err(); + get_localpart_from_id(":").unwrap_err(); + get_localpart_from_id(":asd").unwrap_err(); + get_localpart_from_id("::as::asad").unwrap_err(); + + assert_eq!(get_localpart_from_id("@test:foo").unwrap(), "test"); + assert_eq!(get_localpart_from_id("@:").unwrap(), ""); + assert_eq!(get_localpart_from_id("@test:foo:907").unwrap(), "test"); +} + +#[test] +fn tset_glob() -> Result<(), Error> { + assert_eq!( + glob_to_regex("simple", GlobMatchType::Whole)?.as_str(), + r"\Asimple\z" + ); + assert_eq!( + glob_to_regex("simple*", GlobMatchType::Whole)?.as_str(), + r"\Asimple.{0,}\z" + ); + assert_eq!( + glob_to_regex("simple?", GlobMatchType::Whole)?.as_str(), + r"\Asimple.{1}\z" + ); + assert_eq!( + glob_to_regex("simple?*?*", GlobMatchType::Whole)?.as_str(), + r"\Asimple.{2,}\z" + ); + assert_eq!( + glob_to_regex("simple???", GlobMatchType::Whole)?.as_str(), + r"\Asimple.{3}\z" + ); + + assert_eq!( + glob_to_regex("escape.", GlobMatchType::Whole)?.as_str(), + r"\Aescape\.\z" + ); + + assert_eq!( + glob_to_regex("simple", GlobMatchType::Word)?.as_str(), + r"\bsimple\b" + ); + + assert!(glob_to_regex("simple", GlobMatchType::Word)?.is_match("some simple.")); + assert!(glob_to_regex("simple", GlobMatchType::Word)?.is_match("simple")); + + Ok(()) +} diff --git a/stubs/synapse/synapse_rust/push.pyi b/stubs/synapse/synapse_rust/push.pyi index 93c4e69d424f..80853901ec2b 100644 --- a/stubs/synapse/synapse_rust/push.pyi +++ b/stubs/synapse/synapse_rust/push.pyi @@ -1,4 +1,4 @@ -from typing import Any, Collection, Dict, Mapping, Sequence, Tuple, Union +from typing import Any, Collection, Dict, Mapping, Optional, Sequence, Set, Tuple, Union from synapse.types import JsonDict @@ -35,3 +35,20 @@ class FilteredPushRules: def rules(self) -> Collection[Tuple[PushRule, bool]]: ... def get_base_rule_ids() -> Collection[str]: ... + +class PushRuleEvaluator: + def __init__( + self, + flattened_keys: Mapping[str, str], + room_member_count: int, + sender_power_level: int, + notification_power_levels: Mapping[str, int], + relations: Mapping[str, Set[Tuple[str, str]]], + relation_match_enabled: bool, + ): ... + def run( + self, + push_rules: FilteredPushRules, + user_id: Optional[str], + display_name: Optional[str], + ) -> Collection[dict]: ... diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index 404379ef67d3..8a0c9272ce45 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -17,6 +17,7 @@ import logging from typing import ( TYPE_CHECKING, + Any, Collection, Dict, Iterable, @@ -37,13 +38,11 @@ from synapse.state import POWER_KEY from synapse.storage.databases.main.roommember import EventIdMembership from synapse.storage.state import StateFilter -from synapse.synapse_rust.push import FilteredPushRules, PushRule +from synapse.synapse_rust.push import FilteredPushRules, PushRule, PushRuleEvaluator from synapse.util.caches import register_cache from synapse.util.metrics import measure_func from synapse.visibility import filter_event_for_clients_with_state -from .push_rule_evaluator import PushRuleEvaluatorForEvent - if TYPE_CHECKING: from synapse.server import HomeServer @@ -286,11 +285,11 @@ async def action_for_event_by_user( if relation.rel_type == RelationTypes.THREAD: thread_id = relation.parent_id - evaluator = PushRuleEvaluatorForEvent( - event, + evaluator = PushRuleEvaluator( + _flatten_dict(event), room_member_count, sender_power_level, - power_levels, + power_levels.get("notifications", {}), relations, self._relations_match_enabled, ) @@ -334,17 +333,10 @@ async def action_for_event_by_user( # current user, it'll be added to the dict later. actions_by_user[uid] = [] - for rule, enabled in rules.rules(): - if not enabled: - continue - - matches = evaluator.check_conditions(rule.conditions, uid, display_name) - if matches: - 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 - break + actions = evaluator.run(rules, uid, display_name) + if actions and "notify" in actions: + # Push rules say we should notify the user of this event + actions_by_user[uid] = actions # Mark in the DB staging area the push actions for users who should be # notified for this event. (This will then get handled when we persist @@ -361,3 +353,21 @@ async def action_for_event_by_user( Rule = Dict[str, dict] RulesByUser = Dict[str, List[Rule]] StateGroup = Union[object, int] + + +def _flatten_dict( + d: Union[EventBase, Mapping[str, Any]], + prefix: Optional[List[str]] = None, + result: Optional[Dict[str, str]] = None, +) -> Dict[str, str]: + if prefix is None: + prefix = [] + if result is None: + result = {} + for key, value in d.items(): + if isinstance(value, str): + result[".".join(prefix + [key])] = value.lower() + elif isinstance(value, Mapping): + _flatten_dict(value, prefix=(prefix + [key]), result=result) + + return result diff --git a/synapse/push/httppusher.py b/synapse/push/httppusher.py index e96fb45e9f55..b048b03a7477 100644 --- a/synapse/push/httppusher.py +++ b/synapse/push/httppusher.py @@ -14,7 +14,7 @@ # limitations under the License. import logging import urllib.parse -from typing import TYPE_CHECKING, Any, Dict, Iterable, Optional, Union +from typing import TYPE_CHECKING, Any, Dict, Iterable, List, Optional, Union from prometheus_client import Counter @@ -28,7 +28,7 @@ from synapse.push import Pusher, PusherConfig, PusherConfigException from synapse.storage.databases.main.event_push_actions import HttpPushAction -from . import push_rule_evaluator, push_tools +from . import push_tools if TYPE_CHECKING: from synapse.server import HomeServer @@ -56,6 +56,39 @@ ) +def tweaks_for_actions(actions: List[Union[str, Dict]]) -> Dict[str, Any]: + """ + Converts a list of actions into a `tweaks` dict (which can then be passed to + the push gateway). + + This function ignores all actions other than `set_tweak` actions, and treats + absent `value`s as `True`, which agrees with the only spec-defined treatment + of absent `value`s (namely, for `highlight` tweaks). + + Args: + actions: list of actions + e.g. [ + {"set_tweak": "a", "value": "AAA"}, + {"set_tweak": "b", "value": "BBB"}, + {"set_tweak": "highlight"}, + "notify" + ] + + Returns: + dictionary of tweaks for those actions + e.g. {"a": "AAA", "b": "BBB", "highlight": True} + """ + tweaks = {} + for a in actions: + if not isinstance(a, dict): + continue + if "set_tweak" in a: + # value is allowed to be absent in which case the value assumed + # should be True. + tweaks[a["set_tweak"]] = a.get("value", True) + return tweaks + + class HttpPusher(Pusher): INITIAL_BACKOFF_SEC = 1 # in seconds because that's what Twisted takes MAX_BACKOFF_SEC = 60 * 60 @@ -281,7 +314,7 @@ async def _process_one(self, push_action: HttpPushAction) -> bool: if "notify" not in push_action.actions: return True - tweaks = push_rule_evaluator.tweaks_for_actions(push_action.actions) + tweaks = tweaks_for_actions(push_action.actions) badge = await push_tools.get_badge_count( self.hs.get_datastores().main, self.user_id, diff --git a/synapse/push/push_rule_evaluator.py b/synapse/push/push_rule_evaluator.py deleted file mode 100644 index 3c5632cd9153..000000000000 --- a/synapse/push/push_rule_evaluator.py +++ /dev/null @@ -1,361 +0,0 @@ -# Copyright 2015, 2016 OpenMarket Ltd -# Copyright 2017 New Vector Ltd -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# 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. - -import logging -import re -from typing import ( - Any, - Dict, - List, - Mapping, - Optional, - Pattern, - Sequence, - Set, - Tuple, - Union, -) - -from matrix_common.regex import glob_to_regex, to_word_pattern - -from synapse.events import EventBase -from synapse.types import UserID -from synapse.util.caches.lrucache import LruCache - -logger = logging.getLogger(__name__) - - -GLOB_REGEX = re.compile(r"\\\[(\\\!|)(.*)\\\]") -IS_GLOB = re.compile(r"[\?\*\[\]]") -INEQUALITY_EXPR = re.compile("^([=<>]*)([0-9]*)$") - - -def _room_member_count( - 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: Mapping[str, Any], - sender_power_level: int, - power_levels: Dict[str, Union[int, Dict[str, int]]], -) -> bool: - notif_level_key = condition.get("key") - if notif_level_key is None: - return False - - notif_levels = power_levels.get("notifications", {}) - assert isinstance(notif_levels, dict) - room_notif_level = notif_levels.get(notif_level_key, 50) - - return sender_power_level >= room_notif_level - - -def _test_ineq_condition(condition: Mapping[str, Any], number: int) -> bool: - if "is" not in condition: - return False - m = INEQUALITY_EXPR.match(condition["is"]) - if not m: - return False - ineq = m.group(1) - rhs = m.group(2) - if not rhs.isdigit(): - return False - rhs_int = int(rhs) - - if ineq == "" or ineq == "==": - return number == rhs_int - elif ineq == "<": - return number < rhs_int - elif ineq == ">": - return number > rhs_int - elif ineq == ">=": - return number >= rhs_int - elif ineq == "<=": - return number <= rhs_int - else: - return False - - -def tweaks_for_actions(actions: List[Union[str, Dict]]) -> Dict[str, Any]: - """ - Converts a list of actions into a `tweaks` dict (which can then be passed to - the push gateway). - - This function ignores all actions other than `set_tweak` actions, and treats - absent `value`s as `True`, which agrees with the only spec-defined treatment - of absent `value`s (namely, for `highlight` tweaks). - - Args: - actions: list of actions - e.g. [ - {"set_tweak": "a", "value": "AAA"}, - {"set_tweak": "b", "value": "BBB"}, - {"set_tweak": "highlight"}, - "notify" - ] - - Returns: - dictionary of tweaks for those actions - e.g. {"a": "AAA", "b": "BBB", "highlight": True} - """ - tweaks = {} - for a in actions: - if not isinstance(a, dict): - continue - if "set_tweak" in a: - # value is allowed to be absent in which case the value assumed - # should be True. - tweaks[a["set_tweak"]] = a.get("value", True) - return tweaks - - -class PushRuleEvaluatorForEvent: - def __init__( - self, - event: EventBase, - room_member_count: int, - sender_power_level: int, - power_levels: Dict[str, Union[int, Dict[str, int]]], - relations: Dict[str, Set[Tuple[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) - - # Maps cache keys to final values. - self._condition_cache: Dict[str, bool] = {} - - def check_conditions( - 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. - - Args: - conditions: The user's conditions to match. - uid: The user's MXID. - display_name: The display name. - - Returns: - True if all conditions match the event, False otherwise. - """ - for cond in conditions: - _cache_key = cond.get("_cache_key", None) - if _cache_key: - res = self._condition_cache.get(_cache_key, None) - if res is False: - return False - elif res is True: - continue - - res = self.matches(cond, uid, display_name) - if _cache_key: - self._condition_cache[_cache_key] = bool(res) - - if not res: - return False - - return True - - def matches( - 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. - - Args: - condition: The user's condition to match. - uid: The user's MXID. - display_name: The display name, or None if there is not one. - - Returns: - True if the condition matches the event, False otherwise. - """ - if condition["kind"] == "event_match": - return self._event_match(condition, user_id) - elif condition["kind"] == "contains_display_name": - return self._contains_display_name(display_name) - elif condition["kind"] == "room_member_count": - return _room_member_count(self._event, condition, self._room_member_count) - elif condition["kind"] == "sender_notification_permission": - return _sender_notification_permission( - self._event, condition, self._sender_power_level, self._power_levels - ) - elif ( - condition["kind"] == "org.matrix.msc3772.relation_match" - and self._relations_match_enabled - ): - 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: Mapping, user_id: str) -> bool: - """ - Check an "event_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. - """ - pattern = condition.get("pattern", None) - - if not pattern: - pattern_type = condition.get("pattern_type", None) - if pattern_type == "user_id": - pattern = user_id - elif pattern_type == "user_localpart": - pattern = UserID.from_string(user_id).localpart - - if not pattern: - logger.warning("event_match condition with no pattern") - return False - - # XXX: optimisation: cache our pattern regexps - if condition["key"] == "content.body": - body = self._event.content.get("body", None) - if not body or not isinstance(body, str): - return False - - return _glob_matches(pattern, body, word_boundary=True) - else: - haystack = self._value_cache.get(condition["key"], None) - if haystack is None: - return False - - return _glob_matches(pattern, haystack) - - def _contains_display_name(self, display_name: Optional[str]) -> bool: - """ - Check an "event_match" push rule condition. - - Args: - display_name: The display name, or None if there is not one. - - Returns: - True if the display name is found in the event body, False otherwise. - """ - if not display_name: - return False - - body = self._event.content.get("body", None) - if not body or not isinstance(body, str): - return False - - # Similar to _glob_matches, but do not treat display_name as a glob. - r = regex_cache.get((display_name, False, True), None) - if not r: - r1 = re.escape(display_name) - r1 = to_word_pattern(r1) - r = re.compile(r1, flags=re.IGNORECASE) - regex_cache[(display_name, False, True)] = r - - return bool(r.search(body)) - - def _relation_match(self, condition: Mapping, 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 = 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") - if sender_type == "user_id": - sender_pattern = user_id - type_pattern = condition.get("type") - - # If any other relations matches, return True. - for sender, event_type in self._relations.get(rel_type, ()): - if sender_pattern and not _glob_matches(sender_pattern, sender): - continue - if type_pattern and not _glob_matches(type_pattern, event_type): - 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( - 50000, "regex_push_cache" -) - - -def _glob_matches(glob: str, value: str, word_boundary: bool = False) -> bool: - """Tests if value matches glob. - - Args: - glob - value: String to test against glob. - word_boundary: Whether to match against word boundaries or entire - string. Defaults to False. - """ - - try: - r = regex_cache.get((glob, True, word_boundary), None) - if not r: - r = glob_to_regex(glob, word_boundary=word_boundary) - regex_cache[(glob, True, word_boundary)] = r - return bool(r.search(value)) - except re.error: - logger.warning("Failed to parse glob to regex: %r", glob) - return False - - -def _flatten_dict( - d: Union[EventBase, Mapping[str, Any]], - prefix: Optional[List[str]] = None, - result: Optional[Dict[str, str]] = None, -) -> Dict[str, str]: - if prefix is None: - prefix = [] - if result is None: - result = {} - for key, value in d.items(): - if isinstance(value, str): - result[".".join(prefix + [key])] = value.lower() - elif isinstance(value, Mapping): - _flatten_dict(value, prefix=(prefix + [key]), result=result) - - return result diff --git a/tests/push/test_push_rule_evaluator.py b/tests/push/test_push_rule_evaluator.py index 718f48957748..9b70c46aa0e3 100644 --- a/tests/push/test_push_rule_evaluator.py +++ b/tests/push/test_push_rule_evaluator.py @@ -23,11 +23,12 @@ from synapse.api.room_versions import RoomVersions from synapse.appservice import ApplicationService from synapse.events import FrozenEvent -from synapse.push import push_rule_evaluator -from synapse.push.push_rule_evaluator import PushRuleEvaluatorForEvent +from synapse.push.bulk_push_rule_evaluator import _flatten_dict +from synapse.push.httppusher import tweaks_for_actions from synapse.rest.client import login, register, room from synapse.server import HomeServer from synapse.storage.databases.main.appservice import _make_exclusive_regex +from synapse.synapse_rust.push import PushRuleEvaluator from synapse.types import JsonDict from synapse.util import Clock @@ -41,7 +42,7 @@ def _get_evaluator( content: JsonDict, relations: Optional[Dict[str, Set[Tuple[str, str]]]] = None, relations_match_enabled: bool = False, - ) -> PushRuleEvaluatorForEvent: + ) -> PushRuleEvaluator: event = FrozenEvent( { "event_id": "$event_id", @@ -56,12 +57,12 @@ def _get_evaluator( room_member_count = 0 sender_power_level = 0 power_levels: Dict[str, Union[int, Dict[str, int]]] = {} - return PushRuleEvaluatorForEvent( - event, + return PushRuleEvaluator( + _flatten_dict(event), room_member_count, sender_power_level, - power_levels, - relations or set(), + power_levels.get("notifications", {}), + relations or {}, relations_match_enabled, ) @@ -293,7 +294,7 @@ def test_tweaks_for_actions(self) -> None: ] self.assertEqual( - push_rule_evaluator.tweaks_for_actions(actions), + tweaks_for_actions(actions), {"sound": "default", "highlight": True}, ) From d62a7ec54dbb39e4bff5a291f940c4fc15f2eb83 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 19 Sep 2022 12:19:12 +0100 Subject: [PATCH 18/35] Ignore unknown conditions to fix unit tests --- rust/src/push/evaluator.rs | 8 +------- tests/push/test_push_rule_evaluator.py | 3 --- 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index e6caebc49fd3..262310fbed1a 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -142,14 +142,8 @@ impl PushRuleEvaluator { ) -> Result { let known_condition = match condition { Condition::Known(known) => known, - - // 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. Condition::Unknown(_) => { - return Ok(true); + return Ok(false); } }; diff --git a/tests/push/test_push_rule_evaluator.py b/tests/push/test_push_rule_evaluator.py index 9b70c46aa0e3..b8308cbc0538 100644 --- a/tests/push/test_push_rule_evaluator.py +++ b/tests/push/test_push_rule_evaluator.py @@ -305,9 +305,6 @@ def test_relation_match(self) -> None: 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( From 65f55a27a609bd70024fec19e677a437cf5a6614 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 20 Sep 2022 08:47:50 +0100 Subject: [PATCH 19/35] Update rust/src/push/evaluator.rs Co-authored-by: Mathieu Velten --- rust/src/push/evaluator.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index 262310fbed1a..7b596664d619 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -19,7 +19,7 @@ lazy_static! { #[pyclass] pub struct PushRuleEvaluator { /// A mapping of "flattened" keys to string values in the event, e.g. - /// includes things liek "type" and "content.msgtype". + /// includes things like "type" and "content.msgtype". flattened_keys: BTreeMap, /// The "content.body", if any. From e75b755b8d69694ec8a17411034a5a7ab8b52a39 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 20 Sep 2022 12:15:06 +0100 Subject: [PATCH 20/35] Newsfile --- changelog.d/13838.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/13838.misc diff --git a/changelog.d/13838.misc b/changelog.d/13838.misc new file mode 100644 index 000000000000..28bddb705967 --- /dev/null +++ b/changelog.d/13838.misc @@ -0,0 +1 @@ +Port push rules to using Rust. From bc3b216495215687cb40875b79a2ce774974613f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 20 Sep 2022 12:54:44 +0100 Subject: [PATCH 21/35] Fix rust test --- rust/src/push/utils.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/src/push/utils.rs b/rust/src/push/utils.rs index 88d82b410a3a..7ec93714ff4a 100644 --- a/rust/src/push/utils.rs +++ b/rust/src/push/utils.rs @@ -118,7 +118,7 @@ fn tset_glob() -> Result<(), Error> { assert_eq!( glob_to_regex("simple", GlobMatchType::Word)?.as_str(), - r"\bsimple\b" + r"(?:^|\W|\b)simple(?:\b|\W|$)" ); assert!(glob_to_regex("simple", GlobMatchType::Word)?.is_match("some simple.")); From da1dc661bfb9aa0e1670f7f494b9881674976d1b Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 21 Sep 2022 09:49:28 +0100 Subject: [PATCH 22/35] Fast path --- rust/Cargo.toml | 4 +++- rust/src/push/evaluator.rs | 26 ++++++++++++++++++-------- rust/src/push/mod.rs | 16 ++++++++-------- rust/src/push/utils.rs | 5 +++-- 4 files changed, 32 insertions(+), 19 deletions(-) diff --git a/rust/Cargo.toml b/rust/Cargo.toml index 8dc5f93ff111..2d7ff39253d6 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -11,7 +11,9 @@ rust-version = "1.61.0" [lib] name = "synapse" -crate-type = ["cdylib"] +# We generate a `cdylib` for Python and a standrad `lib` for running +# tests/benchmarks. +crate-type = ["lib", "cdylib"] [package.metadata.maturin] # This is where we tell maturin where to place the built library. diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index 7b596664d619..391c1a9fd17e 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -2,7 +2,7 @@ use std::collections::{BTreeMap, BTreeSet}; use anyhow::{Context, Error}; use lazy_static::lazy_static; -use log::warn; +use log::{info, warn}; use pyo3::prelude::*; use regex::Regex; @@ -46,7 +46,7 @@ pub struct PushRuleEvaluator { impl PushRuleEvaluator { /// Create a new `PushRuleEvaluator`. See struct docstring for details. #[new] - fn py_new( + pub fn py_new( flattened_keys: BTreeMap, room_member_count: u64, sender_power_level: i64, @@ -78,7 +78,7 @@ impl PushRuleEvaluator { /// /// Returns the set of actions, if any, that match (filtering out any /// `dont_notify` actions). - fn run( + pub fn run( &self, push_rules: &FilteredPushRules, user_id: Option<&str>, @@ -271,6 +271,12 @@ impl PushRuleEvaluator { return Ok(false); }; + let haystack = if let Some(haystack) = self.flattened_keys.get(&*event_match.key) { + haystack + } else { + return Ok(false); + }; + // For the content.body we match against "words", but for everything // else we match against the entire value. let match_type = if event_match.key == "content.body" { @@ -279,12 +285,16 @@ impl PushRuleEvaluator { GlobMatchType::Whole }; - if let Some(value) = self.flattened_keys.get(&*event_match.key) { - let compiled_pattern = glob_to_regex(pattern, match_type)?; - Ok(compiled_pattern.is_match(value)) - } else { - Ok(false) + if !pattern.contains(['*', '?']) { + if match_type == GlobMatchType::Word && !haystack.contains(&pattern.to_lowercase()) { + return Ok(false); + } else if match_type == GlobMatchType::Whole { + return Ok(haystack == &pattern.to_lowercase()); + } } + + let compiled_pattern = glob_to_regex(pattern, match_type)?; + Ok(compiled_pattern.is_match(haystack)) } /// Match the member count against an 'is' condition diff --git a/rust/src/push/mod.rs b/rust/src/push/mod.rs index ff010821e5d1..1860fc80dfc2 100644 --- a/rust/src/push/mod.rs +++ b/rust/src/push/mod.rs @@ -66,8 +66,8 @@ use serde_json::Value; use self::evaluator::PushRuleEvaluator; mod base_rules; -mod evaluator; -mod utils; +pub mod evaluator; +pub mod utils; /// Called when registering modules with python. pub fn register_module(py: Python<'_>, m: &PyModule) -> PyResult<()> { @@ -303,17 +303,17 @@ impl<'source> FromPyObject<'source> for Condition { /// The body of a [`Condition::EventMatch`] #[derive(Serialize, Deserialize, Debug, Clone)] pub struct EventMatchCondition { - key: Cow<'static, str>, + pub key: Cow<'static, str>, #[serde(skip_serializing_if = "Option::is_none")] - pattern: Option>, + pub pattern: Option>, #[serde(skip_serializing_if = "Option::is_none")] - pattern_type: Option>, + pub pattern_type: Option>, } /// The collection of push rules for a user. #[derive(Debug, Clone, Default)] #[pyclass(frozen)] -struct PushRules { +pub struct PushRules { /// Custom push rules that override a base rule. overridden_base_rules: HashMap, PushRule>, @@ -332,7 +332,7 @@ struct PushRules { #[pymethods] impl PushRules { #[new] - fn new(rules: Vec) -> PushRules { + pub fn new(rules: Vec) -> PushRules { let mut push_rules: PushRules = Default::default(); for rule in rules { @@ -409,7 +409,7 @@ pub struct FilteredPushRules { #[pymethods] impl FilteredPushRules { #[new] - fn py_new( + pub fn py_new( push_rules: PushRules, enabled_map: BTreeMap, msc3786_enabled: bool, diff --git a/rust/src/push/utils.rs b/rust/src/push/utils.rs index 7ec93714ff4a..5025c94b81cd 100644 --- a/rust/src/push/utils.rs +++ b/rust/src/push/utils.rs @@ -26,7 +26,8 @@ pub(crate) fn get_localpart_from_id(id: &str) -> Result<&str, Error> { } /// Used by `glob_to_regex` to specify what to match the regex against. -pub(crate) enum GlobMatchType { +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum GlobMatchType { /// The generated regex will match against the entire input. Whole, /// The generated regex will match against words. @@ -35,7 +36,7 @@ pub(crate) enum GlobMatchType { /// Convert a "glob" style expression to a regex, anchoring either to the entire /// input or to individual words. -pub(crate) fn glob_to_regex(glob: &str, match_type: GlobMatchType) -> Result { +pub fn glob_to_regex(glob: &str, match_type: GlobMatchType) -> Result { let mut chunks = Vec::new(); // Patterns with wildcards must be simplified to avoid performance cliffs From 42ab02ebade3442e7e04e2650aef4d1dcc7b296d Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 21 Sep 2022 09:49:35 +0100 Subject: [PATCH 23/35] Benchmarks --- rust/benches/evaluator.rs | 114 ++++++++++++++++++++++++++++++++++++++ rust/benches/glob.rs | 26 +++++++++ 2 files changed, 140 insertions(+) create mode 100644 rust/benches/evaluator.rs create mode 100644 rust/benches/glob.rs diff --git a/rust/benches/evaluator.rs b/rust/benches/evaluator.rs new file mode 100644 index 000000000000..722b001f866c --- /dev/null +++ b/rust/benches/evaluator.rs @@ -0,0 +1,114 @@ +#![feature(test)] + +use synapse::push::{evaluator::PushRuleEvaluator, Condition, EventMatchCondition}; +use test::Bencher; + +extern crate test; + +#[bench] +fn bench_match_exact(b: &mut Bencher) { + let flattened_keys = [ + ("type".to_string(), "m.text".to_string()), + ("room_id".to_string(), "!room:server".to_string()), + ("content.body".to_string(), "test message".to_string()), + ] + .into_iter() + .collect(); + + let eval = PushRuleEvaluator::py_new( + flattened_keys, + 10, + 0, + Default::default(), + Default::default(), + true, + ) + .unwrap(); + + let condition = Condition::Known(synapse::push::KnownCondition::EventMatch( + EventMatchCondition { + key: "room_id".into(), + pattern: Some("!room:server".into()), + pattern_type: None, + }, + )); + + let matched = eval.match_condition(&condition, None, None).unwrap(); + if !matched { + panic!("Didn't match") + } + + b.iter(|| eval.match_condition(&condition, None, None).unwrap()); +} + +#[bench] +fn bench_match_word(b: &mut Bencher) { + let flattened_keys = [ + ("type".to_string(), "m.text".to_string()), + ("room_id".to_string(), "!room:server".to_string()), + ("content.body".to_string(), "test message".to_string()), + ] + .into_iter() + .collect(); + + let eval = PushRuleEvaluator::py_new( + flattened_keys, + 10, + 0, + Default::default(), + Default::default(), + true, + ) + .unwrap(); + + let condition = Condition::Known(synapse::push::KnownCondition::EventMatch( + EventMatchCondition { + key: "content.body".into(), + pattern: Some("test".into()), + pattern_type: None, + }, + )); + + let matched = eval.match_condition(&condition, None, None).unwrap(); + if !matched { + panic!("Didn't match") + } + + b.iter(|| eval.match_condition(&condition, None, None).unwrap()); +} + +#[bench] +fn bench_match_word_miss(b: &mut Bencher) { + let flattened_keys = [ + ("type".to_string(), "m.text".to_string()), + ("room_id".to_string(), "!room:server".to_string()), + ("content.body".to_string(), "test message".to_string()), + ] + .into_iter() + .collect(); + + let eval = PushRuleEvaluator::py_new( + flattened_keys, + 10, + 0, + Default::default(), + Default::default(), + true, + ) + .unwrap(); + + let condition = Condition::Known(synapse::push::KnownCondition::EventMatch( + EventMatchCondition { + key: "content.body".into(), + pattern: Some("foobar".into()), + pattern_type: None, + }, + )); + + let matched = eval.match_condition(&condition, None, None).unwrap(); + if matched { + panic!("Didn't match") + } + + b.iter(|| eval.match_condition(&condition, None, None).unwrap()); +} diff --git a/rust/benches/glob.rs b/rust/benches/glob.rs new file mode 100644 index 000000000000..f2be54eb53a8 --- /dev/null +++ b/rust/benches/glob.rs @@ -0,0 +1,26 @@ +#![feature(test)] + +use synapse::push::utils::{glob_to_regex, GlobMatchType}; +use test::Bencher; + +extern crate test; + +#[bench] +fn bench_whole(b: &mut Bencher) { + b.iter(|| glob_to_regex("test", GlobMatchType::Whole)); +} + +#[bench] +fn bench_word(b: &mut Bencher) { + b.iter(|| glob_to_regex("test", GlobMatchType::Word)); +} + +#[bench] +fn bench_whole_wildcard_run(b: &mut Bencher) { + b.iter(|| glob_to_regex("test***??*?*?foo", GlobMatchType::Whole)); +} + +#[bench] +fn bench_word_wildcard_run(b: &mut Bencher) { + b.iter(|| glob_to_regex("test***??*?*?foo", GlobMatchType::Whole)); +} From e6d9341af4bdac3fc761a41d7458e8c9e4ab9e77 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Sat, 24 Sep 2022 12:06:57 +0100 Subject: [PATCH 24/35] Speed up --- rust/benches/evaluator.rs | 32 +++++++++++++++++++++++++++++++- rust/src/push/evaluator.rs | 2 +- rust/src/push/utils.rs | 5 +++-- 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/rust/benches/evaluator.rs b/rust/benches/evaluator.rs index 722b001f866c..78baedacb4f9 100644 --- a/rust/benches/evaluator.rs +++ b/rust/benches/evaluator.rs @@ -1,6 +1,10 @@ #![feature(test)] -use synapse::push::{evaluator::PushRuleEvaluator, Condition, EventMatchCondition}; +use std::collections::BTreeMap; + +use synapse::push::{ + evaluator::PushRuleEvaluator, Condition, EventMatchCondition, FilteredPushRules, PushRules, +}; use test::Bencher; extern crate test; @@ -112,3 +116,29 @@ fn bench_match_word_miss(b: &mut Bencher) { b.iter(|| eval.match_condition(&condition, None, None).unwrap()); } + +#[bench] +fn bench_eval_message(b: &mut Bencher) { + let flattened_keys = [ + ("type".to_string(), "m.text".to_string()), + ("room_id".to_string(), "!room:server".to_string()), + ("content.body".to_string(), "test message".to_string()), + ] + .into_iter() + .collect(); + + let eval = PushRuleEvaluator::py_new( + flattened_keys, + 10, + 0, + Default::default(), + Default::default(), + true, + ) + .unwrap(); + + let rules = + FilteredPushRules::py_new(PushRules::new(Vec::new()), Default::default(), false, false); + + b.iter(|| eval.run(&rules, Some("bob"), Some("person"))); +} diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index 391c1a9fd17e..0b306551a1d8 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -153,7 +153,7 @@ impl PushRuleEvaluator { } KnownCondition::ContainsDisplayName => { if let Some(dn) = display_name { - if !dn.is_empty() { + if !dn.is_empty() && self.body.contains(dn) { let matcher = glob_to_regex(dn, GlobMatchType::Word)?; matcher.is_match(&self.body) } else { diff --git a/rust/src/push/utils.rs b/rust/src/push/utils.rs index 5025c94b81cd..f219b7ef78df 100644 --- a/rust/src/push/utils.rs +++ b/rust/src/push/utils.rs @@ -69,7 +69,7 @@ pub fn glob_to_regex(glob: &str, match_type: GlobMatchType) -> Result format!(r"(?:^|\W|\b){joined}(?:\b|\W|$)"), + GlobMatchType::Word => format!(r"\b{joined}\b"), }; Ok(RegexBuilder::new(®ex_str) @@ -119,11 +119,12 @@ fn tset_glob() -> Result<(), Error> { assert_eq!( glob_to_regex("simple", GlobMatchType::Word)?.as_str(), - r"(?:^|\W|\b)simple(?:\b|\W|$)" + r"\bsimple\b", ); assert!(glob_to_regex("simple", GlobMatchType::Word)?.is_match("some simple.")); assert!(glob_to_regex("simple", GlobMatchType::Word)?.is_match("simple")); + assert!(!glob_to_regex("simple", GlobMatchType::Word)?.is_match("simples")); Ok(()) } From bef2298a8412a8e6ca4a134ff7439c65f49f8c10 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Sat, 24 Sep 2022 15:39:45 +0100 Subject: [PATCH 25/35] Add a Matcher type --- rust/benches/evaluator.rs | 3 -- rust/src/push/evaluator.rs | 27 ++++++------- rust/src/push/mod.rs | 1 - rust/src/push/utils.rs | 81 +++++++++++++++++++++++++++++++++++--- 4 files changed, 89 insertions(+), 23 deletions(-) diff --git a/rust/benches/evaluator.rs b/rust/benches/evaluator.rs index 78baedacb4f9..a88560582a28 100644 --- a/rust/benches/evaluator.rs +++ b/rust/benches/evaluator.rs @@ -1,7 +1,4 @@ #![feature(test)] - -use std::collections::BTreeMap; - use synapse::push::{ evaluator::PushRuleEvaluator, Condition, EventMatchCondition, FilteredPushRules, PushRules, }; diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index 0b306551a1d8..676d5f10ad46 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -2,12 +2,12 @@ use std::collections::{BTreeMap, BTreeSet}; use anyhow::{Context, Error}; use lazy_static::lazy_static; -use log::{info, warn}; +use log::warn; use pyo3::prelude::*; use regex::Regex; use super::{ - utils::{get_localpart_from_id, glob_to_regex, GlobMatchType}, + utils::{get_glob_matcher, get_localpart_from_id, GlobMatchType}, Action, Condition, EventMatchCondition, FilteredPushRules, KnownCondition, }; @@ -154,8 +154,7 @@ impl PushRuleEvaluator { KnownCondition::ContainsDisplayName => { if let Some(dn) = display_name { if !dn.is_empty() && self.body.contains(dn) { - let matcher = glob_to_regex(dn, GlobMatchType::Word)?; - matcher.is_match(&self.body) + get_glob_matcher(dn, GlobMatchType::Word)?.is_match(&self.body)? } else { // We specifically ignore empty display names, as otherwise // they would always match. @@ -214,27 +213,27 @@ impl PushRuleEvaluator { None }; - let sender_compiled_pattern = if let Some(pattern) = sender_pattern { - Some(glob_to_regex(pattern, GlobMatchType::Whole)?) + let mut sender_compiled_pattern = if let Some(pattern) = sender_pattern { + Some(get_glob_matcher(pattern, GlobMatchType::Whole)?) } else { None }; - let type_compiled_pattern = if let Some(pattern) = event_type_pattern { - Some(glob_to_regex(pattern, GlobMatchType::Whole)?) + let mut type_compiled_pattern = if let Some(pattern) = event_type_pattern { + Some(get_glob_matcher(pattern, GlobMatchType::Whole)?) } else { None }; for (relation_sender, event_type) in relations { - if let Some(pattern) = &sender_compiled_pattern { - if !pattern.is_match(relation_sender) { + if let Some(pattern) = &mut sender_compiled_pattern { + if !pattern.is_match(relation_sender)? { continue; } } - if let Some(pattern) = &type_compiled_pattern { - if !pattern.is_match(event_type) { + if let Some(pattern) = &mut type_compiled_pattern { + if !pattern.is_match(event_type)? { continue; } } @@ -293,8 +292,8 @@ impl PushRuleEvaluator { } } - let compiled_pattern = glob_to_regex(pattern, match_type)?; - Ok(compiled_pattern.is_match(haystack)) + let mut compiled_pattern = get_glob_matcher(pattern, match_type)?; + Ok(compiled_pattern.is_match(haystack)?) } /// Match the member count against an 'is' condition diff --git a/rust/src/push/mod.rs b/rust/src/push/mod.rs index 1860fc80dfc2..30fffc31adba 100644 --- a/rust/src/push/mod.rs +++ b/rust/src/push/mod.rs @@ -42,7 +42,6 @@ //! //! 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. diff --git a/rust/src/push/utils.rs b/rust/src/push/utils.rs index f219b7ef78df..220719495953 100644 --- a/rust/src/push/utils.rs +++ b/rust/src/push/utils.rs @@ -69,7 +69,7 @@ pub fn glob_to_regex(glob: &str, match_type: GlobMatchType) -> Result format!(r"\b{joined}\b"), + GlobMatchType::Word => format!(r"(?:^|\b|\W){joined}(?:\b|\W|$)"), }; Ok(RegexBuilder::new(®ex_str) @@ -77,6 +77,73 @@ pub fn glob_to_regex(glob: &str, match_type: GlobMatchType) -> Result Result { + // There are a number of shortcuts we can make if the glob doesn't contain a + // wild card. + let matcher = if glob.contains(['*', '?']) { + let regex = glob_to_regex(glob, match_type)?; + Matcher::Regex(regex) + } else if match_type == GlobMatchType::Whole { + // If there aren't any wildcards and we're matching the whole thing, + // then we simply can do a case-insensitive string match. + Matcher::Whole(glob.to_lowercase()) + } else { + // Otherwise, if we're matching against words then can first check + // if the haystack contains the glob at all. + Matcher::Word { + word: glob.to_lowercase(), + regex: None, + } + }; + + Ok(matcher) +} + +/// Matches against a glob +pub enum Matcher { + /// Plain regex matching. + Regex(Regex), + + /// Case-insensitive equality. + Whole(String), + + /// Word matching. `regex` is a cache of calling [`glob_to_regex`] on word. + Word { word: String, regex: Option }, +} + +impl Matcher { + /// Checks if the glob matches the given haystack. + pub fn is_match(&mut self, haystack: &str) -> Result { + // We want to to do case-insensitive matching, so we convert to + // lowercase first. + let haystack = haystack.to_lowercase(); + + match self { + Matcher::Regex(regex) => Ok(regex.is_match(&haystack)), + Matcher::Whole(whole) => Ok(whole == &haystack), + Matcher::Word { word, regex } => { + // If we're looking for a literal word, then we first check if + // the haystack contains the word as a substring. + if !haystack.contains(&*word) { + return Ok(false); + } + + // If it does contain the word as a substring, then we need to + // check if it is an actual word by testing it against the regex. + let regex = if let Some(regex) = regex { + regex + } else { + let compiled_regex = glob_to_regex(word, GlobMatchType::Word)?; + regex.insert(compiled_regex) + }; + + Ok(regex.is_match(&haystack)) + } + } + } +} + #[test] fn test_get_domain_from_id() { get_localpart_from_id("").unwrap_err(); @@ -117,14 +184,18 @@ fn tset_glob() -> Result<(), Error> { r"\Aescape\.\z" ); - assert_eq!( - glob_to_regex("simple", GlobMatchType::Word)?.as_str(), - r"\bsimple\b", - ); + assert!(glob_to_regex("simple", GlobMatchType::Whole)?.is_match("simple")); + assert!(!glob_to_regex("simple", GlobMatchType::Whole)?.is_match("simples")); + assert!(glob_to_regex("simple*", GlobMatchType::Whole)?.is_match("simples")); + assert!(glob_to_regex("simple?", GlobMatchType::Whole)?.is_match("simples")); + assert!(glob_to_regex("simple*", GlobMatchType::Whole)?.is_match("simple")); assert!(glob_to_regex("simple", GlobMatchType::Word)?.is_match("some simple.")); assert!(glob_to_regex("simple", GlobMatchType::Word)?.is_match("simple")); assert!(!glob_to_regex("simple", GlobMatchType::Word)?.is_match("simples")); + assert!(glob_to_regex("@user:foo", GlobMatchType::Word)?.is_match("Some @user:foo test")); + assert!(glob_to_regex("@user:foo", GlobMatchType::Word)?.is_match("@user:foo")); + Ok(()) } From c51bf1edf2eae33567d37002ef6c0619ec4d6ea1 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Sat, 24 Sep 2022 15:43:22 +0100 Subject: [PATCH 26/35] Tidy up --- rust/src/push/evaluator.rs | 125 +++++++++++++++++++++---------------- 1 file changed, 72 insertions(+), 53 deletions(-) diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index 676d5f10ad46..fcdb2362afee 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -1,4 +1,7 @@ -use std::collections::{BTreeMap, BTreeSet}; +use std::{ + borrow::Cow, + collections::{BTreeMap, BTreeSet}, +}; use anyhow::{Context, Error}; use lazy_static::lazy_static; @@ -153,7 +156,7 @@ impl PushRuleEvaluator { } KnownCondition::ContainsDisplayName => { if let Some(dn) = display_name { - if !dn.is_empty() && self.body.contains(dn) { + if !dn.is_empty() { get_glob_matcher(dn, GlobMatchType::Word)?.is_match(&self.body)? } else { // We specifically ignore empty display names, as otherwise @@ -186,68 +189,84 @@ impl PushRuleEvaluator { sender, sender_type, } => { - if !self.relation_match_enabled { - return Ok(false); - } + self.match_relations(rel_type, sender, sender_type, user_id, event_type_pattern)? + } + }; - let relations = if let Some(relations) = self.relations.get(&**rel_type) { - relations - } else { - return Ok(false); - }; - - let sender_pattern = if let Some(sender) = sender { - Some(sender.as_ref()) - } else if let Some(sender_type) = sender_type { - if sender_type == "user_id" { - if let Some(user_id) = user_id { - Some(user_id) - } else { - return Ok(false); - } - } else { - warn!("Unrecognized sender_type: {sender_type}"); - return Ok(false); - } - } else { - None - }; + Ok(result) + } - let mut sender_compiled_pattern = if let Some(pattern) = sender_pattern { - Some(get_glob_matcher(pattern, GlobMatchType::Whole)?) - } else { - None - }; + /// Evaluates a relation condition. + fn match_relations( + &self, + rel_type: &str, + sender: &Option>, + sender_type: &Option>, + user_id: Option<&str>, + event_type_pattern: &Option>, + ) -> Result { + // First check if relation matching is enabled... + if !self.relation_match_enabled { + return Ok(false); + } + + // ... and if there are any relations to match against. + let relations = if let Some(relations) = self.relations.get(rel_type) { + relations + } else { + return Ok(false); + }; - let mut type_compiled_pattern = if let Some(pattern) = event_type_pattern { - Some(get_glob_matcher(pattern, GlobMatchType::Whole)?) + // Extract the sender pattern from the condition + let sender_pattern = if let Some(sender) = sender { + Some(sender.as_ref()) + } else if let Some(sender_type) = sender_type { + if sender_type == "user_id" { + if let Some(user_id) = user_id { + Some(user_id) } else { - None - }; - - for (relation_sender, event_type) in relations { - if let Some(pattern) = &mut sender_compiled_pattern { - if !pattern.is_match(relation_sender)? { - continue; - } - } + return Ok(false); + } + } else { + warn!("Unrecognized sender_type: {sender_type}"); + return Ok(false); + } + } else { + None + }; - if let Some(pattern) = &mut type_compiled_pattern { - if !pattern.is_match(event_type)? { - continue; - } - } + let mut sender_compiled_pattern = if let Some(pattern) = sender_pattern { + Some(get_glob_matcher(pattern, GlobMatchType::Whole)?) + } else { + None + }; - return Ok(true); + let mut type_compiled_pattern = if let Some(pattern) = event_type_pattern { + Some(get_glob_matcher(pattern, GlobMatchType::Whole)?) + } else { + None + }; + + for (relation_sender, event_type) in relations { + if let Some(pattern) = &mut sender_compiled_pattern { + if !pattern.is_match(relation_sender)? { + continue; } + } - false + if let Some(pattern) = &mut type_compiled_pattern { + if !pattern.is_match(event_type)? { + continue; + } } - }; - Ok(result) + return Ok(true); + } + + Ok(false) } + /// Evaluates a `event_match` condition. fn match_event_match( &self, event_match: &EventMatchCondition, @@ -293,7 +312,7 @@ impl PushRuleEvaluator { } let mut compiled_pattern = get_glob_matcher(pattern, match_type)?; - Ok(compiled_pattern.is_match(haystack)?) + compiled_pattern.is_match(haystack) } /// Match the member count against an 'is' condition From 307e77a5208abb54f6ca3512962d5aa39a494525 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Sun, 25 Sep 2022 14:54:26 +0100 Subject: [PATCH 27/35] Remove unnecessary optimisation --- rust/src/push/evaluator.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index fcdb2362afee..571e42c8cf84 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -303,14 +303,6 @@ impl PushRuleEvaluator { GlobMatchType::Whole }; - if !pattern.contains(['*', '?']) { - if match_type == GlobMatchType::Word && !haystack.contains(&pattern.to_lowercase()) { - return Ok(false); - } else if match_type == GlobMatchType::Whole { - return Ok(haystack == &pattern.to_lowercase()); - } - } - let mut compiled_pattern = get_glob_matcher(pattern, match_type)?; compiled_pattern.is_match(haystack) } From f760ada5232c451bbc241e5c92170a5ea988211f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Sun, 25 Sep 2022 14:55:34 +0100 Subject: [PATCH 28/35] Comment --- rust/src/push/evaluator.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index 571e42c8cf84..9ab2e0e800a7 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -275,11 +275,15 @@ impl PushRuleEvaluator { let pattern = if let Some(pattern) = &event_match.pattern { pattern } else if let Some(pattern_type) = &event_match.pattern_type { + // The `pattern_type` can either be "user_id" or "user_localpart", + // either way if we don't have a `user_id` then the condition can't + // match. let user_id = if let Some(user_id) = user_id { user_id } else { return Ok(false); }; + match &**pattern_type { "user_id" => user_id, "user_localpart" => get_localpart_from_id(user_id)?, From 9df6d370acfedca4da75facf0b57bbe68c9cd747 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Sun, 25 Sep 2022 14:58:29 +0100 Subject: [PATCH 29/35] clippy --- rust/benches/evaluator.rs | 12 +++--------- rust/build.rs | 2 +- rust/src/push/evaluator.rs | 2 +- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/rust/benches/evaluator.rs b/rust/benches/evaluator.rs index a88560582a28..bed0ff0c8951 100644 --- a/rust/benches/evaluator.rs +++ b/rust/benches/evaluator.rs @@ -35,9 +35,7 @@ fn bench_match_exact(b: &mut Bencher) { )); let matched = eval.match_condition(&condition, None, None).unwrap(); - if !matched { - panic!("Didn't match") - } + assert!(matched, "Didn't match"); b.iter(|| eval.match_condition(&condition, None, None).unwrap()); } @@ -71,9 +69,7 @@ fn bench_match_word(b: &mut Bencher) { )); let matched = eval.match_condition(&condition, None, None).unwrap(); - if !matched { - panic!("Didn't match") - } + assert!(matched, "Didn't match"); b.iter(|| eval.match_condition(&condition, None, None).unwrap()); } @@ -107,9 +103,7 @@ fn bench_match_word_miss(b: &mut Bencher) { )); let matched = eval.match_condition(&condition, None, None).unwrap(); - if matched { - panic!("Didn't match") - } + assert!(!matched, "Didn't match"); b.iter(|| eval.match_condition(&condition, None, None).unwrap()); } diff --git a/rust/build.rs b/rust/build.rs index 2117975e56f7..ef370e6b4188 100644 --- a/rust/build.rs +++ b/rust/build.rs @@ -22,7 +22,7 @@ fn main() -> Result<(), std::io::Error> { for entry in entries { if entry.is_dir() { - dirs.push(entry) + dirs.push(entry); } else { paths.push(entry.to_str().expect("valid rust paths").to_string()); } diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index 9ab2e0e800a7..6981a220d345 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -315,7 +315,7 @@ impl PushRuleEvaluator { fn match_member_count(&self, is: &str) -> Result { // The 'is' condition can be things like '>2', '==3' or event just '4'. let captures = INEQUALITY_EXPR.captures(is).context("bad 'is' clause")?; - let ineq = captures.get(1).map(|m| m.as_str()).unwrap_or("=="); + let ineq = captures.get(1).map_or("==", |m| m.as_str()); let rhs: u64 = captures .get(2) .context("missing number")? From e4cd075570b51b82e2554e7d8265190eb3a5e394 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Sun, 25 Sep 2022 15:01:52 +0100 Subject: [PATCH 30/35] Copyright headers --- rust/benches/evaluator.rs | 14 ++++++++++++++ rust/benches/glob.rs | 14 ++++++++++++++ rust/src/push/evaluator.rs | 14 ++++++++++++++ rust/src/push/utils.rs | 14 ++++++++++++++ 4 files changed, 56 insertions(+) diff --git a/rust/benches/evaluator.rs b/rust/benches/evaluator.rs index bed0ff0c8951..ed411461d139 100644 --- a/rust/benches/evaluator.rs +++ b/rust/benches/evaluator.rs @@ -1,3 +1,17 @@ +// Copyright 2022 The Matrix.org Foundation C.I.C. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// 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. + #![feature(test)] use synapse::push::{ evaluator::PushRuleEvaluator, Condition, EventMatchCondition, FilteredPushRules, PushRules, diff --git a/rust/benches/glob.rs b/rust/benches/glob.rs index f2be54eb53a8..b6697d9285c3 100644 --- a/rust/benches/glob.rs +++ b/rust/benches/glob.rs @@ -1,3 +1,17 @@ +// Copyright 2022 The Matrix.org Foundation C.I.C. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// 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. + #![feature(test)] use synapse::push::utils::{glob_to_regex, GlobMatchType}; diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index 6981a220d345..0581e33173b2 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -1,3 +1,17 @@ +// Copyright 2022 The Matrix.org Foundation C.I.C. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// 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. + use std::{ borrow::Cow, collections::{BTreeMap, BTreeSet}, diff --git a/rust/src/push/utils.rs b/rust/src/push/utils.rs index 220719495953..e71dcacb1773 100644 --- a/rust/src/push/utils.rs +++ b/rust/src/push/utils.rs @@ -1,3 +1,17 @@ +// Copyright 2022 The Matrix.org Foundation C.I.C. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// 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. + use anyhow::bail; use anyhow::Context; use anyhow::Error; From 0035f14562a4c6a7ab1aab6be3900edd7eb53a19 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 28 Sep 2022 17:29:54 +0100 Subject: [PATCH 31/35] Update rust/Cargo.toml Co-authored-by: Patrick Cloke --- rust/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/Cargo.toml b/rust/Cargo.toml index 6de58ab77d0e..cffaa5b51b94 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -11,7 +11,7 @@ rust-version = "1.58.1" [lib] name = "synapse" -# We generate a `cdylib` for Python and a standrad `lib` for running +# We generate a `cdylib` for Python and a standard `lib` for running # tests/benchmarks. crate-type = ["lib", "cdylib"] From 1034e7fc672cfacb58486d3d84a29c1a3c0d8045 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 29 Sep 2022 14:03:41 +0100 Subject: [PATCH 32/35] Review comments --- rust/src/push/evaluator.rs | 5 +++-- rust/src/push/utils.rs | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index 0581e33173b2..b37aaf39f308 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -29,7 +29,8 @@ use super::{ }; lazy_static! { - static ref INEQUALITY_EXPR: Regex = Regex::new(r"^([=<>]*)([0-9]*)$").expect("valid regex"); + /// Used to parse the `is` clause in the room member count condition. + static ref INEQUALITY_EXPR: Regex = Regex::new(r"^([=<>]*)([0-9]+)$").expect("valid regex"); } /// Allows running a set of push rules against a particular event. @@ -143,7 +144,7 @@ impl PushRuleEvaluator { Ok(false) => false, Err(err) => { warn!("Condition match failed {err}"); - true + false } } } diff --git a/rust/src/push/utils.rs b/rust/src/push/utils.rs index e71dcacb1773..87593404730a 100644 --- a/rust/src/push/utils.rs +++ b/rust/src/push/utils.rs @@ -21,7 +21,7 @@ use regex::Regex; use regex::RegexBuilder; lazy_static! { - /// Matches runs of "glob" style wild cards + /// Matches runs of non-wildcard characters followed by wildcard characters. static ref WILDCARD_RUN: Regex = Regex::new(r"([^\?\*]*)([\?\*]*)").expect("valid regex"); } From de494f2f2af7b87501380aa0de9cd13675d4e28a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 29 Sep 2022 14:08:52 +0100 Subject: [PATCH 33/35] Fix merge --- rust/src/push/evaluator.rs | 25 +++++++++++++++---------- stubs/synapse/synapse_rust/push.pyi | 2 +- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index b37aaf39f308..87dee596456c 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -56,8 +56,9 @@ pub struct PushRuleEvaluator { /// Is running "relation" conditions enabled? relation_match_enabled: bool, - /// The power level of the sender of the event - sender_power_level: i64, + /// The power level of the sender of the event, or None if event is an + /// outlier. + sender_power_level: Option, } #[pymethods] @@ -67,7 +68,7 @@ impl PushRuleEvaluator { pub fn py_new( flattened_keys: BTreeMap, room_member_count: u64, - sender_power_level: i64, + sender_power_level: Option, notification_power_levels: BTreeMap, relations: BTreeMap>, relation_match_enabled: bool, @@ -190,13 +191,17 @@ impl PushRuleEvaluator { } } KnownCondition::SenderNotificationPermission { key } => { - let required_level = self - .notification_power_levels - .get(key.as_ref()) - .copied() - .unwrap_or(50); - - self.sender_power_level >= required_level + if let Some(sender_power_level) = &self.sender_power_level { + let required_level = self + .notification_power_levels + .get(key.as_ref()) + .copied() + .unwrap_or(50); + + *sender_power_level >= required_level + } else { + false + } } KnownCondition::RelationMatch { rel_type, diff --git a/stubs/synapse/synapse_rust/push.pyi b/stubs/synapse/synapse_rust/push.pyi index 80853901ec2b..fffb8419c63d 100644 --- a/stubs/synapse/synapse_rust/push.pyi +++ b/stubs/synapse/synapse_rust/push.pyi @@ -41,7 +41,7 @@ class PushRuleEvaluator: self, flattened_keys: Mapping[str, str], room_member_count: int, - sender_power_level: int, + sender_power_level: Optional[int], notification_power_levels: Mapping[str, int], relations: Mapping[str, Set[Tuple[str, str]]], relation_match_enabled: bool, From 23b307d0bb30d6f4e3404ff2b8927ee3fe501ff0 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 29 Sep 2022 14:09:36 +0100 Subject: [PATCH 34/35] Apply suggestions from code review Co-authored-by: reivilibre --- rust/src/push/evaluator.rs | 2 +- synapse/push/bulk_push_rule_evaluator.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index 87dee596456c..77bf9fe8453e 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -332,8 +332,8 @@ impl PushRuleEvaluator { } /// Match the member count against an 'is' condition + /// The `is` condition can be things like '>2', '==3' or even just '4'. fn match_member_count(&self, is: &str) -> Result { - // The 'is' condition can be things like '>2', '==3' or event just '4'. let captures = INEQUALITY_EXPR.captures(is).context("bad 'is' clause")?; let ineq = captures.get(1).map_or("==", |m| m.as_str()); let rhs: u64 = captures diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index 8fe5b9beddf8..60f312900588 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -338,7 +338,7 @@ async def action_for_event_by_user( actions_by_user[uid] = [] actions = evaluator.run(rules, uid, display_name) - if actions and "notify" in actions: + if "notify" in actions: # Push rules say we should notify the user of this event actions_by_user[uid] = actions From 09b728d6456624608e65efe21100edd5dd5b4a6e Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 29 Sep 2022 14:17:43 +0100 Subject: [PATCH 35/35] Fix test --- rust/src/push/evaluator.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index 77bf9fe8453e..efe88ec76e54 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -362,7 +362,7 @@ fn push_rule_evaluator() { let evaluator = PushRuleEvaluator::py_new( flattened_keys, 10, - 0, + Some(0), BTreeMap::new(), BTreeMap::new(), true,