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

SimonBrandner · 2022-06-12T17:48:14Z

Implement MSC3827: Filtering of /publicRooms by room type
Towards element-hq/element-meta#338

Pull request is based on the develop branch
Pull request includes a changelog file.
Pull request includes a sign off
Code style is correct (run the linters)

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

squahtx

As a follow on for another time: Now that room_stats_state contains room_type, there are one or two places where we can avoid an event fetch. eg. if we make get_room_with_stats return room_type, we can avoid fetching the m.room.create event in RoomSummaryHandler._build_room_entry.

synapse/api/constants.py

synapse/handlers/room_list.py

synapse/handlers/stats.py

synapse/storage/databases/main/room.py

synapse/storage/databases/main/stats.py

tests/rest/client/test_rooms.py

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com>

…at/msc3827

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

squahtx

One last comment, otherwise it's looking really good!

synapse/storage/databases/main/room.py

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

squahtx

Looks good!

I missed another small thing earlier - the docstring for test_background_add_room_type_column needs an update.

tests/storage/databases/main/test_room.py

Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com>

SimonBrandner · 2022-06-29T16:26:36Z

Looks good!

I missed another small thing earlier - the docstring for test_background_add_room_type_column needs an update.

Thanks!

clokep · 2022-06-29T17:24:17Z

we can avoid fetching the m.room.create event in RoomSummaryHandler._build_room_entry.

You figured out my plan with adding this here! 🎉

(Seems this got filed as #13130.)

clokep · 2022-06-29T17:29:16Z

synapse/storage/databases/main/room.py

+    async def _background_add_room_type_column(
+        self, progress: JsonDict, batch_size: int
+    ) -> int:


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?

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

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

matrix-org/synapse#13031 originally added support for the feature to Synapse, which although doesn't include an obvious federation route it does end up sending the field over federation. [Here](https://github.com/matrix-org/synapse/blob/a6895dd576f96d7fd086fb4128d48ac8a3f098c5/synapse/federation/transport/client.py#L481) the server copies the search filter just before it goes over the wire, which is supplied by through a chain of function calls originating [here](https://github.com/matrix-org/synapse/blob/c6d617641186221829c644204f24654430858826/synapse/rest/client/room.py#L456). Additionally, it is clear that this sort of feature would have included federation given the filtering is able to be proxied directly like this (as demonstrated by Synapse above). As such, this is determined to be a clarification/minor edit to the MSC, not requiring a second MSC to add the functionality.

SimonBrandner mentioned this pull request Jun 12, 2022

MSC3827: Filtering of /publicRooms by room type matrix-org/matrix-spec-proposals#3827

Merged

SimonBrandner force-pushed the SimonBrandner/feat/msc3827 branch 6 times, most recently from cb8bea3 to ec975f1 Compare June 18, 2022 10:15

SimonBrandner added 3 commits June 18, 2022 14:48

Add changelog

990c7d2

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

Handle room_type in room_stats_state

50d0c2a

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

Add basic room_type filtering to /public_rooms

2eb6714

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

SimonBrandner force-pushed the SimonBrandner/feat/msc3827 branch from ec975f1 to 297f9ad Compare June 18, 2022 13:04

SimonBrandner added 5 commits June 18, 2022 15:37

Add _background_add_room_type_column()

1aa4dbe

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

Handle rooms of default type

726acb2

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

Hide MSC3827 behind a config flag

7225c58

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

Use plural

f7d6402

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

Add PublicRoomsFilterFields

8199e87

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

SimonBrandner force-pushed the SimonBrandner/feat/msc3827 branch from 297f9ad to 8199e87 Compare June 18, 2022 13:37

SimonBrandner added 2 commits June 19, 2022 15:36

Add PublicRoomsRoomTypeFilterTestCase

cb3bf7e

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

Use a prefix

7b57766

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

SimonBrandner marked this pull request as ready for review June 19, 2022 14:42

SimonBrandner requested a review from a team as a code owner June 19, 2022 14:42

squahtx reviewed Jun 27, 2022

View reviewed changes

SimonBrandner added 8 commits June 28, 2022 16:04

Link to the spec

73cf558

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

Filter out all Nones

04c3903

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

Rename variable

825eb77

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

Use None instead of ""

ff9f188

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

Add test_returns_both_rooms_and_spaces_if_array_is_empty

bf39cb6

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

Fix typo

0e8cd97

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

Simplify code

037fe5f

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

Check type

0b3e71c

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

SimonBrandner and others added 9 commits June 29, 2022 06:18

Add docstring

4162303

Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com>

Don't copy and follow conventions

4fd3e51

Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com>

Check for type

a269756

Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com>

Check for type

45e9909

Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com>

Merge remote-tracking branch 'upstream/develop' into SimonBrandner/fe…

f23a20b

…at/msc3827

Delint

281ab62

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

Fix types (hopefully)

0d0dae3

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

Don't change the defautl behaviour

7c342f5

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

Delint

8329f40

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

SimonBrandner requested a review from squahtx June 29, 2022 15:55

squahtx reviewed Jun 29, 2022

View reviewed changes

synapse/storage/databases/main/room.py Outdated Show resolved Hide resolved

Check if MSC3827 is enabled

0ad8a40

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

SimonBrandner requested a review from squahtx June 29, 2022 16:04

squahtx approved these changes Jun 29, 2022

View reviewed changes

tests/storage/databases/main/test_room.py Outdated Show resolved Hide resolved

Fix comment

b5fec2f

Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com>

SimonBrandner closed this Jun 29, 2022

SimonBrandner reopened this Jun 29, 2022

squahtx enabled auto-merge (squash) June 29, 2022 16:30

squahtx mentioned this pull request Jun 29, 2022

Use room_stats_state.room_type column instead of fetching m.room.create event #13130

Closed

squahtx merged commit 13e359a into matrix-org:develop Jun 29, 2022

SimonBrandner deleted the SimonBrandner/feat/msc3827 branch June 29, 2022 17:13

clokep reviewed Jun 29, 2022

View reviewed changes

turt2live mentioned this pull request Aug 3, 2022

Clarify that MSC3827 also affects federation endpoints matrix-org/matrix-spec-proposals#3858

Merged

clokep mentioned this pull request Sep 29, 2022

Support Matrix 1.4 #13954

Closed

13 tasks

DMRobertson mentioned this pull request Oct 4, 2022

Incorrect SQL query construction in _construct_room_type_where_clause #14051

Closed

MadLittleMods mentioned this pull request Aug 6, 2024

Sliding Sync: Pre-populate room data for quick filtering/sorting element-hq/synapse#17512

Merged

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SimonBrandner commented Jun 12, 2022 •

edited

Loading

squahtx left a comment

squahtx left a comment

squahtx left a comment

SimonBrandner commented Jun 29, 2022

clokep commented Jun 29, 2022 •

edited

Loading

clokep Jun 29, 2022

SimonBrandner Jun 29, 2022

clokep Jun 29, 2022

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

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

Conversation

SimonBrandner commented Jun 12, 2022 • edited Loading

squahtx left a comment

Choose a reason for hiding this comment

squahtx left a comment

Choose a reason for hiding this comment

squahtx left a comment

Choose a reason for hiding this comment

SimonBrandner commented Jun 29, 2022

clokep commented Jun 29, 2022 • edited Loading

clokep Jun 29, 2022

Choose a reason for hiding this comment

SimonBrandner Jun 29, 2022

Choose a reason for hiding this comment

clokep Jun 29, 2022

Choose a reason for hiding this comment

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

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

SimonBrandner commented Jun 12, 2022 •

edited

Loading

clokep commented Jun 29, 2022 •

edited

Loading