Fix /room/.../event/... to return the *original* event after any edits (#12476)

This is what the MSC (now) requires. Fixes #10310.

Author: richvdh

changelog.d/12476.bugfix

@@ -0,0 +1 @@
+Fix a long-standing bug which incorrectly caused `GET /_matrix/client/r3/rooms/{roomId}/event/{eventId}` to return edited events rather than the original.

synapse/events/utils.py

@@ -402,17 +402,18 @@ def serialize_event(
         *,
         config: SerializeEventConfig = _DEFAULT_SERIALIZE_EVENT_CONFIG,
         bundle_aggregations: Optional[Dict[str, "BundledAggregations"]] = None,
+        apply_edits: bool = True,
     ) -> JsonDict:
         """Serializes a single event.
 
         Args:
             event: The event being serialized.
             time_now: The current time in milliseconds
             config: Event serialization config
-            bundle_aggregations: Whether to include the bundled aggregations for this
-                event. Only applies to non-state events. (State events never include
-                bundled aggregations.)
-
+            bundle_aggregations: A map from event_id to the aggregations to be bundled
+               into the event.
+            apply_edits: Whether the content of the event should be modified to reflect
+               any replacement in `bundle_aggregations[<event_id>].replace`.
         Returns:
             The serialized event
         """
@@ -430,8 +431,9 @@ def serialize_event(
                     event,
                     time_now,
                     config,
-                    bundle_aggregations[event.event_id],
+                    event_aggregations,
                     serialized_event,
+                    apply_edits=apply_edits,
                 )
 
         return serialized_event
@@ -470,6 +472,7 @@ def _inject_bundled_aggregations(
         config: SerializeEventConfig,
         aggregations: "BundledAggregations",
         serialized_event: JsonDict,
+        apply_edits: bool,
     ) -> None:
         """Potentially injects bundled aggregations into the unsigned portion of the serialized event.
 
@@ -479,7 +482,8 @@ def _inject_bundled_aggregations(
             aggregations: The bundled aggregation to serialize.
             serialized_event: The serialized event which may be modified.
             config: Event serialization config
-
+            apply_edits: Whether the content of the event should be modified to reflect
+               any replacement in `aggregations.replace`.
         """
         serialized_aggregations = {}
 
@@ -490,9 +494,10 @@ def _inject_bundled_aggregations(
             serialized_aggregations[RelationTypes.REFERENCE] = aggregations.references
 
         if aggregations.replace:
-            # If there is an edit, apply it to the event.
+            # If there is an edit, optionally apply it to the event.
             edit = aggregations.replace
-            self._apply_edit(event, serialized_event, edit)
+            if apply_edits:
+                self._apply_edit(event, serialized_event, edit)
 
             # Include information about it in the relations dict.
             serialized_aggregations[RelationTypes.REPLACE] = {

synapse/rest/client/room.py

@@ -669,8 +669,10 @@ async def on_GET(
             )
 
             time_now = self.clock.time_msec()
+            # per MSC2676, /rooms/{roomId}/event/{eventId}, should return the
+            # *original* event, rather than the edited version
             event_dict = self._event_serializer.serialize_event(
-                event, time_now, bundle_aggregations=aggregations
+                event, time_now, bundle_aggregations=aggregations, apply_edits=False
             )
             return 200, event_dict
 

tests/rest/client/test_relations.py

@@ -380,13 +380,16 @@ def assert_bundle(event_json: JsonDict) -> None:
                 {"event_id": edit_event_id, "sender": self.user_id}, m_replace_dict
             )
 
+        # /event should return the *original* event
         channel = self.make_request(
             "GET",
             f"/rooms/{self.room}/event/{self.parent_id}",
             access_token=self.user_token,
         )
         self.assertEqual(200, channel.code, channel.json_body)
-        self.assertEqual(channel.json_body["content"], new_body)
+        self.assertEqual(
+            channel.json_body["content"], {"body": "Hi!", "msgtype": "m.text"}
+        )
         assert_bundle(channel.json_body)
 
         # Request the room messages.
@@ -399,13 +402,15 @@ def assert_bundle(event_json: JsonDict) -> None:
         assert_bundle(self._find_event_in_chunk(channel.json_body["chunk"]))
 
         # Request the room context.
+        # /context should return the edited event.
         channel = self.make_request(
             "GET",
             f"/rooms/{self.room}/context/{self.parent_id}",
             access_token=self.user_token,
         )
         self.assertEqual(200, channel.code, channel.json_body)
         assert_bundle(channel.json_body["event"])
+        self.assertEqual(channel.json_body["event"]["content"], new_body)
 
         # Request sync, but limit the timeline so it becomes limited (and includes
         # bundled aggregations).
@@ -470,14 +475,14 @@ def test_multi_edit(self) -> None:
 
         channel = self.make_request(
             "GET",
-            f"/rooms/{self.room}/event/{self.parent_id}",
+            f"/rooms/{self.room}/context/{self.parent_id}",
             access_token=self.user_token,
         )
         self.assertEqual(200, channel.code, channel.json_body)
 
-        self.assertEqual(channel.json_body["content"], new_body)
+        self.assertEqual(channel.json_body["event"]["content"], new_body)
 
-        relations_dict = channel.json_body["unsigned"].get("m.relations")
+        relations_dict = channel.json_body["event"]["unsigned"].get("m.relations")
         self.assertIn(RelationTypes.REPLACE, relations_dict)
 
         m_replace_dict = relations_dict[RelationTypes.REPLACE]
@@ -492,10 +497,9 @@ def test_edit_reply(self) -> None:
         """Test that editing a reply works."""
 
         # Create a reply to edit.
+        original_body = {"msgtype": "m.text", "body": "A reply!"}
         channel = self._send_relation(
-            RelationTypes.REFERENCE,
-            "m.room.message",
-            content={"msgtype": "m.text", "body": "A reply!"},
+            RelationTypes.REFERENCE, "m.room.message", content=original_body
         )
         reply = channel.json_body["event_id"]
 
@@ -508,38 +512,54 @@ def test_edit_reply(self) -> None:
         )
         edit_event_id = channel.json_body["event_id"]
 
+        # /event returns the original event
         channel = self.make_request(
             "GET",
             f"/rooms/{self.room}/event/{reply}",
             access_token=self.user_token,
         )
         self.assertEqual(200, channel.code, channel.json_body)
+        event_result = channel.json_body
+        self.assertDictContainsSubset(original_body, event_result["content"])
 
-        # We expect to see the new body in the dict, as well as the reference
-        # metadata sill intact.
-        self.assertDictContainsSubset(new_body, channel.json_body["content"])
-        self.assertDictContainsSubset(
-            {
-                "m.relates_to": {
-                    "event_id": self.parent_id,
-                    "rel_type": "m.reference",
-                }
-            },
-            channel.json_body["content"],
+        # also check /context, which returns the *edited* event
+        channel = self.make_request(
+            "GET",
+            f"/rooms/{self.room}/context/{reply}",
+            access_token=self.user_token,
         )
+        self.assertEqual(200, channel.code, channel.json_body)
+        context_result = channel.json_body["event"]
 
-        # We expect that the edit relation appears in the unsigned relations
-        # section.
-        relations_dict = channel.json_body["unsigned"].get("m.relations")
-        self.assertIn(RelationTypes.REPLACE, relations_dict)
+        # check that the relations are correct for both APIs
+        for result_event_dict, desc in (
+            (event_result, "/event"),
+            (context_result, "/context"),
+        ):
+            # The reference metadata should still be intact.
+            self.assertDictContainsSubset(
+                {
+                    "m.relates_to": {
+                        "event_id": self.parent_id,
+                        "rel_type": "m.reference",
+                    }
+                },
+                result_event_dict["content"],
+                desc,
+            )
 
-        m_replace_dict = relations_dict[RelationTypes.REPLACE]
-        for key in ["event_id", "sender", "origin_server_ts"]:
-            self.assertIn(key, m_replace_dict)
+            # We expect that the edit relation appears in the unsigned relations
+            # section.
+            relations_dict = result_event_dict["unsigned"].get("m.relations")
+            self.assertIn(RelationTypes.REPLACE, relations_dict, desc)
 
-        self.assert_dict(
-            {"event_id": edit_event_id, "sender": self.user_id}, m_replace_dict
-        )
+            m_replace_dict = relations_dict[RelationTypes.REPLACE]
+            for key in ["event_id", "sender", "origin_server_ts"]:
+                self.assertIn(key, m_replace_dict, desc)
+
+            self.assert_dict(
+                {"event_id": edit_event_id, "sender": self.user_id}, m_replace_dict
+            )
 
     def test_edit_thread(self) -> None:
         """Test that editing a thread works."""
@@ -605,19 +625,31 @@ def test_edit_edit(self) -> None:
         )
 
         # Request the original event.
+        # /event should return the original event.
         channel = self.make_request(
             "GET",
             f"/rooms/{self.room}/event/{self.parent_id}",
             access_token=self.user_token,
         )
         self.assertEqual(200, channel.code, channel.json_body)
-        # The edit to the edit should be ignored.
-        self.assertEqual(channel.json_body["content"], new_body)
+        self.assertEqual(
+            channel.json_body["content"], {"body": "Hi!", "msgtype": "m.text"}
+        )
 
         # The relations information should not include the edit to the edit.
         relations_dict = channel.json_body["unsigned"].get("m.relations")
         self.assertIn(RelationTypes.REPLACE, relations_dict)
 
+        # /context should return the event updated for the *first* edit
+        # (The edit to the edit should be ignored.)
+        channel = self.make_request(
+            "GET",
+            f"/rooms/{self.room}/context/{self.parent_id}",
+            access_token=self.user_token,
+        )
+        self.assertEqual(200, channel.code, channel.json_body)
+        self.assertEqual(channel.json_body["event"]["content"], new_body)
+
         m_replace_dict = relations_dict[RelationTypes.REPLACE]
         for key in ["event_id", "sender", "origin_server_ts"]:
             self.assertIn(key, m_replace_dict)