From ad72a6dedcbe0a6ae1d25f9b716d2678c539787a Mon Sep 17 00:00:00 2001 From: Johannes Marbach Date: Mon, 7 Apr 2025 19:41:09 +0200 Subject: [PATCH 01/17] refactor(send_queue): generalize SentRequestKey::Media and DependentQueuedRequestKind::UploadFileWithThumbnail to prepare for MSC4274 gallery uploads Signed-off-by: Johannes Marbach --- crates/matrix-sdk-base/Cargo.toml | 3 ++ crates/matrix-sdk-base/src/store/mod.rs | 2 + .../matrix-sdk-base/src/store/send_queue.rs | 48 ++++++++++++++++--- crates/matrix-sdk/Cargo.toml | 3 ++ crates/matrix-sdk/src/send_queue/mod.rs | 30 +++++++++--- crates/matrix-sdk/src/send_queue/upload.rs | 44 +++++++++++++---- 6 files changed, 106 insertions(+), 24 deletions(-) diff --git a/crates/matrix-sdk-base/Cargo.toml b/crates/matrix-sdk-base/Cargo.toml index 330744efe90..3cd2326ca69 100644 --- a/crates/matrix-sdk-base/Cargo.toml +++ b/crates/matrix-sdk-base/Cargo.toml @@ -43,6 +43,9 @@ testing = [ "matrix-sdk-crypto?/testing", ] +# Add support for inline media galleries via msgtypes +unstable-msc4274 = [] + [dependencies] as_variant = { workspace = true } assert_matches = { workspace = true, optional = true } diff --git a/crates/matrix-sdk-base/src/store/mod.rs b/crates/matrix-sdk-base/src/store/mod.rs index 461fc89b6c3..6cf2b821a95 100644 --- a/crates/matrix-sdk-base/src/store/mod.rs +++ b/crates/matrix-sdk-base/src/store/mod.rs @@ -71,6 +71,8 @@ mod send_queue; #[cfg(any(test, feature = "testing"))] pub use self::integration_tests::StateStoreIntegrationTests; +#[cfg(feature = "unstable-msc4274")] +pub use self::send_queue::AccumulatedSentMediaInfo; pub use self::{ memory_store::MemoryStore, send_queue::{ diff --git a/crates/matrix-sdk-base/src/store/send_queue.rs b/crates/matrix-sdk-base/src/store/send_queue.rs index 8c87b6c5ebb..bbdb91a8584 100644 --- a/crates/matrix-sdk-base/src/store/send_queue.rs +++ b/crates/matrix-sdk-base/src/store/send_queue.rs @@ -103,6 +103,12 @@ pub enum QueuedRequestKind { /// To which media event transaction does this upload relate? related_to: OwnedTransactionId, + + /// Accumulated list of infos for previously uploaded files and + /// thumbnails if used during a gallery transaction. Otherwise empty. + #[cfg(feature = "unstable-msc4274")] + #[serde(default)] + accumulated: Vec, }, } @@ -219,17 +225,22 @@ pub enum DependentQueuedRequestKind { key: String, }, - /// Upload a file that had a thumbnail. - UploadFileWithThumbnail { - /// Content type for the file itself (not the thumbnail). + /// Upload a file or thumbnail depending on another file or thumbnail + /// upload. + UploadFileOrThumbnail { + /// Content type for the file or thumbnail. content_type: String, - /// Media request necessary to retrieve the file itself (not the - /// thumbnail). + /// Media request necessary to retrieve the file or thumbnail itself. cache_key: MediaRequestParameters, /// To which media transaction id does this upload relate to? related_to: OwnedTransactionId, + + /// Whether the depended upon request was a thumbnail or a file upload. + #[cfg(feature = "unstable-msc4274")] + #[serde(default)] + depends_on_thumbnail: bool, }, /// Finish an upload by updating references to the media cache and sending @@ -308,7 +319,7 @@ impl From for ChildTransactionId { } } -/// Information about a media (and its thumbnail) that have been sent to an +/// Information about a media (and its thumbnail) that have been sent to a /// homeserver. #[derive(Clone, Debug, Serialize, Deserialize)] pub struct SentMediaInfo { @@ -322,6 +333,29 @@ pub struct SentMediaInfo { /// /// When uploading a thumbnail, this is set to `None`. pub thumbnail: Option, + + /// Accumulated list of infos for previously uploaded files and thumbnails + /// if used during a gallery transaction. Otherwise empty. + #[cfg(feature = "unstable-msc4274")] + #[serde(default)] + pub accumulated: Vec, +} + +/// Accumulated information about a media (and its thumbnail) that have been +/// sent to a homeserver. +#[cfg(feature = "unstable-msc4274")] +#[derive(Clone, Debug, Serialize, Deserialize)] +pub struct AccumulatedSentMediaInfo { + /// File that was uploaded by this request. + /// + /// If the request related to a thumbnail upload, this contains the + /// thumbnail media source. + pub file: MediaSource, + + /// Optional thumbnail previously uploaded, when uploading a file. + /// + /// When uploading a thumbnail, this is set to `None`. + pub thumbnail: Option, } /// A unique key (identifier) indicating that a transaction has been @@ -388,7 +422,7 @@ impl DependentQueuedRequest { DependentQueuedRequestKind::EditEvent { .. } | DependentQueuedRequestKind::RedactEvent | DependentQueuedRequestKind::ReactEvent { .. } - | DependentQueuedRequestKind::UploadFileWithThumbnail { .. } => { + | DependentQueuedRequestKind::UploadFileOrThumbnail { .. } => { // These are all aggregated events, or non-visible items (file upload producing // a new MXC ID). false diff --git a/crates/matrix-sdk/Cargo.toml b/crates/matrix-sdk/Cargo.toml index 11ec5ea6523..07529c10d17 100644 --- a/crates/matrix-sdk/Cargo.toml +++ b/crates/matrix-sdk/Cargo.toml @@ -49,6 +49,9 @@ experimental-widgets = ["dep:uuid"] docsrs = ["e2e-encryption", "sqlite", "indexeddb", "sso-login", "qrcode"] +# Add support for inline media galleries via msgtypes +unstable-msc4274 = ["matrix-sdk-base/unstable-msc4274"] + [dependencies] anyhow = { workspace = true, optional = true } anymap2 = "0.13.0" diff --git a/crates/matrix-sdk/src/send_queue/mod.rs b/crates/matrix-sdk/src/send_queue/mod.rs index 9379c4249b1..05e1227af17 100644 --- a/crates/matrix-sdk/src/send_queue/mod.rs +++ b/crates/matrix-sdk/src/send_queue/mod.rs @@ -109,8 +109,8 @@ //! - the thumbnail is sent first as an [`QueuedRequestKind::MediaUpload`] //! request, //! - the file upload is pushed as a dependent request of kind -//! [`DependentQueuedRequestKind::UploadFileWithThumbnail`] (this variant -//! keeps the file's key used to look it up in the cache store). +//! [`DependentQueuedRequestKind::UploadFileOrThumbnail`] (this variant keeps +//! the file's key used to look it up in the cache store). //! - the media event is then sent as a dependent request as described in the //! previous section. //! @@ -699,6 +699,8 @@ impl RoomSendQueue { cache_key, thumbnail_source, related_to: relates_to, + #[cfg(feature = "unstable-msc4274")] + accumulated, } => { trace!(%relates_to, "uploading media related to event"); @@ -757,6 +759,8 @@ impl RoomSendQueue { Ok(SentRequestKey::Media(SentMediaInfo { file: media_source, thumbnail: thumbnail_source, + #[cfg(feature = "unstable-msc4274")] + accumulated, })) }; @@ -1213,6 +1217,8 @@ impl QueueStorage { cache_key: thumbnail_media_request, thumbnail_source: None, // the thumbnail has no thumbnails :) related_to: send_event_txn.clone(), + #[cfg(feature = "unstable-msc4274")] + accumulated: vec![], }, Self::LOW_PRIORITY, ) @@ -1225,10 +1231,12 @@ impl QueueStorage { &upload_thumbnail_txn, upload_file_txn.clone().into(), created_at, - DependentQueuedRequestKind::UploadFileWithThumbnail { + DependentQueuedRequestKind::UploadFileOrThumbnail { content_type: content_type.to_string(), cache_key: file_media_request, related_to: send_event_txn.clone(), + #[cfg(feature = "unstable-msc4274")] + depends_on_thumbnail: true, }, ) .await?; @@ -1246,6 +1254,8 @@ impl QueueStorage { cache_key: file_media_request, thumbnail_source: None, related_to: send_event_txn.clone(), + #[cfg(feature = "unstable-msc4274")] + accumulated: vec![], }, Self::LOW_PRIORITY, ) @@ -1374,7 +1384,7 @@ impl QueueStorage { }, }), - DependentQueuedRequestKind::UploadFileWithThumbnail { .. } => { + DependentQueuedRequestKind::UploadFileOrThumbnail { .. } => { // Don't reflect these: only the associated event is interesting to observers. None } @@ -1587,22 +1597,28 @@ impl QueueStorage { } } - DependentQueuedRequestKind::UploadFileWithThumbnail { + DependentQueuedRequestKind::UploadFileOrThumbnail { content_type, cache_key, related_to, + #[cfg(feature = "unstable-msc4274")] + depends_on_thumbnail, } => { let Some(parent_key) = parent_key else { // Not finished yet, we should retry later => false. return Ok(false); }; - self.handle_dependent_file_upload_with_thumbnail( + self.handle_dependent_file_or_thumbnail_upload( client, dependent_request.own_transaction_id.into(), parent_key, content_type, cache_key, related_to, + #[cfg(feature = "unstable-msc4274")] + depends_on_thumbnail, + #[cfg(not(feature = "unstable-msc4274"))] + true, ) .await?; } @@ -2207,7 +2223,7 @@ fn canonicalize_dependent_requests( } } - DependentQueuedRequestKind::UploadFileWithThumbnail { .. } + DependentQueuedRequestKind::UploadFileOrThumbnail { .. } | DependentQueuedRequestKind::FinishUpload { .. } | DependentQueuedRequestKind::ReactEvent { .. } => { // These requests can't be canonicalized, push them as is. diff --git a/crates/matrix-sdk/src/send_queue/upload.rs b/crates/matrix-sdk/src/send_queue/upload.rs index 5e281dff08d..05e8a463412 100644 --- a/crates/matrix-sdk/src/send_queue/upload.rs +++ b/crates/matrix-sdk/src/send_queue/upload.rs @@ -14,6 +14,8 @@ //! Private implementations of the media upload mechanism. +#[cfg(feature = "unstable-msc4274")] +use matrix_sdk_base::store::AccumulatedSentMediaInfo; use matrix_sdk_base::{ event_cache::store::media::IgnoreMediaRetentionPolicy, media::{MediaFormat, MediaRequestParameters}, @@ -348,7 +350,8 @@ impl QueueStorage { } /// Consumes a finished upload of a thumbnail and queues the file upload. - pub(super) async fn handle_dependent_file_upload_with_thumbnail( + #[allow(clippy::too_many_arguments)] + pub(super) async fn handle_dependent_file_or_thumbnail_upload( &self, client: &Client, next_upload_txn: OwnedTransactionId, @@ -356,28 +359,49 @@ impl QueueStorage { content_type: String, cache_key: MediaRequestParameters, event_txn: OwnedTransactionId, + depends_on_thumbnail: bool, ) -> Result<(), RoomSendQueueError> { - // The thumbnail has been sent, now transform the dependent file upload request - // into a ready one. + // The previous file or thumbnail has been sent, now transform the dependent + // file or thumbnail upload request into a ready one. let sent_media = parent_key .into_media() .ok_or(RoomSendQueueError::StorageError(RoomSendQueueStorageError::InvalidParentKey))?; - // The media we just uploaded was a thumbnail, so the thumbnail shouldn't have + // If the previous upload was a thumbnail, it shouldn't have // a thumbnail itself. - debug_assert!(sent_media.thumbnail.is_none()); - if sent_media.thumbnail.is_some() { - warn!("unexpected thumbnail for a thumbnail!"); + if depends_on_thumbnail { + debug_assert!(sent_media.thumbnail.is_none()); + if sent_media.thumbnail.is_some() { + warn!("unexpected thumbnail for a thumbnail!"); + } } - trace!(related_to = %event_txn, "done uploading thumbnail, now queuing a request to send the media file itself"); + trace!(related_to = %event_txn, "done uploading file or thumbnail, now queuing the dependent file or thumbnail upload request"); + + // If the previous upload was a thumbnail, forward the accumulated sent media + // unchangend. Otherwise, append the sent media to the accumulated + // vector. + #[cfg(feature = "unstable-msc4274")] + let accumulated = if depends_on_thumbnail { + sent_media.accumulated + } else { + let mut accumulated = sent_media.accumulated.clone(); + accumulated.push(AccumulatedSentMediaInfo { + file: sent_media.file.clone(), + thumbnail: sent_media.thumbnail, + }); + accumulated + }; let request = QueuedRequestKind::MediaUpload { content_type, cache_key, - // The thumbnail for the next upload is the file we just uploaded here. - thumbnail_source: Some(sent_media.file), + // If the previous upload was a thumbnail, it becomse the thumbnail source for the next + // upload. + thumbnail_source: if depends_on_thumbnail { Some(sent_media.file) } else { None }, related_to: event_txn, + #[cfg(feature = "unstable-msc4274")] + accumulated, }; client From c97f8f8d4aee653cc82202890d001e2c183f0c3c Mon Sep 17 00:00:00 2001 From: Johannes Marbach Date: Mon, 7 Apr 2025 20:26:06 +0200 Subject: [PATCH 02/17] Add changelogs --- crates/matrix-sdk-base/CHANGELOG.md | 6 ++++++ crates/matrix-sdk/CHANGELOG.md | 5 +++++ 2 files changed, 11 insertions(+) diff --git a/crates/matrix-sdk-base/CHANGELOG.md b/crates/matrix-sdk-base/CHANGELOG.md index 9bf938aba88..313779ac64c 100644 --- a/crates/matrix-sdk-base/CHANGELOG.md +++ b/crates/matrix-sdk-base/CHANGELOG.md @@ -37,6 +37,12 @@ All notable changes to this project will be documented in this file. - [**breaking**] `BaseClient::set_session_metadata` is renamed `activate`, and `BaseClient::logged_in` is renamed `is_activated` ([#4850](https://github.com/matrix-org/matrix-rust-sdk/pull/4850)) +- [**breaking] `DependentQueuedRequestKind::UploadFileWithThumbnail` + was renamed to `DependentQueuedRequestKind::UploadFileOrThumbnail`. + Under the `unstable-msc4274` feature, `DependentQueuedRequestKind::UploadFileOrThumbnail` + and `SentMediaInfo` were generalized to allow chaining multiple dependent + file / thumbnail uploads. + ([#4897](https://github.com/matrix-org/matrix-rust-sdk/pull/4897)) ## [0.10.0] - 2025-02-04 diff --git a/crates/matrix-sdk/CHANGELOG.md b/crates/matrix-sdk/CHANGELOG.md index d13c7fb3459..c7652e3bdb9 100644 --- a/crates/matrix-sdk/CHANGELOG.md +++ b/crates/matrix-sdk/CHANGELOG.md @@ -235,6 +235,11 @@ simpler methods: ([#4831](https://github.com/matrix-org/matrix-rust-sdk/pull/4831)) - [**breaking**] `Client::store` is renamed `state_store` ([#4851](https://github.com/matrix-org/matrix-rust-sdk/pull/4851)) +- [**breaking] `QueueStorage::handle_dependent_file_upload_with_thumbnail` + was renamed to `handle_dependent_file_or_thumbnail_upload`. Under the + `unstable-msc4274` feature, it was generalized to allow chaining multiple + dependent file / thumbnail uploads. + ([#4897](https://github.com/matrix-org/matrix-rust-sdk/pull/4897)) ## [0.10.0] - 2025-02-04 From fb6100357a500c76c12d706c08fe81647c8f91f5 Mon Sep 17 00:00:00 2001 From: Johannes Marbach Date: Tue, 8 Apr 2025 10:51:53 +0200 Subject: [PATCH 03/17] Fix typo --- crates/matrix-sdk/src/send_queue/upload.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk/src/send_queue/upload.rs b/crates/matrix-sdk/src/send_queue/upload.rs index 80784338b39..59b0c8f7695 100644 --- a/crates/matrix-sdk/src/send_queue/upload.rs +++ b/crates/matrix-sdk/src/send_queue/upload.rs @@ -396,7 +396,7 @@ impl QueueStorage { let request = QueuedRequestKind::MediaUpload { content_type, cache_key, - // If the previous upload was a thumbnail, it becomse the thumbnail source for the next + // If the previous upload was a thumbnail, it becomes the thumbnail source for the next // upload. thumbnail_source: if depends_on_thumbnail { Some(sent_media.file) } else { None }, related_to: event_txn, From 2ab77d4f82be1397add0400ce1d642d7b7de07a2 Mon Sep 17 00:00:00 2001 From: Johannes Marbach Date: Tue, 8 Apr 2025 12:20:01 +0200 Subject: [PATCH 04/17] Fix another typo --- crates/matrix-sdk/src/send_queue/upload.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk/src/send_queue/upload.rs b/crates/matrix-sdk/src/send_queue/upload.rs index 431251dffb0..a0cdbf50f05 100644 --- a/crates/matrix-sdk/src/send_queue/upload.rs +++ b/crates/matrix-sdk/src/send_queue/upload.rs @@ -379,7 +379,7 @@ impl QueueStorage { trace!(related_to = %event_txn, "done uploading file or thumbnail, now queuing the dependent file or thumbnail upload request"); // If the previous upload was a thumbnail, forward the accumulated sent media - // unchangend. Otherwise, append the sent media to the accumulated + // unchanged. Otherwise, append the sent media to the accumulated // vector. #[cfg(feature = "unstable-msc4274")] let accumulated = if depends_on_thumbnail { From 4a9ef07645a278249ad2943aa16e4bd740b9c71e Mon Sep 17 00:00:00 2001 From: Johannes Marbach Date: Tue, 22 Apr 2025 21:59:13 +0200 Subject: [PATCH 05/17] Update doc comment for handle_dependent_file_or_thumbnail_upload --- crates/matrix-sdk/src/send_queue/upload.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/matrix-sdk/src/send_queue/upload.rs b/crates/matrix-sdk/src/send_queue/upload.rs index a0cdbf50f05..8c6ec7b74c4 100644 --- a/crates/matrix-sdk/src/send_queue/upload.rs +++ b/crates/matrix-sdk/src/send_queue/upload.rs @@ -349,7 +349,8 @@ impl QueueStorage { Ok(()) } - /// Consumes a finished upload of a thumbnail and queues the file upload. + /// Consumes a finished file or thumbnail upload and queues the dependent + /// file or thumbnail upload. #[allow(clippy::too_many_arguments)] pub(super) async fn handle_dependent_file_or_thumbnail_upload( &self, From de711e82e9af05cd1be19998597f49da52115684 Mon Sep 17 00:00:00 2001 From: Johannes Marbach Date: Tue, 22 Apr 2025 21:59:40 +0200 Subject: [PATCH 06/17] Don't unnecessarily clone the accumulated vector --- crates/matrix-sdk/src/send_queue/upload.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk/src/send_queue/upload.rs b/crates/matrix-sdk/src/send_queue/upload.rs index 8c6ec7b74c4..55ac964c593 100644 --- a/crates/matrix-sdk/src/send_queue/upload.rs +++ b/crates/matrix-sdk/src/send_queue/upload.rs @@ -386,7 +386,7 @@ impl QueueStorage { let accumulated = if depends_on_thumbnail { sent_media.accumulated } else { - let mut accumulated = sent_media.accumulated.clone(); + let mut accumulated = sent_media.accumulated; accumulated.push(AccumulatedSentMediaInfo { file: sent_media.file.clone(), thumbnail: sent_media.thumbnail, From 3206a05fc0179cd0be3c402e256f18d258dce731 Mon Sep 17 00:00:00 2001 From: Johannes Marbach Date: Tue, 22 Apr 2025 22:00:22 +0200 Subject: [PATCH 07/17] Extract local variable for depends_on_thumbnail --- Cargo.lock | 1 + crates/matrix-sdk/Cargo.toml | 1 + crates/matrix-sdk/src/send_queue/mod.rs | 12 +++++++++--- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 04130da61d5..1b2f54f14ab 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2871,6 +2871,7 @@ dependencies = [ "backoff", "bytes", "bytesize", + "cfg-if", "dirs 6.0.0", "event-listener", "eyeball", diff --git a/crates/matrix-sdk/Cargo.toml b/crates/matrix-sdk/Cargo.toml index 07529c10d17..6c688c2d946 100644 --- a/crates/matrix-sdk/Cargo.toml +++ b/crates/matrix-sdk/Cargo.toml @@ -115,6 +115,7 @@ urlencoding = "2.1.3" uuid = { workspace = true, features = ["serde", "v4"], optional = true } vodozemac = { workspace = true } zeroize = { workspace = true } +cfg-if = "1.0.0" [target.'cfg(target_arch = "wasm32")'.dependencies] gloo-timers = { workspace = true, features = ["futures"] } diff --git a/crates/matrix-sdk/src/send_queue/mod.rs b/crates/matrix-sdk/src/send_queue/mod.rs index 3cca07d9c35..57758e89dd9 100644 --- a/crates/matrix-sdk/src/send_queue/mod.rs +++ b/crates/matrix-sdk/src/send_queue/mod.rs @@ -1610,6 +1610,15 @@ impl QueueStorage { // Not finished yet, we should retry later => false. return Ok(false); }; + let depends_on_thumbnail = { + cfg_if::cfg_if! { + if #[cfg(feature = "unstable-msc4274")] { + depends_on_thumbnail + } else { + true + } + } + }; self.handle_dependent_file_or_thumbnail_upload( client, dependent_request.own_transaction_id.into(), @@ -1617,10 +1626,7 @@ impl QueueStorage { content_type, cache_key, related_to, - #[cfg(feature = "unstable-msc4274")] depends_on_thumbnail, - #[cfg(not(feature = "unstable-msc4274"))] - true, ) .await?; } From 8bbee70ec3f3f994186604c6e3cc545646b20f47 Mon Sep 17 00:00:00 2001 From: Johannes Marbach Date: Tue, 22 Apr 2025 22:12:41 +0200 Subject: [PATCH 08/17] Update comment explaining accumulation --- crates/matrix-sdk/src/send_queue/upload.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/matrix-sdk/src/send_queue/upload.rs b/crates/matrix-sdk/src/send_queue/upload.rs index 55ac964c593..8ce4d2abeda 100644 --- a/crates/matrix-sdk/src/send_queue/upload.rs +++ b/crates/matrix-sdk/src/send_queue/upload.rs @@ -379,9 +379,11 @@ impl QueueStorage { trace!(related_to = %event_txn, "done uploading file or thumbnail, now queuing the dependent file or thumbnail upload request"); - // If the previous upload was a thumbnail, forward the accumulated sent media - // unchanged. Otherwise, append the sent media to the accumulated - // vector. + // If the parent request was a thumbnail upload, don't add it to the list of + // accumulated medias yet because its dependent file upload is still + // pending. If the parent request was a file upload, we know that both + // the file and its thumbnail (if any) have finished uploading and we + // can add them to the accumulated sent media. #[cfg(feature = "unstable-msc4274")] let accumulated = if depends_on_thumbnail { sent_media.accumulated From e2511d9d84ae5d1beb5924250781d6d4a2da1a71 Mon Sep 17 00:00:00 2001 From: Johannes Marbach Date: Tue, 22 Apr 2025 22:13:09 +0200 Subject: [PATCH 09/17] Default depends_on_thumbnail to true if it is missing --- crates/matrix-sdk-base/src/store/send_queue.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/matrix-sdk-base/src/store/send_queue.rs b/crates/matrix-sdk-base/src/store/send_queue.rs index 7ddfd0685bd..4164a624790 100644 --- a/crates/matrix-sdk-base/src/store/send_queue.rs +++ b/crates/matrix-sdk-base/src/store/send_queue.rs @@ -239,7 +239,7 @@ pub enum DependentQueuedRequestKind { /// Whether the depended upon request was a thumbnail or a file upload. #[cfg(feature = "unstable-msc4274")] - #[serde(default)] + #[serde(default = "default_depends_on_thumbnail")] depends_on_thumbnail: bool, }, @@ -259,6 +259,12 @@ pub enum DependentQueuedRequestKind { }, } +/// If depends_on_thumbnail is missing, we assume the request is for a file +/// upload following a thumbnail upload. +fn default_depends_on_thumbnail() -> bool { + true +} + /// Detailed record about a thumbnail used when finishing a media upload. #[derive(Clone, Debug, Serialize, Deserialize)] pub struct FinishUploadThumbnailInfo { From 7bb3335f8e6997bce64062386bd53b702c1dc295 Mon Sep 17 00:00:00 2001 From: Johannes Marbach Date: Tue, 22 Apr 2025 22:17:28 +0200 Subject: [PATCH 10/17] Rename UploadFileOrThumbnail to UploadFileWithThumbnail during (de)serialization --- Cargo.lock | 1 + .../matrix-sdk-base/src/store/send_queue.rs | 1 + crates/matrix-sdk-sqlite/Cargo.toml | 1 + crates/matrix-sdk-sqlite/src/state_store.rs | 46 +++++++++++++++++-- 4 files changed, 46 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1b2f54f14ab..5f4e0db3000 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3204,6 +3204,7 @@ dependencies = [ name = "matrix-sdk-sqlite" version = "0.10.0" dependencies = [ + "as_variant", "assert_matches", "async-trait", "deadpool-sqlite", diff --git a/crates/matrix-sdk-base/src/store/send_queue.rs b/crates/matrix-sdk-base/src/store/send_queue.rs index 4164a624790..72217016480 100644 --- a/crates/matrix-sdk-base/src/store/send_queue.rs +++ b/crates/matrix-sdk-base/src/store/send_queue.rs @@ -227,6 +227,7 @@ pub enum DependentQueuedRequestKind { /// Upload a file or thumbnail depending on another file or thumbnail /// upload. + #[serde(rename = "UploadFileWithThumbnail")] UploadFileOrThumbnail { /// Content type for the file or thumbnail. content_type: String, diff --git a/crates/matrix-sdk-sqlite/Cargo.toml b/crates/matrix-sdk-sqlite/Cargo.toml index ac32218dded..fc567c959f3 100644 --- a/crates/matrix-sdk-sqlite/Cargo.toml +++ b/crates/matrix-sdk-sqlite/Cargo.toml @@ -17,6 +17,7 @@ event-cache = ["dep:matrix-sdk-base"] state-store = ["dep:matrix-sdk-base"] [dependencies] +as_variant = { workspace = true } async-trait = { workspace = true } deadpool-sqlite = "0.9.0" itertools = { workspace = true } diff --git a/crates/matrix-sdk-sqlite/src/state_store.rs b/crates/matrix-sdk-sqlite/src/state_store.rs index 8c496595843..3fa52b89e39 100644 --- a/crates/matrix-sdk-sqlite/src/state_store.rs +++ b/crates/matrix-sdk-sqlite/src/state_store.rs @@ -2237,8 +2237,10 @@ mod migration_tests { }, }; + use as_variant::as_variant; use deadpool_sqlite::Runtime; use matrix_sdk_base::{ + media::{MediaFormat, MediaRequestParameters}, store::{ ChildTransactionId, DependentQueuedRequestKind, RoomLoadSettings, SerializableEventContent, @@ -2250,13 +2252,14 @@ mod migration_tests { use once_cell::sync::Lazy; use ruma::{ events::{ - room::{create::RoomCreateEventContent, message::RoomMessageEventContent}, + room::{create::RoomCreateEventContent, message::RoomMessageEventContent, MediaSource}, StateEventType, }, - room_id, server_name, user_id, EventId, MilliSecondsSinceUnixEpoch, RoomId, TransactionId, - UserId, + room_id, server_name, user_id, EventId, MilliSecondsSinceUnixEpoch, OwnedTransactionId, + RoomId, TransactionId, UserId, }; use rusqlite::Transaction; + use serde::{Deserialize, Serialize}; use serde_json::json; use tempfile::{tempdir, TempDir}; use tokio::fs; @@ -2597,4 +2600,41 @@ mod migration_tests { Ok(()) } + + #[derive(Clone, Debug, Serialize, Deserialize)] + pub enum LegacyDependentQueuedRequestKind { + UploadFileWithThumbnail { + content_type: String, + cache_key: MediaRequestParameters, + related_to: OwnedTransactionId, + }, + } + + #[async_test] + pub async fn test_dependent_queued_request_variant_renaming() { + let path = new_path(); + let db = create_fake_db(&path, 7).await.unwrap(); + + let cache_key = MediaRequestParameters { + format: MediaFormat::File, + source: MediaSource::Plain("https://server.local/foobar".into()), + }; + let related_to = TransactionId::new(); + let request = LegacyDependentQueuedRequestKind::UploadFileWithThumbnail { + content_type: "image/png".to_owned(), + cache_key, + related_to: related_to.clone(), + }; + + let data = db + .serialize_json(&request) + .expect("should be able to serialize legacy dependent request"); + let deserialized: DependentQueuedRequestKind = db.deserialize_json(&data).expect( + "should be able to deserialize dependent request from legacy dependent request", + ); + + as_variant!(deserialized, DependentQueuedRequestKind::UploadFileOrThumbnail { related_to: de_related_to, .. } => { + assert_eq!(de_related_to, related_to); + }); + } } From ee947c6bed6b61591a1160e51c20e146ee113aac Mon Sep 17 00:00:00 2001 From: Johannes Marbach Date: Tue, 22 Apr 2025 22:23:16 +0200 Subject: [PATCH 11/17] Rename depends_on_thumbnail to parent_is_thumbnail_upload --- crates/matrix-sdk-base/src/store/send_queue.rs | 10 +++++----- crates/matrix-sdk/src/send_queue/mod.rs | 10 +++++----- crates/matrix-sdk/src/send_queue/upload.rs | 8 ++++---- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/crates/matrix-sdk-base/src/store/send_queue.rs b/crates/matrix-sdk-base/src/store/send_queue.rs index 72217016480..6f692f89597 100644 --- a/crates/matrix-sdk-base/src/store/send_queue.rs +++ b/crates/matrix-sdk-base/src/store/send_queue.rs @@ -240,8 +240,8 @@ pub enum DependentQueuedRequestKind { /// Whether the depended upon request was a thumbnail or a file upload. #[cfg(feature = "unstable-msc4274")] - #[serde(default = "default_depends_on_thumbnail")] - depends_on_thumbnail: bool, + #[serde(default = "default_parent_is_thumbnail_upload")] + parent_is_thumbnail_upload: bool, }, /// Finish an upload by updating references to the media cache and sending @@ -260,9 +260,9 @@ pub enum DependentQueuedRequestKind { }, } -/// If depends_on_thumbnail is missing, we assume the request is for a file -/// upload following a thumbnail upload. -fn default_depends_on_thumbnail() -> bool { +/// If parent_is_thumbnail_upload is missing, we assume the request is for a +/// file upload following a thumbnail upload. +fn default_parent_is_thumbnail_upload() -> bool { true } diff --git a/crates/matrix-sdk/src/send_queue/mod.rs b/crates/matrix-sdk/src/send_queue/mod.rs index 57758e89dd9..bf4c39493e6 100644 --- a/crates/matrix-sdk/src/send_queue/mod.rs +++ b/crates/matrix-sdk/src/send_queue/mod.rs @@ -1238,7 +1238,7 @@ impl QueueStorage { cache_key: file_media_request, related_to: send_event_txn.clone(), #[cfg(feature = "unstable-msc4274")] - depends_on_thumbnail: true, + parent_is_thumbnail_upload: true, }, ) .await?; @@ -1604,16 +1604,16 @@ impl QueueStorage { cache_key, related_to, #[cfg(feature = "unstable-msc4274")] - depends_on_thumbnail, + parent_is_thumbnail_upload, } => { let Some(parent_key) = parent_key else { // Not finished yet, we should retry later => false. return Ok(false); }; - let depends_on_thumbnail = { + let parent_is_thumbnail_upload = { cfg_if::cfg_if! { if #[cfg(feature = "unstable-msc4274")] { - depends_on_thumbnail + parent_is_thumbnail_upload } else { true } @@ -1626,7 +1626,7 @@ impl QueueStorage { content_type, cache_key, related_to, - depends_on_thumbnail, + parent_is_thumbnail_upload, ) .await?; } diff --git a/crates/matrix-sdk/src/send_queue/upload.rs b/crates/matrix-sdk/src/send_queue/upload.rs index 8ce4d2abeda..3a33f610179 100644 --- a/crates/matrix-sdk/src/send_queue/upload.rs +++ b/crates/matrix-sdk/src/send_queue/upload.rs @@ -360,7 +360,7 @@ impl QueueStorage { content_type: String, cache_key: MediaRequestParameters, event_txn: OwnedTransactionId, - depends_on_thumbnail: bool, + parent_is_thumbnail_upload: bool, ) -> Result<(), RoomSendQueueError> { // The previous file or thumbnail has been sent, now transform the dependent // file or thumbnail upload request into a ready one. @@ -370,7 +370,7 @@ impl QueueStorage { // If the previous upload was a thumbnail, it shouldn't have // a thumbnail itself. - if depends_on_thumbnail { + if parent_is_thumbnail_upload { debug_assert!(sent_media.thumbnail.is_none()); if sent_media.thumbnail.is_some() { warn!("unexpected thumbnail for a thumbnail!"); @@ -385,7 +385,7 @@ impl QueueStorage { // the file and its thumbnail (if any) have finished uploading and we // can add them to the accumulated sent media. #[cfg(feature = "unstable-msc4274")] - let accumulated = if depends_on_thumbnail { + let accumulated = if parent_is_thumbnail_upload { sent_media.accumulated } else { let mut accumulated = sent_media.accumulated; @@ -401,7 +401,7 @@ impl QueueStorage { cache_key, // If the previous upload was a thumbnail, it becomes the thumbnail source for the next // upload. - thumbnail_source: if depends_on_thumbnail { Some(sent_media.file) } else { None }, + thumbnail_source: if parent_is_thumbnail_upload { Some(sent_media.file) } else { None }, related_to: event_txn, #[cfg(feature = "unstable-msc4274")] accumulated, From cc5f6551a03430b5e60b66d5508b7024a0260041 Mon Sep 17 00:00:00 2001 From: Johannes Marbach Date: Tue, 22 Apr 2025 22:37:03 +0200 Subject: [PATCH 12/17] Add missing feature guard --- crates/matrix-sdk-base/src/store/send_queue.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/matrix-sdk-base/src/store/send_queue.rs b/crates/matrix-sdk-base/src/store/send_queue.rs index 6f692f89597..33b8ef7f077 100644 --- a/crates/matrix-sdk-base/src/store/send_queue.rs +++ b/crates/matrix-sdk-base/src/store/send_queue.rs @@ -262,6 +262,7 @@ pub enum DependentQueuedRequestKind { /// If parent_is_thumbnail_upload is missing, we assume the request is for a /// file upload following a thumbnail upload. +#[cfg(feature = "unstable-msc4274")] fn default_parent_is_thumbnail_upload() -> bool { true } From 42023775aae142c06de5e448c47203adf6e6880e Mon Sep 17 00:00:00 2001 From: Johannes Marbach Date: Thu, 24 Apr 2025 11:24:36 +0200 Subject: [PATCH 13/17] Use alias instead of rename Co-authored-by: Benjamin Bouvier Signed-off-by: Johannes Marbach --- crates/matrix-sdk-base/src/store/send_queue.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk-base/src/store/send_queue.rs b/crates/matrix-sdk-base/src/store/send_queue.rs index 33b8ef7f077..2d60e7477db 100644 --- a/crates/matrix-sdk-base/src/store/send_queue.rs +++ b/crates/matrix-sdk-base/src/store/send_queue.rs @@ -227,7 +227,7 @@ pub enum DependentQueuedRequestKind { /// Upload a file or thumbnail depending on another file or thumbnail /// upload. - #[serde(rename = "UploadFileWithThumbnail")] + #[serde(alias = "UploadFileWithThumbnail")] UploadFileOrThumbnail { /// Content type for the file or thumbnail. content_type: String, From 4272eaedd66be7ddae28001fcd97bca7a4a9d74a Mon Sep 17 00:00:00 2001 From: Johannes Marbach Date: Thu, 24 Apr 2025 11:27:14 +0200 Subject: [PATCH 14/17] Explain why parent_is_thumbnail_upload defaults to true --- crates/matrix-sdk-base/src/store/send_queue.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/matrix-sdk-base/src/store/send_queue.rs b/crates/matrix-sdk-base/src/store/send_queue.rs index 2d60e7477db..1d8f3e8c972 100644 --- a/crates/matrix-sdk-base/src/store/send_queue.rs +++ b/crates/matrix-sdk-base/src/store/send_queue.rs @@ -261,7 +261,8 @@ pub enum DependentQueuedRequestKind { } /// If parent_is_thumbnail_upload is missing, we assume the request is for a -/// file upload following a thumbnail upload. +/// file upload following a thumbnail upload. This was the only possible case +/// before parent_is_thumbnail_upload was introduced. #[cfg(feature = "unstable-msc4274")] fn default_parent_is_thumbnail_upload() -> bool { true From 4ac07d5c7fec94e0e31bed4c9783222f0f8a9a92 Mon Sep 17 00:00:00 2001 From: Johannes Marbach Date: Thu, 24 Apr 2025 11:30:58 +0200 Subject: [PATCH 15/17] Remove changelog for private API --- crates/matrix-sdk/CHANGELOG.md | 5 ----- 1 file changed, 5 deletions(-) diff --git a/crates/matrix-sdk/CHANGELOG.md b/crates/matrix-sdk/CHANGELOG.md index c93b7689445..700e86901e9 100644 --- a/crates/matrix-sdk/CHANGELOG.md +++ b/crates/matrix-sdk/CHANGELOG.md @@ -292,11 +292,6 @@ simpler methods: - The `issuer` field of `UserSession` was removed. - `SendHandle::media_handles` was generalized into a vector ([#4898](https://github.com/matrix-org/matrix-rust-sdk/pull/4898)) -- [**breaking] `QueueStorage::handle_dependent_file_upload_with_thumbnail` - was renamed to `handle_dependent_file_or_thumbnail_upload`. Under the - `unstable-msc4274` feature, it was generalized to allow chaining multiple - dependent file / thumbnail uploads. - ([#4897](https://github.com/matrix-org/matrix-rust-sdk/pull/4897)) ## [0.10.0] - 2025-02-04 From 9257a0cae5310e6139859ad3eb9261e5428295ec Mon Sep 17 00:00:00 2001 From: Johannes Marbach Date: Thu, 24 Apr 2025 11:35:41 +0200 Subject: [PATCH 16/17] Explain why parent_is_thumbnail_upload defaults to true in try_apply_single_dependent_request --- crates/matrix-sdk/src/send_queue/mod.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/matrix-sdk/src/send_queue/mod.rs b/crates/matrix-sdk/src/send_queue/mod.rs index bf4c39493e6..a5b223a455a 100644 --- a/crates/matrix-sdk/src/send_queue/mod.rs +++ b/crates/matrix-sdk/src/send_queue/mod.rs @@ -1615,6 +1615,9 @@ impl QueueStorage { if #[cfg(feature = "unstable-msc4274")] { parent_is_thumbnail_upload } else { + // Before parent_is_thumbnail_upload was introduced, the only + // possible usage for this request was a file upload following + // a thumbnail upload. true } } From 773d9287e447ab922d277d851c2d5c20d0affb2c Mon Sep 17 00:00:00 2001 From: Johannes Marbach Date: Thu, 24 Apr 2025 11:37:02 +0200 Subject: [PATCH 17/17] Use then_some instead of inline if/else Co-authored-by: Benjamin Bouvier Signed-off-by: Johannes Marbach --- crates/matrix-sdk/src/send_queue/upload.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk/src/send_queue/upload.rs b/crates/matrix-sdk/src/send_queue/upload.rs index 3a33f610179..fdc432808b7 100644 --- a/crates/matrix-sdk/src/send_queue/upload.rs +++ b/crates/matrix-sdk/src/send_queue/upload.rs @@ -401,7 +401,7 @@ impl QueueStorage { cache_key, // If the previous upload was a thumbnail, it becomes the thumbnail source for the next // upload. - thumbnail_source: if parent_is_thumbnail_upload { Some(sent_media.file) } else { None }, + thumbnail_source: parent_is_thumbnail_upload.then_some(sent_media.file), related_to: event_txn, #[cfg(feature = "unstable-msc4274")] accumulated,