Fix deduplicating of membership events to not create unused state groups.

changelog.d/17164.bugfix

@@ -0,0 +1 @@
+Fix deduplicating of membership events to not create unused state groups.

synapse/handlers/message.py

@@ -496,13 +496,6 @@ def __init__(self, hs: "HomeServer"):
 
         self.room_prejoin_state_types = self.hs.config.api.room_prejoin_state
 
-        self.membership_types_to_include_profile_data_in = {
-            Membership.JOIN,
-            Membership.KNOCK,
-        }
-        if self.hs.config.server.include_profile_data_on_invite:
-            self.membership_types_to_include_profile_data_in.add(Membership.INVITE)
-
         self.send_event = ReplicationSendEventRestServlet.make_client(hs)
         self.send_events = ReplicationSendEventsRestServlet.make_client(hs)
 
@@ -672,29 +665,6 @@ async def create_event(
 
         self.validator.validate_builder(builder)
 
-        if builder.type == EventTypes.Member:
-            membership = builder.content.get("membership", None)
-            target = UserID.from_string(builder.state_key)
-
-            if membership in self.membership_types_to_include_profile_data_in:
-                # If event doesn't include a display name, add one.
-                profile = self.profile_handler
-                content = builder.content
-
-                try:
-                    if "displayname" not in content:
-                        displayname = await profile.get_displayname(target)
-                        if displayname is not None:
-                            content["displayname"] = displayname
-                    if "avatar_url" not in content:
-                        avatar_url = await profile.get_avatar_url(target)
-                        if avatar_url is not None:
-                            content["avatar_url"] = avatar_url
-                except Exception as e:
-                    logger.info(
-                        "Failed to get profile information for %r: %s", target, e
-                    )
-
         is_exempt = await self._is_exempt_from_privacy_policy(builder, requester)
         if require_consent and not is_exempt:
             await self.assert_accepted_privacy_policy(requester)

synapse/handlers/room_member.py

@@ -106,6 +106,13 @@ def __init__(self, hs: "HomeServer"):
         self.event_auth_handler = hs.get_event_auth_handler()
         self._worker_lock_handler = hs.get_worker_locks_handler()
 
+        self.membership_types_to_include_profile_data_in = {
+            Membership.JOIN,
+            Membership.KNOCK,
+        }
+        if self.hs.config.server.include_profile_data_on_invite:
+            self.membership_types_to_include_profile_data_in.add(Membership.INVITE)
+
         self.member_linearizer: Linearizer = Linearizer(name="member")
         self.member_as_limiter = Linearizer(max_count=10, name="member_as_limiter")
 
@@ -785,9 +792,8 @@ async def update_membership_locked(
         if (
             not self.allow_per_room_profiles and not is_requester_server_notices_user
         ) or requester.shadow_banned:
-            # Strip profile data, knowing that new profile data will be added to the
-            # event's content in event_creation_handler.create_event() using the target's
-            # global profile.
+            # Strip profile data, knowing that new profile data will be added to
+            # the event's content below using the target's global profile.
             content.pop("displayname", None)
             content.pop("avatar_url", None)
 
@@ -823,6 +829,29 @@ async def update_membership_locked(
         if action in ["kick", "unban"]:
             effective_membership_state = "leave"
 
+        if effective_membership_state not in Membership.LIST:
+            raise SynapseError(400, "Invalid membership key")
+
+        # Add profile data for joins etc, if no per-room profile.
+        if (
+            effective_membership_state
+            in self.membership_types_to_include_profile_data_in
+        ):
+            # If event doesn't include a display name, add one.
+            profile = self.profile_handler
+
+            try:
+                if "displayname" not in content:
+                    displayname = await profile.get_displayname(target)
+                    if displayname is not None:
+                        content["displayname"] = displayname
+                if "avatar_url" not in content:
+                    avatar_url = await profile.get_avatar_url(target)
+                    if avatar_url is not None:
+                        content["avatar_url"] = avatar_url
+            except Exception as e:
+                logger.info("Failed to get profile information for %r: %s", target, e)
+
         # if this is a join with a 3pid signature, we may need to turn a 3pid
         # invite into a normal invite before we can handle the join.
         if third_party_signed is not None:

tests/handlers/test_room_member.py

@@ -403,3 +403,24 @@ def test_rejoin_forgotten_by_user(self) -> None:
         self.assertFalse(
             self.get_success(self.store.did_forget(self.alice, self.room_id))
         )
+
+    def test_deduplicate_joins(self) -> None:
+        """Test that calling /join multiple times returns the same event ID,
+        without storing a new state group
+        """
+
+        self.helper.join(self.room_id, user=self.bob, tok=self.bob_token)
+
+        sql = "SELECT COUNT(*) FROM state_groups WHERE room_id = ?"
+        rows = self.get_success(
+            self.store.db_pool.execute("test_deduplicate_joins", sql, self.room_id)
+        )
+        initial_count = rows[0][0]
+
+        self.helper.join(self.room_id, user=self.bob, tok=self.bob_token)
+        rows = self.get_success(
+            self.store.db_pool.execute("test_deduplicate_joins", sql, self.room_id)
+        )
+        new_count = rows[0][0]
+
+        self.assertEqual(initial_count, new_count)