Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sliding sync: remove the list ops from the response #17671

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Sliding sync: remove the list ops from the response #17671

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8624d45
Remove list ops from the sliding sync response
erikjohnston Sep 5, 2024
e748761
Remove the sync ops from the code
erikjohnston Sep 5, 2024
b715c8b
Newsfile
erikjohnston Sep 5, 2024
b4a1784
Update doc
erikjohnston Sep 10, 2024
02bc71a
Merge remote-tracking branch 'origin/develop' into erikj/ss_remove_li…
erikjohnston Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/17671.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove unspecced list operations from the sliding sync response.
28 changes: 9 additions & 19 deletions synapse/handlers/sliding_sync/extensions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

from synapse.api.constants import AccountDataTypes, EduTypes
from synapse.handlers.receipts import ReceiptEventSource
from synapse.handlers.sliding_sync.room_lists import SlidingSyncListResult
from synapse.logging.opentracing import trace
from synapse.storage.databases.main.receipts import ReceiptInRoom
from synapse.types import (
Expand All @@ -33,7 +34,6 @@
from synapse.types.handlers.sliding_sync import (
HaveSentRoomFlag,
MutablePerConnectionState,
OperationType,
PerConnectionState,
SlidingSyncConfig,
SlidingSyncResult,
Expand All @@ -60,7 +60,7 @@ async def get_extensions_response(
sync_config: SlidingSyncConfig,
previous_connection_state: "PerConnectionState",
new_connection_state: "MutablePerConnectionState",
actual_lists: Mapping[str, SlidingSyncResult.SlidingWindowList],
actual_lists: Mapping[str, SlidingSyncListResult],
actual_room_ids: Set[str],
actual_room_response_map: Mapping[str, SlidingSyncResult.RoomResult],
to_token: StreamToken,
Expand Down Expand Up @@ -151,7 +151,7 @@ def find_relevant_room_ids_for_extension(
self,
requested_lists: Optional[StrCollection],
requested_room_ids: Optional[StrCollection],
actual_lists: Mapping[str, SlidingSyncResult.SlidingWindowList],
actual_lists: Mapping[str, SlidingSyncListResult],
actual_room_ids: AbstractSet[str],
) -> Set[str]:
"""
Expand Down Expand Up @@ -193,28 +193,18 @@ def find_relevant_room_ids_for_extension(
if requested_lists is not None:
for list_key in requested_lists:
# Just some typing because we share the variable name in multiple places
actual_list: Optional[SlidingSyncResult.SlidingWindowList] = None
actual_list: Optional[SlidingSyncListResult] = None

# A wildcard means we process rooms from all lists
if list_key == "*":
for actual_list in actual_lists.values():
# We only expect a single SYNC operation for any list
assert len(actual_list.ops) == 1
sync_op = actual_list.ops[0]
assert sync_op.op == OperationType.SYNC

relevant_room_ids.update(sync_op.room_ids)
relevant_room_ids.update(actual_list.room_ids)

break

actual_list = actual_lists.get(list_key)
if actual_list is not None:
# We only expect a single SYNC operation for any list
assert len(actual_list.ops) == 1
sync_op = actual_list.ops[0]
assert sync_op.op == OperationType.SYNC

relevant_room_ids.update(sync_op.room_ids)
relevant_room_ids.update(actual_list.room_ids)

return relevant_room_ids

Expand Down Expand Up @@ -348,7 +338,7 @@ async def get_e2ee_extension_response(
async def get_account_data_extension_response(
self,
sync_config: SlidingSyncConfig,
actual_lists: Mapping[str, SlidingSyncResult.SlidingWindowList],
actual_lists: Mapping[str, SlidingSyncListResult],
actual_room_ids: Set[str],
account_data_request: SlidingSyncConfig.Extensions.AccountDataExtension,
to_token: StreamToken,
Expand Down Expand Up @@ -441,7 +431,7 @@ async def get_receipts_extension_response(
sync_config: SlidingSyncConfig,
previous_connection_state: "PerConnectionState",
new_connection_state: "MutablePerConnectionState",
actual_lists: Mapping[str, SlidingSyncResult.SlidingWindowList],
actual_lists: Mapping[str, SlidingSyncListResult],
actual_room_ids: Set[str],
actual_room_response_map: Mapping[str, SlidingSyncResult.RoomResult],
receipts_request: SlidingSyncConfig.Extensions.ReceiptsExtension,
Expand Down Expand Up @@ -637,7 +627,7 @@ async def get_receipts_extension_response(
async def get_typing_extension_response(
self,
sync_config: SlidingSyncConfig,
actual_lists: Mapping[str, SlidingSyncResult.SlidingWindowList],
actual_lists: Mapping[str, SlidingSyncListResult],
actual_room_ids: Set[str],
actual_room_response_map: Mapping[str, SlidingSyncResult.RoomResult],
typing_request: SlidingSyncConfig.Extensions.TypingExtension,
Expand Down
52 changes: 20 additions & 32 deletions synapse/handlers/sliding_sync/room_lists.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@
)
from synapse.types.handlers.sliding_sync import (
HaveSentRoomFlag,
OperationType,
PerConnectionState,
RoomSyncConfig,
SlidingSyncConfig,
Expand Down Expand Up @@ -106,7 +105,7 @@ class SlidingSyncInterestedRooms:
dm_room_ids: The set of rooms the user consider as direct-message (DM) rooms
"""

lists: Mapping[str, SlidingSyncResult.SlidingWindowList]
lists: Mapping[str, "SlidingSyncListResult"]
relevant_room_map: Mapping[str, RoomSyncConfig]
relevant_rooms_to_send_map: Mapping[str, RoomSyncConfig]
all_rooms: Set[str]
Expand All @@ -117,6 +116,17 @@ class SlidingSyncInterestedRooms:
dm_room_ids: AbstractSet[str]


@attr.s(auto_attribs=True, slots=True, frozen=True)
class SlidingSyncListResult(SlidingSyncResult.SlidingWindowList):
"""Add a new field to the lists response that allows us to track the
matching room IDs in the list.

This is not returend to the client.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This is not returend to the client.
This is not returned to the client.

"""

room_ids: StrCollection


class Sentinel(enum.Enum):
# defining a sentinel in this way allows mypy to correctly handle the
# type of a dictionary lookup and subsequent type narrowing.
Expand Down Expand Up @@ -212,7 +222,7 @@ async def _compute_interested_rooms_new_tables(
user_id = sync_config.user.to_string()

# Assemble sliding window lists
lists: Dict[str, SlidingSyncResult.SlidingWindowList] = {}
lists: Dict[str, SlidingSyncListResult] = {}
# Keep track of the rooms that we can display and need to fetch more info about
relevant_room_map: Dict[str, RoomSyncConfig] = {}
# The set of room IDs of all rooms that could appear in any list. These
Expand Down Expand Up @@ -350,7 +360,7 @@ async def _compute_interested_rooms_new_tables(

all_rooms.update(filtered_sync_room_map)

ops: List[SlidingSyncResult.SlidingWindowList.Operation] = []
room_ids_in_list: List[str] = []

if list_config.ranges:
if list_config.ranges == [(0, len(filtered_sync_room_map) - 1)]:
Expand All @@ -363,8 +373,6 @@ async def _compute_interested_rooms_new_tables(
)

for range in list_config.ranges:
room_ids_in_list: List[str] = []

# We're going to loop through the sorted list of rooms starting
# at the range start index and keep adding rooms until we fill
# up the range or run out of rooms.
Expand Down Expand Up @@ -393,17 +401,8 @@ async def _compute_interested_rooms_new_tables(

room_ids_in_list.append(room_id)

ops.append(
SlidingSyncResult.SlidingWindowList.Operation(
op=OperationType.SYNC,
range=range,
room_ids=room_ids_in_list,
)
)

lists[list_key] = SlidingSyncResult.SlidingWindowList(
count=len(sorted_room_info),
ops=ops,
lists[list_key] = SlidingSyncListResult(
count=len(sorted_room_info), room_ids=room_ids_in_list
)

if sync_config.room_subscriptions:
Expand Down Expand Up @@ -484,7 +483,7 @@ async def _compute_interested_rooms_fallback(
dm_room_ids = await self._get_dm_rooms_for_user(sync_config.user.to_string())

# Assemble sliding window lists
lists: Dict[str, SlidingSyncResult.SlidingWindowList] = {}
lists: Dict[str, SlidingSyncListResult] = {}
# Keep track of the rooms that we can display and need to fetch more info about
relevant_room_map: Dict[str, RoomSyncConfig] = {}
# The set of room IDs of all rooms that could appear in any list. These
Expand Down Expand Up @@ -535,11 +534,9 @@ async def _compute_interested_rooms_fallback(
filtered_sync_room_map, to_token
)

ops: List[SlidingSyncResult.SlidingWindowList.Operation] = []
room_ids_in_list: List[str] = []
if list_config.ranges:
for range in list_config.ranges:
room_ids_in_list: List[str] = []

# We're going to loop through the sorted list of rooms starting
# at the range start index and keep adding rooms until we fill
# up the range or run out of rooms.
Expand Down Expand Up @@ -568,17 +565,8 @@ async def _compute_interested_rooms_fallback(

room_ids_in_list.append(room_id)

ops.append(
SlidingSyncResult.SlidingWindowList.Operation(
op=OperationType.SYNC,
range=range,
room_ids=room_ids_in_list,
)
)

lists[list_key] = SlidingSyncResult.SlidingWindowList(
count=len(sorted_room_info),
ops=ops,
lists[list_key] = SlidingSyncListResult(
count=len(sorted_room_info), room_ids=room_ids_in_list
)

if sync_config.room_subscriptions:
Expand Down
18 changes: 0 additions & 18 deletions synapse/rest/client/sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,14 +792,6 @@ class SlidingSyncRestServlet(RestServlet):
"lists": {
"foo-list": {
"count": 1337,
"ops": [{
"op": "SYNC",
"range": [0, 99],
"room_ids": [
"!foo:bar",
// ... 99 more room IDs
]
}]
}
},
// Aggregated rooms from lists and room subscriptions
Expand Down Expand Up @@ -977,20 +969,10 @@ async def encode_response(
def encode_lists(
self, lists: Mapping[str, SlidingSyncResult.SlidingWindowList]
) -> JsonDict:
def encode_operation(
operation: SlidingSyncResult.SlidingWindowList.Operation,
) -> JsonDict:
return {
"op": operation.op.value,
"range": operation.range,
"room_ids": operation.room_ids,
}

serialized_lists = {}
for list_key, list_result in lists.items():
serialized_lists[list_key] = {
"count": list_result.count,
"ops": [encode_operation(op) for op in list_result.ops],
erikjohnston marked this conversation as resolved.
Show resolved Hide resolved
}

return serialized_lists
Expand Down
19 changes: 0 additions & 19 deletions synapse/types/handlers/sliding_sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
Optional,
Sequence,
Set,
Tuple,
TypeVar,
cast,
)
Expand Down Expand Up @@ -225,27 +224,9 @@ class SlidingWindowList:
Attributes:
count: The total number of entries in the list. Always present if this list
is.
ops: The sliding list operations to perform.
"""

@attr.s(slots=True, frozen=True, auto_attribs=True)
class Operation:
"""
Attributes:
op: The operation type to perform.
range: Which index positions are affected by this operation. These are
both inclusive.
room_ids: Which room IDs are affected by this operation. These IDs match
up to the positions in the `range`, so the last room ID in this list
matches the 9th index. The room data is held in a separate object.
"""

op: OperationType
range: Tuple[int, int]
room_ids: List[str]

count: int
ops: List[Operation]

@attr.s(slots=True, frozen=True, auto_attribs=True)
class Extensions:
Expand Down
12 changes: 0 additions & 12 deletions tests/rest/client/sliding_sync/test_rooms_meta.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -926,18 +926,6 @@ def test_rooms_bump_stamp(self) -> None:
response_body["lists"].keys(),
)

# Make sure the list includes the rooms in the right order
self.assertEqual(
len(response_body["lists"]["foo-list"]["ops"]),
1,
response_body["lists"]["foo-list"],
)
op = response_body["lists"]["foo-list"]["ops"][0]
self.assertEqual(op["op"], "SYNC")
self.assertEqual(op["range"], [0, 1])
# Note that we don't order the ops anymore, so we need to compare sets.
self.assertIncludes(set(op["room_ids"]), {room_id1, room_id2}, exact=True)

# The `bump_stamp` for room1 should point at the latest message (not the
# reaction since it's not one of the `DEFAULT_BUMP_EVENT_TYPES`)
self.assertEqual(
Expand Down
30 changes: 14 additions & 16 deletions tests/rest/client/sliding_sync/test_rooms_required_state.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,14 +735,13 @@ def test_rooms_required_state_partial_state(self) -> None:
response_body, _ = self.do_sync(sync_body, tok=user1_tok)

# The list should include both rooms now because we don't need full state
for list_key in response_body["lists"].keys():
self.assertIncludes(
set(response_body["lists"][list_key]["ops"][0]["room_ids"]),
{room_id2, room_id1},
exact=True,
message=f"Expected all rooms to show up for list_key={list_key}. Response "
+ str(response_body["lists"][list_key]),
)
self.assertIncludes(
set(response_body["rooms"].keys()),
{room_id2, room_id1},
exact=True,
message="Expected all rooms to show up. Response "
+ str(response_body["rooms"]),
)

# Take each of the list variants and apply them to room subscriptions to make
# sure the same rules apply
Expand Down Expand Up @@ -826,14 +825,13 @@ def test_rooms_required_state_partial_state(self) -> None:

# Make sure the list includes room1 but room2 is excluded because it's still
# partially-stated
for list_key in response_body["lists"].keys():
self.assertIncludes(
set(response_body["lists"][list_key]["ops"][0]["room_ids"]),
{room_id1},
exact=True,
message=f"Expected only fully-stated rooms to show up for list_key={list_key}. Response "
+ str(response_body["lists"][list_key]),
)
self.assertIncludes(
set(response_body["rooms"].keys()),
{room_id1},
exact=True,
message="Expected only fully-stated rooms to show up. Response "
+ str(response_body["rooms"]),
)

# Take each of the list variants and apply them to room subscriptions to make
# sure the same rules apply
Expand Down
Loading
Loading