From 03cf4385e098ae73730b9c5ef695fa3f16c1806f Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Wed, 2 Oct 2019 15:09:10 +0100
Subject: Fix public room list pagination.

We incorrectly used `room_id` as to bound the result set, even though we
order by `joined_members, room_id`, leading to incorrect results after
pagination.
---
 synapse/storage/room.py | 53 +++++++++++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 17 deletions(-)

(limited to 'synapse/storage')

diff --git a/synapse/storage/room.py b/synapse/storage/room.py
index c02787a73d..9b7e31583c 100644
--- a/synapse/storage/room.py
+++ b/synapse/storage/room.py
@@ -17,6 +17,7 @@
 import collections
 import logging
 import re
+from typing import Optional, Tuple
 
 from canonicaljson import json
 
@@ -25,6 +26,7 @@ from twisted.internet import defer
 from synapse.api.errors import StoreError
 from synapse.storage._base import SQLBaseStore
 from synapse.storage.search import SearchStore
+from synapse.types import ThirdPartyInstanceID
 from synapse.util.caches.descriptors import cached, cachedInlineCallbacks
 
 logger = logging.getLogger(__name__)
@@ -119,24 +121,25 @@ class RoomWorkerStore(SQLBaseStore):
     @defer.inlineCallbacks
     def get_largest_public_rooms(
         self,
-        network_tuple,
-        search_filter,
-        limit,
-        last_room_id,
-        forwards,
-        ignore_non_federatable=False,
+        network_tuple: Optional[ThirdPartyInstanceID],
+        search_filter: Optional[dict],
+        limit: Optional[int],
+        bounds: Optional[Tuple[int, str]],
+        forwards: bool,
+        ignore_non_federatable: bool = False,
     ):
         """Gets the largest public rooms (where largest is in terms of joined
         members, as tracked in the statistics table).
 
         Args:
-            network_tuple (ThirdPartyInstanceID|None):
-            search_filter (dict|None):
-            limit (int|None): Maxmimum number of rows to return, unlimited otherwise.
-            last_room_id (str|None): if present, a room ID which bounds the
-                result set, and is always *excluded* from the result set.
-            forwards (bool): true iff going forwards, going backwards otherwise
-            ignore_non_federatable (bool): If true filters out non-federatable rooms.
+            network_tuple
+            search_filter
+            limit: Maxmimum number of rows to return, unlimited otherwise.
+            bounds: An uppoer or lower bound to apply to result set if given,
+                consists of a joined member count and room_id (these are
+                excluded from result set).
+            forwards: true iff going forwards, going backwards otherwise
+            ignore_non_federatable: If true filters out non-federatable rooms.
 
         Returns:
             Rooms in order: biggest number of joined users first.
@@ -147,13 +150,29 @@ class RoomWorkerStore(SQLBaseStore):
         where_clauses = []
         query_args = []
 
-        if last_room_id:
+        # Work out the bounds if we're given them, these bounds look slightly
+        # odd, but are designed to help query planner use indices by pulling
+        # out a common bound.
+        if bounds:
+            last_joined_members, last_room_id = bounds
             if forwards:
-                where_clauses.append("room_id < ?")
+                where_clauses.append(
+                    """
+                        joined_members <= ? AND (
+                            joined_members < ? OR room_id < ?
+                        )
+                    """
+                )
             else:
-                where_clauses.append("? < room_id")
+                where_clauses.append(
+                    """
+                        joined_members >= ? AND (
+                            joined_members > ? OR room_id > ?
+                        )
+                    """
+                )
 
-            query_args += [last_room_id]
+            query_args += [last_joined_members, last_joined_members, last_room_id]
 
         if search_filter and search_filter.get("generic_search_term", None):
             search_term = "%" + search_filter["generic_search_term"] + "%"
-- 
cgit 1.4.1