-
Notifications
You must be signed in to change notification settings - Fork 269
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 store): add an IndexedDB implementation #4617
base: main
Are you sure you want to change the base?
feat(event cache store): add an IndexedDB implementation #4617
Conversation
crates/matrix-sdk-indexeddb/src/event_cache_store/idb_operations.rs
Outdated
Show resolved
Hide resolved
|
||
/// Handles the functionality of serializing and encrypting data for the | ||
/// indexeddb store. | ||
pub struct IndexeddbSerializer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy and paste from crypto_store
not sure if a different serializer is needed or they can be merged and re-used by both modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we end up having to change this much, or is it a straight copy? If straight copy, is it possible to pull it into a shared mod?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure yet, I will hold on for now since I don't know what changes might I need to implement the rest of the methods
migration_conflict_strategy: MigrationConflictStrategy, | ||
} | ||
|
||
impl IndexeddbEventCacheStoreBuilder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follows the same pattern as state_store, not sure if this is wanted or not.
Ok(()) | ||
} | ||
|
||
async fn try_take_leased_lock( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy and paste from crypto_store. I could not find any info on what this "leased_lock" is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is related to the cross-process lock, the doc is missing a high-level overview I think… Tell me if you need more info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copied the same from the crypto_store, if you think it is fine as it is, then no need. It should just work, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible candidate to try to put into a shared mod as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, I need to get the thing to run first to confirm it is working as intended first
}; | ||
|
||
// let store_config = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be removed later
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4617 +/- ##
==========================================
+ Coverage 85.71% 85.74% +0.02%
==========================================
Files 291 291
Lines 33323 33322 -1
==========================================
+ Hits 28563 28571 +8
+ Misses 4760 4751 -9 ☔ View full report in Codecov by Sentry. |
Be aware that #4632 is likely to create a non-negligible conflict with your patches. The new methods to load chunk from the event cache are going to be simpler and will allow to incrementally load the chunks. |
Ah, interesting... well... maybe it's worth to put this a bit on hold until that is merged. If the trait will be easier to implement, then it's worth it |
The trait will gain 2 new methods: |
fn from(e: IndexeddbEventCacheStoreError) -> Self { | ||
match e { | ||
IndexeddbEventCacheStoreError::Json(e) => StoreError::Json(e), | ||
IndexeddbEventCacheStoreError::StoreError(e) => e, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this compile, I'm surprised it doesn't need a wrapper for StoreError here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using the command Jonas gave and it compiles with that, it will not pass the tests but the compiler shows no errors left:
cargo xtask ci wasm
EventCacheStoreError::Backend(Box::new(e)) | ||
} | ||
IndexeddbEventCacheStoreError::Encryption(e) => EventCacheStoreError::Encryption(e), | ||
_ => EventCacheStoreError::backend(e), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
backend => Backend, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not implemented by me, comes from the sdk-base and the implemented traits
match frm { | ||
IndexeddbEventCacheStoreError::Json(e) => CryptoStoreError::Serialization(e), | ||
IndexeddbEventCacheStoreError::CryptoStoreError(e) => e, | ||
_ => CryptoStoreError::backend(frm), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/backend/Backend/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, same as above, this is the way it's implemented in the base crate
fn from(frm: IndexeddbEventCacheStoreError) -> CryptoStoreError { | ||
match frm { | ||
IndexeddbEventCacheStoreError::Json(e) => CryptoStoreError::Serialization(e), | ||
IndexeddbEventCacheStoreError::CryptoStoreError(e) => e, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think this needs a wrapper as well, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know, copied what was there from the state_store, this all were required there I think
|
||
/// Handles the functionality of serializing and encrypting data for the | ||
/// indexeddb store. | ||
pub struct IndexeddbSerializer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we end up having to change this much, or is it a straight copy? If straight copy, is it possible to pull it into a shared mod?
/// Return all the raw components of a linked chunk, so the caller may | ||
/// reconstruct the linked chunk later. | ||
async fn reload_linked_chunk(&self, _room_id: &RoomId) -> Result<Vec<RawChunk<Event, Gap>>> { | ||
Ok(vec![]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo!, or this is just for tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is todo but don't want to use it for now since my workflow is so slow anyways, seeing hundred of errors because of pending todos() is confusing whenever I have to run the cargo command manually
Ok(()) | ||
} | ||
|
||
async fn try_take_leased_lock( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible candidate to try to put into a shared mod as well.
let store_config = { | ||
tracing::warn!("The IndexedDB backend does not implement an event cache store, falling back to the in-memory event cache store…"); | ||
store_config.event_cache_store(matrix_sdk_base::event_cache::store::MemoryStore::new()) | ||
let event_cache_store = matrix_sdk_indexeddb::open_event_cache_store(name, None).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like sqlite uses the same passphrase for both stores, should we do the same here instead of passing None?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is when the e2e encryption is not turned on, then None is required
|
This PR is WIP.
This PR is meant to add a EventCache store for the the
matrix-sdk-indexeddb
crate of the SDK meant to be used in WASM contexts. Similar to the sqlite implementation is supposed to provide a disk persisted implementation for storing Events in LinkedChunks.It follows the general structure of
matrix-sdk-indexeddb/src/crypto_store
andmatrix-sdk-indexeddb/src/state_store
, a new modulematrix-sdk-indexeddb/src/event_cache_store
has been added.There are still many TODOs and questions in the codebase to which I would appreciate some guidance from the SDK maintainers.
Signed-off-by: Oscar Franco