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

Potential bugs in /relations endpoint #13862

@clokep

Description

@clokep

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-Messages-Endpoint/messages client API endpoint (`RoomMessageListRestServlet`) (which also triggers /backfill)A-Relations-Endpoint/relationsO-UncommonMost users are unlikely to come across this or unexpected workflowS-MinorBlocks non-critical functionality, workarounds exist.T-TaskRefactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.Z-CleanupThings we want to get rid of, but aren't actively causing pain

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions