-
Notifications
You must be signed in to change notification settings - Fork 429
Fix "list quarantined media" API to have semi-stable ordering #19308
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
https://github.com/element-hq/synapse/pull/19268/changes#r2614217300 wasn't applied and the migration had copy/paste artifacts. PR: #19268
| `from` and `limit` are optional parameters, and default to the first page and `100` respectively. `from` is the `next_batch` | ||
| token returned by a previous request and `limit` is the number of rows to return. Note that `next_batch` is not intended | ||
| to survive longer than about a minute and may produce inconsistent results if used after that time. Neither `from` or | ||
| `limit` is a timestamp, though `from` does encode a timestamp. | ||
|
|
||
| If you require a long-lived `from` token, split `next_batch` on `-` and combine the first part with a `0`, separated by | ||
| a `-` again. For example: `1234-5678` becomes `1234-0`. Your application will need to deduplicate `media` rows it has | ||
| already seen if using this method. |
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 do accept that this is custom, weird, and not-quite how pagination works. My defence is I have a custom, weird, not-quite-paginating use case 😇
(though, if someone more qualified wants to make this use PaginationHandler instead, I'd happily close this PR in favour of that one)
| elif len(start) > 0: | ||
| start_index = int(start) |
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 probably don't need this backwards compatibility given the PR which introduced the endpoint has only been on develop for a few days as of writing. I've kept it anyway because I honestly just don't want to fight the unit tests too much. I can be convinced to enter battle, however.
c7ec79d to
1128c59
Compare
1128c59 to
516b740
Compare
Note: This cleans up previous code introduced by #19268
When first writing the API, "unquarantining" media was not properly considered. If media was unquarantined and an application was treating
fromtokens as long-lived when listing quarantined media, the endpoint could skip rows like so:from=21to01from=2, gets[D]To fix this, we invent a pagination token which uses time and a relative index. It's not super stable still because the relative index can still change, but it's likely stable enough for most usage (iterate as fast as possible to the end).
If an application requires a proper time-based stable token, it can generate a timestamp then append
-0to it to set the relative position to the zeroth row. This may return rows the application has already seen, as described by the admin API docs. This particular method of generating the timestamp manually is not documented because it's not as stable as relying on the last seennext_batch's internal timestamp.This PR doesn't shift the whole endpoint to timestamp-only tokens because the prior PR populates rows with
0for a timestamp, which may span thousands (or millions) of rows, breaking the ability to uselimitproperly.Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.