matrix-rust-sdk icon indicating copy to clipboard operation
matrix-rust-sdk copied to clipboard

IndexedDB: Add transaction functionality for `EventCacheStore` backend

Open mgoldenberg opened this issue 6 months ago • 2 comments

Background

This pull request is part of a series of pull requests to add an IndexedDB implementation of the EventCacheStore (see #4617, #4996, #5090, #5138, #5226). This particular pull request primarily focuses on providing functionality for easily issuing IndexedDB transaction.

Changes

The primary change is the addition of IndexeddbEventCacheStoreTransaction, which wraps IdbTransaction and IndexedbEventCacheStoreSerializer. It provides a convenient interface for composing atomic IndexedDB operations. For the sake of brevity, I have only added the generic versions of the functions for these operations, but as we add implementations of the functions in EventCacheStore, I will bring in their typed versions where relevant.

Additionally, in order to support some of the generic functions in IndexeddbEventCacheStoreTransaction, I have introduced another trait and some minor changes to the existing ones.

  • Added IndexedKeyComponentBounds (trait)
    • Similar to IndexedKeyBounds, but generates the lower and upper bounds of the key components, rather than the key itself. This is useful when constructing key ranges, except in the case where a particular key component is encrypted, in which case one must construct the ranges from the keys themselves.
  • Added IndexedKeyRange (type)
    • A type that can be used to express individual keys, as well as key ranges.
  • Changed Indexed (trait)
    • Tracks the name of the object store in which the implementing type stored using an associated constant, OBJECT_STORE: &'static str
  • Changed IndexedKey (trait)
    • Tracks the name of the index with which the key is associated (if any) using a function, index() -> Option<&'static str>
  • Changed IndexedKeyBounds (trait)
    • encode_lower_key -> lower_key to more closely mirror IndexedKeyComponentBounds
    • encode_upper_key -> upper_key to more closely mirror IndexedKeyComponentBounds
  • Changed IndexeddbEventCacheStoreSerializerError
    • Indexing variant uses generic type, rather than dynamic type so that Send and Sync don't have to be used as constraints where they aren't necessary.

Future Work

  • Add implementation of EventCacheStore which uses the transaction type added in this pull request.

  • [ ] Public API changes documented in changelogs (optional)

Signed-off-by: Michael Goldenberg [email protected]

mgoldenberg avatar Jun 24 '25 01:06 mgoldenberg

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.18%. Comparing base (ff52cf3) to head (582cfa1). Report is 55 commits behind head on main.

:white_check_mark: All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5274      +/-   ##
==========================================
- Coverage   90.18%   90.18%   -0.01%     
==========================================
  Files         334      334              
  Lines      104822   104822              
  Branches   104822   104822              
==========================================
- Hits        94538    94536       -2     
- Misses       6231     6233       +2     
  Partials     4053     4053              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jun 24 '25 02:06 codecov[bot]

A quick comment about the IndexedKeyComponentBounds and IndexedKeyBounds traits to help clarify some things.

The IndexedKeyComponentBounds helps to specify the upper and lower bounds of the components that are used to create the final key, while the IndexedKeyBounds are the upper and lower bounds of the final key itself.

While these concepts are similar and often produce the same results, there are cases where these two concepts produce very different results. Namely, when any of the components are encrypted in the process of constructing the final key, then the component bounds and the key bounds produce very different results.

So, for instance, consider the EventId, which may be encrypted before being used in a final key. One cannot construct the upper and lower bounds of the final key using the upper and lower bounds of the EventId, because once the EventId is encrypted, the resultant value will no longer express the proper bound.

Note that this is mentioned in the final version of the traits, see here.

mgoldenberg avatar Jun 24 '25 19:06 mgoldenberg

I was wondering: can we add tests for this? Or is it for a next PR?

So, in my branch with all the changes, I only have tests of the EventCacheStore functions, as I believe that will ultimately be the only public interface.

Do you think we should have more than that? If so, I'd be happy to add more now or in a future pull request.

For context, I'll also be adding typed versions of all the functions in IndexeddbEventCacheStoreTransaction - e.g., get_chunks_by_id, add_event, delete_gap_by_id, etc. - as they are needed in the implementation of the EventCacheStore functions. Would you want to see tests of those typed functions and the generic ones?

Let me know what you're thinking!

mgoldenberg avatar Jun 24 '25 21:06 mgoldenberg

About https://github.com/matrix-org/matrix-rust-sdk/pull/5274#issuecomment-3001669854 — It's great the last part is mentioned in the doc but the first 2/3 of the comment is also truly valuable! Can you add that on the documentation of IndexedKeyBounds for example.

About https://github.com/matrix-org/matrix-rust-sdk/pull/5274#issuecomment-3001846036 — It depends of how much efforts it represents for you. We already have unit tests and integration tests for the EventCacheStore implementations (via macros). But for particular implementation, we also have custom unit tests. If you believe this is necessary, I'm all for it of course, but it's not a blocker for me yet.

Hywan avatar Jun 25 '25 08:06 Hywan

You can rebase your fixup commits, then we can merge the PR.

Hywan avatar Jun 25 '25 08:06 Hywan