Send down removals in sliding sync response when fields get deleted

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

@@ -1195,27 +1195,36 @@ class SlidingSyncExtensionHandler:
             field_names=fields,
             field_names_empty_means_all_fields=False if fields else True,
         )
-        profile_user_ids = set()
-        left_room_user_ids = {
+
+        profile_user_ids = {
             update.user_id
             for update in updates
-            if update.action == ProfileUpdateAction.LEFT_ROOM.value
+            if update.action == ProfileUpdateAction.UPDATE.value
         }
+
+        # Add any newly joined users to our list of users to get updates for later
         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 = {
+        # Collect users who left rooms
+        left_room_user_ids = {
             update.user_id
             for update in updates
-            if update.action == ProfileUpdateAction.UPDATE.value
+            if update.action == ProfileUpdateAction.LEFT_ROOM.value
         }
-        # Add users with updates
-        profile_user_ids.update(updated_users)
+
+        # Collect deletes
+        deletes: list[tuple[str, str]] = [
+            # For typing checks, cast field_name to str, since the schema has
+            # `str | None` which is not true for `action: DELETE`
+            (update.user_id, cast(str, update.field_name))
+            for update in updates
+            if update.action == ProfileUpdateAction.DELETE.value
+        ]
 
         updated_user_fields: dict[str, set[str]] = {}
         # Set fields from updates
@@ -1232,7 +1241,8 @@ class SlidingSyncExtensionHandler:
             updated_user_fields.setdefault(update.user_id, set()).add(update.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[0] for delete in deletes}),
         )
 
         # TODO lazy loading
@@ -1296,10 +1306,29 @@ class SlidingSyncExtensionHandler:
                 }
 
         # 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
+        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 profile_user_id, field_name in deletes:
+            profile_data = profile_data_by_user.get(profile_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[profile_user_id] = None
+                continue
+            if field_name in profile_data.keys():
+                # This field has re-appeared to the profile, skip
+                continue
+            if not response.get(profile_user_id):
+                response[profile_user_id] = {"removed": []}
+            # Ensure we only add the field once
+            # FIXME: ignore typing for now as the response type is being a pain
+            if field_name in response[profile_user_id]["removed"]:  # type: ignore
+                continue
+            # FIXME: ignore typing for now as the response type is being a pain
+            response[profile_user_id]["removed"].append(field_name)  # type: ignore
 
         return SlidingSyncResult.Extensions.ProfilesExtension(
             users=response,

synapse/handlers/sync.py

@@ -2480,6 +2480,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

@@ -387,6 +387,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,
         *,
@@ -496,6 +497,12 @@ class ProfileWorkerStore(SQLBaseStore):
                 field_clause, field_args = make_in_list_sql_clause(
                     txn.database_engine, "pu.field_name", field_names
                 )
+            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:
@@ -518,7 +525,7 @@ class ProfileWorkerStore(SQLBaseStore):
                 WHERE ? < pu.stream_id AND pu.stream_id <= ?
                     AND puf.user_id = ?
                     {user_clause}
-                    AND ({field_clause} OR pu.action != ?)
+                    AND ({field_clause} OR {action_clause})
                 ORDER BY pu.stream_id ASC
                 """,
                 (
@@ -527,7 +534,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())
@@ -759,6 +766,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,
@@ -820,9 +828,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
@@ -835,7 +844,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
@@ -875,8 +884,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
@@ -1004,7 +1013,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

@@ -317,6 +317,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

@@ -294,11 +294,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(
@@ -679,6 +683,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: