Skip to content

Conversation

@mgoldenberg
Copy link
Contributor

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]

…ializer error

This prevents us from having to add type constraints on the dynamic type
and instead only having to specify the type constraint at the call site.

Signed-off-by: Michael Goldenberg <[email protected]>
…lower, encode_upper}

Signed-off-by: Michael Goldenberg <[email protected]>
@codecov
Copy link

codecov bot commented Jun 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

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

✅ 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              

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

@mgoldenberg mgoldenberg marked this pull request as ready for review June 24, 2025 03:59
@mgoldenberg mgoldenberg requested a review from a team as a code owner June 24, 2025 03:59
@mgoldenberg mgoldenberg requested review from andybalaam and removed request for a team June 24, 2025 03:59
@Hywan Hywan requested review from Hywan and removed request for andybalaam June 24, 2025 07:40
@Hywan Hywan self-assigned this Jun 24, 2025
Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! Thank you for the clean patches, it's really helpful.

I made a few feedback but nothing important.

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

@mgoldenberg
Copy link
Contributor Author

mgoldenberg commented Jun 24, 2025

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
Copy link
Contributor Author

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 mgoldenberg requested a review from Hywan June 24, 2025 21:00
@Hywan
Copy link
Member

Hywan commented Jun 25, 2025

About #5274 (comment) — 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 #5274 (comment) — 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
Copy link
Member

Hywan commented Jun 25, 2025

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

…cific to event cache store

Signed-off-by: Michael Goldenberg <[email protected]>
…l data from an object store in IndexedDB

Signed-off-by: Michael Goldenberg <[email protected]>
@Hywan Hywan force-pushed the indexeddb-event-cache-store-transaction branch from bb8c01b to 582cfa1 Compare June 25, 2025 14:05
@Hywan Hywan enabled auto-merge (rebase) June 25, 2025 14:07
@Hywan Hywan merged commit 836c643 into matrix-org:main Jun 25, 2025
44 checks passed
@mgoldenberg mgoldenberg deleted the indexeddb-event-cache-store-transaction branch July 9, 2025 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants