feat(room): add JoinRequest subscriptions#4338
Conversation
5d8f036 to
f5d859b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4338 +/- ##
==========================================
- Coverage 85.31% 85.30% -0.01%
==========================================
Files 282 283 +1
Lines 31259 31422 +163
==========================================
+ Hits 26668 26805 +137
- Misses 4591 4617 +26 ☔ View full report in Codecov by Sentry. |
4b60abf to
e3cd7cc
Compare
04fef9c to
5068a0e
Compare
RequestToJoinRoom subscriptionsJoinRequest subscriptions
a9f12a4 to
2372394
Compare
|
I converted the PR back to draft, I'm trying to fix a test failure I can't reproduce locally 🫤 . |
bindings/matrix-sdk-ffi/src/room.rs
Outdated
|
|
||
| /// A listener for receiving new requests to a join a room. | ||
| #[matrix_sdk_ffi_macros::export(callback_interface)] | ||
| pub trait RequestsToJoinListener: Send + Sync { |
There was a problem hiding this comment.
rename also this to JoinRequestsListener
07cc01c to
2b9e330
Compare
bnjbvr
left a comment
There was a problem hiding this comment.
Thanks. I might need a bit of time and to take another look after the fundamental issue has been addressed, to understand a bit better the problem's requirements and the proposed solution.
I appreciate trying to make small commits to make the review easier, but I find that commits changing code that was introduced in previous commits doesn't quite help me, as a reviewer: I'm learning about new concept ABC in commit N, but then in commit N+P it's renamed to XYZ. Would it be possible to fold the commits in such a way that the new names are used from the start, instead?
Also, I haven't seen a test until the 5th commit 😨 Unit tests are not only useful to ensure high code coverage, but also to make sure that the functionality we're introducing is always tested (even if callers stop using it, after some time). So you get bonus points if whenever you introduce a new functionality/bugfix (public API), it gets tested in the same commit :) (I see one of the last commits does this perfectly, so it's a matter of aligning the rest of the commits from this PR.)
Keep up the good work 💪
| room_id.to_owned(), | ||
| value | ||
| .into_seen_join_requests() | ||
| .expect("Session data not a set of ignored join requests"), |
There was a problem hiding this comment.
It's actually s/ignored/seen/, I decided on 'seen' in the end but I forgot to change these error messages. Good catch!
| StateStoreDataKey::SeenRequestsToJoin(_) => self.serialize_value( | ||
| &value | ||
| .into_seen_join_requests() | ||
| .expect("Session data not a set of ignored requests to join"), |
| StateStoreDataKey::SeenRequestsToJoin(_) => self.serialize_value( | ||
| &value | ||
| .into_seen_join_requests() | ||
| .expect("Session data not a set of ignored requests to join"), |
crates/matrix-sdk/src/room/mod.rs
Outdated
| /// Subscribes to the set of requests to join that have been marked as | ||
| /// 'seen'. | ||
| pub async fn subscribe_to_seen_requests_to_join_ids( | ||
| &self, | ||
| ) -> Result<(HashSet<OwnedEventId>, impl Stream<Item = HashSet<OwnedEventId>>)> { | ||
| let current = self.get_seen_requests_to_join_ids().await?; | ||
| let subscriber = | ||
| self.seen_requests_to_join_ids.subscribe().map(|values| values.unwrap_or_default()); | ||
| Ok((current, subscriber)) | ||
| } |
There was a problem hiding this comment.
Unfortunately, this is false observability. seen_requests_to_join_ids exists in Room, which is created on-demand, e.g. in Client::get_room(), Client::rooms(). Given a RoomId, there might be multiple, different Room objects alive at the same time, all representing the room with that id.
As a result, if you do the following:
- create
Roomobject for room id XYZ - create a second
Roomobject for the same room id - call this method ^ on the second object
- set a seen_requests_to_join on the first object
Then the second Room object will NOT emit an update about the seen_requests_to_join emitted from the first object.
Solving this requires getting back to the drawing board, and figuring a better place where to put the seen_requests_to_join_ids set:
- the
BaseRoomakamatrix_sdk_base::Roomwould be a good candidate, as all the data it contains always links to the same identity. (It'd be worth having some kind ofBaseRoomInnerto make this clearer, but oh well, so many wrappers already.) So it'd be possible to include anArcin there for the set, and modify it from all theBaseRoom, thenRoominstances. - an alternative is to put this into
RoomInfo, which is already unique per room + observable 👀 If you've included this set in theRoomInfoin a subsequent patch, it might be worth doing it sooner rather than later. Otherwise, putting it inBaseRoommight be sufficient.
There was a problem hiding this comment.
the BaseRoom aka matrix_sdk_base::Room would be a good candidate
I might be wrong, but isn't there where it is at the moment? I think it just gives you the impression that it's in sdk::Room because it's using the Deref impl to access BaseRoom.
I have another question though: since this data lives in the store, the field gets populated the first time Room::get_seen_join_requests is called. Maybe there's a better way to handle this?
There was a problem hiding this comment.
I might be wrong, but isn't there where it is at the moment? I think it just gives you the impression that it's in sdk::Room because it's using the Defer impl to access BaseRoom.
oh my, absolute facepalm of a review, please ignore me then :'D
I have another question though: since this data lives in the store, the field gets populated the first time Room::get_seen_join_requests is called. Maybe there's a better way to handle this?
It's fine, I get it that it's an in-memory cache for data that lives in the store, so there's no way to get it sooner than that anyways (and it's good that it's filled lazily on demand too).
| /// A request to join a room with `knock` join rule. | ||
| #[derive(Debug, Clone)] | ||
| pub struct RequestToJoinRoom { | ||
| room: Arc<Room>, |
There was a problem hiding this comment.
(This pattern of using an Arc<Room> lets me think that you've ran into the Room identity issue I've spotted above. We usually don't store Room in Arc, because we shouldn't have to.)
There was a problem hiding this comment.
Nice, I think I got over-paranoid with not cloning stuff, so I'm reverting this to using a Room directly and cloning it as needed.
323ccca to
f205034
Compare
|
I hope this is now in a better shape to be reviewed. I also added a few extra tests. |
bnjbvr
left a comment
There was a problem hiding this comment.
Okay, it makes a lot more sense with the nicely cut commits, thanks a bunch for splitting them that way and adding tests per commit ❤️
I've got a few blocking comments below, and one overall comment about the approach:
It seems we could spare the existence of JoinRequest overall in the SDK, by emitting stream updates about the set of pending knock member events for that room. Then, at the FFI layer, we'd listen to that stream, and apply the extra is_seen bit based on a call to the most recent get_seen_john_requests(). It's also fine to have some kind of higher-level object at the FFI layer, that calls the Room methods to accept/kick/ban/decline a user's knock, if justified that it would avoid crossing the FFI boundary by doing more work at once.
Doing that would avoid introducing new high-level APIs in the SDK, that can be avoided by calling plain functions. It would mean there would be fewer ways to do the same thing, which has some appeal to me. We would have less public API surface, and thus fewer things to test (although I do really appreciate the new mocking endpoints, that I think we should keep in any case). It means a bit more simplicity for consumers of the library, because there are fewer things to look for, in the documentation.
How does that sound?
| custom: StdRwLock<HashMap<Vec<u8>, Vec<u8>>>, | ||
| send_queue_events: StdRwLock<BTreeMap<OwnedRoomId, Vec<QueuedRequest>>>, | ||
| dependent_send_queue_events: StdRwLock<BTreeMap<OwnedRoomId, Vec<DependentQueuedRequest>>>, | ||
| seen_requests_to_join: StdRwLock<BTreeMap<OwnedRoomId, HashSet<OwnedEventId>>>, |
There was a problem hiding this comment.
If you don't mind, it'd be great to wait for #4403 to land, as it removes the concept of an individual lock per field in the memory store; or maybe you could rebase over this PR?
There was a problem hiding this comment.
Sure, I can rebase.
| /// The event ids for seen request to join room events. | ||
| pub seen_join_request_ids: SharedObservable<Option<HashSet<OwnedEventId>>>, |
There was a problem hiding this comment.
Now that I understand a bit more what this is: this is the set of event ids, for m.room.member knocking events that should be pending.
As discussed privately: this is only growing over time, and unbounded in size, so we'll need, in a follow-up to empty it over time, as requests get handled.
I have two requests here:
- could we expand the comment, to be much more precise about what those events are: let's mention that they are room membership events, in the knocking state, maybe? "see request to join room events" is a bit hard to parse for me. One proposal would be:
The set of ids for room membership events in the knocking state, and that the user has already marked as seen, for instance. Feel free to use a different paragraph to detail what this is. - tied to this doc comment, I find the name a bit confusing, as it doesn't speak of event ids, or events, for that matter. Maybe we could rename this
seen_knock_event_ids, orseen_knock_event_id_set, even? - going from this renaming, I think it'd be then reasonable to rename
JoinRequest(and associated vocabulary) toKnockRequest.Joinmakes me think of aJoinedRoom, that is, a room joined by the current user, and this is not what we're talking about here. Would it make sense?
crates/matrix-sdk/src/room/mod.rs
Outdated
| if let Some(requests) = current_join_request_ids.as_ref() { | ||
| requests.clone() | ||
| } else { |
There was a problem hiding this comment.
style suggestion: to have one fewer indent level, could you return Ok(requests.clone()); here?
crates/matrix-sdk/src/room/mod.rs
Outdated
| /// Mark a list of requests to join the room as seen, given their state | ||
| /// event ids. | ||
| pub async fn mark_join_requests_as_seen(&self, event_ids: &[OwnedEventId]) -> Result<()> { |
There was a problem hiding this comment.
So, there's a bit of an issue here: nothing prevents you from saving an arbitrary event id:
- the event id may refer to an event that doesn't exist
- the event id may refer to an event that exists but isn't a
m.room.memberevent in the knocked state
This is exemplified by the test, which saves an event id that doesn't tie to an event.
I see a few ways of improving the API here:
- either have the method take a deserialized
SyncTimelineEvent, check that it's a room membership event with the expected knock state (return an error if it is not) - or take
RoomMembershipEventinstead ofevent_ids, and check they're all in the knocking state (return an error otherwise), - or generalize the feature as "a set of seen events, and it can be any kind of events". I suspect this wouldn't fare well with the rest of the PR, though…
There was a problem hiding this comment.
What if I still take the list of event ids and resolve the knock member events inside the method, failing if any of the provided event ids is not valid (doesn't exist or is not a knock member event) ?
There was a problem hiding this comment.
Yep, that could work too. It might be a bit suboptimal if the events haven't been cached yet, and we need to re-resolve them one way or another. Is there no way to pass the actual events here?
There was a problem hiding this comment.
There is, but it would mean having to find and deserialize the raw event to the right types in a couple of places instead of a single one. Another option could be to pass the KnockJoinRequest directly, maybe?
There was a problem hiding this comment.
Passing the KnockRequest could also work nicely, good idea!
There was a problem hiding this comment.
Ah damn, I just remembered why I did it this way: we'll need a 'mark all current knock requests as seen' method, and I was doing that by just passing the event ids of those current knock requests.
We could use the knock request instead, but it seems a bit of an overkill to pass dozens of items back and forth through the FFI layer just to get the internal event id. WDYT?
It might be a bit suboptimal if the events haven't been cached yet, and we need to re-resolve them one way or another.
Since these are state events and those should be persistently cached, if we're asked to mark a knock request we don't have yet from the client, that means something went really wrong, like: why would you want to allow the user to mark something they haven't received yet as seen? In that case, I'd just return an error saying we don't have the related event.
There was a problem hiding this comment.
Ah good point that they're state events, so don't require the event cache \o/
Fine to pass the event ids, and check they match some known knock requests. If they don't, I'm not sure if we'd prefer to filter them out (i.e. ignore them), or return an error indicating so to the caller.
There was a problem hiding this comment.
I was going to use the event ids but then I realised store.get_state_events_for_keys uses the state_key which is the user id, not the event id. So maybe passing the user id is the best way to handle it: you got a knock request, have the user id as the state key, pass that to mark_knock_requests_as_seen, if the state event is in knock state, we mark it as seen.
crates/matrix-sdk/src/room/mod.rs
Outdated
| /// Mark a list of requests to join the room as seen, given their state | ||
| /// event ids. | ||
| pub async fn mark_join_requests_as_seen(&self, event_ids: &[OwnedEventId]) -> Result<()> { | ||
| let mut current_seen_events = self.get_seen_join_request_ids().await?; |
There was a problem hiding this comment.
There's an await point between a read and a write, so there's a theoretical race where two callers may call the method at the same time, and one of them would end up overwriting what the other is adding. Is that a problem? If not, can you precise it's not a problem (and why) in the doc comment, please?
There was a problem hiding this comment.
You're right, I forgot I'm not dealing with the underlying observable with rw locks directly... I guess I'll have to add a Mutex somewhere in BaseRoom for this, maybe? Is there a better way to handle it?
There was a problem hiding this comment.
Could we move these methods to the underlying BaseRoom?
There was a problem hiding this comment.
I think so, yes.
There was a problem hiding this comment.
Once moved, should I add a Mutex to it? Or should I add some helper function to initialise the shared observable with the contents in the Store and return it wrapped in a WriteGuard, then reuse that for both getting the ids and modifying them?
|
|
||
| // We receive an initial request to join from Alice | ||
| let initial = assert_next_with_timeout!(stream, 100); | ||
| assert!(!initial.is_empty()); |
There was a problem hiding this comment.
instead of checking !is_empty(), if you check the expected .len(), you'll be sure there's exactly 1 request, and no more.
|
|
||
| // Now it's received again as seen | ||
| let seen = assert_next_with_timeout!(stream, 100); | ||
| assert!(!seen.is_empty()); |
|
|
||
| // If we then receive a new member event for Alice that's not 'knock' | ||
| let alice_join_event_id = event_id!("$alice-join:b.c"); | ||
| let joined_room_builder = JoinedRoomBuilder::new(room_id).add_state_bulk(vec![f |
There was a problem hiding this comment.
if you use add_state_event, you shouldn't need the vec![]
There was a problem hiding this comment.
add_state_event only allows you to pass a StateTestEvent instance. I could use StateTestEvent::Custom, but then I need to use a hardcoded json.
|
|
||
| // The requests to join are now empty | ||
| let updated_requests = assert_next_with_timeout!(stream, 100); | ||
| assert!(updated_requests.is_empty()); |
There was a problem hiding this comment.
Can you check at the end of this test that the stream is empty?
crates/matrix-sdk/src/room/mod.rs
Outdated
|
|
||
| /// Subscribes to the set of requests to join that have been marked as | ||
| /// 'seen'. | ||
| pub async fn subscribe_to_seen_join_request_ids( |
There was a problem hiding this comment.
I think this isn't tested in isolation. Maybe we can inline this, and not make it public?
I don't think that would work when you mark a request as seen, since you wouldn't have anything to tell you there are new 'seen' knock requests. You'd have to build that stream of seen ids in the FFI layer if possible, or expose it from the SDK, then combine it with the knock request stream in the FFI layer, which seems like maybe too much logic in the FFI layer and we'd have no way to test it. |
|
Fair enough! So maybe the original stream can keep on listening to changes to seen, but yield the knock events / event IDs themselves, rather than |
|
Sorry to turn my head on the necessity of having |
f205034 to
7a158c2
Compare
|
I think I added fixes for all the comments above in the 3 new commits uploaded. Sorry that they're not atomic, after the renamings it was quite difficult to upload only partial changes in commits that can be built independently. In any case, I plan to rebase again and re-organize everything before merging. |
| /// Get this value if it is the data for the ignored join requests. | ||
| pub fn into_seen_join_requests(self) -> Option<HashSet<OwnedEventId>> { | ||
| as_variant!(self, Self::SeenJoinRequests) | ||
| pub fn into_seen_join_requests(self) -> Option<BTreeMap<OwnedEventId, OwnedUserId>> { |
There was a problem hiding this comment.
Let's rename this to into_seen_knock_requests
| /// A list of requests to join in a room marked as seen. | ||
| SeenJoinRequests(&'a RoomId), | ||
| SeenKnockRequests(&'a RoomId), |
There was a problem hiding this comment.
Let's update the doc comment here.
| StateStoreDataKey::SeenKnockRequests(_) => self.serialize_value( | ||
| &value | ||
| .into_seen_join_requests() | ||
| .expect("Session data is not a set of seen join request ids"), |
There was a problem hiding this comment.
Let's update seen join request to seen knock request here.
| StateStoreDataKey::SeenKnockRequests(_) => self.serialize_value( | ||
| &value | ||
| .into_seen_join_requests() | ||
| .expect("Session data is not a set of seen join request ids"), |
| Ok(guard.clone().unwrap_or_default()) | ||
| } | ||
|
|
||
| async fn load_latest_knock_request_ids( |
There was a problem hiding this comment.
Let's add a doc-comment here. Does the latest carry some meaningful information? Do we mean the "active" knock requests, i.e. those that have not been acted upon?
crates/matrix-sdk/src/room/mod.rs
Outdated
| } | ||
|
|
||
| /// Helper to requests to join this `Room`. | ||
| /// Helper to subscribe to knock requests in this `Room`. |
crates/matrix-sdk/src/room/mod.rs
Outdated
| MembershipChange::KnockAccepted | | ||
| MembershipChange::KnockDenied | | ||
| MembershipChange::KnockRetracted => { | ||
| match this.get_current_join_requests(&seen_ids).await { |
There was a problem hiding this comment.
Instead of duplicating the match this.get_current_join_requests(,..) (which ought to be renamed to s/join/knock/), could we have a bool emit, and then, if emit { match this.get_current_join_request() { ... }} to avoid the code duplication?
| } | ||
| Some(new_seen_ids) = seen_request_ids_stream.next() => { |
There was a problem hiding this comment.
uber nit: please add new lines between match arm variants
crates/matrix-sdk/src/room/mod.rs
Outdated
| } | ||
| } else if has_new_seen_ids || has_missing_room_members { | ||
| // If seen requests have changed or we have missing room members, | ||
| // we need to recalculate all the requests to join | ||
| match this.clone().get_current_join_requests(&seen_ids).await { | ||
| Ok(requests) => yield requests, | ||
| Err(e) => { | ||
| warn!("Failed to get updated requests to join on seen ids changed: {e:?}") | ||
| Some(room_info) = room_info_stream.next() => { |
| } | ||
| } | ||
| } | ||
| else => break, |
There was a problem hiding this comment.
What does this do? Can you add a code comment?
This will allow us to keep track of which join room requests are marked as 'seen' by the current user and return them as such. Also, add some methods to `Room` to mark new join requests as seen and to get the current ids for the seen join requests.
This struct is an abstraction over a room member or state event with knock membership.
This subscription will combine 3 streams: one notifying the members in the room have changed, another notifying the seen join requests have changed, and finally a third one notifying when the room members are no longer synced. With this info we can track when we need to generate a new list of join requests to be emitted so the client can always have an up to date list.
…:mark_knock_requests_as_seen` thread safe and pass `user_ids` instead of `event_ids`: the user ids will be used to get the related member state events and they'll only be marked as seen if they're in a knock state. Also, add extra checks to the integration tests.
7a158c2 to
8ff5f33
Compare
What
This PR adds support for listening to changes in requests to join a room (aka: knock memberships): to do this, we listen to new room member events to trigger the emission of a new list of join requests, as well as to the
RoomInfo::members_syncedflag to understand if we need to reload the room members because a gappy sync happenedSince having too many/old requests to join to display can be an UX problem, I added the concept of a 'seen' join request, and an internal set of join request ids (these referencing the knock membership event), so we can mark some of these join requests as seen and filter them out in the clients at will. Also, when a join request is marked as seen, a new list of join requests is emitted with the updated value.
Why
When a user of a knockable room has the power level to invite to or kick members from the room, there should be a way to display the incoming join requests in a live manner so this user can know about them and act on them ASAP, instead of having to manually reload the room member list when performing some UI interaction: that's why we need a way to subscribe to these kinds of changes in a live manner.
However, having some component that persistently displays the latest requests to join and having no way to dismiss them can make the room UI difficult to use in the clients, so there should be some way to ignore the displayed requests, or mark them as 'seen' so they can still be reviewed but they can be hidden from the user in that case.
This PR should be reviewed commit-by-commit, since it's fairly large.
Signed-off-by: