From 56245bed6ccc8e464d292ad73f4f7ed8b18a7d68 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 31 Aug 2021 14:17:20 -0400 Subject: [PATCH 1/3] Consider the `origin_server_ts` of the `m.space.child` event when ordering rooms. --- changelog.d/10728.bugfix | 1 + synapse/handlers/room_summary.py | 13 ++++++------- tests/handlers/test_room_summary.py | 18 +++++++++++++----- 3 files changed, 20 insertions(+), 12 deletions(-) create mode 100644 changelog.d/10728.bugfix diff --git a/changelog.d/10728.bugfix b/changelog.d/10728.bugfix new file mode 100644 index 000000000000..f1612d3c088c --- /dev/null +++ b/changelog.d/10728.bugfix @@ -0,0 +1 @@ +Fix a bug where the ordering algorithm was skipping the `origin_server_ts` step in the spaces summary resulting in unstable room orderings. diff --git a/synapse/handlers/room_summary.py b/synapse/handlers/room_summary.py index 906985c754d9..0ec165efc81b 100644 --- a/synapse/handlers/room_summary.py +++ b/synapse/handlers/room_summary.py @@ -1139,25 +1139,24 @@ def _is_suggested_child_event(edge_event: EventBase) -> bool: _INVALID_ORDER_CHARS_RE = re.compile(r"[^\x20-\x7E]") -def _child_events_comparison_key(child: EventBase) -> Tuple[bool, Optional[str], str]: +def _child_events_comparison_key(child: EventBase) -> Tuple[bool, Optional[str], int, str]: """ Generate a value for comparing two child events for ordering. - The rules for ordering are supposed to be: + The rules for ordering are: 1. The 'order' key, if it is valid. - 2. The 'origin_server_ts' of the 'm.room.create' event. + 2. The 'origin_server_ts' of the 'm.space.child' 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 'order' field or None if it is not given or invalid. + The 'origin_server_ts' field. The room ID. """ order = child.content.get("order") @@ -1168,4 +1167,4 @@ def _child_events_comparison_key(child: EventBase) -> Tuple[bool, Optional[str], order = None # Items without an order come last. - return (order is None, order, child.room_id) + return (order is None, order, child.origin_server_ts, child.room_id) diff --git a/tests/handlers/test_room_summary.py b/tests/handlers/test_room_summary.py index ac800afa7d3e..449ba89e5a56 100644 --- a/tests/handlers/test_room_summary.py +++ b/tests/handlers/test_room_summary.py @@ -35,10 +35,11 @@ from tests import unittest -def _create_event(room_id: str, order: Optional[Any] = None): - result = mock.Mock() +def _create_event(room_id: str, order: Optional[Any] = None, origin_server_ts: int = 0): + result = mock.Mock(name=room_id) result.room_id = room_id result.content = {} + result.origin_server_ts = origin_server_ts if order is not None: result.content["order"] = order return result @@ -63,10 +64,17 @@ def test_order(self): self.assertEqual([ev2, ev1], _order(ev1, ev2)) + def test_order_origin_server_ts(self): + """Origin server is a tie-breaker for ordering.""" + ev1 = _create_event("!abc:test", origin_server_ts=10) + ev2 = _create_event("!xyz:test", origin_server_ts=30) + + self.assertEqual([ev1, ev2], _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") + """Room ID is a final tie-breaker for ordering.""" + ev1 = _create_event("!abc:test") + ev2 = _create_event("!xyz:test") self.assertEqual([ev1, ev2], _order(ev1, ev2)) From 7563b73702848eaafed422fcc626aa3287d801bb Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 31 Aug 2021 15:05:00 -0400 Subject: [PATCH 2/3] Lint --- synapse/handlers/room_summary.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/room_summary.py b/synapse/handlers/room_summary.py index 0ec165efc81b..d1b6f3253e0c 100644 --- a/synapse/handlers/room_summary.py +++ b/synapse/handlers/room_summary.py @@ -1139,7 +1139,9 @@ def _is_suggested_child_event(edge_event: EventBase) -> bool: _INVALID_ORDER_CHARS_RE = re.compile(r"[^\x20-\x7E]") -def _child_events_comparison_key(child: EventBase) -> Tuple[bool, Optional[str], int, str]: +def _child_events_comparison_key( + child: EventBase, +) -> Tuple[bool, Optional[str], int, str]: """ Generate a value for comparing two child events for ordering. From 4bd8d45026d152c36c4db404618bfe7e621f3447 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 31 Aug 2021 15:06:28 -0400 Subject: [PATCH 3/3] Fix newsfile. --- changelog.d/{10728.bugfix => 10730.bugfix} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{10728.bugfix => 10730.bugfix} (100%) diff --git a/changelog.d/10728.bugfix b/changelog.d/10730.bugfix similarity index 100% rename from changelog.d/10728.bugfix rename to changelog.d/10730.bugfix