Add experimental support for MSC3391: deleting account data

[anoadragon453]

This PR adds experimental support for MSC3391. It implements the "Base behaviour" section of #14244. This means that while account data is deleted from the client's perspective, entries are not removed from the database. Doing so (the "Permanently deleting items from the account data stream" section of the same issue) will come in a later PR - this one was big enough!

Experimental support for deleting account data can be enabled by setting experimental_features.msc3391_enabled to true in your homeserver config.

Paired with Complement tests at matrix-org/complement#574

Reviewers are encouraged to read MSC3391, then the implementation plan in #14244, before then reviewing commit-by-commit.

[clokep]

Code looks plausible over all. I'm a little skeptical about not deleting the data though (and filling it in with {}). #14244 doesn't really seem to address this approach at all and seems to document a completely different approach?

Would using null as the value for deleted items let us keep track of the stream ID while differentiating "empty" from "deleted"?

[clokep]

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

[anoadragon453]

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.

[clokep]

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".

[anoadragon453]

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.

[anoadragon453]

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.

[anoadragon453]

Code looks plausible over all. I'm a little skeptical about not deleting the data though (and filling it in with {}). #14244 doesn't really seem to address this approach at all and seems to document a completely different approach?

Would using null as the value for deleted items let us keep track of the stream ID while differentiating "empty" from "deleted"?

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 the content column of account_data and room_account_data is currently marked as NOT 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.

[clokep]

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.

But we could still provide that to clients if we stored it as NULL by coercing it when passing back to clients.

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.

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?

Also note that the content column of account_data and room_account_data is currently marked as NOT NULL.

I believe making a column non-null is not difficult: I don't think this would be a blocker.

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.

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?

[anoadragon453]

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?

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 the account_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.

[clokep]

We talked a bit more about this https://matrix.to/#/!vcyiEtMVHIhWXcJAfl:sw1v.org/$PtqcJXBdLvCtt32AsXkZvYaEKvo6QRl3yYrsvUzSnwA?via=matrix.org&via=element.io&via=envs.net