element-hq / element-hq/synapse

Potential bugs in `/relations` endpoint

Open
#13,862 0 comments 0 reactions 0 assignees View on GitHub
A-Messages-Endpoint A-Relations-Endpoint O-Uncommon S-Minor T-Task Z-Cleanup
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.