Implement pagination for MSC2666 #19279
Conversation
| HTTPStatus.BAD_REQUEST, | ||
| "You cannot request a list of shared rooms with yourself", | ||
| errcode=Codes.INVALID_PARAM, | ||
| errcode=Codes.UNKNOWN, |
There was a problem hiding this comment.
This change was made a couple years back on the MSC because 422 is a weird http status that nobody uses
anoadragon453
left a comment
anoadragon453
left a comment
There was a problem hiding this comment.
A few things here and there, but on the whole this is mostly good out of the gate. Thank you for moving this highly-requested feature along @tulir!
| # handle funny room IDs in old pre-v12 rooms properly. We also truncate it | ||
| # to stay within the 255-character limit of opaque tokens. | ||
| next_batch = encode_base64(rooms[-1].encode("utf-8"), urlsafe=True)[:255] |
There was a problem hiding this comment.
We should acknowledge in a comment that this list can and will change underneath the user, and as such may omit or duplicate a room - but that otherwise this is a simple and pragmatic solution.
There was a problem hiding this comment.
I updated the comment in the bisect code
| if b"batch_token" in args: | ||
| raise SynapseError( | ||
| HTTPStatus.BAD_REQUEST, | ||
| "Unknown batch_token", |
There was a problem hiding this comment.
Wow this was an exceptionally unhelpful error.
There was a problem hiding this comment.
If you're feeling up to it, I would love to see some Complement tests for this endpoint as well.
But not a blocker for this PR (or the spec).
| u1_token = self.login(u1, "pass") | ||
| u2 = self.register_user("user2", "pass") | ||
| u2_token = self.login(u2, "pass") | ||
| self.get_success(self.hs.get_datastores().main.set_ratelimit_for_user(u1, 0, 0)) |
There was a problem hiding this comment.
We should be eliminating the ratelimit in the test homeserver config already:
Lines 186 to 203 in a096fba
Did you find this was necessary? (Or is it just due to us missing rc_room_creation in that config?)
There was a problem hiding this comment.
The test exploded with a 429 in room creation until I added that, I guess it's the missing rc_room_creation
There was a problem hiding this comment.
I had to add both rc_room_creation and rc_invites->per_issuer to make it pass without the ratelimit override
| if from_batch: | ||
| # A from_batch token was provided, so cut off any rooms where the ID is | ||
| # lower than or equal to the token | ||
| rooms = rooms[bisect(rooms, from_batch) :] |
There was a problem hiding this comment.
Note that in the case where an invalid from token is supplied (but is still a base64-decodable string), bisect will still return an index where it can be inserted and the user will get a list.
I think this is fine, since it's unlikely that someone will provide a valid base64 string that's not the from token (and thus we don't really need to handle this edge case with a specific error). But we should probably leave a comment about it.
There was a problem hiding this comment.
Yeah it's intentional that the exact value of the token isn't validated. Due to the truncation it's not even guaranteed to be a real room ID, plus even if it is, the room might have disappeared after the first request.
| self.assertEqual(200, channel.code, channel.result) | ||
| self.assertEqual(channel.json_body["joined"], room_ids[10:20]) | ||
| self.assertNotIn("next_batch", channel.json_body) | ||
|
|
There was a problem hiding this comment.
Could you add some additional tests for:
- an invalid from token being passed (specifically something that would trigger INVALID_PARAM).
- a batch limit of 10, and a set of 10 mutual rooms. check that next_batch is not provided.
- trying to get mutual rooms with a user that does not exist.
Co-authored-by: Andrew Morgan <[email protected]>
2 participants
Pull Request Checklist