feat(knocking): process knocked rooms separately during sync #4128

jmartinesp · 2024-10-15T15:06:19Z

Changes

This PR contains 2 commits:

The first one processes the membership state events in process_sliding_sync_room_membership, deserializes them and decides whether the resulting room should be a knock or invite one default to invite if not sure. Then it propagates the changes up the flow.
Since we already have handle_stripped_state to parse the contents of the invite_state stripped state events it seemed better to reuse this and deserialize the events only once, then mark the room as invited or knocked in this method. Ideally this should be part of the inner fn BaseRoomInfo::handle_stripped_state_event, but that doesn't have access to the Room::state property.

Both solutions work AFAICT, the 1st one centralises the membership processing in a single point but it's more inefficient, the 2nd one does only one round of deserialization but hides the membership change a bit lower in the sliding sync processing flow. Let me know if you have any preferences and I'll use one of those versions, or propose a better alternative.

Existing tests have been extended to check the invite vs knock room state processing.

Public API changes documented in changelogs (optional)

Signed-off-by:

jmartinesp · 2024-10-15T15:18:22Z

I just rebased the changes to avoid a use ... conflict, there aren't any other changes.

codecov · 2024-10-15T15:32:25Z

Codecov Report

Attention: Patch coverage is 88.88889% with 9 lines in your changes missing coverage. Please review.

Project coverage is 84.74%. Comparing base (30f3a3c) to head (8636c3a).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-base/src/client.rs	90.90%	3 Missing ⚠️
crates/matrix-sdk-base/src/sliding_sync/mod.rs	92.30%	3 Missing ⚠️
crates/matrix-sdk/src/sync.rs	62.50%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4128      +/-   ##
==========================================
+ Coverage   84.72%   84.74%   +0.01%     
==========================================
  Files         269      269              
  Lines       28779    28836      +57     
==========================================
+ Hits        24384    24437      +53     
- Misses       4395     4399       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bnjbvr

Looks promising. I think I prefer the way it's handled in the first commit, especially since there might be a nice simplification there too.

bnjbvr · 2024-10-15T15:48:03Z

crates/matrix-sdk-base/src/client.rs

+ );
+
+ let mut room_info = room.clone_info();
+ room_info.mark_as_invited();


mark_as_knocked?

bnjbvr · 2024-10-15T15:51:42Z

crates/matrix-sdk-base/src/sliding_sync/mod.rs

+ let membership_event_content = invite_state.iter().find_map(|raw| {
+ raw.deserialize().ok().iter().find_map(|e| {
+ if let AnyStrippedStateEvent::RoomMember(membership_event) = e {
+ if membership_event.sender == user_id {


Can you double-check this logic, please: I think the sender for an invite would not be the user themselves, but rather, the person who sent the invite. So we'd need to take a look at the state_key, in this case. (If that's true, consider adding a test which would have failed because of this?)

Yes, I think this is an auto-completion typo 😬 . Nice catch!

01b3bc9 should take care of the typo, 50f7fb0 adds more tests for corner cases with wrong membership events.

bnjbvr · 2024-10-15T15:58:30Z

crates/matrix-sdk-base/src/sliding_sync/mod.rs

+ let user_id = self
+ .session_meta()
+ .expect("Sliding sync shouldn't run without an authenticated user.")
+ .user_id
+ .to_owned();


Could we pass this user_id as a parameter, to avoid repeating the work for each room?

01b3bc9 should take care of this

bnjbvr · 2024-10-15T16:00:46Z

crates/matrix-sdk-base/src/sliding_sync/mod.rs

+ // We need to find the membership event since it could be for either an invited
+ // or knocked room
+ let membership_event_content = invite_state.iter().find_map(|raw| {
+ raw.deserialize().ok().iter().find_map(|e| {


So I'm not sure we're looking in the right place here: the required_state events have already been deserialized and passed as a function parameter called state_events. Might be worth checking against a real-world Synapse, but I'd expect the membership event to be there instead. If that's true, then we don't need the deserialization step here, and this solution feels better than the second one IMO (with the previous sync protocol, knocked rooms would appear in the knocked subset of the response, which is nice and simple — I'd rather not change the room state based on extra other information).

Invite and Knock rooms use invite_state instead of required_state, I suspect their required_state would be empty, but I can double check.

Yes, there's no required_state for knocked rooms:

"!luXevcpKTZauFIpPnL:matrix.org": { "bump_stamp": 21538303, "notification_count": 0, "highlight_count": 0, "initial": true, "invite_state": [ { "content": { "creator": "@jorgem_test:matrix.org", "room_version": "7" }, "sender": "@jorgem_test:matrix.org", "state_key": "", "type": "m.room.create" }, { "content": { "join_rule": "knock" }, "sender": "@jorgem_test:matrix.org", "state_key": "", "type": "m.room.join_rules" }, { "content": { "name": "Knocking room test" }, "sender": "@jorgem_test:matrix.org", "state_key": "", "type": "m.room.name" }, { "content": { "algorithm": "m.megolm.v1.aes-sha2" }, "sender": "@jorgem_test:matrix.org", "state_key": "", "type": "m.room.encryption" }, { "type": "m.room.member", "state_key": "@jorgem:element.io", "content": { "avatar_url": "mxc://element.io/75d9a551895233d284aff390d2086814a2f7e875", "displayname": "Jorge", "membership": "knock" }, "sender": "@jorgem:element.io" } ] }

Another option would be to handle the stripped state events in invite_state as part of the membership in this same method, but I think that's not possible because we need the push_rules too which are processed after this.

We could also deserialize the stripped state events in process_sliding_sync_room and pass it down both functions, this one and handle_invited_state, so they'd be deserialized just once.

I tried this approach ⬆️ in 0b1ef5b

andybalaam

Some suggestions, all optional. Looks good to me.

andybalaam · 2024-10-16T08:13:50Z

crates/matrix-sdk-base/src/client.rs

@@ -527,30 +527,20 @@ impl BaseClient {
 pub(crate) async fn handle_invited_state(
 &self,
 room: &Room,
- events: &[Raw<AnyStrippedStateEvent>],
+ events: &[(Raw<AnyStrippedStateEvent>, AnyStrippedStateEvent)],


Some documentation of the meaning of this parameter would be appreciated. It's a bit cryptic at the moment: a slice of pairs of things that look the same at first glance.

andybalaam · 2024-10-16T08:20:26Z

crates/matrix-sdk-base/src/sliding_sync/mod.rs

@@ -1042,6 +1090,76 @@ mod tests {
 assert_eq!(client_room.compute_display_name().await.unwrap().to_string(), "The Name");
 }

+ #[async_test]
+ async fn test_knocked_room_membership_event() {


test_receiving_a_knocked_room_membership_event_creates_a_knocked_room ?

andybalaam · 2024-10-16T08:20:53Z

crates/matrix-sdk-base/src/sliding_sync/mod.rs

+ }
+
+ #[async_test]
+ async fn test_knocked_room_membership_event_with_wrong_state_key() {


test_receiving_a_knocked_room_membership_event_with_wrong_state_key_creates_an_invited_room ?

andybalaam · 2024-10-16T08:22:01Z

crates/matrix-sdk-base/src/sliding_sync/mod.rs

+ }
+
+ #[async_test]
+ async fn test_unknown_room_membership_event_in_invite_state() {


test_receiving_an_unknown_room_membership_event_in_invite_state_creates_an_invited_room ?

jmartinesp · 2024-10-16T09:05:42Z

Thanks for the feedback @andybalaam , I'll double check with @bnjbvr if these changes are what we want for the feature 🤞 .

bnjbvr

Thanks, LGTM! only minor nits, so unless something big comes up, feel free to address these (or not) and merge. Good job!

bnjbvr · 2024-10-16T13:21:05Z

crates/matrix-sdk-base/src/client.rs

+ .filter_map(|raw_event| match raw_event.deserialize() {
+ Ok(event) => Some((raw_event.clone(), event)),
+ Err(e) => {
+ warn!("Couldn't deserialize state event: {e}");


Suggested change

warn!("Couldn't deserialize state event: {e}");

warn!("Couldn't deserialize stripped state event: {e}");

bnjbvr · 2024-10-16T13:22:00Z

crates/matrix-sdk-base/src/sliding_sync/mod.rs

+ Option<JoinedRoomUpdate>,
+ Option<LeftRoomUpdate>,
+ Option<InvitedRoom>,
+ Option<KnockedRoom>,


(god what have we done here 😨)(no need to change anything right now, unless you see an obvious better way here)

bnjbvr · 2024-10-16T13:22:31Z

crates/matrix-sdk-base/src/sliding_sync/mod.rs

@@ -428,7 +457,8 @@ impl BaseClient {

 let push_rules = self.get_push_rules(account_data_processor).await?;

- if let Some(invite_state) = &room_data.invite_state {
+ // This will be used for both invited and knocked rooms


uber-nit:

Suggested change

// This will be used for both invited and knocked rooms

// This will be used for both invited and knocked rooms.

bnjbvr · 2024-10-16T13:23:36Z

crates/matrix-sdk-base/src/sliding_sync/mod.rs

 state_events: &[AnySyncStateEvent],
+ invite_state: &Option<Vec<(Raw<AnyStrippedStateEvent>, AnyStrippedStateEvent)>>,


nit: in Rust it's more idiomatic to pass Option<&T> rather than &Option<T>, by calling as_ref() at the caller's call-site.

Alternatively, you could make this non Optional and have the caller .unwrap_or_default().

Could we name this stripped_state, though, since it applies to both invites and knocked rooms?

bnjbvr · 2024-10-16T13:26:12Z

testing/matrix-sdk-test/src/test_json/sync.rs

+ "sender": "@alice:example.com",
+ "type": "m.room.member",
+ "state_key": "@bob:example.com",


Can someone knock on behalf of somebody else? 🤔 Should the sender/state_key be the same?

Ooops. Copy&paste issue 😅 .

jmartinesp requested a review from a team as a code owner October 15, 2024 15:06

jmartinesp requested review from andybalaam and removed request for a team October 15, 2024 15:06

jmartinesp force-pushed the feat/add-knocked-rooms-sync branch from 73bf2f4 to 383b09d Compare October 15, 2024 15:17

bnjbvr reviewed Oct 15, 2024

View reviewed changes

andybalaam approved these changes Oct 16, 2024

View reviewed changes

jmartinesp force-pushed the feat/add-knocked-rooms-sync branch from 687166e to 59aaf22 Compare October 16, 2024 08:53

jmartinesp requested a review from bnjbvr October 16, 2024 09:04

bnjbvr approved these changes Oct 16, 2024

View reviewed changes

feat(knocking): add code to process knocked rooms separately during sync

8636c3a

jmartinesp force-pushed the feat/add-knocked-rooms-sync branch from 59aaf22 to 8636c3a Compare October 16, 2024 13:55

jmartinesp merged commit 664f6d5 into main Oct 16, 2024
40 checks passed

jmartinesp deleted the feat/add-knocked-rooms-sync branch October 16, 2024 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(knocking): process knocked rooms separately during sync #4128

feat(knocking): process knocked rooms separately during sync #4128

jmartinesp commented Oct 15, 2024

jmartinesp commented Oct 15, 2024

codecov bot commented Oct 15, 2024 •

edited

Loading

bnjbvr left a comment

bnjbvr Oct 15, 2024

jmartinesp Oct 16, 2024

bnjbvr Oct 15, 2024

jmartinesp Oct 16, 2024

jmartinesp Oct 16, 2024

bnjbvr Oct 15, 2024

jmartinesp Oct 16, 2024

bnjbvr Oct 15, 2024

jmartinesp Oct 16, 2024

jmartinesp Oct 16, 2024

jmartinesp Oct 16, 2024

jmartinesp Oct 16, 2024

jmartinesp Oct 16, 2024

andybalaam left a comment

andybalaam Oct 16, 2024

andybalaam Oct 16, 2024

andybalaam Oct 16, 2024

andybalaam Oct 16, 2024

jmartinesp commented Oct 16, 2024

bnjbvr left a comment

bnjbvr Oct 16, 2024

bnjbvr Oct 16, 2024

bnjbvr Oct 16, 2024

bnjbvr Oct 16, 2024

bnjbvr Oct 16, 2024

bnjbvr Oct 16, 2024

jmartinesp Oct 16, 2024

	warn!("Couldn't deserialize state event: {e}");
	warn!("Couldn't deserialize stripped state event: {e}");

	// This will be used for both invited and knocked rooms
	// This will be used for both invited and knocked rooms.

		state_events: &[AnySyncStateEvent],
		invite_state: &Option<Vec<(Raw<AnyStrippedStateEvent>, AnyStrippedStateEvent)>>,

feat(knocking): process knocked rooms separately during sync #4128

feat(knocking): process knocked rooms separately during sync #4128

Conversation

jmartinesp commented Oct 15, 2024

Changes

jmartinesp commented Oct 15, 2024

codecov bot commented Oct 15, 2024 • edited Loading

Codecov Report

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andybalaam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp commented Oct 16, 2024

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 15, 2024 •

edited

Loading