From 6b0987385e2ce055920d198b59c61201b8e2fa5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Thu, 21 Nov 2024 17:01:50 +0100 Subject: [PATCH] refactor(room_preview): make `RoomPreview` use the local known data only for joined rooms When instantiating a room preview, previously it would try to just check if the room exists locally either as joined, invited, knocked, left, etc., and then retrieve the info we cached about it. While this seems fine for most cases, it turns out for non-joined rooms, the info we have locally will **always** be the one we received when the invite/knock/leave action took place and it'll never be updated, so we may have the case where we knock into a room, never receive a response, someone changes the join rule of the room to something else and we'll think about this room as a 'request to join' room until we clear the local cache. To prevent that, we can only use the local data for joined rooms, which are constantly updated, and try to use the room summary API and other fallbacks for the rest, even if they're rooms known to us. --- crates/matrix-sdk/src/client/mod.rs | 14 ++++++++--- crates/matrix-sdk/src/room_preview.rs | 7 +++--- .../tests/integration/room_preview.rs | 25 +++++++++++++++++-- 3 files changed, 36 insertions(+), 10 deletions(-) diff --git a/crates/matrix-sdk/src/client/mod.rs b/crates/matrix-sdk/src/client/mod.rs index 9be1fcd7b..a16603c02 100644 --- a/crates/matrix-sdk/src/client/mod.rs +++ b/crates/matrix-sdk/src/client/mod.rs @@ -1142,8 +1142,8 @@ impl Client { self.base_client().get_room(room_id).map(|room| Room::new(self.clone(), room)) } - /// Gets the preview of a room, whether the current user knows it (because - /// they've joined/left/been invited to it) or not. + /// Gets the preview of a room, whether the current user has joined it or + /// not. pub async fn get_room_preview( &self, room_or_alias_id: &RoomOrAliasId, @@ -1155,10 +1155,16 @@ impl Client { }; if let Some(room) = self.get_room(&room_id) { - return Ok(RoomPreview::from_known(&room).await); + // The cached data can only be trusted if the room is joined: for invite and + // knock rooms, no updates will be received for the rooms after the invite/knock + // action took place so we may have very out to date data for important fields + // such as `join_rule` + if room.state() == RoomState::Joined { + return Ok(RoomPreview::from_joined(&room).await); + } } - RoomPreview::from_unknown(self, room_id, room_or_alias_id, via).await + RoomPreview::from_not_joined(self, room_id, room_or_alias_id, via).await } /// Resolve a room alias to a room id and a list of servers which know diff --git a/crates/matrix-sdk/src/room_preview.rs b/crates/matrix-sdk/src/room_preview.rs index 9238b9eeb..198d335ef 100644 --- a/crates/matrix-sdk/src/room_preview.rs +++ b/crates/matrix-sdk/src/room_preview.rs @@ -124,9 +124,8 @@ impl RoomPreview { } } - /// Create a room preview from a known room (i.e. one we've been invited to, - /// we've joined or we've left). - pub(crate) async fn from_known(room: &Room) -> Self { + /// Create a room preview from a known room we've joined. + pub(crate) async fn from_joined(room: &Room) -> Self { let is_direct = room.is_direct().await.ok(); let display_name = room.compute_display_name().await.ok().map(|name| name.to_string()); @@ -142,7 +141,7 @@ impl RoomPreview { } #[instrument(skip(client))] - pub(crate) async fn from_unknown( + pub(crate) async fn from_not_joined( client: &Client, room_id: OwnedRoomId, room_or_alias_id: &RoomOrAliasId, diff --git a/crates/matrix-sdk/tests/integration/room_preview.rs b/crates/matrix-sdk/tests/integration/room_preview.rs index 1d78439c1..85e887e2e 100644 --- a/crates/matrix-sdk/tests/integration/room_preview.rs +++ b/crates/matrix-sdk/tests/integration/room_preview.rs @@ -9,7 +9,9 @@ use matrix_sdk_test::{ }; #[cfg(feature = "experimental-sliding-sync")] use ruma::{api::client::sync::sync_events::v5::response::Hero, assign}; -use ruma::{owned_user_id, room_id, space::SpaceRoomJoinRule, RoomId}; +use ruma::{ + events::room::member::MembershipState, owned_user_id, room_id, space::SpaceRoomJoinRule, RoomId, +}; use serde_json::json; use wiremock::{ matchers::{header, method, path_regex}, @@ -30,6 +32,14 @@ async fn test_room_preview_leave_invited() { client.sync_once(SyncSettings::default()).await.unwrap(); server.reset().await; + mock_unknown_summary( + room_id, + None, + SpaceRoomJoinRule::Knock, + Some(MembershipState::Invite), + &server, + ) + .await; mock_leave(room_id, &server).await; let room_preview = client.get_room_preview(room_id.into(), Vec::new()).await.unwrap(); @@ -52,6 +62,14 @@ async fn test_room_preview_leave_knocked() { client.sync_once(SyncSettings::default()).await.unwrap(); server.reset().await; + mock_unknown_summary( + room_id, + None, + SpaceRoomJoinRule::Knock, + Some(MembershipState::Knock), + &server, + ) + .await; mock_leave(room_id, &server).await; let room_preview = client.get_room_preview(room_id.into(), Vec::new()).await.unwrap(); @@ -91,7 +109,7 @@ async fn test_room_preview_leave_unknown_room_fails() { let (client, server) = logged_in_client_with_server().await; let room_id = room_id!("!room:localhost"); - mock_unknown_summary(room_id, None, SpaceRoomJoinRule::Knock, &server).await; + mock_unknown_summary(room_id, None, SpaceRoomJoinRule::Knock, None, &server).await; let room_preview = client.get_room_preview(room_id.into(), Vec::new()).await.unwrap(); assert!(room_preview.state.is_none()); @@ -142,6 +160,7 @@ async fn mock_unknown_summary( room_id: &RoomId, alias: Option, join_rule: SpaceRoomJoinRule, + membership: Option, server: &MockServer, ) { let body = if let Some(alias) = alias { @@ -152,6 +171,7 @@ async fn mock_unknown_summary( "num_joined_members": 1, "world_readable": true, "join_rule": join_rule, + "membership": membership, }) } else { json!({ @@ -160,6 +180,7 @@ async fn mock_unknown_summary( "num_joined_members": 1, "world_readable": true, "join_rule": join_rule, + "membership": membership, }) }; Mock::given(method("GET"))