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.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4740 +/- ##
==========================================
- Coverage 86.14% 86.11% -0.04%
==========================================
Files 292 292
Lines 34295 34309 +14
==========================================
+ Hits 29544 29545 +1
- Misses 4751 4764 +13 ☔ 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).
There was a problem hiding this comment.
Okay. I've added a new commit (happy to squash it) that removes EventsPostProcessing and updates the documentation of with_events_mut.
| @@ -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![Update::ReplaceItem { | |||
There was a problem hiding this comment.
Out of curiosity, why to include the fix to your own previous commit as a separate commit?
There was a problem hiding this comment.
I wanted to go step-by-step: first the refactoring, then the fix (that's only possible because of the refactoring). Glad to merge these 2 patches in a single one when review will be finished.
…`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.
…onId`. This patch updates `maybe_apply_new_redaction` to remove the first `&RoomVersionId` argument. Indeed, due to the refactoring, it's now possible for `maybe_apply_new_redaction` to read this value directly from `Self::room_version`.
This patch removes the `EventsPostProcessing` type, it assumes `with_events_muts` will always return events that will be post-process. The case where `EventsPostProcessing::None` becomes a `vec![]`.
Hywan
force-pushed
the
fix-sdk-event-cache-maybe_apply_new_redaction
branch
from
13f9f6c to
9dc7a37
Compare
|
I forgot to add the test… here it is, #4760! |
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