Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Implement MSC3827: Filtering of /publicRooms by room type #13031

Merged
merged 38 commits into from
Jun 29, 2022
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
990c7d2
Add changelog
SimonBrandner Jun 18, 2022
50d0c2a
Handle `room_type` in `room_stats_state`
SimonBrandner Jun 12, 2022
2eb6714
Add basic `room_type` filtering to `/public_rooms`
SimonBrandner Jun 12, 2022
1aa4dbe
Add `_background_add_room_type_column()`
SimonBrandner Jun 18, 2022
726acb2
Handle rooms of default type
SimonBrandner Jun 18, 2022
7225c58
Hide MSC3827 behind a config flag
SimonBrandner Jun 18, 2022
f7d6402
Use plural
SimonBrandner Jun 18, 2022
8199e87
Add `PublicRoomsFilterFields`
SimonBrandner Jun 18, 2022
cb3bf7e
Add `PublicRoomsRoomTypeFilterTestCase`
SimonBrandner Jun 19, 2022
7b57766
Use a prefix
SimonBrandner Jun 19, 2022
73cf558
Link to the spec
SimonBrandner Jun 28, 2022
04c3903
Filter out all `None`s
SimonBrandner Jun 28, 2022
825eb77
Rename variable
SimonBrandner Jun 28, 2022
ff9f188
Use `None` instead of `""`
SimonBrandner Jun 28, 2022
bf39cb6
Add `test_returns_both_rooms_and_spaces_if_array_is_empty`
SimonBrandner Jun 28, 2022
0e8cd97
Fix typo
SimonBrandner Jun 28, 2022
037fe5f
Simplify code
SimonBrandner Jun 28, 2022
0b3e71c
Check type
SimonBrandner Jun 28, 2022
7e50b78
Don't change default behaviour
SimonBrandner Jun 28, 2022
da7ed94
Simplify code and revert las commit
SimonBrandner Jun 28, 2022
84af156
Delint
SimonBrandner Jun 28, 2022
579e404
Merge remote-tracking branch 'upstream/develop' into SimonBrandner/fe…
SimonBrandner Jun 28, 2022
6a0f0fa
More delint
SimonBrandner Jun 28, 2022
a21938f
Merge remote-tracking branch 'upstream/develop' into SimonBrandner/fe…
SimonBrandner Jun 28, 2022
217f63a
Another delint attempt
SimonBrandner Jun 28, 2022
d440f0a
Getting a little repetetive
SimonBrandner Jun 28, 2022
4840244
Improve comment
SimonBrandner Jun 29, 2022
4162303
Add docstring
SimonBrandner Jun 29, 2022
4fd3e51
Don't copy and follow conventions
SimonBrandner Jun 29, 2022
a269756
Check for type
SimonBrandner Jun 29, 2022
45e9909
Check for type
SimonBrandner Jun 29, 2022
f23a20b
Merge remote-tracking branch 'upstream/develop' into SimonBrandner/fe…
SimonBrandner Jun 29, 2022
281ab62
Delint
SimonBrandner Jun 29, 2022
0d0dae3
Fix types (hopefully)
SimonBrandner Jun 29, 2022
7c342f5
Don't change the defautl behaviour
SimonBrandner Jun 29, 2022
8329f40
Delint
SimonBrandner Jun 29, 2022
0ad8a40
Check if MSC3827 is enabled
SimonBrandner Jun 29, 2022
b5fec2f
Fix comment
SimonBrandner Jun 29, 2022
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/13031.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Implement [MSC3827](https://github.com/matrix-org/matrix-spec-proposals/pull/3827): Filtering of /publicRooms by room type.
10 changes: 10 additions & 0 deletions synapse/api/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -259,3 +259,13 @@ class ReceiptTypes:
READ: Final = "m.read"
READ_PRIVATE: Final = "org.matrix.msc2285.read.private"
FULLY_READ: Final = "m.fully_read"


class PublicRoomsFilterFields:
SimonBrandner marked this conversation as resolved.
Show resolved Hide resolved
"""Fields in the search filter for `/publicRooms` that we understand.

As defined in https://spec.matrix.org/v1.3/client-server-api/#post_matrixclientv3publicrooms
"""

GENERIC_SEARCH_TERM: Final = "generic_search_term"
ROOM_TYPES: Final = "org.matrix.msc3827.room_types"
3 changes: 3 additions & 0 deletions synapse/config/experimental.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:

# MSC3715: dir param on /relations.
self.msc3715_enabled: bool = experimental.get("msc3715_enabled", False)

# MSC3827: Filtering of /publicRooms by room type
self.msc3827_enabled: bool = experimental.get("msc3827_enabled", False)
23 changes: 20 additions & 3 deletions synapse/handlers/room_list.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
GuestAccess,
HistoryVisibility,
JoinRules,
PublicRoomsFilterFields,
)
from synapse.api.errors import (
Codes,
Expand Down Expand Up @@ -181,6 +182,7 @@ def build_room_entry(room: JsonDict) -> JsonDict:
== HistoryVisibility.WORLD_READABLE,
"guest_can_join": room["guest_access"] == "can_join",
"join_rule": room["join_rules"],
"org.matrix.msc3827.room_type": room["room_type"],
}

# Filter out Nones – rather omit the field altogether
Expand Down Expand Up @@ -239,7 +241,9 @@ def build_room_entry(room: JsonDict) -> JsonDict:
response["chunk"] = results

response["total_room_count_estimate"] = await self.store.count_public_rooms(
network_tuple, ignore_non_federatable=from_federation
network_tuple,
ignore_non_federatable=from_federation,
search_filter=search_filter,
)

return response
Expand Down Expand Up @@ -508,8 +512,21 @@ def copy_and_replace(self, **kwds: Any) -> "RoomListNextBatch":


def _matches_room_entry(room_entry: JsonDict, search_filter: dict) -> bool:
if search_filter and search_filter.get("generic_search_term", None):
generic_search_term = search_filter["generic_search_term"].upper()
"""Determines whether the given search filter matches a room entry returned over
federation.

Only used if the remote server does not support MSC2197 remote-filtered search, and
hence does not support MSC3827 filtering of `/publicRooms` by room type either.

In this case, we cannot apply the `room_type` filter since no `room_type` field is
returned.
"""
if search_filter and search_filter.get(
SimonBrandner marked this conversation as resolved.
Show resolved Hide resolved
PublicRoomsFilterFields.GENERIC_SEARCH_TERM, None
):
generic_search_term = search_filter[
PublicRoomsFilterFields.GENERIC_SEARCH_TERM
].upper()
if generic_search_term in room_entry.get("name", "").upper():
return True
elif generic_search_term in room_entry.get("topic", "").upper():
Expand Down
3 changes: 3 additions & 0 deletions synapse/handlers/stats.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,9 @@ async def _handle_deltas(
room_state["is_federatable"] = (
event_content.get(EventContentFields.FEDERATE, True) is True
)
room_type = event_content.get(EventContentFields.ROOM_TYPE)
if isinstance(room_type, str):
room_state["room_type"] = room_type
elif typ == EventTypes.JoinRules:
room_state["join_rules"] = event_content.get("join_rule")
elif typ == EventTypes.RoomHistoryVisibility:
Expand Down
2 changes: 2 additions & 0 deletions synapse/rest/client/versions.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ def on_GET(self, request: Request) -> Tuple[int, JsonDict]:
"org.matrix.msc3026.busy_presence": self.config.experimental.msc3026_enabled,
# Supports receiving private read receipts as per MSC2285
"org.matrix.msc2285": self.config.experimental.msc2285_enabled,
# Supports filtering of /publicRooms by room type MSC3827
"org.matrix.msc3827": self.config.experimental.msc3827_enabled,
# Adds support for importing historical messages as per MSC2716
"org.matrix.msc2716": self.config.experimental.msc2716_enabled,
# Adds support for jump to date endpoints (/timestamp_to_event) as per MSC3030
Expand Down
126 changes: 121 additions & 5 deletions synapse/storage/databases/main/room.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,17 @@

import attr

from synapse.api.constants import EventContentFields, EventTypes, JoinRules
from synapse.api.constants import (
EventContentFields,
EventTypes,
JoinRules,
PublicRoomsFilterFields,
)
from synapse.api.errors import StoreError
from synapse.api.room_versions import RoomVersion, RoomVersions
from synapse.config.homeserver import HomeServerConfig
from synapse.events import EventBase
from synapse.storage._base import SQLBaseStore, db_to_json
from synapse.storage._base import SQLBaseStore, db_to_json, make_in_list_sql_clause
from synapse.storage.database import (
DatabasePool,
LoggingDatabaseConnection,
Expand Down Expand Up @@ -199,22 +204,50 @@ async def get_public_room_ids(self) -> List[str]:
desc="get_public_room_ids",
)

def _construct_room_type_where_clause(
self, room_types: Union[List[Union[str, None]], None]
) -> Tuple[Union[str, None], List[str]]:
if not room_types:
squahtx marked this conversation as resolved.
Show resolved Hide resolved
return None, []
else:
# We use None when we want get rooms without a type
is_null_clause = ""
if None in room_types:
is_null_clause = "OR room_type IS NULL"
room_types = [value for value in room_types if value is not None]

list_clause, args = make_in_list_sql_clause(
self.database_engine, "room_type", room_types
)

return f"({list_clause} {is_null_clause})", args

async def count_public_rooms(
self,
network_tuple: Optional[ThirdPartyInstanceID],
ignore_non_federatable: bool,
search_filter: Optional[dict],
) -> int:
"""Counts the number of public rooms as tracked in the room_stats_current
and room_stats_state table.

Args:
network_tuple
ignore_non_federatable: If true filters out non-federatable rooms
search_filter
"""

def _count_public_rooms_txn(txn: LoggingTransaction) -> int:
query_args = []

room_type_clause, args = self._construct_room_type_where_clause(
search_filter.get(PublicRoomsFilterFields.ROOM_TYPES, None)
if search_filter
else None
)
room_type_clause = f" AND {room_type_clause}" if room_type_clause else ""
query_args += args

if network_tuple:
if network_tuple.appservice_id:
published_sql = """
Expand Down Expand Up @@ -249,6 +282,7 @@ def _count_public_rooms_txn(txn: LoggingTransaction) -> int:
OR join_rules = '{JoinRules.KNOCK_RESTRICTED}'
OR history_visibility = 'world_readable'
)
{room_type_clause}
AND joined_members > 0
"""

Expand Down Expand Up @@ -347,8 +381,12 @@ async def get_largest_public_rooms(
if ignore_non_federatable:
where_clauses.append("is_federatable")

if search_filter and search_filter.get("generic_search_term", None):
search_term = "%" + search_filter["generic_search_term"] + "%"
if search_filter and search_filter.get(
PublicRoomsFilterFields.GENERIC_SEARCH_TERM, None
):
search_term = (
"%" + search_filter[PublicRoomsFilterFields.GENERIC_SEARCH_TERM] + "%"
)

where_clauses.append(
"""
Expand All @@ -365,6 +403,15 @@ async def get_largest_public_rooms(
search_term.lower(),
]

room_type_clause, args = self._construct_room_type_where_clause(
search_filter.get(PublicRoomsFilterFields.ROOM_TYPES, None)
if search_filter
else None
)
if room_type_clause:
where_clauses.append(room_type_clause)
query_args += args

where_clause = ""
if where_clauses:
where_clause = " AND " + " AND ".join(where_clauses)
Expand All @@ -373,7 +420,7 @@ async def get_largest_public_rooms(
sql = f"""
SELECT
room_id, name, topic, canonical_alias, joined_members,
avatar, history_visibility, guest_access, join_rules
avatar, history_visibility, guest_access, join_rules, room_type
FROM (
{published_sql}
) published
Expand Down Expand Up @@ -1166,6 +1213,7 @@ class _BackgroundUpdates:
POPULATE_ROOM_DEPTH_MIN_DEPTH2 = "populate_room_depth_min_depth2"
REPLACE_ROOM_DEPTH_MIN_DEPTH = "replace_room_depth_min_depth"
POPULATE_ROOMS_CREATOR_COLUMN = "populate_rooms_creator_column"
ADD_ROOM_TYPE_COLUMN = "add_room_type_column"


_REPLACE_ROOM_DEPTH_SQL_COMMANDS = (
Expand Down Expand Up @@ -1200,6 +1248,11 @@ def __init__(
self._background_add_rooms_room_version_column,
)

self.db_pool.updates.register_background_update_handler(
_BackgroundUpdates.ADD_ROOM_TYPE_COLUMN,
self._background_add_room_type_column,
)

# BG updates to change the type of room_depth.min_depth
self.db_pool.updates.register_background_update_handler(
_BackgroundUpdates.POPULATE_ROOM_DEPTH_MIN_DEPTH2,
Expand Down Expand Up @@ -1569,6 +1622,69 @@ def _background_populate_rooms_creator_column_txn(

return batch_size

async def _background_add_room_type_column(
self, progress: JsonDict, batch_size: int
) -> int:
Comment on lines +1625 to +1627
Copy link
Member

Choose a reason for hiding this comment

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

Not really sure where to make this thread, so I'll do it here (I didn't see this previously discussed on the PR, but do let me know if I missed it!)

Since this is a background update I'm assuming the new room_type column is null for all rows until we fill in it. This will cause incorrect results to be returned until the background update is complete -- do we think this is "fine" until it is done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is "fine" though I am not sure if I should be the one to judge that

Not sure if this could be avoided anyway

Copy link
Member

Choose a reason for hiding this comment

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

I think for this use-case it is most likely fine too, just was curious if I had missed a conversation about it.

"""Background update to go and add room_type information to `room_stats_state`
table from `event_json` table.
"""

last_room_id = progress.get("room_id", "")

def _background_add_room_type_column_txn(
txn: LoggingTransaction,
) -> bool:
sql = """
SELECT state.room_id, json FROM event_json
INNER JOIN current_state_events AS state USING (event_id)
WHERE state.room_id > ? AND type = 'm.room.create'
ORDER BY state.room_id
LIMIT ?
"""

txn.execute(sql, (last_room_id, batch_size))
room_id_to_create_event_results = txn.fetchall()

new_last_room_id = None
for room_id, event_json in room_id_to_create_event_results:
event_dict = db_to_json(event_json)

room_type = event_dict.get("content", {}).get(
EventContentFields.ROOM_TYPE, None
)
if isinstance(room_type, str):
self.db_pool.simple_update_txn(
txn,
table="room_stats_state",
keyvalues={"room_id": room_id},
updatevalues={"room_type": room_type},
)

new_last_room_id = room_id

if new_last_room_id is None:
return True

self.db_pool.updates._background_update_progress_txn(
txn,
_BackgroundUpdates.ADD_ROOM_TYPE_COLUMN,
{"room_id": new_last_room_id},
)

return False

end = await self.db_pool.runInteraction(
"_background_add_room_type_column",
_background_add_room_type_column_txn,
)

if end:
await self.db_pool.updates._end_background_update(
_BackgroundUpdates.ADD_ROOM_TYPE_COLUMN
)

return batch_size


class RoomStore(RoomBackgroundUpdateStore, RoomWorkerStore):
def __init__(
Expand Down
10 changes: 8 additions & 2 deletions synapse/storage/databases/main/stats.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
import logging
from enum import Enum
from itertools import chain
from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple, cast
from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple, Union, cast

from typing_extensions import Counter

Expand Down Expand Up @@ -238,6 +238,7 @@ async def update_room_state(self, room_id: str, fields: Dict[str, Any]) -> None:
* avatar
* canonical_alias
* guest_access
* room_type

A is_federatable key can also be included with a boolean value.

Expand All @@ -263,6 +264,7 @@ async def update_room_state(self, room_id: str, fields: Dict[str, Any]) -> None:
"avatar",
"canonical_alias",
"guest_access",
"room_type",
):
field = fields.get(col, sentinel)
if field is not sentinel and (not isinstance(field, str) or "\0" in field):
Expand Down Expand Up @@ -572,7 +574,7 @@ def _fetch_current_state_stats(

state_event_map = await self.get_events(event_ids, get_prev_content=False) # type: ignore[attr-defined]

room_state = {
room_state: Dict[str, Union[None, bool, str]] = {
"join_rules": None,
"history_visibility": None,
"encryption": None,
Expand All @@ -581,6 +583,7 @@ def _fetch_current_state_stats(
"avatar": None,
"canonical_alias": None,
"is_federatable": True,
"room_type": None,
}

for event in state_event_map.values():
Expand All @@ -604,6 +607,9 @@ def _fetch_current_state_stats(
room_state["is_federatable"] = (
event.content.get(EventContentFields.FEDERATE, True) is True
)
room_type = event.content.get(EventContentFields.ROOM_TYPE)
if isinstance(room_type, str):
room_state["room_type"] = room_type

await self.update_room_state(room_id, room_state)

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/* Copyright 2022 The Matrix.org Foundation C.I.C
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

ALTER TABLE room_stats_state ADD room_type TEXT;

INSERT INTO background_updates (update_name, progress_json)
VALUES ('add_room_type_column', '{}');
Loading