Apply suggestions from code review

Comment updates, docstring, remove extra logs

Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>

synapse/events/__init__.py

@@ -405,14 +405,15 @@ class FrozenEventV2(EventBase):
             for name, sigs in event_dict.pop("signatures", {}).items()
         }
 
-        # An event should only have an event_id at this point if it's for a v1/v2 like room.
-        # In future room versions, the `event_id` is derived from the event canonical JSON.
-
-        # So if we see a `event_id` but the room version doesn't support
-        # v1/v2 events, then it's invalid and we should reject it.
+        # In newer room versions (3+), the `event_id` is derived from a hash of the
+        # event canonical JSON, so it should not be explicitly provided in the event
+        # dictionary.
+        #
+        # If we see an `event_id` in a newer room version, then it's an invalid event
+        # and we should reject it.
         assert (
             "event_id" not in event_dict
-        ), "Event ID should not be supplied in non-v1/v2 room"
+        ), "Event ID should not be provided for events in non-v1/v2 room versions"
 
         unsigned = dict(event_dict.pop("unsigned", {}))
 

synapse/federation/federation_server.py

@@ -479,9 +479,11 @@ class FederationServer(FederationBase):
             try:
                 event = event_from_pdu_json(p, room_version)
             except Exception as e:
-                # We can only provide feedback to the federating server if we can determine what the event_id is
-                # but since we failed to parse the event, we can't derive the `event_id` so there is nothing
-                # to use as the `pdu_results` key. Best we can do is just log for our own record and move on.
+                # We can only provide feedback to the federating server if we can
+                # determine what the `event_id` is but since we failed to parse the
+                # event, we can't derive the `event_id` so there is nothing to use as
+                # the `pdu_results` key. Best we can do is just log for our own record
+                # and move on.
                 if possible_event_id != _UNKNOWN_EVENT_ID:
                     pdu_results[possible_event_id] = {
                         "error": f"Failed to convert JSON into event: {e}"

tests/federation/test_federation_server.py

@@ -88,6 +88,11 @@ class FederationServerTests(unittest.FederatingHomeserverTestCase):
         self.assertEqual(500, channel.code, channel.result)
 
     def test_accept_valid_pdus_and_ignore_invalid(self) -> None:
+        """
+        Test to make sure that old v1/v2 formatted events (that include `event_id`) are
+        rejected from a newer room version that don't support it but we still accept
+        properly formatted/valid events from the same batch.
+        """
         user = self.register_user("user1", "test")
         tok = self.login("user1", "test")
         room_id = self.helper.create_room_as("user1", tok=tok)
@@ -117,9 +122,8 @@ class FederationServerTests(unittest.FederatingHomeserverTestCase):
         event1_json = event1.get_pdu_json()
         event2_json = event2.get_pdu_json()
         event3_json = event3.get_pdu_json()
-        logging.info(event1_json)
 
-        # Purposely adding event id that shouldn't be there
+        # Purposely adding `event_id` that shouldn't be there
         event2_json["event_id"] = event2.event_id
 
         channel = self.make_signed_federation_request(
@@ -132,10 +136,12 @@ class FederationServerTests(unittest.FederatingHomeserverTestCase):
         # and that it indicates success for valid events
         pdus: JsonDict = body["pdus"]
         self.assertIncludes(
-            set(pdus.keys()), {event1.event_id, event2.event_id, event3.event_id}
+            set(pdus.keys()),
+            {event1.event_id, event2.event_id, event3.event_id},
+            exact=True,
         )
         self.assertEqual(pdus[event1.event_id], {})
-        self.assertNotEqual(body["pdus"][event2.event_id]["error"], "")
+        self.assertNotEqual(pdus[event2.event_id]["error"], "")
         self.assertEqual(pdus[event3.event_id], {})
 
         # Make sure other valid events from the send transaction were persisted successfully