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

Instead, treat them as absent fields as they feel like they should be.

The database implementation detail that these fields have a dedicated
column with `NULL`
when unset is kept to the storage layer.

The goal here is to reduce the amount of special casing needed for these
two original profile fields and treat them a little bit more like
regular profile fields.

Follows: #20003

Follows: #20147 (needed as a bugfix to continue sending them down
oldschool sync when they get deleted. Without #20147, this PR would
break that — which matches how custom profile fields were broken too.)

---------

Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>

changelog.d/20145.bugfix

@@ -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 @@ 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:

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
 

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

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