Skip to content
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

Potential bugs in /relations endpoint #13862

Open
matrixbot opened this issue Dec 20, 2023 · 0 comments
Open

Potential bugs in /relations endpoint #13862

matrixbot opened this issue Dec 20, 2023 · 0 comments

Comments

@matrixbot
Copy link
Collaborator

matrixbot commented Dec 20, 2023

This issue has been migrated from #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant