Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(event cache): redact previously-seen events in the linked chunk too #4536

Merged
merged 4 commits into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions crates/matrix-sdk-base/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ mod rooms;

pub mod read_receipts;
pub use read_receipts::PreviousEventsProvider;
pub use rooms::RoomMembersUpdate;
pub mod sliding_sync;

pub mod store;
Expand All @@ -56,9 +55,9 @@ pub use http;
pub use matrix_sdk_crypto as crypto;
pub use once_cell;
pub use rooms::{
Room, RoomCreateWithCreatorEventContent, RoomDisplayName, RoomHero, RoomInfo,
RoomInfoNotableUpdate, RoomInfoNotableUpdateReasons, RoomMember, RoomMemberships, RoomState,
RoomStateFilter,
apply_redaction, Room, RoomCreateWithCreatorEventContent, RoomDisplayName, RoomHero, RoomInfo,
RoomInfoNotableUpdate, RoomInfoNotableUpdateReasons, RoomMember, RoomMembersUpdate,
RoomMemberships, RoomState, RoomStateFilter,
};
pub use store::{
ComposerDraft, ComposerDraftType, QueueWedgeError, StateChanges, StateStore, StateStoreDataKey,
Expand Down
2 changes: 1 addition & 1 deletion crates/matrix-sdk-base/src/rooms/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use std::{
use bitflags::bitflags;
pub use members::RoomMember;
pub use normal::{
Room, RoomHero, RoomInfo, RoomInfoNotableUpdate, RoomInfoNotableUpdateReasons,
apply_redaction, Room, RoomHero, RoomInfo, RoomInfoNotableUpdate, RoomInfoNotableUpdateReasons,
RoomMembersUpdate, RoomState, RoomStateFilter,
};
use regex::Regex;
Expand Down
6 changes: 4 additions & 2 deletions crates/matrix-sdk-base/src/rooms/normal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2007,7 +2007,9 @@ impl RoomInfo {
}
}

fn apply_redaction(
/// Apply a redaction to the given target `event`, given the raw redaction event
/// and the room version.
pub fn apply_redaction(
event: &Raw<AnySyncTimelineEvent>,
raw_redaction: &Raw<SyncRoomRedactionEvent>,
room_version: &RoomVersionId,
Expand All @@ -2033,7 +2035,7 @@ fn apply_redaction(
let redact_result = redact_in_place(&mut event_json, room_version, Some(redacted_because));

if let Err(e) = redact_result {
warn!("Failed to redact latest event: {e}");
warn!("Failed to redact event: {e}");
return None;
}

Expand Down
13 changes: 13 additions & 0 deletions crates/matrix-sdk-common/src/deserialized_responses.rs
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,19 @@ impl SyncTimelineEvent {
self.kind.raw()
}

/// Replace the raw event included in this item by another one.
pub fn replace_raw(&mut self, replacement: Raw<AnyMessageLikeEvent>) {
match &mut self.kind {
TimelineEventKind::Decrypted(decrypted) => decrypted.event = replacement,
TimelineEventKind::UnableToDecrypt { event, .. }
| TimelineEventKind::PlainText { event } => {
// It's safe to cast `AnyMessageLikeEvent` into `AnySyncMessageLikeEvent`,
// because the former contains a superset of the fields included in the latter.
*event = replacement.cast();
}
}
}

/// If the event was a decrypted event that was successfully decrypted, get
/// its encryption info. Otherwise, `None`.
pub fn encryption_info(&self) -> Option<&EncryptionInfo> {
Expand Down
43 changes: 34 additions & 9 deletions crates/matrix-sdk-common/src/linked_chunk/as_vector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,14 @@ impl UpdateToVectorDiff {
}
}

Update::ReplaceItem { at: position, item } => {
let (offset, (_chunk_index, _chunk_length)) = self.map_to_offset(position);

// The chunk length doesn't change.

diffs.push(VectorDiff::Set { index: offset, value: item.clone() });
}

Update::RemoveItem { at: position } => {
let (offset, (_chunk_index, chunk_length)) = self.map_to_offset(position);

Expand Down Expand Up @@ -484,15 +492,7 @@ mod tests {
assert_eq!(diffs, expected_diffs);

for diff in diffs {
match diff {
VectorDiff::Insert { index, value } => accumulator.insert(index, value),
VectorDiff::Append { values } => accumulator.append(values),
VectorDiff::Remove { index } => {
accumulator.remove(index);
}
VectorDiff::Clear => accumulator.clear(),
diff => unimplemented!("{diff:?}"),
}
diff.apply(accumulator);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}
}

Expand Down Expand Up @@ -710,6 +710,31 @@ mod tests {
vector!['m', 'a', 'w', 'x', 'y', 'b', 'd', 'i', 'j', 'k', 'l', 'e', 'f', 'g', 'z', 'h']
);

// Replace element 8 by an uppercase J.
linked_chunk
.replace_item_at(linked_chunk.item_position(|item| *item == 'j').unwrap(), 'J')
.unwrap();

assert_items_eq!(
linked_chunk,
['m', 'a', 'w'] ['x'] ['y', 'b'] ['d'] ['i', 'J', 'k'] ['l'] ['e', 'f', 'g'] ['z', 'h']
);

// From an `ObservableVector` point of view, it would look like:
//
// 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16
// +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
// | m | a | w | x | y | b | d | i | J | k | l | e | f | g | z | h |
// +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
// ^^^^
// |
// new!
apply_and_assert_eq(
&mut accumulator,
as_vector.take(),
&[VectorDiff::Set { index: 8, value: 'J' }],
);

// Let's try to clear the linked chunk now.
linked_chunk.clear();

Expand Down
67 changes: 67 additions & 0 deletions crates/matrix-sdk-common/src/linked_chunk/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,47 @@ impl<const CAP: usize, Item, Gap> LinkedChunk<CAP, Item, Gap> {
Ok(removed_item)
}

/// Replace item at a specified position in the [`LinkedChunk`].
///
/// `position` must point to a valid item, otherwise the method returns
/// `Err`.
pub fn replace_item_at(&mut self, position: Position, item: Item) -> Result<(), Error>
where
Item: Clone,
{
let chunk_identifier = position.chunk_identifier();
let item_index = position.index();

let chunk = self
.links
.chunk_mut(chunk_identifier)
.ok_or(Error::InvalidChunkIdentifier { identifier: chunk_identifier })?;

match &mut chunk.content {
ChunkContent::Gap(..) => {
return Err(Error::ChunkIsAGap { identifier: chunk_identifier })
}

ChunkContent::Items(current_items) => {
if item_index >= current_items.len() {
return Err(Error::InvalidItemIndex { index: item_index });
}

// Avoid one spurious clone by notifying about the update *before* applying it.
if let Some(updates) = self.updates.as_mut() {
updates.push(Update::ReplaceItem {
at: Position(chunk_identifier, item_index),
item: item.clone(),
});
}
Comment on lines +558 to +563
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Push the update after current_items[item_index] = item;: do the action before triggering the update.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means we have to clone before, and the original thing is unused if self.updates.as_mut().is_none(). Plus, the updates aren't reflected in real-time, they're buffered until they're consumed, as far as I can tell? So this should be fine to keep as is. I can add a comment explaining that this avoids a clone if you'd like.


current_items[item_index] = item;
}
}

Ok(())
}

/// Insert a gap at a specified position in the [`LinkedChunk`].
///
/// Because the `position` can be invalid, this method returns a
Expand Down Expand Up @@ -2919,4 +2960,30 @@ mod tests {
]
);
}

#[test]
fn test_replace_item() {
let mut linked_chunk = LinkedChunk::<3, char, ()>::new();

linked_chunk.push_items_back(['a', 'b', 'c']);
linked_chunk.push_gap_back(());
// Sanity check.
assert_items_eq!(linked_chunk, ['a', 'b', 'c'] [-]);

// Replace item in bounds.
linked_chunk.replace_item_at(Position(ChunkIdentifier::new(0), 1), 'B').unwrap();
assert_items_eq!(linked_chunk, ['a', 'B', 'c'] [-]);

// Attempt to replace out-of-bounds.
assert_matches!(
linked_chunk.replace_item_at(Position(ChunkIdentifier::new(0), 3), 'Z'),
Err(Error::InvalidItemIndex { index: 3 })
);

// Attempt to replace gap.
assert_matches!(
linked_chunk.replace_item_at(Position(ChunkIdentifier::new(1), 0), 'Z'),
Err(Error::ChunkIsAGap { .. })
);
}
}
64 changes: 64 additions & 0 deletions crates/matrix-sdk-common/src/linked_chunk/relational.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,19 @@ impl<Item, Gap> RelationalLinkedChunk<Item, Gap> {
}
}

Update::ReplaceItem { at, item } => {
let existing = self
.items
.iter_mut()
.find(|item| item.position == at)
.expect("trying to replace at an unknown position");
assert!(
matches!(existing.item, Either::Item(..)),
"trying to replace a gap with an item"
);
existing.item = Either::Item(item);
}

Update::RemoveItem { at } => {
let mut entry_to_remove = None;

Expand Down Expand Up @@ -938,4 +951,55 @@ mod tests {
// The linked chunk is correctly reloaded.
assert_items_eq!(lc, ['a', 'b', 'c'] [-] ['d', 'e', 'f']);
}

#[test]
fn test_replace_item() {
let room_id = room_id!("!r0:matrix.org");
let mut relational_linked_chunk = RelationalLinkedChunk::<char, ()>::new();

relational_linked_chunk.apply_updates(
room_id,
vec![
// new chunk (this is not mandatory for this test, but let's try to be realistic)
Update::NewItemsChunk { previous: None, new: CId::new(0), next: None },
// new items on 0
Update::PushItems { at: Position::new(CId::new(0), 0), items: vec!['a', 'b', 'c'] },
// update item at (0; 1).
Update::ReplaceItem { at: Position::new(CId::new(0), 1), item: 'B' },
],
);

// Chunks are correctly linked.
assert_eq!(
relational_linked_chunk.chunks,
&[ChunkRow {
room_id: room_id.to_owned(),
previous_chunk: None,
chunk: CId::new(0),
next_chunk: None,
},],
);

// Items contains the pushed *and* replaced items.
assert_eq!(
relational_linked_chunk.items,
&[
ItemRow {
room_id: room_id.to_owned(),
position: Position::new(CId::new(0), 0),
item: Either::Item('a')
},
ItemRow {
room_id: room_id.to_owned(),
position: Position::new(CId::new(0), 1),
item: Either::Item('B')
},
ItemRow {
room_id: room_id.to_owned(),
position: Position::new(CId::new(0), 2),
item: Either::Item('c')
},
],
);
}
}
12 changes: 12 additions & 0 deletions crates/matrix-sdk-common/src/linked_chunk/updates.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,18 @@ pub enum Update<Item, Gap> {
items: Vec<Item>,
},

/// An item has been replaced in the linked chunk.
///
/// The `at` position MUST resolve to the actual position an existing *item*
/// (not a gap).
ReplaceItem {
bnjbvr marked this conversation as resolved.
Show resolved Hide resolved
/// The position of the item that's being replaced.
at: Position,

/// The new value for the item.
item: Item,
},

/// An item has been removed inside a chunk of kind Items.
RemoveItem {
/// The [`Position`] of the item.
Expand Down
68 changes: 68 additions & 0 deletions crates/matrix-sdk-sqlite/src/event_cache_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,28 @@ impl EventCacheStore for SqliteEventCacheStore {
}
}

Update::ReplaceItem { at, item: event } => {
let chunk_id = at.chunk_identifier().index();
let index = at.index();

trace!(%room_id, "replacing item @ {chunk_id}:{index}");

let serialized = serde_json::to_vec(&event)?;
let content = this.encode_value(serialized)?;

// The event id should be the same, but just in case it changed…
let event_id = event.event_id().map(|event_id| event_id.to_string());

txn.execute(
r#"
UPDATE events
SET content = ?, event_id = ?
WHERE room_id = ? AND chunk_id = ? AND position = ?
"#,
(content, event_id, &hashed_room_id, chunk_id, index,)
)?;
}

Update::RemoveItem { at } => {
let chunk_id = at.chunk_identifier().index();
let index = at.index();
Expand Down Expand Up @@ -978,6 +1000,52 @@ mod tests {
});
}

#[async_test]
async fn test_linked_chunk_replace_item() {
let store = get_event_cache_store().await.expect("creating cache store failed");

let room_id = &DEFAULT_TEST_ROOM_ID;

store
.handle_linked_chunk_updates(
room_id,
vec![
Update::NewItemsChunk {
previous: None,
new: ChunkIdentifier::new(42),
next: None,
},
Update::PushItems {
at: Position::new(ChunkIdentifier::new(42), 0),
items: vec![
make_test_event(room_id, "hello"),
make_test_event(room_id, "world"),
],
},
Update::ReplaceItem {
at: Position::new(ChunkIdentifier::new(42), 1),
item: make_test_event(room_id, "yolo"),
},
],
)
.await
.unwrap();

let mut chunks = store.reload_linked_chunk(room_id).await.unwrap();

assert_eq!(chunks.len(), 1);

let c = chunks.remove(0);
assert_eq!(c.identifier, ChunkIdentifier::new(42));
assert_eq!(c.previous, None);
assert_eq!(c.next, None);
assert_matches!(c.content, ChunkContent::Items(events) => {
assert_eq!(events.len(), 2);
check_test_event(&events[0], "hello");
check_test_event(&events[1], "yolo");
});
}

#[async_test]
async fn test_linked_chunk_remove_chunk() {
let store = get_event_cache_store().await.expect("creating cache store failed");
Expand Down
Loading
Loading