From 9725c59247131d243316ff299e6864098d9bdc58 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 28 Jul 2020 19:20:55 +0100 Subject: [PATCH 01/13] Implement new experimental push rules with a database hack to enable them --- synapse/push/baserules.py | 217 +++++++++++++++++- synapse/storage/data_stores/main/push_rule.py | 35 ++- .../schema/delta/58/13new_push_rules_tmp.sql | 21 ++ 3 files changed, 259 insertions(+), 14 deletions(-) create mode 100644 synapse/storage/data_stores/main/schema/delta/58/13new_push_rules_tmp.sql diff --git a/synapse/push/baserules.py b/synapse/push/baserules.py index 286374d0b..e06b1a01e 100644 --- a/synapse/push/baserules.py +++ b/synapse/push/baserules.py @@ -19,7 +19,7 @@ import copy from synapse.push.rulekinds import PRIORITY_CLASS_INVERSE_MAP, PRIORITY_CLASS_MAP -def list_with_base_rules(rawrules): +def list_with_base_rules(rawrules, use_new_defaults=False): """Combine the list of rules set by the user with the default push rules Args: @@ -43,7 +43,9 @@ def list_with_base_rules(rawrules): ruleslist.extend( make_base_prepend_rules( - PRIORITY_CLASS_INVERSE_MAP[current_prio_class], modified_base_rules + PRIORITY_CLASS_INVERSE_MAP[current_prio_class], + modified_base_rules, + use_new_defaults, ) ) @@ -54,6 +56,7 @@ def list_with_base_rules(rawrules): make_base_append_rules( PRIORITY_CLASS_INVERSE_MAP[current_prio_class], modified_base_rules, + use_new_defaults, ) ) current_prio_class -= 1 @@ -62,6 +65,7 @@ def list_with_base_rules(rawrules): make_base_prepend_rules( PRIORITY_CLASS_INVERSE_MAP[current_prio_class], modified_base_rules, + use_new_defaults, ) ) @@ -70,27 +74,31 @@ def list_with_base_rules(rawrules): while current_prio_class > 0: ruleslist.extend( make_base_append_rules( - PRIORITY_CLASS_INVERSE_MAP[current_prio_class], modified_base_rules + PRIORITY_CLASS_INVERSE_MAP[current_prio_class], + modified_base_rules, + use_new_defaults, ) ) current_prio_class -= 1 if current_prio_class > 0: ruleslist.extend( make_base_prepend_rules( - PRIORITY_CLASS_INVERSE_MAP[current_prio_class], modified_base_rules + PRIORITY_CLASS_INVERSE_MAP[current_prio_class], + modified_base_rules, + use_new_defaults, ) ) return ruleslist -def make_base_append_rules(kind, modified_base_rules): +def make_base_append_rules(kind, modified_base_rules, use_new_defaults=False): rules = [] if kind == "override": - rules = BASE_APPEND_OVERRIDE_RULES + rules = NEW_APPEND_OVERRIDE_RULES if use_new_defaults else BASE_APPEND_OVERRIDE_RULES elif kind == "underride": - rules = BASE_APPEND_UNDERRIDE_RULES + rules = NEW_APPEND_UNDERRIDE_RULES if use_new_defaults else BASE_APPEND_UNDERRIDE_RULES elif kind == "content": rules = BASE_APPEND_CONTENT_RULES @@ -105,11 +113,11 @@ def make_base_append_rules(kind, modified_base_rules): return rules -def make_base_prepend_rules(kind, modified_base_rules): +def make_base_prepend_rules(kind, modified_base_rules, use_new_defaults=False): rules = [] if kind == "override": - rules = BASE_PREPEND_OVERRIDE_RULES + rules = NEW_PREPEND_OVERRIDE_RULES if use_new_defaults else BASE_PREPEND_OVERRIDE_RULES # Copy the rules before modifying them rules = copy.deepcopy(rules) @@ -151,6 +159,16 @@ BASE_PREPEND_OVERRIDE_RULES = [ ] +NEW_PREPEND_OVERRIDE_RULES = [ + { + "rule_id": "global/override/.m.rule.master", + "enabled": False, + "conditions": [], + "actions": [], + } +] + + BASE_APPEND_OVERRIDE_RULES = [ { "rule_id": "global/override/.m.rule.suppress_notices", @@ -270,6 +288,141 @@ BASE_APPEND_OVERRIDE_RULES = [ ] +NEW_APPEND_OVERRIDE_RULES = [ + { + "rule_id": "global/override/.m.rule.encrypted", + "conditions": [ + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.encrypted", + "_id": "_encrypted", + } + ], + "actions": ["notify"], + }, + { + "rule_id": "global/override/.m.rule.suppress_notices", + "conditions": [ + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.message", + "_id": "_suppress_notices_type", + }, + { + "kind": "event_match", + "key": "content.msgtype", + "pattern": "m.notice", + "_id": "_suppress_notices", + } + ], + "actions": [], + }, + { + "rule_id": "global/underride/.m.rule.suppress_edits", + "conditions": [ + { + "kind": "event_match", + "key": "m.relates_to.m.rel_type", + "pattern": "m.replace", + "_id": "_suppress_edits", + } + ], + "actions": [], + }, + { + "rule_id": "global/override/.m.rule.invite_for_me", + "conditions": [ + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.member", + "_id": "_member", + }, + { + "kind": "event_match", + "key": "content.membership", + "pattern": "invite", + "_id": "_invite_member", + }, + {"kind": "event_match", "key": "state_key", "pattern_type": "user_id"}, + ], + "actions": [ + "notify", + {"set_tweak": "sound", "value": "default"}, + ], + }, + { + "rule_id": "global/override/.m.rule.contains_display_name", + "conditions": [{"kind": "contains_display_name"}], + "actions": [ + "notify", + {"set_tweak": "sound", "value": "default"}, + {"set_tweak": "highlight"}, + ], + }, + { + "rule_id": "global/override/.m.rule.tombstone", + "conditions": [ + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.tombstone", + "_id": "_tombstone", + }, + { + "kind": "event_match", + "key": "state_key", + "pattern": "", + "_id": "_tombstone_statekey", + }, + ], + "actions": [ + "notify", + {"set_tweak": "sound", "value": "default"}, + {"set_tweak": "highlight"}, + ], + }, + { + "rule_id": "global/override/.m.rule.roomnotif", + "conditions": [ + { + "kind": "event_match", + "key": "content.body", + "pattern": "@room", + "_id": "_roomnotif_content", + }, + { + "kind": "sender_notification_permission", + "key": "room", + "_id": "_roomnotif_pl", + }, + ], + "actions": [ + "notify", + {"set_tweak": "highlight"}, + {"set_tweak": "sound", "value": "default"}, + ], + }, + { + "rule_id": "global/override/.m.rule.call", + "conditions": [ + { + "kind": "event_match", + "key": "type", + "pattern": "m.call.invite", + "_id": "_call", + } + ], + "actions": [ + "notify", + {"set_tweak": "sound", "value": "ring"}, + ], + } +] + + BASE_APPEND_UNDERRIDE_RULES = [ { "rule_id": "global/underride/.m.rule.call", @@ -354,6 +507,29 @@ BASE_APPEND_UNDERRIDE_RULES = [ ] +NEW_APPEND_UNDERRIDE_RULES = [ + { + "rule_id": "global/underride/.m.rule.room_one_to_one", + "conditions": [ + {"kind": "room_member_count", "is": "2", "_id": "member_count"}, + {"kind": "event_match", "key": "content.body", "pattern": "*", "_id": "body"}, + ], + "actions": [ + "notify", + {"set_tweak": "sound", "value": "default"}, + ], + }, + { + "rule_id": "global/underride/.m.rule.message", + "conditions": [ + {"kind": "event_match", "key": "content.body", "pattern": "*", "_id": "body"}, + ], + "actions": ["notify"], + "enabled": False, + }, +] + + BASE_RULE_IDS = set() for r in BASE_APPEND_CONTENT_RULES: @@ -375,3 +551,26 @@ for r in BASE_APPEND_UNDERRIDE_RULES: r["priority_class"] = PRIORITY_CLASS_MAP["underride"] r["default"] = True BASE_RULE_IDS.add(r["rule_id"]) + + +NEW_RULE_IDS = set() + +for r in BASE_APPEND_CONTENT_RULES: + r["priority_class"] = PRIORITY_CLASS_MAP["content"] + r["default"] = True + NEW_RULE_IDS.add(r["rule_id"]) + +for r in NEW_PREPEND_OVERRIDE_RULES: + r["priority_class"] = PRIORITY_CLASS_MAP["override"] + r["default"] = True + NEW_RULE_IDS.add(r["rule_id"]) + +for r in NEW_APPEND_OVERRIDE_RULES: + r["priority_class"] = PRIORITY_CLASS_MAP["override"] + r["default"] = True + NEW_RULE_IDS.add(r["rule_id"]) + +for r in NEW_APPEND_UNDERRIDE_RULES: + r["priority_class"] = PRIORITY_CLASS_MAP["underride"] + r["default"] = True + NEW_RULE_IDS.add(r["rule_id"]) diff --git a/synapse/storage/data_stores/main/push_rule.py b/synapse/storage/data_stores/main/push_rule.py index d181488db..c10da245d 100644 --- a/synapse/storage/data_stores/main/push_rule.py +++ b/synapse/storage/data_stores/main/push_rule.py @@ -39,7 +39,7 @@ from synapse.util.caches.stream_change_cache import StreamChangeCache logger = logging.getLogger(__name__) -def _load_rules(rawrules, enabled_map): +def _load_rules(rawrules, enabled_map, use_new_defaults=False): ruleslist = [] for rawrule in rawrules: rule = dict(rawrule) @@ -49,7 +49,7 @@ def _load_rules(rawrules, enabled_map): ruleslist.append(rule) # We're going to be mutating this a lot, so do a deep copy - rules = list(list_with_base_rules(ruleslist)) + rules = list(list_with_base_rules(ruleslist, use_new_defaults)) for i, rule in enumerate(rules): rule_id = rule["rule_id"] @@ -115,7 +115,7 @@ class PushRulesWorkerStore( raise NotImplementedError() @cachedInlineCallbacks(max_entries=5000) - def get_push_rules_for_user(self, user_id): + def _get_push_rules_for_user(self, user_id, use_new_defaults=False): rows = yield self.db.simple_select_list( table="push_rules", keyvalues={"user_name": user_id}, @@ -134,10 +134,24 @@ class PushRulesWorkerStore( enabled_map = yield self.get_push_rules_enabled_for_user(user_id) - rules = _load_rules(rows, enabled_map) + rules = _load_rules(rows, enabled_map, use_new_defaults) return rules + @defer.inlineCallbacks + def get_push_rules_for_user(self, user_id): + # Temporary hack so we can use the new experimental default push rules to some + # users without impacting others. + use_new_defaults = yield self.db.simple_select_list( + table="new_push_rules_users_tmp", + keyvalues={"user_id": user_id}, + retcols=("user_id",), + desc="get_user_new_default_push_rules", + ) + + rules = yield self._get_push_rules_for_user(user_id, bool(use_new_defaults)) + return rules + @cachedInlineCallbacks(max_entries=5000) def get_push_rules_enabled_for_user(self, user_id): results = yield self.db.simple_select_list( @@ -194,7 +208,18 @@ class PushRulesWorkerStore( enabled_map_by_user = yield self.bulk_get_push_rules_enabled(user_ids) for user_id, rules in results.items(): - results[user_id] = _load_rules(rules, enabled_map_by_user.get(user_id, {})) + # Temporary hack so we can use the new experimental default push rules to some + # users without impacting others. + use_new_defaults = yield self.db.simple_select_list( + table="new_push_rules_users_tmp", + keyvalues={"user_id": user_id}, + retcols=("user_id",), + desc="get_user_new_default_push_rules", + ) + + results[user_id] = _load_rules( + rules, enabled_map_by_user.get(user_id, {}), bool(use_new_defaults), + ) return results diff --git a/synapse/storage/data_stores/main/schema/delta/58/13new_push_rules_tmp.sql b/synapse/storage/data_stores/main/schema/delta/58/13new_push_rules_tmp.sql new file mode 100644 index 000000000..b7daf1c67 --- /dev/null +++ b/synapse/storage/data_stores/main/schema/delta/58/13new_push_rules_tmp.sql @@ -0,0 +1,21 @@ +/* Copyright 2020 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. + */ + +-- This is a temporary table in which we store the IDs of the users for which we need to +-- serve the new experimental default push rules. The purpose of this table is to help +-- test these new defaults, so it shall be dropped when the experimentation is done. +CREATE TABLE IF NOT EXISTS new_push_rules_users_tmp ( + user_id TEXT PRIMARY KEY +); \ No newline at end of file From 8b04c4cd70fea2114e1ca1d0daab93be56a4f382 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 30 Jul 2020 17:43:17 +0100 Subject: [PATCH 02/13] Changelog --- changelog.d/7997.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/7997.misc diff --git a/changelog.d/7997.misc b/changelog.d/7997.misc new file mode 100644 index 000000000..fd53674bc --- /dev/null +++ b/changelog.d/7997.misc @@ -0,0 +1 @@ +Implement new experimental push rules for some users. From 60328ce9fbe90299253ba740f2648c42b9091920 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 30 Jul 2020 19:02:28 +0100 Subject: [PATCH 03/13] Lint --- synapse/push/baserules.py | 51 ++++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/synapse/push/baserules.py b/synapse/push/baserules.py index e06b1a01e..172fd00f1 100644 --- a/synapse/push/baserules.py +++ b/synapse/push/baserules.py @@ -96,9 +96,17 @@ def make_base_append_rules(kind, modified_base_rules, use_new_defaults=False): rules = [] if kind == "override": - rules = NEW_APPEND_OVERRIDE_RULES if use_new_defaults else BASE_APPEND_OVERRIDE_RULES + rules = ( + NEW_APPEND_OVERRIDE_RULES + if use_new_defaults + else BASE_APPEND_OVERRIDE_RULES + ) elif kind == "underride": - rules = NEW_APPEND_UNDERRIDE_RULES if use_new_defaults else BASE_APPEND_UNDERRIDE_RULES + rules = ( + NEW_APPEND_UNDERRIDE_RULES + if use_new_defaults + else BASE_APPEND_UNDERRIDE_RULES + ) elif kind == "content": rules = BASE_APPEND_CONTENT_RULES @@ -117,7 +125,11 @@ def make_base_prepend_rules(kind, modified_base_rules, use_new_defaults=False): rules = [] if kind == "override": - rules = NEW_PREPEND_OVERRIDE_RULES if use_new_defaults else BASE_PREPEND_OVERRIDE_RULES + rules = ( + NEW_PREPEND_OVERRIDE_RULES + if use_new_defaults + else BASE_PREPEND_OVERRIDE_RULES + ) # Copy the rules before modifying them rules = copy.deepcopy(rules) @@ -315,7 +327,7 @@ NEW_APPEND_OVERRIDE_RULES = [ "key": "content.msgtype", "pattern": "m.notice", "_id": "_suppress_notices", - } + }, ], "actions": [], }, @@ -348,10 +360,7 @@ NEW_APPEND_OVERRIDE_RULES = [ }, {"kind": "event_match", "key": "state_key", "pattern_type": "user_id"}, ], - "actions": [ - "notify", - {"set_tweak": "sound", "value": "default"}, - ], + "actions": ["notify", {"set_tweak": "sound", "value": "default"}], }, { "rule_id": "global/override/.m.rule.contains_display_name", @@ -415,11 +424,8 @@ NEW_APPEND_OVERRIDE_RULES = [ "_id": "_call", } ], - "actions": [ - "notify", - {"set_tweak": "sound", "value": "ring"}, - ], - } + "actions": ["notify", {"set_tweak": "sound", "value": "ring"}], + }, ] @@ -512,17 +518,24 @@ NEW_APPEND_UNDERRIDE_RULES = [ "rule_id": "global/underride/.m.rule.room_one_to_one", "conditions": [ {"kind": "room_member_count", "is": "2", "_id": "member_count"}, - {"kind": "event_match", "key": "content.body", "pattern": "*", "_id": "body"}, - ], - "actions": [ - "notify", - {"set_tweak": "sound", "value": "default"}, + { + "kind": "event_match", + "key": "content.body", + "pattern": "*", + "_id": "body", + }, ], + "actions": ["notify", {"set_tweak": "sound", "value": "default"}], }, { "rule_id": "global/underride/.m.rule.message", "conditions": [ - {"kind": "event_match", "key": "content.body", "pattern": "*", "_id": "body"}, + { + "kind": "event_match", + "key": "content.body", + "pattern": "*", + "_id": "body", + }, ], "actions": ["notify"], "enabled": False, From 79d991eff060abaa074ef23201f0e68cc8228e7e Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 31 Jul 2020 13:58:42 +0100 Subject: [PATCH 04/13] Fix cache invalidation calls --- synapse/replication/slave/storage/push_rule.py | 2 +- synapse/storage/data_stores/main/push_rule.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/replication/slave/storage/push_rule.py b/synapse/replication/slave/storage/push_rule.py index 23ec1c5b1..6bebd4d5c 100644 --- a/synapse/replication/slave/storage/push_rule.py +++ b/synapse/replication/slave/storage/push_rule.py @@ -34,7 +34,7 @@ class SlavedPushRuleStore(SlavedEventStore, PushRulesWorkerStore): if stream_name == PushRulesStream.NAME: self._push_rules_stream_id_gen.advance(token) for row in rows: - self.get_push_rules_for_user.invalidate((row.user_id,)) + self._get_push_rules_for_user.invalidate((row.user_id,)) self.get_push_rules_enabled_for_user.invalidate((row.user_id,)) self.push_rules_stream_cache.entity_has_changed(row.user_id, token) return super().process_replication_rows(stream_name, instance_name, token, rows) diff --git a/synapse/storage/data_stores/main/push_rule.py b/synapse/storage/data_stores/main/push_rule.py index 861050814..85cd24ce7 100644 --- a/synapse/storage/data_stores/main/push_rule.py +++ b/synapse/storage/data_stores/main/push_rule.py @@ -768,7 +768,7 @@ class PushRuleStore(PushRulesWorkerStore): self.db.simple_insert_txn(txn, "push_rules_stream", values=values) - txn.call_after(self.get_push_rules_for_user.invalidate, (user_id,)) + txn.call_after(self._get_push_rules_for_user.invalidate, (user_id,)) txn.call_after(self.get_push_rules_enabled_for_user.invalidate, (user_id,)) txn.call_after( self.push_rules_stream_cache.entity_has_changed, user_id, stream_id From cf42d0a60cce6cbb4b56f58bdb25d7a90e3ecff6 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 31 Jul 2020 15:06:41 +0100 Subject: [PATCH 05/13] Fix cache name --- synapse/storage/data_stores/main/push_rule.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/data_stores/main/push_rule.py b/synapse/storage/data_stores/main/push_rule.py index 85cd24ce7..267fc5f5a 100644 --- a/synapse/storage/data_stores/main/push_rule.py +++ b/synapse/storage/data_stores/main/push_rule.py @@ -181,7 +181,7 @@ class PushRulesWorkerStore( ) @cachedList( - cached_method_name="get_push_rules_for_user", + cached_method_name="_get_push_rules_for_user", list_name="user_ids", num_args=1, inlineCallbacks=True, From 1678057b56a82467c25259d4727a69097dad0ea3 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 3 Aug 2020 11:22:22 +0100 Subject: [PATCH 06/13] Back out the database hack and replace it with a temporary config setting --- synapse/config/server.py | 10 ++++++ .../replication/slave/storage/push_rule.py | 2 +- synapse/storage/data_stores/main/push_rule.py | 35 +++++-------------- .../schema/delta/58/13new_push_rules_tmp.sql | 21 ----------- 4 files changed, 20 insertions(+), 48 deletions(-) delete mode 100644 synapse/storage/data_stores/main/schema/delta/58/13new_push_rules_tmp.sql diff --git a/synapse/config/server.py b/synapse/config/server.py index 848587d23..68d143410 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -530,6 +530,16 @@ class ServerConfig(Config): "request_token_inhibit_3pid_errors", False, ) + # List of users trialing the new experimental default push rules. This setting is + # not included in the sample configuration file on purpose as it's a temporary + # hack, so that some users can trial the new defaults without impacting every + # user on the homeserver. + self.users_new_default_push_rules = ( + config.get("users_new_default_push_rules") or [] + ) + if not isinstance(self.users_new_default_push_rules, list): + raise ConfigError("'users_new_default_push_rules' must be a list") + def has_tls_listener(self) -> bool: return any(listener.tls for listener in self.listeners) diff --git a/synapse/replication/slave/storage/push_rule.py b/synapse/replication/slave/storage/push_rule.py index 6bebd4d5c..23ec1c5b1 100644 --- a/synapse/replication/slave/storage/push_rule.py +++ b/synapse/replication/slave/storage/push_rule.py @@ -34,7 +34,7 @@ class SlavedPushRuleStore(SlavedEventStore, PushRulesWorkerStore): if stream_name == PushRulesStream.NAME: self._push_rules_stream_id_gen.advance(token) for row in rows: - self._get_push_rules_for_user.invalidate((row.user_id,)) + self.get_push_rules_for_user.invalidate((row.user_id,)) self.get_push_rules_enabled_for_user.invalidate((row.user_id,)) self.push_rules_stream_cache.entity_has_changed(row.user_id, token) return super().process_replication_rows(stream_name, instance_name, token, rows) diff --git a/synapse/storage/data_stores/main/push_rule.py b/synapse/storage/data_stores/main/push_rule.py index 267fc5f5a..d644a0b8c 100644 --- a/synapse/storage/data_stores/main/push_rule.py +++ b/synapse/storage/data_stores/main/push_rule.py @@ -105,6 +105,8 @@ class PushRulesWorkerStore( prefilled_cache=push_rules_prefill, ) + self.users_new_default_push_rules = hs.config.users_new_default_push_rules + @abc.abstractmethod def get_max_push_rules_stream_id(self): """Get the position of the push rules stream. @@ -115,7 +117,7 @@ class PushRulesWorkerStore( raise NotImplementedError() @cachedInlineCallbacks(max_entries=5000) - def _get_push_rules_for_user(self, user_id, use_new_defaults=False): + def get_push_rules_for_user(self, user_id): rows = yield self.db.simple_select_list( table="push_rules", keyvalues={"user_name": user_id}, @@ -134,24 +136,12 @@ class PushRulesWorkerStore( enabled_map = yield self.get_push_rules_enabled_for_user(user_id) + use_new_defaults = user_id in self.users_new_default_push_rules + rules = _load_rules(rows, enabled_map, use_new_defaults) return rules - @defer.inlineCallbacks - def get_push_rules_for_user(self, user_id): - # Temporary hack so we can use the new experimental default push rules to some - # users without impacting others. - use_new_defaults = yield self.db.simple_select_list( - table="new_push_rules_users_tmp", - keyvalues={"user_id": user_id}, - retcols=("user_id",), - desc="get_user_new_default_push_rules", - ) - - rules = yield self._get_push_rules_for_user(user_id, bool(use_new_defaults)) - return rules - @cachedInlineCallbacks(max_entries=5000) def get_push_rules_enabled_for_user(self, user_id): results = yield self.db.simple_select_list( @@ -181,7 +171,7 @@ class PushRulesWorkerStore( ) @cachedList( - cached_method_name="_get_push_rules_for_user", + cached_method_name="get_push_rules_for_user", list_name="user_ids", num_args=1, inlineCallbacks=True, @@ -208,17 +198,10 @@ class PushRulesWorkerStore( enabled_map_by_user = yield self.bulk_get_push_rules_enabled(user_ids) for user_id, rules in results.items(): - # Temporary hack so we can use the new experimental default push rules to some - # users without impacting others. - use_new_defaults = yield self.db.simple_select_list( - table="new_push_rules_users_tmp", - keyvalues={"user_id": user_id}, - retcols=("user_id",), - desc="get_user_new_default_push_rules", - ) + use_new_defaults = user_id in self.users_new_default_push_rules results[user_id] = _load_rules( - rules, enabled_map_by_user.get(user_id, {}), bool(use_new_defaults), + rules, enabled_map_by_user.get(user_id, {}), use_new_defaults, ) return results @@ -768,7 +751,7 @@ class PushRuleStore(PushRulesWorkerStore): self.db.simple_insert_txn(txn, "push_rules_stream", values=values) - txn.call_after(self._get_push_rules_for_user.invalidate, (user_id,)) + txn.call_after(self.get_push_rules_for_user.invalidate, (user_id,)) txn.call_after(self.get_push_rules_enabled_for_user.invalidate, (user_id,)) txn.call_after( self.push_rules_stream_cache.entity_has_changed, user_id, stream_id diff --git a/synapse/storage/data_stores/main/schema/delta/58/13new_push_rules_tmp.sql b/synapse/storage/data_stores/main/schema/delta/58/13new_push_rules_tmp.sql deleted file mode 100644 index b7daf1c67..000000000 --- a/synapse/storage/data_stores/main/schema/delta/58/13new_push_rules_tmp.sql +++ /dev/null @@ -1,21 +0,0 @@ -/* Copyright 2020 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. - */ - --- This is a temporary table in which we store the IDs of the users for which we need to --- serve the new experimental default push rules. The purpose of this table is to help --- test these new defaults, so it shall be dropped when the experimentation is done. -CREATE TABLE IF NOT EXISTS new_push_rules_users_tmp ( - user_id TEXT PRIMARY KEY -); \ No newline at end of file From e2f1cccc8aec83f265220283c1f9d3707d49bb7e Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 3 Aug 2020 11:52:52 +0100 Subject: [PATCH 07/13] Fix PUT /pushrules to use the right rule IDs --- synapse/rest/client/v1/push_rule.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/synapse/rest/client/v1/push_rule.py b/synapse/rest/client/v1/push_rule.py index 9fd490813..f66b8fa7c 100644 --- a/synapse/rest/client/v1/push_rule.py +++ b/synapse/rest/client/v1/push_rule.py @@ -25,7 +25,7 @@ from synapse.http.servlet import ( parse_json_value_from_request, parse_string, ) -from synapse.push.baserules import BASE_RULE_IDS +from synapse.push.baserules import BASE_RULE_IDS, NEW_RULE_IDS from synapse.push.clientformat import format_push_rules_for_user from synapse.push.rulekinds import PRIORITY_CLASS_MAP from synapse.rest.client.v2_alpha._base import client_patterns @@ -45,6 +45,8 @@ class PushRuleRestServlet(RestServlet): self.notifier = hs.get_notifier() self._is_worker = hs.config.worker_app is not None + self.users_new_default_push_rules = hs.config.users_new_default_push_rules + async def on_PUT(self, request, path): if self._is_worker: raise Exception("Cannot handle PUT /push_rules on worker") @@ -179,7 +181,12 @@ class PushRuleRestServlet(RestServlet): rule_id = spec["rule_id"] is_default_rule = rule_id.startswith(".") if is_default_rule: - if namespaced_rule_id not in BASE_RULE_IDS: + if user_id in self.users_new_default_push_rules: + rule_ids = NEW_RULE_IDS + else: + rule_ids = BASE_RULE_IDS + + if namespaced_rule_id not in rule_ids: raise SynapseError(404, "Unknown rule %r" % (namespaced_rule_id,)) return self.store.set_push_rule_actions( user_id, namespaced_rule_id, actions, is_default_rule From dd11f575a29b59aced6cfa7ea7b9faea6f968f8d Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 6 Aug 2020 10:52:26 +0100 Subject: [PATCH 08/13] Incorporate review --- synapse/config/server.py | 3 +++ synapse/push/baserules.py | 20 ++++--------------- synapse/rest/client/v1/push_rule.py | 4 ++-- synapse/storage/data_stores/main/push_rule.py | 6 +++--- 4 files changed, 12 insertions(+), 21 deletions(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index 68d143410..00fa07c22 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -540,6 +540,9 @@ class ServerConfig(Config): if not isinstance(self.users_new_default_push_rules, list): raise ConfigError("'users_new_default_push_rules' must be a list") + # Turn the list into a set to improve lookup speed. + self.users_new_default_push_rules = set(self.users_new_default_push_rules) + def has_tls_listener(self) -> bool: return any(listener.tls for listener in self.listeners) diff --git a/synapse/push/baserules.py b/synapse/push/baserules.py index 172fd00f1..8047873ff 100644 --- a/synapse/push/baserules.py +++ b/synapse/push/baserules.py @@ -24,6 +24,8 @@ def list_with_base_rules(rawrules, use_new_defaults=False): Args: rawrules(list): The rules the user has modified or set. + use_new_defaults(bool): Whether to use the new experimental default rules when + appending or prepending default rules. Returns: A new list with the rules set by the user combined with the defaults. @@ -125,11 +127,7 @@ def make_base_prepend_rules(kind, modified_base_rules, use_new_defaults=False): rules = [] if kind == "override": - rules = ( - NEW_PREPEND_OVERRIDE_RULES - if use_new_defaults - else BASE_PREPEND_OVERRIDE_RULES - ) + rules = BASE_PREPEND_OVERRIDE_RULES # Copy the rules before modifying them rules = copy.deepcopy(rules) @@ -171,16 +169,6 @@ BASE_PREPEND_OVERRIDE_RULES = [ ] -NEW_PREPEND_OVERRIDE_RULES = [ - { - "rule_id": "global/override/.m.rule.master", - "enabled": False, - "conditions": [], - "actions": [], - } -] - - BASE_APPEND_OVERRIDE_RULES = [ { "rule_id": "global/override/.m.rule.suppress_notices", @@ -573,7 +561,7 @@ for r in BASE_APPEND_CONTENT_RULES: r["default"] = True NEW_RULE_IDS.add(r["rule_id"]) -for r in NEW_PREPEND_OVERRIDE_RULES: +for r in BASE_PREPEND_OVERRIDE_RULES: r["priority_class"] = PRIORITY_CLASS_MAP["override"] r["default"] = True NEW_RULE_IDS.add(r["rule_id"]) diff --git a/synapse/rest/client/v1/push_rule.py b/synapse/rest/client/v1/push_rule.py index f66b8fa7c..00831879f 100644 --- a/synapse/rest/client/v1/push_rule.py +++ b/synapse/rest/client/v1/push_rule.py @@ -45,7 +45,7 @@ class PushRuleRestServlet(RestServlet): self.notifier = hs.get_notifier() self._is_worker = hs.config.worker_app is not None - self.users_new_default_push_rules = hs.config.users_new_default_push_rules + self._users_new_default_push_rules = hs.config.users_new_default_push_rules async def on_PUT(self, request, path): if self._is_worker: @@ -181,7 +181,7 @@ class PushRuleRestServlet(RestServlet): rule_id = spec["rule_id"] is_default_rule = rule_id.startswith(".") if is_default_rule: - if user_id in self.users_new_default_push_rules: + if user_id in self._users_new_default_push_rules: rule_ids = NEW_RULE_IDS else: rule_ids = BASE_RULE_IDS diff --git a/synapse/storage/data_stores/main/push_rule.py b/synapse/storage/data_stores/main/push_rule.py index d644a0b8c..6b650f8ba 100644 --- a/synapse/storage/data_stores/main/push_rule.py +++ b/synapse/storage/data_stores/main/push_rule.py @@ -105,7 +105,7 @@ class PushRulesWorkerStore( prefilled_cache=push_rules_prefill, ) - self.users_new_default_push_rules = hs.config.users_new_default_push_rules + self._users_new_default_push_rules = hs.config.users_new_default_push_rules @abc.abstractmethod def get_max_push_rules_stream_id(self): @@ -136,7 +136,7 @@ class PushRulesWorkerStore( enabled_map = yield self.get_push_rules_enabled_for_user(user_id) - use_new_defaults = user_id in self.users_new_default_push_rules + use_new_defaults = user_id in self._users_new_default_push_rules rules = _load_rules(rows, enabled_map, use_new_defaults) @@ -198,7 +198,7 @@ class PushRulesWorkerStore( enabled_map_by_user = yield self.bulk_get_push_rules_enabled(user_ids) for user_id, rules in results.items(): - use_new_defaults = user_id in self.users_new_default_push_rules + use_new_defaults = user_id in self._users_new_default_push_rules results[user_id] = _load_rules( rules, enabled_map_by_user.get(user_id, {}), use_new_defaults, From bf33d5c457643c0b69ebcc26401b7f3090a09e6c Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 6 Aug 2020 17:52:34 +0100 Subject: [PATCH 09/13] Incorporate review --- synapse/config/server.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index 00fa07c22..22673be9d 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -534,10 +534,10 @@ class ServerConfig(Config): # not included in the sample configuration file on purpose as it's a temporary # hack, so that some users can trial the new defaults without impacting every # user on the homeserver. - self.users_new_default_push_rules = ( + users_new_default_push_rules = ( config.get("users_new_default_push_rules") or [] ) - if not isinstance(self.users_new_default_push_rules, list): + if not isinstance(users_new_default_push_rules, list): raise ConfigError("'users_new_default_push_rules' must be a list") # Turn the list into a set to improve lookup speed. From 367e9e6e9ec081a08361cff14e248dd03610f57f Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 6 Aug 2020 17:57:58 +0100 Subject: [PATCH 10/13] Lint --- synapse/config/server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index 22673be9d..23ddd0ab0 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -536,7 +536,7 @@ class ServerConfig(Config): # user on the homeserver. users_new_default_push_rules = ( config.get("users_new_default_push_rules") or [] - ) + ) # type: list if not isinstance(users_new_default_push_rules, list): raise ConfigError("'users_new_default_push_rules' must be a list") From cee6c6012ed11f1b8407e57679ab66ad308ff590 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 10 Aug 2020 11:10:34 +0100 Subject: [PATCH 11/13] why mypy why --- synapse/config/server.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index 23ddd0ab0..493d2bd51 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -541,7 +541,9 @@ class ServerConfig(Config): raise ConfigError("'users_new_default_push_rules' must be a list") # Turn the list into a set to improve lookup speed. - self.users_new_default_push_rules = set(self.users_new_default_push_rules) + self.users_new_default_push_rules = ( + set(self.users_new_default_push_rules) + ) # type: set def has_tls_listener(self) -> bool: return any(listener.tls for listener in self.listeners) From 1a3aabcf3facd38f6d4c58fb4f55ad79450acefa Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 10 Aug 2020 11:13:21 +0100 Subject: [PATCH 12/13] Lint --- synapse/config/server.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index 493d2bd51..b5c2098ec 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -541,8 +541,8 @@ class ServerConfig(Config): raise ConfigError("'users_new_default_push_rules' must be a list") # Turn the list into a set to improve lookup speed. - self.users_new_default_push_rules = ( - set(self.users_new_default_push_rules) + self.users_new_default_push_rules = set( + self.users_new_default_push_rules ) # type: set def has_tls_listener(self) -> bool: From 5c43c43240a85ca6b65ad327b6a5b1c9e29bd653 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 10 Aug 2020 11:23:24 +0100 Subject: [PATCH 13/13] Typo --- synapse/config/server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index b5c2098ec..9f15ed109 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -542,7 +542,7 @@ class ServerConfig(Config): # Turn the list into a set to improve lookup speed. self.users_new_default_push_rules = set( - self.users_new_default_push_rules + users_new_default_push_rules ) # type: set def has_tls_listener(self) -> bool: