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.

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

Fix filtering room types on remote rooms #17434

Merged
merged 2 commits into from
Jul 11, 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/17434.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix bug in experimental [MSC3575](https://github.com/matrix-org/matrix-spec-proposals/pull/3575) Sliding Sync `/sync` endpoint when using room type filters and the user has one or more remote invites.
22 changes: 10 additions & 12 deletions synapse/handlers/sliding_sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,7 @@
import attr
from immutabledict import immutabledict

from synapse.api.constants import (
AccountDataTypes,
Direction,
EventContentFields,
EventTypes,
Membership,
)
from synapse.api.constants import AccountDataTypes, Direction, EventTypes, Membership
from synapse.events import EventBase
from synapse.events.utils import strip_event
from synapse.handlers.relations import BundledAggregations
Expand Down Expand Up @@ -959,11 +953,15 @@ async def filter_rooms(
# provided in the list. `None` is a valid type for rooms which do not have a
# room type.
if filters.room_types is not None or filters.not_room_types is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to apply this same remote-invite friendly treatment to filters.is_encrypted

(I'm working on a PR to update this)

Copy link
Contributor

Choose a reason for hiding this comment

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

-> #17450

# 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():
create_event = await self.store.get_create_event_for_room(room_id)
room_type = create_event.content.get(EventContentFields.ROOM_TYPE)
room_to_type = await self.store.bulk_get_room_type(
{
room_id
for room_id in filtered_room_id_set
# We only know the room types for joined rooms
if sync_room_map[room_id].membership == Membership.JOIN
}
)
for room_id, room_type in room_to_type.items():
if (
filters.room_types is not None
and room_type not in filters.room_types
Expand Down
52 changes: 51 additions & 1 deletion synapse/storage/databases/main/state.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@

import attr

from synapse.api.constants import EventTypes, Membership
from synapse.api.constants import EventContentFields, EventTypes, Membership
from synapse.api.errors import NotFoundError, UnsupportedRoomVersionError
from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion
from synapse.events import EventBase
Expand Down Expand Up @@ -298,6 +298,56 @@ async def get_create_event_for_room(self, room_id: str) -> EventBase:
create_event = await self.get_event(create_id)
return create_event

@cached(max_entries=10000)
async def get_room_type(self, room_id: str) -> Optional[str]:
"""Get the room type for a given room. The server must be joined to the
given room.
"""

row = await self.db_pool.simple_select_one(
table="room_stats_state",
keyvalues={"room_id": room_id},
retcols=("room_type",),
allow_none=True,
desc="get_room_type",
)

if row is not None:
return row[0]

# If we haven't updated `room_stats_state` with the room yet, query the
# create event directly.
create_event = await self.get_create_event_for_room(room_id)
room_type = create_event.content.get(EventContentFields.ROOM_TYPE)
return room_type

@cachedList(cached_method_name="get_room_type", list_name="room_ids")
async def bulk_get_room_type(
self, room_ids: Set[str]
) -> Mapping[str, Optional[str]]:
"""Bulk fetch room types for the given rooms, the server must be in all
the rooms given.
"""

rows = await self.db_pool.simple_select_many_batch(
table="room_stats_state",
column="room_id",
iterable=room_ids,
retcols=("room_id", "room_type"),
desc="bulk_get_room_type",
)

# If we haven't updated `room_stats_state` with the room yet, query the
# create events directly. This should happen only rarely so we don't
# mind if we do this in a loop.
results = dict(rows)
for room_id in room_ids - results.keys():
create_event = await self.get_create_event_for_room(room_id)
room_type = create_event.content.get(EventContentFields.ROOM_TYPE)
results[room_id] = room_type

return results

@cached(max_entries=100000, iterable=True)
async def get_partial_current_state_ids(self, room_id: str) -> StateMap[str]:
"""Get the current state event ids for a room based on the
Expand Down
68 changes: 68 additions & 0 deletions tests/handlers/test_sliding_sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
RoomTypes,
)
from synapse.api.room_versions import RoomVersions
from synapse.events import make_event_from_dict
from synapse.events.snapshot import EventContext
from synapse.handlers.sliding_sync import RoomSyncConfig, StateValues
from synapse.rest import admin
from synapse.rest.client import knock, login, room
Expand Down Expand Up @@ -2791,6 +2793,72 @@ def test_filter_not_room_types(self) -> None:

self.assertEqual(filtered_room_map.keys(), {space_room_id})

def test_filter_room_types_with_invite_remote_room(self) -> None:
"""Test that we can apply a room type filter, even if we have an invite
for a remote room.

This is a regression test.
"""

user1_id = self.register_user("user1", "pass")
user1_tok = self.login(user1_id, "pass")

# Create a fake remote invite and persist it.
invite_room_id = "!some:room"
invite_event = make_event_from_dict(
{
"room_id": invite_room_id,
"sender": "@user:test.serv",
"state_key": user1_id,
"depth": 1,
"origin_server_ts": 1,
"type": EventTypes.Member,
"content": {"membership": Membership.INVITE},
"auth_events": [],
"prev_events": [],
},
room_version=RoomVersions.V10,
)
invite_event.internal_metadata.outlier = True
invite_event.internal_metadata.out_of_band_membership = True

self.get_success(
self.store.maybe_store_room_on_outlier_membership(
room_id=invite_room_id, room_version=invite_event.room_version
)
)
context = EventContext.for_outlier(self.hs.get_storage_controllers())
persist_controller = self.hs.get_storage_controllers().persistence
assert persist_controller is not None
self.get_success(persist_controller.persist_event(invite_event, context))

# Create a normal room (no room type)
room_id = self.helper.create_room_as(user1_id, tok=user1_tok)

after_rooms_token = self.event_sources.get_current_token()

# Get the rooms the user should be syncing with
sync_room_map = self.get_success(
self.sliding_sync_handler.get_sync_room_ids_for_user(
UserID.from_string(user1_id),
from_token=None,
to_token=after_rooms_token,
)
)

filtered_room_map = self.get_success(
self.sliding_sync_handler.filter_rooms(
UserID.from_string(user1_id),
sync_room_map,
SlidingSyncConfig.SlidingSyncList.Filters(
room_types=[None, RoomTypes.SPACE],
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
),
after_rooms_token,
)
)

self.assertEqual(filtered_room_map.keys(), {room_id, invite_room_id})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.assertEqual(filtered_room_map.keys(), {room_id, invite_room_id})
# Confirm that the sync response with the filter applied does not filter out
# remote rooms.
self.assertEqual(filtered_room_map.keys(), {room_id, invite_room_id})

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this comment is a bit misleading, as we really just care that it worked in general

Copy link
Contributor

@MadLittleMods MadLittleMods Jul 15, 2024

Choose a reason for hiding this comment

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

Some comment to explain the expectation here would be nice.

For example, it feels weird to me that we don't filter out rooms where we don't know the type.

(I'm working on a PR to update this) -> #17450



class SortRoomsTestCase(HomeserverTestCase):
"""
Expand Down
Loading