From 84facf769eb79112be5f21942c18047b2b85f0bd Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 10 May 2022 23:39:14 -0500 Subject: [PATCH] Fix `/messages` throwing a 500 when querying for non-existent room (#12683) Fix https://github.com/matrix-org/synapse/issues/12678 Complement test added: https://github.com/matrix-org/complement/pull/369 **Before:** 500 internal server error **After:** According to the [spec](https://spec.matrix.org/latest/client-server-api/#get_matrixclientv3roomsroomidmessages), calling `/messages` against a non-existent `room_id` should throw a 403 forbidden (since you're not part of the room). This also matches the behavior before https://github.com/matrix-org/synapse/pull/12370 which regressed Synapse to the 500 behavior. ```json { "errcode": "M_FORBIDDEN", "error": "User @test:my.synapse.server not in room !dne:my.synapse.server, and room previews are disabled" } ``` --- changelog.d/12683.bugfix | 1 + synapse/handlers/pagination.py | 2 +- synapse/storage/databases/main/stream.py | 26 ++++++++++-------------- 3 files changed, 13 insertions(+), 16 deletions(-) create mode 100644 changelog.d/12683.bugfix diff --git a/changelog.d/12683.bugfix b/changelog.d/12683.bugfix new file mode 100644 index 000000000..2ce84a223 --- /dev/null +++ b/changelog.d/12683.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in Synapse 1.57.0 where `/messages` would throw a 500 error when querying for a non-existent room. diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index 7ee334037..2e3018009 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -448,7 +448,7 @@ class PaginationHandler: ) # We expect `/messages` to use historic pagination tokens by default but # `/messages` should still works with live tokens when manually provided. - assert from_token.room_key.topological + assert from_token.room_key.topological is not None if pagin_config.limit is None: # This shouldn't happen as we've set a default limit before this diff --git a/synapse/storage/databases/main/stream.py b/synapse/storage/databases/main/stream.py index 793e90663..4e1d9647b 100644 --- a/synapse/storage/databases/main/stream.py +++ b/synapse/storage/databases/main/stream.py @@ -785,22 +785,14 @@ class StreamWorkerStore(EventsWorkerStore, SQLBaseStore): return None async def get_current_room_stream_token_for_room_id( - self, room_id: Optional[str] = None + self, room_id: str ) -> RoomStreamToken: - """Returns the current position of the rooms stream. - - By default, it returns a live token with the current global stream - token. Specifying a `room_id` causes it to return a historic token with - the room specific topological token. - """ + """Returns the current position of the rooms stream (historic token).""" stream_ordering = self.get_room_max_stream_ordering() - if room_id is None: - return RoomStreamToken(None, stream_ordering) - else: - topo = await self.db_pool.runInteraction( - "_get_max_topological_txn", self._get_max_topological_txn, room_id - ) - return RoomStreamToken(topo, stream_ordering) + topo = await self.db_pool.runInteraction( + "_get_max_topological_txn", self._get_max_topological_txn, room_id + ) + return RoomStreamToken(topo, stream_ordering) def get_stream_id_for_event_txn( self, @@ -870,7 +862,11 @@ class StreamWorkerStore(EventsWorkerStore, SQLBaseStore): ) rows = txn.fetchall() - return rows[0][0] if rows else 0 + # An aggregate function like MAX() will always return one row per group + # so we can safely rely on the lookup here. For example, when a we + # lookup a `room_id` which does not exist, `rows` will look like + # `[(None,)]` + return rows[0][0] if rows[0][0] is not None else 0 @staticmethod def _set_before_and_after(