diff --git a/changelog.d/20145.bugfix b/changelog.d/20145.bugfix new file mode 100644 index 0000000000..e2ce9d52db --- /dev/null +++ b/changelog.d/20145.bugfix @@ -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 7ee26079ed..0e193cae36 100644 --- a/synapse/handlers/sliding_sync/extensions.py +++ b/synapse/handlers/sliding_sync/extensions.py @@ -29,7 +29,6 @@ from synapse.api.constants import ( AccountDataTypes, EduTypes, EventTypes, - ProfileFields, ProfileUpdateAction, StickyEvent, ) @@ -1421,28 +1420,13 @@ class SlidingSyncExtensionHandler: 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 # `updated_fields`, that means an existing field has been removed. - # We need the check against `updated_fields` as some profile fields - # 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 1a2dea7460..7f860258ee 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 @@ -644,12 +644,19 @@ class ProfileWorkerStore(SQLBaseStore): 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, @@ -659,15 +666,16 @@ class ProfileWorkerStore(SQLBaseStore): 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 e100943413..a74bd68df1 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -1175,6 +1175,13 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): 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 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): ) 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, @@ -2259,7 +2269,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): 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), @@ -2292,14 +2302,12 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): [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( @@ -2335,6 +2343,14 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): request_key=generate_request_key(), ) ) + # Sanity-check that initial sync includes the fields + self.assertEqual( + initial_result.profile_updates["@user:test"], + { + "m.status": {"text": "Swimming in the Great Lakes!", "emoji": "🏊"}, + "avatar_url": "mxc://example.invalid/abcdef", + }, + ) self.get_success( self.profile_handler.set_field( target_user=UserID.from_string(self.user), @@ -2359,12 +2375,12 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): ) 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 not included (it didn't change during this sync window) + # displayname not included (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 40d426fb87..5c2d17547c 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 @@ class SlidingSyncProfilesTestCase(SlidingSyncBase): ], { "updated": { - "avatar_url": None, "displayname": "other_user", "field": "value", } @@ -582,8 +581,6 @@ class SlidingSyncProfilesTestCase(SlidingSyncBase): { "updated": { "displayname": "other_user", - # FIXME: This shouldn't be returned, but currently is - "avatar_url": None, } }, ) @@ -625,7 +622,6 @@ class SlidingSyncProfilesTestCase(SlidingSyncBase): expectation = { "updated": { - "avatar_url": None, "displayname": "third_user", } }