From 894350306b899ed873608633ccab6ab179477fad Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 20 Oct 2022 08:15:27 -0400 Subject: [PATCH 01/11] Refactor to return early if no relation. --- synapse/storage/databases/main/events.py | 43 +++++++++++++----------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index 6698cbf66495..39a8f19df85a 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -2028,7 +2028,7 @@ def _handle_redact_relations( redacted_event_id: The event that was redacted. """ - # Fetch the current relation of the event being redacted. + # Fetch the relation of the event being redacted. redacted_relates_to = self.db_pool.simple_select_one_onecol_txn( txn, table="event_relations", @@ -2036,26 +2036,29 @@ def _handle_redact_relations( retcol="relates_to_id", allow_none=True, ) + # Nothing to do if no relation is found. + if redacted_relates_to is None: + return + # Any relation information for the related event must be cleared. - if redacted_relates_to is not None: - self.store._invalidate_cache_and_stream( - txn, self.store.get_relations_for_event, (redacted_relates_to,) - ) - self.store._invalidate_cache_and_stream( - txn, self.store.get_aggregation_groups_for_event, (redacted_relates_to,) - ) - self.store._invalidate_cache_and_stream( - txn, self.store.get_applicable_edit, (redacted_relates_to,) - ) - self.store._invalidate_cache_and_stream( - txn, self.store.get_thread_summary, (redacted_relates_to,) - ) - self.store._invalidate_cache_and_stream( - txn, self.store.get_thread_participated, (redacted_relates_to,) - ) - self.store._invalidate_cache_and_stream( - txn, self.store.get_threads, (room_id,) - ) + self.store._invalidate_cache_and_stream( + txn, self.store.get_relations_for_event, (redacted_relates_to,) + ) + self.store._invalidate_cache_and_stream( + txn, self.store.get_aggregation_groups_for_event, (redacted_relates_to,) + ) + self.store._invalidate_cache_and_stream( + txn, self.store.get_applicable_edit, (redacted_relates_to,) + ) + self.store._invalidate_cache_and_stream( + txn, self.store.get_thread_summary, (redacted_relates_to,) + ) + self.store._invalidate_cache_and_stream( + txn, self.store.get_thread_participated, (redacted_relates_to,) + ) + self.store._invalidate_cache_and_stream( + txn, self.store.get_threads, (room_id,) + ) self.db_pool.simple_delete_txn( txn, table="event_relations", keyvalues={"event_id": redacted_event_id} From bd0b13225c74a882e8b296e2ef9362caadf5ddc6 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 20 Oct 2022 08:23:24 -0400 Subject: [PATCH 02/11] Return the relation type of redacted events. --- synapse/storage/databases/main/events.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index 39a8f19df85a..19a30a3c6aaf 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -2029,18 +2029,20 @@ def _handle_redact_relations( """ # Fetch the relation of the event being redacted. - redacted_relates_to = self.db_pool.simple_select_one_onecol_txn( + row = self.db_pool.simple_select_one_txn( txn, table="event_relations", keyvalues={"event_id": redacted_event_id}, - retcol="relates_to_id", + retcols=("relates_to_id", "relation_type"), allow_none=True, ) # Nothing to do if no relation is found. - if redacted_relates_to is None: + if row is None: return # Any relation information for the related event must be cleared. + redacted_relates_to = row["relates_to_id"] + rel_type = row["relation_type"] self.store._invalidate_cache_and_stream( txn, self.store.get_relations_for_event, (redacted_relates_to,) ) From fc172150170f84e2964ad67fdce94d3e5e827e37 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 20 Oct 2022 08:33:06 -0400 Subject: [PATCH 03/11] Invalidate caches based on relation type. --- synapse/storage/databases/main/events.py | 33 +++++++++++++----------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index 19a30a3c6aaf..0f0b11eea614 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -2046,21 +2046,24 @@ def _handle_redact_relations( self.store._invalidate_cache_and_stream( txn, self.store.get_relations_for_event, (redacted_relates_to,) ) - self.store._invalidate_cache_and_stream( - txn, self.store.get_aggregation_groups_for_event, (redacted_relates_to,) - ) - self.store._invalidate_cache_and_stream( - txn, self.store.get_applicable_edit, (redacted_relates_to,) - ) - self.store._invalidate_cache_and_stream( - txn, self.store.get_thread_summary, (redacted_relates_to,) - ) - self.store._invalidate_cache_and_stream( - txn, self.store.get_thread_participated, (redacted_relates_to,) - ) - self.store._invalidate_cache_and_stream( - txn, self.store.get_threads, (room_id,) - ) + if rel_type == RelationTypes.ANNOTATION: + self.store._invalidate_cache_and_stream( + txn, self.store.get_aggregation_groups_for_event, (redacted_relates_to,) + ) + if rel_type == RelationTypes.REPLACE: + self.store._invalidate_cache_and_stream( + txn, self.store.get_applicable_edit, (redacted_relates_to,) + ) + if rel_type == RelationTypes.THREAD: + self.store._invalidate_cache_and_stream( + txn, self.store.get_thread_summary, (redacted_relates_to,) + ) + self.store._invalidate_cache_and_stream( + txn, self.store.get_thread_participated, (redacted_relates_to,) + ) + self.store._invalidate_cache_and_stream( + txn, self.store.get_threads, (room_id,) + ) self.db_pool.simple_delete_txn( txn, table="event_relations", keyvalues={"event_id": redacted_event_id} From d92d8ff439c7f66b7b8336d6bde89f6f69b4a4ae Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 20 Oct 2022 08:35:47 -0400 Subject: [PATCH 04/11] Re-order some code. --- synapse/storage/databases/main/events.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index 0f0b11eea614..05132793494b 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -2040,9 +2040,13 @@ def _handle_redact_relations( if row is None: return - # Any relation information for the related event must be cleared. redacted_relates_to = row["relates_to_id"] rel_type = row["relation_type"] + self.db_pool.simple_delete_txn( + txn, table="event_relations", keyvalues={"event_id": redacted_event_id} + ) + + # Any relation information for the related event must be cleared. self.store._invalidate_cache_and_stream( txn, self.store.get_relations_for_event, (redacted_relates_to,) ) @@ -2065,10 +2069,6 @@ def _handle_redact_relations( txn, self.store.get_threads, (room_id,) ) - self.db_pool.simple_delete_txn( - txn, table="event_relations", keyvalues={"event_id": redacted_event_id} - ) - def _store_room_topic_txn(self, txn: LoggingTransaction, event: EventBase) -> None: if isinstance(event.content.get("topic"), str): self.store_event_search_txn( From 524dab2dfa4a7897abc1eb019cb6f54156ffc561 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 20 Oct 2022 08:55:19 -0400 Subject: [PATCH 05/11] Properly invalidate the threads table. --- synapse/storage/databases/main/events.py | 31 ++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index 05132793494b..77f9e648333a 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -2069,6 +2069,37 @@ def _handle_redact_relations( txn, self.store.get_threads, (room_id,) ) + # Find the new latest event in the thread. + sql = """ + SELECT event_id, topological_ordering, stream_ordering + FROM event_relations + INNER JOIN events USING (event_id) + WHERE relates_to_id = ? AND relation_type = ? + ORDER BY topological_ordering DESC, stream_ordering DESC + LIMIT 1 + """ + txn.execute(sql, (redacted_relates_to, RelationTypes.THREAD)) + + row = txn.fetchone() + # If a new latest event is found, update the threads table. + if row: + self.db_pool.simple_upsert_txn( + txn, + table="threads", + keyvalues={"room_id": room_id, "thread_id": redacted_relates_to}, + values={ + "latest_event_id": row[0], + "topological_ordering": row[1], + "stream_ordering": row[2], + }, + ) + + # Otherwise, delete the thread: it no longer exists. + else: + self.db_pool.simple_delete_one_txn( + txn, table="threads", keyvalues={"thread_id": redacted_relates_to} + ) + def _store_room_topic_txn(self, txn: LoggingTransaction, event: EventBase) -> None: if isinstance(event.content.get("topic"), str): self.store_event_search_txn( From 76e049c64d3097998240e49d894f50ae14dd91f1 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 20 Oct 2022 09:16:51 -0400 Subject: [PATCH 06/11] Update tests. --- tests/rest/client/test_relations.py | 52 +++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tests/rest/client/test_relations.py b/tests/rest/client/test_relations.py index ddf315b894c9..91f3cb5ea667 100644 --- a/tests/rest/client/test_relations.py +++ b/tests/rest/client/test_relations.py @@ -1600,6 +1600,23 @@ def test_redact_relation_thread(self) -> None: to_redact_event_id, ) + # Request the threads in the room. + channel = self.make_request( + "GET", + f"/_matrix/client/v1/rooms/{self.room}/threads", + access_token=self.user_token, + ) + self.assertEquals(200, channel.code, channel.json_body) + threads = channel.json_body["chunk"] + # There should be one thread, the latest event is the event that will be redacted. + self.assertEqual([t["event_id"] for t in threads], [self.parent_id]) + self.assertEqual( + threads[0]["unsigned"]["m.relations"][RelationTypes.THREAD]["latest_event"][ + "event_id" + ], + to_redact_event_id, + ) + # Redact one of the reactions. self._redact(to_redact_event_id) @@ -1620,6 +1637,41 @@ def test_redact_relation_thread(self) -> None: unredacted_event_id, ) + # Request the threads in the room again. + channel = self.make_request( + "GET", + f"/_matrix/client/v1/rooms/{self.room}/threads", + access_token=self.user_token, + ) + self.assertEquals(200, channel.code, channel.json_body) + threads = channel.json_body["chunk"] + # There should still be one thread, but the latest event is the unredacted event. + self.assertEqual([t["event_id"] for t in threads], [self.parent_id]) + self.assertEqual( + threads[0]["unsigned"]["m.relations"][RelationTypes.THREAD]["latest_event"][ + "event_id" + ], + unredacted_event_id, + ) + + # Redact the remaining event in the thread. + self._redact(unredacted_event_id) + + # The event should no longer be considered a thread. + event_ids = self._get_related_events() + relations = self._get_bundled_aggregations() + self.assertEquals(event_ids, []) + self.assertEquals(relations, {}) + + channel = self.make_request( + "GET", + f"/_matrix/client/v1/rooms/{self.room}/threads", + access_token=self.user_token, + ) + self.assertEquals(200, channel.code, channel.json_body) + # There should still be one thread, but the latest event is the unredacted event. + self.assertEqual([], channel.json_body["chunk"]) + def test_redact_parent_edit(self) -> None: """Test that edits of an event are redacted when the original event is redacted. From b7687325fe9a369cb7b9bebed5221ff34dea2cdd Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 20 Oct 2022 11:59:07 -0400 Subject: [PATCH 07/11] Create a helper method. --- tests/rest/client/test_relations.py | 63 +++++++++++++---------------- 1 file changed, 27 insertions(+), 36 deletions(-) diff --git a/tests/rest/client/test_relations.py b/tests/rest/client/test_relations.py index 91f3cb5ea667..c3855b373960 100644 --- a/tests/rest/client/test_relations.py +++ b/tests/rest/client/test_relations.py @@ -1523,6 +1523,26 @@ def _redact(self, event_id: str) -> None: ) self.assertEqual(200, channel.code, channel.json_body) + def _get_threads(self) -> List[Tuple[str, str]]: + """Request the threads in the room and returns a list of thread ID and latest event ID.""" + # Request the threads in the room. + channel = self.make_request( + "GET", + f"/_matrix/client/v1/rooms/{self.room}/threads", + access_token=self.user_token, + ) + self.assertEquals(200, channel.code, channel.json_body) + threads = channel.json_body["chunk"] + return [ + ( + t["event_id"], + t["unsigned"]["m.relations"][RelationTypes.THREAD]["latest_event"][ + "event_id" + ], + ) + for t in threads + ] + def test_redact_relation_annotation(self) -> None: """ Test that annotations of an event are properly handled after the @@ -1601,21 +1621,9 @@ def test_redact_relation_thread(self) -> None: ) # Request the threads in the room. - channel = self.make_request( - "GET", - f"/_matrix/client/v1/rooms/{self.room}/threads", - access_token=self.user_token, - ) - self.assertEquals(200, channel.code, channel.json_body) - threads = channel.json_body["chunk"] + threads = self._get_threads() # There should be one thread, the latest event is the event that will be redacted. - self.assertEqual([t["event_id"] for t in threads], [self.parent_id]) - self.assertEqual( - threads[0]["unsigned"]["m.relations"][RelationTypes.THREAD]["latest_event"][ - "event_id" - ], - to_redact_event_id, - ) + self.assertEqual(threads, [(self.parent_id, to_redact_event_id)]) # Redact one of the reactions. self._redact(to_redact_event_id) @@ -1638,21 +1646,9 @@ def test_redact_relation_thread(self) -> None: ) # Request the threads in the room again. - channel = self.make_request( - "GET", - f"/_matrix/client/v1/rooms/{self.room}/threads", - access_token=self.user_token, - ) - self.assertEquals(200, channel.code, channel.json_body) - threads = channel.json_body["chunk"] + threads = self._get_threads() # There should still be one thread, but the latest event is the unredacted event. - self.assertEqual([t["event_id"] for t in threads], [self.parent_id]) - self.assertEqual( - threads[0]["unsigned"]["m.relations"][RelationTypes.THREAD]["latest_event"][ - "event_id" - ], - unredacted_event_id, - ) + self.assertEqual(threads, [(self.parent_id, unredacted_event_id)]) # Redact the remaining event in the thread. self._redact(unredacted_event_id) @@ -1663,14 +1659,9 @@ def test_redact_relation_thread(self) -> None: self.assertEquals(event_ids, []) self.assertEquals(relations, {}) - channel = self.make_request( - "GET", - f"/_matrix/client/v1/rooms/{self.room}/threads", - access_token=self.user_token, - ) - self.assertEquals(200, channel.code, channel.json_body) - # There should still be one thread, but the latest event is the unredacted event. - self.assertEqual([], channel.json_body["chunk"]) + threads = self._get_threads() + # There should be no threads. + self.assertEqual(threads, []) def test_redact_parent_edit(self) -> None: """Test that edits of an event are redacted when the original event From 85fbad79153a33cfc058876f4fbba004c57ec921 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 20 Oct 2022 12:25:41 -0400 Subject: [PATCH 08/11] Augment tests for redacting an event in the middle of a thread. --- tests/rest/client/test_relations.py | 105 ++++++++++++++-------------- 1 file changed, 53 insertions(+), 52 deletions(-) diff --git a/tests/rest/client/test_relations.py b/tests/rest/client/test_relations.py index c3855b373960..e3d801f7a88f 100644 --- a/tests/rest/client/test_relations.py +++ b/tests/rest/client/test_relations.py @@ -1587,81 +1587,82 @@ def test_redact_relation_thread(self) -> None: The redacted event should not be included in bundled aggregations or the response to relations. """ - channel = self._send_relation( - RelationTypes.THREAD, - EventTypes.Message, - content={"body": "reply 1", "msgtype": "m.text"}, - ) - unredacted_event_id = channel.json_body["event_id"] - - # Note that the *last* event in the thread is redacted, as that gets - # included in the bundled aggregation. - channel = self._send_relation( - RelationTypes.THREAD, - EventTypes.Message, - content={"body": "reply 2", "msgtype": "m.text"}, - ) - to_redact_event_id = channel.json_body["event_id"] + # Create a thread with a few events in it. + thread_replies = [] + for i in range(3): + channel = self._send_relation( + RelationTypes.THREAD, + EventTypes.Message, + content={"body": f"reply {i}", "msgtype": "m.text"}, + ) + thread_replies.append(channel.json_body["event_id"]) - # Both relations exist. - event_ids = self._get_related_events() + ################################################## + # Check the test data is configured as expected. # + ################################################## + self.assertEquals(self._get_related_events(), list(reversed(thread_replies))) relations = self._get_bundled_aggregations() - self.assertEquals(event_ids, [to_redact_event_id, unredacted_event_id]) self.assertDictContainsSubset( - { - "count": 2, - "current_user_participated": True, - }, + {"count": 3, "current_user_participated": True}, relations[RelationTypes.THREAD], ) - # And the latest event returned is the event that will be redacted. + # The latest event is the last sent event. self.assertEqual( relations[RelationTypes.THREAD]["latest_event"]["event_id"], - to_redact_event_id, + thread_replies[-1], ) - # Request the threads in the room. - threads = self._get_threads() # There should be one thread, the latest event is the event that will be redacted. - self.assertEqual(threads, [(self.parent_id, to_redact_event_id)]) + self.assertEqual(self._get_threads(), [(self.parent_id, thread_replies[-1])]) - # Redact one of the reactions. - self._redact(to_redact_event_id) + ########################## + # Redact the last event. # + ########################## + self._redact(thread_replies.pop()) - # The unredacted relation should still exist. - event_ids = self._get_related_events() + # The thread should still exist, but the latest event should be updated. + self.assertEquals(self._get_related_events(), list(reversed(thread_replies))) relations = self._get_bundled_aggregations() - self.assertEquals(event_ids, [unredacted_event_id]) self.assertDictContainsSubset( - { - "count": 1, - "current_user_participated": True, - }, + {"count": 2, "current_user_participated": True}, relations[RelationTypes.THREAD], ) - # And the latest event is now the unredacted event. + # And the latest event is the last unredacted event. self.assertEqual( relations[RelationTypes.THREAD]["latest_event"]["event_id"], - unredacted_event_id, + thread_replies[-1], ) + self.assertEqual(self._get_threads(), [(self.parent_id, thread_replies[-1])]) - # Request the threads in the room again. - threads = self._get_threads() - # There should still be one thread, but the latest event is the unredacted event. - self.assertEqual(threads, [(self.parent_id, unredacted_event_id)]) + ########################################### + # Redact the *first* event in the thread. # + ########################################### + self._redact(thread_replies.pop(0)) - # Redact the remaining event in the thread. - self._redact(unredacted_event_id) - - # The event should no longer be considered a thread. - event_ids = self._get_related_events() + # Nothing should have changed (except the thread count). + self.assertEquals(self._get_related_events(), thread_replies) relations = self._get_bundled_aggregations() - self.assertEquals(event_ids, []) - self.assertEquals(relations, {}) + self.assertDictContainsSubset( + {"count": 1, "current_user_participated": True}, + relations[RelationTypes.THREAD], + ) + # And the latest event is the last unredacted event. + self.assertEqual( + relations[RelationTypes.THREAD]["latest_event"]["event_id"], + thread_replies[-1], + ) + self.assertEqual(self._get_threads(), [(self.parent_id, thread_replies[-1])]) - threads = self._get_threads() - # There should be no threads. - self.assertEqual(threads, []) + #################################### + # Redact the last remaining event. # + #################################### + self._redact(thread_replies.pop(0)) + self.assertEquals(thread_replies, []) + + # The event should no longer be considered a thread. + self.assertEquals(self._get_related_events(), []) + self.assertEquals(self._get_bundled_aggregations(), {}) + self.assertEqual(self._get_threads(), []) def test_redact_parent_edit(self) -> None: """Test that edits of an event are redacted when the original event From bbe251b78872730f36917faa40bd881796fc2ea8 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 20 Oct 2022 12:27:18 -0400 Subject: [PATCH 09/11] Clarify comment. --- synapse/storage/databases/main/events.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index 77f9e648333a..80ccf98d2cb7 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -2081,7 +2081,9 @@ def _handle_redact_relations( txn.execute(sql, (redacted_relates_to, RelationTypes.THREAD)) row = txn.fetchone() - # If a new latest event is found, update the threads table. + # If a latest event is found, update the threads table, this might + # be the same current latest event (if an earlier event in the thread + # was redacted). if row: self.db_pool.simple_upsert_txn( txn, From 55b4809469ca6febf6259c15d101630069e45a65 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 20 Oct 2022 12:57:07 -0400 Subject: [PATCH 10/11] Newsfragment --- changelog.d/14248.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/14248.bugfix diff --git a/changelog.d/14248.bugfix b/changelog.d/14248.bugfix new file mode 100644 index 000000000000..203c52c16b1c --- /dev/null +++ b/changelog.d/14248.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in Synapse 1.70.0rc1 where the information returned from the `/threads` API could be stale when threaded events are redacted. From 59ac7c7f373bf2b2705614bde62644c2eff96d62 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 20 Oct 2022 13:41:33 -0400 Subject: [PATCH 11/11] Lint --- synapse/storage/databases/main/events.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index 80ccf98d2cb7..00880bb37dfc 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -2080,19 +2080,19 @@ def _handle_redact_relations( """ txn.execute(sql, (redacted_relates_to, RelationTypes.THREAD)) - row = txn.fetchone() # If a latest event is found, update the threads table, this might # be the same current latest event (if an earlier event in the thread # was redacted). - if row: + latest_event_row = txn.fetchone() + if latest_event_row: self.db_pool.simple_upsert_txn( txn, table="threads", keyvalues={"room_id": room_id, "thread_id": redacted_relates_to}, values={ - "latest_event_id": row[0], - "topological_ordering": row[1], - "stream_ordering": row[2], + "latest_event_id": latest_event_row[0], + "topological_ordering": latest_event_row[1], + "stream_ordering": latest_event_row[2], }, )