Sort child events according to MSC1772 for the spaces summary API. (#9954)

This should help ensure that equivalent results are achieved between
homeservers querying for the summary of a space.

This implements modified MSC1772 rules, according to MSC2946.
The different is that the origin_server_ts of the m.room.create event
is not used as a tie-breaker since this might not be known if the
homeserver is not part of the room.

3 files changed, 151 insertions, 2 deletions

diff --git a/changelog.d/9954.feature b/changelog.d/9954.feature
new file mode 100644
index 0000000000..ce8874f810
--- /dev/null
+++ b/changelog.d/9954.feature
@@ -0,0 +1 @@
+Update support for [MSC2946](https://github.com/matrix-org/matrix-doc/pull/2946): Spaces Summary.
diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py
index 2e997841f1..e35d91832b 100644
--- a/synapse/handlers/space_summary.py
+++ b/synapse/handlers/space_summary.py
@@ -14,6 +14,7 @@
 
 import itertools
 import logging
+import re
 from collections import deque
 from typing import TYPE_CHECKING, Iterable, List, Optional, Sequence, Set, Tuple, cast
 
@@ -226,6 +227,23 @@ class SpaceSummaryHandler:
         suggested_only: bool,
         max_children: Optional[int],
     ) -> Tuple[Sequence[JsonDict], Sequence[JsonDict]]:
+        """
+        Generate a room entry and a list of event entries for a given room.
+
+        Args:
+            requester: The requesting user, or None if this is over federation.
+            room_id: The room ID to summarize.
+            suggested_only: True if only suggested children should be returned.
+                Otherwise, all children are returned.
+            max_children: The maximum number of children to return for this node.
+
+        Returns:
+            A tuple of:
+                An iterable of a single value of the room.
+
+                An iterable of the sorted children events. This may be limited
+                to a maximum size or may include all children.
+        """
         if not await self._is_room_accessible(room_id, requester):
             return (), ()
 
@@ -357,6 +375,18 @@ class SpaceSummaryHandler:
         return room_entry
 
     async def _get_child_events(self, room_id: str) -> Iterable[EventBase]:
+        """
+        Get the child events for a given room.
+
+        The returned results are sorted for stability.
+
+        Args:
+            room_id: The room id to get the children of.
+
+        Returns:
+            An iterable of sorted child events.
+        """
+
         # look for child rooms/spaces.
         current_state_ids = await self._store.get_current_state_ids(room_id)
 
@@ -370,8 +400,9 @@ class SpaceSummaryHandler:
             ]
         )
 
-        # filter out any events without a "via" (which implies it has been redacted)
-        return (e for e in events if _has_valid_via(e))
+        # filter out any events without a "via" (which implies it has been redacted),
+        # and order to ensure we return stable results.
+        return sorted(filter(_has_valid_via, events), key=_child_events_comparison_key)
 
 
 @attr.s(frozen=True, slots=True)
@@ -397,3 +428,39 @@ def _is_suggested_child_event(edge_event: EventBase) -> bool:
         return True
     logger.debug("Ignorning not-suggested child %s", edge_event.state_key)
     return False
+
+
+# Order may only contain characters in the range of \x20 (space) to \x7F (~).
+_INVALID_ORDER_CHARS_RE = re.compile(r"[^\x20-\x7F]")
+
+
+def _child_events_comparison_key(child: EventBase) -> Tuple[bool, Optional[str], str]:
+    """
+    Generate a value for comparing two child events for ordering.
+
+    The rules for ordering are supposed to be:
+
+    1. The 'order' key, if it is valid.
+    2. The 'origin_server_ts' of the 'm.room.create' event.
+    3. The 'room_id'.
+
+    But we skip step 2 since we may not have any state from the room.
+
+    Args:
+        child: The event for generating a comparison key.
+
+    Returns:
+        The comparison key as a tuple of:
+            False if the ordering is valid.
+            The ordering field.
+            The room ID.
+    """
+    order = child.content.get("order")
+    # If order is not a string or doesn't meet the requirements, ignore it.
+    if not isinstance(order, str):
+        order = None
+    elif len(order) > 50 or _INVALID_ORDER_CHARS_RE.search(order):
+        order = None
+
+    # Items without an order come last.
+    return (order is None, order, child.room_id)
diff --git a/tests/handlers/test_space_summary.py b/tests/handlers/test_space_summary.py
new file mode 100644
index 0000000000..2c5e81531b
--- /dev/null
+++ b/tests/handlers/test_space_summary.py
@@ -0,0 +1,81 @@
+#  Copyright 2021 The Matrix.org Foundation C.I.C.
+#
+#  Licensed under the Apache License, Version 2.0 (the "License");
+#  you may not use this file except in compliance with the License.
+#  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+from typing import Any, Optional
+from unittest import mock
+
+from synapse.handlers.space_summary import _child_events_comparison_key
+
+from tests import unittest
+
+
+def _create_event(room_id: str, order: Optional[Any] = None):
+    result = mock.Mock()
+    result.room_id = room_id
+    result.content = {}
+    if order is not None:
+        result.content["order"] = order
+    return result
+
+
+def _order(*events):
+    return sorted(events, key=_child_events_comparison_key)
+
+
+class TestSpaceSummarySort(unittest.TestCase):
+    def test_no_order_last(self):
+        """An event with no ordering is placed behind those with an ordering."""
+        ev1 = _create_event("!abc:test")
+        ev2 = _create_event("!xyz:test", "xyz")
+
+        self.assertEqual([ev2, ev1], _order(ev1, ev2))
+
+    def test_order(self):
+        """The ordering should be used."""
+        ev1 = _create_event("!abc:test", "xyz")
+        ev2 = _create_event("!xyz:test", "abc")
+
+        self.assertEqual([ev2, ev1], _order(ev1, ev2))
+
+    def test_order_room_id(self):
+        """Room ID is a tie-breaker for ordering."""
+        ev1 = _create_event("!abc:test", "abc")
+        ev2 = _create_event("!xyz:test", "abc")
+
+        self.assertEqual([ev1, ev2], _order(ev1, ev2))
+
+    def test_invalid_ordering_type(self):
+        """Invalid orderings are considered the same as missing."""
+        ev1 = _create_event("!abc:test", 1)
+        ev2 = _create_event("!xyz:test", "xyz")
+
+        self.assertEqual([ev2, ev1], _order(ev1, ev2))
+
+        ev1 = _create_event("!abc:test", {})
+        self.assertEqual([ev2, ev1], _order(ev1, ev2))
+
+        ev1 = _create_event("!abc:test", [])
+        self.assertEqual([ev2, ev1], _order(ev1, ev2))
+
+        ev1 = _create_event("!abc:test", True)
+        self.assertEqual([ev2, ev1], _order(ev1, ev2))
+
+    def test_invalid_ordering_value(self):
+        """Invalid orderings are considered the same as missing."""
+        ev1 = _create_event("!abc:test", "foo\n")
+        ev2 = _create_event("!xyz:test", "xyz")
+
+        self.assertEqual([ev2, ev1], _order(ev1, ev2))
+
+        ev1 = _create_event("!abc:test", "a" * 51)
+        self.assertEqual([ev2, ev1], _order(ev1, ev2))