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

Potential bugs in /relations endpoint #13862

Open
clokep opened this issue Sep 21, 2022 · 0 comments
Open

Potential bugs in /relations endpoint #13862

clokep opened this issue Sep 21, 2022 · 0 comments
Labels
A-Messages-Endpoint /messages client API endpoint (`RoomMessageListRestServlet`) (which also triggers /backfill) A-Relations-Endpoint /relations O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Blocks non-critical functionality, workarounds exist. T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. Z-Cleanup Things we want to get rid of, but aren't actively causing pain

Comments

@clokep
Copy link
Member

clokep commented Sep 21, 2022

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.

@clokep clokep added S-Minor Blocks non-critical functionality, workarounds exist. T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. O-Uncommon Most users are unlikely to come across this or unexpected workflow labels Sep 21, 2022
@MadLittleMods MadLittleMods changed the title Potential bugs in /relations endpoint Potential bugs in /relations endpoint Apr 25, 2023
@MadLittleMods MadLittleMods added A-Messages-Endpoint /messages client API endpoint (`RoomMessageListRestServlet`) (which also triggers /backfill) A-Relations-Endpoint /relations Z-Cleanup Things we want to get rid of, but aren't actively causing pain labels Apr 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Messages-Endpoint /messages client API endpoint (`RoomMessageListRestServlet`) (which also triggers /backfill) A-Relations-Endpoint /relations O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Blocks non-critical functionality, workarounds exist. T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. Z-Cleanup Things we want to get rid of, but aren't actively causing pain
Projects
None yet
Development

No branches or pull requests

2 participants