From 75b29aa63a652df1876950f627aef541f45581de Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 11 May 2020 18:15:26 +0100 Subject: [PATCH 1/9] Prevent 0-member rooms from showing up --- .../storage/data_stores/main/group_server.py | 57 +++++++++++++++---- 1 file changed, 47 insertions(+), 10 deletions(-) diff --git a/synapse/storage/data_stores/main/group_server.py b/synapse/storage/data_stores/main/group_server.py index 0963e6c250b2..9c9e5911cbc5 100644 --- a/synapse/storage/data_stores/main/group_server.py +++ b/synapse/storage/data_stores/main/group_server.py @@ -68,19 +68,50 @@ def get_invited_users_in_group(self, group_id): desc="get_invited_users_in_group", ) - def get_rooms_in_group(self, group_id, include_private=False): + def get_rooms_in_group(self, group_id: str, include_private: bool = False): + """Retrieve the rooms that belong to a given group. Does not return rooms that + lack members. + + Args: + group_id: The ID of the group to query for rooms + include_private: Whether to return private rooms + + Returns: + Deferred[List[Dict[str, str|bool]]]: A list of dictionaries, each in the + form of: + + { + "room_id": "!a_room_id:example.com", # The ID of the room + "is_public": False # Whether this is a public room or not + } + """ # TODO: Pagination - keyvalues = {"group_id": group_id} - if not include_private: - keyvalues["is_public"] = True + def _get_rooms_in_group_txn(txn): + sql = """ + SELECT room_id, is_public FROM group_rooms + WHERE group_id = ? + AND room_id IN ( + SELECT group_rooms.room_id FROM group_rooms + LEFT JOIN room_stats_current ON + group_rooms.room_id = room_stats_current.room_id + WHERE joined_members > 0 + ) + """ + args = [group_id] - return self.db.simple_select_list( - table="group_rooms", - keyvalues=keyvalues, - retcols=("room_id", "is_public"), - desc="get_rooms_in_group", - ) + if not include_private: + sql += " AND is_public = ?" + args += [True] + + txn.execute(sql, args) + + return [ + {"room_id": room_id, "is_public": is_public} + for room_id, is_public in txn + ] + + return self.db.runInteraction("get_rooms_in_group", _get_rooms_in_group_txn) def get_rooms_for_summary_by_category(self, group_id, include_private=False): """Get the rooms and categories that should be included in a summary request @@ -97,6 +128,12 @@ def _get_rooms_for_summary_txn(txn): SELECT room_id, is_public, category_id, room_order FROM group_summary_rooms WHERE group_id = ? + AND room_id IN ( + SELECT group_rooms.room_id FROM group_rooms + LEFT JOIN room_stats_current ON + group_rooms.room_id = room_stats_current.room_id + WHERE joined_members > 0 + ) """ if not include_private: From 66638f3e70bf34d709390b7f3b547d67fd35bb23 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 11 May 2020 18:59:19 +0100 Subject: [PATCH 2/9] Prevent rooms with NULL or '' room_version from showing up --- synapse/storage/data_stores/main/group_server.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/storage/data_stores/main/group_server.py b/synapse/storage/data_stores/main/group_server.py index 9c9e5911cbc5..03707acdc67d 100644 --- a/synapse/storage/data_stores/main/group_server.py +++ b/synapse/storage/data_stores/main/group_server.py @@ -96,6 +96,7 @@ def _get_rooms_in_group_txn(txn): LEFT JOIN room_stats_current ON group_rooms.room_id = room_stats_current.room_id WHERE joined_members > 0 + AND (room_version <> '') IS FALSE ) """ args = [group_id] @@ -133,6 +134,7 @@ def _get_rooms_for_summary_txn(txn): LEFT JOIN room_stats_current ON group_rooms.room_id = room_stats_current.room_id WHERE joined_members > 0 + AND (room_version <> '') IS FALSE ) """ From 7cddead4621df2e36da412fbca6eaa72de38c998 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 11 May 2020 19:14:56 +0100 Subject: [PATCH 3/9] docstring for get_rooms_for_summary_by_category --- .../storage/data_stores/main/group_server.py | 27 ++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/synapse/storage/data_stores/main/group_server.py b/synapse/storage/data_stores/main/group_server.py index 03707acdc67d..ed3297db19ac 100644 --- a/synapse/storage/data_stores/main/group_server.py +++ b/synapse/storage/data_stores/main/group_server.py @@ -74,7 +74,7 @@ def get_rooms_in_group(self, group_id: str, include_private: bool = False): Args: group_id: The ID of the group to query for rooms - include_private: Whether to return private rooms + include_private: Whether to return private rooms in results Returns: Deferred[List[Dict[str, str|bool]]]: A list of dictionaries, each in the @@ -114,10 +114,31 @@ def _get_rooms_in_group_txn(txn): return self.db.runInteraction("get_rooms_in_group", _get_rooms_in_group_txn) - def get_rooms_for_summary_by_category(self, group_id, include_private=False): + def get_rooms_for_summary_by_category( + self, + group_id: str, + include_private: bool = False, + ): """Get the rooms and categories that should be included in a summary request - Returns ([rooms], [categories]) + Args: + group_id: The ID of the group to query the summary for + include_private: Whether to return private rooms in results + + Returns: + Deferred[Tuple[List, Dict]]: A tuple containing: + + * A list of dictionaries with the keys: + * "room_id": str, the room ID + * "is_public": bool, whether the room is public + * "category_id": str|None, the category ID if set, else None + * "order": int, the sort order of rooms + + * A dictionary with the key: + * category_id (str): a dictionary with the keys: + * "is_public": bool, whether the category is public + * "profile": str, the category profile + * "order": int, the sort order of rooms in this category """ def _get_rooms_for_summary_txn(txn): From c4308db208e04e7fb0be9bba803ccadd4f639472 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 11 May 2020 19:17:24 +0100 Subject: [PATCH 4/9] Changelog --- changelog.d/7465.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/7465.bugfix diff --git a/changelog.d/7465.bugfix b/changelog.d/7465.bugfix new file mode 100644 index 000000000000..1cbe50caa5e4 --- /dev/null +++ b/changelog.d/7465.bugfix @@ -0,0 +1 @@ +Prevent rooms with 0 members or with invalid version strings from breaking group queries. \ No newline at end of file From dd78e5635a9972975f8b9cfb3e8bc7c1e7ab72f6 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 11 May 2020 19:19:59 +0100 Subject: [PATCH 5/9] The one commit I don't run linting scripts on --- synapse/storage/data_stores/main/group_server.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/synapse/storage/data_stores/main/group_server.py b/synapse/storage/data_stores/main/group_server.py index ed3297db19ac..e432693ae255 100644 --- a/synapse/storage/data_stores/main/group_server.py +++ b/synapse/storage/data_stores/main/group_server.py @@ -115,9 +115,7 @@ def _get_rooms_in_group_txn(txn): return self.db.runInteraction("get_rooms_in_group", _get_rooms_in_group_txn) def get_rooms_for_summary_by_category( - self, - group_id: str, - include_private: bool = False, + self, group_id: str, include_private: bool = False, ): """Get the rooms and categories that should be included in a summary request From 5bda21c333a915deab6d8befc91a2f05a0df0954 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 11 May 2020 19:41:18 +0100 Subject: [PATCH 6/9] room_version is on the rooms table --- synapse/storage/data_stores/main/group_server.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/synapse/storage/data_stores/main/group_server.py b/synapse/storage/data_stores/main/group_server.py index e432693ae255..1c9093ab5022 100644 --- a/synapse/storage/data_stores/main/group_server.py +++ b/synapse/storage/data_stores/main/group_server.py @@ -96,7 +96,9 @@ def _get_rooms_in_group_txn(txn): LEFT JOIN room_stats_current ON group_rooms.room_id = room_stats_current.room_id WHERE joined_members > 0 - AND (room_version <> '') IS FALSE + LEFT JOIN rooms ON + group_rooms.room_id = rooms.room_id + WHERE (room_version <> '') IS FALSE ) """ args = [group_id] @@ -153,7 +155,9 @@ def _get_rooms_for_summary_txn(txn): LEFT JOIN room_stats_current ON group_rooms.room_id = room_stats_current.room_id WHERE joined_members > 0 - AND (room_version <> '') IS FALSE + LEFT JOIN rooms ON + group_rooms.room_id = rooms.room_id + WHERE (room_version <> '') IS FALSE ) """ From 42b2d1355a9b10854128e1ce804418965fb94932 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 11 May 2020 19:45:39 +0100 Subject: [PATCH 7/9] Correct sql syntax --- synapse/storage/data_stores/main/group_server.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/storage/data_stores/main/group_server.py b/synapse/storage/data_stores/main/group_server.py index 1c9093ab5022..a43c6e3e3194 100644 --- a/synapse/storage/data_stores/main/group_server.py +++ b/synapse/storage/data_stores/main/group_server.py @@ -95,10 +95,10 @@ def _get_rooms_in_group_txn(txn): SELECT group_rooms.room_id FROM group_rooms LEFT JOIN room_stats_current ON group_rooms.room_id = room_stats_current.room_id - WHERE joined_members > 0 + AND joined_members > 0 LEFT JOIN rooms ON group_rooms.room_id = rooms.room_id - WHERE (room_version <> '') IS FALSE + AND (room_version <> '') IS FALSE ) """ args = [group_id] @@ -154,10 +154,10 @@ def _get_rooms_for_summary_txn(txn): SELECT group_rooms.room_id FROM group_rooms LEFT JOIN room_stats_current ON group_rooms.room_id = room_stats_current.room_id - WHERE joined_members > 0 + AND joined_members > 0 LEFT JOIN rooms ON group_rooms.room_id = rooms.room_id - WHERE (room_version <> '') IS FALSE + AND (room_version <> '') IS FALSE ) """ From 622137815337b0ecdc8bb43c0938f51a42aaf37c Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 11 May 2020 20:11:29 +0100 Subject: [PATCH 8/9] false is only supported in sqlite 3.23.0 and up --- synapse/storage/data_stores/main/group_server.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/storage/data_stores/main/group_server.py b/synapse/storage/data_stores/main/group_server.py index a43c6e3e3194..da2608ad0807 100644 --- a/synapse/storage/data_stores/main/group_server.py +++ b/synapse/storage/data_stores/main/group_server.py @@ -98,10 +98,10 @@ def _get_rooms_in_group_txn(txn): AND joined_members > 0 LEFT JOIN rooms ON group_rooms.room_id = rooms.room_id - AND (room_version <> '') IS FALSE + AND (room_version <> '') = ? ) """ - args = [group_id] + args = [group_id, False] if not include_private: sql += " AND is_public = ?" @@ -157,15 +157,15 @@ def _get_rooms_for_summary_txn(txn): AND joined_members > 0 LEFT JOIN rooms ON group_rooms.room_id = rooms.room_id - AND (room_version <> '') IS FALSE + AND (room_version <> '') = ? ) """ if not include_private: sql += " AND is_public = ?" - txn.execute(sql, (group_id, True)) + txn.execute(sql, (group_id, False, True)) else: - txn.execute(sql, (group_id,)) + txn.execute(sql, (group_id, False)) rooms = [ { From 155cb5ce8516f0ff3beb1134d546d127fca2e5ec Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 14 May 2020 17:34:30 +0100 Subject: [PATCH 9/9] Also filter on rooms with 0 local_users_in_room --- synapse/storage/data_stores/main/group_server.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/storage/data_stores/main/group_server.py b/synapse/storage/data_stores/main/group_server.py index da2608ad0807..fb1361f1c197 100644 --- a/synapse/storage/data_stores/main/group_server.py +++ b/synapse/storage/data_stores/main/group_server.py @@ -96,6 +96,7 @@ def _get_rooms_in_group_txn(txn): LEFT JOIN room_stats_current ON group_rooms.room_id = room_stats_current.room_id AND joined_members > 0 + AND local_users_in_room > 0 LEFT JOIN rooms ON group_rooms.room_id = rooms.room_id AND (room_version <> '') = ? @@ -155,6 +156,7 @@ def _get_rooms_for_summary_txn(txn): LEFT JOIN room_stats_current ON group_rooms.room_id = room_stats_current.room_id AND joined_members > 0 + AND local_users_in_room > 0 LEFT JOIN rooms ON group_rooms.room_id = rooms.room_id AND (room_version <> '') = ?