remove support for per-user msc2654

Author: H-Shay

changelog.d/15345.feature

@@ -1,3 +1,3 @@
 Follow-up to adding experimental feature flags per-user (#15344) which moves experimental features MSC3026 (busy presence),
-MSC2654 (unread counts), MSC3881 (remotely toggle push notifications for another client), and
-MSC3967 (Do not require UIA when first uploading cross signing keys) from the experimental config to per-user flags.
+MSC3881 (remotely toggle push notifications for another client), and MSC3967 (Do not require UIA when first uploading
+cross signing keys) from the experimental config to per-user flags.

synapse/push/bulk_push_rule_evaluator.py

@@ -27,7 +27,6 @@
     Union,
 )
 
-import attr
 from prometheus_client import Counter
 
 from synapse.api.constants import (
@@ -108,17 +107,6 @@ def _should_count_as_unread(event: EventBase, context: EventContext) -> bool:
     return False
 
 
-@attr.s(slots=True, auto_attribs=True)
-class ActionsForUser:
-    """
-    A class to hold the actions for a given event and whether the event should
-    increment the unread count.
-    """
-
-    actions: Collection[Union[Mapping, str]]
-    count_as_unread: bool
-
-
 class BulkPushRuleEvaluator:
     """Calculates the outcome of push rules for an event for all users in the
     room at once.
@@ -348,8 +336,15 @@ async def _action_for_event_by_user(
             # (historical messages persisted in reverse-chronological order).
             return
 
+        # Disable counting as unread unless the experimental configuration is
+        # enabled, as it can cause additional (unwanted) rows to be added to the
+        # event_push_actions table.
+        count_as_unread = False
+        if self.hs.config.experimental.msc2654_enabled:
+            count_as_unread = _should_count_as_unread(event, context)
+
         rules_by_user = await self._get_rules_for_event(event)
-        actions_by_user: Dict[str, ActionsForUser] = {}
+        actions_by_user: Dict[str, Collection[Union[Mapping, str]]] = {}
 
         room_member_count = await self.store.get_number_joined_users_in_room(
             event.room_id
@@ -434,22 +429,17 @@ async def _action_for_event_by_user(
                     if not isinstance(display_name, str):
                         display_name = None
 
-            actions = evaluator.run(rules, uid, display_name)
-
-            # check whether unread counts are enabled for this user
-            unread_enabled = await self.store.get_feature_enabled(uid, "msc2654")
-            if not unread_enabled:
-                unread_enabled = self.hs.config.experimental.msc2654_enabled
+            if count_as_unread:
+                # Add an element for the current user if the event needs to be marked as
+                # unread, so that add_push_actions_to_staging iterates over it.
+                # If the event shouldn't be marked as unread but should notify the
+                # current user, it'll be added to the dict later.
+                actions_by_user[uid] = []
 
-            if unread_enabled:
-                count_as_unread = _should_count_as_unread(event, context)
-            else:
-                count_as_unread = False
-
-            if "notify" in actions or count_as_unread:
-                # Push rules say we should notify the user of this event or the event should
-                # increment the unread count
-                actions_by_user[uid] = ActionsForUser(actions, count_as_unread)
+            actions = evaluator.run(rules, uid, display_name)
+            if "notify" in actions:
+                # Push rules say we should notify the user of this event
+                actions_by_user[uid] = actions
 
         # If there aren't any actions then we can skip the rest of the
         # processing.
@@ -477,6 +467,7 @@ async def _action_for_event_by_user(
         await self.store.add_push_actions_to_staging(
             event.event_id,
             actions_by_user,
+            count_as_unread,
             thread_id,
         )
 

synapse/rest/client/sync.py

@@ -100,6 +100,7 @@ def __init__(self, hs: "HomeServer"):
         self.presence_handler = hs.get_presence_handler()
         self._server_notices_sender = hs.get_server_notices_sender()
         self._event_serializer = hs.get_event_client_serializer()
+        self._msc2654_enabled = hs.config.experimental.msc2654_enabled
         self._msc3773_enabled = hs.config.experimental.msc3773_enabled
 
     async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
@@ -260,7 +261,7 @@ async def encode_response(
         )
 
         joined = await self.encode_joined(
-            sync_result.joined, time_now, serialize_options, requester
+            sync_result.joined, time_now, serialize_options
         )
 
         invited = await self.encode_invited(
@@ -272,7 +273,7 @@ async def encode_response(
         )
 
         archived = await self.encode_archived(
-            sync_result.archived, time_now, serialize_options, requester
+            sync_result.archived, time_now, serialize_options
         )
 
         logger.debug("building sync response dict")
@@ -343,7 +344,6 @@ async def encode_joined(
         rooms: List[JoinedSyncResult],
         time_now: int,
         serialize_options: SerializeEventConfig,
-        requester: Requester,
     ) -> JsonDict:
         """
         Encode the joined rooms in a sync result
@@ -352,7 +352,6 @@ async def encode_joined(
             rooms: list of sync results for rooms this user is joined to
             time_now: current time - used as a baseline for age calculations
             serialize_options: Event serializer options
-            requester: The requester of the sync
         Returns:
             The joined rooms list, in our response format
         """
@@ -363,7 +362,6 @@ async def encode_joined(
                 time_now,
                 joined=True,
                 serialize_options=serialize_options,
-                requester=requester,
             )
 
         return joined
@@ -454,7 +452,6 @@ async def encode_archived(
         rooms: List[ArchivedSyncResult],
         time_now: int,
         serialize_options: SerializeEventConfig,
-        requester: Requester,
     ) -> JsonDict:
         """
         Encode the archived rooms in a sync result
@@ -463,7 +460,6 @@ async def encode_archived(
             rooms: list of sync results for rooms this user is joined to
             time_now: current time - used as a baseline for age calculations
             serialize_options: Event serializer options
-            requester: the requester of the sync
         Returns:
             The archived rooms list, in our response format
         """
@@ -474,7 +470,6 @@ async def encode_archived(
                 time_now,
                 joined=False,
                 serialize_options=serialize_options,
-                requester=requester,
             )
 
         return joined
@@ -485,7 +480,6 @@ async def encode_room(
         time_now: int,
         joined: bool,
         serialize_options: SerializeEventConfig,
-        requester: Requester,
     ) -> JsonDict:
         """
         Args:
@@ -498,7 +492,6 @@ async def encode_room(
             only_fields: Optional. The list of event fields to include.
             event_formatter: function to convert from federation format
                 to client format
-            Requester: The requester of the sync
         Returns:
             The room, encoded in our response format
         """
@@ -552,14 +545,7 @@ async def encode_room(
                         "org.matrix.msc3773.unread_thread_notifications"
                     ] = room.unread_thread_notifications
             result["summary"] = room.summary
-
-            msc2654_enabled = await self.hs.get_datastores().main.get_feature_enabled(
-                requester.user.to_string(), "msc2654"
-            )
-            if not msc2654_enabled:
-                msc2654_enabled = self.hs.config.experimental.msc2654_enabled
-
-            if msc2654_enabled:
+            if self._msc2654_enabled:
                 result["org.matrix.msc2654.unread_count"] = room.unread_count
 
         return result

synapse/storage/databases/main/event_push_actions.py

@@ -105,7 +105,6 @@
 from synapse.util.caches.descriptors import cached
 
 if TYPE_CHECKING:
-    from synapse.push.bulk_push_rule_evaluator import ActionsForUser
     from synapse.server import HomeServer
 
 logger = logging.getLogger(__name__)
@@ -1216,7 +1215,8 @@ def _get_if_maybe_push_in_range_for_user_txn(txn: LoggingTransaction) -> bool:
     async def add_push_actions_to_staging(
         self,
         event_id: str,
-        user_id_actions: Dict[str, "ActionsForUser"],
+        user_id_actions: Dict[str, Collection[Union[Mapping, str]]],
+        count_as_unread: bool,
         thread_id: str,
     ) -> None:
         """Add the push actions for the event to the push action staging area.
@@ -1234,19 +1234,17 @@ async def add_push_actions_to_staging(
         # This is a helper function for generating the necessary tuple that
         # can be used to insert into the `event_push_actions_staging` table.
         def _gen_entry(
-            user_id: str, actions_by_user: "ActionsForUser"
+            user_id: str, actions: Collection[Union[Mapping, str]]
         ) -> Tuple[str, str, str, int, int, int, str, int]:
-            is_highlight = 1 if _action_has_highlight(actions_by_user.actions) else 0
-            notif = 1 if "notify" in actions_by_user.actions else 0
+            is_highlight = 1 if _action_has_highlight(actions) else 0
+            notif = 1 if "notify" in actions else 0
             return (
                 event_id,  # event_id column
                 user_id,  # user_id column
-                _serialize_action(
-                    actions_by_user.actions, bool(is_highlight)
-                ),  # actions column
+                _serialize_action(actions, bool(is_highlight)),  # actions column
                 notif,  # notif column
                 is_highlight,  # highlight column
-                int(actions_by_user.count_as_unread),  # unread column
+                int(count_as_unread),  # unread column
                 thread_id,  # thread_id column
                 self._clock.time_msec(),  # inserted_ts column
             )
@@ -1264,8 +1262,8 @@ def _gen_entry(
                 "inserted_ts",
             ),
             values=[
-                _gen_entry(user_id, actions_by_user)
-                for user_id, actions_by_user in user_id_actions.items()
+                _gen_entry(user_id, actions)
+                for user_id, actions in user_id_actions.items()
             ],
             desc="add_push_actions_to_staging",
         )

tests/replication/slave/storage/test_events.py

@@ -24,7 +24,6 @@
 from synapse.events import EventBase, _EventInternalMetadata, make_event_from_dict
 from synapse.events.snapshot import EventContext
 from synapse.handlers.room import RoomEventSource
-from synapse.push.bulk_push_rule_evaluator import ActionsForUser
 from synapse.server import HomeServer
 from synapse.storage.databases.main.event_push_actions import (
     NotifCounts,
@@ -413,10 +412,8 @@ def build_event(
         self.get_success(
             self.master_store.add_push_actions_to_staging(
                 event.event_id,
-                {
-                    user_id: ActionsForUser(actions, False)
-                    for user_id, actions in push_actions
-                },
+                dict(push_actions),
+                False,
                 "main",
             )
         )

tests/rest/client/test_sync.py

@@ -28,7 +28,6 @@
     ReceiptTypes,
     RelationTypes,
 )
-from synapse.rest.admin.experimental_features import ExperimentalFeature
 from synapse.rest.client import devices, knock, login, read_marker, receipts, room, sync
 from synapse.server import HomeServer
 from synapse.types import JsonDict
@@ -539,12 +538,14 @@ class UnreadMessagesTestCase(unittest.HomeserverTestCase):
 
     def default_config(self) -> JsonDict:
         config = super().default_config()
+        config["experimental_features"] = {
+            "msc2654_enabled": True,
+        }
         return config
 
     def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
         self.url = "/sync?since=%s"
         self.next_batch = "s0"
-        self.hs = hs
 
         # Register the first user (used to check the unread counts).
         self.user_id = self.register_user("kermit", "monkey")
@@ -587,12 +588,6 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
 
     def test_unread_counts(self) -> None:
         """Tests that /sync returns the right value for the unread count (MSC2654)."""
-        # add per-user flag to the DB
-        self.get_success(
-            self.hs.get_datastores().main.set_features_for_user(
-                self.user_id, {ExperimentalFeature.MSC2654: True}
-            )
-        )
 
         # Check that our own messages don't increase the unread count.
         self.helper.send(self.room_id, "hello", tok=self.tok)