kaslo/synapse
1
0
Fork 0
Code Wiki Activity

Switch fixup order to fix edge case with newly_left rooms

Browse Source 
Previously, we added back newly_left rooms and our second
fixup was removing them. We can just switch the order
of the fixups to solve this.
This commit is contained in:
Eric Eastwood
2024-06-04 18:29:14 -05:00
parent d3ce27b957
commit dfee21a1f4
2 changed files with 90 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files
synapse/handlers/sliding_sync.py
+44 -42
View File
@@ -480,9 +480,9 @@ class SlidingSyncHandler:
# tokens, we need to revert/rewind some membership changes to match the point in
# time of the `to_token`. In particular, we need to make these fixups:
#
# - 1) Add back newly_left rooms (> `from_token` and <= `to_token`)
# - 2a) Remove rooms that the user joined after the `to_token`
# - 2b) Add back rooms that the user left after the `to_token`
# - 1a) Remove rooms that the user joined after the `to_token`
# - 1b) Add back rooms that the user left after the `to_token`
# - 2) Add back newly_left rooms (> `from_token` and <= `to_token`)
#
# Below, we're doing two separate lookups for membership changes. We could
# request everything for both fixups in one range, [`from_token.room_key`,
@@ -491,43 +491,9 @@ class SlidingSyncHandler:
# `event.internal_metadata` to include `instance_name` but it might turn out a
# little difficult and a bigger, broader Synapse change than we want to make.
# 1) -----------------------------------------------------
# 1) Fetch membership changes that fall in the range from `from_token` up to `to_token`
membership_change_events_in_from_to_range = []
if from_token:
membership_change_events_in_from_to_range = (
await self.store.get_membership_changes_for_user(
user_id,
from_key=from_token.room_key,
to_key=to_token.room_key,
excluded_rooms=self.rooms_to_exclude_globally,
)
)
# 1) Assemble a list of the last membership events in some given ranges. Someone
# could have left and joined multiple times during the given range but we only
# care about end-result so we grab the last one.
last_membership_change_by_room_id_in_from_to_range: Dict[str, EventBase] = {}
for event in membership_change_events_in_from_to_range:
assert event.internal_metadata.stream_ordering
last_membership_change_by_room_id_in_from_to_range[event.room_id] = event
# 1) Fixup
for (
last_membership_change_in_from_to_range
) in last_membership_change_by_room_id_in_from_to_range.values():
room_id = last_membership_change_in_from_to_range.room_id
# 1) Add back newly_left rooms (> `from_token` and <= `to_token`). We
# include newly_left rooms because the last event that the user should see
# is their own leave event
if last_membership_change_in_from_to_range.membership == Membership.LEAVE:
sync_room_id_set.add(room_id)
# 2) -----------------------------------------------------
# 2) Fetch membership changes that fall in the range from `to_token` up to
# 1) Fetch membership changes that fall in the range from `to_token` up to
# `membership_snapshot_token`
membership_change_events_after_to_token = (
await self.store.get_membership_changes_for_user(
@@ -538,7 +504,7 @@ class SlidingSyncHandler:
)
)
# 2) Assemble a list of the last membership events in some given ranges. Someone
# 1) Assemble a list of the last membership events in some given ranges. Someone
# could have left and joined multiple times during the given range but we only
# care about end-result so we grab the last one.
last_membership_change_by_room_id_after_to_token: Dict[str, EventBase] = {}
@@ -554,7 +520,7 @@ class SlidingSyncHandler:
event.room_id, event
)
# 2) Fixup
# 1) Fixup
for (
last_membership_change_after_to_token
) in last_membership_change_by_room_id_after_to_token.values():
@@ -611,7 +577,7 @@ class SlidingSyncHandler:
sender=last_membership_change_after_to_token.sender,
)
# 2a) Add back rooms that the user left after the `to_token`
# 1a) Add back rooms that the user left after the `to_token`
#
# For example, if the last membership event after the `to_token` is a leave
# event, then the room was excluded from `sync_room_id_set` when we first
@@ -622,7 +588,7 @@ class SlidingSyncHandler:
and should_prev_membership_be_included
):
sync_room_id_set.add(room_id)
# 2b) Remove rooms that the user joined (hasn't left) after the `to_token`
# 1b) Remove rooms that the user joined (hasn't left) after the `to_token`
#
# For example, if the last membership event after the `to_token` is a "join"
# event, then the room was included `sync_room_id_set` when we first crafted
@@ -634,4 +600,40 @@ class SlidingSyncHandler:
):
sync_room_id_set.discard(room_id)
# 2) -----------------------------------------------------
# We fix-up newly_left rooms after the first fixup because it may have removed
# some left rooms that we can figure out our newly_left in the following code
# 2) Fetch membership changes that fall in the range from `from_token` up to `to_token`
membership_change_events_in_from_to_range = []
if from_token:
membership_change_events_in_from_to_range = (
await self.store.get_membership_changes_for_user(
user_id,
from_key=from_token.room_key,
to_key=to_token.room_key,
excluded_rooms=self.rooms_to_exclude_globally,
)
)
# 2) Assemble a list of the last membership events in some given ranges. Someone
# could have left and joined multiple times during the given range but we only
# care about end-result so we grab the last one.
last_membership_change_by_room_id_in_from_to_range: Dict[str, EventBase] = {}
for event in membership_change_events_in_from_to_range:
assert event.internal_metadata.stream_ordering
last_membership_change_by_room_id_in_from_to_range[event.room_id] = event
# 2) Fixup
for (
last_membership_change_in_from_to_range
) in last_membership_change_by_room_id_in_from_to_range.values():
room_id = last_membership_change_in_from_to_range.room_id
# 2) Add back newly_left rooms (> `from_token` and <= `to_token`). We
# include newly_left rooms because the last event that the user should see
# is their own leave event
if last_membership_change_in_from_to_range.membership == Membership.LEAVE:
sync_room_id_set.add(room_id)
return sync_room_id_set
tests/handlers/test_sliding_sync.py
+46 -10
View File
@@ -305,7 +305,7 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase):
def test_only_newly_left_rooms_show_up(self) -> None:
"""
Test that newly_left rooms still show up in the sync response but rooms that
were left before the `from_token` don't show up. See condition "1)" comments in
were left before the `from_token` don't show up. See condition "2)" comments in
the `get_sync_room_ids_for_user` method.
"""
user1_id = self.register_user("user1", "pass")
@@ -336,7 +336,7 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase):
def test_no_joins_after_to_token(self) -> None:
"""
Rooms we join after the `to_token` should *not* show up. See condition "2b)"
Rooms we join after the `to_token` should *not* show up. See condition "1b)"
comments in the `get_sync_room_ids_for_user()` method.
"""
user1_id = self.register_user("user1", "pass")
@@ -365,7 +365,7 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase):
def test_join_during_range_and_left_room_after_to_token(self) -> None:
"""
Room still shows up if we left the room but were joined during the
from_token/to_token. See condition "2a)" comments in the
from_token/to_token. See condition "1a)" comments in the
`get_sync_room_ids_for_user()` method.
"""
user1_id = self.register_user("user1", "pass")
@@ -395,7 +395,7 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase):
def test_join_before_range_and_left_room_after_to_token(self) -> None:
"""
Room still shows up if we left the room but were joined before the `from_token`
so it should show up. See condition "2a)" comments in the
so it should show up. See condition "1a)" comments in the
`get_sync_room_ids_for_user()` method.
"""
user1_id = self.register_user("user1", "pass")
@@ -422,7 +422,7 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase):
def test_kicked_before_range_and_left_after_to_token(self) -> None:
"""
Room still shows up if we left the room but were kicked before the `from_token`
so it should show up. See condition "2a)" comments in the
so it should show up. See condition "1a)" comments in the
`get_sync_room_ids_for_user()` method.
"""
user1_id = self.register_user("user1", "pass")
@@ -470,8 +470,8 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase):
def test_newly_left_during_range_and_join_leave_after_to_token(self) -> None:
"""
Newly left room should show up. But we're also testing that joining and leaving
after the `to_token` doesn't mess with the results. See condition "2a)" comments
in the `get_sync_room_ids_for_user()` method.
after the `to_token` doesn't mess with the results. See condition "2)" and "1a)"
comments in the `get_sync_room_ids_for_user()` method.
"""
user1_id = self.register_user("user1", "pass")
user1_tok = self.login(user1_id, "pass")
@@ -504,10 +504,46 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase):
# Room should still show up because it's newly_left during the from/to range
self.assertEqual(room_id_results, {room_id1})
def test_newly_left_during_range_and_join_after_to_token(self) -> None:
"""
Newly left room should show up. But we're also testing that joining after the
`to_token` doesn't mess with the results. See condition "2)" and "1b)" comments
in the `get_sync_room_ids_for_user()` method.
"""
user1_id = self.register_user("user1", "pass")
user1_tok = self.login(user1_id, "pass")
user2_id = self.register_user("user2", "pass")
user2_tok = self.login(user2_id, "pass")
before_room1_token = self.event_sources.get_current_token()
# We create the room with user2 so the room isn't left with no members when we
# leave and can still re-join.
room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=True)
# Join and leave the room during the from/to range
self.helper.join(room_id1, user1_id, tok=user1_tok)
self.helper.leave(room_id1, user1_id, tok=user1_tok)
after_room1_token = self.event_sources.get_current_token()
# Join the room after we already have our tokens
self.helper.join(room_id1, user1_id, tok=user1_tok)
room_id_results = self.get_success(
self.sliding_sync_handler.get_sync_room_ids_for_user(
UserID.from_string(user1_id),
from_token=before_room1_token,
to_token=after_room1_token,
)
)
# Room should still show up because it's newly_left during the from/to range
self.assertEqual(room_id_results, {room_id1})
def test_leave_before_range_and_join_leave_after_to_token(self) -> None:
"""
Old left room shouldn't show up. But we're also testing that joining and leaving
after the `to_token` doesn't mess with the results. See condition "2a)" comments
after the `to_token` doesn't mess with the results. See condition "1a)" comments
in the `get_sync_room_ids_for_user()` method.
"""
user1_id = self.register_user("user1", "pass")
@@ -542,7 +578,7 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase):
def test_leave_before_range_and_join_after_to_token(self) -> None:
"""
Old left room shouldn't show up. But we're also testing that joining after the
`to_token` doesn't mess with the results. See condition "2b)" comments in the
`to_token` doesn't mess with the results. See condition "1b)" comments in the
`get_sync_room_ids_for_user()` method.
"""
user1_id = self.register_user("user1", "pass")
@@ -660,7 +696,7 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase):
) -> None:
"""
Make it look like we joined after the token range but we were invited before the
from/to range so the room should still show up. See condition "2a)" comments in
from/to range so the room should still show up. See condition "1a)" comments in
the `get_sync_room_ids_for_user()` method.
"""
user1_id = self.register_user("user1", "pass")