From befdfc78d67788dfb1ae9bce6385201d1e83ff15 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Thu, 2 Feb 2023 13:47:52 -0500
Subject: [PATCH 1/7] Split get_updated_account_data_for_user into separate
 global & room methods.

---
 synapse/handlers/account_data.py              | 10 +--
 synapse/handlers/sync.py                      | 14 ++--
 .../storage/databases/main/account_data.py    | 66 +++++++++++++------
 3 files changed, 61 insertions(+), 29 deletions(-)

diff --git a/synapse/handlers/account_data.py b/synapse/handlers/account_data.py
index 67e789eef70e..797de46dbc3b 100644
--- a/synapse/handlers/account_data.py
+++ b/synapse/handlers/account_data.py
@@ -343,10 +343,12 @@ async def get_new_events(
                 }
             )
 
-        (
-            account_data,
-            room_account_data,
-        ) = await self.store.get_updated_account_data_for_user(user_id, last_stream_id)
+        account_data = await self.store.get_updated_global_account_data_for_user(
+            user_id, last_stream_id
+        )
+        room_account_data = await self.store.get_updated_room_account_data_for_user(
+            user_id, last_stream_id
+        )
 
         for account_data_type, content in account_data.items():
             results.append({"type": account_data_type, "content": content})
diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py
index 3566537894e6..4973255146f8 100644
--- a/synapse/handlers/sync.py
+++ b/synapse/handlers/sync.py
@@ -1741,11 +1741,15 @@ async def _generate_sync_entry_for_account_data(
 
         if since_token and not sync_result_builder.full_state:
             # TODO Do not fetch room account data if it will be unused.
-            (
-                global_account_data,
-                account_data_by_room,
-            ) = await self.store.get_updated_account_data_for_user(
-                user_id, since_token.account_data_key
+            global_account_data = (
+                await self.store.get_updated_global_account_data_for_user(
+                    user_id, since_token.account_data_key
+                )
+            )
+            account_data_by_room = (
+                await self.store.get_updated_room_account_data_for_user(
+                    user_id, since_token.account_data_key
+                )
             )
 
             push_rules_changed = await self.store.have_push_rules_changed_for_user(
diff --git a/synapse/storage/databases/main/account_data.py b/synapse/storage/databases/main/account_data.py
index 8a359d7eb89c..b6db78948d34 100644
--- a/synapse/storage/databases/main/account_data.py
+++ b/synapse/storage/databases/main/account_data.py
@@ -342,36 +342,61 @@ def get_updated_room_account_data_txn(
             "get_updated_room_account_data", get_updated_room_account_data_txn
         )
 
-    async def get_updated_account_data_for_user(
+    async def get_updated_global_account_data_for_user(
         self, user_id: str, stream_id: int
-    ) -> Tuple[Dict[str, JsonDict], Dict[str, Dict[str, JsonDict]]]:
-        """Get all the client account_data for a that's changed for a user
+    ) -> Dict[str, JsonDict]:
+        """Get all the global account_data that's changed for a user.
 
         Args:
             user_id: The user to get the account_data for.
             stream_id: The point in the stream since which to get updates
+
         Returns:
-            A deferred pair of a dict of global account_data and a dict
-            mapping from room_id string to per room account_data dicts.
+            A dict of global account_data.
         """
 
-        def get_updated_account_data_for_user_txn(
+        def get_updated_global_account_data_for_user(
             txn: LoggingTransaction,
-        ) -> Tuple[Dict[str, JsonDict], Dict[str, Dict[str, JsonDict]]]:
-            sql = (
-                "SELECT account_data_type, content FROM account_data"
-                " WHERE user_id = ? AND stream_id > ?"
-            )
-
+        ) -> Dict[str, JsonDict]:
+            sql = """
+                SELECT account_data_type, content FROM account_data
+                WHERE user_id = ? AND stream_id > ?
+            """
             txn.execute(sql, (user_id, stream_id))
 
-            global_account_data = {row[0]: db_to_json(row[1]) for row in txn}
+            return {row[0]: db_to_json(row[1]) for row in txn}
 
-            sql = (
-                "SELECT room_id, account_data_type, content FROM room_account_data"
-                " WHERE user_id = ? AND stream_id > ?"
-            )
+        changed = self._account_data_stream_cache.has_entity_changed(
+            user_id, int(stream_id)
+        )
+        if not changed:
+            return {}
 
+        return await self.db_pool.runInteraction(
+            "get_updated_global_account_data_for_user",
+            get_updated_global_account_data_for_user,
+        )
+
+    async def get_updated_room_account_data_for_user(
+        self, user_id: str, stream_id: int
+    ) -> Dict[str, Dict[str, JsonDict]]:
+        """Get all the room account_data that's changed for a user.
+
+        Args:
+            user_id: The user to get the account_data for.
+            stream_id: The point in the stream since which to get updates
+
+        Returns:
+            A dict mapping from room_id string to per room account_data dicts.
+        """
+
+        def get_updated_room_account_data_for_user_txn(
+            txn: LoggingTransaction,
+        ) -> Dict[str, Dict[str, JsonDict]]:
+            sql = """
+                SELECT room_id, account_data_type, content FROM room_account_data
+                WHERE user_id = ? AND stream_id > ?
+            """
             txn.execute(sql, (user_id, stream_id))
 
             account_data_by_room: Dict[str, Dict[str, JsonDict]] = {}
@@ -379,16 +404,17 @@ def get_updated_account_data_for_user_txn(
                 room_account_data = account_data_by_room.setdefault(row[0], {})
                 room_account_data[row[1]] = db_to_json(row[2])
 
-            return global_account_data, account_data_by_room
+            return account_data_by_room
 
         changed = self._account_data_stream_cache.has_entity_changed(
             user_id, int(stream_id)
         )
         if not changed:
-            return {}, {}
+            return {}
 
         return await self.db_pool.runInteraction(
-            "get_updated_account_data_for_user", get_updated_account_data_for_user_txn
+            "get_updated_room_account_data_for_user",
+            get_updated_room_account_data_for_user_txn,
         )
 
     @cached(max_entries=5000, iterable=True)

From 07ebdcdf44bd4dd1850cef0581d0ee8901943227 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Thu, 2 Feb 2023 14:07:37 -0500
Subject: [PATCH 2/7] Split get_account_data_for_user into separate global &
 room methods.

---
 synapse/handlers/initial_sync.py              |  5 +-
 synapse/handlers/room_member.py               |  2 +-
 synapse/handlers/sync.py                      | 10 ++--
 synapse/rest/admin/users.py                   |  3 +-
 .../storage/databases/main/account_data.py    | 58 ++++++++++++++-----
 5 files changed, 54 insertions(+), 24 deletions(-)

diff --git a/synapse/handlers/initial_sync.py b/synapse/handlers/initial_sync.py
index 191529bd8e27..1a29abde9839 100644
--- a/synapse/handlers/initial_sync.py
+++ b/synapse/handlers/initial_sync.py
@@ -154,9 +154,8 @@ async def _snapshot_all_rooms(
 
         tags_by_room = await self.store.get_tags_for_user(user_id)
 
-        account_data, account_data_by_room = await self.store.get_account_data_for_user(
-            user_id
-        )
+        account_data = await self.store.get_global_account_data_for_user(user_id)
+        account_data_by_room = await self.store.get_room_account_data_for_user(user_id)
 
         public_room_ids = await self.store.get_public_room_ids()
 
diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index d236cc09b526..6e7141d2ef53 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -484,7 +484,7 @@ async def copy_room_tags_and_direct_to_room(
             user_id: The user's ID.
         """
         # Retrieve user account data for predecessor room
-        user_account_data, _ = await self.store.get_account_data_for_user(user_id)
+        user_account_data = await self.store.get_global_account_data_for_user(user_id)
 
         # Copy direct message state if applicable
         direct_rooms = user_account_data.get(AccountDataTypes.DIRECT, {})
diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py
index 4973255146f8..92e5f42fcaec 100644
--- a/synapse/handlers/sync.py
+++ b/synapse/handlers/sync.py
@@ -1762,10 +1762,12 @@ async def _generate_sync_entry_for_account_data(
                 )
         else:
             # TODO Do not fetch room account data if it will be unused.
-            (
-                global_account_data,
-                account_data_by_room,
-            ) = await self.store.get_account_data_for_user(sync_config.user.to_string())
+            global_account_data = await self.store.get_global_account_data_for_user(
+                sync_config.user.to_string()
+            )
+            account_data_by_room = await self.store.get_room_account_data_for_user(
+                sync_config.user.to_string()
+            )
 
             global_account_data["m.push_rules"] = await self.push_rules_for_user(
                 sync_config.user
diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py
index b9dca8ef3a34..0c0bf540b9ac 100644
--- a/synapse/rest/admin/users.py
+++ b/synapse/rest/admin/users.py
@@ -1192,7 +1192,8 @@ async def on_GET(
         if not await self._store.get_user_by_id(user_id):
             raise NotFoundError("User not found")
 
-        global_data, by_room_data = await self._store.get_account_data_for_user(user_id)
+        global_data = await self._store.get_global_account_data_for_user(user_id)
+        by_room_data = await self._store.get_room_account_data_for_user(user_id)
         return HTTPStatus.OK, {
             "account_data": {
                 "global": global_data,
diff --git a/synapse/storage/databases/main/account_data.py b/synapse/storage/databases/main/account_data.py
index b6db78948d34..b01e67c7922f 100644
--- a/synapse/storage/databases/main/account_data.py
+++ b/synapse/storage/databases/main/account_data.py
@@ -122,9 +122,9 @@ def get_max_account_data_stream_id(self) -> int:
         return self._account_data_id_gen.get_current_token()
 
     @cached()
-    async def get_account_data_for_user(
+    async def get_global_account_data_for_user(
         self, user_id: str
-    ) -> Tuple[Dict[str, JsonDict], Dict[str, Dict[str, JsonDict]]]:
+    ) -> Dict[str, JsonDict]:
         """
         Get all the client account_data for a user.
 
@@ -133,14 +133,14 @@ async def get_account_data_for_user(
 
         Args:
             user_id: The user to get the account_data for.
+
         Returns:
-            A 2-tuple of a dict of global account_data and a dict mapping from
-            room_id string to per room account_data dicts.
+            The global account_data.
         """
 
-        def get_account_data_for_user_txn(
+        def get_global_account_data_for_user(
             txn: LoggingTransaction,
-        ) -> Tuple[Dict[str, JsonDict], Dict[str, Dict[str, JsonDict]]]:
+        ) -> Dict[str, JsonDict]:
             # The 'content != '{}' condition below prevents us from using
             # `simple_select_list_txn` here, as it doesn't support conditions
             # other than 'equals'.
@@ -158,10 +158,34 @@ def get_account_data_for_user_txn(
             txn.execute(sql, (user_id,))
             rows = self.db_pool.cursor_to_dict(txn)
 
-            global_account_data = {
+            return {
                 row["account_data_type"]: db_to_json(row["content"]) for row in rows
             }
 
+        return await self.db_pool.runInteraction(
+            "get_global_account_data_for_user", get_global_account_data_for_user
+        )
+
+    @cached()
+    async def get_room_account_data_for_user(
+        self, user_id: str
+    ) -> Dict[str, Dict[str, JsonDict]]:
+        """
+        Get all the client account_data for a user.
+
+        If experimental MSC3391 support is enabled, any entries with an empty
+        content body are excluded; as this means they have been deleted.
+
+        Args:
+            user_id: The user to get the account_data for.
+
+        Returns:
+            A  dict mapping from room_id string to per room account_data dicts.
+        """
+
+        def get_room_account_data_for_user_txn(
+            txn: LoggingTransaction,
+        ) -> Dict[str, Dict[str, JsonDict]]:
             # The 'content != '{}' condition below prevents us from using
             # `simple_select_list_txn` here, as it doesn't support conditions
             # other than 'equals'.
@@ -185,10 +209,10 @@ def get_account_data_for_user_txn(
 
                 room_data[row["account_data_type"]] = db_to_json(row["content"])
 
-            return global_account_data, by_room
+            return by_room
 
         return await self.db_pool.runInteraction(
-            "get_account_data_for_user", get_account_data_for_user_txn
+            "get_room_account_data_for_user_txn", get_room_account_data_for_user_txn
         )
 
     @cached(num_args=2, max_entries=5000, tree=True)
@@ -470,7 +494,8 @@ def process_replication_rows(
                     self.get_global_account_data_by_type_for_user.invalidate(
                         (row.user_id, row.data_type)
                     )
-                self.get_account_data_for_user.invalidate((row.user_id,))
+                self.get_global_account_data_for_user.invalidate((row.user_id,))
+                self.get_room_account_data_for_user.invalidate((row.user_id,))
                 self.get_account_data_for_room.invalidate((row.user_id, row.room_id))
                 self.get_account_data_for_room_and_type.invalidate(
                     (row.user_id, row.room_id, row.data_type)
@@ -518,7 +543,7 @@ async def add_account_data_to_room(
             )
 
             self._account_data_stream_cache.entity_has_changed(user_id, next_id)
-            self.get_account_data_for_user.invalidate((user_id,))
+            self.get_room_account_data_for_user.invalidate((user_id,))
             self.get_account_data_for_room.invalidate((user_id, room_id))
             self.get_account_data_for_room_and_type.prefill(
                 (user_id, room_id, account_data_type), content
@@ -584,7 +609,7 @@ def _remove_account_data_for_room_txn(
                 return None
 
             self._account_data_stream_cache.entity_has_changed(user_id, next_id)
-            self.get_account_data_for_user.invalidate((user_id,))
+            self.get_room_account_data_for_user.invalidate((user_id,))
             self.get_account_data_for_room.invalidate((user_id, room_id))
             self.get_account_data_for_room_and_type.prefill(
                 (user_id, room_id, account_data_type), {}
@@ -619,7 +644,7 @@ async def add_account_data_for_user(
             )
 
             self._account_data_stream_cache.entity_has_changed(user_id, next_id)
-            self.get_account_data_for_user.invalidate((user_id,))
+            self.get_global_account_data_for_user.invalidate((user_id,))
             self.get_global_account_data_by_type_for_user.invalidate(
                 (user_id, account_data_type)
             )
@@ -787,7 +812,7 @@ def _remove_account_data_for_user_txn(
                 return None
 
             self._account_data_stream_cache.entity_has_changed(user_id, next_id)
-            self.get_account_data_for_user.invalidate((user_id,))
+            self.get_global_account_data_for_user.invalidate((user_id,))
             self.get_global_account_data_by_type_for_user.prefill(
                 (user_id, account_data_type), {}
             )
@@ -848,7 +873,10 @@ def _purge_account_data_for_user_txn(
             txn, self.get_account_data_for_room_and_type, (user_id,)
         )
         self._invalidate_cache_and_stream(
-            txn, self.get_account_data_for_user, (user_id,)
+            txn, self.get_global_account_data_for_user, (user_id,)
+        )
+        self._invalidate_cache_and_stream(
+            txn, self.get_room_account_data_for_user, (user_id,)
         )
         self._invalidate_cache_and_stream(
             txn, self.get_global_account_data_by_type_for_user, (user_id,)

From 891ecb4ec3ee102c0cb7b545d86a1e639a4d508f Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Thu, 2 Feb 2023 14:19:05 -0500
Subject: [PATCH 3/7] Fetch room-specific account data closer to where it is
 used.

---
 changelog.d/14973.misc   |  1 +
 synapse/handlers/sync.py | 43 ++++++++++++++++++----------------------
 2 files changed, 20 insertions(+), 24 deletions(-)
 create mode 100644 changelog.d/14973.misc

diff --git a/changelog.d/14973.misc b/changelog.d/14973.misc
new file mode 100644
index 000000000000..365762360285
--- /dev/null
+++ b/changelog.d/14973.misc
@@ -0,0 +1 @@
+Improve performance of `/sync` in a few situations.
diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py
index 92e5f42fcaec..fc7cd6e5b325 100644
--- a/synapse/handlers/sync.py
+++ b/synapse/handlers/sync.py
@@ -1444,9 +1444,7 @@ async def generate_sync_result(
 
         logger.debug("Fetching account data")
 
-        account_data_by_room = await self._generate_sync_entry_for_account_data(
-            sync_result_builder
-        )
+        await self._generate_sync_entry_for_account_data(sync_result_builder)
 
         # Presence data is included if the server has it enabled and not filtered out.
         include_presence_data = bool(
@@ -1472,9 +1470,7 @@ async def generate_sync_result(
             (
                 newly_joined_rooms,
                 newly_left_rooms,
-            ) = await self._generate_sync_entry_for_rooms(
-                sync_result_builder, account_data_by_room
-            )
+            ) = await self._generate_sync_entry_for_rooms(sync_result_builder)
 
             # Work out which users have joined or left rooms we're in. We use this
             # to build the presence and device_list parts of the sync response in
@@ -1717,7 +1713,7 @@ async def _generate_sync_entry_for_to_device(
 
     async def _generate_sync_entry_for_account_data(
         self, sync_result_builder: "SyncResultBuilder"
-    ) -> Dict[str, Dict[str, JsonDict]]:
+    ) -> None:
         """Generates the account data portion of the sync response.
 
         Account data (called "Client Config" in the spec) can be set either globally
@@ -1740,17 +1736,11 @@ async def _generate_sync_entry_for_account_data(
         since_token = sync_result_builder.since_token
 
         if since_token and not sync_result_builder.full_state:
-            # TODO Do not fetch room account data if it will be unused.
             global_account_data = (
                 await self.store.get_updated_global_account_data_for_user(
                     user_id, since_token.account_data_key
                 )
             )
-            account_data_by_room = (
-                await self.store.get_updated_room_account_data_for_user(
-                    user_id, since_token.account_data_key
-                )
-            )
 
             push_rules_changed = await self.store.have_push_rules_changed_for_user(
                 user_id, int(since_token.push_rules_key)
@@ -1761,12 +1751,8 @@ async def _generate_sync_entry_for_account_data(
                     sync_config.user
                 )
         else:
-            # TODO Do not fetch room account data if it will be unused.
             global_account_data = await self.store.get_global_account_data_for_user(
-                sync_config.user.to_string()
-            )
-            account_data_by_room = await self.store.get_room_account_data_for_user(
-                sync_config.user.to_string()
+                user_id
             )
 
             global_account_data["m.push_rules"] = await self.push_rules_for_user(
@@ -1782,8 +1768,6 @@ async def _generate_sync_entry_for_account_data(
 
         sync_result_builder.account_data = account_data_for_user
 
-        return account_data_by_room
-
     async def _generate_sync_entry_for_presence(
         self,
         sync_result_builder: "SyncResultBuilder",
@@ -1843,9 +1827,7 @@ async def _generate_sync_entry_for_presence(
         sync_result_builder.presence = presence
 
     async def _generate_sync_entry_for_rooms(
-        self,
-        sync_result_builder: "SyncResultBuilder",
-        account_data_by_room: Dict[str, Dict[str, JsonDict]],
+        self, sync_result_builder: "SyncResultBuilder"
     ) -> Tuple[AbstractSet[str], AbstractSet[str]]:
         """Generates the rooms portion of the sync response. Populates the
         `sync_result_builder` with the result.
@@ -1856,7 +1838,6 @@ async def _generate_sync_entry_for_rooms(
 
         Args:
             sync_result_builder
-            account_data_by_room: Dictionary of per room account data
 
         Returns:
             Returns a 2-tuple describing rooms the user has joined or left.
@@ -1869,6 +1850,20 @@ async def _generate_sync_entry_for_rooms(
         since_token = sync_result_builder.since_token
         user_id = sync_result_builder.sync_config.user.to_string()
 
+        # 0. Start by fetching room account data.
+        #
+        # TODO Do not fetch room account data if it will be unused.
+        if since_token and not sync_result_builder.full_state:
+            account_data_by_room = (
+                await self.store.get_updated_room_account_data_for_user(
+                    user_id, since_token.account_data_key
+                )
+            )
+        else:
+            account_data_by_room = await self.store.get_room_account_data_for_user(
+                user_id
+            )
+
         # 1. Start by fetching all ephemeral events in rooms we've joined (if required).
         block_all_room_ephemeral = (
             sync_result_builder.sync_config.filter_collection.blocks_all_rooms()

From 4b3420e0ec38d253d5cb044da650025818023f82 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Thu, 9 Feb 2023 10:22:38 -0500
Subject: [PATCH 4/7] Do not fetch room account data if it will be filtered
 out.

---
 synapse/api/filtering.py | 13 +++++++++++--
 synapse/handlers/sync.py | 17 ++++++++++++-----
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/synapse/api/filtering.py b/synapse/api/filtering.py
index 83c42fc25a22..02cef976e2d3 100644
--- a/synapse/api/filtering.py
+++ b/synapse/api/filtering.py
@@ -219,7 +219,9 @@ def __init__(self, hs: "HomeServer", filter_json: JsonDict):
         self._room_timeline_filter = Filter(hs, room_filter_json.get("timeline", {}))
         self._room_state_filter = Filter(hs, room_filter_json.get("state", {}))
         self._room_ephemeral_filter = Filter(hs, room_filter_json.get("ephemeral", {}))
-        self._room_account_data = Filter(hs, room_filter_json.get("account_data", {}))
+        self._room_account_data_filter = Filter(
+            hs, room_filter_json.get("account_data", {})
+        )
         self._presence_filter = Filter(hs, filter_json.get("presence", {}))
         self._account_data = Filter(hs, filter_json.get("account_data", {}))
 
@@ -279,7 +281,7 @@ async def filter_room_ephemeral(self, events: Iterable[JsonDict]) -> List[JsonDi
     async def filter_room_account_data(
         self, events: Iterable[JsonDict]
     ) -> List[JsonDict]:
-        return await self._room_account_data.filter(
+        return await self._room_account_data_filter.filter(
             await self._room_filter.filter(events)
         )
 
@@ -299,6 +301,13 @@ def blocks_all_room_ephemeral(self) -> bool:
             or self._room_ephemeral_filter.filters_all_rooms()
         )
 
+    def blocks_all_room_account_data(self) -> bool:
+        return (
+            self._room_account_data_filter.filters_all_types()
+            or self._room_account_data_filter.filters_all_senders()
+            or self._room_account_data_filter.filters_all_rooms()
+        )
+
     def blocks_all_room_timeline(self) -> bool:
         return (
             self._room_timeline_filter.filters_all_types()
diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py
index fc7cd6e5b325..3ef8c6437762 100644
--- a/synapse/handlers/sync.py
+++ b/synapse/handlers/sync.py
@@ -1850,10 +1850,17 @@ async def _generate_sync_entry_for_rooms(
         since_token = sync_result_builder.since_token
         user_id = sync_result_builder.sync_config.user.to_string()
 
-        # 0. Start by fetching room account data.
-        #
-        # TODO Do not fetch room account data if it will be unused.
-        if since_token and not sync_result_builder.full_state:
+        blocks_all_rooms = (
+            sync_result_builder.sync_config.filter_collection.blocks_all_rooms()
+        )
+
+        # 0. Start by fetching room account data (if required).
+        if (
+            blocks_all_rooms
+            or sync_result_builder.sync_config.filter_collection.blocks_all_room_account_data()
+        ):
+            account_data_by_room = {}
+        elif since_token and not sync_result_builder.full_state:
             account_data_by_room = (
                 await self.store.get_updated_room_account_data_for_user(
                     user_id, since_token.account_data_key
@@ -1866,7 +1873,7 @@ async def _generate_sync_entry_for_rooms(
 
         # 1. Start by fetching all ephemeral events in rooms we've joined (if required).
         block_all_room_ephemeral = (
-            sync_result_builder.sync_config.filter_collection.blocks_all_rooms()
+            blocks_all_rooms
             or sync_result_builder.sync_config.filter_collection.blocks_all_room_ephemeral()
         )
         if block_all_room_ephemeral:

From ab605e54dc36e26654b4de9dc11d91eddddecb06 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Thu, 9 Feb 2023 10:44:41 -0500
Subject: [PATCH 5/7] Global acct dat

---
 synapse/api/filtering.py | 11 +++++++++--
 synapse/handlers/sync.py |  4 +++-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/synapse/api/filtering.py b/synapse/api/filtering.py
index 02cef976e2d3..7c41c30d4f1c 100644
--- a/synapse/api/filtering.py
+++ b/synapse/api/filtering.py
@@ -223,7 +223,7 @@ def __init__(self, hs: "HomeServer", filter_json: JsonDict):
             hs, room_filter_json.get("account_data", {})
         )
         self._presence_filter = Filter(hs, filter_json.get("presence", {}))
-        self._account_data = Filter(hs, filter_json.get("account_data", {}))
+        self._account_data_filter = Filter(hs, filter_json.get("account_data", {}))
 
         self.include_leave = filter_json.get("room", {}).get("include_leave", False)
         self.event_fields = filter_json.get("event_fields", [])
@@ -259,7 +259,7 @@ async def filter_presence(
         return await self._presence_filter.filter(presence_states)
 
     async def filter_account_data(self, events: Iterable[JsonDict]) -> List[JsonDict]:
-        return await self._account_data.filter(events)
+        return await self._account_data_filter.filter(events)
 
     async def filter_room_state(self, events: Iterable[EventBase]) -> List[EventBase]:
         return await self._room_state_filter.filter(
@@ -294,6 +294,13 @@ def blocks_all_presence(self) -> bool:
             or self._presence_filter.filters_all_senders()
         )
 
+    def blocks_all_account_data(self) -> bool:
+        """True if all global acount data will be filtered out."""
+        return (
+            self._account_data_filter.filters_all_types()
+            or self._account_data_filter.filters_all_senders()
+        )
+
     def blocks_all_room_ephemeral(self) -> bool:
         return (
             self._room_ephemeral_filter.filters_all_types()
diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py
index 3ef8c6437762..20e8c9786c8b 100644
--- a/synapse/handlers/sync.py
+++ b/synapse/handlers/sync.py
@@ -1444,7 +1444,9 @@ async def generate_sync_result(
 
         logger.debug("Fetching account data")
 
-        await self._generate_sync_entry_for_account_data(sync_result_builder)
+        # Account data is included if it is not filtered out.
+        if not sync_config.filter_collection.blocks_all_account_data():
+            await self._generate_sync_entry_for_account_data(sync_result_builder)
 
         # Presence data is included if the server has it enabled and not filtered out.
         include_presence_data = bool(

From 67de6cf50d2f4978694e15078558d80e4496abee Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Fri, 10 Feb 2023 08:31:28 -0500
Subject: [PATCH 6/7] Rename filter properties.

---
 synapse/api/filtering.py | 16 ++++++++++------
 synapse/handlers/sync.py | 14 ++++++++------
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/synapse/api/filtering.py b/synapse/api/filtering.py
index 7c41c30d4f1c..b9f432cc234a 100644
--- a/synapse/api/filtering.py
+++ b/synapse/api/filtering.py
@@ -223,7 +223,9 @@ def __init__(self, hs: "HomeServer", filter_json: JsonDict):
             hs, room_filter_json.get("account_data", {})
         )
         self._presence_filter = Filter(hs, filter_json.get("presence", {}))
-        self._account_data_filter = Filter(hs, filter_json.get("account_data", {}))
+        self._global_account_data_filter = Filter(
+            hs, filter_json.get("account_data", {})
+        )
 
         self.include_leave = filter_json.get("room", {}).get("include_leave", False)
         self.event_fields = filter_json.get("event_fields", [])
@@ -258,8 +260,10 @@ async def filter_presence(
     ) -> List[UserPresenceState]:
         return await self._presence_filter.filter(presence_states)
 
-    async def filter_account_data(self, events: Iterable[JsonDict]) -> List[JsonDict]:
-        return await self._account_data_filter.filter(events)
+    async def filter_global_account_data(
+        self, events: Iterable[JsonDict]
+    ) -> List[JsonDict]:
+        return await self._global_account_data_filter.filter(events)
 
     async def filter_room_state(self, events: Iterable[EventBase]) -> List[EventBase]:
         return await self._room_state_filter.filter(
@@ -294,11 +298,11 @@ def blocks_all_presence(self) -> bool:
             or self._presence_filter.filters_all_senders()
         )
 
-    def blocks_all_account_data(self) -> bool:
+    def blocks_all_global_account_data(self) -> bool:
         """True if all global acount data will be filtered out."""
         return (
-            self._account_data_filter.filters_all_types()
-            or self._account_data_filter.filters_all_senders()
+            self._global_account_data_filter.filters_all_types()
+            or self._global_account_data_filter.filters_all_senders()
         )
 
     def blocks_all_room_ephemeral(self) -> bool:
diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py
index 2b397bad59a5..5d22d6cb1559 100644
--- a/synapse/handlers/sync.py
+++ b/synapse/handlers/sync.py
@@ -1445,7 +1445,7 @@ async def generate_sync_result(
         logger.debug("Fetching account data")
 
         # Account data is included if it is not filtered out.
-        if not sync_config.filter_collection.blocks_all_account_data():
+        if not sync_config.filter_collection.blocks_all_global_account_data():
             await self._generate_sync_entry_for_account_data(sync_result_builder)
 
         # Presence data is included if the server has it enabled and not filtered out.
@@ -1763,11 +1763,13 @@ async def _generate_sync_entry_for_account_data(
                 sync_config.user
             )
 
-        account_data_for_user = await sync_config.filter_collection.filter_account_data(
-            [
-                {"type": account_data_type, "content": content}
-                for account_data_type, content in global_account_data.items()
-            ]
+        account_data_for_user = (
+            await sync_config.filter_collection.filter_global_account_data(
+                [
+                    {"type": account_data_type, "content": content}
+                    for account_data_type, content in global_account_data.items()
+                ]
+            )
         )
 
         sync_result_builder.account_data = account_data_for_user

From 21f57038fa26127f967f0e4c6eab7f7936cb571b Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Fri, 10 Feb 2023 08:46:18 -0500
Subject: [PATCH 7/7] Use Mapping instead of Dict.

---
 synapse/handlers/sync.py                      | 22 ++++++++-----------
 .../storage/databases/main/account_data.py    | 11 +++++-----
 2 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py
index 5d22d6cb1559..399685e5b7d5 100644
--- a/synapse/handlers/sync.py
+++ b/synapse/handlers/sync.py
@@ -1444,7 +1444,7 @@ async def generate_sync_result(
 
         logger.debug("Fetching account data")
 
-        # Account data is included if it is not filtered out.
+        # Global account data is included if it is not filtered out.
         if not sync_config.filter_collection.blocks_all_global_account_data():
             await self._generate_sync_entry_for_account_data(sync_result_builder)
 
@@ -1716,22 +1716,18 @@ async def _generate_sync_entry_for_to_device(
     async def _generate_sync_entry_for_account_data(
         self, sync_result_builder: "SyncResultBuilder"
     ) -> None:
-        """Generates the account data portion of the sync response.
+        """Generates the global account data portion of the sync response.
 
         Account data (called "Client Config" in the spec) can be set either globally
         or for a specific room. Account data consists of a list of events which
         accumulate state, much like a room.
 
-        This function retrieves global and per-room account data. The former is written
-        to the given `sync_result_builder`. The latter is returned directly, to be
-        later written to the `sync_result_builder` on a room-by-room basis.
+        This function retrieves global account data and writes it to the given
+        `sync_result_builder`. See `_generate_sync_entry_for_rooms` for handling
+         of per-room account data.
 
         Args:
             sync_result_builder
-
-        Returns:
-            A dictionary whose keys (room ids) map to the per room account data for that
-            room.
         """
         sync_config = sync_result_builder.sync_config
         user_id = sync_result_builder.sync_config.user.to_string()
@@ -1754,11 +1750,11 @@ async def _generate_sync_entry_for_account_data(
                     sync_config.user
                 )
         else:
-            global_account_data = await self.store.get_global_account_data_for_user(
+            all_global_account_data = await self.store.get_global_account_data_for_user(
                 user_id
             )
 
-            global_account_data = dict(global_account_data)
+            global_account_data = dict(all_global_account_data)
             global_account_data["m.push_rules"] = await self.push_rules_for_user(
                 sync_config.user
             )
@@ -1865,7 +1861,7 @@ async def _generate_sync_entry_for_rooms(
             blocks_all_rooms
             or sync_result_builder.sync_config.filter_collection.blocks_all_room_account_data()
         ):
-            account_data_by_room = {}
+            account_data_by_room: Mapping[str, Mapping[str, JsonDict]] = {}
         elif since_token and not sync_result_builder.full_state:
             account_data_by_room = (
                 await self.store.get_updated_room_account_data_for_user(
@@ -2306,7 +2302,7 @@ async def _generate_room_entry(
         room_builder: "RoomSyncResultBuilder",
         ephemeral: List[JsonDict],
         tags: Optional[Dict[str, Dict[str, Any]]],
-        account_data: Dict[str, JsonDict],
+        account_data: Mapping[str, JsonDict],
         always_include: bool = False,
     ) -> None:
         """Populates the `joined` and `archived` section of `sync_result_builder`
diff --git a/synapse/storage/databases/main/account_data.py b/synapse/storage/databases/main/account_data.py
index b01e67c7922f..2d6f02c14fe5 100644
--- a/synapse/storage/databases/main/account_data.py
+++ b/synapse/storage/databases/main/account_data.py
@@ -21,6 +21,7 @@
     FrozenSet,
     Iterable,
     List,
+    Mapping,
     Optional,
     Tuple,
     cast,
@@ -124,9 +125,9 @@ def get_max_account_data_stream_id(self) -> int:
     @cached()
     async def get_global_account_data_for_user(
         self, user_id: str
-    ) -> Dict[str, JsonDict]:
+    ) -> Mapping[str, JsonDict]:
         """
-        Get all the client account_data for a user.
+        Get all the global client account_data for a user.
 
         If experimental MSC3391 support is enabled, any entries with an empty
         content body are excluded; as this means they have been deleted.
@@ -169,9 +170,9 @@ def get_global_account_data_for_user(
     @cached()
     async def get_room_account_data_for_user(
         self, user_id: str
-    ) -> Dict[str, Dict[str, JsonDict]]:
+    ) -> Mapping[str, Mapping[str, JsonDict]]:
         """
-        Get all the client account_data for a user.
+        Get all of the per-room client account_data for a user.
 
         If experimental MSC3391 support is enabled, any entries with an empty
         content body are excluded; as this means they have been deleted.
@@ -180,7 +181,7 @@ async def get_room_account_data_for_user(
             user_id: The user to get the account_data for.
 
         Returns:
-            A  dict mapping from room_id string to per room account_data dicts.
+            A dict mapping from room_id string to per-room account_data dicts.
         """
 
         def get_room_account_data_for_user_txn(