From f691c9bdc95483e952c4ff240e0cb05afed6a0cc Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 9 Oct 2024 12:55:34 -0500 Subject: [PATCH] Fix up and add more tests --- synapse/handlers/sliding_sync/__init__.py | 38 ++++++++++------ tests/handlers/test_sliding_sync.py | 55 +++++++++-------------- 2 files changed, 44 insertions(+), 49 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index ab1ad2ba6a..c75101b0ae 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -1432,12 +1432,6 @@ def _required_state_changes( request_state_keys = request_required_state_map.get(event_type, set()) changed_state_keys = changed_types_to_state_keys.get(event_type, set()) - invalidated_state_keys = ( - old_state_keys - - request_state_keys - - {StateValues.WILDCARD, StateValues.LAZY} - ) & changed_state_keys - if old_state_keys == request_state_keys: # No change to this type continue @@ -1446,9 +1440,20 @@ def _required_state_changes( # Nothing *added*, so we skip. Removals happen below. continue + # 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 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_state_keys = ( + old_state_keys - request_state_keys + ) & changed_state_keys + # Always update changes to include the newly added keys changes[event_type] = request_state_keys | ( - old_state_keys - invalidated_state_keys + # Wildcard and lazy state keys are not sticky from previous requests + (old_state_keys - {StateValues.WILDCARD, StateValues.LAZY}) + - invalidated_state_keys ) if StateValues.WILDCARD in old_state_keys: @@ -1482,16 +1487,19 @@ def _required_state_changes( old_state_keys = prev_required_state_map.get(event_type, set()) request_state_keys = request_required_state_map.get(event_type, set()) - invalidated_state_keys = ( - old_state_keys - - request_state_keys - - {StateValues.WILDCARD, StateValues.LAZY} - ) & changed_state_keys - if old_state_keys == request_state_keys: # No change. continue + # 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 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_state_keys = ( + old_state_keys - request_state_keys + ) & changed_state_keys + 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. @@ -1501,7 +1509,9 @@ def _required_state_changes( # that's a sufficient edge case that we can ignore (as its only a # performance optimization). changes[event_type] = request_state_keys | ( - old_state_keys - invalidated_state_keys + # Wildcard and lazy state keys are not sticky from previous requests + (old_state_keys - {StateValues.WILDCARD, StateValues.LAZY}) + - invalidated_state_keys ) continue diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index ba93eacc37..ecdf1ad6ee 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -3835,24 +3835,22 @@ class RequiredStateChangesTestCase(unittest.TestCase): request_required_state_map={}, state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"}, expected_with_state_deltas=( - # TODO - { - EventTypes.Member: { - "@user3:test", - } - }, + # Remove `EventTypes.Member` since there's been a change to that + # state, (persist the change to required state). That way next + # time, they request `EventTypes.Member`, 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=( - # TODO - { - EventTypes.Member: { - "@user2:test", - "@user3:test", - } - }, + # `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(), @@ -4012,28 +4010,20 @@ class RequiredStateChangesTestCase(unittest.TestCase): # 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}}, + {"type1": {StateValues.WILDCARD, "state_key2", "state_key3"}}, StateFilter.from_types([("type1", None)]), ), - # expected_with_state_deltas=( - # {"type1": {StateValues.WILDCARD, "state_key2", "state_key3"}}, - # StateFilter.from_types([("type1", None)]), - # ), expected_without_state_deltas=( - {"type1": {StateValues.WILDCARD}}, + { + "type1": { + StateValues.WILDCARD, + "state_key1", + "state_key2", + "state_key3", + } + }, StateFilter.from_types([("type1", None)]), ), - # expected_without_state_deltas=( - # { - # "type1": { - # StateValues.WILDCARD, - # "state_key1", - # "state_key2", - # "state_key3", - # } - # }, - # StateFilter.from_types([("type1", None)]), - # ), ), ), ] @@ -4057,11 +4047,6 @@ class RequiredStateChangesTestCase(unittest.TestCase): test_parameters.expected_without_state_deltas[0], "changed_required_state_map does not match (without state_deltas)", ) - logger.info("asdf actual added_state_filter: %s", added_state_filter) - logger.info( - "asdf expected added_state_filter: %s", - test_parameters.expected_with_state_deltas[1], - ) self.assertEqual( added_state_filter, test_parameters.expected_without_state_deltas[1],