incorporate review

2 files changed, 47 insertions, 34 deletions

diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py
index 205be3ea4f..39ef2af08e 100644
--- a/synapse/handlers/sync.py
+++ b/synapse/handlers/sync.py
@@ -578,14 +578,14 @@ class SyncHandler(object):
 
         # FIXME: only build up a member_ids list for our heroes
         member_ids = {}
-        for m in (
+        for membership in (
             Membership.JOIN,
             Membership.INVITE,
             Membership.LEAVE,
             Membership.BAN
         ):
-            for r in details.get(m, ([], 0))[0]:
-                member_ids[r[0]] = r[1]
+            for user_id, event_id in details.get(membership, empty_ms).members:
+                member_ids[user_id] = event_id
 
         # FIXME: order by stream ordering rather than as returned by SQL
         me = sync_config.user.to_string()
@@ -762,8 +762,9 @@ class SyncHandler(object):
                 state_ids = {}
                 if lazy_load_members:
                     if types:
-                        # We're returning an incremental (or initial) sync, with no "gap" since
-                        # the previous sync, so normally there would be no state to return
+                        # We're returning an incremental (or initial) sync, with no
+                        # "gap" since the previous sync, so normally there would be
+                        # no state to return.
                         # But we're lazy-loading, so the client might need some more
                         # member events to understand the events in this timeline.
                         # So we fish out all the member events corresponding to the
@@ -1633,15 +1634,23 @@ class SyncHandler(object):
         )
 
         summary = {}
+
+        # we include a summary in room responses when we're lazy loading
+        # members (as the client otherwise doesn't have enough info to form
+        # the name itself).
         if (
             sync_config.filter_collection.lazy_load_members() and
             (
+                # we recalulate the summary:
+                #   if there are membership changes in the timeline, or
+                #   if membership has changed during a gappy sync, or
+                #   if this is an initial sync.
                 any(ev.type == EventTypes.Member for ev in batch.events) or
                 (
                     # XXX: this may include false positives in the form of LL
                     # members which have snuck into state
                     batch.limited and
-                    any(t == EventTypes.Member for (t, k) in state.keys())
+                    any(t == EventTypes.Member for (t, k) in state)
                 ) or
                 since_token is None
             )
diff --git a/synapse/storage/roommember.py b/synapse/storage/roommember.py
index 6de1431488..003d0e30c0 100644
--- a/synapse/storage/roommember.py
+++ b/synapse/storage/roommember.py
@@ -88,7 +88,7 @@ class RoomMemberWorkerStore(EventsWorkerStore):
             return [to_ascii(r[0]) for r in txn]
         return self.runInteraction("get_users_in_room", f)
 
-    @cached(max_entries=100000, iterable=True)
+    @cached(max_entries=100000)
     def get_room_summary(self, room_id):
         """ Get the details of a room roughly suitable for use by the room
         summary extension to /sync. Useful when lazy loading room members.
@@ -99,45 +99,49 @@ class RoomMemberWorkerStore(EventsWorkerStore):
                 dict of membership states, pointing to a MemberSummary named tuple.
         """
 
-        def f(txn):
+        def _get_room_summary_txn(txn):
             # first get counts.
             # We do this all in one transaction to keep the cache small.
             # FIXME: get rid of this when we have room_stats
-            sql = (
-                "SELECT count(*), m.membership FROM room_memberships as m"
-                " INNER JOIN current_state_events as c"
-                " ON m.event_id = c.event_id"
-                " AND m.room_id = c.room_id"
-                " AND m.user_id = c.state_key"
-                " WHERE c.type = 'm.room.member' AND c.room_id = ?"
-                " GROUP BY m.membership"
-            )
+            sql = """
+                SELECT count(*), m.membership FROM room_memberships as m
+                 INNER JOIN current_state_events as c
+                 ON m.event_id = c.event_id
+                 AND m.room_id = c.room_id
+                 AND m.user_id = c.state_key
+                 WHERE c.type = 'm.room.member' AND c.room_id = ?
+                 GROUP BY m.membership
+            """
 
             txn.execute(sql, (room_id,))
             res = {}
-            for r in txn:
-                summary = res.setdefault(to_ascii(r[1]), MemberSummary([], r[0]))
-
-            sql = (
-                "SELECT m.user_id, m.membership, m.event_id "
-                "FROM room_memberships as m"
-                " INNER JOIN current_state_events as c"
-                " ON m.event_id = c.event_id"
-                " AND m.room_id = c.room_id"
-                " AND m.user_id = c.state_key"
-                " WHERE c.type = 'm.room.member' AND c.room_id = ? limit ?"
-            )
+            for count, membership in txn:
+                summary = res.setdefault(to_ascii(membership), MemberSummary([], count))
+
+            sql = """
+                SELECT m.user_id, m.membership, m.event_id
+                FROM room_memberships as m
+                 INNER JOIN current_state_events as c
+                 ON m.event_id = c.event_id
+                 AND m.room_id = c.room_id
+                 AND m.user_id = c.state_key
+                 WHERE c.type = 'm.room.member' AND c.room_id = ?
+                 ORDER BY (CASE m.membership WHEN '?' THEN 1 WHEN '?' THEN 2 ELSE 3) ASC
+                 LIMIT ?
+            """
 
             # 6 is 5 (number of heroes) plus 1, in case one of them is the calling user.
-            txn.execute(sql, (room_id, 6))
-            for r in txn:
-                summary = res.get(to_ascii(r[1]))
+            txn.execute(sql, (room_id, 6, Membership.JOIN, Membership.INVITE))
+            for user_id, membership, event_id in txn:
+                summary = res[to_ascii(membership)]
+                # we will always have a summary for this membership type at this
+                # point given the summary currently contains the counts.
                 members = summary.members
-                members.append((to_ascii(r[0]), to_ascii(r[2])))
+                members.append((to_ascii(user_id), to_ascii(event_id)))
 
             return res
 
-        return self.runInteraction("get_room_summary", f)
+        return self.runInteraction("get_room_summary", _get_room_summary_txn)
 
     @cached()
     def get_invited_rooms_for_user(self, user_id):