fix(sdk): Better handling of redacted and redaction events in Latest Event (#5932)

This patch revisits the way redacted and redaction events are handled in
the Latest Event.

Previously, all redacted events were considered suitable candidate. It's
no longer the case.

Redaction and redacted events are no longer considered suitable.

This patch also revisits `rfind_map_event_in_memory_by` to return a
`&TimelineEvent` instead of an `OwnedEventId`, which could be more
performant in the future.

The tests have been updated accordingly.

---

* Fix #5899
* Address #4112

Signed-off-by: Stefan Ceriu <[email protected]>
Co-authored-by: Stefan Ceriu <[email protected]>

Author: Hywan

crates/matrix-sdk/src/event_cache/room/mod.rs

@@ -334,7 +334,7 @@ impl RoomEventCache {
     /// chunk **only**. It doesn't look inside the storage.
     pub async fn rfind_map_event_in_memory_by<O, P>(&self, predicate: P) -> Result<Option<O>>
     where
-        P: FnMut(&Event, Option<OwnedEventId>) -> Option<O>,
+        P: FnMut(&Event, Option<&Event>) -> Option<O>,
     {
         Ok(self.inner.state.read().await?.rfind_map_event_in_memory_by(predicate))
     }
@@ -1170,24 +1170,20 @@ mod private {
         /// **Warning**! It looks into the loaded events from the in-memory
         /// linked chunk **only**. It doesn't look inside the storage,
         /// contrary to [`Self::find_event`].
-        pub fn rfind_map_event_in_memory_by<O, P>(&self, mut predicate: P) -> Option<O>
+        pub fn rfind_map_event_in_memory_by<'i, O, P>(&'i self, mut predicate: P) -> Option<O>
         where
-            P: FnMut(&Event, Option<OwnedEventId>) -> Option<O>,
+            P: FnMut(&'i Event, Option<&'i Event>) -> Option<O>,
         {
             self.state
                 .room_linked_chunk
                 .revents()
                 .peekable()
                 .batching(|iter| {
                     iter.next().map(|(_position, event)| {
-                        (
-                            event,
-                            iter.peek()
-                                .and_then(|(_next_position, next_event)| next_event.event_id()),
-                        )
+                        (event, iter.peek().map(|(_next_position, next_event)| *next_event))
                     })
                 })
-                .find_map(|(event, next_event_id)| predicate(event, next_event_id))
+                .find_map(|(event, next_event)| predicate(event, next_event))
         }
 
         #[cfg(test)]
@@ -3911,8 +3907,8 @@ mod timed_tests {
         // Look for an event from `BOB`: it must be `event_0`.
         assert_matches!(
             room_event_cache
-                .rfind_map_event_in_memory_by(|event, previous_event_id| {
-                    (event.raw().get_field::<OwnedUserId>("sender").unwrap().as_deref() == Some(*BOB)).then(|| (event.event_id(), previous_event_id))
+                .rfind_map_event_in_memory_by(|event, previous_event| {
+                    (event.raw().get_field::<OwnedUserId>("sender").unwrap().as_deref() == Some(*BOB)).then(|| (event.event_id(), previous_event.and_then(|event| event.event_id())))
                 })
                 .await,
             Ok(Some((event_id, previous_event_id))) => {
@@ -3925,8 +3921,8 @@ mod timed_tests {
         // because events are looked for in reverse order.
         assert_matches!(
             room_event_cache
-                .rfind_map_event_in_memory_by(|event, previous_event_id| {
-                    (event.raw().get_field::<OwnedUserId>("sender").unwrap().as_deref() == Some(*ALICE)).then(|| (event.event_id(), previous_event_id))
+                .rfind_map_event_in_memory_by(|event, previous_event| {
+                    (event.raw().get_field::<OwnedUserId>("sender").unwrap().as_deref() == Some(*ALICE)).then(|| (event.event_id(), previous_event.and_then(|event| event.event_id())))
                 })
                 .await,
             Ok(Some((event_id, previous_event_id))) => {

crates/matrix-sdk/src/latest_events/latest_event.rs

@@ -32,7 +32,7 @@ use ruma::{
         relation::Replacement,
         room::{
             member::MembershipState,
-            message::{MessageType, Relation},
+            message::{MessageType, Relation, RoomMessageEventContent},
             power_levels::RoomPowerLevels,
         },
     },
@@ -135,15 +135,16 @@ impl LatestEvent {
         self.update(new_value).await;
     }
 
-    /// Update [`Self::current_value`] if and only if the `new_value` is not
-    /// [`LatestEventValue::None`].
+    /// Update [`Self::current_value`], and persist the `new_value` in the
+    /// store.
+    ///
+    /// If the `new_value` is [`LatestEventValue::None`], it is accepted: if the
+    /// previous latest event value has been redacted and no other candidate has
+    /// been found, we want to latest event value to be `None`, so that it is
+    /// erased correctly.
     async fn update(&mut self, new_value: LatestEventValue) {
-        if let LatestEventValue::None = new_value {
-            // Do not update to a `None` value.
-        } else {
-            self.current_value.set(new_value.clone()).await;
-            self.store(new_value).await;
-        }
+        self.current_value.set(new_value.clone()).await;
+        self.store(new_value).await;
     }
 
     /// Update the `RoomInfo` associated to this room to set the new
@@ -232,7 +233,7 @@ mod tests_latest_event {
     }
 
     #[async_test]
-    async fn test_update_ignores_none_value() {
+    async fn test_update_do_not_ignore_none_value() {
         let room_id = room_id!("!r0");
 
         let server = MatrixMockServer::new().await;
@@ -262,13 +263,10 @@ mod tests_latest_event {
             LatestEventValue::LocalIsSending(_)
         );
 
-        // Finally, set a new `None` value. It must be ignored.
+        // Finally, set a new `None` value. It must NOT be ignored.
         latest_event.update(LatestEventValue::None).await;
 
-        assert_matches!(
-            latest_event.current_value.get().await,
-            LatestEventValue::LocalIsSending(_)
-        );
+        assert_matches!(latest_event.current_value.get().await, LatestEventValue::None);
     }
 
     #[async_test]
@@ -952,7 +950,7 @@ impl LatestEventValuesForLocalEvents {
 
 fn filter_timeline_event(
     event: &TimelineEvent,
-    previous_event_id: Option<OwnedEventId>,
+    previous_event: Option<&TimelineEvent>,
     own_user_id: Option<&UserId>,
     power_levels: Option<&RoomPowerLevels>,
 ) -> bool {
@@ -975,11 +973,11 @@ fn filter_timeline_event(
             match message_like_event.original_content() {
                 Some(any_message_like_event_content) => filter_any_message_like_event_content(
                     any_message_like_event_content,
-                    previous_event_id,
+                    previous_event,
                 ),
 
                 // The event has been redacted.
-                None => true,
+                None => false,
             }
         }
 
@@ -991,34 +989,48 @@ fn filter_timeline_event(
 
 fn filter_any_message_like_event_content(
     event: AnyMessageLikeEventContent,
-    previous_event_id: Option<OwnedEventId>,
+    previous_event: Option<&TimelineEvent>,
 ) -> bool {
     match event {
-        AnyMessageLikeEventContent::RoomMessage(message) => {
+        // `m.room.message`
+        AnyMessageLikeEventContent::RoomMessage(RoomMessageEventContent {
+            msgtype,
+            relates_to,
+            ..
+        }) => {
             // Don't show incoming verification requests.
-            if let MessageType::VerificationRequest(_) = message.msgtype {
+            if let MessageType::VerificationRequest(_) = msgtype {
                 return false;
             }
 
             // Not all relations are accepted. Let's filter them.
-            match &message.relates_to {
+            match relates_to {
                 Some(Relation::Replacement(Replacement { event_id, .. })) => {
                     // If the edit relates to the immediate previous event, this is an acceptable
-                    // latest event, otherwise let's ignore it.
-                    Some(event_id) == previous_event_id.as_ref()
+                    // latest event candidate, otherwise let's ignore it.
+                    Some(event_id) == previous_event.and_then(|event| event.event_id())
                 }
 
                 _ => true,
             }
         }
 
+        // `org.matrix.msc3381.poll.start`
+        // `m.call.invite`
+        // `m.rtc.notification`
+        // `m.sticker`
         AnyMessageLikeEventContent::UnstablePollStart(_)
         | AnyMessageLikeEventContent::CallInvite(_)
         | AnyMessageLikeEventContent::RtcNotification(_)
         | AnyMessageLikeEventContent::Sticker(_) => true,
 
-        // Encrypted events are not suitable.
-        AnyMessageLikeEventContent::RoomEncrypted(_) => false,
+        // `m.room.redaction`
+        // `m.room.encrypted`
+        AnyMessageLikeEventContent::RoomRedaction(_)
+        | AnyMessageLikeEventContent::RoomEncrypted(_) => {
+            // These events are **explicitly** not suitable.
+            false
+        }
 
         // Everything else is considered not suitable.
         _ => false,
@@ -1119,21 +1131,6 @@ mod tests_latest_event_content {
         );
     }
 
-    #[test]
-    fn test_redacted() {
-        assert_latest_event_content!(
-            event | event_factory | {
-                event_factory
-                    .redacted(
-                        user_id!("@mnt_io:matrix.org"),
-                        ruma::events::room::message::RedactedRoomMessageEventContent::new(),
-                    )
-                    .into_event()
-            }
-            is a candidate
-        );
-    }
-
     #[test]
     fn test_room_message_replacement() {
         let user_id = user_id!("@mnt_io:matrix.org");
@@ -1152,45 +1149,52 @@ mod tests_latest_event_content {
         {
             let previous_event_id = None;
 
-            assert!(
-                filter_timeline_event(
-                    &event,
-                    previous_event_id,
-                    Some(user_id!("@mnt_io:matrix.org")),
-                    None
-                )
-                .not()
-            );
+            assert!(filter_timeline_event(&event, previous_event_id, Some(user_id), None).not());
         }
 
-        // With a previous event, but the one being replaced.
+        // With a previous event, but not the one being replaced.
         {
-            let previous_event_id = Some(event_id!("$ev1").to_owned());
+            let previous_event =
+                Some(event_factory.text_msg("no!").event_id(event_id!("$ev1")).into_event());
 
             assert!(
-                filter_timeline_event(
-                    &event,
-                    previous_event_id,
-                    Some(user_id!("@mnt_io:matrix.org")),
-                    None
-                )
-                .not()
+                filter_timeline_event(&event, previous_event.as_ref(), Some(user_id), None).not()
             );
         }
 
         // With a previous event, and that's the one being replaced!
         {
-            let previous_event_id = Some(event_id!("$ev0").to_owned());
-
-            assert!(filter_timeline_event(
-                &event,
-                previous_event_id,
-                Some(user_id!("@mnt_io:matrix.org")),
-                None
-            ));
+            let previous_event =
+                Some(event_factory.text_msg("hello").event_id(event_id!("$ev0")).into_event());
+
+            assert!(filter_timeline_event(&event, previous_event.as_ref(), Some(user_id), None));
         }
     }
 
+    #[test]
+    fn test_redaction() {
+        assert_latest_event_content!(
+            event | event_factory | { event_factory.redaction(event_id!("$ev0")).into_event() }
+            is not a candidate
+        );
+    }
+
+    #[test]
+    fn test_redacted() {
+        assert_latest_event_content!(
+            event | event_factory | {
+                event_factory
+                    .redacted(
+                        user_id!("@mnt_io:matrix.org"),
+                        ruma::events::room::message::RedactedRoomMessageEventContent::new(),
+                    )
+                    .event_id(event_id!("$ev0"))
+                    .into_event()
+            }
+            is not a candidate
+        );
+    }
+
     #[test]
     fn test_poll() {
         assert_latest_event_content!(