Stop treating unset display names and avatar URLs as profile fields with a null value.

changelog.d/20145.misc

@@ -0,0 +1 @@
+Stop treating unset display names and avatar URLs as profile fields with a `null` value.

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:

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
 

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]])

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",
             }
         }