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

Implement message forward pagination from start when no from is given, fixes #12383 #14149

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/14149.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix #12383: paginate room messages from the start if no from is given. Contributed by @gnunicorn .
gnunicorn marked this conversation as resolved.
Show resolved Hide resolved
6 changes: 6 additions & 0 deletions synapse/handlers/pagination.py
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,12 @@ async def get_messages(

if pagin_config.from_token:
from_token = pagin_config.from_token
elif pagin_config.direction == "f":
from_token = (
await self.hs.get_event_sources().get_start_token_for_pagination(
room_id
)
)
Comment on lines +452 to +456
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should clamp to the user's join or just let them get filtered out?

Copy link
Member

Choose a reason for hiding this comment

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

Although I guess that depends on history visibility, so likely doesn't make sense to add that complexity here.

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 have no idea how visibility is managed right now, isn't that happening in the filtering later? That's how I understood the requesting part...

If you think this isn't properly covered I can look into adding a test that ensure we are doing this right...

Copy link
Contributor

Choose a reason for hiding this comment

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

Fairly sure the visibility should already be getting filtered later, otherwise you could already read messages you're not supposed to by crafting the start token yourself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@reivilibre I read that right as "no other changes needed"?

else:
from_token = (
await self.hs.get_event_sources().get_current_token_for_pagination(
Expand Down
13 changes: 13 additions & 0 deletions synapse/streams/events.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,19 @@ def get_current_token(self) -> StreamToken:
)
return token

@trace
async def get_start_token_for_pagination(self, room_id: str) -> StreamToken:
"""Get the start token for a given room to be used to paginate
events.

The returned token does not have the current values for fields other
than `room`, since they are not used during pagination.

Returns:
The start token for pagination.
"""
return StreamToken.START

@trace
async def get_current_token_for_pagination(self, room_id: str) -> StreamToken:
"""Get the current token for a given room to be used to paginate
Expand Down
40 changes: 40 additions & 0 deletions tests/rest/admin/test_room.py
Original file line number Diff line number Diff line change
Expand Up @@ -1857,6 +1857,46 @@ def test_stream_token_is_accepted_for_fwd_pagianation(self) -> None:
self.assertIn("chunk", channel.json_body)
self.assertIn("end", channel.json_body)

def test_room_messages_backward(self) -> None:
"""Test room messages can be retrieved by an admin that isn't in the room."""
latest_event_id = self.helper.send(
self.room_id, body="message 1", tok=self.user_tok
)["event_id"]

# Check that we get the first and second message when querying /messages.
channel = self.make_request(
"GET",
"/_synapse/admin/v1/rooms/%s/messages?dir=b" % (self.room_id,),
access_token=self.admin_user_tok,
)
self.assertEqual(channel.code, 200, channel.json_body)

chunk = channel.json_body["chunk"]
self.assertEqual(len(chunk), 6, [event["content"] for event in chunk])

# in backwards, this is the first event
self.assertEqual(chunk[0]["event_id"], latest_event_id)

def test_room_messages_forward(self) -> None:
"""Test room messages can be retrieved by an admin that isn't in the room."""
latest_event_id = self.helper.send(
self.room_id, body="message 1", tok=self.user_tok
)["event_id"]

# Check that we get the first and second message when querying /messages.
channel = self.make_request(
"GET",
"/_synapse/admin/v1/rooms/%s/messages?dir=f" % (self.room_id,),
access_token=self.admin_user_tok,
)
self.assertEqual(channel.code, 200, channel.json_body)

chunk = channel.json_body["chunk"]
self.assertEqual(len(chunk), 6, [event["content"] for event in chunk])

# in forward, this is the last event
self.assertEqual(chunk[5]["event_id"], latest_event_id)

def test_room_messages_purge(self) -> None:
"""Test room messages can be retrieved by an admin that isn't in the room."""
store = self.hs.get_datastores().main
Expand Down