diff --git a/changelog.d/20147.bugfix b/changelog.d/20147.bugfix new file mode 100644 index 0000000000..74768dc63e --- /dev/null +++ b/changelog.d/20147.bugfix @@ -0,0 +1 @@ +Fix sending custom profile field removals to legacy sync clients when the field is deleted using the profile field delete endpoint. \ No newline at end of file diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index 943105415a..f8a766feee 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -2420,8 +2420,22 @@ class SyncHandler: if include_users and other_user_id in include_users: # 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) + # fields we've recently sent in a previous lazy loaded sync response. + # We must include _updated_ fields even if the profile doesn't have + # this field. The value will be sent down as `None`. We must do + # this as currently legacy sync delivers field removals by + # delivering a null value to clients, and if a field is completely + # deleted, we can't otherwise do that. The fact this field has + # a `ProfileUpdateAction.UPDATE` is enough to tell us it should + # be sent down. + # TODO once removals are sent down in a dedicated key instead of + # null values, the `.union(updated_user_fields.get(other_user_id, []))` + # part here can be removed. + fields = ( + set(profile_data.keys()) + .union(updated_user_fields.get(other_user_id, [])) + .intersection(profile_fields) + ) for field_name in fields: cache_key = ( sync_config.user.to_string(), @@ -2469,9 +2483,24 @@ class SyncHandler: if other_user_id in joined_room_user_ids else set(updated_user_fields.get(other_user_id, [])) ) - fields = set(profile_data.keys()).intersection(fields) + # We must include _updated_ fields even if the profile doesn't have + # this field. The value will be sent down as `None`. We must do + # this as currently legacy sync delivers field removals by + # delivering a null value to clients, and if a field is completely + # deleted, we can't otherwise do that. The fact this field has + # a `ProfileUpdateAction.UPDATE` is enough to tell us it should + # be sent down. + # TODO once removals are sent down in a dedicated key instead of + # null values, the `.union(updated_user_fields.get(other_user_id, []))` + # part here can be removed. + fields = ( + set(profile_data.keys()) + .union(updated_user_fields.get(other_user_id, [])) + .intersection(fields) + ) + # fields.update(set(updated_user_fields.get(other_user_id, []))) for field_name in fields: - per_user_updates[field_name] = profile_data[field_name] + per_user_updates[field_name] = profile_data.get(field_name) if per_user_updates: profile_updates[other_user_id] = per_user_updates diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index 5771e60533..b732c501d9 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -26,7 +26,7 @@ from parameterized import parameterized, parameterized_class from twisted.internet import defer from twisted.internet.testing import MemoryReactor -from synapse.api.constants import AccountDataTypes, EventTypes, JoinRules +from synapse.api.constants import AccountDataTypes, EventTypes, JoinRules, ProfileFields from synapse.api.errors import Codes, ResourceLimitError from synapse.api.filtering import FilterCollection, Filtering from synapse.api.room_versions import RoomVersion, RoomVersions @@ -2109,6 +2109,104 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): incremental_result.profile_updates["@other_user:test"], ) + @parameterized.expand( + [ + True, + False, + ] + ) + @override_config({"include_profile_updates_in_sync": True}) + def test_incremental_sync_sends_down_deleted_fields(self, is_lazy: bool) -> None: + """ + Tests that, with `include_profile_updates_in_sync` enabled, + an incremental sync returns deleted fields as a `null` value, both for + `displayname` (stored as its own column) and for + generic custom fields (stored as JSON). + """ + # Set up a user with `displayname` and `m.status` profile fields + requester = create_requester(self.user) + other_requester = create_requester(self.other_user) + other_user = UserID.from_string(self.other_user) + filter_json: dict = { + "org.matrix.msc4429.profile_fields": {"ids": ["displayname", "m.status"]}, + } + if is_lazy: + filter_json["room"] = { + "state": { + "lazy_load_members": True, + }, + } + sync_config = generate_sync_config( + user_id=self.user, + filter_collection=FilterCollection( + hs=self.hs, + filter_json=filter_json, + ), + ) + self.get_success( + self.profile_handler.set_field( + target_user=other_user, + requester=other_requester, + field_name=ProfileFields.DISPLAYNAME, + new_value="Bob", + ) + ) + self.get_success( + self.profile_handler.set_field( + target_user=other_user, + requester=other_requester, + field_name="m.status", + new_value={"text": "On holiday"}, + ) + ) + + # Do an initial sync after the point of those fields being set + initial_result = self.get_success( + self.sync_handler.wait_for_sync_for_user( + requester, + sync_config=sync_config, + request_key=generate_request_key(), + ) + ) + self.assertEqual( + initial_result.profile_updates["@other_user:test"], + {"displayname": "Bob", "m.status": {"text": "On holiday"}}, + ) + + # Delete the displayname and the `m.status` profile fields + self.get_success( + self.profile_handler.set_field( + target_user=other_user, + requester=other_requester, + field_name=ProfileFields.DISPLAYNAME, + new_value="", + ) + ) + self.get_success( + self.profile_handler.delete_profile_field( + target_user=other_user, + requester=other_requester, + field_name="m.status", + ) + ) + + # Do an incremental sync. + # Expect the deletion of both fields to be communicated in it. + incremental_result = self.get_success( + self.sync_handler.wait_for_sync_for_user( + requester, + since_token=initial_result.next_batch, + sync_config=sync_config, + request_key=generate_request_key(), + ) + ) + self.assertEqual( + incremental_result.profile_updates, + # We currently represent deleted fields as `null`, even though + # it's ambiguous (TODO MSC change) + {"@other_user:test": {"displayname": None, "m.status": None}}, + ) + @override_config({"include_profile_updates_in_sync": True}) def test_incremental_sync_sends_down_all_requested_fields_for_users_who_have_joined( self,