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

Add heroes and room summary fields to Sliding Sync /sync #17419

Merged
merged 22 commits into from
Jul 11, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
9deb387
Initial work of adding name/avatar to rooms response
MadLittleMods Jul 8, 2024
b0bb37f
Add tests
MadLittleMods Jul 8, 2024
b6e36ef
Add changelog
MadLittleMods Jul 8, 2024
6ef39dd
Actually test that invited should see current state
MadLittleMods Jul 8, 2024
32c5409
Start of heroes in Sliding Sync
MadLittleMods Jul 8, 2024
9641ca7
Merge branch 'develop' into madlittlemods/sliding-sync-room-name-avatar
MadLittleMods Jul 8, 2024
9692c76
Merge branch 'madlittlemods/sliding-sync-room-name-avatar' into madli…
MadLittleMods Jul 8, 2024
ee9114c
Merge branch 'develop' into madlittlemods/sliding-sync-heroes
MadLittleMods Jul 9, 2024
f58d6fc
`heroes` is not `required_state` for now
MadLittleMods Jul 9, 2024
82bf80c
Add changelog
MadLittleMods Jul 9, 2024
10f8540
Add tests
MadLittleMods Jul 9, 2024
62925b6
Remove unncessary check
MadLittleMods Jul 9, 2024
b9f1eb1
Test `joined_count`/`invited_count`
MadLittleMods Jul 9, 2024
e50bf86
Test invite before/after ban
MadLittleMods Jul 9, 2024
2fb77b3
Sort `leave` before `knock`
MadLittleMods Jul 9, 2024
275da50
Explain the order
MadLittleMods Jul 9, 2024
6060408
Add test to make sure we only return 5 heroes
MadLittleMods Jul 9, 2024
91cefaa
Fix lint
MadLittleMods Jul 9, 2024
868dcdc
Revert `heroes` order changes
MadLittleMods Jul 11, 2024
a4753bf
Better comment on what we expect
MadLittleMods Jul 11, 2024
5583ac1
Merge branch 'develop' into madlittlemods/sliding-sync-heroes
MadLittleMods Jul 11, 2024
e2138b7
Have to use basic assertion for now
MadLittleMods Jul 11, 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/17419.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Populate `heroes` and room summary fields (`joined_count`, `invited_count`) in experimental [MSC3575](https://github.com/matrix-org/matrix-spec-proposals/pull/3575) Sliding Sync `/sync` endpoint.
280 changes: 197 additions & 83 deletions synapse/handlers/sliding_sync.py

Large diffs are not rendered by default.

32 changes: 27 additions & 5 deletions synapse/rest/client/sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -995,16 +995,32 @@ async def encode_rooms(
if room_result.avatar:
serialized_rooms[room_id]["avatar"] = room_result.avatar

if room_result.heroes:
serialized_rooms[room_id]["heroes"] = room_result.heroes
if room_result.heroes is not None and len(room_result.heroes) > 0:
serialized_heroes = []
for hero in room_result.heroes:
serialized_hero = {
"user_id": hero.user_id,
}
if hero.display_name is not None:
# Not a typo, just how "displayname" is spelled in the spec
serialized_hero["displayname"] = hero.display_name

if hero.avatar_url is not None:
serialized_hero["avatar_url"] = hero.avatar_url

serialized_heroes.append(serialized_hero)
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`.
if room_result.initial:
serialized_rooms[room_id]["initial"] = room_result.initial

# This will be omitted for invite/knock rooms with `stripped_state`
if room_result.required_state is not None:
if (
room_result.required_state is not None
and len(room_result.required_state) > 0
):
serialized_required_state = (
await self.event_serializer.serialize_events(
room_result.required_state,
Expand All @@ -1015,7 +1031,10 @@ async def encode_rooms(
serialized_rooms[room_id]["required_state"] = serialized_required_state

# This will be omitted for invite/knock rooms with `stripped_state`
if room_result.timeline_events is not None:
if (
room_result.timeline_events is not None
and len(room_result.timeline_events) > 0
):
serialized_timeline = await self.event_serializer.serialize_events(
room_result.timeline_events,
time_now,
Expand Down Expand Up @@ -1043,7 +1062,10 @@ async def encode_rooms(
serialized_rooms[room_id]["is_dm"] = room_result.is_dm

# Stripped state only applies to invite/knock rooms
if room_result.stripped_state is not None:
if (
room_result.stripped_state is not None
and len(room_result.stripped_state) > 0
):
# TODO: `knocked_state` but that isn't specced yet.
#
# TODO: Instead of adding `knocked_state`, it would be good to rename
Expand Down
31 changes: 23 additions & 8 deletions synapse/storage/databases/main/roommember.py
Original file line number Diff line number Diff line change
Expand Up @@ -279,8 +279,19 @@ def _get_users_in_room_with_profiles(

@cached(max_entries=100000) # type: ignore[synapse-@cached-mutable]
async def get_room_summary(self, room_id: str) -> Mapping[str, MemberSummary]:
"""Get the details of a room roughly suitable for use by the room
"""
Get the details of a room roughly suitable for use by the room
summary extension to /sync. Useful when lazy loading room members.

Returns the total count of members in the room by membership type, and a
truncated list of members (the heroes). This will be the first 6 members of the
room:
- We want 5 heroes plus 1, in case one of them is the
calling user.
- They are ordered by `stream_ordering`, which are joined or
invited. When no joined or invited members are available, this also includes
banned and left users.

Args:
room_id: The room ID to query
Returns:
Expand Down Expand Up @@ -308,23 +319,27 @@ def _get_room_summary_txn(
for count, membership in txn:
res.setdefault(membership, MemberSummary([], count))

# we order by membership and then fairly arbitrarily by event_id so
# heroes are consistent
# Note, rejected events will have a null membership field, so
# we we manually filter them out.
# Order by membership (joins -> invites -> leave -> everything else), then by
# `stream_ordering` so the first members in the room show up first and to
# make the sort stable (consistent heroes).
#
# Note: rejected events will have a null membership field, so we we manually
# filter them out.
sql = """
SELECT state_key, membership, event_id
FROM current_state_events
WHERE type = 'm.room.member' AND room_id = ?
AND membership IS NOT NULL
ORDER BY
CASE membership WHEN ? THEN 1 WHEN ? THEN 2 ELSE 3 END ASC,
event_id ASC
CASE membership WHEN ? THEN 1 WHEN ? THEN 2 WHEN ? THEN 3 ELSE 4 END ASC,
event_stream_ordering ASC
Copy link
Collaborator Author

@MadLittleMods MadLittleMods Jul 9, 2024

Choose a reason for hiding this comment

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

By changing this now, it will probably cause peoples rooms names to calculate to something different across the board (this function is also used by Sync v2). Are we ok with that? Potentially disruptive if people are really used to their old "flawed" name.

It's already a FIXME comment in extract_heroes_from_room_summary(...) but we still need to pull in the right order from the database.

Related to matrix-org/matrix-spec#1334

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to change the order now? I think we only need to have a "stable" ordering?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The spec says this:

This should be the first 5 members of the room, ordered by stream ordering, which are joined or invited. The list must never include the client’s own user ID. When no joined or invited members are available, this should consist of the banned and left users.

-- https://spec.matrix.org/v1.10/client-server-api/#_matrixclientv3sync_roomsummary

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, interesting. Can we keep as is for now and open a separate PR / issue to consider this? I'm failing to think through whether this is going to cause some UX issues or not

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reverted in this PR and split out to #17435

LIMIT ?
"""

# 6 is 5 (number of heroes) plus 1, in case one of them is the calling user.
txn.execute(sql, (room_id, Membership.JOIN, Membership.INVITE, 6))
txn.execute(
sql, (room_id, Membership.JOIN, Membership.INVITE, Membership.LEAVE, 6)
)
for user_id, membership, event_id in txn:
summary = res[membership]
# we will always have a summary for this membership type at this
Expand Down
18 changes: 12 additions & 6 deletions synapse/types/handlers/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,18 +200,24 @@ class RoomResult:
flag set. (same as sync v2)
"""

@attr.s(slots=True, frozen=True, auto_attribs=True)
class StrippedHero:
user_id: str
display_name: Optional[str]
avatar_url: Optional[str]

name: Optional[str]
avatar: Optional[str]
heroes: Optional[List[EventBase]]
heroes: Optional[List[StrippedHero]]
is_dm: bool
initial: bool
# Only optional because it won't be included for invite/knock rooms with `stripped_state`
required_state: Optional[List[EventBase]]
# Only optional because it won't be included for invite/knock rooms with `stripped_state`
timeline_events: Optional[List[EventBase]]
# Should be empty for invite/knock rooms with `stripped_state`
required_state: List[EventBase]
# Should be empty for invite/knock rooms with `stripped_state`
timeline_events: List[EventBase]
bundled_aggregations: Optional[Dict[str, "BundledAggregations"]]
# Optional because it's only relevant to invite/knock rooms
stripped_state: Optional[List[JsonDict]]
stripped_state: List[JsonDict]
# Only optional because it won't be included for invite/knock rooms with `stripped_state`
prev_batch: Optional[StreamToken]
# Only optional because it won't be included for invite/knock rooms with `stripped_state`
Expand Down
4 changes: 0 additions & 4 deletions synapse/types/rest/client/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,9 +200,6 @@ class SlidingSyncList(CommonRoomParameters):
}

timeline_limit: The maximum number of timeline events to return per response.
include_heroes: Return a stripped variant of membership events (containing
`user_id` and optionally `avatar_url` and `displayname`) for the users used
to calculate the room name.
filters: Filters to apply to the list before sorting.
"""

Expand Down Expand Up @@ -270,7 +267,6 @@ class Filters(RequestBodyModel):
else:
ranges: Optional[List[Tuple[conint(ge=0, strict=True), conint(ge=0, strict=True)]]] = None # type: ignore[valid-type]
slow_get_all_rooms: Optional[StrictBool] = False
include_heroes: Optional[StrictBool] = False
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed include_heroes since it seems redundant, see matrix-org/matrix-spec-proposals#3575 (comment)

filters: Optional[Filters] = None

class RoomSubscription(CommonRoomParameters):
Expand Down
Loading
Loading