Skip to content

Sliding Sync: Move filters tests to rest layer #17703

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.

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

Merged
merged 8 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/17703.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Refactor sliding sync filter unit tests so the sliding sync API has better test coverage.
17 changes: 16 additions & 1 deletion synapse/handlers/sliding_sync/room_lists.py
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ async def _compute_interested_rooms_new_tables(
event_pos=change.event_pos,
room_version_id=change.room_version_id,
# We keep the current state of the room though
has_known_state=existing_room.has_known_state,
room_type=existing_room.room_type,
is_encrypted=existing_room.is_encrypted,
)
Expand All @@ -270,6 +271,7 @@ async def _compute_interested_rooms_new_tables(
event_id=change.event_id,
event_pos=change.event_pos,
room_version_id=change.room_version_id,
has_known_state=True,
room_type=room_type,
is_encrypted=is_encrypted,
)
Expand Down Expand Up @@ -305,6 +307,7 @@ async def _compute_interested_rooms_new_tables(
event_id=None,
event_pos=newly_left_room_map[room_id],
room_version_id=await self.store.get_room_version_id(room_id),
has_known_state=True,
room_type=room_type,
is_encrypted=is_encrypted,
)
Expand Down Expand Up @@ -1630,12 +1633,14 @@ async def filter_rooms(
and room_type not in filters.room_types
):
filtered_room_id_set.remove(room_id)
continue

if (
filters.not_room_types is not None
and room_type in filters.not_room_types
):
filtered_room_id_set.remove(room_id)
continue

if filters.room_name_like is not None:
with start_active_span("filters.room_name_like"):
Expand Down Expand Up @@ -1705,7 +1710,10 @@ async def filter_rooms_using_tables(
filtered_room_id_set = {
room_id
for room_id in filtered_room_id_set
if sync_room_map[room_id].is_encrypted == filters.is_encrypted
# Remove rooms if we can't figure out what the encryption status is
if sync_room_map[room_id].has_known_state
# Or remove if it doesn't match the filter
and sync_room_map[room_id].is_encrypted == filters.is_encrypted
}

# Filter for rooms that the user has been invited to
Expand Down Expand Up @@ -1734,19 +1742,26 @@ async def filter_rooms_using_tables(
# Make a copy so we don't run into an error: `Set changed size during
# iteration`, when we filter out and remove items
for room_id in filtered_room_id_set.copy():
# Remove rooms if we can't figure out what room type it is
if not sync_room_map[room_id].has_known_state:
filtered_room_id_set.remove(room_id)
continue
Comment on lines +1745 to +1748
Copy link
Contributor Author

@MadLittleMods MadLittleMods Sep 11, 2024

Choose a reason for hiding this comment

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

Fixing filter_rooms_using_tables (the new path using the new sliding sync tables) to take into account has_known_state.

The _no_stripped_state tests exercise these scenarios.

Found this bug because we're actually testing the new path now with this PR.


room_type = sync_room_map[room_id].room_type

if (
filters.room_types is not None
and room_type not in filters.room_types
):
filtered_room_id_set.remove(room_id)
continue

if (
filters.not_room_types is not None
and room_type in filters.not_room_types
):
filtered_room_id_set.remove(room_id)
continue

if filters.room_name_like is not None:
with start_active_span("filters.room_name_like"):
Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/client/sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -1044,7 +1044,7 @@ async def encode_rooms(
serialized_rooms[room_id]["heroes"] = serialized_heroes

# We should only include the `initial` key if it's `True` to save bandwidth.
# The absense of this flag means `False`.
# The absence of this flag means `False`.
if room_result.initial:
serialized_rooms[room_id]["initial"] = room_result.initial

Expand Down
6 changes: 4 additions & 2 deletions synapse/storage/databases/main/roommember.py
Original file line number Diff line number Diff line change
Expand Up @@ -1420,6 +1420,7 @@ def get_sliding_sync_rooms_for_user_txn(
SELECT m.room_id, m.sender, m.membership, m.membership_event_id,
r.room_version,
m.event_instance_name, m.event_stream_ordering,
m.has_known_state,
COALESCE(j.room_type, m.room_type),
COALESCE(j.is_encrypted, m.is_encrypted)
FROM sliding_sync_membership_snapshots AS m
Expand All @@ -1437,8 +1438,9 @@ def get_sliding_sync_rooms_for_user_txn(
event_id=row[3],
room_version_id=row[4],
event_pos=PersistedEventPosition(row[5], row[6]),
room_type=row[7],
is_encrypted=bool(row[8]),
has_known_state=bool(row[7]),
room_type=row[8],
is_encrypted=bool(row[9]),
)
for row in txn
}
Expand Down
1 change: 1 addition & 0 deletions synapse/storage/roommember.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ class RoomsForUserSlidingSync:
event_pos: PersistedEventPosition
room_version_id: str

has_known_state: bool
room_type: Optional[str]
is_encrypted: bool

Expand Down
Loading
Loading