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

Admin API for server notice: consistently bypass rate limits #16670

Merged
merged 3 commits into from
Nov 22, 2023

Conversation

MatMaul
Copy link
Contributor

@MatMaul MatMaul commented Nov 21, 2023

Signed-off-by: Mathieu Velten matmaul@gmail.com

A lot of the underlying calls in the server notice code are already using ratelimit=False, but some are missing leading to still being rate limited when using the API in bulk.

Pull Request Checklist

@MatMaul MatMaul requested a review from a team as a code owner November 21, 2023 15:01
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Thanks Mathieu. How confident are we that this covers everything, i.e. that we haven't missed any other handler calls?

@MatMaul MatMaul changed the title Admin API for server notice: disable rate limit for all calls Admin API for server notice: consistently bypass rate limits Nov 22, 2023
@MatMaul MatMaul changed the title Admin API for server notice: consistently bypass rate limits Admin API for server notices: consistently bypass rate limits Nov 22, 2023
@MatMaul MatMaul changed the title Admin API for server notices: consistently bypass rate limits Admin API for server notice: consistently bypass rate limits Nov 22, 2023
Co-authored-by: David Robertson <david.m.robertson1@gmail.com>
@MatMaul
Copy link
Contributor Author

MatMaul commented Nov 22, 2023

Thanks Mathieu. How confident are we that this covers everything, i.e. that we haven't missed any other handler calls?

I am now able to use this API in bulk for all the users of a test homeserver with 1000 test users, while I couldn't before, so I am pretty confident it is fine now.
I also had a look at the whole code in this handler file since it's pretty small and couldn't spot anything else.

@DMRobertson
Copy link
Contributor

Force-merging because CI passed on the previous commit, and the only change since then is to the changelog.

@DMRobertson DMRobertson merged commit c432d8f into matrix-org:develop Nov 22, 2023
35 of 36 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants