-
Notifications
You must be signed in to change notification settings - Fork 424
Implement pagination for MSC2666 #19279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
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!
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be eliminating the ratelimit in the test homeserver config already:
Lines 186 to 203 in a096fba
| "rc_message": {"per_second": 10000, "burst_count": 10000}, | |
| "rc_registration": {"per_second": 10000, "burst_count": 10000}, | |
| "rc_login": { | |
| "address": {"per_second": 10000, "burst_count": 10000}, | |
| "account": {"per_second": 10000, "burst_count": 10000}, | |
| "failed_attempts": {"per_second": 10000, "burst_count": 10000}, | |
| }, | |
| "rc_joins": { | |
| "local": {"per_second": 10000, "burst_count": 10000}, | |
| "remote": {"per_second": 10000, "burst_count": 10000}, | |
| }, | |
| "rc_joins_per_room": {"per_second": 10000, "burst_count": 10000}, | |
| "rc_invites": { | |
| "per_room": {"per_second": 10000, "burst_count": 10000}, | |
| "per_user": {"per_second": 10000, "burst_count": 10000}, | |
| }, | |
| "rc_3pid_validation": {"per_second": 10000, "burst_count": 10000}, | |
| "rc_presence": {"per_user": {"per_second": 10000, "burst_count": 10000}}, |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to add both rc_room_creation and rc_invites->per_issuer to make it pass without the ratelimit override
Co-authored-by: Andrew Morgan <[email protected]>
Pull Request Checklist