From bc2a3dbc47ccdfdbd5a1515a21c865be2718c165 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Sat, 5 Mar 2022 09:14:36 +0100 Subject: [PATCH 01/42] Changelog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- changelog.d/12168.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12168.feature diff --git a/changelog.d/12168.feature b/changelog.d/12168.feature new file mode 100644 index 000000000000..cd5c45029ee1 --- /dev/null +++ b/changelog.d/12168.feature @@ -0,0 +1 @@ +Implement [changes](https://github.com/matrix-org/matrix-spec-proposals/pull/2285/commits/4a77139249c2e830aec3c7d6bd5501a514d1cc27) to [MSC2285 (hidden read receipts)](https://github.com/matrix-org/matrix-spec-proposals/pull/2285). Contributed by @SimonBrandner. From 926bd8434c9c9fcb534d23d16d9cbd135bbaa3c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Sat, 5 Mar 2022 09:01:18 +0100 Subject: [PATCH 02/42] Implement changes to MSC2285 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/api/constants.py | 6 +- synapse/handlers/receipts.py | 51 ++++++-------- synapse/handlers/sync.py | 3 +- synapse/push/push_tools.py | 3 +- synapse/rest/client/notifications.py | 3 +- synapse/rest/client/read_marker.py | 26 ++++--- synapse/rest/client/receipts.py | 47 +++++++------ synapse/storage/databases/main/receipts.py | 46 +++++++------ tests/handlers/test_receipts.py | 68 ++++++++++--------- .../slave/storage/test_receipts.py | 4 +- tests/rest/client/test_sync.py | 19 ++---- 11 files changed, 134 insertions(+), 142 deletions(-) diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 36ace7c6134f..b4534ddd1ca9 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -255,7 +255,5 @@ class GuestAccess: class ReceiptTypes: READ: Final = "m.read" - - -class ReadReceiptEventFields: - MSC2285_HIDDEN: Final = "org.matrix.msc2285.hidden" + READ_PRIVATE: Final = "org.matrix.msc2285.read.private" + FULLY_READ: Final = "m.fully_read" diff --git a/synapse/handlers/receipts.py b/synapse/handlers/receipts.py index 6250bb3bdf2b..b5971bdbfc1b 100644 --- a/synapse/handlers/receipts.py +++ b/synapse/handlers/receipts.py @@ -14,7 +14,7 @@ import logging from typing import TYPE_CHECKING, Iterable, List, Optional, Tuple -from synapse.api.constants import ReadReceiptEventFields, ReceiptTypes +from synapse.api.constants import ReceiptTypes from synapse.appservice import ApplicationService from synapse.streams import EventSource from synapse.types import JsonDict, ReadReceipt, UserID, get_domain_from_id @@ -138,7 +138,7 @@ async def _handle_new_receipts(self, receipts: List[ReadReceipt]) -> bool: return True async def received_client_receipt( - self, room_id: str, receipt_type: str, user_id: str, event_id: str, hidden: bool + self, room_id: str, receipt_type: str, user_id: str, event_id: str ) -> None: """Called when a client tells us a local user has read up to the given event_id in the room. @@ -148,7 +148,7 @@ async def received_client_receipt( receipt_type=receipt_type, user_id=user_id, event_ids=[event_id], - data={"ts": int(self.clock.time_msec()), "hidden": hidden}, + data={"ts": int(self.clock.time_msec())}, ) is_new = await self._handle_new_receipts([receipt]) @@ -156,7 +156,8 @@ async def received_client_receipt( return if self.federation_sender and not ( - self.hs.config.experimental.msc2285_enabled and hidden + self.hs.config.experimental.msc2285_enabled + and receipt_type == ReceiptTypes.READ_PRIVATE ): await self.federation_sender.send_read_receipt(receipt) @@ -178,35 +179,27 @@ def filter_out_hidden(events: List[JsonDict], user_id: str) -> List[JsonDict]: for event_id in content.keys(): event_content = content.get(event_id, {}) - m_read = event_content.get(ReceiptTypes.READ, {}) - # If m_read is missing copy over the original event_content as there is nothing to process here - if not m_read: - new_event["content"][event_id] = event_content.copy() + m_read = event_content.get(ReceiptTypes.READ, None) + if m_read: + new_event["content"][event_id] = {ReceiptTypes.READ: m_read} continue - new_users = {} - for rr_user_id, user_rr in m_read.items(): - try: - hidden = user_rr.get("hidden") - except AttributeError: - # Due to https://github.com/matrix-org/synapse/issues/10376 - # there are cases where user_rr is a string, in those cases - # we just ignore the read receipt - continue + m_read_private = event_content.get(ReceiptTypes.READ_PRIVATE, None) + if m_read_private: + new_users = {} + for rr_user_id, user_rr in m_read_private.items(): + if rr_user_id == user_id: + new_users[rr_user_id] = user_rr.copy() + + # Set new users unless empty + if len(new_users.keys()) > 0: + new_event["content"][event_id] = { + ReceiptTypes.READ_PRIVATE: new_users + } + continue - if hidden is not True or rr_user_id == user_id: - new_users[rr_user_id] = user_rr.copy() - # If hidden has a value replace hidden with the correct prefixed key - if hidden is not None: - new_users[rr_user_id].pop("hidden") - new_users[rr_user_id][ - ReadReceiptEventFields.MSC2285_HIDDEN - ] = hidden - - # Set new users unless empty - if len(new_users.keys()) > 0: - new_event["content"][event_id] = {ReceiptTypes.READ: new_users} + new_event["content"][event_id] = event_content # Append new_event to visible_events unless empty if len(new_event["content"].keys()) > 0: diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index 0aa3052fd6ac..137905f084ed 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -28,7 +28,7 @@ import attr from prometheus_client import Counter -from synapse.api.constants import AccountDataTypes, EventTypes, Membership, ReceiptTypes +from synapse.api.constants import AccountDataTypes, EventTypes, Membership from synapse.api.filtering import FilterCollection from synapse.api.presence import UserPresenceState from synapse.api.room_versions import KNOWN_ROOM_VERSIONS @@ -1065,7 +1065,6 @@ async def unread_notifs_for_room_id( last_unread_event_id = await self.store.get_last_receipt_event_id_for_user( user_id=sync_config.user.to_string(), room_id=room_id, - receipt_type=ReceiptTypes.READ, ) return await self.store.get_unread_event_push_actions_by_room_for_user( diff --git a/synapse/push/push_tools.py b/synapse/push/push_tools.py index 957c9b780b94..b10d9b03ef17 100644 --- a/synapse/push/push_tools.py +++ b/synapse/push/push_tools.py @@ -13,7 +13,6 @@ # limitations under the License. from typing import Dict -from synapse.api.constants import ReceiptTypes from synapse.events import EventBase from synapse.push.presentable_names import calculate_room_name, name_from_member_event from synapse.storage import Storage @@ -24,7 +23,7 @@ async def get_badge_count(store: DataStore, user_id: str, group_by_room: bool) - invites = await store.get_invited_rooms_for_local_user(user_id) joins = await store.get_rooms_for_user(user_id) - my_receipts_by_room = await store.get_receipts_for_user(user_id, ReceiptTypes.READ) + my_receipts_by_room = await store.get_receipts_for_user(user_id) badge = len(invites) diff --git a/synapse/rest/client/notifications.py b/synapse/rest/client/notifications.py index ff040de6b840..8ee92d9d47b7 100644 --- a/synapse/rest/client/notifications.py +++ b/synapse/rest/client/notifications.py @@ -15,7 +15,6 @@ import logging from typing import TYPE_CHECKING, Tuple -from synapse.api.constants import ReceiptTypes from synapse.events.utils import ( SerializeEventConfig, format_event_for_client_v2_without_room_id, @@ -58,7 +57,7 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: ) receipts_by_room = await self.store.get_receipts_for_user_with_orderings( - user_id, ReceiptTypes.READ + user_id ) notif_event_ids = [pa.event_id for pa in push_actions] diff --git a/synapse/rest/client/read_marker.py b/synapse/rest/client/read_marker.py index f51be511d1f4..47625d150f4e 100644 --- a/synapse/rest/client/read_marker.py +++ b/synapse/rest/client/read_marker.py @@ -15,8 +15,7 @@ import logging from typing import TYPE_CHECKING, Tuple -from synapse.api.constants import ReadReceiptEventFields, ReceiptTypes -from synapse.api.errors import Codes, SynapseError +from synapse.api.constants import ReceiptTypes from synapse.http.server import HttpServer from synapse.http.servlet import RestServlet, parse_json_object_from_request from synapse.http.site import SynapseRequest @@ -48,27 +47,26 @@ async def on_POST( await self.presence_handler.bump_presence_active_time(requester.user) body = parse_json_object_from_request(request) - read_event_id = body.get(ReceiptTypes.READ, None) - hidden = body.get(ReadReceiptEventFields.MSC2285_HIDDEN, False) - - if not isinstance(hidden, bool): - raise SynapseError( - 400, - "Param %s must be a boolean, if given" - % ReadReceiptEventFields.MSC2285_HIDDEN, - Codes.BAD_JSON, - ) + read_event_id = body.get(ReceiptTypes.READ, None) if read_event_id: await self.receipts_handler.received_client_receipt( room_id, ReceiptTypes.READ, user_id=requester.user.to_string(), event_id=read_event_id, - hidden=hidden, ) - read_marker_event_id = body.get("m.fully_read", None) + read_private_event_id = body.get(ReceiptTypes.READ_PRIVATE, None) + if read_private_event_id: + await self.receipts_handler.received_client_receipt( + room_id, + ReceiptTypes.READ_PRIVATE, + user_id=requester.user.to_string(), + event_id=read_private_event_id, + ) + + read_marker_event_id = body.get(ReceiptTypes.FULLY_READ, None) if read_marker_event_id: await self.read_marker_handler.received_client_read_marker( room_id, diff --git a/synapse/rest/client/receipts.py b/synapse/rest/client/receipts.py index b24ad2d1be13..d0a564ebf43f 100644 --- a/synapse/rest/client/receipts.py +++ b/synapse/rest/client/receipts.py @@ -16,8 +16,8 @@ import re from typing import TYPE_CHECKING, Tuple -from synapse.api.constants import ReadReceiptEventFields, ReceiptTypes -from synapse.api.errors import Codes, SynapseError +from synapse.api.constants import ReceiptTypes +from synapse.api.errors import SynapseError from synapse.http import get_request_user_agent from synapse.http.server import HttpServer from synapse.http.servlet import RestServlet, parse_json_object_from_request @@ -46,6 +46,7 @@ def __init__(self, hs: "HomeServer"): self.hs = hs self.auth = hs.get_auth() self.receipts_handler = hs.get_receipts_handler() + self.read_marker_handler = hs.get_read_marker_handler() self.presence_handler = hs.get_presence_handler() async def on_POST( @@ -53,8 +54,15 @@ async def on_POST( ) -> Tuple[int, JsonDict]: requester = await self.auth.get_user_by_req(request) - if receipt_type != ReceiptTypes.READ: - raise SynapseError(400, "Receipt type must be 'm.read'") + if receipt_type not in [ + ReceiptTypes.READ, + ReceiptTypes.READ_PRIVATE, + ReceiptTypes.FULLY_READ, + ]: + raise SynapseError( + 400, + "Receipt type must be 'm.read', 'org.matrix.msc2285.read.private' or 'm.fully_read'", + ) # Do not allow older SchildiChat and Element Android clients (prior to Element/1.[012].x) to send an empty body. user_agent = get_request_user_agent(request) @@ -62,26 +70,23 @@ async def on_POST( if "Android" in user_agent: if pattern.match(user_agent) or "Riot" in user_agent: allow_empty_body = True - body = parse_json_object_from_request(request, allow_empty_body) - hidden = body.get(ReadReceiptEventFields.MSC2285_HIDDEN, False) - - if not isinstance(hidden, bool): - raise SynapseError( - 400, - "Param %s must be a boolean, if given" - % ReadReceiptEventFields.MSC2285_HIDDEN, - Codes.BAD_JSON, - ) + parse_json_object_from_request(request, allow_empty_body) await self.presence_handler.bump_presence_active_time(requester.user) - await self.receipts_handler.received_client_receipt( - room_id, - receipt_type, - user_id=requester.user.to_string(), - event_id=event_id, - hidden=hidden, - ) + if receipt_type == ReceiptTypes.FULLY_READ: + await self.read_marker_handler.received_client_read_marker( + room_id, + user_id=requester.user.to_string(), + event_id=event_id, + ) + else: + await self.receipts_handler.received_client_receipt( + room_id, + receipt_type, + user_id=requester.user.to_string(), + event_id=event_id, + ) return 200, {} diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index bf0b903af2fc..5f9ea4991807 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -118,15 +118,14 @@ async def get_receipts_for_room( desc="get_receipts_for_room", ) - @cached(num_args=3) + @cached(num_args=2) async def get_last_receipt_event_id_for_user( - self, user_id: str, room_id: str, receipt_type: str + self, user_id: str, room_id: str ) -> Optional[str]: return await self.db_pool.simple_select_one_onecol( table="receipts_linearized", keyvalues={ "room_id": room_id, - "receipt_type": receipt_type, "user_id": user_id, }, retcol="event_id", @@ -134,22 +133,18 @@ async def get_last_receipt_event_id_for_user( allow_none=True, ) - @cached(num_args=2) - async def get_receipts_for_user( - self, user_id: str, receipt_type: str - ) -> Dict[str, str]: + @cached(num_args=1) + async def get_receipts_for_user(self, user_id: str) -> Dict[str, str]: rows = await self.db_pool.simple_select_list( table="receipts_linearized", - keyvalues={"user_id": user_id, "receipt_type": receipt_type}, + keyvalues={"user_id": user_id}, retcols=("room_id", "event_id"), desc="get_receipts_for_user", ) return {row["room_id"]: row["event_id"] for row in rows} - async def get_receipts_for_user_with_orderings( - self, user_id: str, receipt_type: str - ) -> JsonDict: + async def get_receipts_for_user_with_orderings(self, user_id: str) -> JsonDict: def f(txn: LoggingTransaction) -> List[Tuple[str, str, int, int]]: sql = ( "SELECT rl.room_id, rl.event_id," @@ -490,9 +485,7 @@ def invalidate_caches_for_receipt( ) -> None: self.get_receipts_for_user.invalidate((user_id, receipt_type)) self._get_linearized_receipts_for_room.invalidate((room_id,)) - self.get_last_receipt_event_id_for_user.invalidate( - (user_id, room_id, receipt_type) - ) + self.get_last_receipt_event_id_for_user.invalidate((user_id, room_id)) self._invalidate_get_users_with_receipts_in_room(room_id, receipt_type, user_id) self.get_receipts_for_room.invalidate((room_id, receipt_type)) @@ -541,14 +534,20 @@ def insert_linearized_receipt_txn( # have to compare orderings of existing receipts if stream_ordering is not None: sql = ( - "SELECT stream_ordering, event_id FROM events" - " INNER JOIN receipts_linearized as r USING (event_id, room_id)" - " WHERE r.room_id = ? AND r.receipt_type = ? AND r.user_id = ?" + "SELECT e.stream_ordering, e.event_id, r.receipt_type FROM events AS e" + " INNER JOIN receipts_linearized AS r USING (event_id, room_id)" + " WHERE r.room_id = ? AND r.user_id = ?" ) - txn.execute(sql, (room_id, receipt_type, user_id)) - - for so, eid in txn: - if int(so) >= stream_ordering: + txn.execute(sql, (room_id, user_id)) + + for so, eid, rt in txn: + if int(so) >= stream_ordering and ( + receipt_type == rt + or ( + rt == ReceiptTypes.READ + and receipt_type == ReceiptTypes.READ_PRIVATE + ) + ): logger.debug( "Ignoring new receipt for %s in favour of existing " "one for later event %s", @@ -583,7 +582,10 @@ def insert_linearized_receipt_txn( lock=False, ) - if receipt_type == ReceiptTypes.READ and stream_ordering is not None: + if ( + receipt_type in [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE] + and stream_ordering is not None + ): self._remove_old_push_actions_before_txn( txn, room_id=room_id, user_id=user_id, stream_ordering=stream_ordering ) diff --git a/tests/handlers/test_receipts.py b/tests/handlers/test_receipts.py index 5081b97573a0..f00275109388 100644 --- a/tests/handlers/test_receipts.py +++ b/tests/handlers/test_receipts.py @@ -15,7 +15,7 @@ from typing import List -from synapse.api.constants import ReadReceiptEventFields +from synapse.api.constants import ReceiptTypes from synapse.types import JsonDict from tests import unittest @@ -25,20 +25,15 @@ class ReceiptsTestCase(unittest.HomeserverTestCase): def prepare(self, reactor, clock, hs): self.event_source = hs.get_event_sources().sources.receipt - # In the first param of _test_filters_hidden we use "hidden" instead of - # ReadReceiptEventFields.MSC2285_HIDDEN. We do this because we're mocking - # the data from the database which doesn't use the prefix - def test_filters_out_hidden_receipt(self): self._test_filters_hidden( [ { "content": { "$1435641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ_PRIVATE: { "@rikj:jki.re": { "ts": 1436451550453, - "hidden": True, } } } @@ -56,10 +51,9 @@ def test_does_not_filter_out_our_hidden_receipt(self): { "content": { "$1435641916hfgh4394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ_PRIVATE: { "@me:server.org": { "ts": 1436451550453, - "hidden": True, }, } } @@ -72,10 +66,9 @@ def test_does_not_filter_out_our_hidden_receipt(self): { "content": { "$1435641916hfgh4394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ_PRIVATE: { "@me:server.org": { "ts": 1436451550453, - ReadReceiptEventFields.MSC2285_HIDDEN: True, }, } } @@ -92,16 +85,17 @@ def test_filters_out_hidden_receipt_and_ignores_rest(self): { "content": { "$1dgdgrd5641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ_PRIVATE: { "@rikj:jki.re": { "ts": 1436451550453, - "hidden": True, }, + }, + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, }, - } - } + }, + }, }, "room_id": "!jEsUZKDJdhlrceRyVU:example.org", "type": "m.receipt", @@ -111,7 +105,7 @@ def test_filters_out_hidden_receipt_and_ignores_rest(self): { "content": { "$1dgdgrd5641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, } @@ -130,15 +124,14 @@ def test_filters_out_event_with_only_hidden_receipts_and_ignores_the_rest(self): { "content": { "$14356419edgd14394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ_PRIVATE: { "@rikj:jki.re": { "ts": 1436451550453, - "hidden": True, }, } }, "$1435641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, } @@ -153,7 +146,7 @@ def test_filters_out_event_with_only_hidden_receipts_and_ignores_the_rest(self): { "content": { "$1435641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, } @@ -171,9 +164,9 @@ def test_handles_missing_content_of_m_read(self): [ { "content": { - "$14356419ggffg114394fHBLK:matrix.org": {"m.read": {}}, + "$14356419ggffg114394fHBLK:matrix.org": {ReceiptTypes.READ: {}}, "$1435641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, } @@ -187,9 +180,9 @@ def test_handles_missing_content_of_m_read(self): [ { "content": { - "$14356419ggffg114394fHBLK:matrix.org": {"m.read": {}}, + "$14356419ggffg114394fHBLK:matrix.org": {ReceiptTypes.READ: {}}, "$1435641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, } @@ -209,7 +202,7 @@ def test_handles_empty_event(self): "content": { "$143564gdfg6114394fHBLK:matrix.org": {}, "$1435641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, } @@ -225,7 +218,7 @@ def test_handles_empty_event(self): "content": { "$143564gdfg6114394fHBLK:matrix.org": {}, "$1435641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, } @@ -244,10 +237,9 @@ def test_filters_out_receipt_event_with_only_hidden_receipt_and_ignores_rest(sel { "content": { "$14356419edgd14394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ_PRIVATE: { "@rikj:jki.re": { "ts": 1436451550453, - "hidden": True, }, } }, @@ -258,7 +250,7 @@ def test_filters_out_receipt_event_with_only_hidden_receipt_and_ignores_rest(sel { "content": { "$1435641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, } @@ -273,7 +265,7 @@ def test_filters_out_receipt_event_with_only_hidden_receipt_and_ignores_rest(sel { "content": { "$1435641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, } @@ -297,7 +289,20 @@ def test_handles_string_data(self): { "content": { "$14356419edgd14394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { + "@rikj:jki.re": "string", + } + }, + }, + "room_id": "!jEsUZKDJdhlrceRyVU:example.org", + "type": "m.receipt", + }, + ], + [ + { + "content": { + "$14356419edgd14394fHBLK:matrix.org": { + ReceiptTypes.READ: { "@rikj:jki.re": "string", } }, @@ -306,7 +311,6 @@ def test_handles_string_data(self): "type": "m.receipt", }, ], - [], ) def _test_filters_hidden( diff --git a/tests/replication/slave/storage/test_receipts.py b/tests/replication/slave/storage/test_receipts.py index f47d94f690f9..f03bc9882eaa 100644 --- a/tests/replication/slave/storage/test_receipts.py +++ b/tests/replication/slave/storage/test_receipts.py @@ -26,9 +26,9 @@ class SlavedReceiptTestCase(BaseSlavedStoreTestCase): STORE_TYPE = SlavedReceiptsStore def test_receipt(self): - self.check("get_receipts_for_user", [USER_ID, "m.read"], {}) + self.check("get_receipts_for_user", [USER_ID], {}) self.get_success( self.master_store.insert_receipt(ROOM_ID, "m.read", USER_ID, [EVENT_ID], {}) ) self.replicate() - self.check("get_receipts_for_user", [USER_ID, "m.read"], {ROOM_ID: EVENT_ID}) + self.check("get_receipts_for_user", [USER_ID], {ROOM_ID: EVENT_ID}) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 435101395204..ddf03f75a0fc 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -20,12 +20,7 @@ from twisted.test.proto_helpers import MemoryReactor import synapse.rest.admin -from synapse.api.constants import ( - EventContentFields, - EventTypes, - ReadReceiptEventFields, - RelationTypes, -) +from synapse.api.constants import EventContentFields, EventTypes, RelationTypes from synapse.rest.client import devices, knock, login, read_marker, receipts, room, sync from synapse.server import HomeServer from synapse.types import JsonDict @@ -413,11 +408,11 @@ def test_hidden_read_receipts(self) -> None: res = self.helper.send(self.room_id, body="hello", tok=self.tok) # Send a read receipt to tell the server the first user's message was read - body = json.dumps({ReadReceiptEventFields.MSC2285_HIDDEN: True}).encode("utf8") channel = self.make_request( "POST", - "/rooms/%s/receipt/m.read/%s" % (self.room_id, res["event_id"]), - body, + "/rooms/%s/receipt/org.matrix.msc2285.read.private/%s" + % (self.room_id, res["event_id"]), + {}, access_token=self.tok2, ) self.assertEqual(channel.code, 200) @@ -573,11 +568,11 @@ def test_unread_counts(self) -> None: self._check_unread_count(1) # Send a read receipt to tell the server we've read the latest event. - body = json.dumps({ReadReceiptEventFields.MSC2285_HIDDEN: True}).encode("utf8") channel = self.make_request( "POST", - "/rooms/%s/receipt/m.read/%s" % (self.room_id, res["event_id"]), - body, + "/rooms/%s/receipt/org.matrix.msc2285.read.private/%s" + % (self.room_id, res["event_id"]), + {}, access_token=self.tok, ) self.assertEqual(channel.code, 200, channel.json_body) From 4bf0a3687c2441bad7e228cead12126a41be9ecf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Sat, 5 Mar 2022 21:51:18 +0100 Subject: [PATCH 03/42] Improve tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- tests/rest/client/test_sync.py | 79 +++++++++++++++++++++++++++++++++- 1 file changed, 78 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index ddf03f75a0fc..f326d4aced36 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -634,13 +634,72 @@ def test_unread_counts(self) -> None: self._check_unread_count(4) # Check that tombstone events changes increase the unread counter. - self.helper.send_state( + res1 = self.helper.send_state( self.room_id, EventTypes.Tombstone, {"replacement_room": "!someroom:test"}, tok=self.tok2, ) self._check_unread_count(5) + res2 = self.helper.send(self.room_id, "hello", tok=self.tok2) + + # Make sure both m.read and org.matrix.msc2285.read.private advance + channel = self.make_request( + "POST", + "/rooms/%s/receipt/m.read/%s" % (self.room_id, res1["event_id"]), + {}, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self._check_unread_count(1) + + channel = self.make_request( + "POST", + "/rooms/%s/receipt/org.matrix.msc2285.read.private/%s" + % (self.room_id, res2["event_id"]), + {}, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self._check_unread_count(0) + + def test_doesnt_reverse_read_receipts(self) -> None: + # Join the new user + self.helper.join(room=self.room_id, user=self.user2, tok=self.tok2) + + # Send messages + res1 = self.helper.send(self.room_id, "hello", tok=self.tok2) + res2 = self.helper.send(self.room_id, "hello", tok=self.tok2) + + # Read last event + channel = self.make_request( + "POST", + "/rooms/%s/receipt/m.read/%s" % (self.room_id, res2["event_id"]), + {}, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self._check_unread_count(0) + + # Make sure neither m.read not org.matrix.msc2285.read.private reverse + channel = self.make_request( + "POST", + "/rooms/%s/receipt/org.matrix.msc2285.read.private/%s" + % (self.room_id, res1["event_id"]), + {}, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self._check_no_room_changes() + + channel = self.make_request( + "POST", + "/rooms/%s/receipt/m.read/%s" % (self.room_id, res1["event_id"]), + {}, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self._check_no_room_changes() def _check_unread_count(self, expected_count: int) -> None: """Syncs and compares the unread count with the expected value.""" @@ -663,6 +722,24 @@ def _check_unread_count(self, expected_count: int) -> None: # Store the next batch for the next request. self.next_batch = channel.json_body["next_batch"] + def _check_no_room_changes(self) -> None: + """Syncs and makes sure the rooms part of sync is empty.""" + + channel = self.make_request( + "GET", + self.url % self.next_batch, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self.assertEqual( + channel.json_body.get("rooms", None), + None, + channel.json_body, + ) + + # Store the next batch for the next request. + self.next_batch = channel.json_body["next_batch"] + class SyncCacheTestCase(unittest.HomeserverTestCase): servlets = [ From b4e6eeafc0d81bbc04b971aaacb5445b33d89bf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Thu, 10 Mar 2022 17:33:27 +0100 Subject: [PATCH 04/42] Add checks for `msc2285_enabled` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/rest/client/receipts.py | 7 ++++++- tests/rest/client/test_sync.py | 2 ++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/synapse/rest/client/receipts.py b/synapse/rest/client/receipts.py index d0a564ebf43f..a53b584ec564 100644 --- a/synapse/rest/client/receipts.py +++ b/synapse/rest/client/receipts.py @@ -54,7 +54,7 @@ async def on_POST( ) -> Tuple[int, JsonDict]: requester = await self.auth.get_user_by_req(request) - if receipt_type not in [ + if self.hs.config.experimental.msc2285_enabled and receipt_type not in [ ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE, ReceiptTypes.FULLY_READ, @@ -63,6 +63,11 @@ async def on_POST( 400, "Receipt type must be 'm.read', 'org.matrix.msc2285.read.private' or 'm.fully_read'", ) + elif ( + not self.hs.config.experimental.msc2285_enabled + and receipt_type != ReceiptTypes.READ + ): + raise SynapseError(400, "Receipt type must be 'm.read'") # Do not allow older SchildiChat and Element Android clients (prior to Element/1.[012].x) to send an empty body. user_agent = get_request_user_agent(request) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index f326d4aced36..be41c48c8f3e 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -535,6 +535,7 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: tok=self.tok, ) + @override_config({"experimental_features": {"msc2285_enabled": True}}) def test_unread_counts(self) -> None: """Tests that /sync returns the right value for the unread count (MSC2654).""" @@ -663,6 +664,7 @@ def test_unread_counts(self) -> None: self.assertEqual(channel.code, 200, channel.json_body) self._check_unread_count(0) + @override_config({"experimental_features": {"msc2285_enabled": True}}) def test_doesnt_reverse_read_receipts(self) -> None: # Join the new user self.helper.join(room=self.room_id, user=self.user2, tok=self.tok2) From 18a96d24d0b26e4545293309323a9414fcb91c1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Thu, 10 Mar 2022 17:38:13 +0100 Subject: [PATCH 05/42] Simplifie `filter_out_hidden()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/handlers/receipts.py | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/synapse/handlers/receipts.py b/synapse/handlers/receipts.py index b5971bdbfc1b..470c75fbf0c8 100644 --- a/synapse/handlers/receipts.py +++ b/synapse/handlers/receipts.py @@ -187,15 +187,10 @@ def filter_out_hidden(events: List[JsonDict], user_id: str) -> List[JsonDict]: m_read_private = event_content.get(ReceiptTypes.READ_PRIVATE, None) if m_read_private: - new_users = {} - for rr_user_id, user_rr in m_read_private.items(): - if rr_user_id == user_id: - new_users[rr_user_id] = user_rr.copy() - - # Set new users unless empty - if len(new_users.keys()) > 0: + user_rr = m_read_private.get(user_id, None) + if user_rr: new_event["content"][event_id] = { - ReceiptTypes.READ_PRIVATE: new_users + ReceiptTypes.READ_PRIVATE: {user_id: user_rr.copy()} } continue From 6d017cee0d3b10760d4b17353f55c51482f3c28c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Thu, 10 Mar 2022 17:45:22 +0100 Subject: [PATCH 06/42] Don't recheck `msc2285_enabled` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `m.read.private` shouldn't even get here if `msc2285_enabled` is disabled Signed-off-by: Šimon Brandner --- synapse/handlers/receipts.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/synapse/handlers/receipts.py b/synapse/handlers/receipts.py index 470c75fbf0c8..58cee8d4f0d0 100644 --- a/synapse/handlers/receipts.py +++ b/synapse/handlers/receipts.py @@ -155,10 +155,7 @@ async def received_client_receipt( if not is_new: return - if self.federation_sender and not ( - self.hs.config.experimental.msc2285_enabled - and receipt_type == ReceiptTypes.READ_PRIVATE - ): + if self.federation_sender and receipt_type != ReceiptTypes.READ_PRIVATE: await self.federation_sender.send_read_receipt(receipt) From 77879c00e2e25846d5072c4cfdd5e2ad2a26e26a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Thu, 10 Mar 2022 17:51:22 +0100 Subject: [PATCH 07/42] Add some comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/handlers/receipts.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/synapse/handlers/receipts.py b/synapse/handlers/receipts.py index 58cee8d4f0d0..6c75b7b3527d 100644 --- a/synapse/handlers/receipts.py +++ b/synapse/handlers/receipts.py @@ -166,6 +166,13 @@ def __init__(self, hs: "HomeServer"): @staticmethod def filter_out_hidden(events: List[JsonDict], user_id: str) -> List[JsonDict]: + """ + This method takes in what is returned by + get_linearized_receipts_for_rooms() and goes through read receipts + filtering out m.read.private receipts if they were not sent by the + current user. + """ + visible_events = [] # filter out hidden receipts the user shouldn't see @@ -182,6 +189,7 @@ def filter_out_hidden(events: List[JsonDict], user_id: str) -> List[JsonDict]: new_event["content"][event_id] = {ReceiptTypes.READ: m_read} continue + # Filter the private read receipts to only the requesting user m_read_private = event_content.get(ReceiptTypes.READ_PRIVATE, None) if m_read_private: user_rr = m_read_private.get(user_id, None) From c574c86dea8c21ca974c7b748533228e81d350f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Thu, 10 Mar 2022 20:13:12 +0100 Subject: [PATCH 08/42] Add a comment regarding `parse_json_object_from_request()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/rest/client/receipts.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/rest/client/receipts.py b/synapse/rest/client/receipts.py index a53b584ec564..f9caab663523 100644 --- a/synapse/rest/client/receipts.py +++ b/synapse/rest/client/receipts.py @@ -75,6 +75,7 @@ async def on_POST( if "Android" in user_agent: if pattern.match(user_agent) or "Riot" in user_agent: allow_empty_body = True + # This call makes sure possible empty body is handled correctly parse_json_object_from_request(request, allow_empty_body) await self.presence_handler.bump_presence_active_time(requester.user) From 5f1a9633fbd18a7a4d019c84f37ee9d18566cf46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Tue, 15 Mar 2022 20:52:06 +0100 Subject: [PATCH 09/42] Make handling of multiple receipt types sound MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/handlers/receipts.py | 30 +++++++++--------- tests/handlers/test_receipts.py | 55 ++++++++++++++++++++++++++++++++- 2 files changed, 68 insertions(+), 17 deletions(-) diff --git a/synapse/handlers/receipts.py b/synapse/handlers/receipts.py index 6c75b7b3527d..a50509b9360a 100644 --- a/synapse/handlers/receipts.py +++ b/synapse/handlers/receipts.py @@ -184,22 +184,20 @@ def filter_out_hidden(events: List[JsonDict], user_id: str) -> List[JsonDict]: for event_id in content.keys(): event_content = content.get(event_id, {}) - m_read = event_content.get(ReceiptTypes.READ, None) - if m_read: - new_event["content"][event_id] = {ReceiptTypes.READ: m_read} - continue - - # Filter the private read receipts to only the requesting user - m_read_private = event_content.get(ReceiptTypes.READ_PRIVATE, None) - if m_read_private: - user_rr = m_read_private.get(user_id, None) - if user_rr: - new_event["content"][event_id] = { - ReceiptTypes.READ_PRIVATE: {user_id: user_rr.copy()} - } - continue - - new_event["content"][event_id] = event_content + receipt_event = {} + for receipt_type, receipt_content in event_content.items(): + if receipt_type == ReceiptTypes.READ_PRIVATE: + user_rr = receipt_content.get(user_id, None) + if user_rr: + receipt_event[ReceiptTypes.READ_PRIVATE] = { + user_id: user_rr.copy() + } + else: + receipt_event[receipt_type] = receipt_content.copy() + + # Append receipt_event to new_event unless empty + if len(receipt_event.keys()) > 0: + new_event["content"][event_id] = receipt_event.copy() # Append new_event to visible_events unless empty if len(new_event["content"].keys()) > 0: diff --git a/tests/handlers/test_receipts.py b/tests/handlers/test_receipts.py index f00275109388..dce61319ae11 100644 --- a/tests/handlers/test_receipts.py +++ b/tests/handlers/test_receipts.py @@ -216,7 +216,6 @@ def test_handles_empty_event(self): [ { "content": { - "$143564gdfg6114394fHBLK:matrix.org": {}, "$1435641916114394fHBLK:matrix.org": { ReceiptTypes.READ: { "@user:jki.re": { @@ -313,6 +312,60 @@ def test_handles_string_data(self): ], ) + def test_leaves_our_hidden_and_their_public(self): + self._test_filters_hidden( + [ + { + "content": { + "$1dgdgrd5641916114394fHBLK:matrix.org": { + ReceiptTypes.READ_PRIVATE: { + "@me:server.org": { + "ts": 1436451550453, + }, + }, + ReceiptTypes.READ: { + "@rikj:jki.re": { + "ts": 1436451550453, + }, + }, + "a.receipt.type": { + "@rikj:jki.re": { + "ts": 1436451550453, + }, + }, + }, + }, + "room_id": "!jEsUZKDJdhlrceRyVU:example.org", + "type": "m.receipt", + } + ], + [ + { + "content": { + "$1dgdgrd5641916114394fHBLK:matrix.org": { + ReceiptTypes.READ_PRIVATE: { + "@me:server.org": { + "ts": 1436451550453, + }, + }, + ReceiptTypes.READ: { + "@rikj:jki.re": { + "ts": 1436451550453, + }, + }, + "a.receipt.type": { + "@rikj:jki.re": { + "ts": 1436451550453, + }, + }, + } + }, + "room_id": "!jEsUZKDJdhlrceRyVU:example.org", + "type": "m.receipt", + } + ], + ) + def _test_filters_hidden( self, events: List[JsonDict], expected_output: List[JsonDict] ): From 533421c9ea26f6824a01082d86f232f67780d4c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Thu, 17 Mar 2022 17:17:41 +0100 Subject: [PATCH 10/42] Simplifie code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/handlers/receipts.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/synapse/handlers/receipts.py b/synapse/handlers/receipts.py index a50509b9360a..53889fd42065 100644 --- a/synapse/handlers/receipts.py +++ b/synapse/handlers/receipts.py @@ -181,9 +181,7 @@ def filter_out_hidden(events: List[JsonDict], user_id: str) -> List[JsonDict]: new_event = event.copy() new_event["content"] = {} - for event_id in content.keys(): - event_content = content.get(event_id, {}) - + for event_id, event_content in content.items(): receipt_event = {} for receipt_type, receipt_content in event_content.items(): if receipt_type == ReceiptTypes.READ_PRIVATE: From d060e127a5b09a55c2b54836c2ff4226dd55648d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Thu, 17 Mar 2022 17:21:40 +0100 Subject: [PATCH 11/42] Only handle `m.read.private` if MSC enabled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/rest/client/read_marker.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/rest/client/read_marker.py b/synapse/rest/client/read_marker.py index 47625d150f4e..be96387ba482 100644 --- a/synapse/rest/client/read_marker.py +++ b/synapse/rest/client/read_marker.py @@ -35,6 +35,7 @@ class ReadMarkerRestServlet(RestServlet): def __init__(self, hs: "HomeServer"): super().__init__() self.auth = hs.get_auth() + self.config = hs.config self.receipts_handler = hs.get_receipts_handler() self.read_marker_handler = hs.get_read_marker_handler() self.presence_handler = hs.get_presence_handler() @@ -58,7 +59,7 @@ async def on_POST( ) read_private_event_id = body.get(ReceiptTypes.READ_PRIVATE, None) - if read_private_event_id: + if read_private_event_id and self.config.experimental.msc2285_enabled: await self.receipts_handler.received_client_receipt( room_id, ReceiptTypes.READ_PRIVATE, From c73005f54157beab4031ea1dbe500d585144ac29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Thu, 17 Mar 2022 17:32:26 +0100 Subject: [PATCH 12/42] Use f-strings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- tests/rest/client/test_sync.py | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index be41c48c8f3e..cfc5abb82b34 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -342,7 +342,7 @@ def test_knock_room_state(self) -> None: # Knock on a room channel = self.make_request( "POST", - "/_matrix/client/r0/knock/%s" % (self.room_id,), + f"/_matrix/client/r0/knock/{self.room_id}", b"{}", self.knocker_tok, ) @@ -410,8 +410,7 @@ def test_hidden_read_receipts(self) -> None: # Send a read receipt to tell the server the first user's message was read channel = self.make_request( "POST", - "/rooms/%s/receipt/org.matrix.msc2285.read.private/%s" - % (self.room_id, res["event_id"]), + f"/rooms/{self.room_id}/receipt/org.matrix.msc2285.read.private/{res['event_id']}", {}, access_token=self.tok2, ) @@ -450,7 +449,7 @@ def test_read_receipt_with_empty_body( # Send a read receipt for this message with an empty body channel = self.make_request( "POST", - "/rooms/%s/receipt/m.read/%s" % (self.room_id, res["event_id"]), + f"/rooms/{self.room_id}/receipt/m.read/{res['event_id']}", access_token=self.tok2, custom_headers=[("User-Agent", user_agent)], ) @@ -555,7 +554,7 @@ def test_unread_counts(self) -> None: body = json.dumps({"m.read": res["event_id"]}).encode("utf8") channel = self.make_request( "POST", - "/rooms/%s/read_markers" % self.room_id, + f"/rooms/{self.room_id}/read_markers", body, access_token=self.tok, ) @@ -571,8 +570,7 @@ def test_unread_counts(self) -> None: # Send a read receipt to tell the server we've read the latest event. channel = self.make_request( "POST", - "/rooms/%s/receipt/org.matrix.msc2285.read.private/%s" - % (self.room_id, res["event_id"]), + f"/rooms/{self.room_id}/receipt/org.matrix.msc2285.read.private/{res['event_id']}", {}, access_token=self.tok, ) @@ -647,7 +645,7 @@ def test_unread_counts(self) -> None: # Make sure both m.read and org.matrix.msc2285.read.private advance channel = self.make_request( "POST", - "/rooms/%s/receipt/m.read/%s" % (self.room_id, res1["event_id"]), + f"/rooms/{self.room_id}/receipt/m.read/{res1['event_id']}", {}, access_token=self.tok, ) @@ -656,8 +654,7 @@ def test_unread_counts(self) -> None: channel = self.make_request( "POST", - "/rooms/%s/receipt/org.matrix.msc2285.read.private/%s" - % (self.room_id, res2["event_id"]), + f"/rooms/{self.room_id}/receipt/org.matrix.msc2285.read.private/{res2['event_id']}", {}, access_token=self.tok, ) @@ -676,7 +673,7 @@ def test_doesnt_reverse_read_receipts(self) -> None: # Read last event channel = self.make_request( "POST", - "/rooms/%s/receipt/m.read/%s" % (self.room_id, res2["event_id"]), + f"/rooms/{self.room_id}/receipt/m.read/{res2['event_id']}", {}, access_token=self.tok, ) @@ -686,8 +683,7 @@ def test_doesnt_reverse_read_receipts(self) -> None: # Make sure neither m.read not org.matrix.msc2285.read.private reverse channel = self.make_request( "POST", - "/rooms/%s/receipt/org.matrix.msc2285.read.private/%s" - % (self.room_id, res1["event_id"]), + f"/rooms/{self.room_id}/receipt/org.matrix.msc2285.read.private/{res1['event_id']}", {}, access_token=self.tok, ) @@ -696,7 +692,7 @@ def test_doesnt_reverse_read_receipts(self) -> None: channel = self.make_request( "POST", - "/rooms/%s/receipt/m.read/%s" % (self.room_id, res1["event_id"]), + f"/rooms/{self.room_id}/receipt/m.read/{res1['event_id']}", {}, access_token=self.tok, ) From 100120fce433847bc95f576f5ceba9e2710e6a37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Thu, 17 Mar 2022 17:35:14 +0100 Subject: [PATCH 13/42] Make things more readable - don't use "reverse" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- tests/rest/client/test_sync.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index cfc5abb82b34..2f052faf35d9 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -662,7 +662,7 @@ def test_unread_counts(self) -> None: self._check_unread_count(0) @override_config({"experimental_features": {"msc2285_enabled": True}}) - def test_doesnt_reverse_read_receipts(self) -> None: + def test_read_receipts_only_go_down(self) -> None: # Join the new user self.helper.join(room=self.room_id, user=self.user2, tok=self.tok2) @@ -680,7 +680,8 @@ def test_doesnt_reverse_read_receipts(self) -> None: self.assertEqual(channel.code, 200, channel.json_body) self._check_unread_count(0) - # Make sure neither m.read not org.matrix.msc2285.read.private reverse + # Make sure neither m.read nor org.matrix.msc2285.read.private make the + # read receipt go up to an older event channel = self.make_request( "POST", f"/rooms/{self.room_id}/receipt/org.matrix.msc2285.read.private/{res1['event_id']}", From 35279660b3073ae4cbefa1c55cf6b43ce51c3682 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Thu, 17 Mar 2022 17:40:27 +0100 Subject: [PATCH 14/42] Remove `test_does_not_filter_out_our_hidden_receipt()` as `test_leaves_our_hidden_and_their_public()` already does all the work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- tests/handlers/test_receipts.py | 34 --------------------------------- 1 file changed, 34 deletions(-) diff --git a/tests/handlers/test_receipts.py b/tests/handlers/test_receipts.py index dce61319ae11..c12a9120f029 100644 --- a/tests/handlers/test_receipts.py +++ b/tests/handlers/test_receipts.py @@ -45,40 +45,6 @@ def test_filters_out_hidden_receipt(self): [], ) - def test_does_not_filter_out_our_hidden_receipt(self): - self._test_filters_hidden( - [ - { - "content": { - "$1435641916hfgh4394fHBLK:matrix.org": { - ReceiptTypes.READ_PRIVATE: { - "@me:server.org": { - "ts": 1436451550453, - }, - } - } - }, - "room_id": "!jEsUZKDJdhlrceRyVU:example.org", - "type": "m.receipt", - } - ], - [ - { - "content": { - "$1435641916hfgh4394fHBLK:matrix.org": { - ReceiptTypes.READ_PRIVATE: { - "@me:server.org": { - "ts": 1436451550453, - }, - } - } - }, - "room_id": "!jEsUZKDJdhlrceRyVU:example.org", - "type": "m.receipt", - } - ], - ) - def test_filters_out_hidden_receipt_and_ignores_rest(self): self._test_filters_hidden( [ From 1d7e274d3ac3d0bd60a5311a0b06ee8ab85e5838 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Thu, 17 Mar 2022 19:18:16 +0100 Subject: [PATCH 15/42] Test that the read receipt doesn't go up for either receipt type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- tests/rest/client/test_sync.py | 75 +++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 34 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 2f052faf35d9..8efa0ebf7b60 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -20,7 +20,12 @@ from twisted.test.proto_helpers import MemoryReactor import synapse.rest.admin -from synapse.api.constants import EventContentFields, EventTypes, RelationTypes +from synapse.api.constants import ( + EventContentFields, + EventTypes, + ReceiptTypes, + RelationTypes, +) from synapse.rest.client import devices, knock, login, read_marker, receipts, room, sync from synapse.server import HomeServer from synapse.types import JsonDict @@ -666,39 +671,41 @@ def test_read_receipts_only_go_down(self) -> None: # Join the new user self.helper.join(room=self.room_id, user=self.user2, tok=self.tok2) - # Send messages - res1 = self.helper.send(self.room_id, "hello", tok=self.tok2) - res2 = self.helper.send(self.room_id, "hello", tok=self.tok2) - - # Read last event - channel = self.make_request( - "POST", - f"/rooms/{self.room_id}/receipt/m.read/{res2['event_id']}", - {}, - access_token=self.tok, - ) - self.assertEqual(channel.code, 200, channel.json_body) - self._check_unread_count(0) - - # Make sure neither m.read nor org.matrix.msc2285.read.private make the - # read receipt go up to an older event - channel = self.make_request( - "POST", - f"/rooms/{self.room_id}/receipt/org.matrix.msc2285.read.private/{res1['event_id']}", - {}, - access_token=self.tok, - ) - self.assertEqual(channel.code, 200, channel.json_body) - self._check_no_room_changes() - - channel = self.make_request( - "POST", - f"/rooms/{self.room_id}/receipt/m.read/{res1['event_id']}", - {}, - access_token=self.tok, - ) - self.assertEqual(channel.code, 200, channel.json_body) - self._check_no_room_changes() + # We tests this for both receipt types that influence notification counts + for receipt_type in [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE]: + # Send messages + res1 = self.helper.send(self.room_id, "hello", tok=self.tok2) + res2 = self.helper.send(self.room_id, "hello", tok=self.tok2) + + # Read last event + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/{receipt_type}/{res2['event_id']}", + {}, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self._check_unread_count(0) + + # Make sure neither m.read nor org.matrix.msc2285.read.private make the + # read receipt go up to an older event + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/org.matrix.msc2285.read.private/{res1['event_id']}", + {}, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self._check_no_room_changes() + + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/m.read/{res1['event_id']}", + {}, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self._check_no_room_changes() def _check_unread_count(self, expected_count: int) -> None: """Syncs and compares the unread count with the expected value.""" From 26aae3e77e4de028cc0791cf0691093722c553e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Thu, 17 Mar 2022 19:24:43 +0100 Subject: [PATCH 16/42] Test that we can't override private read receipts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- tests/rest/client/test_sync.py | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 8efa0ebf7b60..4585b3c68504 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -412,7 +412,7 @@ def test_hidden_read_receipts(self) -> None: # Send a message as the first user res = self.helper.send(self.room_id, body="hello", tok=self.tok) - # Send a read receipt to tell the server the first user's message was read + # Send a private read receipt to tell the server the first user's message was read channel = self.make_request( "POST", f"/rooms/{self.room_id}/receipt/org.matrix.msc2285.read.private/{res['event_id']}", @@ -421,7 +421,19 @@ def test_hidden_read_receipts(self) -> None: ) self.assertEqual(channel.code, 200) - # Test that the first user can't see the other user's hidden read receipt + # Test that the first user can't see the other user's private read receipt + self.assertEqual(self._get_read_receipt(), None) + + # Send a public read receipt to tell the server the first user's message was read + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/m.read/{res['event_id']}", + {}, + access_token=self.tok2, + ) + self.assertEqual(channel.code, 200) + + # Test that we didn't override the private read receipt self.assertEqual(self._get_read_receipt(), None) @parameterized.expand( @@ -478,6 +490,9 @@ def is_read_receipt(event: JsonDict) -> bool: # Store the next batch for the next request. self.next_batch = channel.json_body["next_batch"] + if channel.json_body.get("rooms", None) is None: + return None + # Return the read receipt ephemeral_events = channel.json_body["rooms"]["join"][self.room_id][ "ephemeral" From 245b72361c3ab4c2e801481d34300293909f4041 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Thu, 17 Mar 2022 19:25:51 +0100 Subject: [PATCH 17/42] Make sure read receipts don't go up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/storage/databases/main/receipts.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index 5f9ea4991807..5943c0adb7dc 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -547,6 +547,10 @@ def insert_linearized_receipt_txn( rt == ReceiptTypes.READ and receipt_type == ReceiptTypes.READ_PRIVATE ) + or ( + rt == ReceiptTypes.READ_PRIVATE + and receipt_type == ReceiptTypes.READ + ) ): logger.debug( "Ignoring new receipt for %s in favour of existing " From 2044845ffb97ad51854c5ce7754627280d85197d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Fri, 1 Apr 2022 18:07:28 +0200 Subject: [PATCH 18/42] Fix tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- tests/rest/client/test_sync.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 6f9d52f3dc7d..8414a710f618 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -512,7 +512,10 @@ class UnreadMessagesTestCase(unittest.HomeserverTestCase): def default_config(self) -> JsonDict: config = super().default_config() - config["experimental_features"] = {"msc2654_enabled": True} + config["experimental_features"] = { + "msc2654_enabled": True, + "msc2285_enabled": True, + } return config def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: @@ -558,7 +561,6 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: tok=self.tok, ) - @override_config({"experimental_features": {"msc2285_enabled": True}}) def test_unread_counts(self) -> None: """Tests that /sync returns the right value for the unread count (MSC2654).""" @@ -685,7 +687,6 @@ def test_unread_counts(self) -> None: self.assertEqual(channel.code, 200, channel.json_body) self._check_unread_count(0) - @override_config({"experimental_features": {"msc2285_enabled": True}}) def test_read_receipts_only_go_down(self) -> None: # Join the new user self.helper.join(room=self.room_id, user=self.user2, tok=self.tok2) From 21754eadef37170dbe96f9ac2ab2b9ae02e4a4f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Fri, 1 Apr 2022 18:08:10 +0200 Subject: [PATCH 19/42] Simplify code and improve comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/handlers/receipts.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/receipts.py b/synapse/handlers/receipts.py index 53889fd42065..4b91afa806d4 100644 --- a/synapse/handlers/receipts.py +++ b/synapse/handlers/receipts.py @@ -193,9 +193,9 @@ def filter_out_hidden(events: List[JsonDict], user_id: str) -> List[JsonDict]: else: receipt_event[receipt_type] = receipt_content.copy() - # Append receipt_event to new_event unless empty - if len(receipt_event.keys()) > 0: - new_event["content"][event_id] = receipt_event.copy() + # Only include the receipt event if it is non-empty. + if receipt_event: + new_event["content"][event_id] = receipt_event # Append new_event to visible_events unless empty if len(new_event["content"].keys()) > 0: From 7aca89c8ad3be3d31dadfd89f8454decc5a3b8f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Fri, 1 Apr 2022 18:23:55 +0200 Subject: [PATCH 20/42] Use `parameterized` instead of a for loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- tests/rest/client/test_sync.py | 72 +++++++++++++++++----------------- 1 file changed, 36 insertions(+), 36 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 8414a710f618..cf165e13fc02 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -687,45 +687,45 @@ def test_unread_counts(self) -> None: self.assertEqual(channel.code, 200, channel.json_body) self._check_unread_count(0) - def test_read_receipts_only_go_down(self) -> None: + # We test for both receipt types that influence notification counts + @parameterized.expand([ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE]) + def test_read_receipts_only_go_down(self, receipt_type) -> None: # Join the new user self.helper.join(room=self.room_id, user=self.user2, tok=self.tok2) - # We tests this for both receipt types that influence notification counts - for receipt_type in [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE]: - # Send messages - res1 = self.helper.send(self.room_id, "hello", tok=self.tok2) - res2 = self.helper.send(self.room_id, "hello", tok=self.tok2) - - # Read last event - channel = self.make_request( - "POST", - f"/rooms/{self.room_id}/receipt/{receipt_type}/{res2['event_id']}", - {}, - access_token=self.tok, - ) - self.assertEqual(channel.code, 200, channel.json_body) - self._check_unread_count(0) - - # Make sure neither m.read nor org.matrix.msc2285.read.private make the - # read receipt go up to an older event - channel = self.make_request( - "POST", - f"/rooms/{self.room_id}/receipt/org.matrix.msc2285.read.private/{res1['event_id']}", - {}, - access_token=self.tok, - ) - self.assertEqual(channel.code, 200, channel.json_body) - self._check_no_room_changes() - - channel = self.make_request( - "POST", - f"/rooms/{self.room_id}/receipt/m.read/{res1['event_id']}", - {}, - access_token=self.tok, - ) - self.assertEqual(channel.code, 200, channel.json_body) - self._check_no_room_changes() + # Send messages + res1 = self.helper.send(self.room_id, "hello", tok=self.tok2) + res2 = self.helper.send(self.room_id, "hello", tok=self.tok2) + + # Read last event + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/{receipt_type}/{res2['event_id']}", + {}, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self._check_unread_count(0) + + # Make sure neither m.read nor org.matrix.msc2285.read.private make the + # read receipt go up to an older event + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/org.matrix.msc2285.read.private/{res1['event_id']}", + {}, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self._check_no_room_changes() + + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/m.read/{res1['event_id']}", + {}, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self._check_no_room_changes() def _check_unread_count(self, expected_count: int) -> None: """Syncs and compares the unread count with the expected value.""" From 88f47b611c1aaa33657ce36a676bebd87aca0feb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Fri, 1 Apr 2022 18:44:59 +0200 Subject: [PATCH 21/42] Delint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- tests/rest/client/test_sync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index cf165e13fc02..74ee84b554e1 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -689,7 +689,7 @@ def test_unread_counts(self) -> None: # We test for both receipt types that influence notification counts @parameterized.expand([ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE]) - def test_read_receipts_only_go_down(self, receipt_type) -> None: + def test_read_receipts_only_go_down(self, receipt_type: ReceiptTypes) -> None: # Join the new user self.helper.join(room=self.room_id, user=self.user2, tok=self.tok2) From ac3c0ea15497a14e186e0023abd2bf41ac45eb81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Fri, 1 Apr 2022 18:50:13 +0200 Subject: [PATCH 22/42] Throw when receipt type is not known MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/rest/client/read_marker.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/synapse/rest/client/read_marker.py b/synapse/rest/client/read_marker.py index be96387ba482..4da4aadeab8c 100644 --- a/synapse/rest/client/read_marker.py +++ b/synapse/rest/client/read_marker.py @@ -16,6 +16,7 @@ from typing import TYPE_CHECKING, Tuple from synapse.api.constants import ReceiptTypes +from synapse.api.errors import SynapseError from synapse.http.server import HttpServer from synapse.http.servlet import RestServlet, parse_json_object_from_request from synapse.http.site import SynapseRequest @@ -75,6 +76,24 @@ async def on_POST( event_id=read_marker_event_id, ) + for key in body.keys(): + if self.config.experimental.msc2285_enabled and key not in [ + ReceiptTypes.READ, + ReceiptTypes.READ_PRIVATE, + ReceiptTypes.FULLY_READ, + ]: + raise SynapseError( + 400, + "Receipt type must be 'm.read', 'org.matrix.msc2285.read.private' or 'm.fully_read'", + ) + elif not self.config.experimental.msc2285_enabled and key not in [ + ReceiptTypes.READ, + ReceiptTypes.FULLY_READ, + ]: + raise SynapseError( + 400, "Receipt type must be 'm.read' or 'm.fully_read'" + ) + return 200, {} From 85aa704f61e50305a3581444e7846f71c3e44944 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Sat, 2 Apr 2022 09:25:32 +0200 Subject: [PATCH 23/42] Replace `_check_no_room_changes()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- tests/rest/client/test_sync.py | 26 ++++---------------------- 1 file changed, 4 insertions(+), 22 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 74ee84b554e1..f0e6768d082a 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -716,7 +716,7 @@ def test_read_receipts_only_go_down(self, receipt_type: ReceiptTypes) -> None: access_token=self.tok, ) self.assertEqual(channel.code, 200, channel.json_body) - self._check_no_room_changes() + self._check_unread_count(0) channel = self.make_request( "POST", @@ -725,7 +725,7 @@ def test_read_receipts_only_go_down(self, receipt_type: ReceiptTypes) -> None: access_token=self.tok, ) self.assertEqual(channel.code, 200, channel.json_body) - self._check_no_room_changes() + self._check_unread_count(0) def _check_unread_count(self, expected_count: int) -> None: """Syncs and compares the unread count with the expected value.""" @@ -738,9 +738,9 @@ def _check_unread_count(self, expected_count: int) -> None: self.assertEqual(channel.code, 200, channel.json_body) - room_entry = channel.json_body["rooms"]["join"][self.room_id] + room_entry = channel.json_body.get("rooms", {}).get("join", {}).get(self.room_id, {}) self.assertEqual( - room_entry["org.matrix.msc2654.unread_count"], + room_entry.get("org.matrix.msc2654.unread_count", 0), expected_count, room_entry, ) @@ -748,24 +748,6 @@ def _check_unread_count(self, expected_count: int) -> None: # Store the next batch for the next request. self.next_batch = channel.json_body["next_batch"] - def _check_no_room_changes(self) -> None: - """Syncs and makes sure the rooms part of sync is empty.""" - - channel = self.make_request( - "GET", - self.url % self.next_batch, - access_token=self.tok, - ) - self.assertEqual(channel.code, 200, channel.json_body) - self.assertEqual( - channel.json_body.get("rooms", None), - None, - channel.json_body, - ) - - # Store the next batch for the next request. - self.next_batch = channel.json_body["next_batch"] - class SyncCacheTestCase(unittest.HomeserverTestCase): servlets = [ From f83798193132cefa683fc76ddf48ac72fa1cad57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Sat, 2 Apr 2022 09:39:55 +0200 Subject: [PATCH 24/42] Test receipt overriding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- tests/rest/client/test_sync.py | 52 +++++++++++++++++++++++++++++++--- 1 file changed, 48 insertions(+), 4 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index f0e6768d082a..43539f777730 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -423,16 +423,58 @@ def test_hidden_read_receipts(self) -> None: # Test that the first user can't see the other user's private read receipt self.assertEqual(self._get_read_receipt(), None) - # Send a public read receipt to tell the server the first user's message was read + @override_config({"experimental_features": {"msc2285_enabled": True}}) + def test_public_receipt_cannot_override_private(self) -> None: + # Send a message as the first user + res = self.helper.send(self.room_id, body="hello", tok=self.tok) + + # Send a private read receipt channel = self.make_request( "POST", - f"/rooms/{self.room_id}/receipt/m.read/{res['event_id']}", + f"/rooms/{self.room_id}/receipt/{ReceiptTypes.READ_PRIVATE}/{res['event_id']}", {}, access_token=self.tok2, ) self.assertEqual(channel.code, 200) + self.assertEqual(self._get_read_receipt(), None) - # Test that we didn't override the private read receipt + # Send a public read receipt + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/{ReceiptTypes.READ}/{res['event_id']}", + {}, + access_token=self.tok2, + ) + self.assertEqual(channel.code, 200) + + # Test that we did override the private read receipt + self.assertNotEqual(self._get_read_receipt(), None) + + @override_config({"experimental_features": {"msc2285_enabled": True}}) + def test_private_receipt_cannot_override_public(self) -> None: + # Send a message as the first user + res = self.helper.send(self.room_id, body="hello", tok=self.tok) + + # Send a public read receipt + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/{ReceiptTypes.READ}/{res['event_id']}", + {}, + access_token=self.tok2, + ) + self.assertEqual(channel.code, 200) + self.assertNotEqual(self._get_read_receipt(), None) + + # Send a private read receipt + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/{ReceiptTypes.READ_PRIVATE}/{res['event_id']}", + {}, + access_token=self.tok2, + ) + self.assertEqual(channel.code, 200) + + # Test that we didn't override the public read receipt self.assertEqual(self._get_read_receipt(), None) @parameterized.expand( @@ -738,7 +780,9 @@ def _check_unread_count(self, expected_count: int) -> None: self.assertEqual(channel.code, 200, channel.json_body) - room_entry = channel.json_body.get("rooms", {}).get("join", {}).get(self.room_id, {}) + room_entry = ( + channel.json_body.get("rooms", {}).get("join", {}).get(self.room_id, {}) + ) self.assertEqual( room_entry.get("org.matrix.msc2654.unread_count", 0), expected_count, From 21ac95e5abe8e260504a56e74d0e4fe28b129b84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Fri, 8 Apr 2022 21:01:15 +0200 Subject: [PATCH 25/42] Support specifying the receipt types we want MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/handlers/sync.py | 3 +- synapse/push/push_tools.py | 5 +- synapse/storage/databases/main/receipts.py | 74 ++++++++++++++-------- 3 files changed, 52 insertions(+), 30 deletions(-) diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index ced057ba82bb..343898f6a42c 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -18,7 +18,7 @@ import attr from prometheus_client import Counter -from synapse.api.constants import EventTypes, Membership +from synapse.api.constants import EventTypes, Membership, ReceiptTypes from synapse.api.filtering import FilterCollection from synapse.api.presence import UserPresenceState from synapse.api.room_versions import KNOWN_ROOM_VERSIONS @@ -1046,6 +1046,7 @@ async def unread_notifs_for_room_id( last_unread_event_id = await self.store.get_last_receipt_event_id_for_user( user_id=sync_config.user.to_string(), room_id=room_id, + receipt_types=[ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE], ) return await self.store.get_unread_event_push_actions_by_room_for_user( diff --git a/synapse/push/push_tools.py b/synapse/push/push_tools.py index b10d9b03ef17..0137b0ad6e15 100644 --- a/synapse/push/push_tools.py +++ b/synapse/push/push_tools.py @@ -13,6 +13,7 @@ # limitations under the License. from typing import Dict +from synapse.api.constants import ReceiptTypes from synapse.events import EventBase from synapse.push.presentable_names import calculate_room_name, name_from_member_event from synapse.storage import Storage @@ -23,7 +24,9 @@ async def get_badge_count(store: DataStore, user_id: str, group_by_room: bool) - invites = await store.get_invited_rooms_for_local_user(user_id) joins = await store.get_rooms_for_user(user_id) - my_receipts_by_room = await store.get_receipts_for_user(user_id) + my_receipts_by_room = await store.get_latest_receipts_for_user( + user_id, [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE] + ) badge = len(invites) diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index b980fbd729f0..cbd5648c08ad 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -122,31 +122,53 @@ async def get_receipts_for_room( desc="get_receipts_for_room", ) - @cached(num_args=2) async def get_last_receipt_event_id_for_user( - self, user_id: str, room_id: str + self, user_id: str, room_id: str, receipt_types: List[str] ) -> Optional[str]: - return await self.db_pool.simple_select_one_onecol( - table="receipts_linearized", - keyvalues={ - "room_id": room_id, - "user_id": user_id, - }, - retcol="event_id", - desc="get_own_receipt_for_user", - allow_none=True, - ) + def f(txn: LoggingTransaction) -> List[Tuple[str, str]]: + clause, args = make_in_list_sql_clause( + self.database_engine, "rl.receipt_type", receipt_types + ) + sql = """ + SELECT rl.event_id, MAX(e.stream_ordering) + FROM receipts_linearized AS rl + INNER JOIN events AS e USING (room_id, event_id) + WHERE rl.user_id = ? + AND rl.room_id = ? + AND %s + """ % ( + clause, + ) - @cached(num_args=1) - async def get_receipts_for_user(self, user_id: str) -> Dict[str, str]: - rows = await self.db_pool.simple_select_list( - table="receipts_linearized", - keyvalues={"user_id": user_id}, - retcols=("room_id", "event_id"), - desc="get_receipts_for_user", - ) + txn.execute(sql, [user_id, room_id] + list(args)) + return cast(List[Tuple[str, str]], txn.fetchall()) + + rows = await self.db_pool.runInteraction("get_own_receipt_for_user", f) + return rows[0][0] + + async def get_latest_receipts_for_user( + self, user_id: str, receipt_types: List[str] + ) -> Dict[str, str]: + def f(txn: LoggingTransaction) -> List[Tuple[str, str]]: + clause, args = make_in_list_sql_clause( + self.database_engine, "rl.receipt_type", receipt_types + ) + sql = """ + SELECT rl.room_id, rl.event_id, MAX(e.stream_ordering) + FROM receipts_linearized AS rl + INNER JOIN events AS e USING (room_id, event_id) + WHERE rl.user_id = ? + AND %s + GROUP BY rl.room_id + """ % ( + clause, + ) + + txn.execute(sql, [user_id] + list(args)) + return cast(List[Tuple[str, str]], txn.fetchall()) - return {row["room_id"]: row["event_id"] for row in rows} + rows = await self.db_pool.runInteraction("get_latest_receipts_for_user", f) + return {row[0]: row[1] for row in rows} async def get_receipts_for_user_with_orderings(self, user_id: str) -> JsonDict: def f(txn: LoggingTransaction) -> List[Tuple[str, str, int, int]]: @@ -490,9 +512,7 @@ def _invalidate_get_users_with_receipts_in_room( def invalidate_caches_for_receipt( self, room_id: str, receipt_type: str, user_id: str ) -> None: - self.get_receipts_for_user.invalidate((user_id, receipt_type)) self._get_linearized_receipts_for_room.invalidate((room_id,)) - self.get_last_receipt_event_id_for_user.invalidate((user_id, room_id)) self._invalidate_get_users_with_receipts_in_room(room_id, receipt_type, user_id) self.get_receipts_for_room.invalidate((room_id, receipt_type)) @@ -556,14 +576,13 @@ def insert_linearized_receipt_txn( for so, eid, rt in txn: if int(so) >= stream_ordering and ( receipt_type == rt + # We don't allow private read receipts to override public + # ones since that would look as if the read receipt is going + # up in the timeline from the perspective of the other users or ( rt == ReceiptTypes.READ and receipt_type == ReceiptTypes.READ_PRIVATE ) - or ( - rt == ReceiptTypes.READ_PRIVATE - and receipt_type == ReceiptTypes.READ - ) ): logger.debug( "Ignoring new receipt for %s in favour of existing " @@ -723,7 +742,6 @@ def insert_graph_receipt_txn( receipt_type, user_id, ) - txn.call_after(self.get_receipts_for_user.invalidate, (user_id, receipt_type)) # FIXME: This shouldn't invalidate the whole cache txn.call_after(self._get_linearized_receipts_for_room.invalidate, (room_id,)) From 373f0b716a9c86256270ee1155cdeb722841360e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Fri, 8 Apr 2022 21:04:30 +0200 Subject: [PATCH 26/42] Improve `SlavedReceiptTestCase` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- .../slave/storage/test_receipts.py | 237 +++++++++++++++++- 1 file changed, 229 insertions(+), 8 deletions(-) diff --git a/tests/replication/slave/storage/test_receipts.py b/tests/replication/slave/storage/test_receipts.py index f03bc9882eaa..a0667d63d23f 100644 --- a/tests/replication/slave/storage/test_receipts.py +++ b/tests/replication/slave/storage/test_receipts.py @@ -12,23 +12,244 @@ # See the License for the specific language governing permissions and # limitations under the License. +from synapse.api.constants import ReceiptTypes from synapse.replication.slave.storage.receipts import SlavedReceiptsStore +from synapse.types import UserID, create_requester + +from tests.test_utils.event_injection import create_event from ._base import BaseSlavedStoreTestCase -USER_ID = "@feeling:blue" -ROOM_ID = "!room:blue" -EVENT_ID = "$event:blue" +OTHER_USER_ID = "@other:test" +OUR_USER_ID = "@our:test" class SlavedReceiptTestCase(BaseSlavedStoreTestCase): STORE_TYPE = SlavedReceiptsStore - def test_receipt(self): - self.check("get_receipts_for_user", [USER_ID], {}) + def prepare(self, reactor, clock, homeserver): + super().prepare(reactor, clock, homeserver) + self.room_creator = homeserver.get_room_creation_handler() + self.persist_event_storage = self.hs.get_storage().persistence + + # Create a test user + self.ourUser = UserID.from_string(OUR_USER_ID) + self.ourRequester = create_requester(self.ourUser) + + # Create a second test user + self.otherUser = UserID.from_string(OTHER_USER_ID) + self.otherRequester = create_requester(self.otherUser) + + # Create a test room + info, _ = self.get_success(self.room_creator.create_room(self.ourRequester, {})) + self.room_id1 = info["room_id"] + + # Create a second test room + info, _ = self.get_success(self.room_creator.create_room(self.ourRequester, {})) + self.room_id2 = info["room_id"] + + # Join the second user to the first room + memberEvent, memberEventContext = self.get_success( + create_event( + self.hs, + room_id=self.room_id1, + type="m.room.member", + sender=self.otherRequester.user.to_string(), + state_key=self.otherRequester.user.to_string(), + content={"membership": "join"}, + ) + ) + self.get_success( + self.persist_event_storage.persist_event(memberEvent, memberEventContext) + ) + + # Join the second user to the second room + memberEvent, memberEventContext = self.get_success( + create_event( + self.hs, + room_id=self.room_id2, + type="m.room.member", + sender=self.otherRequester.user.to_string(), + state_key=self.otherRequester.user.to_string(), + content={"membership": "join"}, + ) + ) + self.get_success( + self.persist_event_storage.persist_event(memberEvent, memberEventContext) + ) + + def test_return_empty_with_no_data(self): + res = self.get_success( + self.master_store.get_latest_receipts_for_user( + OUR_USER_ID, [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE] + ) + ) + self.assertEqual(res, {}) + + res = self.get_success( + self.master_store.get_last_receipt_event_id_for_user( + OUR_USER_ID, + self.room_id1, + [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE], + ) + ) + self.assertEqual(res, None) + + def test_get_latest_receipts_for_user(self): + # Send some events into the first room + event1_1_id = self.create_and_send_event( + self.room_id1, UserID.from_string(OTHER_USER_ID) + ) + event1_2_id = self.create_and_send_event( + self.room_id1, UserID.from_string(OTHER_USER_ID) + ) + + # Send public read receipt for the first event + self.get_success( + self.master_store.insert_receipt( + self.room_id1, ReceiptTypes.READ, OUR_USER_ID, [event1_1_id], {} + ) + ) + # Send private read receipt for the second event + self.get_success( + self.master_store.insert_receipt( + self.room_id1, ReceiptTypes.READ_PRIVATE, OUR_USER_ID, [event1_2_id], {} + ) + ) + + # Test we get the latest event when we want both private and public receipts + res = self.get_success( + self.master_store.get_latest_receipts_for_user( + OUR_USER_ID, [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE] + ) + ) + self.assertEqual(res, {self.room_id1: event1_2_id}) + + # Test we get the older event when we want only public receipt + res = self.get_success( + self.master_store.get_latest_receipts_for_user( + OUR_USER_ID, [ReceiptTypes.READ] + ) + ) + self.assertEqual(res, {self.room_id1: event1_1_id}) + + # Test we get the latest event when we want only the public receipt + res = self.get_success( + self.master_store.get_latest_receipts_for_user( + OUR_USER_ID, [ReceiptTypes.READ_PRIVATE] + ) + ) + self.assertEqual(res, {self.room_id1: event1_2_id}) + + # Test receipt updating self.get_success( - self.master_store.insert_receipt(ROOM_ID, "m.read", USER_ID, [EVENT_ID], {}) + self.master_store.insert_receipt( + self.room_id1, ReceiptTypes.READ, OUR_USER_ID, [event1_2_id], {} + ) + ) + res = self.get_success( + self.master_store.get_latest_receipts_for_user( + OUR_USER_ID, [ReceiptTypes.READ] + ) + ) + self.assertEqual(res, {self.room_id1: event1_2_id}) + + # Send some events into the second room + event2_1_id = self.create_and_send_event( + self.room_id2, UserID.from_string(OTHER_USER_ID) + ) + + # Test new room is reflected in what the method returns + self.get_success( + self.master_store.insert_receipt( + self.room_id2, ReceiptTypes.READ_PRIVATE, OUR_USER_ID, [event2_1_id], {} + ) + ) + res = self.get_success( + self.master_store.get_latest_receipts_for_user( + OUR_USER_ID, [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE] + ) + ) + self.assertEqual(res, {self.room_id1: event1_2_id, self.room_id2: event2_1_id}) + + def test_get_last_receipt_event_id_for_user(self): + # Send some events into the first room + event1_1_id = self.create_and_send_event( + self.room_id1, UserID.from_string(OTHER_USER_ID) + ) + event1_2_id = self.create_and_send_event( + self.room_id1, UserID.from_string(OTHER_USER_ID) + ) + + # Send public read receipt for the first event + self.get_success( + self.master_store.insert_receipt( + self.room_id1, ReceiptTypes.READ, OUR_USER_ID, [event1_1_id], {} + ) + ) + # Send private read receipt for the second event + self.get_success( + self.master_store.insert_receipt( + self.room_id1, ReceiptTypes.READ_PRIVATE, OUR_USER_ID, [event1_2_id], {} + ) + ) + + # Test we get the latest event when we want both private and public receipts + res = self.get_success( + self.master_store.get_last_receipt_event_id_for_user( + OUR_USER_ID, + self.room_id1, + [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE], + ) + ) + self.assertEqual(res, event1_2_id) + + # Test we get the older event when we want only public receipt + res = self.get_success( + self.master_store.get_last_receipt_event_id_for_user( + OUR_USER_ID, self.room_id1, [ReceiptTypes.READ] + ) + ) + self.assertEqual(res, event1_1_id) + + # Test we get the latest event when we want only the private receipt + res = self.get_success( + self.master_store.get_last_receipt_event_id_for_user( + OUR_USER_ID, self.room_id1, [ReceiptTypes.READ_PRIVATE] + ) + ) + self.assertEqual(res, event1_2_id) + + # Test receipt updating + self.get_success( + self.master_store.insert_receipt( + self.room_id1, ReceiptTypes.READ, OUR_USER_ID, [event1_2_id], {} + ) + ) + res = self.get_success( + self.master_store.get_last_receipt_event_id_for_user( + OUR_USER_ID, self.room_id1, [ReceiptTypes.READ] + ) + ) + self.assertEqual(res, event1_2_id) + + # Send some events into the second room + event2_1_id = self.create_and_send_event( + self.room_id2, UserID.from_string(OTHER_USER_ID) + ) + + # Test new room is reflected in what the method returns + self.get_success( + self.master_store.insert_receipt( + self.room_id2, ReceiptTypes.READ_PRIVATE, OUR_USER_ID, [event2_1_id], {} + ) + ) + res = self.get_success( + self.master_store.get_last_receipt_event_id_for_user( + OUR_USER_ID, + self.room_id2, + [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE], + ) ) - self.replicate() - self.check("get_receipts_for_user", [USER_ID], {ROOM_ID: EVENT_ID}) + self.assertEqual(res, event2_1_id) From 455d500bfcecab6c57eb8afa2cda4dbbf43b77ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Fri, 8 Apr 2022 22:25:23 +0200 Subject: [PATCH 27/42] Use `GROUP BY` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/storage/databases/main/receipts.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index 124da20445b1..ebfec79dbe45 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -147,6 +147,7 @@ def f(txn: LoggingTransaction) -> List[Tuple[str, str]]: WHERE rl.user_id = ? AND rl.room_id = ? AND %s + GROUP BY rl.event_id """ % ( clause, ) @@ -155,6 +156,9 @@ def f(txn: LoggingTransaction) -> List[Tuple[str, str]]: return cast(List[Tuple[str, str]], txn.fetchall()) rows = await self.db_pool.runInteraction("get_own_receipt_for_user", f) + + if len(rows) == 0: + return None return rows[0][0] async def get_latest_receipts_for_user( From 809596a54b31a51e3aed711f68a6cb0a0ca8e528 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Sat, 9 Apr 2022 12:59:16 +0200 Subject: [PATCH 28/42] Use `ORDER BY` and `LIMIT` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/storage/databases/main/receipts.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index ebfec79dbe45..ac8e944f104a 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -141,13 +141,14 @@ def f(txn: LoggingTransaction) -> List[Tuple[str, str]]: self.database_engine, "rl.receipt_type", receipt_types ) sql = """ - SELECT rl.event_id, MAX(e.stream_ordering) + SELECT rl.event_id, e.stream_ordering FROM receipts_linearized AS rl INNER JOIN events AS e USING (room_id, event_id) WHERE rl.user_id = ? AND rl.room_id = ? AND %s - GROUP BY rl.event_id + ORDER BY e.stream_ordering DESC + LIMIT 1 """ % ( clause, ) From 2e198fe31edd8b552570597d21b3a2aa8fe8cde4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Sat, 9 Apr 2022 15:39:06 +0200 Subject: [PATCH 29/42] Fix `GROUP BY` error? MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/storage/databases/main/receipts.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index ac8e944f104a..76785138878f 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -170,12 +170,15 @@ def f(txn: LoggingTransaction) -> List[Tuple[str, str]]: self.database_engine, "rl.receipt_type", receipt_types ) sql = """ - SELECT rl.room_id, rl.event_id, MAX(e.stream_ordering) - FROM receipts_linearized AS rl - INNER JOIN events AS e USING (room_id, event_id) - WHERE rl.user_id = ? - AND %s - GROUP BY rl.room_id + SELECT t.room_id, t.event_id FROM ( + SELECT rl.room_id, rl.event_id, row_number() over (partition by rl.room_id order by e.stream_ordering desc) rw + FROM receipts_linearized AS rl + INNER JOIN events AS e USING (room_id, event_id) + WHERE rl.user_id = ? + AND %s + ) t + WHERE t.rw = 1; + """ % ( clause, ) From 116d1c92a2c4d3f253258da5da35247635673303 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Thu, 14 Apr 2022 08:31:38 +0200 Subject: [PATCH 30/42] Make `get_receipts_for_user_with_orderings()` handle multiple receipt types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/rest/client/notifications.py | 3 +- synapse/storage/databases/main/receipts.py | 35 ++++++++++++------- .../slave/storage/test_receipts.py | 8 +++++ 3 files changed, 33 insertions(+), 13 deletions(-) diff --git a/synapse/rest/client/notifications.py b/synapse/rest/client/notifications.py index 8ee92d9d47b7..24bc7c90957f 100644 --- a/synapse/rest/client/notifications.py +++ b/synapse/rest/client/notifications.py @@ -15,6 +15,7 @@ import logging from typing import TYPE_CHECKING, Tuple +from synapse.api.constants import ReceiptTypes from synapse.events.utils import ( SerializeEventConfig, format_event_for_client_v2_without_room_id, @@ -57,7 +58,7 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: ) receipts_by_room = await self.store.get_receipts_for_user_with_orderings( - user_id + user_id, [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE] ) notif_event_ids = [pa.event_id for pa in push_actions] diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index 76785138878f..7075c87981ab 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -189,18 +189,28 @@ def f(txn: LoggingTransaction) -> List[Tuple[str, str]]: rows = await self.db_pool.runInteraction("get_latest_receipts_for_user", f) return {row[0]: row[1] for row in rows} - async def get_receipts_for_user_with_orderings(self, user_id: str) -> JsonDict: + async def get_receipts_for_user_with_orderings( + self, user_id: str, receipt_types: List[str] + ) -> JsonDict: def f(txn: LoggingTransaction) -> List[Tuple[str, str, int, int]]: - sql = ( - "SELECT rl.room_id, rl.event_id," - " e.topological_ordering, e.stream_ordering" - " FROM receipts_linearized AS rl" - " INNER JOIN events AS e USING (room_id, event_id)" - " WHERE rl.room_id = e.room_id" - " AND rl.event_id = e.event_id" - " AND user_id = ?" + clause, args = make_in_list_sql_clause( + self.database_engine, "rl.receipt_type", receipt_types + ) + + sql = """ + SELECT rl.room_id, rl.event_id, rl.receipt_type, + e.topological_ordering, e.stream_ordering + FROM receipts_linearized AS rl + INNER JOIN events AS e USING (room_id, event_id) + WHERE rl.room_id = e.room_id + AND rl.event_id = e.event_id + AND user_id = ? + AND %s + """ % ( + clause, ) - txn.execute(sql, (user_id,)) + + txn.execute(sql, [user_id] + list(args)) return cast(List[Tuple[str, str, int, int]], txn.fetchall()) rows = await self.db_pool.runInteraction( @@ -209,8 +219,9 @@ def f(txn: LoggingTransaction) -> List[Tuple[str, str, int, int]]: return { row[0]: { "event_id": row[1], - "topological_ordering": row[2], - "stream_ordering": row[3], + "receipt_type": row[2], + "topological_ordering": row[3], + "stream_ordering": row[4], } for row in rows } diff --git a/tests/replication/slave/storage/test_receipts.py b/tests/replication/slave/storage/test_receipts.py index a0667d63d23f..dc37226fbdd9 100644 --- a/tests/replication/slave/storage/test_receipts.py +++ b/tests/replication/slave/storage/test_receipts.py @@ -87,6 +87,14 @@ def test_return_empty_with_no_data(self): ) self.assertEqual(res, {}) + res = self.get_success( + self.master_store.get_receipts_for_user_with_orderings( + OUR_USER_ID, + [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE], + ) + ) + self.assertEqual(res, {}) + res = self.get_success( self.master_store.get_last_receipt_event_id_for_user( OUR_USER_ID, From 7c221080b4ffae2d897a238e20838f45a442a56c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Thu, 14 Apr 2022 08:42:48 +0200 Subject: [PATCH 31/42] Be smarter when handling non-allowed keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/rest/client/read_marker.py | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/synapse/rest/client/read_marker.py b/synapse/rest/client/read_marker.py index 4da4aadeab8c..a6da6c32b476 100644 --- a/synapse/rest/client/read_marker.py +++ b/synapse/rest/client/read_marker.py @@ -50,6 +50,18 @@ async def on_POST( body = parse_json_object_from_request(request) + valid_receipt_types = {ReceiptTypes.READ, ReceiptTypes.FULLY_READ} + if self.config.experimental.msc2285_enabled: + valid_receipt_types.add(ReceiptTypes.READ_PRIVATE) + + if set(body.keys()) > (valid_receipt_types): + raise SynapseError( + 400, + "Receipt type must be 'm.read', 'org.matrix.msc2285.read.private' or 'm.fully_read'" + if self.config.experimental.msc2285_enabled + else "Receipt type must be 'm.read' or 'm.fully_read'", + ) + read_event_id = body.get(ReceiptTypes.READ, None) if read_event_id: await self.receipts_handler.received_client_receipt( @@ -76,24 +88,6 @@ async def on_POST( event_id=read_marker_event_id, ) - for key in body.keys(): - if self.config.experimental.msc2285_enabled and key not in [ - ReceiptTypes.READ, - ReceiptTypes.READ_PRIVATE, - ReceiptTypes.FULLY_READ, - ]: - raise SynapseError( - 400, - "Receipt type must be 'm.read', 'org.matrix.msc2285.read.private' or 'm.fully_read'", - ) - elif not self.config.experimental.msc2285_enabled and key not in [ - ReceiptTypes.READ, - ReceiptTypes.FULLY_READ, - ]: - raise SynapseError( - 400, "Receipt type must be 'm.read' or 'm.fully_read'" - ) - return 200, {} From 0f117e4ab4cb00bb69092c50005c20ca65ef8e9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Thu, 14 Apr 2022 08:48:04 +0200 Subject: [PATCH 32/42] Fix types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/storage/databases/main/receipts.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index 7075c87981ab..c42746cef8b6 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -192,7 +192,7 @@ def f(txn: LoggingTransaction) -> List[Tuple[str, str]]: async def get_receipts_for_user_with_orderings( self, user_id: str, receipt_types: List[str] ) -> JsonDict: - def f(txn: LoggingTransaction) -> List[Tuple[str, str, int, int]]: + def f(txn: LoggingTransaction) -> List[Tuple[str, str, str, int, int]]: clause, args = make_in_list_sql_clause( self.database_engine, "rl.receipt_type", receipt_types ) From 084b7ce186134bce7a7645f3d12332f098e19997 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Thu, 14 Apr 2022 08:50:00 +0200 Subject: [PATCH 33/42] Fix types (again) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/storage/databases/main/receipts.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index c42746cef8b6..eee8b507c4b0 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -211,7 +211,7 @@ def f(txn: LoggingTransaction) -> List[Tuple[str, str, str, int, int]]: ) txn.execute(sql, [user_id] + list(args)) - return cast(List[Tuple[str, str, int, int]], txn.fetchall()) + return cast(List[Tuple[str, str, str, int, int]], txn.fetchall()) rows = await self.db_pool.runInteraction( "get_receipts_for_user_with_orderings", f From 91a4bf1646cbb86050db0e2e57dea573ad3a475c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Fri, 22 Apr 2022 21:50:51 +0200 Subject: [PATCH 34/42] Fix typo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/rest/client/read_marker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/rest/client/read_marker.py b/synapse/rest/client/read_marker.py index a6da6c32b476..1583e903cd88 100644 --- a/synapse/rest/client/read_marker.py +++ b/synapse/rest/client/read_marker.py @@ -54,7 +54,7 @@ async def on_POST( if self.config.experimental.msc2285_enabled: valid_receipt_types.add(ReceiptTypes.READ_PRIVATE) - if set(body.keys()) > (valid_receipt_types): + if set(body.keys()) > valid_receipt_types: raise SynapseError( 400, "Receipt type must be 'm.read', 'org.matrix.msc2285.read.private' or 'm.fully_read'" From ae0bf979c42a2dce7a95fa1e9f349f8cf1219ec1 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 27 Apr 2022 14:21:23 -0400 Subject: [PATCH 35/42] Add some notes. --- synapse/storage/databases/main/receipts.py | 35 +++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index eee8b507c4b0..b9938c37026b 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -136,6 +136,17 @@ async def get_receipts_for_room( async def get_last_receipt_event_id_for_user( self, user_id: str, room_id: str, receipt_types: List[str] ) -> Optional[str]: + """ + Fetch the latest receipt's event ID for a rooms for a user for the given receipt types. + + Args: + user_id: The user to fetch receipts for. + room_id: The room ID to fetch the receipt for. + receipt_types: The receipt types to check. + + Returns: + The latest receipt, if one exists, for any of the given types. + """ def f(txn: LoggingTransaction) -> List[Tuple[str, str]]: clause, args = make_in_list_sql_clause( self.database_engine, "rl.receipt_type", receipt_types @@ -165,6 +176,16 @@ def f(txn: LoggingTransaction) -> List[Tuple[str, str]]: async def get_latest_receipts_for_user( self, user_id: str, receipt_types: List[str] ) -> Dict[str, str]: + """ + Fetch the latest receipt's event ID for all rooms for a user for the given receipt types. + + Args: + user_id: The user to fetch receipts for. + receipt_types: The receipt types to check. + + Returns: + A map of room ID to the latest receipt for that room for any of the given types. + """ def f(txn: LoggingTransaction) -> List[Tuple[str, str]]: clause, args = make_in_list_sql_clause( self.database_engine, "rl.receipt_type", receipt_types @@ -192,6 +213,16 @@ def f(txn: LoggingTransaction) -> List[Tuple[str, str]]: async def get_receipts_for_user_with_orderings( self, user_id: str, receipt_types: List[str] ) -> JsonDict: + """ + Fetch receipts in all rooms for a user. + + Args: + user_id: The user to fetch receipts for. + receipt_types: The receipt types to fetch. + + Returns: + A map of room ID to the latest receipt (for the given types). + """ def f(txn: LoggingTransaction) -> List[Tuple[str, str, str, int, int]]: clause, args = make_in_list_sql_clause( self.database_engine, "rl.receipt_type", receipt_types @@ -216,6 +247,8 @@ def f(txn: LoggingTransaction) -> List[Tuple[str, str, str, int, int]]: rows = await self.db_pool.runInteraction( "get_receipts_for_user_with_orderings", f ) + # TODO This looks wrong, there's no ordering being applied and the above + # query may return multiple results per room. return { row[0]: { "event_id": row[1], @@ -573,7 +606,7 @@ def insert_linearized_receipt_txn( data: JsonDict, stream_id: int, ) -> Optional[int]: - """Inserts a read-receipt into the database if it's newer than the current RR + """Inserts a receipt into the database if it's newer than the current one. Returns: None if the RR is older than the current RR From 4e255a60bd03e38e259f4f544fc306c508960490 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 28 Apr 2022 07:24:09 -0400 Subject: [PATCH 36/42] Add caching back to get_last_receipt_event_id_for_user. --- synapse/handlers/sync.py | 2 +- synapse/storage/databases/main/receipts.py | 74 ++++++++++++++-------- 2 files changed, 48 insertions(+), 28 deletions(-) diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index 5f7df38d49ac..2c555a66d066 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -1045,7 +1045,7 @@ async def unread_notifs_for_room_id( last_unread_event_id = await self.store.get_last_receipt_event_id_for_user( user_id=sync_config.user.to_string(), room_id=room_id, - receipt_types=[ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE], + receipt_types=(ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE), ) return await self.store.get_unread_event_push_actions_by_room_for_user( diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index b9938c37026b..57495795064a 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -134,44 +134,62 @@ async def get_receipts_for_room( ) async def get_last_receipt_event_id_for_user( - self, user_id: str, room_id: str, receipt_types: List[str] + self, user_id: str, room_id: str, receipt_types: Iterable[str] ) -> Optional[str]: """ - Fetch the latest receipt's event ID for a rooms for a user for the given receipt types. + Fetch the event ID for the latest receipt (of any of the given types). Args: user_id: The user to fetch receipts for. room_id: The room ID to fetch the receipt for. - receipt_types: The receipt types to check. + receipt_type: The receipt type to fetch. Returns: - The latest receipt, if one exists, for any of the given types. + The latest receipt and stream ordering, if one exists. """ - def f(txn: LoggingTransaction) -> List[Tuple[str, str]]: - clause, args = make_in_list_sql_clause( - self.database_engine, "rl.receipt_type", receipt_types - ) - sql = """ - SELECT rl.event_id, e.stream_ordering - FROM receipts_linearized AS rl - INNER JOIN events AS e USING (room_id, event_id) - WHERE rl.user_id = ? - AND rl.room_id = ? - AND %s - ORDER BY e.stream_ordering DESC - LIMIT 1 - """ % ( - clause, - ) + latest_event_id: Optional[str] = None + latest_stream_ordering = 0 + for receipt_type in receipt_types: + result = await self._get_last_receipt_event_id_for_user(user_id, room_id, receipt_type) + if result is None: + continue + event_id, stream_ordering = result - txn.execute(sql, [user_id, room_id] + list(args)) - return cast(List[Tuple[str, str]], txn.fetchall()) + if latest_event_id is None or latest_stream_ordering < stream_ordering: + latest_event_id = event_id + latest_stream_ordering = stream_ordering - rows = await self.db_pool.runInteraction("get_own_receipt_for_user", f) + return latest_event_id - if len(rows) == 0: - return None - return rows[0][0] + @cached() + async def _get_last_receipt_event_id_for_user( + self, user_id: str, room_id: str, receipt_type: str + ) -> Optional[Tuple[str, int]]: + """ + Fetch the event ID and stream ordering for the latest receipt. + + Args: + user_id: The user to fetch receipts for. + room_id: The room ID to fetch the receipt for. + receipt_type: The receipt type to fetch. + + Returns: + The latest receipt and stream ordering, if one exists. + """ + sql = """ + SELECT event_id, stream_ordering + FROM receipts_linearized + INNER JOIN events USING (room_id, event_id) + WHERE user_id = ? + AND room_id = ? + AND receipt_type = ? + """ + + def f(txn: LoggingTransaction) -> Optional[Tuple[str, int]]: + txn.execute(sql, (user_id, room_id, receipt_type)) + return cast(Optional[Tuple[str, int]], txn.fetchone()) + + return await self.db_pool.runInteraction("get_own_receipt_for_user", f) async def get_latest_receipts_for_user( self, user_id: str, receipt_types: List[str] @@ -199,7 +217,6 @@ def f(txn: LoggingTransaction) -> List[Tuple[str, str]]: AND %s ) t WHERE t.rw = 1; - """ % ( clause, ) @@ -576,6 +593,9 @@ def invalidate_caches_for_receipt( self, room_id: str, receipt_type: str, user_id: str ) -> None: self._get_linearized_receipts_for_room.invalidate((room_id,)) + self._get_last_receipt_event_id_for_user.invalidate( + (user_id, room_id, receipt_type) + ) self._invalidate_get_users_with_receipts_in_room(room_id, receipt_type, user_id) self.get_receipts_for_room.invalidate((room_id, receipt_type)) From e9055ac3bd21541e7bfea3c4b029ff2e3e77a1aa Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 28 Apr 2022 08:32:44 -0400 Subject: [PATCH 37/42] Add caching back to get_latest_receipts_for_user. --- synapse/push/push_tools.py | 2 +- synapse/storage/databases/main/receipts.py | 109 ++++++++++++--------- 2 files changed, 63 insertions(+), 48 deletions(-) diff --git a/synapse/push/push_tools.py b/synapse/push/push_tools.py index 0137b0ad6e15..dc74eb7afba0 100644 --- a/synapse/push/push_tools.py +++ b/synapse/push/push_tools.py @@ -25,7 +25,7 @@ async def get_badge_count(store: DataStore, user_id: str, group_by_room: bool) - joins = await store.get_rooms_for_user(user_id) my_receipts_by_room = await store.get_latest_receipts_for_user( - user_id, [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE] + user_id, (ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE) ) badge = len(invites) diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index 57495795064a..7d18096f7ef0 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -150,7 +150,9 @@ async def get_last_receipt_event_id_for_user( latest_event_id: Optional[str] = None latest_stream_ordering = 0 for receipt_type in receipt_types: - result = await self._get_last_receipt_event_id_for_user(user_id, room_id, receipt_type) + result = await self._get_last_receipt_event_id_for_user( + user_id, room_id, receipt_type + ) if result is None: continue event_id, stream_ordering = result @@ -192,10 +194,10 @@ def f(txn: LoggingTransaction) -> Optional[Tuple[str, int]]: return await self.db_pool.runInteraction("get_own_receipt_for_user", f) async def get_latest_receipts_for_user( - self, user_id: str, receipt_types: List[str] + self, user_id: str, receipt_types: Iterable[str] ) -> Dict[str, str]: """ - Fetch the latest receipt's event ID for all rooms for a user for the given receipt types. + Fetch the event IDs of a user's receipts for all rooms for the given receipt types. Args: user_id: The user to fetch receipts for. @@ -204,31 +206,17 @@ async def get_latest_receipts_for_user( Returns: A map of room ID to the latest receipt for that room for any of the given types. """ - def f(txn: LoggingTransaction) -> List[Tuple[str, str]]: - clause, args = make_in_list_sql_clause( - self.database_engine, "rl.receipt_type", receipt_types - ) - sql = """ - SELECT t.room_id, t.event_id FROM ( - SELECT rl.room_id, rl.event_id, row_number() over (partition by rl.room_id order by e.stream_ordering desc) rw - FROM receipts_linearized AS rl - INNER JOIN events AS e USING (room_id, event_id) - WHERE rl.user_id = ? - AND %s - ) t - WHERE t.rw = 1; - """ % ( - clause, - ) - - txn.execute(sql, [user_id] + list(args)) - return cast(List[Tuple[str, str]], txn.fetchall()) + results = await self.get_receipts_for_user_with_orderings( + user_id, receipt_types + ) - rows = await self.db_pool.runInteraction("get_latest_receipts_for_user", f) - return {row[0]: row[1] for row in rows} + # Reduce the result to room ID -> event ID. + return { + room_id: room_result["event_id"] for room_id, room_result in results.items() + } async def get_receipts_for_user_with_orderings( - self, user_id: str, receipt_types: List[str] + self, user_id: str, receipt_types: Iterable[str] ) -> JsonDict: """ Fetch receipts in all rooms for a user. @@ -240,38 +228,60 @@ async def get_receipts_for_user_with_orderings( Returns: A map of room ID to the latest receipt (for the given types). """ - def f(txn: LoggingTransaction) -> List[Tuple[str, str, str, int, int]]: - clause, args = make_in_list_sql_clause( - self.database_engine, "rl.receipt_type", receipt_types + results: JsonDict = {} + for receipt_type in receipt_types: + partial_result = await self._get_receipts_for_user_with_orderings( + user_id, receipt_type ) + for room_id, room_result in partial_result.items(): + # If the room has not yet been seen, or the receipt is newer, + # use it. + if ( + room_id not in results + or results[room_id]["stream_ordering"] + < room_result["stream_ordering"] + ): + results[room_id] = room_result - sql = """ - SELECT rl.room_id, rl.event_id, rl.receipt_type, - e.topological_ordering, e.stream_ordering - FROM receipts_linearized AS rl - INNER JOIN events AS e USING (room_id, event_id) - WHERE rl.room_id = e.room_id - AND rl.event_id = e.event_id - AND user_id = ? - AND %s - """ % ( - clause, - ) + return results + + @cached() + async def _get_receipts_for_user_with_orderings( + self, user_id: str, receipt_type: str + ) -> JsonDict: + """ + Fetch receipts in all rooms for a user. - txn.execute(sql, [user_id] + list(args)) - return cast(List[Tuple[str, str, str, int, int]], txn.fetchall()) + Args: + user_id: The user to fetch receipts for. + receipt_type: The receipt type to fetch. + + Returns: + A map of room ID to the latest receipt information. + """ + + def f(txn: LoggingTransaction) -> List[Tuple[str, str, int, int]]: + sql = ( + "SELECT rl.room_id, rl.event_id," + " e.topological_ordering, e.stream_ordering" + " FROM receipts_linearized AS rl" + " INNER JOIN events AS e USING (room_id, event_id)" + " WHERE rl.room_id = e.room_id" + " AND rl.event_id = e.event_id" + " AND user_id = ?" + " AND receipt_type = ?" + ) + txn.execute(sql, (user_id, receipt_type)) + return cast(List[Tuple[str, str, int, int]], txn.fetchall()) rows = await self.db_pool.runInteraction( "get_receipts_for_user_with_orderings", f ) - # TODO This looks wrong, there's no ordering being applied and the above - # query may return multiple results per room. return { row[0]: { "event_id": row[1], - "receipt_type": row[2], - "topological_ordering": row[3], - "stream_ordering": row[4], + "topological_ordering": row[2], + "stream_ordering": row[3], } for row in rows } @@ -592,6 +602,7 @@ def _invalidate_get_users_with_receipts_in_room( def invalidate_caches_for_receipt( self, room_id: str, receipt_type: str, user_id: str ) -> None: + self._get_receipts_for_user_with_orderings.invalidate((user_id, receipt_type)) self._get_linearized_receipts_for_room.invalidate((room_id,)) self._get_last_receipt_event_id_for_user.invalidate( (user_id, room_id, receipt_type) @@ -825,6 +836,10 @@ def insert_graph_receipt_txn( receipt_type, user_id, ) + txn.call_after( + self._get_receipts_for_user_with_orderings.invalidate, + (user_id, receipt_type), + ) # FIXME: This shouldn't invalidate the whole cache txn.call_after(self._get_linearized_receipts_for_room.invalidate, (room_id,)) From 8041f829cdca4d774c5cc9bb2966ddefdbc5e5f6 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 28 Apr 2022 08:33:41 -0400 Subject: [PATCH 38/42] Rename get_latest_receipts_for_user back to get_receipts_for_user to reduce diff. --- synapse/push/push_tools.py | 2 +- synapse/storage/databases/main/receipts.py | 2 +- .../replication/slave/storage/test_receipts.py | 18 +++++++----------- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/synapse/push/push_tools.py b/synapse/push/push_tools.py index dc74eb7afba0..a1bf5b20dd42 100644 --- a/synapse/push/push_tools.py +++ b/synapse/push/push_tools.py @@ -24,7 +24,7 @@ async def get_badge_count(store: DataStore, user_id: str, group_by_room: bool) - invites = await store.get_invited_rooms_for_local_user(user_id) joins = await store.get_rooms_for_user(user_id) - my_receipts_by_room = await store.get_latest_receipts_for_user( + my_receipts_by_room = await store.get_receipts_for_user( user_id, (ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE) ) diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index 7d18096f7ef0..4821efef5885 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -193,7 +193,7 @@ def f(txn: LoggingTransaction) -> Optional[Tuple[str, int]]: return await self.db_pool.runInteraction("get_own_receipt_for_user", f) - async def get_latest_receipts_for_user( + async def get_receipts_for_user( self, user_id: str, receipt_types: Iterable[str] ) -> Dict[str, str]: """ diff --git a/tests/replication/slave/storage/test_receipts.py b/tests/replication/slave/storage/test_receipts.py index dc37226fbdd9..5bbbd5fbcbab 100644 --- a/tests/replication/slave/storage/test_receipts.py +++ b/tests/replication/slave/storage/test_receipts.py @@ -81,7 +81,7 @@ def prepare(self, reactor, clock, homeserver): def test_return_empty_with_no_data(self): res = self.get_success( - self.master_store.get_latest_receipts_for_user( + self.master_store.get_receipts_for_user( OUR_USER_ID, [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE] ) ) @@ -104,7 +104,7 @@ def test_return_empty_with_no_data(self): ) self.assertEqual(res, None) - def test_get_latest_receipts_for_user(self): + def test_get_receipts_for_user(self): # Send some events into the first room event1_1_id = self.create_and_send_event( self.room_id1, UserID.from_string(OTHER_USER_ID) @@ -128,7 +128,7 @@ def test_get_latest_receipts_for_user(self): # Test we get the latest event when we want both private and public receipts res = self.get_success( - self.master_store.get_latest_receipts_for_user( + self.master_store.get_receipts_for_user( OUR_USER_ID, [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE] ) ) @@ -136,15 +136,13 @@ def test_get_latest_receipts_for_user(self): # Test we get the older event when we want only public receipt res = self.get_success( - self.master_store.get_latest_receipts_for_user( - OUR_USER_ID, [ReceiptTypes.READ] - ) + self.master_store.get_receipts_for_user(OUR_USER_ID, [ReceiptTypes.READ]) ) self.assertEqual(res, {self.room_id1: event1_1_id}) # Test we get the latest event when we want only the public receipt res = self.get_success( - self.master_store.get_latest_receipts_for_user( + self.master_store.get_receipts_for_user( OUR_USER_ID, [ReceiptTypes.READ_PRIVATE] ) ) @@ -157,9 +155,7 @@ def test_get_latest_receipts_for_user(self): ) ) res = self.get_success( - self.master_store.get_latest_receipts_for_user( - OUR_USER_ID, [ReceiptTypes.READ] - ) + self.master_store.get_receipts_for_user(OUR_USER_ID, [ReceiptTypes.READ]) ) self.assertEqual(res, {self.room_id1: event1_2_id}) @@ -175,7 +171,7 @@ def test_get_latest_receipts_for_user(self): ) ) res = self.get_success( - self.master_store.get_latest_receipts_for_user( + self.master_store.get_receipts_for_user( OUR_USER_ID, [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE] ) ) From 670e3e4d30b55fb5da0e552097c9ec5b6f595864 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 29 Apr 2022 10:13:13 -0400 Subject: [PATCH 39/42] Improve docstrings. --- synapse/handlers/receipts.py | 2 +- synapse/storage/databases/main/receipts.py | 9 +++++++-- tests/rest/client/test_sync.py | 10 +++++++++- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/receipts.py b/synapse/handlers/receipts.py index ec29f96c5f94..ae41fd674e13 100644 --- a/synapse/handlers/receipts.py +++ b/synapse/handlers/receipts.py @@ -112,7 +112,7 @@ async def _handle_new_receipts(self, receipts: List[ReadReceipt]) -> bool: ) if not res: - # res will be None if this read receipt is 'old' + # res will be None if this receipt is 'old' continue stream_id, max_persisted_id = res diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index 21d888c7f76d..3c9a2a330bbd 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -655,8 +655,8 @@ def insert_linearized_receipt_txn( """Inserts a receipt into the database if it's newer than the current one. Returns: - None if the RR is older than the current RR - otherwise, the rx timestamp of the event that the RR corresponds to + None if the receipt is older than the current receipt + otherwise, the rx timestamp of the event that the receipt corresponds to (or 0 if the event is unknown) """ assert self._can_write_to_receipts @@ -749,6 +749,10 @@ async def insert_receipt( Automatically does conversion between linearized and graph representations. + + Returns: + The new receipts stream ID and token, if the receipt is newer than + what was previously persisted. None, otherwise. """ assert self._can_write_to_receipts @@ -796,6 +800,7 @@ def graph_to_linear(txn: LoggingTransaction) -> str: stream_id=stream_id, ) + # If the receipt was older than the currently persisted one, nothing to do. if event_ts is None: return None diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index c4b5b5f50e5d..f7eee54ed020 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -424,7 +424,11 @@ def test_hidden_read_receipts(self) -> None: self.assertEqual(self._get_read_receipt(), None) @override_config({"experimental_features": {"msc2285_enabled": True}}) - def test_public_receipt_cannot_override_private(self) -> None: + def test_public_receipt_can_override_private(self) -> None: + """ + Sending a public read receipt to the same event which has a private read + receipt should cause that receipt to become public. + """ # Send a message as the first user res = self.helper.send(self.room_id, body="hello", tok=self.tok) @@ -452,6 +456,10 @@ def test_public_receipt_cannot_override_private(self) -> None: @override_config({"experimental_features": {"msc2285_enabled": True}}) def test_private_receipt_cannot_override_public(self) -> None: + """ + Sending a private read receipt to the same event which has a public read + receipt should cause no change. + """ # Send a message as the first user res = self.helper.send(self.room_id, body="hello", tok=self.tok) From 9e60cdbda19f60d2780278fbc1fe9670e28b741b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 29 Apr 2022 10:30:35 -0400 Subject: [PATCH 40/42] Document behavior of ordering of receipt types mattering. --- synapse/storage/databases/main/receipts.py | 27 ++++++++-------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index 3c9a2a330bbd..4c36083d9f19 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -153,7 +153,8 @@ async def get_last_receipt_event_id_for_user( Args: user_id: The user to fetch receipts for. room_id: The room ID to fetch the receipt for. - receipt_type: The receipt type to fetch. + receipt_type: The receipt types to fetch. Earlier receipt types + are given priority if multiple receipts point to the same event. Returns: The latest receipt and stream ordering, if one exists. @@ -238,7 +239,8 @@ async def get_receipts_for_user_with_orderings( Args: user_id: The user to fetch receipts for. - receipt_types: The receipt types to fetch. + receipt_types: The receipt types to fetch. Earlier receipt types + are given priority if multiple receipts point to the same event. Returns: A map of room ID to the latest receipt (for the given types). @@ -676,23 +678,14 @@ def insert_linearized_receipt_txn( # have to compare orderings of existing receipts if stream_ordering is not None: sql = ( - "SELECT e.stream_ordering, e.event_id, r.receipt_type FROM events AS e" + "SELECT stream_ordering, event_id FROM events" " INNER JOIN receipts_linearized AS r USING (event_id, room_id)" - " WHERE r.room_id = ? AND r.user_id = ?" + " WHERE r.room_id = ? AND r.receipt_type = ? AND r.user_id = ?" ) - txn.execute(sql, (room_id, user_id)) - - for so, eid, rt in txn: - if int(so) >= stream_ordering and ( - receipt_type == rt - # We don't allow private read receipts to override public - # ones since that would look as if the read receipt is going - # up in the timeline from the perspective of the other users - or ( - rt == ReceiptTypes.READ - and receipt_type == ReceiptTypes.READ_PRIVATE - ) - ): + txn.execute(sql, (room_id, receipt_type, user_id)) + + for so, eid in txn: + if int(so) >= stream_ordering: logger.debug( "Ignoring new receipt for %s in favour of existing " "one for later event %s", From 10822e19bec7288d4354be309c09060cfe30de04 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 29 Apr 2022 10:30:44 -0400 Subject: [PATCH 41/42] Minor refactoring. --- synapse/storage/databases/main/receipts.py | 2 +- tests/rest/client/test_sync.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index 4c36083d9f19..bba5660ef081 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -721,7 +721,7 @@ def insert_linearized_receipt_txn( ) if ( - receipt_type in [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE] + receipt_type in (ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE) and stream_ordering is not None ): self._remove_old_push_actions_before_txn( # type: ignore[attr-defined] diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index f7eee54ed020..67c94dd18fbd 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -421,7 +421,7 @@ def test_hidden_read_receipts(self) -> None: self.assertEqual(channel.code, 200) # Test that the first user can't see the other user's private read receipt - self.assertEqual(self._get_read_receipt(), None) + self.assertIsNone(self._get_read_receipt()) @override_config({"experimental_features": {"msc2285_enabled": True}}) def test_public_receipt_can_override_private(self) -> None: @@ -440,7 +440,7 @@ def test_public_receipt_can_override_private(self) -> None: access_token=self.tok2, ) self.assertEqual(channel.code, 200) - self.assertEqual(self._get_read_receipt(), None) + self.assertIsNone(self._get_read_receipt()) # Send a public read receipt channel = self.make_request( @@ -483,7 +483,7 @@ def test_private_receipt_cannot_override_public(self) -> None: self.assertEqual(channel.code, 200) # Test that we didn't override the public read receipt - self.assertEqual(self._get_read_receipt(), None) + self.assertIsNone(self._get_read_receipt()) @parameterized.expand( [ From 40782b8f9e8fc82e054473ca93c2ac4e77dc5c3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Sat, 30 Apr 2022 16:16:28 +0200 Subject: [PATCH 42/42] Fix comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/storage/databases/main/receipts.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index bba5660ef081..9e3d838eab9a 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -157,7 +157,7 @@ async def get_last_receipt_event_id_for_user( are given priority if multiple receipts point to the same event. Returns: - The latest receipt and stream ordering, if one exists. + The latest receipt, if one exists. """ latest_event_id: Optional[str] = None latest_stream_ordering = 0