From f9dd1c0e775e47683aff97a276db71a82205b23c Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 19 Sep 2022 10:31:53 -0400 Subject: [PATCH 01/10] Fix skipping items when paginating forward. --- synapse/storage/databases/main/relations.py | 6 +++++ tests/rest/client/test_relations.py | 27 +++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/synapse/storage/databases/main/relations.py b/synapse/storage/databases/main/relations.py index 7bd27790ebfe..8b5c6854fdd8 100644 --- a/synapse/storage/databases/main/relations.py +++ b/synapse/storage/databases/main/relations.py @@ -153,6 +153,12 @@ def _get_recent_references_for_event_txn( # If there are more events, generate the next pagination key. next_token = None if len(events) > limit and last_topo_id and last_stream_id: + # Due to how the pagination clause is generated, the stream ID is + # handled as an exclusive range when paginating forward. Correct + # for that here. + if direction == "f": + last_stream_id -= 1 + next_key = RoomStreamToken(last_topo_id, last_stream_id) if from_token: next_token = from_token.copy_and_replace( diff --git a/tests/rest/client/test_relations.py b/tests/rest/client/test_relations.py index 651f4f415d1d..875f12815edb 100644 --- a/tests/rest/client/test_relations.py +++ b/tests/rest/client/test_relations.py @@ -788,6 +788,7 @@ def test_basic_paginate_relations(self) -> None: channel.json_body["chunk"][0], ) + @unittest.override_config({"experimental_features": {"msc3715_enabled": True}}) def test_repeated_paginate_relations(self) -> None: """Test that if we paginate using a limit and tokens then we get the expected events. @@ -827,6 +828,32 @@ def test_repeated_paginate_relations(self) -> None: found_event_ids.reverse() self.assertEqual(found_event_ids, expected_event_ids) + # Test forward pagination. + prev_token: Optional[str] = "" + found_event_ids: List[str] = [] + for _ in range(20): + from_token = "" + if prev_token: + from_token = "&from=" + prev_token + + channel = self.make_request( + "GET", + f"/_matrix/client/v1/rooms/{self.room}/relations/{self.parent_id}?org.matrix.msc3715.dir=f&limit=1{from_token}", + access_token=self.user_token, + ) + self.assertEqual(200, channel.code, channel.json_body) + + found_event_ids.extend(e["event_id"] for e in channel.json_body["chunk"]) + next_batch = channel.json_body.get("next_batch") + + self.assertNotEqual(prev_token, next_batch) + prev_token = next_batch + + if not prev_token: + break + + self.assertEqual(found_event_ids, expected_event_ids) + def test_pagination_from_sync_and_messages(self) -> None: """Pagination tokens from /sync and /messages can be used to paginate /relations.""" channel = self._send_relation(RelationTypes.ANNOTATION, "m.reaction", "A") From 6fb149ba49fce1f93bbc30490d7fa32f72ca9e07 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 19 Sep 2022 10:32:15 -0400 Subject: [PATCH 02/10] Ensure the ordering of each page is correct. --- tests/rest/client/test_relations.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/rest/client/test_relations.py b/tests/rest/client/test_relations.py index 875f12815edb..219b06219f70 100644 --- a/tests/rest/client/test_relations.py +++ b/tests/rest/client/test_relations.py @@ -810,7 +810,7 @@ def test_repeated_paginate_relations(self) -> None: channel = self.make_request( "GET", - f"/_matrix/client/v1/rooms/{self.room}/relations/{self.parent_id}?limit=1{from_token}", + f"/_matrix/client/v1/rooms/{self.room}/relations/{self.parent_id}?limit=3{from_token}", access_token=self.user_token, ) self.assertEqual(200, channel.code, channel.json_body) @@ -838,7 +838,7 @@ def test_repeated_paginate_relations(self) -> None: channel = self.make_request( "GET", - f"/_matrix/client/v1/rooms/{self.room}/relations/{self.parent_id}?org.matrix.msc3715.dir=f&limit=1{from_token}", + f"/_matrix/client/v1/rooms/{self.room}/relations/{self.parent_id}?org.matrix.msc3715.dir=f&limit=3{from_token}", access_token=self.user_token, ) self.assertEqual(200, channel.code, channel.json_body) From 3361a341728b2c1db73fa38f2b1f1d4723b605bf Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 19 Sep 2022 10:36:01 -0400 Subject: [PATCH 03/10] Lint --- tests/rest/client/test_relations.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/rest/client/test_relations.py b/tests/rest/client/test_relations.py index 219b06219f70..d33e34d82957 100644 --- a/tests/rest/client/test_relations.py +++ b/tests/rest/client/test_relations.py @@ -829,8 +829,8 @@ def test_repeated_paginate_relations(self) -> None: self.assertEqual(found_event_ids, expected_event_ids) # Test forward pagination. - prev_token: Optional[str] = "" - found_event_ids: List[str] = [] + prev_token = "" + found_event_ids = [] for _ in range(20): from_token = "" if prev_token: From aff98ff17f5df502f656d2de4f459d91e61c4333 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 19 Sep 2022 10:38:11 -0400 Subject: [PATCH 04/10] Newsfragment --- changelog.d/13840.bugfix | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog.d/13840.bugfix diff --git a/changelog.d/13840.bugfix b/changelog.d/13840.bugfix new file mode 100644 index 000000000000..b0748ed495c5 --- /dev/null +++ b/changelog.d/13840.bugfix @@ -0,0 +1,3 @@ +Fix a bug introduced in Synapse v1.53.0 where the experimental implementation of +[MSC3715](https://github.com/matrix-org/matrix-spec-proposals/pull/3715) would give incorrect results when paginating +forward. From bbc754f15321c3e3c88fb5daf02a1602893a1631 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 21 Sep 2022 13:04:44 -0400 Subject: [PATCH 05/10] Clarify mypy needs. --- synapse/storage/databases/main/relations.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/relations.py b/synapse/storage/databases/main/relations.py index 8b5c6854fdd8..79a886873650 100644 --- a/synapse/storage/databases/main/relations.py +++ b/synapse/storage/databases/main/relations.py @@ -91,6 +91,9 @@ async def get_relations_for_event( # it. The `event_id` must match the `event.event_id`. assert event.event_id == event_id + # Ensure bad limits aren't being passed in. + assert limit >= 0 + where_clause = ["relates_to_id = ?", "room_id = ?"] where_args: List[Union[str, int]] = [event.event_id, room_id] is_redacted = event.internal_metadata.is_redacted() @@ -152,7 +155,11 @@ def _get_recent_references_for_event_txn( # If there are more events, generate the next pagination key. next_token = None - if len(events) > limit and last_topo_id and last_stream_id: + if len(events) > limit: + # If there are events we must have pulled at least one row. + assert last_topo_id is not None + assert last_stream_id is not None + # Due to how the pagination clause is generated, the stream ID is # handled as an exclusive range when paginating forward. Correct # for that here. From e863d15c2d1425da2e284d42c693a1080ea33f2a Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 21 Sep 2022 13:07:27 -0400 Subject: [PATCH 06/10] Deconstruct row. --- synapse/storage/databases/main/relations.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/storage/databases/main/relations.py b/synapse/storage/databases/main/relations.py index 79a886873650..952a9f9f07e0 100644 --- a/synapse/storage/databases/main/relations.py +++ b/synapse/storage/databases/main/relations.py @@ -145,13 +145,13 @@ def _get_recent_references_for_event_txn( last_topo_id = None last_stream_id = None events = [] - for row in txn: + for event_id, relation_type, sender, topo_ordering, stream_ordering in txn: # Do not include edits for redacted events as they leak event # content. - if not is_redacted or row[1] != RelationTypes.REPLACE: - events.append(_RelatedEvent(row[0], row[2])) - last_topo_id = row[3] - last_stream_id = row[4] + if not is_redacted or relation_type != RelationTypes.REPLACE: + events.append(_RelatedEvent(event_id, sender)) + last_topo_id = topo_ordering + last_stream_id = stream_ordering # If there are more events, generate the next pagination key. next_token = None From 8828fa7c03c1ffe4d6186262a40dd7b901cad29d Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 21 Sep 2022 13:19:44 -0400 Subject: [PATCH 07/10] Fix changelog. --- changelog.d/13840.bugfix | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/changelog.d/13840.bugfix b/changelog.d/13840.bugfix index b0748ed495c5..0f014439a8ed 100644 --- a/changelog.d/13840.bugfix +++ b/changelog.d/13840.bugfix @@ -1,3 +1 @@ -Fix a bug introduced in Synapse v1.53.0 where the experimental implementation of -[MSC3715](https://github.com/matrix-org/matrix-spec-proposals/pull/3715) would give incorrect results when paginating -forward. +Fix a bug introduced in Synapse v1.53.0 where the experimental implementation of [MSC3715](https://github.com/matrix-org/matrix-spec-proposals/pull/3715) would give incorrect results when paginating forward. From 8ce01fbfcd976c541ba3f61e2a6eab8e55b136f2 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 21 Sep 2022 14:34:40 -0400 Subject: [PATCH 08/10] Copy some code out of paginate_room_events. --- synapse/storage/databases/main/relations.py | 35 +++++++++++---------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/synapse/storage/databases/main/relations.py b/synapse/storage/databases/main/relations.py index 952a9f9f07e0..8ff9c36aa9a8 100644 --- a/synapse/storage/databases/main/relations.py +++ b/synapse/storage/databases/main/relations.py @@ -51,6 +51,8 @@ class _RelatedEvent: event_id: str # The sender of the related event. sender: str + topological_ordering: Optional[int] + stream_ordering: int class RelationsWorkerStore(SQLBaseStore): @@ -142,31 +144,32 @@ def _get_recent_references_for_event_txn( ) -> Tuple[List[_RelatedEvent], Optional[StreamToken]]: txn.execute(sql, where_args + [limit + 1]) - last_topo_id = None - last_stream_id = None events = [] for event_id, relation_type, sender, topo_ordering, stream_ordering in txn: # Do not include edits for redacted events as they leak event # content. if not is_redacted or relation_type != RelationTypes.REPLACE: - events.append(_RelatedEvent(event_id, sender)) - last_topo_id = topo_ordering - last_stream_id = stream_ordering + events.append( + _RelatedEvent(event_id, sender, topo_ordering, stream_ordering) + ) - # If there are more events, generate the next pagination key. + # If there are more events, generate the next pagination key from the + # last event returned. next_token = None if len(events) > limit: - # If there are events we must have pulled at least one row. - assert last_topo_id is not None - assert last_stream_id is not None - - # Due to how the pagination clause is generated, the stream ID is - # handled as an exclusive range when paginating forward. Correct - # for that here. - if direction == "f": - last_stream_id -= 1 + events = events[:limit] + + topo = events[-1].topological_ordering + toke = events[-1].stream_ordering + if direction == "b": + # Tokens are positions between events. + # This token points *after* the last event in the chunk. + # We need it to point to the event before it in the chunk + # when we are going backwards so we subtract one from the + # stream part. + toke -= 1 + next_key = RoomStreamToken(topo, toke) - next_key = RoomStreamToken(last_topo_id, last_stream_id) if from_token: next_token = from_token.copy_and_replace( StreamKeyType.ROOM, next_key From 408e4e0ee02a7ab5e0eee31001e85bea102f9767 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 22 Sep 2022 08:10:24 -0400 Subject: [PATCH 09/10] Fix typo. --- synapse/storage/databases/main/relations.py | 6 +++--- synapse/storage/databases/main/stream.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/storage/databases/main/relations.py b/synapse/storage/databases/main/relations.py index 8ff9c36aa9a8..b68384968f4d 100644 --- a/synapse/storage/databases/main/relations.py +++ b/synapse/storage/databases/main/relations.py @@ -160,15 +160,15 @@ def _get_recent_references_for_event_txn( events = events[:limit] topo = events[-1].topological_ordering - toke = events[-1].stream_ordering + token = events[-1].stream_ordering if direction == "b": # Tokens are positions between events. # This token points *after* the last event in the chunk. # We need it to point to the event before it in the chunk # when we are going backwards so we subtract one from the # stream part. - toke -= 1 - next_key = RoomStreamToken(topo, toke) + token -= 1 + next_key = RoomStreamToken(topo, token) if from_token: next_token = from_token.copy_and_replace( diff --git a/synapse/storage/databases/main/stream.py b/synapse/storage/databases/main/stream.py index 3f9bfaeac5cb..530f04e149b3 100644 --- a/synapse/storage/databases/main/stream.py +++ b/synapse/storage/databases/main/stream.py @@ -1334,15 +1334,15 @@ def _paginate_room_events_txn( if rows: topo = rows[-1].topological_ordering - toke = rows[-1].stream_ordering + token = rows[-1].stream_ordering if direction == "b": # Tokens are positions between events. # This token points *after* the last event in the chunk. # We need it to point to the event before it in the chunk # when we are going backwards so we subtract one from the # stream part. - toke -= 1 - next_token = RoomStreamToken(topo, toke) + token -= 1 + next_token = RoomStreamToken(topo, token) else: # TODO (erikj): We should work out what to do here instead. next_token = to_token if to_token else from_token From 726983d3bb24435974e4d2055df544bc6632ba9d Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 22 Sep 2022 08:13:19 -0400 Subject: [PATCH 10/10] Add comment. --- synapse/storage/databases/main/relations.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/storage/databases/main/relations.py b/synapse/storage/databases/main/relations.py index b68384968f4d..898947af9536 100644 --- a/synapse/storage/databases/main/relations.py +++ b/synapse/storage/databases/main/relations.py @@ -157,6 +157,8 @@ def _get_recent_references_for_event_txn( # last event returned. next_token = None if len(events) > limit: + # Instead of using the last row (which tells us there is more + # data), use the last row to be returned. events = events[:limit] topo = events[-1].topological_ordering