MSC2437: Store tagged events in Room Account Data #2437
Conversation
|
relates to Riot issues https://github.com/vector-im/riot-web/issues/2938 and https://github.com/vector-im/riot-web/issues/5649 |
|
Maybe it would make sense to extend the mechanism a little to be able to solve Personal ('pet name') room name for chats by allowing to tag the room name state event with a custom name, or even the membership events of other users to be able to rename them? 🤔 |
|
@florianjacob I would prefer to not consider this point here. This should be handled by another proposal. |
Co-Authored-By: Bruno Windels <brunow@matrix.org>
|
|
||
| The name of a tag MUST NOT exceed 255 bytes. | ||
|
|
||
| The tag namespace is defined as follows: |
There was a problem hiding this comment.
Thank you for using the same tagging scheme as other tags :D
Reusability ftw
| DELETE /_matrix/client/r0/user/{userId}/rooms/{roomId}/tags/{tag} | ||
| ``` | ||
|
|
||
| Should we consider this approach for any new event type? |
There was a problem hiding this comment.
A guiding principle here is whether or not the schema is complicated enough to need an API on top of it. Something like pinned events doesn't get its own API because it's ~3 lines of code in most languages to do the operation, and the risk of overwriting other people's changes is largely a social problem and not a technical one (tell your friend to stop doing things so you can do it instead).
For this particular API, it's a complicated operation to add/remove things to the account data so a set of endpoints could be useful. The overlapping writes will still be a problem, but at least we can make it easier on client authors.
I'd suggest something like the following:
GET /_matrix/client/r0/user/{userId}/rooms/{roomId}/tagged_events
PUT /_matrix/client/r0/user/{userId}/rooms/{roomId}/tagged_events/{eventId}
Takes a JSON body of {"tag": "m.favourite", "keywords": []} - the server can work out the rest of the fields.
DELETE /_matrix/client/r0/user/{userId}/rooms/{roomId}/tagged_events/{eventId}
DELETE /_matrix/client/r0/user/{userId}/rooms/{roomId}
For easier implementations of "clear all tagged events"
There would be an open question of whether or not we need an API to get all events tagged as a certain key, but I think the /tagged_events API proposed here would be fine enough for clients to locally filter.
turt2live
added
the
needs-implementation
There was a problem hiding this comment.
I really like the approach, was pondering something similar in Quotient but never got to trying it out. The tags system looks especially powerful here. Before moving with this further, it would be great to have some extension to the Client Config (aka account_data) section of the spec, so that APIs there could deal with lists and not just data atoms; but it's rather a "nice to have" and not a must.
| account data value, something like: | ||
|
|
||
| ``` | ||
| PUT /_matrix/client/r0/user/{userId}/rooms/{roomId}/account_data/<type>/append |
There was a problem hiding this comment.
The RESTful approach would bring something like:
| PUT /_matrix/client/r0/user/{userId}/rooms/{roomId}/account_data/<type>/append | |
| POST /_matrix/client/r0/user/{userId}/rooms/{roomId}/account_data/<type> |
| For the `DELETE` method, you'd need some kind of `ETag` value to pass as an | ||
| `If-Match` header so avoid deleting an indexed that is out of date because of | ||
| other clients deleting an index as well. Perhaps the server could add some kind | ||
| of unique tag to the account data value when it is updated. |
There was a problem hiding this comment.
I don't think deleting by index is generally a good idea, exactly because it's very prone to races. Some kind of a locally-unique identifier would be much better here (and then, again pretty much in line with REST principles it could be DELETE /.../account_data/<type>/<element_id>.
I personally like this approach more than devising API endpoints for particular account data types. In particular, it's very well extensible to custom account data (and it's not too much of a hassle to manage where these generic endpoints can/cannot be used in m. namespace). I believe that it would be a great subject for a separate MSC though.
| The event information are given under the following fields: | ||
|
|
||
| * `keywords` (`[string]`) - A list of words which should describe the event, | ||
| useful for searching or viewing favourite events without decrypting them. |
There was a problem hiding this comment.
It's worth being clear here that the words in the list are user-defined and not something the spec even intends to go into (I don't think it makes sense to spec some "pre-defined keywords").
|
|
||
| This is a known limitation for any new event type added to the `account_data` | ||
| section of a room, or to the top-level `account_data`. For example, we already | ||
| suffer from it to handle the `m.direct` event type. |
There was a problem hiding this comment.
A leftover comment from the previous review - there is an important difference between m.direct and this kind of data:
m.directis defined in global account data, which greatly increases the race scope.m.directcan be updated by running clients with no user interaction - e.g. when a DM is joined from another side, a client may setm.directat exactly the same time the inviter updatedm.directfor another room - which make the race condition uncontrollable by the user.
In contrast, the tagged events here are defined on the room level; and all changes to them originate from the user interaction (at least that's my impression from the MSC). This means that one user has to change the list on two different clients at once, which is certainly possible but not very practical; and the mitigation for that boils down to "just don't edit your tagged events on two phones at the same second".
This is why I think that the existing generic account_data API should usually be not too problematic for the purpose, and the robust list data management API is only a nice to have addition.
| of unique tag to the account data value when it is updated. | ||
|
|
||
| We mentioned here the discussion about this limitation to keep it in our mind. | ||
| This should be the subject of another proposal. |
There was a problem hiding this comment.
Also - can I have an unstable prefix for this?
| The event information don't seem very useful. We have provided some of them as | ||
| example. | ||
|
|
||
| ## Limitation |
There was a problem hiding this comment.
All tags and all tag types are retrieved at once. For large numbers of tags it may make sense to load only specific types of tags or load them incrementally. For example if I have been using Matrix for a decade I don't want to repeatedly transfer 1000 "remembered" messages or similar. This could become a real issue with custom tags as the user may be using them to categorize incoming messages and the number of tags may grow very quickly.
I think that it is probably best to use a different storage system that can optimize common use cases.
- Tagging a message should be
O(1). - Untagging a message should be
O(1). - Fetching the most recent
ntags of a given type should beO(n) - Fetching messages with a given tag in the room should be
O(n). - Syncing should be
O(added+removed).
Currently all of these are O(total number of tags) which seems unacceptable. The exception is syncing when no tags have been changed in which case it is O(1).
| defined in the `m.*` namespace: | ||
|
|
||
| * `m.favourite`: The user's favourite events in the room. | ||
| * `m.hidden`: The events that the user wants to hide from the room history. |
There was a problem hiding this comment.
Wouldn't it make more sense to attach m.hidden to the event itself, so that it is only fetched if the event is? There is no sense transferring a large number of hidden tags on every update.
giomfo
removed
the
needs-implementation
|
This MSC has been implemented on mobile sdk level (ios, android) @turt2live, what is the right labels to assign to it in order to get a final review and/or be able to merge it? |
turt2live
added
the
needs-implementation
|
@giomfo the implementations need to be linked in the PR description please. From there it's a message in the SCT office for us to consider it for FCP. |
| The event information don't seem very useful. We have provided some of them as | ||
| example. | ||
|
|
||
| ## Limitation |
There was a problem hiding this comment.
Some more ideas to address the race, somewhat applicable to account data in general:
Encode the event id in the account data type like m.tagged_events.<event id>. We don't currently have the habit of encoding ids in the type though and likely need to escape . in the event id. We'd also need to extend the GET /account_data/ endpoint to accept a match by prefix rather than exact match for the type so you can list m.tagged_events.*.
Another way would be to do something akin to state events, add some key property which you can then specify when requesting GET /account_data/<type>/<key>. For m.tagged_events the key would be the event id. For m.direct it could be the user id of the DM target.
Rendered
Mobile Client implementations: