From 4245bdc6f048e8682751fd8092666184bb4033fb Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Thu, 27 Aug 2026 09:29:50 +0300 Subject: [PATCH] Fix sending down removed custom profile fields in legacy sync (#20147) When a client (correctly) calls the `DELETE` endpoint to remove a custom profile field (like Element X does with `m.status`), we incorrectly don't include it in the sync response in legacy sync. This was due to the fact that we cleaned up the sent fields down to what fields the profile currently has. Always ensure any fields in `ProfileUpdateAction.UPDATE` are sent down, as `null` values for profile fields which have been deleted. Fixes an issue where clearing a user status from Element X does not reflect in the user status being cleared on Element Web. Note, target is the v1.160.0 release branch due to customer commitments, and this fixes web and mobile clients not working together correctly. ### Pull Request Checklist * [ ] Pull request is based on the develop branch * [x] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). The entry should: - Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from `EventStore` to `EventWorkerStore`.". - Use markdown where necessary, mostly for `code blocks`. - End with either a period (.) or an exclamation mark (!). - Start with a capital letter. - Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry. * [x] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters)) --- changelog.d/20147.bugfix | 1 + synapse/handlers/sync.py | 37 +++++++++++-- tests/handlers/test_sync.py | 100 +++++++++++++++++++++++++++++++++++- 3 files changed, 133 insertions(+), 5 deletions(-) create mode 100644 changelog.d/20147.bugfix 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,