From 03e53e991b9ca44087486aab663cc640fe147e7d Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Mon, 15 Jun 2020 17:04:10 +0200 Subject: [PATCH] Hoist prev_content to top-level in both timeline and state events. Also refactor and document why this hoisting is needed. This change makes the user_presence test fail because the hoisting exposes an error encoded into the test's expected result. Previously, the test expected 2 members in the room at the end. This is incorrect since one of the members in the test data leaves the room. However, since the prev_content of state events was previously not hoisted to the top level, the `membership_change` method would not notice it and thus not realize the member had left the room. The test was corrected to expect only a single member in the room. Another test change was made due to a limitation of EventBuilder: due to the fact that it makes the test data go through a de/ser cycle, it cannot easily hoist prev_content to the top level. Because of this, the tests were change to put prev_content into the top level from the outset. --- matrix_sdk/src/client.rs | 2 +- matrix_sdk_base/src/client.rs | 70 ++++++++++++++++++++++++------ matrix_sdk_base/src/models/room.rs | 2 +- 3 files changed, 58 insertions(+), 16 deletions(-) diff --git a/matrix_sdk/src/client.rs b/matrix_sdk/src/client.rs index 5c3380a4..5dd87ff1 100644 --- a/matrix_sdk/src/client.rs +++ b/matrix_sdk/src/client.rs @@ -1919,7 +1919,7 @@ mod test { .read() .await; - assert_eq!(2, room.joined_members.len()); + assert_eq!(1, room.joined_members.len()); assert!(room.power_levels.is_some()) } diff --git a/matrix_sdk_base/src/client.rs b/matrix_sdk_base/src/client.rs index 92cbf4a7..d644235b 100644 --- a/matrix_sdk_base/src/client.rs +++ b/matrix_sdk_base/src/client.rs @@ -82,8 +82,15 @@ pub struct AdditionalUnsignedData { pub prev_content: Option>, } -/// If a `prev_content` field is found inside of `unsigned` we move it up to the events `prev_content` field. -fn deserialize_prev_content(event: &EventJson) -> Option> { +/// Transform room event by hoisting `prev_content` field from `unsigned` to the top level. +/// +/// Due to a [bug in synapse][synapse-bug], `prev_content` often ends up in `unsigned` contrary to +/// the C2S spec. Some more discussion can be found [here][discussion]. Until this is fixed in +/// synapse or handled in Ruma, we use this to hoist up `prev_content` to the top level. +/// +/// [synapse-bug]: +/// [discussion]: +fn hoist_room_event_prev_content(event: &mut EventJson) -> Option> { let prev_content = serde_json::from_str::(event.json().get()) .map(|more_unsigned| more_unsigned.unsigned) .map(|additional| additional.prev_content) @@ -93,7 +100,33 @@ fn deserialize_prev_content(event: &EventJson) -> Option { - member.prev_content = prev_content.deserialize().ok(); + if let Some(prev) = prev_content.deserialize().ok() { + member.prev_content = Some(prev) + } + + Some(EventJson::from(ev)) + } + _ => None, + } +} + +/// Transform state event by hoisting `prev_content` field from `unsigned` to the top level. +/// +/// See comment of `hoist_room_event_prev_content`. +fn hoist_state_event_prev_content(event: &EventJson) -> Option> { + let prev_content = serde_json::from_str::(event.json().get()) + .map(|more_unsigned| more_unsigned.unsigned) + .map(|additional| additional.prev_content) + .ok() + .flatten()?; + + let mut ev = event.deserialize().ok()?; + match &mut ev { + StateEvent::RoomMember(ref mut member) if member.prev_content.is_none() => { + if let Some(prev) = prev_content.deserialize().ok() { + member.prev_content = Some(prev) + } + Some(EventJson::from(ev)) } _ => None, @@ -675,13 +708,6 @@ impl BaseClient { room_id: &RoomId, event: &mut EventJson, ) -> Result<(Option>, bool)> { - // if the event is a m.room.member event the server will sometimes - // send the `prev_content` field as part of the unsigned field this extracts and - // places it where everything else expects it. - if let Some(e) = deserialize_prev_content(event) { - *event = e; - } - match event.deserialize() { #[allow(unused_mut)] Ok(mut e) => { @@ -941,7 +967,13 @@ impl BaseClient { let mut updated = false; for (room_id, joined_room) in &mut response.rooms.join { let matrix_room = { - for event in &joined_room.state.events { + for event in &mut joined_room.state.events { + // XXX: Related to `prev_content` and `unsigned`; see the doc comment of + // `hoist_room_event_prev_content` + if let Some(e) = hoist_state_event_prev_content(event) { + *event = e; + } + if let Ok(e) = event.deserialize() { if self.receive_joined_state_event(&room_id, &e).await? { updated = true; @@ -996,7 +1028,9 @@ impl BaseClient { *event = e; } - if let Some(e) = deserialize_prev_content(&event) { + // XXX: Related to `prev_content` and `unsigned`; see the doc comment of + // `hoist_room_event_prev_content` + if let Some(e) = hoist_room_event_prev_content(event) { *event = e; } @@ -1071,7 +1105,13 @@ impl BaseClient { let mut updated = false; for (room_id, left_room) in &mut response.rooms.leave { let matrix_room = { - for event in &left_room.state.events { + for event in &mut left_room.state.events { + // XXX: Related to `prev_content` and `unsigned`; see the doc comment of + // `hoist_room_event_prev_content` + if let Some(e) = hoist_state_event_prev_content(event) { + *event = e; + } + if let Ok(e) = event.deserialize() { if self.receive_left_state_event(&room_id, &e).await? { updated = true; @@ -1090,7 +1130,9 @@ impl BaseClient { } for event in &mut left_room.timeline.events { - if let Some(e) = deserialize_prev_content(&event) { + // XXX: Related to `prev_content` and `unsigned`; see the doc comment of + // `hoist_room_event_prev_content` + if let Some(e) = hoist_room_event_prev_content(event) { *event = e; } diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index a2a82260..c98a9843 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -793,7 +793,7 @@ mod test { .read() .await; - assert_eq!(2, room.joined_members.len()); + assert_eq!(1, room.joined_members.len()); assert!(room.deref().power_levels.is_some()) }