Removed request_key from the SyncConfig (moved outside as its own function parameter) (#17201)

Removed `request_key` from the `SyncConfig` (moved outside as its own function parameter) so it doesn't have to flow into `_generate_sync_entry_for_xxx` methods. This way we can separate the concerns of caching from generating the response and reuse the `_generate_sync_entry_for_xxx` functions as we see fit. Plus caching doesn't really have anything to do with the config of sync.

Split from #17167

Spawning from #17167 (comment)

Author: MadLittleMods

changelog.d/17201.misc

@@ -0,0 +1 @@
+Organize the sync cache key parameter outside of the sync config (separate concerns).

synapse/handlers/sync.py

@@ -135,7 +135,6 @@ class SyncConfig:
     user: UserID
     filter_collection: FilterCollection
     is_guest: bool
-    request_key: SyncRequestKey
     device_id: Optional[str]
 
 
@@ -328,6 +327,7 @@ async def wait_for_sync_for_user(
         requester: Requester,
         sync_config: SyncConfig,
         sync_version: SyncVersion,
+        request_key: SyncRequestKey,
         since_token: Optional[StreamToken] = None,
         timeout: int = 0,
         full_state: bool = False,
@@ -340,10 +340,10 @@ async def wait_for_sync_for_user(
             requester: The user requesting the sync response.
             sync_config: Config/info necessary to process the sync request.
             sync_version: Determines what kind of sync response to generate.
+            request_key: The key to use for caching the response.
             since_token: The point in the stream to sync from.
             timeout: How long to wait for new data to arrive before giving up.
             full_state: Whether to return the full state for each room.
-
         Returns:
             When `SyncVersion.SYNC_V2`, returns a full `SyncResult`.
         """
@@ -354,7 +354,7 @@ async def wait_for_sync_for_user(
         await self.auth_blocking.check_auth_blocking(requester=requester)
 
         res = await self.response_cache.wrap(
-            sync_config.request_key,
+            request_key,
             self._wait_for_sync_for_user,
             sync_config,
             sync_version,

synapse/rest/client/sync.py

@@ -210,7 +210,6 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
             user=user,
             filter_collection=filter_collection,
             is_guest=requester.is_guest,
-            request_key=request_key,
             device_id=device_id,
         )
 
@@ -234,6 +233,7 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
                 requester,
                 sync_config,
                 SyncVersion.SYNC_V2,
+                request_key,
                 since_token=since_token,
                 timeout=timeout,
                 full_state=full_state,

tests/events/test_presence_router.py

@@ -36,7 +36,7 @@
 from synapse.types import JsonDict, StreamToken, create_requester
 from synapse.util import Clock
 
-from tests.handlers.test_sync import SyncVersion, generate_sync_config
+from tests.handlers.test_sync import SyncRequestKey, SyncVersion, generate_sync_config
 from tests.unittest import (
     FederatingHomeserverTestCase,
     HomeserverTestCase,
@@ -498,6 +498,15 @@ def send_presence_update(
     return channel.json_body
 
 
+_request_key = 0
+
+
+def generate_request_key() -> SyncRequestKey:
+    global _request_key
+    _request_key += 1
+    return ("request_key", _request_key)
+
+
 def sync_presence(
     testcase: HomeserverTestCase,
     user_id: str,
@@ -521,7 +530,11 @@ def sync_presence(
     sync_config = generate_sync_config(requester.user.to_string())
     sync_result = testcase.get_success(
         testcase.hs.get_sync_handler().wait_for_sync_for_user(
-            requester, sync_config, SyncVersion.SYNC_V2, since_token
+            requester,
+            sync_config,
+            SyncVersion.SYNC_V2,
+            generate_request_key(),
+            since_token,
         )
     )
 

tests/handlers/test_sync.py

@@ -31,7 +31,7 @@
 from synapse.events import EventBase
 from synapse.events.snapshot import EventContext
 from synapse.federation.federation_base import event_from_pdu_json
-from synapse.handlers.sync import SyncConfig, SyncResult, SyncVersion
+from synapse.handlers.sync import SyncConfig, SyncRequestKey, SyncResult, SyncVersion
 from synapse.rest import admin
 from synapse.rest.client import knock, login, room
 from synapse.server import HomeServer
@@ -41,6 +41,14 @@
 import tests.unittest
 import tests.utils
 
+_request_key = 0
+
+
+def generate_request_key() -> SyncRequestKey:
+    global _request_key
+    _request_key += 1
+    return ("request_key", _request_key)
+
 
 class SyncTestCase(tests.unittest.HomeserverTestCase):
     """Tests Sync Handler."""
@@ -77,6 +85,7 @@ def test_wait_for_sync_for_user_auth_blocking(self) -> None:
                 requester,
                 sync_config,
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
             )
         )
 
@@ -87,6 +96,7 @@ def test_wait_for_sync_for_user_auth_blocking(self) -> None:
                 requester,
                 sync_config,
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
             ),
             ResourceLimitError,
         )
@@ -102,6 +112,7 @@ def test_wait_for_sync_for_user_auth_blocking(self) -> None:
                 requester,
                 sync_config,
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
             ),
             ResourceLimitError,
         )
@@ -124,6 +135,7 @@ def test_unknown_room_version(self) -> None:
                 requester,
                 sync_config=generate_sync_config(user, device_id="dev"),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
             )
         )
 
@@ -157,6 +169,7 @@ def test_unknown_room_version(self) -> None:
                 requester,
                 sync_config=generate_sync_config(user),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
             )
         )
         self.assertIn(joined_room, [r.room_id for r in result.joined])
@@ -169,6 +182,7 @@ def test_unknown_room_version(self) -> None:
                 requester,
                 sync_config=generate_sync_config(user, device_id="dev"),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
                 since_token=initial_result.next_batch,
             )
         )
@@ -200,6 +214,7 @@ def test_unknown_room_version(self) -> None:
                 requester,
                 sync_config=generate_sync_config(user),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
             )
         )
         self.assertNotIn(joined_room, [r.room_id for r in result.joined])
@@ -212,6 +227,7 @@ def test_unknown_room_version(self) -> None:
                 requester,
                 sync_config=generate_sync_config(user, device_id="dev"),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
                 since_token=initial_result.next_batch,
             )
         )
@@ -254,6 +270,7 @@ def test_ban_wins_race_with_join(self) -> None:
                 create_requester(owner),
                 generate_sync_config(owner),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
             )
         )
         self.assertEqual(len(alice_sync_result.joined), 1)
@@ -277,6 +294,7 @@ def test_ban_wins_race_with_join(self) -> None:
                 eve_requester,
                 eve_sync_config,
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
             )
         )
 
@@ -295,6 +313,7 @@ def test_ban_wins_race_with_join(self) -> None:
                 eve_requester,
                 eve_sync_config,
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
                 since_token=eve_sync_after_ban.next_batch,
             )
         )
@@ -307,6 +326,7 @@ def test_ban_wins_race_with_join(self) -> None:
                 eve_requester,
                 eve_sync_config,
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
                 since_token=None,
             )
         )
@@ -341,6 +361,7 @@ def test_state_includes_changes_on_forks(self) -> None:
                 alice_requester,
                 generate_sync_config(alice),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
             )
         )
         last_room_creation_event_id = (
@@ -369,6 +390,7 @@ def test_state_includes_changes_on_forks(self) -> None:
                     ),
                 ),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
                 since_token=initial_sync_result.next_batch,
             )
         )
@@ -414,6 +436,7 @@ def test_state_includes_changes_on_forks_when_events_excluded(self) -> None:
                 alice_requester,
                 generate_sync_config(alice),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
             )
         )
         last_room_creation_event_id = (
@@ -452,6 +475,7 @@ def test_state_includes_changes_on_forks_when_events_excluded(self) -> None:
                     ),
                 ),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
                 since_token=initial_sync_result.next_batch,
             )
         )
@@ -498,6 +522,7 @@ def test_state_includes_changes_on_long_lived_forks(self) -> None:
                 alice_requester,
                 generate_sync_config(alice),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
             )
         )
         last_room_creation_event_id = (
@@ -523,6 +548,7 @@ def test_state_includes_changes_on_long_lived_forks(self) -> None:
                     ),
                 ),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
                 since_token=initial_sync_result.next_batch,
             )
         )
@@ -553,6 +579,7 @@ def test_state_includes_changes_on_long_lived_forks(self) -> None:
                     ),
                 ),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
                 since_token=incremental_sync.next_batch,
             )
         )
@@ -615,6 +642,7 @@ def test_state_includes_changes_on_ungappy_syncs(self) -> None:
                 alice_requester,
                 generate_sync_config(alice),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
             )
         )
         last_room_creation_event_id = (
@@ -639,6 +667,7 @@ def test_state_includes_changes_on_ungappy_syncs(self) -> None:
                     ),
                 ),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
             )
         )
         room_sync = initial_sync_result.joined[0]
@@ -660,6 +689,7 @@ def test_state_includes_changes_on_ungappy_syncs(self) -> None:
                 alice_requester,
                 generate_sync_config(alice),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
                 since_token=initial_sync_result.next_batch,
             )
         )
@@ -713,6 +743,7 @@ def test_archived_rooms_do_not_include_state_after_leave(
                 bob_requester,
                 generate_sync_config(bob),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
             )
         )
 
@@ -744,6 +775,7 @@ def test_archived_rooms_do_not_include_state_after_leave(
                     bob, filter_collection=FilterCollection(self.hs, filter_dict)
                 ),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
                 since_token=None if initial_sync else initial_sync_result.next_batch,
             )
         ).archived[0]
@@ -839,6 +871,7 @@ async def _check_sigs_and_hash_for_pulled_events_and_fetch(
                 create_requester(user),
                 generate_sync_config(user),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
             )
         )
         event_ids = []
@@ -887,6 +920,7 @@ async def _check_sigs_and_hash_for_pulled_events_and_fetch(
                 create_requester(user2),
                 generate_sync_config(user2),
                 sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
             )
         )
         priv_event_ids = []
@@ -909,7 +943,10 @@ def test_push_rules_with_bad_account_data(self) -> None:
 
         sync_result: SyncResult = self.get_success(
             self.sync_handler.wait_for_sync_for_user(
-                create_requester(user), generate_sync_config(user)
+                create_requester(user),
+                generate_sync_config(user),
+                sync_version=SyncVersion.SYNC_V2,
+                request_key=generate_request_key(),
             )
         )
 
@@ -923,9 +960,6 @@ def test_push_rules_with_bad_account_data(self) -> None:
         self.fail("No push rules found")
 
 
-_request_key = 0
-
-
 def generate_sync_config(
     user_id: str,
     device_id: Optional[str] = "device_id",
@@ -942,12 +976,9 @@ def generate_sync_config(
     if filter_collection is None:
         filter_collection = Filtering(Mock()).DEFAULT_FILTER_COLLECTION
 
-    global _request_key
-    _request_key += 1
     return SyncConfig(
         user=UserID.from_string(user_id),
         filter_collection=filter_collection,
         is_guest=False,
-        request_key=("request_key", _request_key),
         device_id=device_id,
     )