From ac7c6ba84c0b5400cf346902829b72bcd99927da Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Wed, 7 Jun 2023 15:42:39 +0100 Subject: [PATCH 01/10] Test for finding an avatar in a room via sliding sync --- crates/matrix-sdk-base/src/sliding_sync.rs | 67 +++++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 4d9753448..9efcc9a8b 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -276,7 +276,14 @@ impl BaseClient { #[cfg(test)] mod test { - use ruma::{device_id, room_id, uint, user_id, RoomId}; + use ruma::{ + device_id, event_id, + events::{room::avatar::RoomAvatarEventContent, StateEventContent}, + mxc_uri, room_id, + serde::Raw, + uint, user_id, MxcUri, RoomId, UserId, + }; + use serde_json::{json, Value as JsonValue}; use super::*; use crate::SessionMeta; @@ -324,6 +331,26 @@ mod test { assert_eq!(client_room.name(), Some("little room".to_owned())); } + #[tokio::test] + async fn avatar_is_found_when_processing_sliding_sync_response() { + // Given a logged-in client + let client = logged_in_client().await; + let room_id = room_id!("!r:e.uk"); + let user_id = user_id!("@u:e.uk"); + + // When I send sliding sync response containing a room with an avatar + let room = room_with_avatar(mxc_uri!("mxc://e.uk/med1"), user_id); + let response = response_with_room(room_id, room).await; + client.process_sliding_sync(&response).await.expect("Failed to process sync"); + + // Then the room in the client has the avatar + let client_room = client.get_room(room_id).expect("No room found"); + assert_eq!( + client_room.avatar_url().expect("No avatar URL").media_id().expect("No media ID"), + "med1" + ); + } + async fn logged_in_client() -> BaseClient { let client = BaseClient::new(); client @@ -341,4 +368,42 @@ mod test { response.rooms.insert(room_id.to_owned(), room); response } + + fn room_with_avatar(avatar_uri: &MxcUri, user_id: &UserId) -> v4::SlidingSyncRoom { + let mut room = v4::SlidingSyncRoom::new(); + + let mut avatar_event_content = RoomAvatarEventContent::new(); + avatar_event_content.url = Some(avatar_uri.to_owned()); + + room.required_state.push( + Raw::new(&make_state_event(user_id, "", avatar_event_content, None)) + .expect("Failed to create state event") + .cast(), + ); + + room + } + + fn make_state_event( + sender: &UserId, + state_key: &str, + content: C, + prev_content: Option, + ) -> JsonValue { + let unsigned = if let Some(prev_content) = prev_content { + json!({ "prev_content": prev_content }) + } else { + json!({}) + }; + + json!({ + "type": content.event_type(), + "state_key": state_key, + "content": content, + "event_id": event_id!("$evt"), + "sender": sender, + "origin_server_ts": 10, + "unsigned": unsigned, + }) + } } From 5545c98a85b0a20bf750575aeb80815f0bb937e6 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Wed, 7 Jun 2023 15:58:56 +0100 Subject: [PATCH 02/10] Test for finding avatars in invite rooms --- crates/matrix-sdk-base/src/sliding_sync.rs | 58 ++++++++++++++++++---- 1 file changed, 47 insertions(+), 11 deletions(-) diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 9efcc9a8b..18dd3a3cf 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -278,12 +278,18 @@ impl BaseClient { mod test { use ruma::{ device_id, event_id, - events::{room::avatar::RoomAvatarEventContent, StateEventContent}, + events::{ + room::{ + avatar::RoomAvatarEventContent, + member::{MembershipState, RoomMemberEventContent}, + }, + StateEventContent, + }, mxc_uri, room_id, serde::Raw, uint, user_id, MxcUri, RoomId, UserId, }; - use serde_json::{json, Value as JsonValue}; + use serde_json::json; use super::*; use crate::SessionMeta; @@ -351,6 +357,28 @@ mod test { ); } + #[tokio::test] + #[ignore = "fails because we don't process avatars for invite rooms"] + async fn avatar_is_found_invitation_room_when_processing_sliding_sync_response() { + // Given a logged-in client + let client = logged_in_client().await; + let room_id = room_id!("!r:e.uk"); + let user_id = user_id!("@u:e.uk"); + + // When I send sliding sync response containing an invited room with an avatar + let mut room = room_with_avatar(mxc_uri!("mxc://e.uk/med1"), user_id); + set_room_membership(&mut room, user_id, MembershipState::Invite); + let response = response_with_room(room_id, room).await; + client.process_sliding_sync(&response).await.expect("Failed to process sync"); + + // Then the room in the client has the avatar + let client_room = client.get_room(room_id).expect("No room found"); + assert_eq!( + client_room.avatar_url().expect("No avatar URL").media_id().expect("No media ID"), + "med1" + ); + } + async fn logged_in_client() -> BaseClient { let client = BaseClient::new(); client @@ -375,28 +403,34 @@ mod test { let mut avatar_event_content = RoomAvatarEventContent::new(); avatar_event_content.url = Some(avatar_uri.to_owned()); - room.required_state.push( - Raw::new(&make_state_event(user_id, "", avatar_event_content, None)) - .expect("Failed to create state event") - .cast(), - ); + room.required_state.push(make_state_event(user_id, "", avatar_event_content, None)); room } - fn make_state_event( + fn set_room_membership( + room: &mut v4::SlidingSyncRoom, + user_id: &UserId, + membership_state: MembershipState, + ) { + let invite_content = RoomMemberEventContent::new(membership_state); + room.invite_state = + Some(vec![make_state_event(user_id, user_id.as_ref(), invite_content, None)]); + } + + fn make_state_event( sender: &UserId, state_key: &str, content: C, prev_content: Option, - ) -> JsonValue { + ) -> Raw { let unsigned = if let Some(prev_content) = prev_content { json!({ "prev_content": prev_content }) } else { json!({}) }; - json!({ + Raw::new(&json!({ "type": content.event_type(), "state_key": state_key, "content": content, @@ -404,6 +438,8 @@ mod test { "sender": sender, "origin_server_ts": 10, "unsigned": unsigned, - }) + })) + .expect("Failed to create state event") + .cast() } } From a85c48136816e97698a78c7401401721df2ae038 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Thu, 8 Jun 2023 10:20:48 +0100 Subject: [PATCH 03/10] Test for adding an invited room to the client and invite list --- crates/matrix-sdk-base/src/sliding_sync.rs | 34 ++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 18dd3a3cf..88fda44e9 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -302,7 +302,7 @@ mod test { } #[tokio::test] - async fn process_sliding_sync_response_to_add_a_room() { + async fn room_with_unspecified_state_is_added_to_client_and_joined_list() { // Given a logged-in client let client = logged_in_client().await; let room_id = room_id!("!r:e.uk"); @@ -312,12 +312,18 @@ mod test { let mut room = v4::SlidingSyncRoom::new(); room.joined_count = Some(uint!(41)); let response = response_with_room(room_id, room).await; - client.process_sliding_sync(&response).await.expect("Failed to process sync"); + let sync_resp = + client.process_sliding_sync(&response).await.expect("Failed to process sync"); // Then the room appears in the client (with the same joined count) let client_room = client.get_room(room_id).expect("No room found"); assert_eq!(client_room.room_id(), room_id); assert_eq!(client_room.joined_members_count(), 41); + assert_eq!(client_room.state(), RoomState::Joined); + + // And it is added to the list of joined rooms, and not the invited ones + assert!(sync_resp.rooms.join.get(room_id).is_some()); + assert!(sync_resp.rooms.invite.get(room_id).is_none()); } #[tokio::test] @@ -357,6 +363,30 @@ mod test { ); } + #[tokio::test] + async fn invitation_room_is_added_to_client_and_invite_list() { + // Given a logged-in client + let client = logged_in_client().await; + let room_id = room_id!("!r:e.uk"); + let user_id = user_id!("@u:e.uk"); + + // When I send sliding sync response containing an invited room + let mut room = v4::SlidingSyncRoom::new(); + set_room_membership(&mut room, user_id, MembershipState::Invite); + let response = response_with_room(room_id, room).await; + let sync_resp = + client.process_sliding_sync(&response).await.expect("Failed to process sync"); + + // Then the room is added to the client + let client_room = client.get_room(room_id).expect("No room found"); + assert_eq!(client_room.room_id(), room_id); + assert_eq!(client_room.state(), RoomState::Invited); + + // And it is added to the list of invited rooms, not the joined ones + assert!(!sync_resp.rooms.invite[room_id].invite_state.is_empty()); + assert!(sync_resp.rooms.join.get(room_id).is_none()); + } + #[tokio::test] #[ignore = "fails because we don't process avatars for invite rooms"] async fn avatar_is_found_invitation_room_when_processing_sliding_sync_response() { From 24ff62befca5bfb8fb168798da117e37c36945af Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Thu, 8 Jun 2023 09:42:50 +0100 Subject: [PATCH 04/10] Refactor process_sliding_sync to extract a method for dealing with rooms --- crates/matrix-sdk-base/src/sliding_sync.rs | 261 ++++++++++++--------- 1 file changed, 146 insertions(+), 115 deletions(-) diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 88fda44e9..12a33d6ed 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -15,9 +15,12 @@ #[cfg(feature = "e2e-encryption")] use std::ops::Deref; -use ruma::api::client::sync::sync_events::{ - v3::{self, RoomSummary}, - v4, +use ruma::{ + api::client::sync::sync_events::{ + v3::{self, InvitedRoom, RoomSummary}, + v4::{self, AccountData}, + }, + RoomId, }; use tracing::{debug, info, instrument}; @@ -28,8 +31,9 @@ use crate::{ deserialized_responses::AmbiguityChanges, error::Result, rooms::RoomState, - store::{ambiguity_map::AmbiguityCache, StateChanges}, + store::{ambiguity_map::AmbiguityCache, StateChanges, Store}, sync::{JoinedRoom, Rooms, SyncResponse}, + RoomInfo, }; impl BaseClient { @@ -97,120 +101,27 @@ impl BaseClient { self.handle_account_data(&account_data.global, &mut changes).await; } - let push_rules = self.get_push_rules(&changes).await?; - let mut new_rooms = Rooms::default(); for (room_id, room_data) in rooms { - if let Some(invite_state) = &room_data.invite_state { - let room = store.get_or_create_stripped_room(room_id).await; - let mut room_info = room.clone_info(); - room_info.mark_state_partially_synced(); - - if let Some(r) = store.get_room(room_id) { - let mut room_info = r.clone_info(); - room_info.mark_as_invited(); // FIXME: this might not be accurate - room_info.mark_state_partially_synced(); - changes.add_room(room_info); - } - - self.handle_invited_state(invite_state.as_slice(), &mut room_info, &mut changes); - - new_rooms.invite.insert( - room_id.clone(), - v3::InvitedRoom::from(v3::InviteState::from(invite_state.clone())), - ); - } else { - let room = store.get_or_create_room(room_id, RoomState::Joined).await; - let mut room_info = room.clone_info(); - room_info.mark_as_joined(); // FIXME: this might not be accurate - room_info.mark_state_partially_synced(); - - if let Some(name) = &room_data.name { - room_info.update_name(name.to_owned()); - } - - // Sliding sync doesn't have a room summary, nevertheless it contains the joined - // and invited member counts. It likely will never have a heroes concept since - // it calculates the room display name for us. - // - // Let's at least fetch the member counts, since they might be useful. - let mut room_summary = RoomSummary::new(); - room_summary.invited_member_count = room_data.invited_count; - room_summary.joined_member_count = room_data.joined_count; - room_info.update_summary(&room_summary); - - room_info.set_prev_batch(room_data.prev_batch.as_deref()); - - let mut user_ids = if !room_data.required_state.is_empty() { - self.handle_state( - &room_data.required_state, - &mut room_info, - &mut changes, - &mut ambiguity_cache, - ) - .await? - } else { - Default::default() - }; - - let room_account_data = if let Some(events) = account_data.rooms.get(room_id) { - self.handle_room_account_data(room_id, events, &mut changes).await; - Some(events.to_vec()) - } else { - None - }; - - if room_data.limited { - room_info.mark_members_missing(); - } - - let timeline = self - .handle_timeline( - &room, - room_data.limited, - room_data.timeline.clone(), - room_data.prev_batch.clone(), - &push_rules, - &mut user_ids, - &mut room_info, - &mut changes, - &mut ambiguity_cache, - ) - .await?; - - #[cfg(feature = "e2e-encryption")] - if room_info.is_encrypted() { - if let Some(o) = self.olm_machine() { - if !room.is_encrypted() { - // The room turned on encryption in this sync, we need - // to also get all the existing users and mark them for - // tracking. - let user_ids = - store.get_user_ids(room_id, RoomMemberships::ACTIVE).await?; - o.update_tracked_users(user_ids.iter().map(Deref::deref)).await? - } - - if !user_ids.is_empty() { - o.update_tracked_users(user_ids.iter().map(Deref::deref)).await?; - } - } - } - let notification_count = room_data.unread_notifications.clone().into(); - room_info.update_notification_count(notification_count); - - new_rooms.join.insert( - room_id.clone(), - JoinedRoom::new( - timeline, - room_data.required_state.clone(), - room_account_data.unwrap_or_default(), - Vec::new(), - notification_count, - ), - ); - - changes.add_room(room_info); + let (room_to_add, joined_room, invited_room) = self + .process_sliding_sync_room( + room_id, + room_data, + &store, + &mut changes, + &mut ambiguity_cache, + account_data, + ) + .await?; + if let Some(room_to_add) = room_to_add { + changes.add_room(room_to_add); + } + if let Some(joined_room) = joined_room { + new_rooms.join.insert(room_id.clone(), joined_room); + } + if let Some(invited_room) = invited_room { + new_rooms.invite.insert(room_id.clone(), invited_room); } } @@ -272,6 +183,126 @@ impl BaseClient { device_one_time_keys_count, }) } + + async fn process_sliding_sync_room( + &self, + room_id: &RoomId, + room_data: &v4::SlidingSyncRoom, + store: &Store, + changes: &mut StateChanges, + ambiguity_cache: &mut AmbiguityCache, + account_data: &AccountData, + ) -> Result<(Option, Option, Option)> { + if let Some(invite_state) = &room_data.invite_state { + let room = store.get_or_create_stripped_room(room_id).await; + let mut room_info = room.clone_info(); + room_info.mark_state_partially_synced(); + + let room_to_add = if let Some(r) = store.get_room(room_id) { + let mut room_info = r.clone_info(); + room_info.mark_as_invited(); // FIXME: this might not be accurate + room_info.mark_state_partially_synced(); + Some(room_info) + } else { + None + }; + + self.handle_invited_state(invite_state.as_slice(), &mut room_info, changes); + + let invited_room = v3::InvitedRoom::from(v3::InviteState::from(invite_state.clone())); + + Ok((room_to_add, None, Some(invited_room))) + } else { + let room = store.get_or_create_room(room_id, RoomState::Joined).await; + let mut room_info = room.clone_info(); + room_info.mark_as_joined(); // FIXME: this might not be accurate + room_info.mark_state_partially_synced(); + + if let Some(name) = &room_data.name { + room_info.update_name(name.to_owned()); + } + + // Sliding sync doesn't have a room summary, nevertheless it contains the joined + // and invited member counts. It likely will never have a heroes concept since + // it calculates the room display name for us. + // + // Let's at least fetch the member counts, since they might be useful. + let mut room_summary = RoomSummary::new(); + room_summary.invited_member_count = room_data.invited_count; + room_summary.joined_member_count = room_data.joined_count; + room_info.update_summary(&room_summary); + + room_info.set_prev_batch(room_data.prev_batch.as_deref()); + + let mut user_ids = if !room_data.required_state.is_empty() { + self.handle_state( + &room_data.required_state, + &mut room_info, + changes, + ambiguity_cache, + ) + .await? + } else { + Default::default() + }; + + let room_account_data = if let Some(events) = account_data.rooms.get(room_id) { + self.handle_room_account_data(room_id, events, changes).await; + Some(events.to_vec()) + } else { + None + }; + + if room_data.limited { + room_info.mark_members_missing(); + } + + let push_rules = self.get_push_rules(changes).await?; + + let timeline = self + .handle_timeline( + &room, + room_data.limited, + room_data.timeline.clone(), + room_data.prev_batch.clone(), + &push_rules, + &mut user_ids, + &mut room_info, + changes, + ambiguity_cache, + ) + .await?; + + #[cfg(feature = "e2e-encryption")] + if room_info.is_encrypted() { + if let Some(o) = self.olm_machine() { + if !room.is_encrypted() { + // The room turned on encryption in this sync, we need + // to also get all the existing users and mark them for + // tracking. + let user_ids = store.get_user_ids(room_id, RoomMemberships::ACTIVE).await?; + o.update_tracked_users(user_ids.iter().map(Deref::deref)).await? + } + + if !user_ids.is_empty() { + o.update_tracked_users(user_ids.iter().map(Deref::deref)).await?; + } + } + } + let notification_count = room_data.unread_notifications.clone().into(); + room_info.update_notification_count(notification_count); + + let joined_room = JoinedRoom::new( + timeline, + room_data.required_state.clone(), + room_account_data.unwrap_or_default(), + Vec::new(), + notification_count, + ); + + Ok((Some(room_info), Some(joined_room), None)) + } + } } #[cfg(test)] From ee8f94cb95bc984e5231242d96f662ad614f8921 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Thu, 8 Jun 2023 13:19:18 +0100 Subject: [PATCH 05/10] Rename room_to_add->room_to_store --- crates/matrix-sdk-base/src/sliding_sync.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 12a33d6ed..2c09d7d22 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -104,7 +104,7 @@ impl BaseClient { let mut new_rooms = Rooms::default(); for (room_id, room_data) in rooms { - let (room_to_add, joined_room, invited_room) = self + let (room_to_store, joined_room, invited_room) = self .process_sliding_sync_room( room_id, room_data, @@ -114,8 +114,8 @@ impl BaseClient { account_data, ) .await?; - if let Some(room_to_add) = room_to_add { - changes.add_room(room_to_add); + if let Some(room_to_store) = room_to_store { + changes.add_room(room_to_store); } if let Some(joined_room) = joined_room { new_rooms.join.insert(room_id.clone(), joined_room); @@ -198,7 +198,7 @@ impl BaseClient { let mut room_info = room.clone_info(); room_info.mark_state_partially_synced(); - let room_to_add = if let Some(r) = store.get_room(room_id) { + let room_to_store = if let Some(r) = store.get_room(room_id) { let mut room_info = r.clone_info(); room_info.mark_as_invited(); // FIXME: this might not be accurate room_info.mark_state_partially_synced(); @@ -211,7 +211,7 @@ impl BaseClient { let invited_room = v3::InvitedRoom::from(v3::InviteState::from(invite_state.clone())); - Ok((room_to_add, None, Some(invited_room))) + Ok((room_to_store, None, Some(invited_room))) } else { let room = store.get_or_create_room(room_id, RoomState::Joined).await; let mut room_info = room.clone_info(); From 4a04c70c451e048cd533508891c6e033bf04eb88 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Thu, 8 Jun 2023 13:24:15 +0100 Subject: [PATCH 06/10] Handle required state in invited rooms as well as normal --- crates/matrix-sdk-base/src/sliding_sync.rs | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 2c09d7d22..e1ef63ef8 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -199,10 +199,19 @@ impl BaseClient { room_info.mark_state_partially_synced(); let room_to_store = if let Some(r) = store.get_room(room_id) { - let mut room_info = r.clone_info(); - room_info.mark_as_invited(); // FIXME: this might not be accurate - room_info.mark_state_partially_synced(); - Some(room_info) + let mut stored_room_info = r.clone_info(); + stored_room_info.mark_as_invited(); // FIXME: this might not be accurate + stored_room_info.mark_state_partially_synced(); + if !room_data.required_state.is_empty() { + self.handle_state( + &room_data.required_state, + &mut stored_room_info, + changes, + ambiguity_cache, + ) + .await?; + } + Some(stored_room_info) } else { None }; @@ -419,8 +428,7 @@ mod test { } #[tokio::test] - #[ignore = "fails because we don't process avatars for invite rooms"] - async fn avatar_is_found_invitation_room_when_processing_sliding_sync_response() { + async fn avatar_is_found_in_invitation_room_when_processing_sliding_sync_response() { // Given a logged-in client let client = logged_in_client().await; let room_id = room_id!("!r:e.uk"); From 8c2cc522bc4a420378e1cebfb27abc7fa875c2a6 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Thu, 8 Jun 2023 14:14:44 +0100 Subject: [PATCH 07/10] Test canonical aliases in invited rooms via sliding sync --- crates/matrix-sdk-base/src/sliding_sync.rs | 43 +++++++++++++++++++++- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index e1ef63ef8..ab29c74d2 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -321,13 +321,14 @@ mod test { events::{ room::{ avatar::RoomAvatarEventContent, + canonical_alias::RoomCanonicalAliasEventContent, member::{MembershipState, RoomMemberEventContent}, }, StateEventContent, }, - mxc_uri, room_id, + mxc_uri, room_alias_id, room_id, serde::Raw, - uint, user_id, MxcUri, RoomId, UserId, + uint, user_id, MxcUri, RoomAliasId, RoomId, UserId, }; use serde_json::json; @@ -448,6 +449,25 @@ mod test { ); } + #[tokio::test] + async fn canonical_alias_is_found_in_invitation_room_when_processing_sliding_sync_response() { + // Given a logged-in client + let client = logged_in_client().await; + let room_id = room_id!("!r:e.uk"); + let user_id = user_id!("@u:e.uk"); + let room_alias_id = room_alias_id!("#myroom:e.uk"); + + // When I send sliding sync response containing an invited room with an avatar + let mut room = room_with_canonical_alias(room_alias_id, user_id); + set_room_membership(&mut room, user_id, MembershipState::Invite); + let response = response_with_room(room_id, room).await; + client.process_sliding_sync(&response).await.expect("Failed to process sync"); + + // Then the room in the client has the avatar + let client_room = client.get_room(room_id).expect("No room found"); + assert_eq!(client_room.canonical_alias(), Some(room_alias_id.to_owned())); + } + async fn logged_in_client() -> BaseClient { let client = BaseClient::new(); client @@ -477,6 +497,25 @@ mod test { room } + fn room_with_canonical_alias( + room_alias_id: &RoomAliasId, + user_id: &UserId, + ) -> v4::SlidingSyncRoom { + let mut room = v4::SlidingSyncRoom::new(); + + let mut canonical_alias_event_content = RoomCanonicalAliasEventContent::new(); + canonical_alias_event_content.alias = Some(room_alias_id.to_owned()); + + room.required_state.push(make_state_event( + user_id, + "", + canonical_alias_event_content, + None, + )); + + room + } + fn set_room_membership( room: &mut v4::SlidingSyncRoom, user_id: &UserId, From 9d943e7c62c19824a0d9174e3f308b8513fac6b7 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Thu, 8 Jun 2023 16:09:52 +0100 Subject: [PATCH 08/10] Clarify in tests that invite_state contains very minimal JSON --- crates/matrix-sdk-base/src/sliding_sync.rs | 30 +++++++++++----------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index ab29c74d2..d20064e82 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -320,9 +320,7 @@ mod test { device_id, event_id, events::{ room::{ - avatar::RoomAvatarEventContent, - canonical_alias::RoomCanonicalAliasEventContent, - member::{MembershipState, RoomMemberEventContent}, + avatar::RoomAvatarEventContent, canonical_alias::RoomCanonicalAliasEventContent, }, StateEventContent, }, @@ -409,11 +407,10 @@ mod test { // Given a logged-in client let client = logged_in_client().await; let room_id = room_id!("!r:e.uk"); - let user_id = user_id!("@u:e.uk"); // When I send sliding sync response containing an invited room let mut room = v4::SlidingSyncRoom::new(); - set_room_membership(&mut room, user_id, MembershipState::Invite); + set_room_invited(&mut room); let response = response_with_room(room_id, room).await; let sync_resp = client.process_sliding_sync(&response).await.expect("Failed to process sync"); @@ -437,7 +434,7 @@ mod test { // When I send sliding sync response containing an invited room with an avatar let mut room = room_with_avatar(mxc_uri!("mxc://e.uk/med1"), user_id); - set_room_membership(&mut room, user_id, MembershipState::Invite); + set_room_invited(&mut room); let response = response_with_room(room_id, room).await; client.process_sliding_sync(&response).await.expect("Failed to process sync"); @@ -459,7 +456,7 @@ mod test { // When I send sliding sync response containing an invited room with an avatar let mut room = room_with_canonical_alias(room_alias_id, user_id); - set_room_membership(&mut room, user_id, MembershipState::Invite); + set_room_invited(&mut room); let response = response_with_room(room_id, room).await; client.process_sliding_sync(&response).await.expect("Failed to process sync"); @@ -516,14 +513,17 @@ mod test { room } - fn set_room_membership( - room: &mut v4::SlidingSyncRoom, - user_id: &UserId, - membership_state: MembershipState, - ) { - let invite_content = RoomMemberEventContent::new(membership_state); - room.invite_state = - Some(vec![make_state_event(user_id, user_id.as_ref(), invite_content, None)]); + fn set_room_invited(room: &mut v4::SlidingSyncRoom) { + // MSC3575 shows an almost-empty event to indicate that we are invited to a room. + // Just the type is supplied. + + let evt = Raw::new(&json!({ + "type": "m.room.member", + })) + .expect("Failed to make raw event") + .cast(); + + room.invite_state = Some(vec![evt]); } fn make_state_event( From 7e197ec8d4234e5a5a7fcc2f6984988d0bab1bb2 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Thu, 8 Jun 2023 16:26:58 +0100 Subject: [PATCH 09/10] Simplify invited room sliding sync processing Added some comments based on my understanding of MSC3575. Previously we had a FIXME about how we were setting the room state to invite or join based on the presence or absence of the invite_state property. I think this behaviour is correct, so I added some comments explaining why. The functional change here is to note that we call store.get_or_create_stripped_room to find/create the stripped room, and this actually deletes any room of this ID that is in the store, so our later call to store.get_room would always fall back to getting the stripped room, which we already have, so there was no need to do the get_room call at all. --- crates/matrix-sdk-base/src/sliding_sync.rs | 50 ++++++++++++++-------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index d20064e82..a05e2c93e 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -193,38 +193,50 @@ impl BaseClient { ambiguity_cache: &mut AmbiguityCache, account_data: &AccountData, ) -> Result<(Option, Option, Option)> { + // If any invite_state exists, we take it to mean that we are invited to this room + // https://github.com/matrix-org/matrix-spec-proposals/blob/kegan/sync-v3/proposals/3575-sync.md#room-list-parameters if let Some(invite_state) = &room_data.invite_state { let room = store.get_or_create_stripped_room(room_id).await; let mut room_info = room.clone_info(); room_info.mark_state_partially_synced(); - let room_to_store = if let Some(r) = store.get_room(room_id) { - let mut stored_room_info = r.clone_info(); - stored_room_info.mark_as_invited(); // FIXME: this might not be accurate - stored_room_info.mark_state_partially_synced(); - if !room_data.required_state.is_empty() { - self.handle_state( - &room_data.required_state, - &mut stored_room_info, - changes, - ambiguity_cache, - ) - .await?; - } - Some(stored_room_info) - } else { - None - }; + // We don't actually know what events are inside invite_state. In theory, they might + // not contain an m.room.member event, or they might set the membership to something + // other than invite. This would be very weird behaviour by the server, because + // invite_state is supposed to contain an m.room.member. + // Later, we will call handle_invited_state, which will reflect any information found + // in the real events inside invite_state, but we default to considering this room + // invited simply because invite_state exists. This is needed in the normal case, + // because the sliding sync server tries to send minimal state, meaning that we + // normally actually just receive {"type": "m.room.member"} with no content at all. + room_info.mark_as_invited(); + + room_info.mark_state_partially_synced(); + + if !room_data.required_state.is_empty() { + self.handle_state( + &room_data.required_state, + &mut room_info, + changes, + ambiguity_cache, + ) + .await?; + } self.handle_invited_state(invite_state.as_slice(), &mut room_info, changes); let invited_room = v3::InvitedRoom::from(v3::InviteState::from(invite_state.clone())); - Ok((room_to_store, None, Some(invited_room))) + Ok((Some(room_info), None, Some(invited_room))) } else { let room = store.get_or_create_room(room_id, RoomState::Joined).await; let mut room_info = room.clone_info(); - room_info.mark_as_joined(); // FIXME: this might not be accurate + + // We default to considering this room joined if it's not an invite, but we expect to + // find a m.room.member event in the required_state, so this should get fixed to the + // real correct value when we call self.handle_state below. + room_info.mark_as_joined(); + room_info.mark_state_partially_synced(); if let Some(name) = &room_data.name { From f4aed74fb1299b524d058b58c9bef3a4f4eb719f Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Mon, 12 Jun 2023 09:50:04 +0100 Subject: [PATCH 10/10] Fix formatting --- crates/matrix-sdk-base/src/sliding_sync.rs | 35 ++++++++++++---------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index a05e2c93e..1fc6c8b30 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -193,22 +193,24 @@ impl BaseClient { ambiguity_cache: &mut AmbiguityCache, account_data: &AccountData, ) -> Result<(Option, Option, Option)> { - // If any invite_state exists, we take it to mean that we are invited to this room - // https://github.com/matrix-org/matrix-spec-proposals/blob/kegan/sync-v3/proposals/3575-sync.md#room-list-parameters + // If any invite_state exists, we take it to mean that we are invited to this + // room https://github.com/matrix-org/matrix-spec-proposals/blob/kegan/sync-v3/proposals/3575-sync.md#room-list-parameters if let Some(invite_state) = &room_data.invite_state { let room = store.get_or_create_stripped_room(room_id).await; let mut room_info = room.clone_info(); room_info.mark_state_partially_synced(); - // We don't actually know what events are inside invite_state. In theory, they might - // not contain an m.room.member event, or they might set the membership to something - // other than invite. This would be very weird behaviour by the server, because - // invite_state is supposed to contain an m.room.member. - // Later, we will call handle_invited_state, which will reflect any information found - // in the real events inside invite_state, but we default to considering this room - // invited simply because invite_state exists. This is needed in the normal case, - // because the sliding sync server tries to send minimal state, meaning that we - // normally actually just receive {"type": "m.room.member"} with no content at all. + // We don't actually know what events are inside invite_state. In theory, they + // might not contain an m.room.member event, or they might set the + // membership to something other than invite. This would be very + // weird behaviour by the server, because invite_state is supposed + // to contain an m.room.member. Later, we will call + // handle_invited_state, which will reflect any information found in + // the real events inside invite_state, but we default to considering this room + // invited simply because invite_state exists. This is needed in the normal + // case, because the sliding sync server tries to send minimal + // state, meaning that we normally actually just receive {"type": + // "m.room.member"} with no content at all. room_info.mark_as_invited(); room_info.mark_state_partially_synced(); @@ -232,9 +234,10 @@ impl BaseClient { let room = store.get_or_create_room(room_id, RoomState::Joined).await; let mut room_info = room.clone_info(); - // We default to considering this room joined if it's not an invite, but we expect to - // find a m.room.member event in the required_state, so this should get fixed to the - // real correct value when we call self.handle_state below. + // We default to considering this room joined if it's not an invite, but we + // expect to find a m.room.member event in the required_state, so + // this should get fixed to the real correct value when we call + // self.handle_state below. room_info.mark_as_joined(); room_info.mark_state_partially_synced(); @@ -526,8 +529,8 @@ mod test { } fn set_room_invited(room: &mut v4::SlidingSyncRoom) { - // MSC3575 shows an almost-empty event to indicate that we are invited to a room. - // Just the type is supplied. + // MSC3575 shows an almost-empty event to indicate that we are invited to a + // room. Just the type is supplied. let evt = Raw::new(&json!({ "type": "m.room.member",