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

<!-- Please read
https://element-hq.github.io/synapse/latest/development/contributing_guide.html
before submitting your pull request -->

* [ ] 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

@@ -0,0 +1 @@
+Fix sending custom profile field removals to legacy sync clients when the field is deleted using the profile field delete endpoint.

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

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,