Sliding sync: Add connection tracking to the account_data extension
#17695
Conversation
Conflicts: synapse/handlers/sliding_sync/extensions.py
MadLittleMods
changed the title
account_data extension
| # and return the set of tags at that time but we don't track | ||
| # changes to tags so we just have to return all tags for the | ||
| # room. | ||
| immutable_tag_map = await self.store.get_tags_for_room( |
There was a problem hiding this comment.
Cache for get_tags_for_room(...) is being added in #17730
Add cache to `get_tags_for_room(...)` This helps Sliding Sync because `get_tags_for_room(...)` is going to be used in #17695 Essentially, we're just trying to match `get_account_data_for_room(...)` which already has a tree cache.
| immutable_tag_map = await self.store.get_tags_for_room( | ||
| user_id, room_id | ||
| ) | ||
| if immutable_tag_map: |
There was a problem hiding this comment.
Isn't it valid if we remove all tags from the room?
There was a problem hiding this comment.
hmm, I think you're right. It does make sense to show an empty set of tags in an incremental sync if they changed
There was a problem hiding this comment.
Updated, check if the tests match your expectations
| @@ -480,6 +482,337 @@ def test_room_account_data_incremental_sync(self) -> None: | |||
| {"tags": {"m.favourite": {}, "m.server_notice": {}}}, | |||
| ) | |||
|
|
|||
| def test_room_account_data_incremental_sync_out_of_range_never(self) -> None: | |||
| """Tests that we don't return account data for rooms that fall out of | |||
| range, but then do send all account data once they're back in range. | |||
There was a problem hiding this comment.
This looks like the doc for PREVIOUSLY rather than NEVER
There was a problem hiding this comment.
Updated
Tests that we don't return account data for rooms that are out of
range, but then do send all account data once they're in range.
| {AccountDataTypes.TAG: {"tags": immutable_tag_map}} | ||
| if immutable_tag_map | ||
| else {}, |
There was a problem hiding this comment.
Do we want to always return tags in an intial sync?
The problem is we can't tell no tag account data from empty set of tags.
There was a problem hiding this comment.
We probably don't? Though I don't think it really matters
There was a problem hiding this comment.
I think I got it right. Only show the empty tag set if we previously told the client about tags. Check if the tests match your expectations
|
Feels like I shouldn't self-approve my own work 🤔 Update: Actually, I can just approve for you since it's just a technicality for you and the PR also looks good to me. |
This is basically exactly the same logic as for receipts. Essentially we just need to track which room account data we have and haven't sent down to clients, and use that when we pull stuff out.
I think this just needs a couple of extra tests written