-
-
Notifications
You must be signed in to change notification settings - Fork 13
Provide access to active transaction in callback provided to OpenDbRequestBuilder::with_on_upgrade_needed{_fut}
#72
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
Open
mgoldenberg
wants to merge
13
commits into
Alorel:master
Choose a base branch
from
mgoldenberg:expose-version-change-transaction
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ded callback BREAKING CHANGE: The callback provided to `OpenDbRequestBuilder::with_on_upgrade_needed` must now accept a reference to a `Transaction` rather than a `Database`. For most cases, existing code can be made compatible by calling `Transaction::database` in order to get access to the underlying database. The caveat is that the `Transaction::database` provides a reference, rather than ownership of the `Database`.
BREAKING CHANGE: `AsyncFnOnce` was stabilized in version 1.85.0 of Rust, so the MSRV had to be bumped to accommodate this. Additionally, synchronous closures that use `async move` blocks are not drop-in replacements for `async` closures, so users of `OpenDbRequestBuilder::with_on_upgrade_needed_fut` will have to make minor changes to their invocations.
…de needed callback BREAKING CHANGE: The callback provided to `OpenDbRequestBuilder::with_on_upgrade_needed_fut` must now accept a reference to a `Transaction` rather than a `Database`. For most cases, existing code can be made compatible by calling `Transaction::database` in order to get access to the underlying database. The caveat is that the `Transaction::database` provides a reference, rather than ownership of the `Database`.
For details on lifetime syntax lint, see https://blog.rust-lang.org/2025/08/07/Rust-1.89.0/#mismatched-lifetime-syntaxes-lint.
For some reason, calling `TransactionSys::do_commit` in the `Drop` implementation of `Transaction` causes tests in a headless Chrome browser to hang, even though they pass in a non-headless context. Ultimately, the default behavior on the JavaScript end is for the transaction to be committed, so it is safe to exclude this action.
9571c47 to
2ac8f0b
Compare
This was referenced Sep 7, 2025
1 task
Contributor
|
Hello @Alorel, did you had time to check this PR? |
e48ec89 to
6ec78c3
Compare
Hywan
pushed a commit
to matrix-org/matrix-rust-sdk
that referenced
this pull request
Oct 3, 2025
**NOTE:** _this should not be merged until matrix-org/rust-indexed-db#1 is merged! The `[patch]` in this branch should point to the official `matrix-org` fork of `rust-indexed_db`, but is currently pointed at my personal fork._ ## Background This pull request makes updates [`indexed_db_futures`](https://docs.rs/indexed_db_futures/latest/indexed_db_futures/index.html) in the `matrix-sdk-indexeddb` crate. The reason we'd like to update this dependency is because the version currently used does not fully support the Chrome browser (see #5420). The latest version of `indexed_db_futures` has significant changes. Many of these changes can be integrated without issue. There is, however, a single change which is incompatible with the `matrix-sdk-indexeddb` crate. Namely, one cannot access the active transaction in the callback to update the database (for details, see Alorel/rust-indexed-db#66). ### An Updated Proposal Originally, new migrations were implemented in order to work around this issue (see #5467). However, the proposal was ultimately rejected (see @andybalaam's [comment](#5467 (comment))). For this reason, the dependency has instead been `[patch]`ed in the top-level `Cargo.toml` with a modified version of `indexed_db_futures` (see matrix-org/rust-indexed-db#1). Furthermore, these changes have been proposed to the maintainer and are awaiting feedback (see Alorel/rust-indexed-db#72). ### Why do we need the active transaction in our migrations? The `crypto_store` module provides access to the active transaction to its migrations (see [here](https://github.com/matrix-org/matrix-rust-sdk/blob/ca89700dfe9f29dcd823bb10861807f9d75e0634/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs#L211)). Furthermore, there is a single migration (`v11_to_v12`) in the `crypto_store` module which actually makes use of the active transaction (see [here](https://github.com/matrix-org/matrix-rust-sdk/blob/ca89700dfe9f29dcd823bb10861807f9d75e0634/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v11_to_v12.rs#L23)). For clarity, the reason `v11_to_v12` is problematic in the latest versions of `indexed_db_futures` is because it is simply adding an index to an object store which was created in a different migration and this requires access to the active transaction. All the other migrations create object stores and indices in the same migration, which does not suffer from the same issue. ## Changes - Move `indexed_db_futures` to the workspace `Cargo.toml` and add a `[patch]` so that it points to a modified version. - Add `GenericError` type and conversions in order to more easily map `indexed_db_futures` errors into `matrix-sdk-*` errors. - Update all IndexedDB interactions so that they use the upgraded interface provided by `indexed_db_futures` - Add functionality for running `wasm-pack` tests against Chrome --- Closes #5420. --- - [ ] Public API changes documented in changelogs (optional) Signed-off-by: Michael Goldenberg <m@mgoldenberg.net> --------- Signed-off-by: Michael Goldenberg <m@mgoldenberg.net>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request contains a number of breaking changes! @Alorel, I am planning to host a fork with these changes, but I wanted to provide them here as a pull request, in case any of them seem appealing to you.
Background
Recently, I created an issue in this repository outlining a change that prevented certain kinds of database upgrades (for details, see #66).
To summarize, the old upgrade callback was able to access the active version change transaction and, for instance, add an index to an existing object store. In more recent versions of this library, this is no longer possible as the values provided to the callback cannot be used to get access to the active transaction.
Changes
In order to support the behavior described above, I have made a number of changes - many of which are breaking changes.
The overarching change is that the active version change transaction is explicitly provided to the callback and the database is not provided to the callback. This seemed like a reasonable change, as
Transaction::dbprovides a reference to the underlyingDatabase, so existing codebases could relatively easily make their callbacks compatible - with minor exceptions detailed below.Details on the changes are below.
Synchronous Upgrade Needed Callback
The signature of the callback provided to
OpenDbRequestBuilder::with_on_upgrade_neededhas changed to accept a reference to the active version changeTransactionrather than theDatabase.Note that the function that invokes the callback -
OpenDbListener::listener- constructs theTransactionfrom theVersionChangeEventwith a newly added function,Transaction::from_raw_version_change_event.Configuring
Transactionbehavior onDropGiven that the callback above is synchronous, it is unable to commit the
TransactionasTransaction::commitis asynchronous. Furthermore, the function which invokes the callback -OpenDbListener::listener- is also synchronous and has the same shortcoming. The consequence is that when theTransactionisDropped at the end ofOpenDbListener::listener, it is automatically aborted, and any changes made to theDatabasein the callback are lost.In order to mitigate this issue, I have updated
Transactionso that theDropbehavior is configurable. That is, one can set a field in theTransactionso that it is either committed or aborted when it isDropped. By default, it is aborted, making this change backward compatible.Asynchronous Upgrade Needed Callback
The signature of the callback provided to
OpenDbRequestBuilder::with_on_upgrade_needed_futhas also changed to accept a reference to the active version changeTransactionrather than theDatabase. And additionally, it uses the recently stabilizedAsynFnOncetrait.Note that using
AsyncFnOncerequires updating the minimum supported Rust version to1.85.0. I'm not entirely sure that usingAsyncFnOnceis absolutely necessary, but I believe that without it, I was bumping up against the lifetime-related limitations detailed in rust-lang/rust#70263.Caveat
The original callbacks provided to
OpenDbRequestBuilder::with_on_upgrade_neededandOpenDbRequestBuilder::with_on_upgrade_needed_futhad access to an ownedDatabase, while the updated callbacks only have access to a reference. This precludes calling the functions below, which consume theDatabase.Database::delete- deletes theDatabaseDatabase::close- closes theDatabaseHowever, both of these functions seemed to me like operations that perhaps shouldn't occur in a version upgrade. And in the case of deleting a database, this seems to be in alignment with the official IndexedDB documentation, where deleting a database is an operation exposed through the
IDBFactoryand not theIDBDatabase.UPDATE (2025/09/07): In order to get everything to pass the C.I. tests, I had to update the toolchain used to generate documentation, update the examples, and make some minimal changes to the code in order to accommodate new lint rules. Also, I force pushed some new commit messages so that they conform to the semantic pull request guidelines outlined in
.github/semantic.yml.Closes #66.