diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index b4b01c9257..99d79a6ea4 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -2329,12 +2329,21 @@ class SyncHandler: # Process field updates and users who have events in the sync response if users: - user_fields: dict[str, set[str]] = {} + updated_user_fields: dict[str, set[str]] = {} # Set fields from updates for update in updates: - if not update.field_name or update.user_id not in users: + # Skip the update if there is no field update (a joined or left room + # action), the client didn't ask for this field, or we're not + # interested in this user. + if ( + not update.field_name + or update.field_name not in profile_fields + or update.user_id not in users + ): continue - user_fields.setdefault(update.user_id, set()).add(update.field_name) + updated_user_fields.setdefault(update.user_id, set()).add( + update.field_name + ) # Note: there's a small race condition here where a profile update may # occur between fetching `now_token` above and reaching this step. In @@ -2357,10 +2366,11 @@ class SyncHandler: per_user_updates: dict[str, JsonValue | dict[str, JsonValue]] = {} if include_users and other_user_id in include_users: - # Include the full profile as this user has events in - # a lazy loaded sync response, except for fields we've recently - # sent in a previous lazy loaded sync response - for field_name in profile_data.keys(): + # Include all the fields the client asked for, as this user + # has events in a lazy loaded sync response, except for + # fields we've recently sent in a previous lazy loaded sync response + fields = set(profile_data.keys()).intersection(profile_fields) + for field_name in fields: cache_key = ( sync_config.user.to_string(), sync_config.device_id, @@ -2388,12 +2398,15 @@ class SyncHandler: cast(str, profile_data.get(field_name)), ) else: - # Include only the diff, unless the user recently joined - # We don't use a cache here as changes are always sent + # Include only the diff, unless the user recently joined, + # then send all the fields the client asked for. + # We don't use a cache here as for non-lazy sync we always + # send changes and/or fields the client asked for, if relevant + # as above joined condition. fields = ( - list(profile_data.keys()) + profile_fields if other_user_id in joined_room_user_ids - else user_fields.get(other_user_id, []) + else set(updated_user_fields.get(other_user_id, [])) ) for field_name in fields: per_user_updates[field_name] = profile_data.get(field_name) diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index 86f7b43166..bb230cdcf6 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -1239,8 +1239,8 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): @override_config({"include_profile_updates_in_sync": True}) def test_initial_sync_responds_with_tracked_profile_updates(self) -> None: """Test that with MSC4429 enabled the initial sync response does - contain profile updates for users who share rooms, including our - syncing user.""" + contain profile updates for users who share rooms, for the fields the + client requests. This response should include our syncing user.""" self.get_success( self.profile_handler.set_field( target_user=UserID.from_string(self.other_user), @@ -1249,6 +1249,15 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): new_value={"text": "On holiday", "emoji": "🏖"}, ) ) + # Also set a field the client doesn't want + self.get_success( + self.profile_handler.set_field( + target_user=UserID.from_string(self.other_user), + requester=create_requester(self.other_user), + field_name="displayname", + new_value="New displayname", + ) + ) requester = create_requester(self.user) initial_result = self.get_success( @@ -1259,9 +1268,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): filter_collection=FilterCollection( hs=self.hs, filter_json={ - "org.matrix.msc4429.profile_fields": { - "ids": ["m.status", "displayname", "avatar_url"] - } + "org.matrix.msc4429.profile_fields": {"ids": ["m.status"]} }, ), ), @@ -1274,6 +1281,9 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): initial_result.profile_updates["@other_user:test"]["m.status"], {"text": "On holiday", "emoji": "🏖"}, ) + self.assertFalse( + "displayname" in initial_result.profile_updates["@other_user:test"].keys(), + ) self.assertCountEqual( initial_result.profile_updates.keys(), [ @@ -1409,11 +1419,11 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): ) @override_config({"include_profile_updates_in_sync": True}) - def test_incremental_sync_sends_down_profile_updates( + def test_incremental_sync_sends_down_profile_update_diffs( self, ) -> None: """Test that with MSC4429 enabled the incremental sync response does - contain profile updates.""" + contain profile update diffs.""" requester = create_requester(self.user) initial_result = self.get_success( self.sync_handler.wait_for_sync_for_user( @@ -1440,6 +1450,15 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): new_value={"text": "On holiday", "emoji": "🏖"}, ) ) + # Set a field the client didn't ask for + self.get_success( + self.profile_handler.set_field( + target_user=UserID.from_string(self.other_user), + requester=create_requester(self.other_user), + field_name="uninterestingfield", + new_value="Content", + ) + ) incremental_result = self.get_success( self.sync_handler.wait_for_sync_for_user( requester, @@ -1464,8 +1483,14 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): {"text": "On holiday", "emoji": "🏖"}, ) # We only send diffs in incremental sync for profile field updates - self.assertIsNone( - incremental_result.profile_updates["@other_user:test"].get("displayname"), + self.assertFalse( + "displayname" + in incremental_result.profile_updates["@other_user:test"].keys(), + ) + # The client didn't ask for this field + self.assertFalse( + "uninterestingfield" + in incremental_result.profile_updates["@other_user:test"].keys(), ) @override_config({"include_profile_updates_in_sync": True}) @@ -1494,7 +1519,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): hs=self.hs, filter_json={ "org.matrix.msc4429.profile_fields": { - "ids": ["m.status", "displayname", "avatar_url"] + "ids": ["m.status", "displayname"] } }, ), @@ -1524,6 +1549,14 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): self.helper.send_messages( room_id=self.joined_room, num_events=10, tok=third_tok ) + self.get_success( + self.profile_handler.set_field( + target_user=UserID.from_string(third_user), + requester=create_requester(third_user), + field_name="uninterestingfield", + new_value="Content", + ) + ) # Join a federated user to the room self.get_success( inject_member_event( @@ -1543,7 +1576,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): hs=self.hs, filter_json={ "org.matrix.msc4429.profile_fields": { - "ids": ["m.status", "displayname", "avatar_url"] + "ids": ["m.status", "displayname"] }, "room": { "state": { @@ -1562,6 +1595,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): self.assertFalse( "@federateduser:federatedhs" in incremental_result.profile_updates.keys() ) + # Lazy loading only filters initial sync profile updates. Incremental syncs # should include all tracked profile updates for the syncing user. self.assertCountEqual( @@ -1572,22 +1606,31 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): ], ) assert incremental_result.profile_updates["@other_user:test"] is not None + # This is a field update, so should be here self.assertEqual( incremental_result.profile_updates["@other_user:test"]["m.status"], {"text": "On holiday", "emoji": "🏖"}, ) + # We don't have events for this user in this response, so their full profile # is not included - self.assertIsNone( - incremental_result.profile_updates["@other_user:test"].get("displayname"), + self.assertFalse( + "displayname" + in incremental_result.profile_updates["@other_user:test"].keys(), ) assert incremental_result.profile_updates["@third_user:test"] is not None - # This user has events in the timeline, thus their full profile is included + + # This user has events in the timeline, thus the fields the client asked for + # are included self.assertEqual( incremental_result.profile_updates["@third_user:test"]["m.status"], {"text": "On fire", "emoji": "🔥"}, ) + self.assertFalse( + "uninterestingfield" + in incremental_result.profile_updates["@third_user:test"].keys(), + ) self.assertEqual( incremental_result.profile_updates["@third_user:test"]["displayname"], "third_user", @@ -1595,16 +1638,19 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): @parameterized.expand( [ - True, - False, + [True, True], + [False, False], + [True, False], + [False, True], ] ) @override_config({"include_profile_updates_in_sync": True}) - def test_lazy_loading_sync_filters_out_profile_updates_from_federated_users( + def test_sync_filters_out_profile_updates_from_federated_users( self, is_initial: bool, + is_lazy: bool, ) -> None: - """Test that with MSC4429 enabled lazy loading sync response + """Test that with MSC4429 enabled any sync response doesn't contain federated users even if there are timeline events from them. """ # Join a federated user to the room, causing a membership event into @@ -1618,6 +1664,17 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): ) ) requester = create_requester(self.user) + filter_json: dict[str, dict] = { + "org.matrix.msc4429.profile_fields": { + "ids": ["m.status", "displayname", "avatar_url"] + }, + } + if is_lazy: + filter_json["room"] = { + "state": { + "lazy_load_members": True, + }, + } initial_result = self.get_success( self.sync_handler.wait_for_sync_for_user( requester, @@ -1625,16 +1682,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): user_id=self.user, filter_collection=FilterCollection( hs=self.hs, - filter_json={ - "org.matrix.msc4429.profile_fields": { - "ids": ["m.status", "displayname", "avatar_url"] - }, - "room": { - "state": { - "lazy_load_members": True, - }, - }, - }, + filter_json=filter_json, ), ), request_key=generate_request_key(), @@ -1664,16 +1712,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): user_id=self.user, filter_collection=FilterCollection( hs=self.hs, - filter_json={ - "org.matrix.msc4429.profile_fields": { - "ids": ["m.status", "displayname", "avatar_url"] - }, - "room": { - "state": { - "lazy_load_members": True, - }, - }, - }, + filter_json=filter_json, ), ), request_key=generate_request_key(), @@ -1688,13 +1727,21 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): ) @override_config({"include_profile_updates_in_sync": True}) - def test_incremental_sync_lazy_loading_cache_filters_recently_sent_profiles( + def test_incremental_sync_lazy_loading_cache_filters_recently_sent_profiles_and_fields( self, ) -> None: """Test that with MSC4429 enabled the incremental sync lazy loading response - filters out unchanged profiles we have recently sent to the client. + filters out unchanged profiles or fields we have recently sent to the client. """ requester = create_requester(self.user) + self.get_success( + self.profile_handler.set_field( + target_user=UserID.from_string(self.other_user), + requester=create_requester(self.other_user), + field_name="sooninterestingfield", + new_value="Content", + ) + ) initial_result = self.get_success( self.sync_handler.wait_for_sync_for_user( requester, @@ -1747,9 +1794,14 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): "@other_user:test", ], ) + assert incremental_result.profile_updates["@other_user:test"] is not None + self.assertEqual( + incremental_result.profile_updates["@other_user:test"].keys(), + ["displayname", "avatar_url"], + ) # If we have more events from the other_user, and do another lazy sync, - # we don't expect the full profile to be sent again due to our cache + # we don't expect the full profile to be sent again due to our cache. self.helper.send_messages( room_id=self.joined_room, num_events=1, tok=self.other_tok ) @@ -1780,6 +1832,50 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): incremental_result.profile_updates.keys(), [], ) + # However, if we again add an event, we do expect any fields the client didn't + # previously ask for to be there. + self.helper.send_messages( + room_id=self.joined_room, num_events=1, tok=self.other_tok + ) + incremental_result = self.get_success( + self.sync_handler.wait_for_sync_for_user( + requester, + since_token=incremental_result.next_batch, + sync_config=generate_sync_config( + user_id=self.user, + filter_collection=FilterCollection( + hs=self.hs, + filter_json={ + "org.matrix.msc4429.profile_fields": { + "ids": [ + "m.status", + "displayname", + "avatar_url", + "sooninterestingfield", + ] + }, + "room": { + "state": { + "lazy_load_members": True, + }, + }, + }, + ), + ), + request_key=generate_request_key(), + ) + ) + self.assertCountEqual( + incremental_result.profile_updates.keys(), + [ + "@other_user:test", + ], + ) + assert incremental_result.profile_updates["@other_user:test"] is not None + self.assertEqual( + incremental_result.profile_updates["@other_user:test"].keys(), + ["sooninterestingfield"], + ) @override_config({"include_profile_updates_in_sync": True}) def test_incremental_sync_sends_down_null_profile_if_user_no_longer_sharing_rooms( @@ -1832,11 +1928,12 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): ) @override_config({"include_profile_updates_in_sync": True}) - def test_incremental_sync_sends_down_full_profile_for_users_who_have_joined( + def test_incremental_sync_sends_down_all_requested_fields_for_users_who_have_joined( self, ) -> None: """Test that with MSC4429 enabled the incremental sync response - includes the full profile of a user who has joined a room with the syncing user. + includes all the requested fields of a user who has joined a room with the + syncing user. """ requester = create_requester(self.user) initial_result = self.get_success( @@ -1848,7 +1945,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): hs=self.hs, filter_json={ "org.matrix.msc4429.profile_fields": { - "ids": ["m.status", "displayname", "avatar_url"] + "ids": ["displayname", "avatar_url"] }, }, ), @@ -1866,7 +1963,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): hs=self.hs, filter_json={ "org.matrix.msc4429.profile_fields": { - "ids": ["m.status", "displayname", "avatar_url"] + "ids": ["displayname", "avatar_url"] }, }, ), @@ -1882,7 +1979,15 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): user=third_user, tok=third_tok, ) - + # Set a status field we don't except to see in sync + self.get_success( + self.profile_handler.set_field( + target_user=UserID.from_string(third_user), + requester=create_requester(third_user), + field_name="m.status", + new_value={"text": "On fire", "emoji": "🔥"}, + ) + ) incremental_result = self.get_success( self.sync_handler.wait_for_sync_for_user( requester, @@ -1893,7 +1998,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): hs=self.hs, filter_json={ "org.matrix.msc4429.profile_fields": { - "ids": ["m.status", "displayname", "avatar_url"] + "ids": ["displayname", "avatar_url"] }, }, ), @@ -1913,6 +2018,9 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): self.assertIsNone( incremental_result.profile_updates["@third_user:test"]["avatar_url"], ) + self.assertFalse( + "m.status" in incremental_result.profile_updates["@third_user:test"].keys(), + ) @parameterized.expand( [ @@ -1926,9 +2034,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): ones own profile updates.""" requester = create_requester(self.user) filter_json: dict[str, dict] = { - "org.matrix.msc4429.profile_fields": { - "ids": ["m.status", "displayname", "avatar_url"] - } + "org.matrix.msc4429.profile_fields": {"ids": ["m.status", "avatar_url"]} } if is_lazy: filter_json["room"] = { @@ -1976,6 +2082,10 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): incremental_result.profile_updates["@user:test"]["m.status"], {"text": "On holiday", "emoji": "🏖"}, ) + # We didn't ask for displayname + self.assertFalse( + "displayname" in incremental_result.profile_updates["@user:test"].keys(), + ) class SyncStateAfterTestCase(tests.unittest.HomeserverTestCase):