fix(sdk): maybe_apply_new_redaction updates in-store events
#4740
Conversation
…sing`. This patch updates the callback passed to `with_events_mut`. It now returns an `EventsPostProcessing` which can automatically run the, now inlined, `on_new_events`. This patch updates where the `RoomVersionId` is also stored. It's not held by `RoomEventCacheState` instead of `RoomEventCacheInner`.
…events. This patch renames `sync_timeline_events` into `timeline_events`. Moreover, this change has spotted a possible improvement in `AllEventsCache` where it now receives events from `collect_valid_and_duplicated_events`, which allows to only store valid events in it.
…`RoomEventCacheState`. This patch moves the `maybe_apply_new_redaction` method from `RoomEvents` inside `RoomEventCacheState` so that it has an access to the store (necessary for the next patch). This patch creates a new `RoomEvents::replace_event_at` method, which is a thin wrapper around `LinkedChunk::replace_item_at`.
This patch introduces `EventLocation` to know if an event has been found in the memory (in `RoomEvents`) or in the store (in `EventCacheStore`). This is used by the `RoomEventCacheState::find_event`.
This patch updates `maybe_apply_new_redaction` to use `find_event`, so that the target event is looked up in memory or in the store. The case where it is in the store is a simple `todo!()` for the moment. I wanted to separate the update of the `maybe_apply_new_redaction` signature from the `InStore` implementation. The method is now async and returns a `Result`.
This patch updates `maybe_apply_new_redaction` so that it is able to update/redact an event found in the store.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4740 +/- ##
==========================================
- Coverage 86.14% 86.12% -0.03%
==========================================
Files 292 292
Lines 34295 34313 +18
==========================================
+ Hits 29544 29552 +8
- Misses 4751 4761 +10 ☔ View full report in Codecov by Sentry. |
Hywan
force-pushed
the
fix-sdk-event-cache-maybe_apply_new_redaction
branch
from
cd19fdc to
9d021a8
Compare
Hywan
requested review from
andybalaam and
bnjbvr
and removed request for
a team and
andybalaam
| .expect("failed to remove an event") | ||
| .expect("failed to remove an event"); | ||
|
|
||
| EventsPostProcessing::None |
There was a problem hiding this comment.
Does it change the test result if you let the post-processing happen? I think it'd be simpler to get rid of the variants, and not have the EventsPostProcessing type overall, by having the callback passed to with_events_mut always return the events, and with_events_mut always call on_new_events() (or the inlined version).
| /// Replace event at a specified position. | ||
| /// | ||
| /// `position` must point to a valid item, otherwise the method returns | ||
| /// `Err`. |
There was a problem hiding this comment.
Maybe precise which error, or rewrite as "the method returns an error"? It's a bit weird to just have Err in isolation.
| @@ -1230,8 +1237,10 @@ mod private { | |||
|
|
|||
| match post_processing { | |||
| EventsPostProcessing::HaveBeenInserted(events) => { | |||
| let room_version = self.room_version.clone(); | |||
There was a problem hiding this comment.
The cloning seems unnecessary here. In fact, maybe_apply_new_redaction could read the self.room_version itself, without having to receive it as a parameter.
| InMemory, | ||
| InStore, |
There was a problem hiding this comment.
Can these be called Memory and Store, only? Would be more concise, and the enum's name already indicates it's a location. (Otherwise it's temping to read it as "where is the event location itself?".)
| @@ -1337,7 +1337,11 @@ mod private { | |||
| .expect("should have been a valid position of an item"); | |||
| } | |||
| EventLocation::InStore => { | |||
| todo!() | |||
| self.send_updates_to_store(vec
This is the last known task required after the event cache lazy-loading feature.
This is unblocked by #4708.
maybe_apply_new_redactionwas looking for events only fromRoomEvents, so only in-memory. And the redaction was only happening in-memory too. With this patch,maybe_apply_new_redactionhandles in memory and in store.This pull request should be reviewed patch by patch for the sake of clarity.
A clean up in the
Deduplicatoris possible by using the newEventLocationenum, but it's not part of this set of patches.EventCachestorage #3280