Indexeddb: Groundwork for fixing `inbound_group_session` lookups #2884

richvdh · 2023-11-25T00:06:20Z

This is a set of non-functional changes which lay some groundwork in preparation for fixing element-hq/element-web#26488 (comment).

Hopefully each commit is reasonably self-explanatory.

... and split it up into smaller functions

codecov · 2023-11-25T00:24:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ceeb5e7) 82.68% compared to head (a4c46b1) 82.69%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2884   +/-   ##
=======================================
  Coverage   82.68%   82.69%           
=======================================
  Files         216      216           
  Lines       22176    22176           
=======================================
+ Hits        18337    18338    +1     
+ Misses       3839     3838    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Turns out we don't need to implement `TryInto/TryFrom<JsValue>`, whihc saves a bunch of boilerplate.

bnjbvr

Makes sense in general; just a bit hard to review, so I'm requesting to split the last commit that mixes code motion and changes to the moved code please.

bnjbvr · 2023-11-27T11:16:03Z

crates/matrix-sdk-indexeddb/src/crypto_store/migrations.rs

+
+/// Open the indexeddb with the given name, upgrading it to the latest version
+/// of the schema if necessary.
+pub async fn open_and_upgrade_db(name: &str) -> Result<IdbDatabase, DomException> {


(looking at the commit Move migration functions to a separate module and split it up into smaller functions)

In the future, can you avoid changes that both 1. move the code, 2. change the code, please? They make the review really hard. In this case the amount of code is small enough that it's not a big deal, but it takes quadratically more time than if the commit had been split into two smaller logical ones.

Noted, and agreed. Sorry about this!

bnjbvr · 2023-11-27T11:23:29Z

crates/matrix-sdk-indexeddb/src/crypto_store/migrations.rs

+
+fn create_stores_for_v2(db: &IdbDatabase) -> Result<(), DomException> {
+    // We changed how we store inbound group sessions, the key used to
+    // be a tuple of `(room_id, sender_key, session_id)` now it's a


(I noticed this comment has changed 👀, good job 👍)

bnjbvr · 2023-11-27T11:27:49Z

crates/matrix-sdk-indexeddb/src/crypto_store/migrations.rs

+        &params,
+    )?;
+
+    if db.object_store_names().any(|n| n == "outgoing_secret_requests") {


I found the previous check with respect to the previous version number more obvious, but I think it should be equivalent, assuming atomic operations.

bnjbvr · 2023-11-27T11:29:50Z

crates/matrix-sdk-indexeddb/src/serialize_bool_for_indexeddb.rs

+//! Booleans don't work as keys in indexeddb (see [ECMA spec]), so instead we
+//! serialize them as `0` or `1`.
+//!
+//! This module implements a custom serializer which can be used on `bool`
+//! struct fields with:
+//!
+//! ```ignore
+//! #[serde(with = "serialize_bool_for_indexeddb")]
+//! ```
+//!
+//! [ECMA spec]: https://w3c.github.io/IndexedDB/#key


To make it easier for further uses of that functionality, instead of using the above serde annotation, would it make sense to use a newtype wrapper? Something like #[repr(transparent)] struct SerializableBool(bool), with From<bool>/Into<bool> impls, and that implements a custom serde::Serialize and serde::Deserialize?

As discussed, this doesn't quite work out as nicely as hoped due to the need to add .into calls everywhere.

bnjbvr · 2023-11-27T11:53:59Z

crates/matrix-sdk-indexeddb/src/crypto_store/mod.rs

@@ -138,6 +138,12 @@ impl From<indexed_db_futures::web_sys::DomException> for IndexeddbCryptoStoreErr
    }
 }

+impl From<serde_wasm_bindgen::Error> for IndexeddbCryptoStoreError {
+    fn from(e: serde_wasm_bindgen::Error) -> Self {
+        IndexeddbCryptoStoreError::Json(serde::de::Error::custom(e.to_string()))


pre-existing: if that's not an API breaking change, could we rename the Json variant to Serialization?

Sure! done in a4c46b1

bnjbvr · 2023-11-27T11:56:30Z

crates/matrix-sdk-indexeddb/src/crypto_store/indexeddb_serializer.rs

+            // XXX Isn't there a way to do this that *doesn't* involve going via a JSON
+            // string?


Sorry, there's at least comment changes and code motion in this commit, and since it's a much bigger chunk than the previous, I'll request to split it into a code motion one first, and then the changes to the code as a second commit please.

Yup, sorry about this. The comments aren't particularly significant imho which is why I rolled them all together, but that was bad of me.

I've now split this into three separate commits.

Mostly, this means that we only need to hold the `store_cipher` in one place.

A couple of improvements and a couple of oddities I noted along the way.

bnjbvr

Thanks for splitting the commits, made the next round of review trivial 🙏

richvdh added 2 commits November 25, 2023 00:03

move crypto_store into a submodule

4243555

Move migration functions to a separate module

fbbdc51

... and split it up into smaller functions

richvdh requested a review from a team as a code owner November 25, 2023 00:06

richvdh requested review from bnjbvr and removed request for a team November 25, 2023 00:06

richvdh force-pushed the rav/prepare_for_igs_migration branch from c0facf0 to 7a979a1 Compare November 25, 2023 00:12

richvdh force-pushed the rav/prepare_for_igs_migration branch 2 times, most recently from 777f218 to d2c2823 Compare November 25, 2023 02:02

richvdh added 2 commits November 25, 2023 02:03

Write custom serializer for bools in indexeddb

e643830

Simplify de/serialization of GossipRequests

046603d

Turns out we don't need to implement `TryInto/TryFrom<JsValue>`, whihc saves a bunch of boilerplate.

richvdh force-pushed the rav/prepare_for_igs_migration branch from d2c2823 to c8688ce Compare November 25, 2023 02:03

richvdh mentioned this pull request Nov 25, 2023

indexeddb: Update storage for inbound_group_sessions #2885

Merged

bnjbvr requested changes Nov 27, 2023

View reviewed changes

richvdh added 4 commits November 27, 2023 12:51

Move {,de}serialize_value{,_as_bytes} to new type

f43de3f

Move key encoding functions to IndexeddbSerializer

a4cdd59

Mostly, this means that we only need to hold the `store_cipher` in one place.

Add some comments

0f16ad6

A couple of improvements and a couple of oddities I noted along the way.

Rename IndexeddbCryptoStoreError::Json -> Serialization

a4c46b1

richvdh force-pushed the rav/prepare_for_igs_migration branch from c8688ce to a4c46b1 Compare November 27, 2023 13:32

richvdh requested a review from bnjbvr November 27, 2023 14:30

bnjbvr approved these changes Nov 27, 2023

View reviewed changes

richvdh merged commit bfe7946 into main Nov 27, 2023
35 checks passed

richvdh deleted the rav/prepare_for_igs_migration branch November 27, 2023 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Indexeddb: Groundwork for fixing `inbound_group_session` lookups #2884

Indexeddb: Groundwork for fixing `inbound_group_session` lookups #2884

richvdh commented Nov 25, 2023

codecov bot commented Nov 25, 2023 •

edited

Loading

bnjbvr left a comment

bnjbvr Nov 27, 2023

richvdh Nov 27, 2023

bnjbvr Nov 27, 2023

bnjbvr Nov 27, 2023

bnjbvr Nov 27, 2023

richvdh Nov 27, 2023

bnjbvr Nov 27, 2023

richvdh Nov 27, 2023

bnjbvr Nov 27, 2023

richvdh Nov 27, 2023

bnjbvr left a comment

		// XXX Isn't there a way to do this that doesn't involve going via a JSON
		// string?

Indexeddb: Groundwork for fixing inbound_group_session lookups #2884

Indexeddb: Groundwork for fixing inbound_group_session lookups #2884

Conversation

richvdh commented Nov 25, 2023

codecov bot commented Nov 25, 2023 • edited Loading

Codecov Report

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnjbvr left a comment

Choose a reason for hiding this comment

Indexeddb: Groundwork for fixing `inbound_group_session` lookups #2884

Indexeddb: Groundwork for fixing `inbound_group_session` lookups #2884

codecov bot commented Nov 25, 2023 •

edited

Loading