-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Clean up types for PaginationConfig #8250
Changes from 3 commits
16b62fb
1ca19ce
bc4f225
2b97d11
8ac9a1d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Clean up type hints for `PaginationConfig`. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,14 +116,13 @@ async def _snapshot_all_rooms( | |
now_token = self.hs.get_event_sources().get_current_token() | ||
|
||
presence_stream = self.hs.get_event_sources().sources["presence"] | ||
pagination_config = PaginationConfig(from_token=now_token) | ||
presence, _ = await presence_stream.get_pagination_rows( | ||
user, pagination_config.get_source_config("presence"), None | ||
presence, _ = await presence_stream.get_new_events( | ||
user, from_key=None, include_offline=False | ||
) | ||
|
||
receipt_stream = self.hs.get_event_sources().sources["receipt"] | ||
receipt, _ = await receipt_stream.get_pagination_rows( | ||
user, pagination_config.get_source_config("receipt"), None | ||
joined_rooms = [r.room_id for r in room_list if r.membership == Membership.JOIN] | ||
receipt = await self.store.get_linearized_receipts_for_rooms( | ||
joined_rooms, to_key=int(now_token.receipt_key), | ||
) | ||
Comment on lines
+124
to
126
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm having a bit of trouble following how the keys were modified as part of this code, previously we have: pagination_config = PaginationConfig(from_token=now_token)
config = pagination_config.get_source_config("receipt")
to_key = int(config.from_key)
if config.to_key:
from_key = int(config.to_key)
else:
from_key = None Previously:
So this looks equivalent. |
||
|
||
tags_by_room = await self.store.get_tags_for_user(user_id) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -335,20 +335,18 @@ async def get_messages( | |
user_id = requester.user.to_string() | ||
|
||
if pagin_config.from_token: | ||
room_token = pagin_config.from_token.room_key | ||
from_token = pagin_config.from_token | ||
else: | ||
pagin_config.from_token = ( | ||
self.hs.get_event_sources().get_current_token_for_pagination() | ||
) | ||
room_token = pagin_config.from_token.room_key | ||
from_token = self.hs.get_event_sources().get_current_token_for_pagination() | ||
|
||
room_token = RoomStreamToken.parse(room_token) | ||
if pagin_config.limit is None: | ||
# This shouldn't happen as we've set a default limit before this | ||
# gets called. | ||
raise Exception("limit not set") | ||
Comment on lines
+342
to
+345
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we've been usually use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My reasoning here was that nothing stops someone from calling this from somewhere else, so it feels more plausible that this could happen |
||
|
||
pagin_config.from_token = pagin_config.from_token.copy_and_replace( | ||
"room_key", str(room_token) | ||
) | ||
room_token = RoomStreamToken.parse(from_token.room_key) | ||
|
||
source_config = pagin_config.get_source_config("room") | ||
from_token = from_token.copy_and_replace("room_key", str(room_token)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks to me like we're round-tripping the same value? What is this actually doing? (It could probably have a comment added.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Errr, I'm pretty sure it was pointless before and is pointless now. |
||
|
||
with await self.pagination_lock.read(room_id): | ||
( | ||
|
@@ -358,7 +356,7 @@ async def get_messages( | |
room_id, user_id, allow_departed_users=True | ||
) | ||
|
||
if source_config.direction == "b": | ||
if pagin_config.direction == "b": | ||
# if we're going backwards, we might need to backfill. This | ||
# requires that we have a topo token. | ||
if room_token.topological: | ||
|
@@ -381,22 +379,28 @@ async def get_messages( | |
member_event_id | ||
) | ||
if RoomStreamToken.parse(leave_token).topological < max_topo: | ||
source_config.from_key = str(leave_token) | ||
from_token = from_token.copy_and_replace( | ||
"room_key", leave_token | ||
) | ||
|
||
await self.hs.get_handlers().federation_handler.maybe_backfill( | ||
room_id, max_topo | ||
) | ||
|
||
to_room_key = None | ||
if pagin_config.to_token: | ||
to_room_key = pagin_config.to_token.room_key | ||
Comment on lines
+388
to
+390
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure where this matches up with the old version of this code? It seems previously we just always used what was in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was special logic in |
||
|
||
events, next_key = await self.store.paginate_room_events( | ||
room_id=room_id, | ||
from_key=source_config.from_key, | ||
to_key=source_config.to_key, | ||
direction=source_config.direction, | ||
limit=source_config.limit, | ||
from_key=from_token.room_key, | ||
to_key=to_room_key, | ||
direction=pagin_config.direction, | ||
limit=pagin_config.limit, | ||
event_filter=event_filter, | ||
) | ||
|
||
next_token = pagin_config.from_token.copy_and_replace("room_key", next_key) | ||
next_token = from_token.copy_and_replace("room_key", next_key) | ||
|
||
if events: | ||
if event_filter: | ||
|
@@ -409,7 +413,7 @@ async def get_messages( | |
if not events: | ||
return { | ||
"chunk": [], | ||
"start": pagin_config.from_token.to_string(), | ||
"start": from_token.to_string(), | ||
"end": next_token.to_string(), | ||
} | ||
|
||
|
@@ -438,7 +442,7 @@ async def get_messages( | |
events, time_now, as_client_event=as_client_event | ||
) | ||
), | ||
"start": pagin_config.from_token.to_string(), | ||
"start": from_token.to_string(), | ||
"end": next_token.to_string(), | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -432,8 +432,9 @@ async def get_events_for( | |
If explicit_room_id is set, that room will be polled for events only if | ||
it is world readable or the user has joined the room. | ||
""" | ||
from_token = pagination_config.from_token | ||
if not from_token: | ||
if pagination_config.from_token: | ||
from_token = pagination_config.from_token | ||
else: | ||
from_token = self.event_sources.get_current_token() | ||
Comment on lines
+435
to
438
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks equivalent, but I'm surprised that mypy complained? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Me too, I think it might be an edge case due to it crossing a function boundary or something. Possibly due to scoping rules? I spent ages hunting it down :/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a problem, just confirmation that it is meant to be the same is fine! 🥳 |
||
|
||
limit = pagination_config.limit | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is replacing:
Which I'm fairly certain is the same.