fixup! feat(room_preview): Use room directory search as another data …

…source

crates/matrix-sdk/src/room_preview.rs

@@ -18,6 +18,7 @@
 //! This offers a few capabilities for previewing the content of the room as
 //! well.
 
+use futures_util::future::join_all;
 use matrix_sdk_base::{RoomInfo, RoomState};
 use ruma::{
     api::client::{membership::joined_members, state::get_state_events},
@@ -30,10 +31,7 @@ use ruma::{
 use tokio::try_join;
 use tracing::{instrument, warn};
 
-use crate::{
-    room_directory_search::{RoomDescription, RoomDirectorySearch},
-    Client, Room,
-};
+use crate::{room_directory_search::RoomDirectorySearch, Client, Room};
 
 /// The preview of a room, be it invited/joined/left, or not.
 #[derive(Debug, Clone)]
@@ -123,7 +121,7 @@ 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 async fn from_known(room: &Room) -> Self {
+    pub(crate) async fn from_known(room: &Room) -> Self {
         let is_direct = room.is_direct().await.ok();
 
         Self::from_room_info(
@@ -173,11 +171,53 @@ impl RoomPreview {
         room_or_alias_id: &RoomOrAliasId,
         via: Vec<OwnedServerName>,
     ) -> crate::Result<Self> {
-        let mut directory_search = RoomDirectorySearch::new(client.clone());
+        async fn search_homeserver(
+            client: Client,
+            filter: Option<String>,
+            batch_size: u32,
+            server: Option<OwnedServerName>,
+            expected_room_id: &RoomId,
+        ) -> crate::Result<RoomPreview> {
+            let mut directory_search = RoomDirectorySearch::new(client);
+            directory_search.search(filter, batch_size, server).await?;
+
+            let (results, _) = directory_search.results();
+
+            for room_description in results {
+                // Iterate until we find a room description with a matching room id
+                if room_description.room_id != expected_room_id {
+                    continue;
+                }
+                return Ok(RoomPreview {
+                    room_id: room_description.room_id,
+                    canonical_alias: room_description.alias,
+                    name: room_description.name,
+                    topic: room_description.topic,
+                    avatar_url: room_description.avatar_url,
+                    num_joined_members: room_description.joined_members,
+                    num_active_members: None,
+                    // Assume it's a room
+                    room_type: None,
+                    join_rule: match room_description.join_rule {
+                        PublicRoomJoinRule::Public => SpaceRoomJoinRule::Public,
+                        PublicRoomJoinRule::Knock => SpaceRoomJoinRule::Knock,
+                        PublicRoomJoinRule::_Custom(rule) => SpaceRoomJoinRule::_Custom(rule),
+                        _ => {
+                            panic!("Unexpected PublicRoomJoinRule {:?}", room_description.join_rule)
+                        }
+                    },
+                    is_world_readable: room_description.is_world_readable,
+                    state: None,
+                    is_direct: None,
+                });
+            }
+
+            Err(crate::Error::InsufficientData)
+        }
 
         // Get either the room alias or the room id without the leading identifier char
         let search_term = if room_or_alias_id.is_room_alias_id() {
-            Some(room_or_alias_id.to_string().chars().skip(1).collect::<String>())
+            Some(room_or_alias_id.as_str()[1..].to_owned())
         } else {
             None
         };
@@ -186,29 +226,28 @@ impl RoomPreview {
         // the first 100 results and try to find the current room #YOLO
         let batch_size = if search_term.is_some() { 20 } else { 100 };
 
-        if via.is_empty() {
+        let futures = if via.is_empty() {
             // Just search in the current homeserver
-            let _ = directory_search.search(search_term, batch_size, None).await;
+            vec![search_homeserver(client.clone(), search_term, batch_size, None, room_id)]
         } else {
+            let mut futures = Vec::new();
             // Iterate all servers until one returns some results
             for server in via {
-                if directory_search.loaded_pages() > 0 {
-                    // If we previously got results don't search anymore
-                    break;
-                }
-
-                let _ =
-                    directory_search.search(search_term.clone(), batch_size, Some(server)).await;
+                futures.push(search_homeserver(
+                    client.clone(),
+                    search_term.clone(),
+                    batch_size,
+                    Some(server),
+                    room_id,
+                ));
             }
-        }
+            futures
+        };
 
-        let (values, _) = directory_search.results();
-        while let Some(room_description) = values.iter().next() {
-            // Iterate until we find a room description with a matching room id
-            if room_description.room_id != room_id {
-                continue;
-            }
-            return Ok(RoomPreview::from_room_description(room_description.to_owned()));
+        let joined_results = join_all(futures).await;
+
+        if let Some(preview) = joined_results.into_iter().flatten().next() {
+            return Ok(preview);
         }
 
         Err(crate::Error::InsufficientData)
@@ -317,31 +356,4 @@ impl RoomPreview {
             state,
         ))
     }
-
-    pub(crate) fn from_room_description(room_description: RoomDescription) -> Self {
-        RoomPreview {
-            room_id: room_description.room_id,
-            canonical_alias: room_description.alias,
-            name: room_description.name,
-            topic: room_description.topic,
-            avatar_url: room_description.avatar_url,
-            num_joined_members: room_description.joined_members,
-            num_active_members: None,
-            // Assume it's a room
-            room_type: None,
-            join_rule: space_rule_from_public_rule(room_description.join_rule),
-            is_world_readable: room_description.is_world_readable,
-            state: None,
-            is_direct: None,
-        }
-    }
-}
-
-fn space_rule_from_public_rule(join_rule: PublicRoomJoinRule) -> SpaceRoomJoinRule {
-    match join_rule {
-        PublicRoomJoinRule::Public => SpaceRoomJoinRule::Public,
-        PublicRoomJoinRule::Knock => SpaceRoomJoinRule::Knock,
-        PublicRoomJoinRule::_Custom(rule) => SpaceRoomJoinRule::_Custom(rule),
-        _ => panic!("Unexpected PublicRoomJoinRule {:?}", join_rule),
-    }
 }

crates/matrix-sdk/src/test_utils/mocks.rs

@@ -460,7 +460,43 @@ impl MatrixMockServer {
         MockEndpoint { mock, server: &self.server, endpoint: CreateRoomAliasEndpoint }
     }
 
-    /// Create a prebuilt mock for creating room aliases.
+    /// Create a prebuilt mock for listing public rooms.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// #
+    /// tokio_test::block_on(async {
+    /// use js_int::uint;
+    /// use ruma::directory::PublicRoomsChunkInit;
+    /// use matrix_sdk::room_directory_search::RoomDirectorySearch;
+    /// use matrix_sdk::{
+    ///     ruma::{event_id, room_id},
+    ///     test_utils::mocks::MatrixMockServer,
+    /// };
+    /// let mock_server = MatrixMockServer::new().await;
+    /// let client = mock_server.client_builder().build().await;
+    /// let event_id = event_id!("$id");
+    /// let room_id = room_id!("!room_id:localhost");
+    ///
+    /// let chunk = vec![PublicRoomsChunkInit {
+    ///     num_joined_members: uint!(0),
+    ///     room_id: room_id.to_owned(),
+    ///     world_readable: true,
+    ///     guest_can_join: true,
+    /// }.into()];
+    ///
+    /// mock_server.mock_public_rooms().ok(chunk, None, None, Some(20)).mock_once().mount().await;
+    /// let mut room_directory_search = RoomDirectorySearch::new(client);
+    ///
+    /// room_directory_search.search(Some("some-alias".to_owned()), 100, None)
+    ///     .await
+    ///     .expect("Room directory search failed");
+    ///
+    /// let (results, _) = room_directory_search.results();
+    /// assert_eq!(results.len(), 1);
+    /// # assert_eq!(results.get(0).unwrap().room_id, room_id.to_owned());
+    /// ```
     pub fn mock_public_rooms(&self) -> MockEndpoint<'_, PublicRoomsEndpoint> {
         let mock = Mock::given(method("POST")).and(path_regex(r"/_matrix/client/v3/publicRooms"));
         MockEndpoint { mock, server: &self.server, endpoint: PublicRoomsEndpoint }

testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs

@@ -40,7 +40,10 @@ use matrix_sdk::{
     test_utils::{logged_in_client_with_server, mocks::MatrixMockServer},
     Client, RoomInfo, RoomMemberships, RoomState, SlidingSyncList, SlidingSyncMode,
 };
-use matrix_sdk_base::{ruma::room_alias_id, sliding_sync::http};
+use matrix_sdk_base::{
+    ruma::{owned_room_id, room_alias_id},
+    sliding_sync::http,
+};
 use matrix_sdk_test::async_test;
 use matrix_sdk_ui::{
     room_list_service::filters::new_filter_all, sync_service::SyncService, timeline::RoomExt,
@@ -1153,6 +1156,7 @@ async fn test_room_preview_with_room_directory_search_and_room_alias_only_in_sev
     let via_map = BTreeMap::from_iter(vec![
         (
             via_1.to_owned(),
+            // The actual room we want
             vec![PublicRoomsChunkInit {
                 num_joined_members: uint!(0),
                 room_id: expected_room_id.to_owned(),
@@ -1161,14 +1165,23 @@ async fn test_room_preview_with_room_directory_search_and_room_alias_only_in_sev
             }
             .into()],
         ),
-        (via_2.to_owned(), Vec::new()),
+        (
+            via_2.to_owned(),
+            // Some other room
+            vec![PublicRoomsChunkInit {
+                num_joined_members: uint!(1),
+                room_id: owned_room_id!("!some-other-room:matrix.org"),
+                world_readable: true,
+                guest_can_join: true,
+            }
+            .into()],
+        ),
     ]);
     server
         .mock_public_rooms()
         .with_via_params(via_map)
-        // Called only once as the first response returns the room, so no need to check other
-        // homeservers
-        .expect(1)
+        // Expect this to be called once for every server in the `via_map`
+        .expect(2)
         .mount()
         .await;