Fix sending heroes in SSS when m.room.name="" (#19468)

As per the spec, a room with m.room.name value that is absent, null or
empty should be treated as if there is no m.room.name event at all:
https://spec.matrix.org/v1.17/client-server-api/#mroomname

This fetches the full m.room.name event and checks the content.name
instead of only checking the existence of the m.room.name event. This
results in correctly sending heroes for those rooms.

Fixes: https://github.com/element-hq/synapse/issues/19447

Signed-off-by: Joe Groocock <me@frebib.net>

changelog.d/19468.bugfix

@@ -0,0 +1 @@
+Fix a bug in [MSC4186: Simplified Sliding Sync](https://github.com/matrix-org/matrix-spec-proposals/pull/4186) that could prevent user avatars from showing if the room had an empty name.

synapse/handlers/sliding_sync/__init__.py

@@ -872,20 +872,10 @@ class SlidingSyncHandler:
         # For incremental syncs, we can do this first to determine if something relevant
         # has changed and strategically avoid fetching other costly things.
         room_state_delta_id_map: MutableStateMap[str] = {}
-        name_event_id: str | None = None
         membership_changed = False
         name_changed = False
         avatar_changed = False
-        if initial:
-            # Check whether the room has a name set
-            name_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=StateFilter.from_types([(EventTypes.Name, "")]),
-                to_token=to_token,
-            )
-            name_event_id = name_state_ids.get((EventTypes.Name, ""))
-        else:
+        if not initial:
             assert from_bound is not None
 
             # TODO: Limit the number of state events we're about to send down
@@ -933,6 +923,27 @@ class SlidingSyncHandler:
                 ):
                     avatar_changed = True
 
+        # If a room has an m.room.name event with an absent, null, or empty
+        # name field, it should be treated the same as a room with no
+        # m.room.name event.
+        # https://spec.matrix.org/v1.17/client-server-api/#mroomname
+        #
+        # TODO: Should we also check for `EventTypes.CanonicalAlias`
+        # (`m.room.canonical_alias`) as a fallback for the room name? see
+        # https://github.com/matrix-org/matrix-spec-proposals/pull/4186/changes#r2860107511
+        room_name: str | None = None
+        if initial or name_changed:
+            # Check whether the room has a name set
+            name_states = await self.get_current_state_at(
+                room_id=room_id,
+                room_membership_for_user_at_to_token=room_membership_for_user_at_to_token,
+                state_filter=StateFilter.from_types([(EventTypes.Name, "")]),
+                to_token=to_token,
+            )
+            name_event = name_states.get((EventTypes.Name, ""))
+            if name_event is not None:
+                room_name = name_event.content.get("name")
+
         # We only need the room summary for calculating heroes, however if we do
         # fetch it then we can use it to calculate `joined_count` and
         # `invited_count`.
@@ -949,12 +960,13 @@ class SlidingSyncHandler:
         hero_user_ids: list[str] = []
         # TODO: Should we also check for `EventTypes.CanonicalAlias`
         # (`m.room.canonical_alias`) as a fallback for the room name? see
-        # https://github.com/matrix-org/matrix-spec-proposals/pull/3575#discussion_r1671260153
+        # https://github.com/matrix-org/matrix-spec-proposals/pull/4186/changes#r2860107511
         #
-        # We need to fetch the `heroes` if the room name is not set. But we only need to
-        # get them on initial syncs (or the first time we send down the room) or if the
+        # We need to fetch the `heroes` if the room name is not set (taking
+        # care to treat an empty string as unset). But we only need to get them
+        # on initial syncs (or the first time we send down the room) or if the
         # membership has changed which may change the heroes.
-        if name_event_id is None and (initial or (not initial and membership_changed)):
+        if not room_name and (initial or (not initial and membership_changed)):
             # We need the room summary to extract the heroes from
             if room_membership_for_user_at_to_token.membership != Membership.JOIN:
                 # TODO: Figure out how to get the membership summary for left/banned rooms
@@ -1332,15 +1344,6 @@ class SlidingSyncHandler:
         if required_state_filter != StateFilter.none():
             required_room_state = required_state_filter.filter_state(room_state)
 
-        # Find the room name and avatar from the state
-        room_name: str | None = None
-        # TODO: Should we also check for `EventTypes.CanonicalAlias`
-        # (`m.room.canonical_alias`) as a fallback for the room name? see
-        # https://github.com/matrix-org/matrix-spec-proposals/pull/3575#discussion_r1671260153
-        name_event = room_state.get((EventTypes.Name, ""))
-        if name_event is not None:
-            room_name = name_event.content.get("name")
-
         room_avatar: str | None = None
         avatar_event = room_state.get((EventTypes.RoomAvatar, ""))
         if avatar_event is not None:

tests/rest/client/sliding_sync/test_rooms_meta.py

@@ -631,6 +631,76 @@ class SlidingSyncRoomsMetaTestCase(SlidingSyncBase):
         # We didn't request any state so we shouldn't see any `required_state`
         self.assertIsNone(response_body["rooms"][room_id1].get("required_state"))
 
+    def test_rooms_meta_heroes_empty_room_name(self) -> None:
+        """
+        Test that the `rooms` `heroes` are included when the room name is an
+        empty string (i.e. unset as per the spec)
+        """
+        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")
+        user3_id = self.register_user("user3", "pass")
+        _user3_tok = self.login(user3_id, "pass")
+
+        room_id = self.helper.create_room_as(
+            user2_id,
+            tok=user2_tok,
+            extra_content={
+                # https://spec.matrix.org/v1.17/client-server-api/#mroomname
+                # > If a room has an m.room.name event with an absent, null, or
+                # > empty name field, it should be treated the same as a room
+                # > with no m.room.name event.
+                "name": "",
+            },
+        )
+        self.helper.join(room_id, user1_id, tok=user1_tok)
+        # User3 is invited
+        self.helper.invite(room_id, src=user2_id, targ=user3_id, tok=user2_tok)
+
+        # Make the Sliding Sync request
+        sync_body = {
+            "lists": {
+                "foo-list": {
+                    "ranges": [[0, 1]],
+                    "required_state": [],
+                    "timeline_limit": 1,
+                }
+            }
+        }
+        response_body, from_token = self.do_sync(sync_body, tok=user1_tok)
+
+        # Room has an empty name so we should see `heroes` populated
+        self.assertEqual(response_body["rooms"][room_id]["initial"], True)
+        self.assertIsNone(response_body["rooms"][room_id].get("name"))
+        self.assertCountEqual(
+            [
+                hero["user_id"]
+                for hero in response_body["rooms"][room_id].get("heroes", [])
+            ],
+            # Heroes shouldn't include the user themselves (we shouldn't see user1)
+            [user2_id, user3_id],
+        )
+        self.assertEqual(
+            response_body["rooms"][room_id]["joined_count"],
+            2,
+        )
+        self.assertEqual(
+            response_body["rooms"][room_id]["invited_count"],
+            1,
+        )
+
+        # We didn't request any state so we shouldn't see any `required_state`
+        self.assertIsNone(response_body["rooms"][room_id].get("required_state"))
+
+        # Send a message to make the room come down sync
+        self.helper.send(room_id, "message in room", tok=user2_tok)
+
+        # Incremental sync
+        incremental_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok)
+        self.assertNotIn("name", incremental_body["rooms"][room_id])
+        self.assertNotIn("heroes", incremental_body["rooms"][room_id])
+
     def test_rooms_meta_heroes_when_banned(self) -> None:
         """
         Test that the `rooms` `heroes` are included in the response when the room