Send down removals in sliding sync response when fields get deleted

Various other minor tweaks.

synapse/api/constants.py

@@ -441,6 +441,7 @@ class ProfileUpdateAction(str, enum.Enum):
     normally includes. This update action currently has no meaning for sync responses
     that are not incremental and non-lazy.
     """
+
     LEFT_ROOM = "left_room"
     """
     This profile update row action represents a user leaving a room.
@@ -450,6 +451,7 @@ class ProfileUpdateAction(str, enum.Enum):
     profiles, so clients can clear their cache containing the users profile data
     they are no longer interested in.
     """
+
     UPDATE = "update"
     """
     This profile update row action represents a user updating a profile field.
@@ -460,6 +462,20 @@ class ProfileUpdateAction(str, enum.Enum):
     a change, but the client may still get fields that have not changed.
     """
 
+    # FIXME: add a test covering adding these to the streams tables
+    DELETE = "delete"
+    """
+    This profile update row action represents a user deleting a profile field.
+
+    In the sync response deleted fields are indicated separately from updated fields,
+    as `None` is a valid value for a field.
+
+    Note that even though deleting displaynames and avatar_url's is done by setting
+    them to an empty string, for consistency the profile update stream gets a DELETE
+    action written to it, as the profile updates sync MSC's don't special case
+    displayname or avatar_url.
+    """
+
 
 class StickyEventField(TypedDict):
     """

synapse/handlers/profile.py

@@ -767,6 +767,9 @@ class ProfileHandler:
     ) -> None:
         """Delete a field from a user's profile.
 
+        This should only be called for custom profile fields,
+        not displayname or avatar_url.
+
         Preconditions:
         - This must NOT be called as part of deactivating the user, because we will
           notify modules about the change whilst claiming it is not related
@@ -780,6 +783,8 @@ class ProfileHandler:
             field_name: The name of the profile field to remove.
             by_admin: Whether this change was made by an administrator.
         """
+        assert field_name not in (ProfileFields.DISPLAYNAME, ProfileFields.AVATAR_URL)
+
         if not self.hs.is_mine(target_user):
             raise SynapseError(400, "User is not hosted on this homeserver")
 

synapse/handlers/sliding_sync/extensions.py

@@ -1103,6 +1103,8 @@ class SlidingSyncExtensionHandler:
         into the timeline will be included, unless they would be included otherwise.
         For other rooms, all members of the room will be included as candidates.
 
+        Note, this does not collect user ID's from the profile updates stream.
+
         Args:
             user_id: The full user ID syncing.
             rooms: A set of rooms that was already calculated as relevant for this
@@ -1295,31 +1297,46 @@ class SlidingSyncExtensionHandler:
             field_names=fields,
             field_names_empty_means_all_fields=True,
         )
-        left_room_user_ids = {
-            update.user_id
-            for update in updates
-            if update.action == ProfileUpdateAction.LEFT_ROOM.value
-        }
+
+        # Add any newly joined users to our list of users
         joined_room_user_ids = {
             update.user_id
             for update in updates
             if update.action == ProfileUpdateAction.JOINED_ROOM.value
         }
-        # Add any newly joined users
         profile_user_ids.update(joined_room_user_ids)
 
-        updated_users = {
+        # Add any users from field updates
+        updated_user_ids = {
             update.user_id
             for update in updates
             if update.action == ProfileUpdateAction.UPDATE.value
         }
-        # Add users with updates
-        profile_user_ids.update(updated_users)
+        profile_user_ids.update(updated_user_ids)
+
+        # Collect users who left rooms
+        left_room_user_ids = {
+            update.user_id
+            for update in updates
+            if update.action == ProfileUpdateAction.LEFT_ROOM.value
+        }
+
+        # Collect users who deleted fields
+        delete_field_user_ids = {
+            update.user_id
+            for update in updates
+            if update.action == ProfileUpdateAction.DELETE.value
+        }
+
+        # Process left rooms
+        for other_user_id in left_room_user_ids:
+            # Return a null response to the client
+            response[other_user_id] = None
 
         updated_user_fields: dict[str, set[str]] = {}
         # Set fields from updates
         for update in updates:
-            if not update.affected_fields:
+            if not update.affected_fields or update.user_id in left_room_user_ids:
                 continue
             for field_name in update.affected_fields:
                 # Skip the update if the client didn't ask for this field, or we're not
@@ -1331,11 +1348,14 @@ class SlidingSyncExtensionHandler:
                 updated_user_fields.setdefault(update.user_id, set()).add(field_name)
 
         profile_data_by_user = await self.store.get_profile_data_for_users(
-            profile_user_ids
+            # Get profiles for both updates and deletes in one go
+            profile_user_ids.union(delete_field_user_ids),
         )
 
         # Serialise the profile updates into the sync response format.
         for profile_user_id in profile_user_ids:
+            if profile_user_id in left_room_user_ids:
+                continue
             profile_data = profile_data_by_user.get(profile_user_id)
             if profile_data is None:
                 # No profile data for this user, just return a blank dictionary
@@ -1386,11 +1406,35 @@ class SlidingSyncExtensionHandler:
                     "updated": per_user_updates,
                 }
 
-        # Process left rooms
-        if left_room_user_ids:
-            for other_user_id in left_room_user_ids:
-                # Return a null response to the client
-                response[other_user_id] = None
+        # Process deleted fields
+        for update in updates:
+            if (
+                update.action != ProfileUpdateAction.DELETE.value
+                or not update.affected_fields
+                or update.user_id in left_room_user_ids
+            ):
+                continue
+            profile_data = profile_data_by_user.get(update.user_id)
+            if not profile_data:
+                # No profile data for this user, just return a blank dictionary
+                # telling the clients to remove all profile information for this user.
+                response[update.user_id] = None
+                continue
+            for field_name in update.affected_fields:
+                if field_name in profile_data.keys():
+                    # This field has re-appeared to the profile, skip
+                    continue
+                if response.get(update.user_id) is None:
+                    response[update.user_id] = {"removed": []}
+
+                # Typing fix as mypy thinks this may be None
+                entry = cast(JsonDict, response[update.user_id])
+                removed = cast(list[str], entry.setdefault("removed", []))
+
+                # Ensure we only add the field once
+                if field_name in removed:
+                    continue
+                removed.append(field_name)
 
         return SlidingSyncResult.Extensions.ProfilesExtension(
             users=response,

synapse/handlers/sync.py

@@ -2482,6 +2482,8 @@ class SyncHandler:
                 # Return an empty dictionary to the client
                 profile_updates[other_user_id] = None
 
+        # FIXME: handle profile field deletions like we do for sliding sync
+
         if profile_updates:
             sync_result_builder.profile_updates = profile_updates
 

synapse/storage/databases/main/profile.py

@@ -403,6 +403,7 @@ class ProfileWorkerStore(SQLBaseStore):
             "get_updated_profile_updates", _get_updated_profile_updates_txn
         )
 
+    # FIXME this function should be deleted, it's not used.
     async def get_profile_updates_for_fields(
         self,
         *,
@@ -556,6 +557,12 @@ class ProfileWorkerStore(SQLBaseStore):
                     )
                 field_clause = f"(EXISTS (SELECT 1 FROM {all_field_names_table_expression} WHERE {wanted_field_in_elems_clause}))"
 
+            action_clause, action_args = make_in_list_sql_clause(
+                txn.database_engine,
+                "pu.action",
+                (ProfileUpdateAction.UPDATE.value, ProfileUpdateAction.DELETE.value),
+                negative=True,
+            )
             user_clause = ""
             user_args: list[str] = []
             if include_users is not None:
@@ -580,7 +587,7 @@ class ProfileWorkerStore(SQLBaseStore):
                     {user_clause}
                     AND (
                         {field_clause}
-                        OR pu.action != ?
+                        OR {action_clause}
                     )
                 ORDER BY pu.stream_id ASC
                 """,
@@ -590,7 +597,7 @@ class ProfileWorkerStore(SQLBaseStore):
                     user_id,
                     *user_args,
                     *field_args,
-                    ProfileUpdateAction.UPDATE.value,
+                    *action_args,
                 ),
             )
             rows = cast(list[tuple[int, str, str, str | None]], txn.fetchall())
@@ -834,6 +841,7 @@ class ProfileWorkerStore(SQLBaseStore):
             return None
 
         # Record updates in the profile updates stream
+        # FIXME: this should be DELETE if displayname/avatarurl are being emptied
         stream_id = self.record_profile_updates_txn(
             txn=txn,
             user_id=user_id,
@@ -895,9 +903,10 @@ class ProfileWorkerStore(SQLBaseStore):
         Args:
             txn: Transaction to use
             user_id: User ID that made the profile update
-            action: The profile update action, either `update`, `left_room` or
-                `joined_room`
-            field_names: A list of fields that were set, if ProfileUpdateAction.UPDATE
+            action: The profile update action, either `update`, `delete`, `left_room`
+                or `joined_room`.
+            field_names: A list of fields that were set, if
+                ProfileUpdateAction.UPDATE/DELETE
             user_rooms: Optionally, a set of rooms that the update concerns. If not
                 given, a database lookup will be done to fetch all the users rooms.
             target_users: Optionally, set of users to create profile update stream rows
@@ -910,7 +919,7 @@ class ProfileWorkerStore(SQLBaseStore):
         if not self._msc4429_enabled:
             return None
 
-        if action == ProfileUpdateAction.UPDATE:
+        if action in (ProfileUpdateAction.UPDATE, ProfileUpdateAction.DELETE):
             assert field_names
         else:
             assert not field_names
@@ -950,8 +959,8 @@ class ProfileWorkerStore(SQLBaseStore):
                 # No point writing an update for ourselves, if a membership change and no
                 # other users interested
                 return None
-        elif action == ProfileUpdateAction.UPDATE:
-            # Always include ourselves when updating field values
+        elif action in (ProfileUpdateAction.UPDATE, ProfileUpdateAction.DELETE):
+            # Always include ourselves when updating/deleting field values
             users.add(user_id.to_string())
 
         # Record the profile update
@@ -1050,7 +1059,7 @@ class ProfileWorkerStore(SQLBaseStore):
             stream_id = self.record_profile_updates_txn(
                 txn=txn,
                 user_id=user_id,
-                action=ProfileUpdateAction.UPDATE,
+                action=ProfileUpdateAction.DELETE,
                 field_names=[field_name],
             )
             return stream_id

tests/handlers/test_profile.py

@@ -320,6 +320,8 @@ class ProfileTestCase(unittest.HomeserverTestCase):
         )
 
         fields_updates = self.get_success(
+            # FIXME this function should be deleted, it's not used.
+            # Adapt this test to use the right one.
             self.store.get_profile_updates_for_fields(
                 from_id=1,
                 to_id=2,

tests/rest/client/sliding_sync/test_extension_profiles.py

@@ -13,7 +13,7 @@
 #
 import logging
 
-from parameterized import parameterized, parameterized_class
+from parameterized import parameterized
 
 from twisted.internet.testing import MemoryReactor
 
@@ -34,16 +34,16 @@ logger = logging.getLogger(__name__)
 # foreground update for
 # `sliding_sync_joined_rooms`/`sliding_sync_membership_snapshots` (tracked by
 # https://github.com/element-hq/synapse/issues/17623)
-@parameterized_class(
-    ("use_new_tables",),
-    [
-        (True,),
-        (False,),
-    ],
-    class_name_func=lambda cls,
-    num,
-    params_dict: f"{cls.__name__}_{'new' if params_dict['use_new_tables'] else 'fallback'}",
-)
+# @parameterized_class(
+#     ("use_new_tables",),
+#     [
+#         (True,),
+#         (False,),
+#     ],
+#     class_name_func=lambda cls,
+#     num,
+#     params_dict: f"{cls.__name__}_{'new' if params_dict['use_new_tables'] else 'fallback'}",
+# )
 class SlidingSyncProfilesTestCase(SlidingSyncBase):
     """Tests for the profile updates sliding sync extension"""
 
@@ -296,11 +296,15 @@ class SlidingSyncProfilesTestCase(SlidingSyncBase):
 
         # Make an incremental Sliding Sync request
         response_body, _ = self.do_sync(sync_body, since=from_token, tok=self.tok)
-
-        # FIXME: once field deletions come down in sync this should
-        # be checking for that
-        self.assertIsNone(
-            response_body["extensions"].get("org.matrix.msc4262.profiles")
+        self.assertEqual(
+            response_body["extensions"]["org.matrix.msc4262.profiles"]["users"][
+                "@other_user:test"
+            ],
+            {
+                "removed": [
+                    "field",
+                ],
+            },
         )
 
     @parameterized.expand(
@@ -681,6 +685,47 @@ class SlidingSyncProfilesTestCase(SlidingSyncBase):
         > Likewise, any field IDs that are cleared/removed from a user's profile will appear under users-><user_id>->removed.
         > Likewise, the removed field should not be present if there were only updates to existing fields (and none were cleared).
         """
+        self.get_success(
+            self.profile_handler.set_field(
+                target_user=UserID.from_string(self.other_user),
+                requester=create_requester(self.other_user),
+                field_name="field",
+                new_value="value",
+            )
+        )
+        # Make an initial Sliding Sync request with the profiles extension enabled
+        sync_body = {
+            "lists": {},
+            "extensions": {
+                "org.matrix.msc4262.profiles": {
+                    "enabled": True,
+                },
+            },
+        }
+        response_body, from_token = self.do_sync(sync_body, tok=self.tok)
+
+        # Delete the field
+        self.get_success(
+            self.profile_handler.delete_profile_field(
+                target_user=UserID.from_string(self.other_user),
+                requester=create_requester(self.other_user),
+                field_name="field",
+            )
+        )
+
+        # Make an incremental Sliding Sync request
+        response_body, _ = self.do_sync(sync_body, since=from_token, tok=self.tok)
+        # We should see the removed field
+        self.assertEqual(
+            response_body["extensions"]["org.matrix.msc4262.profiles"]["users"][
+                "@other_user:test"
+            ],
+            {
+                "removed": [
+                    "field",
+                ],
+            },
+        )
 
     @override_config({"include_profile_updates_in_sync": True})
     def test_updated_key_only_present_if_updates(self) -> None: