diff --git a/changelog.d/18535.bugfix b/changelog.d/18535.bugfix new file mode 100644 index 00000000000..246b1bc01f6 --- /dev/null +++ b/changelog.d/18535.bugfix @@ -0,0 +1 @@ +Fix long-standing bug where sliding sync did not honour the `room_id_to_include` config option. diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index 13e69f18a0d..e196199f8ad 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/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() diff --git a/synapse/util/__init__.py b/synapse/util/__init__.py index e0d876e84bf..17836d21b3a 100644 --- a/synapse/util/__init__.py +++ b/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() diff --git a/tests/rest/client/sliding_sync/test_sliding_sync.py b/tests/rest/client/sliding_sync/test_sliding_sync.py index dcec5b4cf05..412dbcf77bd 100644 --- a/tests/rest/client/sliding_sync/test_sliding_sync.py +++ b/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, + ) diff --git a/tests/util/test_mutable_overlay_mapping.py b/tests/util/test_mutable_overlay_mapping.py new file mode 100644 index 00000000000..a7335fca73a --- /dev/null +++ b/tests/util/test_mutable_overlay_mapping.py @@ -0,0 +1,190 @@ +# +# This file is licensed under the Affero General Public License (AGPL) version 3. +# +# Copyright (C) 2025 New Vector, Ltd +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as +# published by the Free Software Foundation, either version 3 of the +# License, or (at your option) any later version. +# +# See the GNU Affero General Public License for more details: +# . +# + +import unittest +from typing import Dict + +from synapse.util import MutableOverlayMapping + + +class TestMutableOverlayMapping(unittest.TestCase): + """Tests for the MutableOverlayMapping class.""" + + def test_init(self) -> None: + """Test initialization with different input types.""" + # Test with empty dict + empty_dict: Dict[str, int] = {} + mapping = MutableOverlayMapping(empty_dict) + self.assertEqual(len(mapping), 0) + + # Test with populated dict + populated_dict = {"a": 1, "b": 2, "c": 3} + mapping = MutableOverlayMapping(populated_dict) + self.assertEqual(len(mapping), 3) + self.assertEqual(mapping["a"], 1) + + def test_get_item(self) -> None: + """Test getting items from the mapping.""" + underlying = {"a": 1, "b": 2, "c": 3} + mapping = MutableOverlayMapping(underlying) + + # Get from underlying map + self.assertEqual(mapping["a"], 1) + self.assertEqual(mapping["b"], 2) + + # Check KeyError for non-existent key + with self.assertRaises(KeyError): + mapping["d"] + + def test_set_item(self) -> None: + """Test setting items in the mapping.""" + underlying = {"a": 1, "b": 2, "c": 3} + mapping = MutableOverlayMapping(underlying) + + # Set new key + mapping["d"] = 4 + self.assertEqual(mapping["d"], 4) + + # Override existing key + mapping["a"] = 10 + self.assertEqual(mapping["a"], 10) + + # Original map should be unchanged + self.assertEqual(underlying["a"], 1) + self.assertNotIn("d", underlying) + + def test_del_item(self) -> None: + """Test deleting items from the mapping.""" + underlying = {"a": 1, "b": 2, "c": 3} + mapping = MutableOverlayMapping(underlying) + + # Delete a key + del mapping["a"] + with self.assertRaises(KeyError): + mapping["a"] + + # Original map should be unchanged + self.assertEqual(underlying["a"], 1) + + # Delete non-existent key + with self.assertRaises(KeyError): + del mapping["d"] + + def test_len(self) -> None: + """Test the len() function.""" + underlying = {"a": 1, "b": 2, "c": 3} + mapping = MutableOverlayMapping(underlying) + + self.assertEqual(len(mapping), 3) + + # Add a new key + mapping["d"] = 4 + self.assertEqual(len(mapping), 4) + + # Override an existing key + mapping["a"] = 10 + self.assertEqual(len(mapping), 4) + + # Delete a key + del mapping["b"] + self.assertEqual(len(mapping), 3) + + # Delete a key in mutable map + del mapping["d"] + self.assertEqual(len(mapping), 2) + + def test_iteration(self) -> None: + """Test iteration over the mapping.""" + underlying = {"a": 1, "b": 2, "c": 3} + mapping = MutableOverlayMapping(underlying) + + # Add a new key and override an existing one + mapping["d"] = 4 + mapping["a"] = 10 + + # Delete a key + del mapping["c"] + + iterated_keys = set() + for k in mapping: + iterated_keys.add(k) + + # Expected keys: a, b, d (c is deleted) + self.assertEqual(iterated_keys, {"a", "b", "d"}) + + iterated_items = dict(mapping.items()) + self.assertDictEqual(iterated_items, {"a": 10, "b": 2, "d": 4}) + + def test_clear(self) -> None: + """Test the clear method.""" + underlying = {"a": 1, "b": 2, "c": 3} + mapping = MutableOverlayMapping(underlying) + + # Add a new key and override an existing one + mapping["d"] = 4 + mapping["a"] = 10 + + # Clear the mapping + mapping.clear() + self.assertEqual(len(mapping), 0) + + # All keys should be gone + with self.assertRaises(KeyError): + mapping["a"] + + with self.assertRaises(KeyError): + mapping["d"] + + # Adding a new key after clearing + mapping["b"] = 2 + self.assertEqual(mapping["b"], 2) + self.assertEqual(len(mapping), 1) + + # The underlying map should remain unchanged + self.assertDictEqual(underlying, {"a": 1, "b": 2, "c": 3}) + + def test_dict_methods(self) -> None: + """Test standard dict methods.""" + underlying = {"a": 1, "b": 2, "c": 3} + mapping = MutableOverlayMapping(underlying) + + # Test keys, values, and items + self.assertEqual(set(mapping.keys()), {"a", "b", "c"}) + self.assertEqual(set(mapping.values()), {1, 2, 3}) + self.assertEqual(set(mapping.items()), {("a", 1), ("b", 2), ("c", 3)}) + + # Modify, then test again + mapping["d"] = 4 + mapping["a"] = 10 + del mapping["c"] + + self.assertEqual(set(mapping.keys()), {"a", "b", "d"}) + self.assertEqual(set(mapping.values()), {10, 2, 4}) + self.assertEqual(set(mapping.items()), {("a", 10), ("b", 2), ("d", 4)}) + + def test_key_presence(self) -> None: + """Test checking if keys exist in the mapping.""" + underlying = {"a": 1, "b": 2, "c": 3} + mapping = MutableOverlayMapping(underlying) + + mapping["d"] = 4 + mapping["a"] = 10 + del mapping["c"] + + # Test key presence + self.assertIn("a", mapping) + self.assertIn("b", mapping) + self.assertNotIn("c", mapping) + self.assertIn("d", mapping) + self.assertNotIn("e", mapping)