refactor(send_queue): generalize SentRequestKey::Media and DependentQueuedRequestKind::UploadFileWithThumbnail to prepare for MSC4274 gallery uploads by Johennes · Pull Request #4897 · matrix-org/matrix-rust-sdk

Johennes · 2025-04-07T18:04:21Z

This was broken out of #4838 and is a preliminary step towards implementing MSC4274. SentRequestKey::Media and DependentQueuedRequestKind::UploadFileWithThumbnail are generalized to allow chaining dependent media uploads and accumulating sent media sources.

Public API changes documented in changelogs (optional)

…ueuedRequestKind::UploadFileWithThumbnail to prepare for MSC4274 gallery uploads Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>

codecov · 2025-04-07T18:19:03Z

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.70%. Comparing base (f7f07e7) to head (773d928).
Report is 171 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk/src/send_queue/upload.rs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4897   +/-   ##
=======================================
  Coverage   85.69%   85.70%           
=======================================
  Files         309      309           
  Lines       35398    35400    +2     
=======================================
+ Hits        30334    30338    +4     
+ Misses       5064     5062    -2

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Hywan

Thanks for the patch.
I believe it would be lovely to add a couple of tests just to ensure the accumulated field correctly accumulates the media info. Is it doable?

Johennes · 2025-04-08T09:34:08Z

I believe it would be lovely to add a couple of tests just to ensure the accumulated field correctly accumulates the media info. Is it doable?

It is slightly tricky because I think currently it can only be tested indirectly through the final event that is being sent. For galleries the code to send the event is still missing, however. I was planning to add it with a separate PR, including tests. I realize this isn't ideal but I'm not sure how to best test it in this PR here. 😕

Hywan

It makes sense to me, thank you. @bnjbvr would you like to take a look too?

bnjbvr · 2025-04-16T08:57:34Z

Yep, thanks for the ping.

bnjbvr

Thanks! There might be a subtle breaking data format change here, so requesting changes to double-check this with a test, at least.

bnjbvr · 2025-04-17T09:24:44Z

+    /// 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<AccumulatedSentMediaInfo>,


There might be a different possible data scheme here, if I understand correctly:

SentMediaInfo contains only the vec of AccumulatedSentMediaInfo (since a vec can hold the pair for the media, when there's only a single one and no gallery)

AccumulatedSentMediaInfo (maybe rename SingleSentMediaInfo?) can keep on holding its current fields.

Again, I see this struct SentMediaInfo is also marked as Serialized/Deserialize, so maybe what I'm suggesting implies having to a data migration 🥴.

I think technically this would work, yes. I didn't go for it originally because I wanted to minimize breaking changes and because it felt slightly cleaner to have accumulated only contain the requests that are actually finished. This way, we only push to accumulated. If we do it the other way around, we'll need to push or modify the last item depending on what the request is for. Did you have specific advantages of using this variant in mind?

The only advantage is technical, in that it avoids containing what's effectively another flattened AccumulatedSentMediaInfo next to the accumulated vector, and also reduces the number of concepts.

I'm a bit torn, because this is likely a non-trivial change, but on the other hand that might be a nice simplification. Maybe that could be attempted as a follow-up, and we could open an issue to not forget about it?

Yeah, I see the oddness. Sounds good on the issue. I have opened: #4969

…rialization

bnjbvr

Looks good to me, thanks. Approving so that we can merge this as soon as my review comments have been addressed. Any chance we can also include some unit tests relevant to the changes here, or is that not worth it?

bnjbvr · 2025-04-24T08:50:04Z

+    /// 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<AccumulatedSentMediaInfo>,


The only advantage is technical, in that it avoids containing what's effectively another flattened AccumulatedSentMediaInfo next to the accumulated vector, and also reduces the number of concepts.

I'm a bit torn, because this is likely a non-trivial change, but on the other hand that might be a nice simplification. Maybe that could be attempted as a follow-up, and we could open an issue to not forget about it?

bnjbvr · 2025-04-24T08:54:07Z

+        // 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.


Super nice comment, thanks!

Co-authored-by: Benjamin Bouvier <benjamin@bouvier.cc> Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>

…single_dependent_request

Co-authored-by: Benjamin Bouvier <benjamin@bouvier.cc> Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>

Johennes · 2025-04-24T09:55:09Z

Thanks for the merge! Re the unit tests, I just wanted to say that it's a bit tricky on this PR because most of the changes are just routing new fields through existing APIs but nothing really uses these new fields yet. Therefore, I was planning to add tests in future PRs when the accumulation is actually being used by galleries.

refactor(send_queue): generalize SentRequestKey::Media and DependentQ…

ad72a6d

…ueuedRequestKind::UploadFileWithThumbnail to prepare for MSC4274 gallery uploads Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>

Johennes added 2 commits April 7, 2025 20:26

Add changelogs

c97f8f8

Merge branch 'main' into johannes/msc4274-step1

88d7f18

Johennes marked this pull request as ready for review April 7, 2025 18:28

Johennes requested a review from a team as a code owner April 7, 2025 18:28

Johennes requested review from Hywan and removed request for a team April 7, 2025 18:28

Johennes mentioned this pull request Apr 7, 2025

MSC4274: Inline media galleries via msgtypes matrix-org/matrix-spec-proposals#4274

Open

Hywan requested changes Apr 8, 2025

View reviewed changes

Johennes added 2 commits April 8, 2025 10:51

Fix typo

fb61003

Merge branch 'main' into johannes/msc4274-step1

8888ee8

Johennes requested a review from Hywan April 8, 2025 09:34

Fix another typo

2ab77d4

Hywan approved these changes Apr 16, 2025

View reviewed changes

bnjbvr self-requested a review April 16, 2025 08:57

bnjbvr requested changes Apr 17, 2025

View reviewed changes

Johennes added 8 commits April 22, 2025 21:59

Update doc comment for handle_dependent_file_or_thumbnail_upload

4a9ef07

Don't unnecessarily clone the accumulated vector

de711e8

Extract local variable for depends_on_thumbnail

3206a05

Update comment explaining accumulation

8bbee70

Default depends_on_thumbnail to true if it is missing

e2511d9

Rename UploadFileOrThumbnail to UploadFileWithThumbnail during (de)se…

7bb3335

…rialization

Rename depends_on_thumbnail to parent_is_thumbnail_upload

ee947c6

Add missing feature guard

cc5f655

Johennes requested a review from bnjbvr April 22, 2025 20:37

bnjbvr approved these changes Apr 24, 2025

View reviewed changes

Use alias instead of rename

4202377

Co-authored-by: Benjamin Bouvier <benjamin@bouvier.cc> Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>

Johennes and others added 4 commits April 24, 2025 11:27

Explain why parent_is_thumbnail_upload defaults to true

4272eae

Remove changelog for private API

4ac07d5

Explain why parent_is_thumbnail_upload defaults to true in try_apply_…

9257a0c

…single_dependent_request

Use then_some instead of inline if/else

773d928

Co-authored-by: Benjamin Bouvier <benjamin@bouvier.cc> Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>

bnjbvr enabled auto-merge (squash) April 24, 2025 09:38

Johennes mentioned this pull request Apr 24, 2025

Rearrange SentMediaInfo and AccumulatedSentMediaInfo #4969

Open

bnjbvr merged commit 1554e05 into matrix-org:main Apr 24, 2025
43 checks passed

Conversation

Johennes commented Apr 7, 2025

Uh oh!

codecov Bot commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Hywan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Johennes commented Apr 8, 2025

Uh oh!

Hywan left a comment

Choose a reason for hiding this comment

Uh oh!

bnjbvr commented Apr 16, 2025

Uh oh!

bnjbvr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bnjbvr Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

Johennes Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

bnjbvr Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

Johennes Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bnjbvr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bnjbvr Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bnjbvr Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Johennes commented Apr 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Apr 7, 2025 •

edited

Loading