diff --git a/changelog.d/20145.misc b/changelog.d/20145.misc new file mode 100644 index 00000000000..e2ce9d52db0 --- /dev/null +++ b/changelog.d/20145.misc @@ -0,0 +1 @@ +Stop treating unset display names and avatar URLs as profile fields with a `null` value. \ No newline at end of file diff --git a/synapse/handlers/sliding_sync/extensions.py b/synapse/handlers/sliding_sync/extensions.py index 7ee26079ed4..8e89870c3f0 100644 --- a/synapse/handlers/sliding_sync/extensions.py +++ b/synapse/handlers/sliding_sync/extensions.py @@ -29,7 +29,6 @@ AccountDataTypes, EduTypes, EventTypes, - ProfileFields, ProfileUpdateAction, StickyEvent, ) @@ -1421,19 +1420,8 @@ async def get_profiles_extension_response( per_user_updates: dict[str, JsonValue | dict[str, JsonValue]] = {} per_user_removals: set[str] = set() for field_name in user_fields: - # For custom fields the lack of a field means it will be `Absent`, - # for displayname/avatar_url it will be `None`, due to way we store - # things differently. - # FIXME: I intend to simplify this by pushing the special-case logic - # for these 'original' profile fields into the storage layer instead. - absent_type = ( - Absent - if field_name - not in (ProfileFields.DISPLAYNAME, ProfileFields.AVATAR_URL) - else None - ) field_value: JsonValue | dict[str, JsonValue] | AbsentType = ( - profile_data.get(field_name, absent_type) + profile_data.get(field_name, Absent) ) if ( # If the field isn't found on the profile and it is present in @@ -1442,7 +1430,7 @@ async def get_profiles_extension_response( # are `None` by default, for example each and every user created # by Synapse will have `avatar_url: None`, and we don't want to # constantly send that to the clients. - field_value is absent_type and field_name in updated_fields + field_value is Absent and field_name in updated_fields ): per_user_removals.add(field_name) else: diff --git a/synapse/storage/databases/main/profile.py b/synapse/storage/databases/main/profile.py index d5e35fa7cae..7f552a68e5b 100644 --- a/synapse/storage/databases/main/profile.py +++ b/synapse/storage/databases/main/profile.py @@ -20,7 +20,7 @@ # import json from collections.abc import Set -from typing import TYPE_CHECKING, Collection, cast +from typing import TYPE_CHECKING, Collection, Iterable, cast import attr from canonicaljson import encode_canonical_json @@ -636,12 +636,19 @@ async def get_profile_data_for_users( user_ids: List of user IDs to filter against. Returns: - Dictionary of displayname/avatar_url/custom fields for a list of users. + Dictionary from user_id -> field name -> field value + for the requested users. + + This includes `displayname`, `avatar_url` and all custom fields. + For `displayname` and `avatar_url`, when they are stored as NULL + in the database column, the dictionary entry will be omitted. """ if not user_ids: return {} - rows = await self.db_pool.simple_select_many_batch( + rows: Iterable[ + tuple[str, str | None, str | None, str | JsonDict | None] + ] = await self.db_pool.simple_select_many_batch( table="profiles", column="full_user_id", iterable=user_ids, @@ -651,15 +658,16 @@ async def get_profile_data_for_users( results: dict[str, dict[str, JsonValue | dict[str, JsonValue]]] = {} for full_user_id, displayname, avatar_url, fields in rows: - user_fields = fields or {} - # The SQLite driver doesn't have a JSON datatype. - if isinstance(self.database_engine, Sqlite3Engine) and fields: - user_fields = json.loads(fields) - base_fields = { - ProfileFields.DISPLAYNAME: displayname, - ProfileFields.AVATAR_URL: avatar_url, - } - user_fields.update(base_fields) + user_fields = db_to_json(fields or {}) + + # When the displayname and avatar URL aren't set, + # they are stored as NULL in the database. + # To make them behave the same as custom fields, + # when they are NULL, we treat them as not being set at all. + if displayname is not None: + user_fields[ProfileFields.DISPLAYNAME] = displayname + if avatar_url is not None: + user_fields[ProfileFields.AVATAR_URL] = avatar_url results[full_user_id] = user_fields diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index 5771e605338..206b2863b5c 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -26,7 +26,7 @@ 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 @@ -1175,6 +1175,13 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.other_user = self.register_user("other_user", "password") self.other_tok = self.login("other_user", "password") self.joined_room = self.helper.create_room_as(self.user, tok=self.tok) + self.get_success( + self.store.set_profile_field( + UserID.from_string(self.user), + ProfileFields.AVATAR_URL, + "mxc://example.invalid/abcdef", + ) + ) self.get_success( self.store.set_profile_field( user_id=UserID.from_string(self.user), @@ -1978,8 +1985,11 @@ def test_incremental_sync_lazy_loading_cache_filters_recently_sent_profiles_and_ ) assert incremental_result.profile_updates["@other_user:test"] is not None self.assertEqual( - set(incremental_result.profile_updates["@other_user:test"].keys()), - {"avatar_url", "displayname"}, + incremental_result.profile_updates["@other_user:test"], + { + "displayname": "other_user", + # avatar_url unset (user doesn't have one) + }, ) # If we have more events from the other_user, and do another lazy sync, @@ -2161,7 +2171,7 @@ def test_incremental_sync_sends_down_all_requested_fields_for_users_who_have_joi user=third_user, tok=third_tok, ) - # Set a status field we don't except to see in sync + # Set a status field we don't expect to see in sync self.get_success( self.profile_handler.set_field( target_user=UserID.from_string(third_user), @@ -2194,14 +2204,12 @@ def test_incremental_sync_sends_down_all_requested_fields_for_users_who_have_joi [third_user], ) self.assertEqual( - incremental_result.profile_updates["@third_user:test"]["displayname"], - "third_user", - ) - self.assertIsNone( - incremental_result.profile_updates["@third_user:test"]["avatar_url"], - ) - self.assertFalse( - "m.status" in incremental_result.profile_updates["@third_user:test"].keys(), + incremental_result.profile_updates["@third_user:test"], + { + "displayname": "third_user", + # avatar_url unset (user doesn't have one) + # m.status unset (not requested in sync) + }, ) @parameterized.expand( @@ -2261,12 +2269,12 @@ def test_incremental_sync_includes_own_profile_updates(self, is_lazy: bool) -> N ) assert incremental_result.profile_updates["@user:test"] is not None self.assertEqual( - incremental_result.profile_updates["@user:test"]["m.status"], - {"text": "On holiday", "emoji": "🏖"}, - ) - # We didn't ask for displayname - self.assertFalse( - "displayname" in incremental_result.profile_updates["@user:test"].keys(), + incremental_result.profile_updates["@user:test"], + { + "m.status": {"text": "On holiday", "emoji": "🏖"}, + # avatar_url unset (user doesn't have one) + # displayname unset (we didn't request it in sync) + }, ) @parameterized.expand([[True, False], [True, True], [False, False], [False, True]]) diff --git a/tests/rest/client/sliding_sync/test_extension_profiles.py b/tests/rest/client/sliding_sync/test_extension_profiles.py index 40d426fb875..5c2d17547cf 100644 --- a/tests/rest/client/sliding_sync/test_extension_profiles.py +++ b/tests/rest/client/sliding_sync/test_extension_profiles.py @@ -480,7 +480,6 @@ def test_all_fields_returned_if_no_fields_specified(self, is_initial: bool) -> N ], { "updated": { - "avatar_url": None, "displayname": "other_user", "field": "value", } @@ -582,8 +581,6 @@ def test_profile_returned_if_user_left_then_rejoined(self) -> None: { "updated": { "displayname": "other_user", - # FIXME: This shouldn't be returned, but currently is - "avatar_url": None, } }, ) @@ -625,7 +622,6 @@ def test_all_fields_returned_in_incremental_non_lazy_sync_if_someone_joined( expectation = { "updated": { - "avatar_url": None, "displayname": "third_user", } }