Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Implement MSC3930: polls push rules #14787

Merged
merged 12 commits into from
Jan 19, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/14787.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Implement experimental support for MSC3930: Push rules for (MSC3381) Polls.
6 changes: 4 additions & 2 deletions docker/complement/conf/workers-shared-extra.yaml.j2
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,14 @@ experimental_features:
# client-side support for partial state in /send_join responses
faster_joins: true
{% endif %}
# Filtering /messages by relation type.
msc3874_enabled: true
# Enable support for polls
msc3381_polls_enabled: true
# Enable deleting device-specific notification settings stored in account data
msc3890_enabled: true
# Enable removing account data support
msc3391_enabled: true
# Filtering /messages by relation type.
msc3874_enabled: true

server_notices:
system_mxid_localpart: _server
Expand Down
9 changes: 7 additions & 2 deletions rust/benches/evaluator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,13 @@ fn bench_eval_message(b: &mut Bencher) {
)
.unwrap();

let rules =
FilteredPushRules::py_new(PushRules::new(Vec::new()), Default::default(), false, false);
let rules = FilteredPushRules::py_new(
PushRules::new(Vec::new()),
Default::default(),
false,
false,
false,
);

b.iter(|| eval.run(&rules, Some("bob"), Some("person")));
}
78 changes: 77 additions & 1 deletion rust/src/push/base_rules.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2022 The Matrix.org Foundation C.I.C.
// Copyright 2022, 2023 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.
Expand Down Expand Up @@ -208,6 +208,20 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[
default: true,
default_enabled: true,
},
PushRule {
rule_id: Cow::Borrowed("global/override/.org.matrix.msc3930.rule.poll_response"),
priority_class: 5,
conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::EventMatch(
EventMatchCondition {
key: Cow::Borrowed("type"),
pattern: Some(Cow::Borrowed("org.matrix.msc3381.poll.response")),
pattern_type: None,
},
))]),
actions: Cow::Borrowed(&[]),
default: true,
default_enabled: true,
},
];

pub const BASE_APPEND_CONTENT_RULES: &[PushRule] = &[PushRule {
Expand Down Expand Up @@ -596,6 +610,68 @@ pub const BASE_APPEND_UNDERRIDE_RULES: &[PushRule] = &[
default: true,
default_enabled: true,
},
PushRule {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the placement in the order here matter? The MSC doesn't state where it goes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the ordering of these rules will matter yes. We want the *one_to_one rules to be tested in clients first before their group room counterparts. I'm a little confused on how priority works in push rules though. The spec seems to imply that rules are checked by kind, then an "ordering priority". I'm guessing the latter is the order that the rules are sent to clients in, rather than a field? It's a bit vague though.

We can also just sidestep the ordering requirement by placing a room_member_count>2 condition on the group room rules, but that shouldn't be necessary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the ordering of these rules will matter yes.

Sorry for being unclear! I meant how did you decide this went after global/underride/.im.vector.jitsi.

I think internally the ordering is reasonable and matches the MSC.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! I didn't consider ordering against other underride push rules. But perhaps this should go above global/underride/.m.rule.encrypted, such that clients match this push rule before the generic "encrypted message" one?

Though that does make me question all of the other non-state event-related rules in this file. Shouldn't they go above rules that match generic m.room.encrypted events? I suppose the client must be doing some mangling of the order here/filtering out of the m.rule.encrypted rule(s) already?

It turns out matrix-react-sdk is just sorting known rules client-side anyways into this order. Unknown rules appear at the end in the order they were received from the homeserver. Interestingly the generic EncryptedMessage and EncryptedDM rules are put in front. I'm not sure how this achieves the desired result unless there's additional logic I'm missing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding was the spec mandates an order (and thus MSCs must define an order).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have tried to get some clarity in matrix-org/matrix-spec-proposals#3930 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've been given the go ahead to define our own ordering for now: matrix-org/matrix-spec-proposals#3930 (comment)

rule_id: Cow::Borrowed("global/underride/.org.matrix.msc3930.rule.poll_start_one_to_one"),
priority_class: 1,
conditions: Cow::Borrowed(&[
Condition::Known(KnownCondition::RoomMemberCount {
is: Some(Cow::Borrowed("2")),
}),
Condition::Known(KnownCondition::EventMatch(EventMatchCondition {
key: Cow::Borrowed("type"),
pattern: Some(Cow::Borrowed("org.matrix.msc3381.poll.start")),
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
pattern_type: None,
})),
]),
actions: Cow::Borrowed(&[Action::Notify, SOUND_ACTION]),
default: true,
default_enabled: true,
},
PushRule {
rule_id: Cow::Borrowed("global/underride/.org.matrix.msc3930.rule.poll_start"),
priority_class: 1,
conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::EventMatch(
EventMatchCondition {
key: Cow::Borrowed("type"),
pattern: Some(Cow::Borrowed("org.matrix.msc3381.poll.start")),
pattern_type: None,
},
))]),
actions: Cow::Borrowed(&[Action::Notify]),
default: true,
default_enabled: true,
},
PushRule {
rule_id: Cow::Borrowed("global/underride/.org.matrix.msc3930.rule.poll_end_one_to_one"),
priority_class: 1,
conditions: Cow::Borrowed(&[
Condition::Known(KnownCondition::RoomMemberCount {
is: Some(Cow::Borrowed("2")),
}),
Condition::Known(KnownCondition::EventMatch(EventMatchCondition {
key: Cow::Borrowed("type"),
pattern: Some(Cow::Borrowed("org.matrix.msc3381.poll.end")),
pattern_type: None,
})),
]),
actions: Cow::Borrowed(&[Action::Notify, SOUND_ACTION]),
default: true,
default_enabled: true,
},
PushRule {
rule_id: Cow::Borrowed("global/underride/.org.matrix.msc3930.rule.poll_end"),
priority_class: 1,
conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::EventMatch(
EventMatchCondition {
key: Cow::Borrowed("type"),
pattern: Some(Cow::Borrowed("org.matrix.msc3381.poll.end")),
pattern_type: None,
},
))]),
actions: Cow::Borrowed(&[Action::Notify]),
default: true,
default_enabled: true,
},
];

lazy_static! {
Expand Down
2 changes: 1 addition & 1 deletion rust/src/push/evaluator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ fn test_requires_room_version_supports_condition() {
};
let rules = PushRules::new(vec![custom_rule]);
result = evaluator.run(
&FilteredPushRules::py_new(rules, BTreeMap::new(), true, true),
&FilteredPushRules::py_new(rules, BTreeMap::new(), true, false, true),
None,
None,
);
Expand Down
16 changes: 12 additions & 4 deletions rust/src/push/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -411,8 +411,9 @@ impl PushRules {
pub struct FilteredPushRules {
push_rules: PushRules,
enabled_map: BTreeMap<String, bool>,
msc3664_enabled: bool,
msc1767_enabled: bool,
msc3381_polls_enabled: bool,
msc3664_enabled: bool,
}

#[pymethods]
Expand All @@ -421,14 +422,16 @@ impl FilteredPushRules {
pub fn py_new(
push_rules: PushRules,
enabled_map: BTreeMap<String, bool>,
msc3664_enabled: bool,
msc1767_enabled: bool,
msc3381_polls_enabled: bool,
msc3664_enabled: bool,
) -> Self {
Self {
push_rules,
enabled_map,
msc3664_enabled,
msc1767_enabled,
msc3381_polls_enabled,
msc3664_enabled,
}
}

Expand All @@ -447,13 +450,18 @@ impl FilteredPushRules {
.iter()
.filter(|rule| {
// Ignore disabled experimental push rules

if !self.msc1767_enabled && rule.rule_id.contains("org.matrix.msc1767") {
return false;
}

if !self.msc3664_enabled
&& rule.rule_id == "global/override/.im.nheko.msc3664.reply"
{
return false;
}

if !self.msc1767_enabled && rule.rule_id.contains("org.matrix.msc1767") {
if !self.msc3381_polls_enabled && rule.rule_id.contains("org.matrix.msc3930") {
return false;
}

Expand Down
2 changes: 1 addition & 1 deletion scripts-dev/complement.sh
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ fi

extra_test_args=()

test_tags="synapse_blacklist,msc3787,msc3874,msc3890,msc3391"
test_tags="synapse_blacklist,msc3787,msc3874,msc3890,msc3391,msc3930"

# All environment variables starting with PASS_ will be shared.
# (The prefix is stripped off before reaching the container.)
Expand Down
3 changes: 2 additions & 1 deletion stubs/synapse/synapse_rust/push.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,9 @@ class FilteredPushRules:
self,
push_rules: PushRules,
enabled_map: Dict[str, bool],
msc3664_enabled: bool,
msc1767_enabled: bool,
msc3381_polls_enabled: bool,
msc3664_enabled: bool,
): ...
def rules(self) -> Collection[Tuple[PushRule, bool]]: ...

Expand Down
7 changes: 7 additions & 0 deletions synapse/config/experimental.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,13 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
"required to communicate account data deletions to clients."
)

# MSC3381: Polls.
# In practice, supporting polls in Synapse only requires an implementation of
# MSC3930: Push rules for MSC3391 polls; which is what this option enables.
self.msc3381_polls_enabled: bool = experimental.get(
"msc3381_polls_enabled", False
)

# MSC3912: Relation-based redactions.
self.msc3912_enabled: bool = experimental.get("msc3912_enabled", False)

Expand Down
3 changes: 2 additions & 1 deletion synapse/storage/databases/main/push_rule.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,9 @@ def _load_rules(
filtered_rules = FilteredPushRules(
push_rules,
enabled_map,
msc3664_enabled=experimental_config.msc3664_enabled,
msc1767_enabled=experimental_config.msc1767_enabled,
msc3664_enabled=experimental_config.msc3664_enabled,
msc3381_polls_enabled=experimental_config.msc3381_polls_enabled,
)

return filtered_rules
Expand Down