From bc9192f81828d64fcfaf420cd229a0fe42980d8b Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Tue, 24 Jun 2025 14:29:51 +0200 Subject: [PATCH] refactor!(sdk): make the join_rule and related getters optional The join rule state event can be missing from a room state. In this case, it's an API footgun to return a default value; instead, we should return none and let the caller decide what to do with missing information. --- bindings/matrix-sdk-ffi/src/notification.rs | 4 +--- bindings/matrix-sdk-ffi/src/room/mod.rs | 5 ++++- bindings/matrix-sdk-ffi/src/room/room_info.rs | 21 +++++++++++++------ bindings/matrix-sdk-ffi/src/room_preview.rs | 11 +++++----- crates/matrix-sdk-base/src/room/mod.rs | 12 ++++++----- crates/matrix-sdk-base/src/room/room_info.rs | 11 +++++----- .../matrix-sdk-ui/src/notification_client.rs | 14 +++++++++---- crates/matrix-sdk/src/room_preview.rs | 12 +++++------ .../src/tests/sliding_sync/room.rs | 2 +- 9 files changed, 55 insertions(+), 37 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/notification.rs b/bindings/matrix-sdk-ffi/src/notification.rs index 6060f70d3..ca83703c8 100644 --- a/bindings/matrix-sdk-ffi/src/notification.rs +++ b/bindings/matrix-sdk-ffi/src/notification.rs @@ -35,7 +35,6 @@ pub struct NotificationRoomInfo { pub joined_members_count: u64, pub is_encrypted: Option, pub is_direct: bool, - pub is_public: bool, } #[derive(uniffi::Record)] @@ -74,11 +73,10 @@ impl NotificationItem { display_name: item.room_computed_display_name, avatar_url: item.room_avatar_url, canonical_alias: item.room_canonical_alias, - join_rule: item.room_join_rule.try_into().ok(), + join_rule: item.room_join_rule.map(TryInto::try_into).transpose().ok().flatten(), joined_members_count: item.joined_members_count, is_encrypted: item.is_room_encrypted, is_direct: item.is_direct_message_room, - is_public: item.is_room_public, }, is_noisy: item.is_noisy, has_mention: item.has_mention, diff --git a/bindings/matrix-sdk-ffi/src/room/mod.rs b/bindings/matrix-sdk-ffi/src/room/mod.rs index 32890acc9..ab6aa80f7 100644 --- a/bindings/matrix-sdk-ffi/src/room/mod.rs +++ b/bindings/matrix-sdk-ffi/src/room/mod.rs @@ -116,7 +116,10 @@ impl Room { self.inner.is_direct().await.unwrap_or(false) } - pub fn is_public(&self) -> bool { + /// Whether the room can be publicly joined or not, based on its join rule. + /// + /// Can return `None` if the join rule state event is missing. + pub fn is_public(&self) -> Option { self.inner.is_public() } diff --git a/bindings/matrix-sdk-ffi/src/room/room_info.rs b/bindings/matrix-sdk-ffi/src/room/room_info.rs index ebe2bd3ed..881af2d0e 100644 --- a/bindings/matrix-sdk-ffi/src/room/room_info.rs +++ b/bindings/matrix-sdk-ffi/src/room/room_info.rs @@ -26,7 +26,11 @@ pub struct RoomInfo { topic: Option, avatar_url: Option, is_direct: bool, - is_public: bool, + /// Whether the room is public or not, based on the join rules. + /// + /// Can be `None` if the join rules state event is not available for this + /// room. + is_public: Option, is_space: bool, /// If present, it means the room has been archived/upgraded. successor_room: Option, @@ -77,10 +81,15 @@ impl RoomInfo { let pinned_event_ids = room.pinned_event_ids().unwrap_or_default().iter().map(|id| id.to_string()).collect(); - let join_rule = room.join_rule().try_into(); - if let Err(e) = &join_rule { - warn!("Failed to parse join rule: {e:?}"); - } + let join_rule = room + .join_rule() + .map(TryInto::try_into) + .transpose() + .inspect_err(|err| { + warn!("Failed to parse join rule: {err}"); + }) + .ok() + .flatten(); let power_levels = RoomPowerLevels::new( room.power_levels().await.map_err(matrix_sdk::Error::from)?, @@ -135,7 +144,7 @@ impl RoomInfo { num_unread_notifications: room.num_unread_notifications(), num_unread_mentions: room.num_unread_mentions(), pinned_event_ids, - join_rule: join_rule.ok(), + join_rule, history_visibility: room.history_visibility_or_default().try_into()?, power_levels: Arc::new(power_levels), }) diff --git a/bindings/matrix-sdk-ffi/src/room_preview.rs b/bindings/matrix-sdk-ffi/src/room_preview.rs index 15ae11be1..89c95038b 100644 --- a/bindings/matrix-sdk-ffi/src/room_preview.rs +++ b/bindings/matrix-sdk-ffi/src/room_preview.rs @@ -37,8 +37,9 @@ impl RoomPreview { membership: info.state.map(|state| state.into()), join_rule: info .join_rule - .clone() - .try_into() + .as_ref() + .map(TryInto::try_into) + .transpose() .map_err(|_| anyhow::anyhow!("unhandled SpaceRoomJoinRule kind"))?, is_direct: info.is_direct, heroes: info @@ -114,17 +115,17 @@ pub struct RoomPreviewInfo { /// The membership state for the current user, if known. pub membership: Option, /// The join rule for this room (private, public, knock, etc.). - pub join_rule: JoinRule, + pub join_rule: Option, /// Whether the room is direct or not, if known. pub is_direct: Option, /// Room heroes. pub heroes: Option>, } -impl TryFrom for JoinRule { +impl TryFrom<&SpaceRoomJoinRule> for JoinRule { type Error = (); - fn try_from(join_rule: SpaceRoomJoinRule) -> Result { + fn try_from(join_rule: &SpaceRoomJoinRule) -> Result { Ok(match join_rule { SpaceRoomJoinRule::Invite => JoinRule::Invite, SpaceRoomJoinRule::Knock => JoinRule::Knock, diff --git a/crates/matrix-sdk-base/src/room/mod.rs b/crates/matrix-sdk-base/src/room/mod.rs index 839d2a5ef..7797ed57e 100644 --- a/crates/matrix-sdk-base/src/room/mod.rs +++ b/crates/matrix-sdk-base/src/room/mod.rs @@ -340,13 +340,15 @@ impl Room { } /// Is the room considered to be public. - pub fn is_public(&self) -> bool { - matches!(self.join_rule(), JoinRule::Public) + /// + /// May return `None` if the join rule event is not available. + pub fn is_public(&self) -> Option { + self.inner.read().join_rule().map(|join_rule| matches!(join_rule, JoinRule::Public)) } - /// Get the join rule policy of this room. - pub fn join_rule(&self) -> JoinRule { - self.inner.read().join_rule().clone() + /// Get the join rule policy of this room, if available. + pub fn join_rule(&self) -> Option { + self.inner.read().join_rule().cloned() } /// Get the maximum power level that this room contains. diff --git a/crates/matrix-sdk-base/src/room/room_info.rs b/crates/matrix-sdk-base/src/room/room_info.rs index f13eb97cf..ce2bb4f46 100644 --- a/crates/matrix-sdk-base/src/room/room_info.rs +++ b/crates/matrix-sdk-base/src/room/room_info.rs @@ -867,13 +867,12 @@ impl RoomInfo { } } - /// Returns the join rule for this room. - /// - /// Defaults to `Public`, if missing. - pub fn join_rule(&self) -> &JoinRule { + /// Return the join rule for this room, if the `m.room.join_rules` event is + /// available. + pub fn join_rule(&self) -> Option<&JoinRule> { match &self.base_info.join_rules { - Some(MinimalStateEvent::Original(ev)) => &ev.content.join_rule, - _ => &JoinRule::Public, + Some(MinimalStateEvent::Original(ev)) => Some(&ev.content.join_rule), + _ => None, } } diff --git a/crates/matrix-sdk-ui/src/notification_client.rs b/crates/matrix-sdk-ui/src/notification_client.rs index 71eae4e2a..5e1f92d0b 100644 --- a/crates/matrix-sdk-ui/src/notification_client.rs +++ b/crates/matrix-sdk-ui/src/notification_client.rs @@ -910,11 +910,11 @@ pub struct NotificationItem { /// Room canonical alias. pub room_canonical_alias: Option, /// Room join rule. - pub room_join_rule: JoinRule, + /// + /// Set to `None` if the join rule for this room is not available. + pub room_join_rule: Option, /// Is this room encrypted? pub is_room_encrypted: Option, - /// Is this a public room? - pub is_room_public: bool, /// Is this room considered a direct message? pub is_direct_message_room: bool, /// Numbers of members who joined the room. @@ -1010,7 +1010,6 @@ impl NotificationItem { room_canonical_alias: room.canonical_alias().map(|c| c.to_string()), room_join_rule: room.join_rule(), is_direct_message_room: room.is_direct().await?, - is_room_public: room.is_public(), is_room_encrypted: room .latest_encryption_state() .await @@ -1024,6 +1023,13 @@ impl NotificationItem { Ok(item) } + + /// Returns whether this room is public or not, based on the join rule. + /// + /// Maybe return `None` if the join rule is not available. + pub fn is_public(&self) -> Option { + self.room_join_rule.as_ref().map(|rule| matches!(rule, JoinRule::Public)) + } } /// An error for the [`NotificationClient`]. diff --git a/crates/matrix-sdk/src/room_preview.rs b/crates/matrix-sdk/src/room_preview.rs index 315643a4c..22e918db1 100644 --- a/crates/matrix-sdk/src/room_preview.rs +++ b/crates/matrix-sdk/src/room_preview.rs @@ -64,7 +64,7 @@ pub struct RoomPreview { pub room_type: Option, /// What's the join rule for this room? - pub join_rule: SpaceRoomJoinRule, + pub join_rule: Option, /// Is the room world-readable (i.e. is its history_visibility set to /// world_readable)? @@ -102,7 +102,7 @@ impl RoomPreview { topic: room_info.topic().map(ToOwned::to_owned), avatar_url: room_info.avatar_url().map(ToOwned::to_owned), room_type: room_info.room_type().cloned(), - join_rule: match room_info.join_rule() { + join_rule: room_info.join_rule().map(|rule| match rule { JoinRule::Invite => SpaceRoomJoinRule::Invite, JoinRule::Knock => SpaceRoomJoinRule::Knock, JoinRule::Private => SpaceRoomJoinRule::Private, @@ -114,7 +114,7 @@ impl RoomPreview { // private (a cautious choice). SpaceRoomJoinRule::Private } - }, + }), is_world_readable: room_info .history_visibility() .map(|vis| *vis == HistoryVisibility::WorldReadable), @@ -287,7 +287,7 @@ impl RoomPreview { num_joined_members: response.num_joined_members.into(), num_active_members, room_type: response.room_type, - join_rule: response.join_rule, + join_rule: Some(response.join_rule), is_world_readable: Some(response.world_readable), state, is_direct, @@ -375,14 +375,14 @@ async fn search_for_room_preview_in_room_directory( num_active_members: None, // Assume it's a room room_type: None, - join_rule: match room_description.join_rule { + join_rule: Some(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: Some(room_description.is_world_readable), state: None, is_direct: None, diff --git a/testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs b/testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs index d32a1df1e..8918fc9f7 100644 --- a/testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs +++ b/testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs @@ -1228,7 +1228,7 @@ fn assert_room_preview(preview: &RoomPreview, room_alias: &str) { assert_eq!(preview.avatar_url.as_ref().unwrap(), mxc_uri!("mxc://localhost/alice")); assert_eq!(preview.num_joined_members, 1); assert!(preview.room_type.is_none()); - assert_eq!(preview.join_rule, SpaceRoomJoinRule::Invite); + assert_eq!(preview.join_rule, Some(SpaceRoomJoinRule::Invite)); assert!(preview.is_world_readable.unwrap()); }