Fix bug where sliding sync ignored room_id_to_include option

changelog.d/18535.bugfix

@@ -0,0 +1 @@
+Fix long-standing bug where sliding sync did not honour the `room_id_to_include` config option.

synapse/handlers/sliding_sync/room_lists.py

@@ -23,6 +23,7 @@
     List,
     Literal,
     Mapping,
+    MutableMapping,
     Optional,
     Set,
     Tuple,
@@ -73,6 +74,7 @@
     SlidingSyncResult,
 )
 from synapse.types.state import StateFilter
+from synapse.util import MutableOverlayMapping
 
 if TYPE_CHECKING:
     from synapse.server import HomeServer
@@ -245,9 +247,11 @@ async def _compute_interested_rooms_new_tables(
         # Note: this won't include rooms the user has left themselves. We add back
         # `newly_left` rooms below. This is more efficient than fetching all rooms and
         # then filtering out the old left rooms.
-        room_membership_for_user_map = (
-            await self.store.get_sliding_sync_rooms_for_user_from_membership_snapshots(
-                user_id
+        room_membership_for_user_map: MutableMapping[str, RoomsForUserSlidingSync] = (
+            MutableOverlayMapping(
+                await self.store.get_sliding_sync_rooms_for_user_from_membership_snapshots(
+                    user_id
+                )
             )
         )
         # To play nice with the rewind logic below, we need to go fetch the rooms the
@@ -268,26 +272,12 @@ async def _compute_interested_rooms_new_tables(
             )
         )
         if self_leave_room_membership_for_user_map:
-            # FIXME: It would be nice to avoid this copy but since
-            # `get_sliding_sync_rooms_for_user_from_membership_snapshots` is cached, it
-            # can't return a mutable value like a `dict`. We make the copy to get a
-            # mutable dict that we can change. We try to only make a copy when necessary
-            # (if we actually need to change something) as in most cases, the logic
-            # doesn't need to run.
-            room_membership_for_user_map = dict(room_membership_for_user_map)
             room_membership_for_user_map.update(self_leave_room_membership_for_user_map)
 
         # Remove invites from ignored users
         ignored_users = await self.store.ignored_users(user_id)
         invite_config = await self.store.get_invite_config_for_user(user_id)
         if ignored_users:
-            # FIXME: It would be nice to avoid this copy but since
-            # `get_sliding_sync_rooms_for_user_from_membership_snapshots` is cached, it
-            # can't return a mutable value like a `dict`. We make the copy to get a
-            # mutable dict that we can change. We try to only make a copy when necessary
-            # (if we actually need to change something) as in most cases, the logic
-            # doesn't need to run.
-            room_membership_for_user_map = dict(room_membership_for_user_map)
             # Make a copy so we don't run into an error: `dictionary changed size during
             # iteration`, when we remove items
             for room_id in list(room_membership_for_user_map.keys()):
@@ -316,13 +306,6 @@ async def _compute_interested_rooms_new_tables(
             sync_config.user, room_membership_for_user_map, to_token=to_token
         )
         if changes:
-            # FIXME: It would be nice to avoid this copy but since
-            # `get_sliding_sync_rooms_for_user_from_membership_snapshots` is cached, it
-            # can't return a mutable value like a `dict`. We make the copy to get a
-            # mutable dict that we can change. We try to only make a copy when necessary
-            # (if we actually need to change something) as in most cases, the logic
-            # doesn't need to run.
-            room_membership_for_user_map = dict(room_membership_for_user_map)
             for room_id, change in changes.items():
                 if change is None:
                     # Remove rooms that the user joined after the `to_token`
@@ -364,13 +347,6 @@ async def _compute_interested_rooms_new_tables(
             newly_left_room_map.keys() - room_membership_for_user_map.keys()
         )
         if missing_newly_left_rooms:
-            # FIXME: It would be nice to avoid this copy but since
-            # `get_sliding_sync_rooms_for_user_from_membership_snapshots` is cached, it
-            # can't return a mutable value like a `dict`. We make the copy to get a
-            # mutable dict that we can change. We try to only make a copy when necessary
-            # (if we actually need to change something) as in most cases, the logic
-            # doesn't need to run.
-            room_membership_for_user_map = dict(room_membership_for_user_map)
             for room_id in missing_newly_left_rooms:
                 newly_left_room_for_user = newly_left_room_map[room_id]
                 # This should be a given
@@ -461,6 +437,10 @@ async def _compute_interested_rooms_new_tables(
                     else:
                         room_membership_for_user_map.pop(room_id, None)
 
+        # Remove any rooms that we globally exclude from sync.
+        for room_id in self.rooms_to_exclude_globally:
+            room_membership_for_user_map.pop(room_id, None)
+
         dm_room_ids = await self._get_dm_rooms_for_user(user_id)
 
         if sync_config.lists:
@@ -577,14 +557,6 @@ async def _compute_interested_rooms_new_tables(
 
         if sync_config.room_subscriptions:
             with start_active_span("assemble_room_subscriptions"):
-                # FIXME: It would be nice to avoid this copy but since
-                # `get_sliding_sync_rooms_for_user_from_membership_snapshots` is cached, it
-                # can't return a mutable value like a `dict`. We make the copy to get a
-                # mutable dict that we can change. We try to only make a copy when necessary
-                # (if we actually need to change something) as in most cases, the logic
-                # doesn't need to run.
-                room_membership_for_user_map = dict(room_membership_for_user_map)
-
                 # Find which rooms are partially stated and may need to be filtered out
                 # depending on the `required_state` requested (see below).
                 partial_state_rooms = await self.store.get_partial_rooms()

synapse/util/__init__.py

@@ -19,10 +19,22 @@
 #
 #
 
+import collections.abc
 import json
 import logging
 import typing
-from typing import Any, Callable, Dict, Generator, Optional, Sequence
+from typing import (
+    Any,
+    Callable,
+    Dict,
+    Generator,
+    Iterator,
+    Mapping,
+    Optional,
+    Sequence,
+    Set,
+    TypeVar,
+)
 
 import attr
 from immutabledict import immutabledict
@@ -251,3 +263,72 @@ def __init__(self, message: str, exceptions: Sequence[Exception]):
             parts.append(str(e))
         super().__init__("\n  - ".join(parts))
         self.exceptions = exceptions
+
+
+K = TypeVar("K")
+V = TypeVar("V")
+
+
+@attr.s(slots=True, auto_attribs=True)
+class MutableOverlayMapping(collections.abc.MutableMapping[K, V]):
+    """A mutable mapping that allows changes to a read-only underlying
+    mapping. Supports deletions.
+
+    This is useful for cases where you want to allow modifications to a mapping
+    without changing or copying the original mapping.
+
+    Note: the underlying mapping must not change while this proxy is in use.
+    """
+
+    _underlying_map: Mapping[K, V]
+    _mutable_map: Dict[K, V] = attr.ib(factory=dict)
+    _deletions: Set[K] = attr.ib(factory=set)
+
+    def __getitem__(self, key: K) -> V:
+        if key in self._deletions:
+            raise KeyError(key)
+        if key in self._mutable_map:
+            return self._mutable_map[key]
+        return self._underlying_map[key]
+
+    def __setitem__(self, key: K, value: V) -> None:
+        self._deletions.discard(key)
+        self._mutable_map[key] = value
+
+    def __delitem__(self, key: K) -> None:
+        if key not in self:
+            raise KeyError(key)
+
+        self._deletions.add(key)
+        self._mutable_map.pop(key, None)
+
+    def __iter__(self) -> Iterator[K]:
+        for key in self._mutable_map:
+            if key not in self._deletions:
+                yield key
+
+        for key in self._underlying_map:
+            if key not in self._deletions and key not in self._mutable_map:
+                # `key` should not be in both _mutable_map and _deletions
+                assert key not in self._mutable_map
+                yield key
+
+    def __len__(self) -> int:
+        count = len(self._underlying_map)
+        for key in self._deletions:
+            if key in self._underlying_map:
+                count -= 1
+
+        for key in self._mutable_map:
+            # `key` should not be in both _mutable_map and _deletions
+            assert key not in self._deletions
+
+            if key not in self._underlying_map:
+                count += 1
+
+        return count
+
+    def clear(self) -> None:
+        self._underlying_map = {}
+        self._mutable_map.clear()
+        self._deletions.clear()

tests/rest/client/sliding_sync/test_sliding_sync.py

@@ -1616,3 +1616,55 @@ def test_state_reset_never_room_incremental_sync_with_filters(
                 {space_room_id, space_room_id2},
                 exact=True,
             )
+
+    def test_exclude_rooms_from_sync(self) -> None:
+        """Tests that sliding sync honours the `exclude_rooms_from_sync` config
+        option.
+        """
+        user1_id = self.register_user("user1", "pass")
+        user1_tok = self.login(user1_id, "pass")
+
+        room_id_to_exclude = self.helper.create_room_as(
+            user1_id,
+            tok=user1_tok,
+        )
+        room_id_to_include = self.helper.create_room_as(
+            user1_id,
+            tok=user1_tok,
+        )
+
+        # We cheekily modify the stored config here, as we can't add it to the
+        # raw config since we don't know the room ID before we start up.
+        self.hs.get_sliding_sync_handler().rooms_to_exclude_globally.append(
+            room_id_to_exclude
+        )
+        self.hs.get_sliding_sync_handler().room_lists.rooms_to_exclude_globally.append(
+            room_id_to_exclude
+        )
+
+        # Make the Sliding Sync request
+        sync_body = {
+            "lists": {
+                "foo-list": {
+                    "ranges": [[0, 99]],
+                    "required_state": [],
+                    "timeline_limit": 0,
+                },
+            }
+        }
+        response_body, _ = self.do_sync(sync_body, tok=user1_tok)
+
+        # Make sure response only contains room_id_to_include
+        self.assertIncludes(
+            set(response_body["rooms"].keys()),
+            {room_id_to_include},
+            exact=True,
+        )
+
+        # Test that the excluded room is not in the list ops
+        # Make sure the list is sorted in the way we expect
+        self.assertIncludes(
+            set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]),
+            {room_id_to_include},
+            exact=True,
+        )