element-hq / element-hq/synapse
Potential bugs in `/relations` endpoint
- Dominant language
- Python
- Stars
- 4.6k
- Forks
- 600
- Avg merge
- 5d 22h
- Merged PRs (30d)
- 51
Description
This issue has been migrated from [#13862](https://github.com/matrix-org/synapse/issues/13862).
---
The `/relations` endpoint is similar to `/messages`, but essentially filters over a subset of events related to a specific event.
The core code that figures out which events to return from `/relations` is `synapse.handlers.relations.RelationsHandler.get_relations`, while for `/messages` it is `synapse.handlers.pagination.PaginationHandler.get_messages`, these then call down into similar datastore methods.
There's some subtle (and not so subtle) differences between them.
Differences which might cause bugs:
* I think both start at the "current" stream token if no `from_token` is given, but `get_messages` calculates this, while `get_relations` leaves it implicit and just searches to the edge of the table.
* `get_messages` takes a "pagination" lock -- should this be shared with `get_relations`?
* `get_messages` tries harder to not show users things from when they were not in the room.
* `get_messages` might backfill.
* `get_messages` fetches 2 * limit and then filters with `_filter_results` which is something to do with a multi-worker setup. `get_relations` does not do this.
Interface differenes (I don't think these are bugs, but are just confusing when trying to compare the two methods):
* `get_messages` always returns a "next" `StreamToken` and then sometimes doesn't use it; `get_relations` does not return one when there's no more data.
* Not really a bug, but this means that `get_messages` will sometimes return a stream token when there's no additional data, e.g. if the last page has exactly `limit` entries in it. This will (potentially) cause an extra roundtrip from the client which could easily be avoided in most cases.
* `from_token` is optional in `get_relations` (see first bullet point above -- I suspect this logic of finding the proper starting point should be shared).
* Both implementations have a bunch of shared code (e.g. figuring out the from and to boundaries, calling `generate_pagination_where_clause`, generating the "next" token).
The final bit of this is that we likely don't test this endpoint as thoroughly as `/messages`. There's a start to some integration tests at matrix-org/complement#467, but they're very simplistic.
Contributor guide
Assessment
This issue has not been assessed yet.