diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 0c5792dbe1..8fc884ad1c 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -134,54 +134,10 @@ def copy_and_replace(self, **kwds: Any) -> "_RoomMembershipForUser": return attr.evolve(self, **kwds) -@attr.s(slots=True, frozen=True, auto_attribs=True) -class _SortedRoomMembershipForUser(_RoomMembershipForUser): - """ - Same as `_RoomMembershipForUser` but with an additional `bump_stamp` attribute. - - Attributes: - bump_stamp: The `stream_ordering` of the last event according to the - `bump_event_types`. This helps clients sort more readily without them needing to - pull in a bunch of the timeline to determine the last activity. - `bump_event_types` is a thing because for example, we don't want display name - changes to mark the room as unread and bump it to the top. For encrypted rooms, - we just have to consider any activity as a bump because we can't see the content - and the client has to figure it out for themselves. - """ - - bump_stamp: int - - @classmethod - def from_room_membership_for_user( - cls, - rooms_membership_for_user: _RoomMembershipForUser, - *, - bump_stamp: int, - ) -> "_SortedRoomMembershipForUser": - """ - Copy from the given `_RoomMembershipForUser`, sprinkle on the specific - `_SortedRoomMembershipForUser` fields to create a new - `_SortedRoomMembershipForUser`. - """ - - return cls( - room_id=rooms_membership_for_user.room_id, - event_id=rooms_membership_for_user.event_id, - event_pos=rooms_membership_for_user.event_pos, - membership=rooms_membership_for_user.membership, - sender=rooms_membership_for_user.sender, - newly_joined=rooms_membership_for_user.newly_joined, - bump_stamp=bump_stamp, - ) - - def copy_and_replace(self, **kwds: Any) -> "_SortedRoomMembershipForUser": - return attr.evolve(self, **kwds) - - @attr.s(slots=True, frozen=True, auto_attribs=True) class _RelevantRoomEntry: room_sync_config: RoomSyncConfig - room_membership_for_user: _SortedRoomMembershipForUser + room_membership_for_user: _RoomMembershipForUser class SlidingSyncHandler: @@ -847,7 +803,7 @@ async def sort_rooms( self, sync_room_map: Dict[str, _RoomMembershipForUser], to_token: StreamToken, - ) -> List[_SortedRoomMembershipForUser]: + ) -> List[_RoomMembershipForUser]: """ Sort by `stream_ordering` of the last event that the user should see in the room. `stream_ordering` is unique so we get a stable sort. @@ -861,31 +817,27 @@ async def sort_rooms( A sorted list of room IDs by `stream_ordering` along with membership information. """ - sorted_sync_rooms: List[_SortedRoomMembershipForUser] = [] - # Assemble a map of room ID to the `stream_ordering` of the last activity that the # user should see in the room (<= `to_token`) + last_activity_in_room_map: Dict[str, int] = {} for room_id, room_for_user in sync_room_map.items(): # If they are fully-joined to the room, let's find the latest activity # at/before the `to_token`. if room_for_user.membership == Membership.JOIN: last_event_result = ( await self.store.get_last_event_pos_in_room_before_stream_ordering( - room_id, to_token.room_key, event_types=DEFAULT_BUMP_EVENT_TYPES + room_id, to_token.room_key ) ) - # By default, just choose the membership event position - event_pos = room_for_user.event_pos - # But if we found a bump event, use that instead - if last_event_result is not None: - _, event_pos = last_event_result + # If the room has no events at/before the `to_token`, this is probably a + # mistake in the code that generates the `sync_room_map` since that should + # only give us rooms that the user had membership in during the token range. + assert last_event_result is not None - sorted_sync_rooms.append( - _SortedRoomMembershipForUser.from_room_membership_for_user( - room_for_user, bump_stamp=event_pos.stream - ) - ) + _, event_pos = last_event_result + + last_activity_in_room_map[room_id] = event_pos.stream else: # Otherwise, if the user has left/been invited/knocked/been banned from # a room, they shouldn't see anything past that point. @@ -894,16 +846,12 @@ async def sort_rooms( # invited/knocked cases if for example the room has # `invite`/`world_readable` history visibility, see # https://github.com/matrix-org/matrix-spec-proposals/pull/3575#discussion_r1653045932 - sorted_sync_rooms.append( - _SortedRoomMembershipForUser.from_room_membership_for_user( - room_for_user, bump_stamp=room_for_user.event_pos.stream - ) - ) + last_activity_in_room_map[room_id] = room_for_user.event_pos.stream return sorted( - sorted_sync_rooms, + sync_room_map.values(), # Sort by the last activity (stream_ordering) in the room - key=lambda room_info: room_info.bump_stamp, + key=lambda room_info: last_activity_in_room_map[room_info.room_id], # We want descending order reverse=True, ) @@ -913,7 +861,7 @@ async def get_room_sync_data( user: UserID, room_id: str, room_sync_config: RoomSyncConfig, - room_membership_for_user_at_to_token: _SortedRoomMembershipForUser, + room_membership_for_user_at_to_token: _RoomMembershipForUser, from_token: Optional[StreamToken], to_token: StreamToken, ) -> SlidingSyncResult.RoomResult: @@ -1102,6 +1050,20 @@ async def get_room_sync_data( # state reset happened. Perhaps we should indicate this by setting `initial: # True` and empty `required_state`. + # Figure out the last bump event in the room + last_bump_event_result = ( + await self.store.get_last_event_pos_in_room_before_stream_ordering( + room_id, to_token.room_key, event_types=DEFAULT_BUMP_EVENT_TYPES + ) + ) + + # By default, just choose the membership event position + bump_stamp = room_membership_for_user_at_to_token.event_pos.stream + # But if we found a bump event, use that instead + if last_bump_event_result: + _, bump_event_pos = last_bump_event_result + bump_stamp = bump_event_pos.stream + return SlidingSyncResult.RoomResult( # TODO: Dummy value name=None, @@ -1123,7 +1085,7 @@ async def get_room_sync_data( stripped_state=stripped_state, prev_batch=prev_batch_token, limited=limited, - bump_stamp=room_membership_for_user_at_to_token.bump_stamp, + bump_stamp=bump_stamp, # TODO: Dummy values joined_count=0, invited_count=0, diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 636bd32369..de59d614ed 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -2417,7 +2417,8 @@ def test_activity_after_xxx(self, room1_membership: str) -> None: def test_default_bump_event_types(self) -> None: """ - Test that we only consider `bump_event_types` when sorting rooms. + Test that we only consider the *latest* event in the room when sorting (not + `bump_event_types`). """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -2433,8 +2434,8 @@ def test_default_bump_event_types(self) -> None: ) self.helper.send(room_id2, "message in room2", tok=user1_tok) - # Send a reaction in room1 but it shouldn't affect the sort order - # because reactions are not part of the `DEFAULT_BUMP_EVENT_TYPES` + # Send a reaction in room1 which isn't in `DEFAULT_BUMP_EVENT_TYPES` but we only + # care about sorting by the *latest* event in the room. self.helper.send_event( room_id1, type=EventTypes.Reaction, @@ -2469,6 +2470,7 @@ def test_default_bump_event_types(self) -> None: self.assertEqual( [room_membership.room_id for room_membership in sorted_sync_rooms], - # room2 sorts before room1 because reactions don't bump the room - [room_id2, room_id1], + # room1 sorts before room2 because it has the latest event (the reaction). + # We only care about the *latest* event in the room. + [room_id1, room_id2], ) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index f9e4c0c718..07c6ed1570 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -2010,27 +2010,29 @@ def test_rooms_bump_stamp(self) -> None: { "op": "SYNC", "range": [0, 1], - # room2 sorts before room1 because reactions don't bump the room - "room_ids": [room_id2, room_id1], + # room1 sorts before room2 because it has the latest event (the + # reaction) + "room_ids": [room_id1, room_id2], } ], channel.json_body["lists"]["foo-list"], ) - # Make sure the `bump_stamp` for room2 is correct - self.assertEqual( - channel.json_body["rooms"][room_id2]["bump_stamp"], - event_pos2.stream, - channel.json_body["rooms"][room_id2], - ) - - # Make sure the `bump_stamp` for room2 is correct + # The `bump_stamp` for room1 should point at the latest message (not the + # reaction since it's not one of the `DEFAULT_BUMP_EVENT_TYPES`) self.assertEqual( channel.json_body["rooms"][room_id1]["bump_stamp"], event_pos1.stream, channel.json_body["rooms"][room_id1], ) + # The `bump_stamp` for room2 should point at the latest message + self.assertEqual( + channel.json_body["rooms"][room_id2]["bump_stamp"], + event_pos2.stream, + channel.json_body["rooms"][room_id2], + ) + def test_rooms_newly_joined_incremental_sync(self) -> None: """ Test that when we make an incremental sync with a `newly_joined` `rooms`, we are