Add support for sending notification counts in simplified sliding sync #18290
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Add support for sending notification counts and thread notification counts in simplified sliding sync mode. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,17 @@ | |
| import itertools | ||
| import logging | ||
| from itertools import chain | ||
| from typing import ( | ||
| TYPE_CHECKING, | ||
| AbstractSet, | ||
| Dict, | ||
| List, | ||
| Mapping, | ||
| Optional, | ||
| Sequence, | ||
| Set, | ||
| Tuple, | ||
| ) | ||
|
|
||
| from prometheus_client import Histogram | ||
| from typing_extensions import assert_never | ||
|
|
@@ -38,6 +48,7 @@ | |
| tag_args, | ||
| trace, | ||
| ) | ||
| from synapse.storage.databases.main.receipts import ReceiptInRoom | ||
| from synapse.storage.databases.main.roommember import extract_heroes_from_room_summary | ||
| from synapse.storage.databases.main.state_deltas import StateDelta | ||
| from synapse.storage.databases.main.stream import PaginateFunction | ||
|
|
@@ -245,11 +256,31 @@ async def current_sync_for_user( | |
| to_token=to_token, | ||
| ) | ||
|
|
||
| # fetch the user's receipts between the two points: these will be factor | ||
| # in deciding whether to send the room, since it may have changed their | ||
| # notification counts | ||
| receipts = await self.store.get_linearized_receipts_for_user_in_rooms( | ||
| user_id=user_id, | ||
| room_ids=interested_rooms.relevant_room_map.keys(), | ||
| from_key=from_token.stream_token.receipt_key if from_token else None, | ||
| to_key=to_token.receipt_key, | ||
| ) | ||
|
|
||
| # Filtered subset of `relevant_room_map` for rooms that may have updates | ||
| # (in the event stream) | ||
| relevant_rooms_to_send_map = self.room_lists.filter_relevant_rooms_to_send( | ||
| sync_config.user, | ||
| previous_connection_state, | ||
| from_token.stream_token if from_token else None, | ||
| to_token, | ||
| interested_rooms.relevant_room_map, | ||
| receipts, | ||
| ) | ||
|
|
||
| lists = interested_rooms.lists | ||
| relevant_room_map = interested_rooms.relevant_room_map | ||
| all_rooms = interested_rooms.all_rooms | ||
| room_membership_for_user_map = interested_rooms.room_membership_for_user_map | ||
|
|
||
| # Fetch room data | ||
| rooms: Dict[str, SlidingSyncResult.RoomResult] = {} | ||
|
|
@@ -272,6 +303,7 @@ async def handle_room(room_id: str) -> None: | |
| to_token=to_token, | ||
| newly_joined=room_id in interested_rooms.newly_joined_rooms, | ||
| is_dm=room_id in interested_rooms.dm_room_ids, | ||
| room_receipts=receipts[room_id] if room_id in receipts else None, | ||
| ) | ||
|
|
||
| # Filter out empty room results during incremental sync | ||
|
|
@@ -296,6 +328,7 @@ async def handle_room(room_id: str) -> None: | |
| actual_room_response_map=rooms, | ||
| from_token=from_token, | ||
| to_token=to_token, | ||
| user_receipts=receipts, | ||
| ) | ||
|
|
||
| if has_lists or has_room_subscriptions: | ||
|
|
@@ -543,6 +576,7 @@ async def get_room_sync_data( | |
| to_token: StreamToken, | ||
| newly_joined: bool, | ||
| is_dm: bool, | ||
| room_receipts: Optional[Sequence[ReceiptInRoom]], | ||
| ) -> SlidingSyncResult.RoomResult: | ||
| """ | ||
| Fetch room data for the sync response. | ||
|
|
@@ -560,6 +594,8 @@ async def get_room_sync_data( | |
| to_token: The point in the stream to sync up to. | ||
| newly_joined: If the user has newly joined the room | ||
| is_dm: Whether the room is a DM room | ||
| room_receipts: Any read receipts from the in question in that room between | ||
| from_token and to_token | ||
| """ | ||
| user = sync_config.user | ||
|
|
||
|
|
@@ -1312,6 +1348,11 @@ async def get_room_sync_data( | |
|
|
||
| set_tag(SynapseTags.RESULT_PREFIX + "initial", initial) | ||
|
|
||
| unread_notifs = await self.store.get_unread_event_push_actions_by_room_for_user( | ||
| room_id, | ||
| sync_config.user.to_string(), | ||
| ) | ||
|
|
||
| return SlidingSyncResult.RoomResult( | ||
| name=room_name, | ||
| avatar=room_avatar, | ||
|
|
@@ -1329,11 +1370,8 @@ async def get_room_sync_data( | |
| bump_stamp=bump_stamp, | ||
| joined_count=joined_count, | ||
| invited_count=invited_count, | ||
|
Comment on lines
-1332
to
-1336
There was a problem hiding this comment. @erikjohnston Are we even interested in including notification values? I've also asked this question on the MSC itself: matrix-org/matrix-spec-proposals#4186 (comment) There was a problem hiding this comment. @dbkr It would also be useful to understand how notification counts are calculated on Element Web today. I assume the Element X apps are accomplishing something without these counts so could Element Web adopt the same strategy? There was a problem hiding this comment. Yep, EX doesn't do backfill as such but does obviously keep history and generally wake up to receive messages via push, so it calculates them all locally and in practice this seems to be enough. Element Web doesn't do either of these things and so currently does a slightly funky combination of trusting server counts for unencrypted rooms and fixing them up locally for encrypted rooms, except when the server sets them to 0 when we know to clear the count. Ultimately we would probably like to backfill to calculate the notification counts entirely client side, although probably just for e2e rooms: doing this for large public rooms is arguably a lot of work unnecessarily, so I feel like the path to Doing It Right involves having these counts at least to use in unencrypted rooms (where Doing It Right in this case involves Matrix in its dual role of trying to be a both an e2e messenger and one for large public groups, but that's where we are). Meanwhile, EW also downloads the entire history of e2e rooms on desktop in the seshat search index, but unfortunately that's it's own thing, so really we would have to replace seshat first, otherwise we'd have to bodge the two together or end up downloading e2e room history twice. |
||
| notif_counts=unread_notifs, | ||
| room_receipts=room_receipts, | ||
| ) | ||
|
|
||
| @trace | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -547,13 +547,20 @@ def _get_unread_counts_by_receipt_txn( | |
| # If the user has no receipts in the room, retrieve the stream ordering for | ||
| # the latest membership event from this user in this room (which we assume is | ||
| # a join). | ||
| # Sometimes (usually state resets) there can be no membership event either, | ||
| # so we allow None and return no notifications which is probably about | ||
| # the best we can do short of failing outright. | ||
| event_id = self.db_pool.simple_select_one_onecol_txn( | ||
| txn=txn, | ||
| table="local_current_membership", | ||
| keyvalues={"room_id": room_id, "user_id": user_id}, | ||
| retcol="event_id", | ||
| allow_none=True, | ||
| ) | ||
|
|
||
| if event_id is None: | ||
| return _EMPTY_ROOM_NOTIF_COUNTS | ||
|
|
||
|
There was a problem hiding this comment. This bit I'm particularly unsure about but was needed to make https://github.com/element-hq/synapse/blob/develop/tests/rest/client/sliding_sync/test_sliding_sync.py#L1111 test pass again since this function just threw in that case. This is basically lying. We could fail the sync, but the test explicitly tests that it works. We could assume the stream ordering is the lowest possible which might be more accurate? There was a problem hiding this comment. I think in a lot of cases, a normal user would probably have a read receipt in the room and we would return that like normal with the logic above. But in our tests, we never send any read receipts so we hit this logic path. I think it makes sense to return empty notification counts in this case as they've literally never read anything in the room before and were removed before they even tried. |
||
| stream_ordering = self.get_stream_id_for_event_txn(txn, event_id) | ||
|
|
||
| return self._get_unread_counts_by_pos_txn( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way we can share the work with the
receiptsextension if it's enabled. Seems less than ideal to have to runget_linearized_receipts_for_user_in_rooms(...)twice.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle, yes, although this uses a the function
get_linearized_receipts_for_roomswhich returns a different type for reasons I don't really understand:JsonMappingvsReceiptInRoom: is one just converting to a real type and the other isn't? Apart from that, I guess it should be possible to either move the receipts logic upwards to fetch all the receipts and pass them into the main sync part.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh actually, the receipts extension calls
get_linearized_receipts_for_user_in_roomstoo, so presumably we can just pass this in and then we'll be doing the same number of database queries as before. The receipts extension doesn't give a 'from' key, though, which seems weird.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, done.