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

Sliding sync: remove the list ops from the response #17671

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

erikjohnston
Copy link
Member

This isn't in MSC4186, sync ops don't make any sense in the API, nothing currently uses it, and it takes up a lot of space in the sync result.

We probably do want to indicate which rooms are in which lists, but that can be added later.

@erikjohnston erikjohnston marked this pull request as ready for review September 5, 2024 14:06
@erikjohnston erikjohnston requested a review from a team as a code owner September 5, 2024 14:06
@@ -622,57 +581,6 @@ def test_filter_list(self) -> None:
response_body["lists"].keys(),
)

# Make sure the lists have the correct rooms
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These assertions are important to the tests.

We need to check that the rooms are filtered accordingly. We could check in the response_body["rooms"] list at the very-least.

(same with the other spots)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I thought most of them were looking at the rooms already, but actually a few of them aren't.

I think there are a few options:

  • have an option on the sync servlet class that enables returning the rooms so that tests can assert its doing the right thing
  • change the tests so that they do one sync per filter to make sure that the filters are being correctly applied
  • change these to unit tests (if we don't already have them) against compute_interested_rooms

Copy link
Contributor

@MadLittleMods MadLittleMods Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just add the room_ids: [] list now? We've been making the decisions on Simplified Sliding Sync so I don't see why we can't just make it up now if we both agree.


More tests using the list of room ID's added in #17692 and #17703

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're concerned about the size bulk this adds to the response, we could make this an extension. Sounds decent to me

synapse/handlers/sliding_sync/room_lists.py Outdated Show resolved Hide resolved
synapse/rest/client/sync.py Show resolved Hide resolved
"""Add a new field to the lists response that allows us to track the
matching room IDs in the list.

This is not returend to the client.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This is not returend to the client.
This is not returned to the client.

@kegsay
Copy link
Contributor

kegsay commented Sep 16, 2024

We probably do want to indicate which rooms are in which lists, but that can be added later.

Whilst I appreciate wanting to break things up, if you don't indicate which room is in which list it cripples SSS. It makes the use of filters entirely useless because if I make two lists I don't know which room is in which list (which room matches which filter). We can't just strip out this information and not replace it in the same PR imho.

@erikjohnston
Copy link
Member Author

We probably do want to indicate which rooms are in which lists, but that can be added later.

Whilst I appreciate wanting to break things up, if you don't indicate which room is in which list it cripples SSS. It makes the use of filters entirely useless because if I make two lists I don't know which room is in which list (which room matches which filter). We can't just strip out this information and not replace it in the same PR imho.

That's fair, though I'd say that rust sdk doesn't use the list ops and I'd be surprised if anyone could really depend on the current implementation (note list ops are not in the msc). I don't think there is much harm in removing list ops (that no one is using, or really can use) and punting how to encode what room matches which lists till later.

Particularly, there may be no reason to have multiple lists with different filters (but there are use cases for multiple lists with different configs, eg different timeline limit for the top N rooms). And if not, no reason to need to encode which rooms match which lists.

@MadLittleMods
Copy link
Contributor

Element Web uses multiple lists and different filters.

@erikjohnston
Copy link
Member Author

Element Web uses multiple lists and different filters.

Do they use the list ops? Or will need to?

@MadLittleMods
Copy link
Contributor

By the nature of them having multiple lists with different filters, they need a list of room_ids that pertain to each list.

It looks like they do process ops but it could just be a list of room_ids: [] too.

@erikjohnston
Copy link
Member Author

Worth noting those filters seem to pertain to information the client will see, so they can infer? Would be very interested in knowing if EW actually used the list ops for those purposes, or if that code is just legacy from the older implementation.

@dbkr maybe you know?


I'm not sure if EW is something we need to worry about not breaking right now though? It's all behind labs flags and things still I believe

@kegsay
Copy link
Contributor

kegsay commented Sep 17, 2024

Currently EW does still read the ops to know which room goes into which list. I'm currently trying to get SSS working in EW hence why I'm chiming in here.

Worth noting those filters seem to pertain to information the client will see, so they can infer?

Inferring is only possible if the client also fetches the data used to make the filtering decision, which in EW's case is:

  • room tags (room account data)
  • DM map (global account data)
  • room type (create event)
  • self membership (m.room.member event for $ME)

This also then relies on the client and server having precisely the same algorithm to determine inclusion, which can be more annoying and subtle than at first glance (e.g if you have a DM entry with 2 rooms what do you do?).

That being said, EW does fetch this information so inferring should be possible.


Trying to rethink around filters: what we're actually doing here (in the absence of knowing which room went into which list) is controlling the priority order of which rooms to return initially. This is exactly what EW needs because it needs to populate the invite list / favourites / DMs (all of which are above "Rooms") immediately on load: if we just had 1 list then you might have a favourite with a very low bump_stamp because it is ancient which would pop into existence potentially minutes after login, depending how long the full room list pagination takes. By having >1 list, we can guarantee we see the first [0,N] entries with the highest bump_stamps per list.

This distinction (priority ordering vs filtering) is important. If you keep the concept of filters then you need to A) inform the client which rooms falls into which lists (the aforementioned room_ids), B) inform the client when rooms come out of that list (which implies some kind of DELETE op), C) manage the book keeping of all this on the server. If, however, you just have 1 list and you're controlling the priority of which room comes first, you don't need any of this, simplifying the overall API.

@dbkr
Copy link
Member

dbkr commented Sep 18, 2024

Yep, I don't really know - Kegan is the expert here, but this all sounds plausible. It's not completely ideal that the server & clients might have subtle differences / bugs that mean one think a room is in a list & one doesn't, but I think basically as long as the rooms that filter selects appear in the list, EW can just work out their categorisation as it always did pre-SS.

@kegsay
Copy link
Contributor

kegsay commented Oct 1, 2024

So tl;dr - you can remove the list ops from the response. We don't read them in EW anymore.

@anoadragon453
Copy link
Member

@erikjohnston @MadLittleMods It looks to me that this PR has fallen through the cracks a bit. Is it still waiting for team review, or does it need changes (other than conflict resolution) first?

@MadLittleMods MadLittleMods removed the request for review from a team November 6, 2024 18:38
@MadLittleMods
Copy link
Contributor

@anoadragon453 Removed the review request. It needs some work before we can consider it again.

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

Successfully merging this pull request may close these issues.

5 participants