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

Add experimental support for MSC3391: deleting account data #14714

Merged
merged 15 commits into from
Jan 1, 2023
Merged

Add experimental support for MSC3391: deleting account data #14714

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8173de4
Add experimental features flag
anoadragon453 Oct 20, 2022
8cd633f
Enable Complement tests for MSC3391
anoadragon453 Nov 11, 2022
d338a00
Add replication methods for removing account data
anoadragon453 Dec 20, 2022
36d3bd2
Add storage functions for deleting user/room account data
anoadragon453 Dec 20, 2022
dcfacc8
Add handler, servlet methods for deleting user/room account data
anoadragon453 Dec 20, 2022
ef562a2
Allow deleting account data by PUT'ing with empty content
anoadragon453 Nov 14, 2022
0f81122
Return a 404 if an account data type is empty
anoadragon453 Nov 15, 2022
2937973
Prevent deleted account data items appearing in initial syncs
anoadragon453 Nov 16, 2022
0520e4c
changelog
anoadragon453 Dec 20, 2022
80e991f
Simplify txn.rowcount return check
anoadragon453 Dec 29, 2022
3c62cd1
Remove unnecessary self.clock assignments in replication endpoints
anoadragon453 Dec 29, 2022
36eab04
Remove unnecessary self.store's in unstable account data servlets
anoadragon453 Dec 29, 2022
2aa8986
Document why we're not using simple_update
anoadragon453 Dec 29, 2022
ccd4225
Document the return value of simple_select_list(_txn)
anoadragon453 Dec 29, 2022
554dd36
Convert queries in get_account_data_for_user_txn to raw SQL
anoadragon453 Dec 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions synapse/rest/client/account_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class AccountDataServlet(RestServlet):

def __init__(self, hs: "HomeServer"):
super().__init__()
self._hs = hs
self.auth = hs.get_auth()
self.store = hs.get_datastores().main
self.handler = hs.get_account_data_handler()
Expand All @@ -54,6 +55,16 @@ async def on_PUT(

body = parse_json_object_from_request(request)

# If experimental support for MSC3391 is enabled, then providing an empty dict
# as the value for an account data type should be functionally equivalent to
# calling the DELETE method on the same type.
if self._hs.config.experimental.msc3391_enabled:
if body == {}:
await self.handler.remove_account_data_for_user(
user_id, account_data_type
)
return 200, {}
Comment on lines +58 to +66
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why we special case this -- won't adding it with the empty dict be equivalent?

Copy link
Member Author

Choose a reason for hiding this comment

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

While the content of the account data item is the same, the logic for handling the ignored_users table and the code to eventually populate a account_data_undelivered_deletes table is slightly different when deleting account data. That could all be handled conditionally in the add_account_data_for_user when supplying {}, but I found it cleaner to just have a separate remove_account_data_for_user handler function.

Copy link
Member

@clokep clokep Dec 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

the logic for handling the ignored_users table [...] is slightly different when deleting account data

But it gives the same result in a slightly more efficient manner, correct?

eventually populate a account_data_undelivered_deletes table is slightly different when deleting account data.

I guess I'm finding the logic about this hard to reason about since I don't really understand why we need this table. (And it isn't in this PR.)

I'm not really suggesting getting rid of the remove_account_data_for_user method, but rather why are we calling it when someone is setting the value to {} instead of just falling through to the old code.

I don't feel strongly either way, it sounds it is mostly "because a future PR cares".

Copy link
Member Author

Choose a reason for hiding this comment

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

But it gives the same result in a slightly more efficient manner, correct?

Yes, that's correct. Falling back to the old code would (at the point of this PR) produce the same result through a slightly less efficient process.

In my head, calling the remove_account_data_for_user method here is easier to reason about, as MSC3391 states "For backwards compatibility reasons, if a client PUTs {} for account data, that must be seen equivalent as DELETE-ing that account data." But I also appreciate that I was the one to influence that line being added - so it would be ingrained in my understanding of the problem.

eventually populate a account_data_undelivered_deletes table is slightly different when deleting account data.

I guess I'm finding the logic about this hard to reason about since I don't really understand why we need this table. (And it isn't in this PR.)

I'm not really suggesting getting rid of the remove_account_data_for_user method, but rather why are we calling it when someone is setting the value to {} instead of just falling through to the old code.

I don't feel strongly either way, it sounds it is mostly "because a future PR cares".

That is indeed the only hard blocker here (which is difficult to justify as the code's not written yet!).

But I'd still be inclined to keep this slightly unconventional split off into a separate handler even if there wasn't a future PR involved, because a PUT /user/{user_id}/account_data/{type} with a body of {} is supposed to be equivalent to DELETE of the same path. If we didn't have this, future changes to the functionality of DELETE's handler could easily accidentally break that truth.

Writing the above made me realise that we could go even farther into the deep end. Instead of calling the remove_account_data_for_user handler method here, we instead directly call the on_DELETE method of Unstable(Room)AccountDataServlet. That way we don't miss out on any special functionality existing in the DELETE servlet but not the handler.


await self.handler.add_account_data_for_user(user_id, account_data_type, body)

return 200, {}
Expand Down Expand Up @@ -124,6 +135,7 @@ class RoomAccountDataServlet(RestServlet):

def __init__(self, hs: "HomeServer"):
super().__init__()
self._hs = hs
self.auth = hs.get_auth()
self.store = hs.get_datastores().main
self.handler = hs.get_account_data_handler()
Expand Down Expand Up @@ -156,6 +168,16 @@ async def on_PUT(
Codes.BAD_JSON,
)

# If experimental support for MSC3391 is enabled, then providing an empty dict
# as the value for an account data type should be functionally equivalent to
# calling the DELETE method on the same type.
if self._hs.config.experimental.msc3391_enabled:
if body == {}:
await self.handler.remove_account_data_for_room(
user_id, room_id, account_data_type
)
return 200, {}

await self.handler.add_account_data_to_room(
user_id, room_id, account_data_type, body
)
Expand Down