This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add experimental support for MSC3391: deleting account data #14714
Merged
Merged
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
8173de4
Add experimental features flag
anoadragon453 8cd633f
Enable Complement tests for MSC3391
anoadragon453 d338a00
Add replication methods for removing account data
anoadragon453 36d3bd2
Add storage functions for deleting user/room account data
anoadragon453 dcfacc8
Add handler, servlet methods for deleting user/room account data
anoadragon453 ef562a2
Allow deleting account data by PUT'ing with empty content
anoadragon453 0f81122
Return a 404 if an account data type is empty
anoadragon453 2937973
Prevent deleted account data items appearing in initial syncs
anoadragon453 0520e4c
changelog
anoadragon453 80e991f
Simplify txn.rowcount return check
anoadragon453 3c62cd1
Remove unnecessary self.clock assignments in replication endpoints
anoadragon453 36eab04
Remove unnecessary self.store's in unstable account data servlets
anoadragon453 2aa8986
Document why we're not using simple_update
anoadragon453 ccd4225
Document the return value of simple_select_list(_txn)
anoadragon453 554dd36
Convert queries in get_account_data_for_user_txn to raw SQL
anoadragon453 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Add experimental support for [MSC3391](https://github.com/matrix-org/matrix-spec-proposals/pull/3391) (removing account data). | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The advantage of using
{}
is that clients will then automatically sync the account data type with a value of{}
, which as per MSC3391 implies to the client that the account data entry was deleted. It also comes with the benefit that older clients, which set account data content to{}
to "delete" it, would actually start deleting items.We could use
NULL
, but I don't see any benefit other than it being a more commonplace value for marking an item as deleted. Also note that thecontent
column ofaccount_data
androom_account_data
is currently marked asNOT NULL
.The reason for all of the faff with the
account_data_undelivered_deletes
table is the need to keep track of when we can remove the relevant row from(room_)account_data
- after all devices of a user has seen the change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we could still provide that to clients if we stored it as
NULL
by coercing it when passing back to clients.The main benefit is that we know whether the data was actually deleted or just cleared. I'm not sure if this difference actually matters or not though. Maybe we don't want to actually delete rows if the data is just cleared and not deleted?
I believe making a column non-null is not difficult: I don't think this would be a blocker.
Hmm, OK. I don't think I'm following all of the changes being made then. This PR Is only the first of a few which will make these changes, I guess I'm having trouble lining up what parts of #14244 are in this PR and what is a future PR. That has a bunch of things ticked off which I don't see in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, and could be beneficial in a somewhat niche use case. The plan #14244 talks about involves creating an entry in a
account_data_undelivered_deletes
table when a delete occurs. That has the side-effect of also letting us know which rows were deleted vs. set to{}
.After this PR, we don't need to differentiate between delete account data entries and those set to
{}
- as we should consider them the same thing. In either case, clients should see the entry as deleted. And we'll clear rows from the database via the information stored in theaccount_data_undelivered_deletes
table.Without the
account_data_undelivered_deletes
table (or some other method to track which devices still need to be informed about an account data entry being deleted) we can't delete rows from(room_)account_data
anyway.And in the case MSC3391 ends up not being accepted, leaving the rows as
{}
instead of NULL is probably more desirable.