Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix /messages throwing a 500 when querying for non-existent room #12683

Merged
merged 3 commits into from
May 11, 2022
Merged

Fix /messages throwing a 500 when querying for non-existent room #12683

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a9c242b
Fix /messages throwing a 500 when querying for non-existent room
MadLittleMods May 10, 2022
9dc7796
Add changelog
MadLittleMods May 10, 2022
1896cfc
Use standard regression fix changelog
MadLittleMods May 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion synapse/handlers/pagination.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@ async def get_messages(
)
# 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
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved

if pagin_config.limit is None:
# This shouldn't happen as we've set a default limit before this
Expand Down
26 changes: 11 additions & 15 deletions synapse/storage/databases/main/stream.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -785,22 +785,14 @@ async def get_last_event_in_room_before_stream_ordering(
return None

async def get_current_room_stream_token_for_room_id(
self, room_id: Optional[str] = None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've checked the usages. We never have to worry about a case where the room_id is optional.

get_current_token_for_pagination
get_current_key_for_room
get_current_room_stream_token_for_room_id

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,
Expand Down Expand Up @@ -870,7 +862,11 @@ def _get_max_topological_txn(self, txn: LoggingTransaction, room_id: str) -> int
)

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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though this function was annotated to always return int, it previously returned None when the room_id didn't exist in the database (rows = [(None,)] in this case).

Now we properly fallback to 0 topological_ordering when there is no room entry.

Copy link
Contributor

Choose a reason for hiding this comment

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

great spot, thanks!


@staticmethod
def _set_before_and_after(
Expand Down