From af9c72db9df513f3eb0472fe4e1dd5c06eb24a7f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 1 Oct 2024 16:53:18 +0100 Subject: [PATCH 01/14] Correctly changes to required state config Fixes #17698 --- synapse/handlers/sliding_sync/__init__.py | 207 +++++++++++++- .../storage/databases/main/sliding_sync.py | 4 +- synapse/types/state.py | 5 + .../sliding_sync/test_rooms_required_state.py | 265 ++++++++++++++++++ 4 files changed, 475 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 9fcc68ff25..4915682f6e 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -14,7 +14,7 @@ import logging from itertools import chain -from typing import TYPE_CHECKING, Dict, List, Mapping, Optional, Set, Tuple +from typing import TYPE_CHECKING, AbstractSet, Dict, List, Mapping, Optional, Set, Tuple from prometheus_client import Histogram from typing_extensions import assert_never @@ -988,6 +988,10 @@ async def get_room_sync_data( include_others=required_state_filter.include_others, ) + # The required state map to store in the room sync config, if it has + # changed. + changed_required_state_map: Optional[Mapping[str, AbstractSet[str]]] = None + # We can return all of the state that was requested if this was the first # time we've sent the room down this connection. room_state: StateMap[EventBase] = {} @@ -1001,6 +1005,29 @@ async def get_room_sync_data( else: assert from_bound is not None + if prev_room_sync_config is not None: + # Check if there are any changes to the required state config + # that we need to handle. + changed_required_state_map, added_state_filter = ( + _required_state_changes( + user.to_string(), + prev_room_sync_config, + room_sync_config, + room_state_delta_id_map, + ) + ) + + if added_state_filter: + # Some state entries got added, so we pull out the current + # state for them. If we don't do this we'd only send down new deltas. + state_ids = await self.get_current_state_ids_at( + room_id=room_id, + room_membership_for_user_at_to_token=room_membership_for_user_at_to_token, + state_filter=added_state_filter, + to_token=to_token, + ) + room_state_delta_id_map.update(state_ids) + events = await self.store.get_events( state_filter.filter_state(room_state_delta_id_map).values() ) @@ -1083,6 +1110,10 @@ async def get_room_sync_data( prev_room_sync_config = previous_connection_state.room_configs.get(room_id) # Record the `room_sync_config` if we're `ignore_timeline_bound` (which means # that the `timeline_limit` has increased) + room_sync_required_state_map_to_persist = room_sync_config.required_state_map + if changed_required_state_map: + room_sync_required_state_map_to_persist = changed_required_state_map + if ignore_timeline_bound: # FIXME: We signal the fact that we're sending down more events to # the client by setting `unstable_expanded_timeline` to true (see @@ -1091,7 +1122,7 @@ async def get_room_sync_data( new_connection_state.room_configs[room_id] = RoomSyncConfig( timeline_limit=room_sync_config.timeline_limit, - required_state_map=room_sync_config.required_state_map, + required_state_map=room_sync_required_state_map_to_persist, ) elif prev_room_sync_config is not None: # If the result is `limited` then we need to record that the @@ -1120,10 +1151,14 @@ async def get_room_sync_data( ): new_connection_state.room_configs[room_id] = RoomSyncConfig( timeline_limit=room_sync_config.timeline_limit, - required_state_map=room_sync_config.required_state_map, + required_state_map=room_sync_required_state_map_to_persist, ) - # TODO: Record changes in required_state. + elif changed_required_state_map is not None: + new_connection_state.room_configs[room_id] = RoomSyncConfig( + timeline_limit=room_sync_config.timeline_limit, + required_state_map=room_sync_required_state_map_to_persist, + ) else: new_connection_state.room_configs[room_id] = room_sync_config @@ -1242,3 +1277,167 @@ async def _get_bump_stamp( return new_bump_event_pos.stream return None + + +def _required_state_changes( + user_id: str, + previous_room_config: "RoomSyncConfig", + room_sync_config: RoomSyncConfig, + state_deltas: StateMap[str], +) -> Tuple[Optional[Mapping[str, AbstractSet[str]]], StateFilter]: + """Calculates the changes between the required state room config from the + previous requests compared with the current request. + + This does two things. First, it calculates if we need to update the room + config due to changes to required state. Secondly, it works out which state + entries we need to pull from current state and return due to the state entry + now appearing in the required state when it previously wasn't (on top of the + state deltas). + + This function tries to ensure to handle the case where a state entry is + added, removed and then added again to the required state. In that case we + only want to re-send that entry down sync if it has changed. + + Returns: + A 2-tuple of updated required state config and the state filter to use + to fetch extra current state that we need to return. + """ + + prev_required_state_map = previous_room_config.required_state_map + request_required_state_map = room_sync_config.required_state_map + + if prev_required_state_map == request_required_state_map: + # There has been no change. Return immediately. + return None, StateFilter.none() + + prev_wildcard = prev_required_state_map.get(StateValues.WILDCARD, set()) + request_wildcard = request_required_state_map.get(StateValues.WILDCARD, set()) + + # If a event type wildcard has been added or removed we don't try and do + # anything fancy, and instead always update the effective room required + # state config to match the request. + if request_wildcard - prev_wildcard: + # Some keys were added, so we need to fetch everything + return request_required_state_map, StateFilter.all() + if prev_wildcard - request_wildcard: + # Keys were only removed, so we don't have to fetch everything. + return request_required_state_map, StateFilter.none() + + # Contains updates to the required state map compared with the previous room + # config. This has the same format as `RoomSyncConfig.required_state` + changes: Dict[str, AbstractSet[str]] = {} + + # The set of types/state keys that we need to fetch and return to the + # client. Passed to `StateFilter.from_types(...)` + added: List[Tuple[str, Optional[str]]] = [] + + # First we calculate what, if anything, has been *added*. + for event_type in ( + prev_required_state_map.keys() | request_required_state_map.keys() + ): + old_state_keys = prev_required_state_map.get(event_type, set()) + request_state_keys = request_required_state_map.get(event_type, set()) + + if old_state_keys == request_state_keys: + # No change to this type + continue + + if not request_state_keys - old_state_keys: + # Nothing *added*, so we skip. Removals happen below. + continue + + # Always update changes to include the newly added keys + changes[event_type] = request_state_keys + + # Record the new state keys to fetch for this type. + if StateValues.WILDCARD in request_state_keys: + # If we have added a wildcard then we always just fetch everything. + added.append((event_type, None)) + else: + for state_key in request_state_keys - old_state_keys: + if state_key == StateValues.ME: + added.append((event_type, user_id)) + elif state_key == StateValues.LAZY: + # We handle lazy loading separately, so don't need to + # explicitly add anything here. + pass + else: + added.append((event_type, state_key)) + + added_state_filter = StateFilter.from_types(added) + + # Convert the list of state deltas to map from type to state_keys that have + # changed. + changed_types_to_state_keys: Dict[str, Set[str]] = {} + for event_type, state_key in state_deltas: + changed_types_to_state_keys.setdefault(event_type, set()).add(state_key) + + # Figure out what changes we need to apply to the effective required state + # config. + for event_type, changed_state_keys in changed_types_to_state_keys.items(): + old_state_keys = prev_required_state_map.get(event_type, set()) + request_state_keys = request_required_state_map.get(event_type, set()) + + if old_state_keys == request_state_keys: + # No change. + continue + + if request_state_keys - old_state_keys: + # We've expanded the set of state keys, so we just clobber the + # current set with the new set. + # + # We could also ensure that we keep entries where the state hasn't + # changed, but are no longer in the requested required state, but + # that's a sufficient edge case that we can ignore (as its only a + # performance optimization). + changes[event_type] = request_state_keys + continue + + old_state_key_wildcard = StateValues.WILDCARD in old_state_keys + request_state_key_wildcard = StateValues.WILDCARD in request_state_keys + + if old_state_key_wildcard != request_state_key_wildcard: + # If a wildcard has been added or removed we always update the + # required state when any state with the same type has changed. + changes[event_type] = request_state_keys + continue + + old_state_key_lazy = StateValues.LAZY in old_state_keys + request_state_key_lazy = StateValues.LAZY in request_state_keys + + if old_state_key_lazy != request_state_key_lazy: + # If a "$LAZY" has been added or removed we always update the + # required state for simplicity. + changes[event_type] = request_state_keys + continue + + # Handle "$ME" values by replacing state keys that match the user ID. + if user_id in changed_state_keys: + changed_state_keys.discard(user_id) + changed_state_keys.add(StateValues.ME) + + # At this point there are no wildcards and no additions to the set of + # state keys requested, only deletions. + # + # We only remove state keys from the effective state if they've been + # removed from the request *and* the state has changed. This ensures + # that if a client removes and then readds a state key, we only send + # down the associated current state event if its changed (rather than + # sending down the same event twice). + invalidated = (old_state_keys - request_state_keys) & changed_state_keys + if invalidated: + changes[event_type] = old_state_keys - invalidated + + if changes: + # Update the required state config based on the changes. + new_required_state_map = dict(prev_required_state_map) + for event_type, state_keys in changes.items(): + if state_keys: + new_required_state_map[event_type] = state_keys + else: + # Remove entries with empty state keys. + new_required_state_map.pop(event_type, None) + + return new_required_state_map, added_state_filter + else: + return None, added_state_filter diff --git a/synapse/storage/databases/main/sliding_sync.py b/synapse/storage/databases/main/sliding_sync.py index f2df37fec1..7b357c1ffe 100644 --- a/synapse/storage/databases/main/sliding_sync.py +++ b/synapse/storage/databases/main/sliding_sync.py @@ -386,8 +386,8 @@ def _get_and_clear_connection_positions_txn( required_state_map: Dict[int, Dict[str, Set[str]]] = {} for row in rows: state = required_state_map[row[0]] = {} - for event_type, state_keys in db_to_json(row[1]): - state[event_type] = set(state_keys) + for event_type, state_key in db_to_json(row[1]): + state.setdefault(event_type, set()).add(state_key) # Get all the room configs, looking up the required state from the map # above. diff --git a/synapse/types/state.py b/synapse/types/state.py index 1141c4b5c1..060a5b6183 100644 --- a/synapse/types/state.py +++ b/synapse/types/state.py @@ -616,6 +616,11 @@ def __contains__(self, key: Any) -> bool: return False + def __bool__(self) -> bool: + if self.include_others: + return True + return bool(self.types) + _ALL_STATE_FILTER = StateFilter(types=immutabledict(), include_others=True) _ALL_NON_MEMBER_STATE_FILTER = StateFilter( diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index 91ac6c5a0e..0d98a6d889 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -862,3 +862,268 @@ def test_rooms_required_state_partial_state(self) -> None: exact=True, message=f"Expected only fully-stated rooms to show up for test_key={list_key}.", ) + + def test_rooms_required_state_expand(self) -> None: + """Test that when we expand the required state argument we get the + expanded state, and not just the changes to the new expanded.""" + + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + + # Create a room with a room name. + room_id1 = self.helper.create_room_as( + user1_id, tok=user1_tok, extra_content={"name": "Foo"} + ) + + # Only request the state event to begin with + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Create, ""], + ], + "timeline_limit": 1, + } + } + } + response_body, from_token = self.do_sync(sync_body, tok=user1_tok) + + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id1) + ) + + self._assertRequiredStateIncludes( + response_body["rooms"][room_id1]["required_state"], + { + state_map[(EventTypes.Create, "")], + }, + exact=True, + ) + + # Send a message so the room comes down sync. + self.helper.send(room_id1, "msg", tok=user1_tok) + + # Update the sliding sync requests to include the room name + sync_body["lists"]["foo-list"]["required_state"] = [ + [EventTypes.Create, ""], + [EventTypes.Name, ""], + ] + response_body, from_token = self.do_sync( + sync_body, since=from_token, tok=user1_tok + ) + + # We should see the room name, even though there haven't been any + # changes. + self._assertRequiredStateIncludes( + response_body["rooms"][room_id1]["required_state"], + { + state_map[(EventTypes.Name, "")], + }, + exact=True, + ) + + # Send a message so the room comes down sync. + self.helper.send(room_id1, "msg", tok=user1_tok) + + # We should not see any state changes. + response_body, from_token = self.do_sync( + sync_body, since=from_token, tok=user1_tok + ) + self.assertIsNone(response_body["rooms"][room_id1].get("required_state")) + + def test_rooms_required_state_expand_retract_expand(self) -> None: + """Test that when expanding, retracting and then expanding the required + state, we get the changes that happened.""" + + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + + # Create a room with a room name. + room_id1 = self.helper.create_room_as( + user1_id, tok=user1_tok, extra_content={"name": "Foo"} + ) + + # Only request the state event to begin with + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Create, ""], + ], + "timeline_limit": 1, + } + } + } + response_body, from_token = self.do_sync(sync_body, tok=user1_tok) + + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id1) + ) + + self._assertRequiredStateIncludes( + response_body["rooms"][room_id1]["required_state"], + { + state_map[(EventTypes.Create, "")], + }, + exact=True, + ) + + # Send a message so the room comes down sync. + self.helper.send(room_id1, "msg", tok=user1_tok) + + # Update the sliding sync requests to include the room name + sync_body["lists"]["foo-list"]["required_state"] = [ + [EventTypes.Create, ""], + [EventTypes.Name, ""], + ] + response_body, from_token = self.do_sync( + sync_body, since=from_token, tok=user1_tok + ) + + # We should see the room name, even though there haven't been any + # changes. + self._assertRequiredStateIncludes( + response_body["rooms"][room_id1]["required_state"], + { + state_map[(EventTypes.Name, "")], + }, + exact=True, + ) + + # Update the room name + self.helper.send_state( + room_id1, "m.room.name", {"name": "Bar"}, state_key="", tok=user1_tok + ) + + # Update the sliding sync requests to exclude the room name again + sync_body["lists"]["foo-list"]["required_state"] = [ + [EventTypes.Create, ""], + ] + response_body, from_token = self.do_sync( + sync_body, since=from_token, tok=user1_tok + ) + + # We should not see the updated room name in state (though it will be in + # the timeline). + self.assertIsNone(response_body["rooms"][room_id1].get("required_state")) + + # Send a message so the room comes down sync. + self.helper.send(room_id1, "msg", tok=user1_tok) + + # Update the sliding sync requests to include the room name again + sync_body["lists"]["foo-list"]["required_state"] = [ + [EventTypes.Create, ""], + [EventTypes.Name, ""], + ] + response_body, from_token = self.do_sync( + sync_body, since=from_token, tok=user1_tok + ) + + # We should see the *new* room name, even though there haven't been any + # changes. + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id1) + ) + self._assertRequiredStateIncludes( + response_body["rooms"][room_id1]["required_state"], + { + state_map[(EventTypes.Name, "")], + }, + exact=True, + ) + + def test_rooms_required_state_expand_deduplicate(self) -> None: + """Test that when expanding, retracting and then expanding the required + state, we don't get the state down again if it hasn't changed""" + + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + + # Create a room with a room name. + room_id1 = self.helper.create_room_as( + user1_id, tok=user1_tok, extra_content={"name": "Foo"} + ) + + # Only request the state event to begin with + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Create, ""], + ], + "timeline_limit": 1, + } + } + } + response_body, from_token = self.do_sync(sync_body, tok=user1_tok) + + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id1) + ) + + self._assertRequiredStateIncludes( + response_body["rooms"][room_id1]["required_state"], + { + state_map[(EventTypes.Create, "")], + }, + exact=True, + ) + + # Send a message so the room comes down sync. + self.helper.send(room_id1, "msg", tok=user1_tok) + + # Update the sliding sync requests to include the room name + sync_body["lists"]["foo-list"]["required_state"] = [ + [EventTypes.Create, ""], + [EventTypes.Name, ""], + ] + response_body, from_token = self.do_sync( + sync_body, since=from_token, tok=user1_tok + ) + + # We should see the room name, even though there haven't been any + # changes. + self._assertRequiredStateIncludes( + response_body["rooms"][room_id1]["required_state"], + { + state_map[(EventTypes.Name, "")], + }, + exact=True, + ) + + # Send a message so the room comes down sync. + self.helper.send(room_id1, "msg", tok=user1_tok) + + # Update the sliding sync requests to exclude the room name again + sync_body["lists"]["foo-list"]["required_state"] = [ + [EventTypes.Create, ""], + ] + response_body, from_token = self.do_sync( + sync_body, since=from_token, tok=user1_tok + ) + + # We should not see any state updates + self.assertIsNone(response_body["rooms"][room_id1].get("required_state")) + + # Send a message so the room comes down sync. + self.helper.send(room_id1, "msg", tok=user1_tok) + + # Update the sliding sync requests to include the room name again + sync_body["lists"]["foo-list"]["required_state"] = [ + [EventTypes.Create, ""], + [EventTypes.Name, ""], + ] + response_body, from_token = self.do_sync( + sync_body, since=from_token, tok=user1_tok + ) + + # We should not see the room name again, as we have already sent that + # down. + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id1) + ) + # Send a message so the room comes down sync. + self.assertIsNone(response_body["rooms"][room_id1].get("required_state")) From 2fd3a5abaec2932d2ae0964d2cf8ff0a060900e7 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 3 Oct 2024 16:10:41 +0100 Subject: [PATCH 02/14] Add tests --- tests/handlers/test_sliding_sync.py | 404 +++++++++++++++++++++++++++- 1 file changed, 403 insertions(+), 1 deletion(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index e2c7a94ce2..eaee50e4f8 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -18,7 +18,7 @@ # # import logging -from typing import AbstractSet, Dict, Optional, Tuple +from typing import AbstractSet, Dict, Optional, Set, Tuple from unittest.mock import patch from parameterized import parameterized @@ -35,6 +35,7 @@ RoomsForUserType, RoomSyncConfig, StateValues, + _required_state_changes, ) from synapse.rest import admin from synapse.rest.client import knock, login, room @@ -42,8 +43,10 @@ from synapse.storage.util.id_generators import MultiWriterIdGenerator from synapse.types import JsonDict, StreamToken, UserID from synapse.types.handlers.sliding_sync import SlidingSyncConfig +from synapse.types.state import StateFilter from synapse.util import Clock +from tests import unittest from tests.replication._base import BaseMultiWorkerStreamTestCase from tests.unittest import HomeserverTestCase, TestCase @@ -3213,3 +3216,402 @@ def test_default_bump_event_types(self) -> None: # We only care about the *latest* event in the room. [room_id1, room_id2], ) + + +class RequiredStateChangesTestCase(unittest.TestCase): + """Test cases for `_required_state_changes`""" + + def test_simple_no_change(self) -> None: + """Test no change to required state""" + previous_required_state_map = {"type1": {"state_key"}} + request_required_state_map = previous_required_state_map + + to_persist, added = _required_state_changes( + user_id="@user:test", + previous_room_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=previous_required_state_map, + ), + room_sync_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=request_required_state_map, + ), + state_deltas={}, + ) + + # No changes + self.assertIsNone(to_persist) + self.assertEqual(added, StateFilter.none()) + + def test_simple_add_type(self) -> None: + """Test adding a type to the config""" + previous_required_state_map = {"type1": {"state_key"}} + request_required_state_map = {"type1": {"state_key"}, "type2": {"state_key"}} + + to_persist, added = _required_state_changes( + user_id="@user:test", + previous_room_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=previous_required_state_map, + ), + room_sync_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=request_required_state_map, + ), + state_deltas={}, + ) + + # We've added a type so we should persist the changed required state + # config. + assert to_persist is not None + self.assertDictEqual(to_persist, request_required_state_map) + + # We should see the new type added + self.assertEqual(added, StateFilter.from_types([("type2", "state_key")])) + + def test_simple_add_state_key(self) -> None: + """Test adding a state key to the config""" + previous_required_state_map = {"type": {"state_key1"}} + request_required_state_map = {"type": {"state_key1", "state_key2"}} + + to_persist, added = _required_state_changes( + user_id="@user:test", + previous_room_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=previous_required_state_map, + ), + room_sync_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=request_required_state_map, + ), + state_deltas={}, + ) + + # We've added a type so we should persist the changed required state + # config. + assert to_persist is not None + self.assertDictEqual(to_persist, request_required_state_map) + self.assertEqual(added, StateFilter.from_types([("type", "state_key2")])) + + def test_simple_add_state_key_with_change(self) -> None: + """Test adding a state key to the config, and see some changed state""" + previous_required_state_map = {"type": {"state_key1"}} + request_required_state_map = {"type": {"state_key1", "state_key2"}} + + to_persist, added = _required_state_changes( + user_id="@user:test", + previous_room_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=previous_required_state_map, + ), + room_sync_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=request_required_state_map, + ), + state_deltas={("type", "state_key"): "$event_id"}, + ) + + # We've added a type so we should persist the changed required state + # config. + assert to_persist is not None + self.assertDictEqual(to_persist, request_required_state_map) + self.assertEqual(added, StateFilter.from_types([("type", "state_key2")])) + + def test_simple_remove_type_no_change(self) -> None: + """Test removing a type from the config when there are no matching state + deltas not cause the persisted required state config to change""" + + previous_required_state_map = {"type1": {"state_key"}, "type2": {"state_key"}} + request_required_state_map = {"type1": {"state_key"}} + + to_persist, added = _required_state_changes( + user_id="@user:test", + previous_room_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=previous_required_state_map, + ), + room_sync_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=request_required_state_map, + ), + state_deltas={}, + ) + + # We've removed a type without a change, so nothing should change. + self.assertIsNone(to_persist) + self.assertEqual(added, StateFilter.none()) + + def test_simple_remove_type_change(self) -> None: + """Test removing a type from the config when there are is a matching + state delta does cause the persisted required state config to change""" + + previous_required_state_map = {"type1": {"state_key"}, "type2": {"state_key"}} + request_required_state_map = {"type1": {"state_key"}} + + to_persist, added = _required_state_changes( + user_id="@user:test", + previous_room_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=previous_required_state_map, + ), + room_sync_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=request_required_state_map, + ), + state_deltas={("type2", "state_key"): "$event_id"}, + ) + + # We've removed a type and there's been a change to that state, so we + # persist the change to required state. + assert to_persist is not None + self.assertDictEqual(to_persist, request_required_state_map) + self.assertEqual(added, StateFilter.none()) + + def test_type_wildcards_add(self) -> None: + """Test that changing the wildcard type""" + + previous_required_state_map = {"type1": {"state_key"}} + request_required_state_map = {"type1": {"state_key"}, "*": {"state_key"}} + + to_persist, added = _required_state_changes( + user_id="@user:test", + previous_room_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=previous_required_state_map, + ), + room_sync_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=request_required_state_map, + ), + state_deltas={}, + ) + + # We've added a wildcard, so we persist the change and request everything + assert to_persist is not None + self.assertDictEqual(to_persist, request_required_state_map) + self.assertEqual(added, StateFilter.all()) + + def test_type_wildcards_remove(self) -> None: + """Test that changing the wildcard type""" + + previous_required_state_map = {"type1": {"state_key"}, "*": {"state_key"}} + request_required_state_map = {"type1": {"state_key"}} + + to_persist, added = _required_state_changes( + user_id="@user:test", + previous_room_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=previous_required_state_map, + ), + room_sync_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=request_required_state_map, + ), + state_deltas={}, + ) + + # We've added a wildcard, so we persist the change but don't request anything + assert to_persist is not None + self.assertDictEqual(to_persist, request_required_state_map) + self.assertEqual(added, StateFilter.none()) + + def test_state_key_wildcards_add(self) -> None: + """Test that adding a wildcard to a type works""" + + previous_required_state_map = {"type1": {"state_key"}} + request_required_state_map = {"type1": {"state_key"}, "type2": {"*"}} + + to_persist, added = _required_state_changes( + user_id="@user:test", + previous_room_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=previous_required_state_map, + ), + room_sync_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=request_required_state_map, + ), + state_deltas={}, + ) + + # We've added a wildcard, so we persist the change and request the state + # for that type + assert to_persist is not None + self.assertDictEqual(to_persist, request_required_state_map) + self.assertEqual(added, StateFilter.from_types([("type2", None)])) + + def test_state_key_wildcards_remove(self) -> None: + """Test that removing a wildcard to a type works""" + + previous_required_state_map = {"type1": {"state_key"}, "type2": {"*"}} + request_required_state_map = {"type1": {"state_key"}} + + to_persist, added = _required_state_changes( + user_id="@user:test", + previous_room_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=previous_required_state_map, + ), + room_sync_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=request_required_state_map, + ), + state_deltas={("type2", "state_key"): "$event_id"}, + ) + + # We've removed a wildcard, so we persist the change and request nothing + assert to_persist is not None + self.assertDictEqual(to_persist, request_required_state_map) + self.assertEqual(added, StateFilter.none()) + + def test_state_key_wildcards_remove_no_change(self) -> None: + """Test that removing a wildcard to a type works""" + + previous_required_state_map = {"type1": {"state_key"}, "type2": {"*"}} + request_required_state_map = {"type1": {"state_key"}} + + to_persist, added = _required_state_changes( + user_id="@user:test", + previous_room_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=previous_required_state_map, + ), + room_sync_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=request_required_state_map, + ), + state_deltas={}, + ) + + # We've removed a wildcard but there have been no matching state + # changes, so we don't persist anything. + self.assertIsNone(to_persist) + self.assertEqual(added, StateFilter.none()) + + def test_state_key_remove_change_some(self) -> None: + """Test that removing state keys work when only some of the state keys + have changed""" + + previous_required_state_map = { + "type1": {"state_key1", "state_key2", "state_key3"} + } + request_required_state_map = {"type1": {"state_key1"}} + + to_persist, added = _required_state_changes( + user_id="@user:test", + previous_room_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=previous_required_state_map, + ), + room_sync_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=request_required_state_map, + ), + state_deltas={("type1", "state_key3"): "$event_id"}, + ) + + # We've removed some state keys from the type, but only state_key3 was + # changed so only that one should be removed. + assert to_persist is not None + self.assertDictEqual(to_persist, {"type1": {"state_key1", "state_key2"}}) + self.assertEqual(added, StateFilter.none()) + + def test_state_key_added_me(self) -> None: + """Test that adding state keys work when using "$ME""" + + previous_required_state_map: Dict[str, Set[str]] = {} + request_required_state_map = {"type1": {"$ME"}} + + to_persist, added = _required_state_changes( + user_id="@user:test", + previous_room_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=previous_required_state_map, + ), + room_sync_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=request_required_state_map, + ), + state_deltas={}, + ) + + # We've added the $ME state key, so we should see a persist for that + # change. + assert to_persist is not None + self.assertDictEqual(to_persist, request_required_state_map) + self.assertEqual(added, StateFilter.from_types([("type1", "@user:test")])) + + def test_state_key_remove_me(self) -> None: + """Test that removing state keys work when using "$ME""" + + previous_required_state_map = {"type1": {"$ME"}} + request_required_state_map: Dict[str, Set[str]] = {} + + to_persist, added = _required_state_changes( + user_id="@user:test", + previous_room_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=previous_required_state_map, + ), + room_sync_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=request_required_state_map, + ), + state_deltas={("type1", "@user:test"): "$event_id"}, + ) + + # We've removed the $ME state key and seen a state change for the user, + # so we should see a persist for that change. + assert to_persist is not None + self.assertDictEqual(to_persist, request_required_state_map) + self.assertEqual(added, StateFilter.none()) + + def test_state_key_added_lazy(self) -> None: + """Test that adding state keys work when using "$LAZY""" + + previous_required_state_map: Dict[str, Set[str]] = {} + request_required_state_map = {"type1": {"$LAZY"}} + + to_persist, added = _required_state_changes( + user_id="@user:test", + previous_room_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=previous_required_state_map, + ), + room_sync_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=request_required_state_map, + ), + state_deltas={}, + ) + + # We've added the $LAZY state key, but that gets handled separately so + # it should not appear in `added` + assert to_persist is not None + self.assertDictEqual(to_persist, request_required_state_map) + self.assertEqual(added, StateFilter.none()) + + def test_state_key_remove_lazy(self) -> None: + """Test that removing state keys work when using "$LAZY""" + + previous_required_state_map = {"type1": {"$LAZY"}} + request_required_state_map: Dict[str, Set[str]] = {} + + to_persist, added = _required_state_changes( + user_id="@user:test", + previous_room_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=previous_required_state_map, + ), + room_sync_config=RoomSyncConfig( + timeline_limit=0, + required_state_map=request_required_state_map, + ), + state_deltas={("type1", "@user:test"): "$event_id"}, + ) + + # We've removed the $LAZY state key so we persist it when there has been + # any state delta for the type. + assert to_persist is not None + self.assertDictEqual(to_persist, request_required_state_map) + self.assertEqual(added, StateFilter.none()) From 5e66e98540be0035792cc0912ae356eda33458ce Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 3 Oct 2024 16:32:48 +0100 Subject: [PATCH 03/14] Newsfile --- changelog.d/17785.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17785.bugfix diff --git a/changelog.d/17785.bugfix b/changelog.d/17785.bugfix new file mode 100644 index 0000000000..df2898f54e --- /dev/null +++ b/changelog.d/17785.bugfix @@ -0,0 +1 @@ +Fix bug with sliding sync where the server would not return state that was added to the `required_state` config. From 596581a4161b4e540fc1b09b06ecdd882cb5a255 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 9 Oct 2024 10:34:17 -0500 Subject: [PATCH 04/14] Sliding Sync: Parameterize and add more tests for tracking changes to required state (#17805) Parameterize and add more tests for tracking changes to required state (`_required_state_changes(...)`) These are direct modifications to https://github.com/element-hq/synapse/pull/17785 and should be merged into that PR. --- changelog.d/17805.bugfix | 1 + synapse/handlers/sliding_sync/__init__.py | 24 +- tests/handlers/test_sliding_sync.py | 935 ++++++++++++++-------- 3 files changed, 604 insertions(+), 356 deletions(-) create mode 100644 changelog.d/17805.bugfix diff --git a/changelog.d/17805.bugfix b/changelog.d/17805.bugfix new file mode 100644 index 0000000000..df2898f54e --- /dev/null +++ b/changelog.d/17805.bugfix @@ -0,0 +1 @@ +Fix bug with sliding sync where the server would not return state that was added to the `required_state` config. diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 4915682f6e..9afa7a43d1 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1313,6 +1313,13 @@ def _required_state_changes( prev_wildcard = prev_required_state_map.get(StateValues.WILDCARD, set()) request_wildcard = request_required_state_map.get(StateValues.WILDCARD, set()) + # If we were previously fetching everything ("*", "*"), always update the effective + # room required state config to match the request. And since we we're previously + # already fetching everything, we don't have to fetch anything now that they've + # narrowed. + if StateValues.WILDCARD in prev_wildcard: + return request_required_state_map, StateFilter.none() + # If a event type wildcard has been added or removed we don't try and do # anything fancy, and instead always update the effective room required # state config to match the request. @@ -1349,6 +1356,11 @@ def _required_state_changes( # Always update changes to include the newly added keys changes[event_type] = request_state_keys + if StateValues.WILDCARD in old_state_keys: + # We were previously fetching everything for this type, so we don't need to + # fetch anything new. + continue + # Record the new state keys to fetch for this type. if StateValues.WILDCARD in request_state_keys: # If we have added a wildcard then we always just fetch everything. @@ -1358,8 +1370,8 @@ def _required_state_changes( if state_key == StateValues.ME: added.append((event_type, user_id)) elif state_key == StateValues.LAZY: - # We handle lazy loading separately, so don't need to - # explicitly add anything here. + # We handle lazy loading separately (outside this function), so + # don't need to explicitly add anything here. pass else: added.append((event_type, state_key)) @@ -1397,8 +1409,8 @@ def _required_state_changes( request_state_key_wildcard = StateValues.WILDCARD in request_state_keys if old_state_key_wildcard != request_state_key_wildcard: - # If a wildcard has been added or removed we always update the - # required state when any state with the same type has changed. + # If a state_key wildcard has been added or removed, we always update the + # effective room required state config to match the request. changes[event_type] = request_state_keys continue @@ -1406,8 +1418,8 @@ def _required_state_changes( request_state_key_lazy = StateValues.LAZY in request_state_keys if old_state_key_lazy != request_state_key_lazy: - # If a "$LAZY" has been added or removed we always update the - # required state for simplicity. + # If a "$LAZY" has been added or removed we always update the effective room + # required state config to match the request. changes[event_type] = request_state_keys continue diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index eaee50e4f8..3e0abd3fca 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -18,9 +18,10 @@ # # import logging -from typing import AbstractSet, Dict, Optional, Set, Tuple +from typing import AbstractSet, Dict, Mapping, Optional, Set, Tuple from unittest.mock import patch +import attr from parameterized import parameterized from twisted.test.proto_helpers import MemoryReactor @@ -41,7 +42,7 @@ from synapse.rest.client import knock, login, room from synapse.server import HomeServer from synapse.storage.util.id_generators import MultiWriterIdGenerator -from synapse.types import JsonDict, StreamToken, UserID +from synapse.types import JsonDict, StateMap, StreamToken, UserID from synapse.types.handlers.sliding_sync import SlidingSyncConfig from synapse.types.state import StateFilter from synapse.util import Clock @@ -3218,400 +3219,634 @@ def test_default_bump_event_types(self) -> None: ) +@attr.s(slots=True, auto_attribs=True, frozen=True) +class RequiredStateChangesTestParameters: + previous_required_state_map: Dict[str, Set[str]] + request_required_state_map: Dict[str, Set[str]] + state_deltas: StateMap[str] + expected_with_state_deltas: Tuple[ + Optional[Mapping[str, AbstractSet[str]]], StateFilter + ] + expected_without_state_deltas: Tuple[ + Optional[Mapping[str, AbstractSet[str]]], StateFilter + ] + + class RequiredStateChangesTestCase(unittest.TestCase): """Test cases for `_required_state_changes`""" - def test_simple_no_change(self) -> None: - """Test no change to required state""" - previous_required_state_map = {"type1": {"state_key"}} - request_required_state_map = previous_required_state_map - - to_persist, added = _required_state_changes( - user_id="@user:test", - previous_room_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=previous_required_state_map, + @parameterized.expand( + [ + ( + "simple_no_change", + """Test no change to required state""", + RequiredStateChangesTestParameters( + previous_required_state_map={"type1": {"state_key"}}, + request_required_state_map={"type1": {"state_key"}}, + state_deltas={("type1", "state_key"): "$event_id"}, + # No changes + expected_with_state_deltas=(None, StateFilter.none()), + expected_without_state_deltas=(None, StateFilter.none()), + ), ), - room_sync_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=request_required_state_map, + ( + "simple_add_type", + """Test adding a type to the config""", + RequiredStateChangesTestParameters( + previous_required_state_map={"type1": {"state_key"}}, + request_required_state_map={ + "type1": {"state_key"}, + "type2": {"state_key"}, + }, + state_deltas={("type2", "state_key"): "$event_id"}, + expected_with_state_deltas=( + # We've added a type so we should persist the changed required state + # config. + {"type1": {"state_key"}, "type2": {"state_key"}}, + # We should see the new type added + StateFilter.from_types([("type2", "state_key")]), + ), + expected_without_state_deltas=( + {"type1": {"state_key"}, "type2": {"state_key"}}, + StateFilter.from_types([("type2", "state_key")]), + ), + ), ), - state_deltas={}, - ) - - # No changes - self.assertIsNone(to_persist) - self.assertEqual(added, StateFilter.none()) - - def test_simple_add_type(self) -> None: - """Test adding a type to the config""" - previous_required_state_map = {"type1": {"state_key"}} - request_required_state_map = {"type1": {"state_key"}, "type2": {"state_key"}} - - to_persist, added = _required_state_changes( - user_id="@user:test", - previous_room_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=previous_required_state_map, + ( + "simple_add_type_from_nothing", + """Test adding a type to the config when previously requesting nothing""", + RequiredStateChangesTestParameters( + previous_required_state_map={}, + request_required_state_map={ + "type1": {"state_key"}, + "type2": {"state_key"}, + }, + state_deltas={("type2", "state_key"): "$event_id"}, + expected_with_state_deltas=( + # We've added a type so we should persist the changed required state + # config. + {"type1": {"state_key"}, "type2": {"state_key"}}, + # We should see the new types added + StateFilter.from_types( + [("type1", "state_key"), ("type2", "state_key")] + ), + ), + expected_without_state_deltas=( + {"type1": {"state_key"}, "type2": {"state_key"}}, + StateFilter.from_types( + [("type1", "state_key"), ("type2", "state_key")] + ), + ), + ), ), - room_sync_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=request_required_state_map, + ( + "simple_add_state_key", + """Test adding a state key to the config""", + RequiredStateChangesTestParameters( + previous_required_state_map={"type": {"state_key1"}}, + request_required_state_map={"type": {"state_key1", "state_key2"}}, + state_deltas={("type", "state_key2"): "$event_id"}, + expected_with_state_deltas=( + # We've added a key so we should persist the changed required state + # config. + {"type": {"state_key1", "state_key2"}}, + # We should see the new state_keys added + StateFilter.from_types([("type", "state_key2")]), + ), + expected_without_state_deltas=( + {"type": {"state_key1", "state_key2"}}, + StateFilter.from_types([("type", "state_key2")]), + ), + ), ), - state_deltas={}, - ) - - # We've added a type so we should persist the changed required state - # config. - assert to_persist is not None - self.assertDictEqual(to_persist, request_required_state_map) - - # We should see the new type added - self.assertEqual(added, StateFilter.from_types([("type2", "state_key")])) - - def test_simple_add_state_key(self) -> None: - """Test adding a state key to the config""" - previous_required_state_map = {"type": {"state_key1"}} - request_required_state_map = {"type": {"state_key1", "state_key2"}} + ( + "simple_remove_type", + """ + Test removing a type from the config when there are a matching state + delta does cause the persisted required state config to change - to_persist, added = _required_state_changes( - user_id="@user:test", - previous_room_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=previous_required_state_map, - ), - room_sync_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=request_required_state_map, + Test removing a type from the config when there are no matching state + deltas does *not* cause the persisted required state config to change + """, + RequiredStateChangesTestParameters( + previous_required_state_map={ + "type1": {"state_key"}, + "type2": {"state_key"}, + }, + request_required_state_map={"type1": {"state_key"}}, + state_deltas={("type2", "state_key"): "$event_id"}, + expected_with_state_deltas=( + # Remove `type2` since there's been a change to that state, + # (persist the change to required state). That way next time, + # they request `type2`, we see that we haven't sent it before + # and send the new state. (we should still keep track that we've + # sent `type1` before). + {"type1": {"state_key"}}, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), + ), + expected_without_state_deltas=( + # `type2` is no longer requested but since that state hasn't + # changed, nothing should change (we should still keep track + # that we've sent `type2` before). + None, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), + ), + ), ), - state_deltas={}, - ) - - # We've added a type so we should persist the changed required state - # config. - assert to_persist is not None - self.assertDictEqual(to_persist, request_required_state_map) - self.assertEqual(added, StateFilter.from_types([("type", "state_key2")])) - - def test_simple_add_state_key_with_change(self) -> None: - """Test adding a state key to the config, and see some changed state""" - previous_required_state_map = {"type": {"state_key1"}} - request_required_state_map = {"type": {"state_key1", "state_key2"}} - - to_persist, added = _required_state_changes( - user_id="@user:test", - previous_room_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=previous_required_state_map, + ( + "simple_remove_type_to_nothing", + """ + Test removing a type from the config and no longer requesting any state + """, + RequiredStateChangesTestParameters( + previous_required_state_map={ + "type1": {"state_key"}, + "type2": {"state_key"}, + }, + request_required_state_map={}, + state_deltas={("type2", "state_key"): "$event_id"}, + expected_with_state_deltas=( + # Remove `type2` since there's been a change to that state, + # (persist the change to required state). That way next time, + # they request `type2`, we see that we haven't sent it before + # and send the new state. (we should still keep track that we've + # sent `type1` before). + {"type1": {"state_key"}}, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), + ), + expected_without_state_deltas=( + # `type2` is no longer requested but since that state hasn't + # changed, nothing should change (we should still keep track + # that we've sent `type2` before). + None, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), + ), + ), ), - room_sync_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=request_required_state_map, + ( + "simple_remove_state_key", + """ + Test removing a state_key from the config + """, + RequiredStateChangesTestParameters( + previous_required_state_map={"type": {"state_key1", "state_key2"}}, + request_required_state_map={"type": {"state_key1"}}, + state_deltas={("type", "state_key2"): "$event_id"}, + expected_with_state_deltas=( + # Remove `(type, state_key2)` since there's been a change + # to that state (persist the change to required state). + # That way next time, they request `(type, state_key2)`, we see + # that we haven't sent it before and send the new state. (we + # should still keep track that we've sent `(type, state_key1)` + # before). + {"type": {"state_key1"}}, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), + ), + expected_without_state_deltas=( + # `(type, state_key2)` is no longer requested but since that + # state hasn't changed, nothing should change (we should still + # keep track that we've sent `(type, state_key1)` and `(type, + # state_key2)` before). + None, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), + ), + ), ), - state_deltas={("type", "state_key"): "$event_id"}, - ) - - # We've added a type so we should persist the changed required state - # config. - assert to_persist is not None - self.assertDictEqual(to_persist, request_required_state_map) - self.assertEqual(added, StateFilter.from_types([("type", "state_key2")])) - - def test_simple_remove_type_no_change(self) -> None: - """Test removing a type from the config when there are no matching state - deltas not cause the persisted required state config to change""" + ( + "type_wildcards_add", + """ + Test adding a wildcard type causes the persisted required state config + to change and we request everything. - previous_required_state_map = {"type1": {"state_key"}, "type2": {"state_key"}} - request_required_state_map = {"type1": {"state_key"}} + If a event type wildcard has been added or removed we don't try and do + anything fancy, and instead always update the effective room required + state config to match the request. + """, + RequiredStateChangesTestParameters( + previous_required_state_map={"type1": {"state_key2"}}, + request_required_state_map={ + "type1": {"state_key2"}, + StateValues.WILDCARD: {"state_key"}, + }, + state_deltas={ + ("other_type", "state_key"): "$event_id", + }, + # We've added a wildcard, so we persist the change and request everything + expected_with_state_deltas=( + {"type1": {"state_key2"}, StateValues.WILDCARD: {"state_key"}}, + StateFilter.all(), + ), + expected_without_state_deltas=( + {"type1": {"state_key2"}, StateValues.WILDCARD: {"state_key"}}, + StateFilter.all(), + ), + ), + ), + ( + "type_wildcards_remove", + """ + Test removing a wildcard type causes the persisted required state config + to change and request nothing. - to_persist, added = _required_state_changes( - user_id="@user:test", - previous_room_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=previous_required_state_map, + If a event type wildcard has been added or removed we don't try and do + anything fancy, and instead always update the effective room required + state config to match the request. + """, + RequiredStateChangesTestParameters( + previous_required_state_map={ + "type1": {"state_key2"}, + StateValues.WILDCARD: {"state_key"}, + }, + request_required_state_map={"type1": {"state_key2"}}, + state_deltas={ + ("other_type", "state_key"): "$event_id", + }, + # We've removed a type wildcard, so we persist the change but don't request anything + expected_with_state_deltas=( + {"type1": {"state_key2"}}, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), + ), + expected_without_state_deltas=( + {"type1": {"state_key2"}}, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), + ), + ), ), - room_sync_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=request_required_state_map, + ( + "state_key_wildcards_add", + """Test adding a wildcard state_key""", + RequiredStateChangesTestParameters( + previous_required_state_map={"type1": {"state_key"}}, + request_required_state_map={ + "type1": {"state_key"}, + "type2": {StateValues.WILDCARD}, + }, + state_deltas={("type2", "state_key"): "$event_id"}, + # We've added a wildcard state_key, so we persist the change and + # request all of the state for that type + expected_with_state_deltas=( + {"type1": {"state_key"}, "type2": {StateValues.WILDCARD}}, + StateFilter.from_types([("type2", None)]), + ), + expected_without_state_deltas=( + {"type1": {"state_key"}, "type2": {StateValues.WILDCARD}}, + StateFilter.from_types([("type2", None)]), + ), + ), ), - state_deltas={}, - ) - - # We've removed a type without a change, so nothing should change. - self.assertIsNone(to_persist) - self.assertEqual(added, StateFilter.none()) - - def test_simple_remove_type_change(self) -> None: - """Test removing a type from the config when there are is a matching - state delta does cause the persisted required state config to change""" - - previous_required_state_map = {"type1": {"state_key"}, "type2": {"state_key"}} - request_required_state_map = {"type1": {"state_key"}} - - to_persist, added = _required_state_changes( - user_id="@user:test", - previous_room_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=previous_required_state_map, + ( + "state_key_wildcards_remove", + """Test removing a wildcard state_key""", + RequiredStateChangesTestParameters( + previous_required_state_map={ + "type1": {"state_key"}, + "type2": {StateValues.WILDCARD}, + }, + request_required_state_map={"type1": {"state_key"}}, + state_deltas={("type2", "state_key"): "$event_id"}, + # We've removed a state_key wildcard, so we persist the change and + # request nothing + expected_with_state_deltas=( + {"type1": {"state_key"}}, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), + ), + # We've removed a state_key wildcard but there have been no matching + # state changes, so no changes needed, just persist the + # `request_required_state_map` as-is. + expected_without_state_deltas=( + None, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), + ), + ), ), - room_sync_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=request_required_state_map, + ( + "state_key_remove_some", + """ + Test that removing state keys work when only some of the state keys have + changed + """, + RequiredStateChangesTestParameters( + previous_required_state_map={ + "type1": {"state_key1", "state_key2", "state_key3"} + }, + request_required_state_map={"type1": {"state_key1"}}, + state_deltas={("type1", "state_key3"): "$event_id"}, + expected_with_state_deltas=( + # We've removed some state keys from the type, but only state_key3 was + # changed so only that one should be removed. + {"type1": {"state_key1", "state_key2"}}, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), + ), + expected_without_state_deltas=( + # No changes needed, just persist the + # `request_required_state_map` as-is + None, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), + ), + ), ), - state_deltas={("type2", "state_key"): "$event_id"}, - ) - - # We've removed a type and there's been a change to that state, so we - # persist the change to required state. - assert to_persist is not None - self.assertDictEqual(to_persist, request_required_state_map) - self.assertEqual(added, StateFilter.none()) - - def test_type_wildcards_add(self) -> None: - """Test that changing the wildcard type""" - - previous_required_state_map = {"type1": {"state_key"}} - request_required_state_map = {"type1": {"state_key"}, "*": {"state_key"}} - - to_persist, added = _required_state_changes( - user_id="@user:test", - previous_room_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=previous_required_state_map, + ( + "state_key_me_add", + """ + Test adding state keys work when using "$ME" + """, + RequiredStateChangesTestParameters( + previous_required_state_map={}, + request_required_state_map={"type1": {StateValues.ME}}, + state_deltas={("type1", "@user:test"): "$event_id"}, + expected_with_state_deltas=( + # We've added a type so we should persist the changed required state + # config. + {"type1": {StateValues.ME}}, + # We should see the new state_keys added + StateFilter.from_types([("type1", "@user:test")]), + ), + expected_without_state_deltas=( + {"type1": {StateValues.ME}}, + StateFilter.from_types([("type1", "@user:test")]), + ), + ), ), - room_sync_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=request_required_state_map, + ( + "state_key_me_remove", + """ + Test removing state keys work when using "$ME" + """, + RequiredStateChangesTestParameters( + previous_required_state_map={"type1": {StateValues.ME}}, + request_required_state_map={}, + state_deltas={("type1", "@user:test"): "$event_id"}, + expected_with_state_deltas=( + # Remove `type1` since there's been a change to that state, + # (persist the change to required state). That way next time, + # they request `type1`, we see that we haven't sent it before + # and send the new state. (if we were tracking that we sent any + # other state, we should still keep track that). + {}, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), + ), + expected_without_state_deltas=( + # `type1` is no longer requested but since that state hasn't + # changed, nothing should change (we should still keep track + # that we've sent `type1` before). + None, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), + ), + ), ), - state_deltas={}, - ) - - # We've added a wildcard, so we persist the change and request everything - assert to_persist is not None - self.assertDictEqual(to_persist, request_required_state_map) - self.assertEqual(added, StateFilter.all()) - - def test_type_wildcards_remove(self) -> None: - """Test that changing the wildcard type""" - - previous_required_state_map = {"type1": {"state_key"}, "*": {"state_key"}} - request_required_state_map = {"type1": {"state_key"}} - - to_persist, added = _required_state_changes( - user_id="@user:test", - previous_room_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=previous_required_state_map, + ( + "state_key_lazy_add", + """ + Test adding state keys work when using "$LAZY" + """, + RequiredStateChangesTestParameters( + previous_required_state_map={}, + request_required_state_map={EventTypes.Member: {StateValues.LAZY}}, + state_deltas={(EventTypes.Member, "@user:test"): "$event_id"}, + expected_with_state_deltas=( + # If a "$LAZY" has been added or removed we always update the + # required state to what was requested for simplicity. + {EventTypes.Member: {StateValues.LAZY}}, + StateFilter.none(), + ), + expected_without_state_deltas=( + {EventTypes.Member: {StateValues.LAZY}}, + StateFilter.none(), + ), + ), ), - room_sync_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=request_required_state_map, + ( + "state_key_lazy_remove", + """ + Test removing state keys work when using "$LAZY" + """, + RequiredStateChangesTestParameters( + previous_required_state_map={EventTypes.Member: {StateValues.LAZY}}, + request_required_state_map={}, + state_deltas={(EventTypes.Member, "@user:test"): "$event_id"}, + expected_with_state_deltas=( + # If a "$LAZY" has been added or removed we always update the + # required state to what was requested for simplicity. + {}, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), + ), + expected_without_state_deltas=( + # `EventTypes.Member` is no longer requested but since that + # state hasn't changed, nothing should change (we should still + # keep track that we've sent `EventTypes.Member` before). + None, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), + ), + ), ), - state_deltas={}, - ) - - # We've added a wildcard, so we persist the change but don't request anything - assert to_persist is not None - self.assertDictEqual(to_persist, request_required_state_map) - self.assertEqual(added, StateFilter.none()) - - def test_state_key_wildcards_add(self) -> None: - """Test that adding a wildcard to a type works""" - - previous_required_state_map = {"type1": {"state_key"}} - request_required_state_map = {"type1": {"state_key"}, "type2": {"*"}} - - to_persist, added = _required_state_changes( - user_id="@user:test", - previous_room_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=previous_required_state_map, + ( + "type_wildcard_with_state_key_wildcard_to_explicit_state_keys", + """ + Test switching from a wildcard ("*", "*") to explicit state keys + """, + RequiredStateChangesTestParameters( + previous_required_state_map={ + StateValues.WILDCARD: {StateValues.WILDCARD} + }, + request_required_state_map={ + StateValues.WILDCARD: {"state_key1", "state_key2", "state_key3"} + }, + state_deltas={("type1", "state_key1"): "$event_id"}, + # If we were previously fetching everything ("*", "*"), always update the effective + # room required state config to match the request. And since we we're previously + # already fetching everything, we don't have to fetch anything now that they've + # narrowed. + expected_with_state_deltas=( + { + StateValues.WILDCARD: { + "state_key1", + "state_key2", + "state_key3", + } + }, + StateFilter.none(), + ), + expected_without_state_deltas=( + { + StateValues.WILDCARD: { + "state_key1", + "state_key2", + "state_key3", + } + }, + StateFilter.none(), + ), + ), ), - room_sync_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=request_required_state_map, + ( + "type_wildcard_with_explicit_state_keys_to_wildcard_state_key", + """ + Test switching from explicit to wildcard state keys ("*", "*") + """, + RequiredStateChangesTestParameters( + previous_required_state_map={ + StateValues.WILDCARD: {"state_key1", "state_key2", "state_key3"} + }, + request_required_state_map={ + StateValues.WILDCARD: {StateValues.WILDCARD} + }, + state_deltas={("type1", "state_key1"): "$event_id"}, + # We've added a wildcard, so we persist the change and request everything + expected_with_state_deltas=( + {StateValues.WILDCARD: {StateValues.WILDCARD}}, + StateFilter.all(), + ), + expected_without_state_deltas=( + {StateValues.WILDCARD: {StateValues.WILDCARD}}, + StateFilter.all(), + ), + ), ), - state_deltas={}, - ) - - # We've added a wildcard, so we persist the change and request the state - # for that type - assert to_persist is not None - self.assertDictEqual(to_persist, request_required_state_map) - self.assertEqual(added, StateFilter.from_types([("type2", None)])) - - def test_state_key_wildcards_remove(self) -> None: - """Test that removing a wildcard to a type works""" - - previous_required_state_map = {"type1": {"state_key"}, "type2": {"*"}} - request_required_state_map = {"type1": {"state_key"}} - - to_persist, added = _required_state_changes( - user_id="@user:test", - previous_room_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=previous_required_state_map, + ( + "state_key_wildcard_to_explicit_state_keys", + """Test switching from a wildcard to explicit state keys with a concrete type""", + RequiredStateChangesTestParameters( + previous_required_state_map={"type1": {StateValues.WILDCARD}}, + request_required_state_map={ + "type1": {"state_key1", "state_key2", "state_key3"} + }, + state_deltas={("type1", "state_key1"): "$event_id"}, + # If a state_key wildcard has been added or removed, we always + # update the effective room required state config to match the + # request. And since we we're previously already fetching + # everything, we don't have to fetch anything now that they've + # narrowed. + expected_with_state_deltas=( + { + "type1": { + "state_key1", + "state_key2", + "state_key3", + } + }, + StateFilter.none(), + ), + expected_without_state_deltas=( + { + "type1": { + "state_key1", + "state_key2", + "state_key3", + } + }, + StateFilter.none(), + ), + ), ), - room_sync_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=request_required_state_map, + ( + "state_key_wildcard_to_explicit_state_keys", + """Test switching from a wildcard to explicit state keys with a concrete type""", + RequiredStateChangesTestParameters( + previous_required_state_map={ + "type1": {"state_key1", "state_key2", "state_key3"} + }, + request_required_state_map={"type1": {StateValues.WILDCARD}}, + state_deltas={("type1", "state_key1"): "$event_id"}, + # If a state_key wildcard has been added or removed, we always + # update the effective room required state config to match the + # request. And we need to request all of the state for that type + # because we previously, only sent down a few keys. + expected_with_state_deltas=( + {"type1": {StateValues.WILDCARD}}, + StateFilter.from_types([("type1", None)]), + ), + expected_without_state_deltas=( + {"type1": {StateValues.WILDCARD}}, + StateFilter.from_types([("type1", None)]), + ), + ), ), - state_deltas={("type2", "state_key"): "$event_id"}, - ) - - # We've removed a wildcard, so we persist the change and request nothing - assert to_persist is not None - self.assertDictEqual(to_persist, request_required_state_map) - self.assertEqual(added, StateFilter.none()) - - def test_state_key_wildcards_remove_no_change(self) -> None: - """Test that removing a wildcard to a type works""" - - previous_required_state_map = {"type1": {"state_key"}, "type2": {"*"}} - request_required_state_map = {"type1": {"state_key"}} - - to_persist, added = _required_state_changes( + ] + ) + def test_xxx( + self, + _test_label: str, + _test_description: str, + test_parameters: RequiredStateChangesTestParameters, + ) -> None: + # Without `state_deltas` + changed_required_state_map, added_state_filter = _required_state_changes( user_id="@user:test", previous_room_config=RoomSyncConfig( timeline_limit=0, - required_state_map=previous_required_state_map, + required_state_map=test_parameters.previous_required_state_map, ), room_sync_config=RoomSyncConfig( timeline_limit=0, - required_state_map=request_required_state_map, + required_state_map=test_parameters.request_required_state_map, ), state_deltas={}, ) - # We've removed a wildcard but there have been no matching state - # changes, so we don't persist anything. - self.assertIsNone(to_persist) - self.assertEqual(added, StateFilter.none()) - - def test_state_key_remove_change_some(self) -> None: - """Test that removing state keys work when only some of the state keys - have changed""" - - previous_required_state_map = { - "type1": {"state_key1", "state_key2", "state_key3"} - } - request_required_state_map = {"type1": {"state_key1"}} - - to_persist, added = _required_state_changes( - user_id="@user:test", - previous_room_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=previous_required_state_map, - ), - room_sync_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=request_required_state_map, - ), - state_deltas={("type1", "state_key3"): "$event_id"}, + self.assertEqual( + changed_required_state_map, + test_parameters.expected_without_state_deltas[0], + "changed_required_state_map does not match (without state_deltas)", ) - - # We've removed some state keys from the type, but only state_key3 was - # changed so only that one should be removed. - assert to_persist is not None - self.assertDictEqual(to_persist, {"type1": {"state_key1", "state_key2"}}) - self.assertEqual(added, StateFilter.none()) - - def test_state_key_added_me(self) -> None: - """Test that adding state keys work when using "$ME""" - - previous_required_state_map: Dict[str, Set[str]] = {} - request_required_state_map = {"type1": {"$ME"}} - - to_persist, added = _required_state_changes( - user_id="@user:test", - previous_room_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=previous_required_state_map, - ), - room_sync_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=request_required_state_map, - ), - state_deltas={}, + self.assertEqual( + added_state_filter, + test_parameters.expected_without_state_deltas[1], + "added_state_filter does not match (without state_deltas)", ) - # We've added the $ME state key, so we should see a persist for that - # change. - assert to_persist is not None - self.assertDictEqual(to_persist, request_required_state_map) - self.assertEqual(added, StateFilter.from_types([("type1", "@user:test")])) - - def test_state_key_remove_me(self) -> None: - """Test that removing state keys work when using "$ME""" - - previous_required_state_map = {"type1": {"$ME"}} - request_required_state_map: Dict[str, Set[str]] = {} - - to_persist, added = _required_state_changes( + # With `state_deltas` + changed_required_state_map, added_state_filter = _required_state_changes( user_id="@user:test", previous_room_config=RoomSyncConfig( timeline_limit=0, - required_state_map=previous_required_state_map, + required_state_map=test_parameters.previous_required_state_map, ), room_sync_config=RoomSyncConfig( timeline_limit=0, - required_state_map=request_required_state_map, + required_state_map=test_parameters.request_required_state_map, ), - state_deltas={("type1", "@user:test"): "$event_id"}, + state_deltas=test_parameters.state_deltas, ) - # We've removed the $ME state key and seen a state change for the user, - # so we should see a persist for that change. - assert to_persist is not None - self.assertDictEqual(to_persist, request_required_state_map) - self.assertEqual(added, StateFilter.none()) - - def test_state_key_added_lazy(self) -> None: - """Test that adding state keys work when using "$LAZY""" - - previous_required_state_map: Dict[str, Set[str]] = {} - request_required_state_map = {"type1": {"$LAZY"}} - - to_persist, added = _required_state_changes( - user_id="@user:test", - previous_room_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=previous_required_state_map, - ), - room_sync_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=request_required_state_map, - ), - state_deltas={}, + self.assertEqual( + changed_required_state_map, + test_parameters.expected_with_state_deltas[0], + "changed_required_state_map does not match (with state_deltas)", ) - - # We've added the $LAZY state key, but that gets handled separately so - # it should not appear in `added` - assert to_persist is not None - self.assertDictEqual(to_persist, request_required_state_map) - self.assertEqual(added, StateFilter.none()) - - def test_state_key_remove_lazy(self) -> None: - """Test that removing state keys work when using "$LAZY""" - - previous_required_state_map = {"type1": {"$LAZY"}} - request_required_state_map: Dict[str, Set[str]] = {} - - to_persist, added = _required_state_changes( - user_id="@user:test", - previous_room_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=previous_required_state_map, - ), - room_sync_config=RoomSyncConfig( - timeline_limit=0, - required_state_map=request_required_state_map, - ), - state_deltas={("type1", "@user:test"): "$event_id"}, + self.assertEqual( + added_state_filter, + test_parameters.expected_with_state_deltas[1], + "added_state_filter does not match (with state_deltas)", ) - - # We've removed the $LAZY state key so we persist it when there has been - # any state delta for the type. - assert to_persist is not None - self.assertDictEqual(to_persist, request_required_state_map) - self.assertEqual(added, StateFilter.none()) From 4b631a58b596ab56e9d53c0175beff1cc6478c0d Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 9 Oct 2024 16:43:43 +0100 Subject: [PATCH 05/14] Apply suggestions from code review Co-authored-by: Eric Eastwood --- synapse/handlers/sliding_sync/__init__.py | 3 ++- tests/rest/client/sliding_sync/test_rooms_required_state.py | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 9afa7a43d1..c370b9c8aa 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1281,6 +1281,7 @@ async def _get_bump_stamp( def _required_state_changes( user_id: str, + *, previous_room_config: "RoomSyncConfig", room_sync_config: RoomSyncConfig, state_deltas: StateMap[str], @@ -1433,7 +1434,7 @@ def _required_state_changes( # # We only remove state keys from the effective state if they've been # removed from the request *and* the state has changed. This ensures - # that if a client removes and then readds a state key, we only send + # that if a client removes and then re-adds a state key, we only send # down the associated current state event if its changed (rather than # sending down the same event twice). invalidated = (old_state_keys - request_state_keys) & changed_state_keys diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index 0d98a6d889..e5d068b5ad 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -1125,5 +1125,4 @@ def test_rooms_required_state_expand_deduplicate(self) -> None: state_map = self.get_success( self.storage_controllers.state.get_current_state(room_id1) ) - # Send a message so the room comes down sync. self.assertIsNone(response_body["rooms"][room_id1].get("required_state")) From e9130376cb0c2b66b8ab4fa8ccfcf7097314aabe Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 9 Oct 2024 16:47:31 +0100 Subject: [PATCH 06/14] Fixup --- synapse/handlers/sliding_sync/__init__.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index c370b9c8aa..66469b2ea5 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1011,9 +1011,9 @@ async def get_room_sync_data( changed_required_state_map, added_state_filter = ( _required_state_changes( user.to_string(), - prev_room_sync_config, - room_sync_config, - room_state_delta_id_map, + previous_room_config=prev_room_sync_config, + room_sync_config=room_sync_config, + state_deltas=room_state_delta_id_map, ) ) From 35532d664e197f9a38f846b287ca46be432cbe62 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 9 Oct 2024 16:48:45 +0100 Subject: [PATCH 07/14] comment __bool__ --- synapse/types/state.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/types/state.py b/synapse/types/state.py index 060a5b6183..67d1c3fe97 100644 --- a/synapse/types/state.py +++ b/synapse/types/state.py @@ -617,6 +617,8 @@ def __contains__(self, key: Any) -> bool: return False def __bool__(self) -> bool: + """Returns true if this state filter will match any state, or false if + this is the empty filter""" if self.include_others: return True return bool(self.types) From 859d4a850f3c1118423044e744707b30b6116aff Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 9 Oct 2024 16:52:08 +0100 Subject: [PATCH 08/14] Fix handling of your own user ID --- synapse/handlers/sliding_sync/__init__.py | 4 +- tests/handlers/test_sliding_sync.py | 53 +++++++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 66469b2ea5..731431878f 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1424,9 +1424,9 @@ def _required_state_changes( changes[event_type] = request_state_keys continue - # Handle "$ME" values by replacing state keys that match the user ID. + # Handle "$ME" values by adding "$ME" if the state key matches the user + # ID. if user_id in changed_state_keys: - changed_state_keys.discard(user_id) changed_state_keys.add(StateValues.ME) # At this point there are no wildcards and no additions to the set of diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 3e0abd3fca..9a68d1dd95 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -3622,6 +3622,59 @@ class RequiredStateChangesTestCase(unittest.TestCase): ), ), ), + ( + "state_key_user_id_add", + """ + Test adding state keys work when using your own user ID + """, + RequiredStateChangesTestParameters( + previous_required_state_map={}, + request_required_state_map={"type1": {"@user:test"}}, + state_deltas={("type1", "@user:test"): "$event_id"}, + expected_with_state_deltas=( + # We've added a type so we should persist the changed required state + # config. + {"type1": {"@user:test"}}, + # We should see the new state_keys added + StateFilter.from_types([("type1", "@user:test")]), + ), + expected_without_state_deltas=( + {"type1": {"@user:test"}}, + StateFilter.from_types([("type1", "@user:test")]), + ), + ), + ), + ( + "state_key_me_remove", + """ + Test removing state keys work when using your own user ID + """, + RequiredStateChangesTestParameters( + previous_required_state_map={"type1": {"@user:test"}}, + request_required_state_map={}, + state_deltas={("type1", "@user:test"): "$event_id"}, + expected_with_state_deltas=( + # Remove `type1` since there's been a change to that state, + # (persist the change to required state). That way next time, + # they request `type1`, we see that we haven't sent it before + # and send the new state. (if we were tracking that we sent any + # other state, we should still keep track that). + {}, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), + ), + expected_without_state_deltas=( + # `type1` is no longer requested but since that state hasn't + # changed, nothing should change (we should still keep track + # that we've sent `type1` before). + None, + # We don't need to request anything more if they are requesting + # less state now + StateFilter.none(), + ), + ), + ), ( "state_key_lazy_add", """ From 56565dc7447fcb8fbf0559bc14b4fd912ee23348 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 9 Oct 2024 16:52:38 +0100 Subject: [PATCH 09/14] Remove unused var --- tests/rest/client/sliding_sync/test_rooms_required_state.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index e5d068b5ad..7da51d4954 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -1122,7 +1122,4 @@ def test_rooms_required_state_expand_deduplicate(self) -> None: # We should not see the room name again, as we have already sent that # down. - state_map = self.get_success( - self.storage_controllers.state.get_current_state(room_id1) - ) self.assertIsNone(response_body["rooms"][room_id1].get("required_state")) From 0d0207e9ad913d0760f66b8cc3b5597f4e6da638 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 9 Oct 2024 16:53:36 +0100 Subject: [PATCH 10/14] Remove TODOs --- synapse/handlers/sliding_sync/__init__.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 731431878f..16e949af09 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -541,7 +541,6 @@ async def get_room_sync_data( # or `limited` mean for clients that interpret them correctly. In future this # behavior is almost certainly going to change. # - # TODO: Also handle changes to `required_state` from_bound = None initial = True ignore_timeline_bound = False @@ -573,8 +572,6 @@ async def get_room_sync_data( ): ignore_timeline_bound = True - # TODO: Check for changes in `required_state`` - log_kv( { "sliding_sync.from_bound": from_bound, From fafb2fe1cf54944325e6f9e86822f4e7215d1cd9 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 9 Oct 2024 16:54:37 +0100 Subject: [PATCH 11/14] Move comment down --- synapse/handlers/sliding_sync/__init__.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 16e949af09..e24d79630c 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1103,14 +1103,15 @@ async def get_room_sync_data( # sensible order again. bump_stamp = 0 - unstable_expanded_timeline = False prev_room_sync_config = previous_connection_state.room_configs.get(room_id) - # Record the `room_sync_config` if we're `ignore_timeline_bound` (which means - # that the `timeline_limit` has increased) + room_sync_required_state_map_to_persist = room_sync_config.required_state_map if changed_required_state_map: room_sync_required_state_map_to_persist = changed_required_state_map + # Record the `room_sync_config` if we're `ignore_timeline_bound` (which means + # that the `timeline_limit` has increased) + unstable_expanded_timeline = False if ignore_timeline_bound: # FIXME: We signal the fact that we're sending down more events to # the client by setting `unstable_expanded_timeline` to true (see From c23d32602512f38569b75170e3ab97aae6b3a2d8 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 9 Oct 2024 16:55:29 +0100 Subject: [PATCH 12/14] Clarify None comment --- synapse/handlers/sliding_sync/__init__.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index e24d79630c..ed1c1e5666 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1298,8 +1298,9 @@ def _required_state_changes( only want to re-send that entry down sync if it has changed. Returns: - A 2-tuple of updated required state config and the state filter to use - to fetch extra current state that we need to return. + A 2-tuple of updated required state config (or None if there is no update) + and the state filter to use to fetch extra current state that we need to + return. """ prev_required_state_map = previous_room_config.required_state_map From 6391b0f6b57d1f1aef414b3e97721c3ec72dd355 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 9 Oct 2024 16:57:36 +0100 Subject: [PATCH 13/14] LAZY only applies to memberships --- synapse/handlers/sliding_sync/__init__.py | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index ed1c1e5666..d27f416ce5 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1370,8 +1370,11 @@ def _required_state_changes( if state_key == StateValues.ME: added.append((event_type, user_id)) elif state_key == StateValues.LAZY: - # We handle lazy loading separately (outside this function), so - # don't need to explicitly add anything here. + # We handle lazy loading separately (outside this function), + # so don't need to explicitly add anything here. + # + # LAZY values should also be ignore for event types that are + # not membership. pass else: added.append((event_type, state_key)) @@ -1414,14 +1417,15 @@ def _required_state_changes( changes[event_type] = request_state_keys continue - old_state_key_lazy = StateValues.LAZY in old_state_keys - request_state_key_lazy = StateValues.LAZY in request_state_keys + if event_type == EventTypes.Member: + old_state_key_lazy = StateValues.LAZY in old_state_keys + request_state_key_lazy = StateValues.LAZY in request_state_keys - if old_state_key_lazy != request_state_key_lazy: - # If a "$LAZY" has been added or removed we always update the effective room - # required state config to match the request. - changes[event_type] = request_state_keys - continue + if old_state_key_lazy != request_state_key_lazy: + # If a "$LAZY" has been added or removed we always update the effective room + # required state config to match the request. + changes[event_type] = request_state_keys + continue # Handle "$ME" values by adding "$ME" if the state key matches the user # ID. From f8feef5efb7677464e3652a3f85d60e1ea381004 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 9 Oct 2024 15:19:09 -0500 Subject: [PATCH 14/14] Move `prev_room_sync_config` up --- synapse/handlers/sliding_sync/__init__.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index d27f416ce5..548f90f521 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -513,6 +513,8 @@ async def get_room_sync_data( state_reset_out_of_room = True + prev_room_sync_config = previous_connection_state.room_configs.get(room_id) + # Determine whether we should limit the timeline to the token range. # # We should return historical messages (before token range) in the @@ -561,7 +563,6 @@ async def get_room_sync_data( log_kv({"sliding_sync.room_status": room_status}) - prev_room_sync_config = previous_connection_state.room_configs.get(room_id) if prev_room_sync_config is not None: # Check if the timeline limit has increased, if so ignore the # timeline bound and record the change (see "XXX: Odd behavior" @@ -1103,8 +1104,6 @@ async def get_room_sync_data( # sensible order again. bump_stamp = 0 - prev_room_sync_config = previous_connection_state.room_configs.get(room_id) - room_sync_required_state_map_to_persist = room_sync_config.required_state_map if changed_required_state_map: room_sync_required_state_map_to_persist = changed_required_state_map