From 33e936371cd5c207fd88aeb96991e2073a52aa86 Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Fri, 24 Jul 2026 16:49:15 +0300 Subject: [PATCH] Send down removals in sliding sync response when fields get deleted --- synapse/api/constants.py | 16 ++++++ synapse/handlers/profile.py | 5 ++ synapse/handlers/sliding_sync/extensions.py | 55 ++++++++++++++----- synapse/handlers/sync.py | 2 + synapse/storage/databases/main/profile.py | 27 ++++++--- tests/handlers/test_profile.py | 2 + .../sliding_sync/test_extension_profiles.py | 55 +++++++++++++++++-- 7 files changed, 135 insertions(+), 27 deletions(-) diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 041a7f284a..83bdbc33d3 100644 --- a/synapse/api/constants.py +++ b/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): """ diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 5f2cec2366..98efa02952 100644 --- a/synapse/handlers/profile.py +++ b/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") diff --git a/synapse/handlers/sliding_sync/extensions.py b/synapse/handlers/sliding_sync/extensions.py index 516a54a19c..95765e0dc2 100644 --- a/synapse/handlers/sliding_sync/extensions.py +++ b/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, diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index 443f3fb422..92ac31eb1c 100644 --- a/synapse/handlers/sync.py +++ b/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 diff --git a/synapse/storage/databases/main/profile.py b/synapse/storage/databases/main/profile.py index 95f132bb5e..ee20dab5d5 100644 --- a/synapse/storage/databases/main/profile.py +++ b/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 diff --git a/tests/handlers/test_profile.py b/tests/handlers/test_profile.py index 767ab8b574..616c7cae12 100644 --- a/tests/handlers/test_profile.py +++ b/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, diff --git a/tests/rest/client/sliding_sync/test_extension_profiles.py b/tests/rest/client/sliding_sync/test_extension_profiles.py index 58871dfb3b..bb53312e2a 100644 --- a/tests/rest/client/sliding_sync/test_extension_profiles.py +++ b/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->->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: