From 9c09765356cc384b406a16834f8837c614ed0f1f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 10 Jun 2025 10:16:28 +0100 Subject: [PATCH 01/10] Fix bug where sliding sync ignored `room_id_to_include` option This was correctly handled for the "fallback" case where the background updates hadn't finished. --- synapse/handlers/sliding_sync/room_lists.py | 4 ++ .../client/sliding_sync/test_sliding_sync.py | 44 +++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index 13e69f18a0d..b6d1fba2d3d 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -636,6 +636,10 @@ async def _compute_interested_rooms_new_tables( relevant_room_map[room_id] = room_sync_config + # Remove any rooms that we globally exclude from sync. + for room_id in self.rooms_to_exclude_globally: + relevant_room_map.pop(room_id, None) + # Filtered subset of `relevant_room_map` for rooms that may have updates # (in the event stream) relevant_rooms_to_send_map = await self._filter_relevant_rooms_to_send( diff --git a/tests/rest/client/sliding_sync/test_sliding_sync.py b/tests/rest/client/sliding_sync/test_sliding_sync.py index dcec5b4cf05..2f696bb7a4d 100644 --- a/tests/rest/client/sliding_sync/test_sliding_sync.py +++ b/tests/rest/client/sliding_sync/test_sliding_sync.py @@ -1616,3 +1616,47 @@ 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"]), + {room_id_to_include}, + exact=True, + ) From 9537aa5cdb7fd0db96175debe904957a3ebdef15 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 10 Jun 2025 10:18:16 +0100 Subject: [PATCH 02/10] Newsfile --- changelog.d/18535.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/18535.bugfix 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. From 7c47bcca0abd44db0bab254ba0827af52e0f4916 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 11 Jun 2025 15:07:30 +0100 Subject: [PATCH 03/10] Update tests/rest/client/sliding_sync/test_sliding_sync.py Co-authored-by: Eric Eastwood --- tests/rest/client/sliding_sync/test_sliding_sync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/client/sliding_sync/test_sliding_sync.py b/tests/rest/client/sliding_sync/test_sliding_sync.py index 2f696bb7a4d..0dcb2e42b05 100644 --- a/tests/rest/client/sliding_sync/test_sliding_sync.py +++ b/tests/rest/client/sliding_sync/test_sliding_sync.py @@ -1656,7 +1656,7 @@ def test_exclude_rooms_from_sync(self) -> None: # Make sure response only contains room_id_to_include self.assertIncludes( - set(response_body["rooms"]), + set(response_body["rooms"].keys()), {room_id_to_include}, exact=True, ) From 3b6e575ae6bfed81cb8660270648a007336ab74b Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 11 Jun 2025 15:12:34 +0100 Subject: [PATCH 04/10] Remove room from list earlier So that we don't include them in the list ops. --- synapse/handlers/sliding_sync/room_lists.py | 8 ++++---- .../rest/client/sliding_sync/test_sliding_sync.py | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index b6d1fba2d3d..9f7e66a4768 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -461,6 +461,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: @@ -636,10 +640,6 @@ async def _compute_interested_rooms_new_tables( relevant_room_map[room_id] = room_sync_config - # Remove any rooms that we globally exclude from sync. - for room_id in self.rooms_to_exclude_globally: - relevant_room_map.pop(room_id, None) - # Filtered subset of `relevant_room_map` for rooms that may have updates # (in the event stream) relevant_rooms_to_send_map = await self._filter_relevant_rooms_to_send( diff --git a/tests/rest/client/sliding_sync/test_sliding_sync.py b/tests/rest/client/sliding_sync/test_sliding_sync.py index 0dcb2e42b05..78a63d26d8f 100644 --- a/tests/rest/client/sliding_sync/test_sliding_sync.py +++ b/tests/rest/client/sliding_sync/test_sliding_sync.py @@ -1660,3 +1660,17 @@ def test_exclude_rooms_from_sync(self) -> None: {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.assertListEqual( + list(response_body["lists"]["foo-list"]["ops"]), + [ + { + "op": "SYNC", + "range": [0, 99], + "room_ids": [room_id_to_include], + } + ], + response_body["lists"]["foo-list"], + ) From 690d81630d1bd1d7f3e2fb5571d72f1de5035beb Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 12 Jun 2025 11:06:43 +0100 Subject: [PATCH 05/10] Update tests/rest/client/sliding_sync/test_sliding_sync.py Co-authored-by: Eric Eastwood --- .../rest/client/sliding_sync/test_sliding_sync.py | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/tests/rest/client/sliding_sync/test_sliding_sync.py b/tests/rest/client/sliding_sync/test_sliding_sync.py index 78a63d26d8f..412dbcf77bd 100644 --- a/tests/rest/client/sliding_sync/test_sliding_sync.py +++ b/tests/rest/client/sliding_sync/test_sliding_sync.py @@ -1663,14 +1663,8 @@ def test_exclude_rooms_from_sync(self) -> None: # Test that the excluded room is not in the list ops # Make sure the list is sorted in the way we expect - self.assertListEqual( - list(response_body["lists"]["foo-list"]["ops"]), - [ - { - "op": "SYNC", - "range": [0, 99], - "room_ids": [room_id_to_include], - } - ], - response_body["lists"]["foo-list"], + self.assertIncludes( + set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]), + {room_id_to_include}, + exact=True, ) From 13ae286578317340347467a5dbeb21a5ae5fe990 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 12 Jun 2025 13:14:25 +0100 Subject: [PATCH 06/10] Add a ChainMutableMapping --- synapse/util/__init__.py | 72 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-) diff --git a/synapse/util/__init__.py b/synapse/util/__init__.py index e0d876e84bf..e63fb507e5e 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,61 @@ 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 ChainMutableMapping(collections.abc.MutableMapping[K, V]): + """A mutable mapping that allows changes to a read-only underlying + mapping. + + 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.field(factory=dict) + _deletions: Set[K] = attr.field(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: + if key in self._deletions: + raise KeyError(key) + self._mutable_map[key] = value + + def __delitem__(self, key: K) -> None: + 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: + yield key + + def __len__(self) -> int: + count = len(self._underlying_map) + count -= len(self._deletions) + for key in self._mutable_map: + if key not in self._deletions and key not in self._underlying_map: + count += 1 + return count + + def clear(self) -> None: + self._underlying_map = {} + self._mutable_map.clear() + self._deletions.clear() From 26c4ab7e2d529897fb4fecd236f856bd5138a712 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 12 Jun 2025 13:48:43 +0100 Subject: [PATCH 07/10] Add ChainMutableMapping --- synapse/util/__init__.py | 12 +- tests/util/test_chain_mutable_mapping.py | 190 +++++++++++++++++++++++ 2 files changed, 198 insertions(+), 4 deletions(-) create mode 100644 tests/util/test_chain_mutable_mapping.py diff --git a/synapse/util/__init__.py b/synapse/util/__init__.py index e63fb507e5e..79fab329e8c 100644 --- a/synapse/util/__init__.py +++ b/synapse/util/__init__.py @@ -272,7 +272,7 @@ def __init__(self, message: str, exceptions: Sequence[Exception]): @attr.s(slots=True, auto_attribs=True) class ChainMutableMapping(collections.abc.MutableMapping[K, V]): """A mutable mapping that allows changes to a read-only underlying - mapping. + mapping. Supports deletions. This is useful for cases where you want to allow modifications to a mapping without changing or copying the original mapping. @@ -292,11 +292,13 @@ def __getitem__(self, key: K) -> V: return self._underlying_map[key] def __setitem__(self, key: K, value: V) -> None: - if key in self._deletions: - raise KeyError(key) + 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) @@ -311,7 +313,9 @@ def __iter__(self) -> Iterator[K]: def __len__(self) -> int: count = len(self._underlying_map) - count -= len(self._deletions) + for key in self._deletions: + if key in self._underlying_map: + count -= 1 for key in self._mutable_map: if key not in self._deletions and key not in self._underlying_map: count += 1 diff --git a/tests/util/test_chain_mutable_mapping.py b/tests/util/test_chain_mutable_mapping.py new file mode 100644 index 00000000000..e6b95edf023 --- /dev/null +++ b/tests/util/test_chain_mutable_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 ChainMutableMapping + + +class TestChainMutableMapping(unittest.TestCase): + """Tests for the ChainMutableMapping class.""" + + def test_init(self) -> None: + """Test initialization with different input types.""" + # Test with empty dict + empty_dict: Dict[str, int] = {} + mapping = ChainMutableMapping(empty_dict) + self.assertEqual(len(mapping), 0) + + # Test with populated dict + populated_dict = {"a": 1, "b": 2, "c": 3} + mapping = ChainMutableMapping(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 = ChainMutableMapping(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 = ChainMutableMapping(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 = ChainMutableMapping(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 = ChainMutableMapping(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 = ChainMutableMapping(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 = ChainMutableMapping(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 = ChainMutableMapping(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 = ChainMutableMapping(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) From 04d27505fd4a5b6dd7754239d50b8ab93928363b Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 12 Jun 2025 13:54:07 +0100 Subject: [PATCH 08/10] Use new ChainMutableMapping --- synapse/handlers/sliding_sync/room_lists.py | 46 ++++----------------- synapse/util/__init__.py | 4 +- 2 files changed, 9 insertions(+), 41 deletions(-) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index 9f7e66a4768..fc1c9d6517c 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 ChainMutableMapping 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] = ( + ChainMutableMapping( + 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 @@ -581,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 79fab329e8c..3e96ac417c5 100644 --- a/synapse/util/__init__.py +++ b/synapse/util/__init__.py @@ -281,8 +281,8 @@ class ChainMutableMapping(collections.abc.MutableMapping[K, V]): """ _underlying_map: Mapping[K, V] - _mutable_map: Dict[K, V] = attr.field(factory=dict) - _deletions: Set[K] = attr.field(factory=set) + _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: From ebc19488c1f0d5f5524c90db6c8f4ee42c71ef8a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 13 Jun 2025 09:49:10 +0100 Subject: [PATCH 09/10] Assert key is not in mutable map and deletions --- synapse/util/__init__.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/synapse/util/__init__.py b/synapse/util/__init__.py index 3e96ac417c5..0632bf8da9e 100644 --- a/synapse/util/__init__.py +++ b/synapse/util/__init__.py @@ -309,6 +309,8 @@ def __iter__(self) -> Iterator[K]: 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: @@ -316,9 +318,14 @@ def __len__(self) -> int: for key in self._deletions: if key in self._underlying_map: count -= 1 + for key in self._mutable_map: - if key not in self._deletions and key not in self._underlying_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: From e9baf1fbb3c7b6a6ef54b3a803b016a6ac6590b3 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 13 Jun 2025 09:55:56 +0100 Subject: [PATCH 10/10] Rename class to MutableOverlayMapping --- synapse/handlers/sliding_sync/room_lists.py | 4 +-- synapse/util/__init__.py | 2 +- ...ing.py => test_mutable_overlay_mapping.py} | 26 +++++++++---------- 3 files changed, 16 insertions(+), 16 deletions(-) rename tests/util/{test_chain_mutable_mapping.py => test_mutable_overlay_mapping.py} (88%) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index fc1c9d6517c..e196199f8ad 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -74,7 +74,7 @@ SlidingSyncResult, ) from synapse.types.state import StateFilter -from synapse.util import ChainMutableMapping +from synapse.util import MutableOverlayMapping if TYPE_CHECKING: from synapse.server import HomeServer @@ -248,7 +248,7 @@ async def _compute_interested_rooms_new_tables( # `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: MutableMapping[str, RoomsForUserSlidingSync] = ( - ChainMutableMapping( + MutableOverlayMapping( await self.store.get_sliding_sync_rooms_for_user_from_membership_snapshots( user_id ) diff --git a/synapse/util/__init__.py b/synapse/util/__init__.py index 0632bf8da9e..17836d21b3a 100644 --- a/synapse/util/__init__.py +++ b/synapse/util/__init__.py @@ -270,7 +270,7 @@ def __init__(self, message: str, exceptions: Sequence[Exception]): @attr.s(slots=True, auto_attribs=True) -class ChainMutableMapping(collections.abc.MutableMapping[K, V]): +class MutableOverlayMapping(collections.abc.MutableMapping[K, V]): """A mutable mapping that allows changes to a read-only underlying mapping. Supports deletions. diff --git a/tests/util/test_chain_mutable_mapping.py b/tests/util/test_mutable_overlay_mapping.py similarity index 88% rename from tests/util/test_chain_mutable_mapping.py rename to tests/util/test_mutable_overlay_mapping.py index e6b95edf023..a7335fca73a 100644 --- a/tests/util/test_chain_mutable_mapping.py +++ b/tests/util/test_mutable_overlay_mapping.py @@ -15,29 +15,29 @@ import unittest from typing import Dict -from synapse.util import ChainMutableMapping +from synapse.util import MutableOverlayMapping -class TestChainMutableMapping(unittest.TestCase): - """Tests for the ChainMutableMapping class.""" +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 = ChainMutableMapping(empty_dict) + mapping = MutableOverlayMapping(empty_dict) self.assertEqual(len(mapping), 0) # Test with populated dict populated_dict = {"a": 1, "b": 2, "c": 3} - mapping = ChainMutableMapping(populated_dict) + 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 = ChainMutableMapping(underlying) + mapping = MutableOverlayMapping(underlying) # Get from underlying map self.assertEqual(mapping["a"], 1) @@ -50,7 +50,7 @@ def test_get_item(self) -> None: def test_set_item(self) -> None: """Test setting items in the mapping.""" underlying = {"a": 1, "b": 2, "c": 3} - mapping = ChainMutableMapping(underlying) + mapping = MutableOverlayMapping(underlying) # Set new key mapping["d"] = 4 @@ -67,7 +67,7 @@ def test_set_item(self) -> None: def test_del_item(self) -> None: """Test deleting items from the mapping.""" underlying = {"a": 1, "b": 2, "c": 3} - mapping = ChainMutableMapping(underlying) + mapping = MutableOverlayMapping(underlying) # Delete a key del mapping["a"] @@ -84,7 +84,7 @@ def test_del_item(self) -> None: def test_len(self) -> None: """Test the len() function.""" underlying = {"a": 1, "b": 2, "c": 3} - mapping = ChainMutableMapping(underlying) + mapping = MutableOverlayMapping(underlying) self.assertEqual(len(mapping), 3) @@ -107,7 +107,7 @@ def test_len(self) -> None: def test_iteration(self) -> None: """Test iteration over the mapping.""" underlying = {"a": 1, "b": 2, "c": 3} - mapping = ChainMutableMapping(underlying) + mapping = MutableOverlayMapping(underlying) # Add a new key and override an existing one mapping["d"] = 4 @@ -129,7 +129,7 @@ def test_iteration(self) -> None: def test_clear(self) -> None: """Test the clear method.""" underlying = {"a": 1, "b": 2, "c": 3} - mapping = ChainMutableMapping(underlying) + mapping = MutableOverlayMapping(underlying) # Add a new key and override an existing one mapping["d"] = 4 @@ -157,7 +157,7 @@ def test_clear(self) -> None: def test_dict_methods(self) -> None: """Test standard dict methods.""" underlying = {"a": 1, "b": 2, "c": 3} - mapping = ChainMutableMapping(underlying) + mapping = MutableOverlayMapping(underlying) # Test keys, values, and items self.assertEqual(set(mapping.keys()), {"a", "b", "c"}) @@ -176,7 +176,7 @@ def test_dict_methods(self) -> None: def test_key_presence(self) -> None: """Test checking if keys exist in the mapping.""" underlying = {"a": 1, "b": 2, "c": 3} - mapping = ChainMutableMapping(underlying) + mapping = MutableOverlayMapping(underlying) mapping["d"] = 4 mapping["a"] = 10