refactor: Include file name when uploading unencrypted media #5430

jmartinesp · 2025-07-22T09:05:57Z

Implementation for element-hq/element-meta#2459. The fields in the API were already there, but there was nothing providing the file name to the upload requests.

Changes:

Allow uploading unencrypted media files with their original file name.
Implement this for the serialized requests for the send queue too.
Also add support for this in the media gallery APIs.
Add a special case for thumbnails too so they can reference the original file name too.
Public API changes documented in changelogs (optional)

Signed-off-by:

This will rename the uploaded file in the media server so it references the original one.

…iginal file

jmartinesp · 2025-07-22T09:22:21Z

crates/matrix-sdk/src/media.rs

@@ -748,8 +757,9 @@ impl Media {

        let (data, content_type, thumbnail_info) = thumbnail.into_parts();

+        let filename = filename.map(|name| format!("thumbnail-{name}"));


I'm not sure if there would be a better way to reflect this is a thumbnail and at the same time reference the original file 🫤 .

What's the use case for providing a file for the thumbnail, out of curiosity? What values does it provide?

If you want to search for a certain filename to delete some media I guess you'd want to remove the associated thumbnail too. I don't know if the media server has a better way to handle deleting both, this might not be needed.

I think we'd need to take an optional thumbnail filename, to make it entirely user controlled, and not something based on our internals. What do you think? (can come as a subsequent PR/commit too)

bnjbvr

In commit 3: why do we need to pass the filename for each media upload? Isn't the filename supposed to live in the media event content itself? I'm confused.

bnjbvr · 2025-07-22T09:21:55Z

crates/matrix-sdk-base/src/media.rs

@@ -163,6 +170,10 @@ impl MediaEventContent for AudioMessageEventContent {
        Some(self.source.clone())
    }

+    fn filename(&self) -> Option<String> {
+        self.filename.clone()


Unfortunately, I think this might need to be more complicated, see also:

matrix-rust-sdk/crates/matrix-sdk/src/room/mod.rs

Lines 222 to 228 in 7c8f870

// If caption is set, use it as body, and filename as the file name; otherwise,

// body is the filename, and the filename is not set.

// https://github.com/matrix-org/matrix-spec-proposals/blob/main/proposals/2530-body-as-caption.md

let (body, filename) = match $caption {

Some(caption) => (caption, Some($filename)),

None => ($filename, None),

};

I took that into account here for the send queue, although maybe I missed some case and I think the non-send queue part will just use the provided filename parameter.

EDIT: I ended up doing 2e32c25

bnjbvr · 2025-07-22T09:48:01Z

crates/matrix-sdk/src/media.rs

@@ -200,13 +202,15 @@ impl Media {
        &self,
        content_type: &Mime,
        data: Vec<u8>,
+        filename: Option<String>,
        request_config: Option<RequestConfig>,


Having two optional parameters in this method isn't pretty, because it's unclear what None is about. I think we can refactor this method, in the same way as we do for Client.send(), since this already returns a SendMediaUploadRequest object that can be configured:

move the creation of the media::create_content::v3::Request to SendMediaUploadRequest::into_future. This means storing the request parameters + &client instance in the SendMediaUploadRequest.

add a builder method on SendMediaUploadRequest to allow providing a custom RequestConfig; it'd be set to None in the ctor of SendMediaUploadRequest, and extracted and used in the actual request created in into_future

add a builder method for providing a custom filename, the same way.

How does that sound?

Sounds good, the API would be way better this way!

bnjbvr · 2025-07-22T09:59:36Z

crates/matrix-sdk/src/media.rs

@@ -748,8 +757,9 @@ impl Media {

        let (data, content_type, thumbnail_info) = thumbnail.into_parts();

+        let filename = filename.map(|name| format!("thumbnail-{name}"));


What's the use case for providing a file for the thumbnail, out of curiosity? What values does it provide?

jmartinesp · 2025-07-22T10:25:17Z

In commit 3: why do we need to pass the filename for each media upload? Isn't the filename supposed to live in the media event content itself? I'm confused.

You mean in the bindings one? I added the new optional filename parameter because on Android i.e. when we pre-process some media for upload we create a temp file for it which will use an UUID for its filename and we don't want to upload the media with that filename, but with its original one my-cat.jpg instead.

…nt::filename_or_body` since it was previously clashing with another function definition. Also, make sure it returns either the filename or the body if that's missing - which should contain the actual filename in that case.

jmartinesp · 2025-07-22T11:16:24Z

I'm about to rebase to fix the conflict, but nothing changed in the existing commits. A couple of new commits were added.

codecov · 2025-07-22T11:34:18Z

Codecov Report

Attention: Patch coverage is 75.91241% with 33 lines in your changes missing coverage. Please review.

Project coverage is 88.82%. Comparing base (b482ccd) to head (58d5f6e).
Report is 3 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-base/src/media.rs	38.46%	16 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/mod.rs	55.55%	1 Missing and 3 partials ⚠️
crates/matrix-sdk/src/account.rs	0.00%	3 Missing ⚠️
crates/matrix-sdk/src/room/mod.rs	0.00%	3 Missing ⚠️
crates/matrix-sdk/src/send_queue/mod.rs	88.88%	3 Missing ⚠️
crates/matrix-sdk/src/client/futures.rs	94.28%	2 Missing ⚠️
crates/matrix-sdk/src/encryption/futures.rs	80.00%	0 Missing and 1 partial ⚠️
crates/matrix-sdk/src/media.rs	95.65%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5430      +/-   ##
==========================================
- Coverage   88.85%   88.82%   -0.04%     
==========================================
  Files         333      333              
  Lines       91252    91326      +74     
  Branches    91252    91326      +74     
==========================================
+ Hits        81078    81116      +38     
- Misses       6347     6381      +34     
- Partials     3827     3829       +2

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

… a media upload request with a more flexible API. Use `SendMediaUploadRequest` in `Media::upload`

…s `None`, fix wrong implementation for `ImageMessageEventContent`

bnjbvr

Sorry for the long review time! I really wanted the media upload progress changes to land first, as the progress PR was slightly bigger and older.

This PR is touching many files, and I think some of my suggestions have been lost in translation unfortunately, so if you're in the mood, it might be sweet to split it up.

bnjbvr · 2025-07-31T11:48:46Z

crates/matrix-sdk-base/src/media.rs

@@ -142,6 +142,9 @@ pub trait MediaEventContent {
    /// Returns `None` if `Self` has no file.
    fn source(&self) -> Option<MediaSource>;

+    /// Get the name of the uploaded file for `Self`.
+    fn filename_or_body(&self) -> Option<String>;


I don't think this is a good API; a final user wants the filename, or the caption, but they shouldn't have to know about the Matrix Client-Server API to know how to compute one or the other.

Instead, I'd recommend having both fn filename() and fn caption() that do the right thing, and figure the expected value, according to the specification.

bnjbvr · 2025-07-31T11:55:53Z

crates/matrix-sdk-ui/src/timeline/mod.rs

@@ -821,21 +821,24 @@ pub enum AttachmentSource {
    /// An attachment loaded from a file.
    ///
    /// The bytes and the filename will be read from the file at the given path.
-    File(PathBuf),
+    File { filename: Option<String>, path: PathBuf },


Why to require that the filename be provided in addition to the PathBuf, which also includes the filename?

bnjbvr · 2025-07-31T11:59:36Z

crates/matrix-sdk/src/client/futures.rs

+    }
+
+    /// Creates the [`SendRequest`] using the provided parameters.
+    pub fn build_send_request(self) -> SendRequest<media::create_content::v3::Request> {


I don't think this should be pub, in fact it could be inlined in the into_future impl instead

bnjbvr · 2025-07-31T11:59:59Z

crates/matrix-sdk/src/client/futures.rs

+    /// Returns the data to upload.
+    pub fn data(&self) -> &Vec<u8> {
+        &self.request.file
+    }


Don't think it should be needed? (If it is, then it should return &[u8] by convention)

bnjbvr · 2025-07-31T12:03:57Z

crates/matrix-sdk/src/media.rs

+    pub async fn upload(
        &self,
-        content_type: &Mime,
-        data: Vec<u8>,
-        request_config: Option<RequestConfig>,
-    ) -> SendMediaUploadRequest {
-        let request_config = request_config.unwrap_or_else(|| {
-            self.client.request_config().timeout(Self::reasonable_upload_timeout(&data))
-        });
-
-        let request = assign!(media::create_content::v3::Request::new(data), {
-            content_type: Some(content_type.essence_str().to_owned()),
+        send_media_upload_request: SendMediaUploadRequest,
+    ) -> Result<media::create_content::v3::Response, Error> {


This is not right. Instead, it should take the basic parameters that are always needed for a SendMediaUploadRequest, and return it. As a result, it should not be async, and it should resolve the response. The way to use it would be:

client.media().upload(file_bytes_vec).with_request_config(RequestConfig::default()).await?;

and so on.

It may be better to split this change into a separate PR? The history of this one is already a bit messy, and this is a problem orthogonal to the one at hand. (I'd be happy reviewing the split up PR too, if that helps speeding up the review!)

bnjbvr · 2025-07-31T12:07:23Z

crates/matrix-sdk-base/src/store/send_queue.rs

@@ -109,6 +109,9 @@ pub enum QueuedRequestKind {
        #[cfg(feature = "unstable-msc4274")]
        #[serde(default)]
        accumulated: Vec<AccumulatedSentMediaInfo>,
+
+        /// The name of the file to upload, if known or public.


Suggested change

/// The name of the file to upload, if known or public.

/// The name of the file to upload.

bnjbvr · 2025-07-31T12:07:30Z

crates/matrix-sdk-base/src/store/send_queue.rs

@@ -241,6 +244,9 @@ pub enum DependentQueuedRequestKind {
        #[cfg(feature = "unstable-msc4274")]
        #[serde(default = "default_parent_is_thumbnail_upload")]
        parent_is_thumbnail_upload: bool,
+
+        /// The name of the file to upload, if known.


Suggested change

/// The name of the file to upload, if known.

/// The name of the file to upload.

bnjbvr · 2025-07-31T12:08:19Z

crates/matrix-sdk/src/client/futures.rs

-    pub fn new(request: SendRequest<media::create_content::v3::Request>) -> Self {
-        Self { send_request: request }
+    /// Creates a new instance.
+    pub fn new(client: Client, file: Vec<u8>) -> Self {


(This ctor shouldn't be public, if taking into account the change I suggested with respect to Media::upload() returning the SendMediaUploadRequest)

bnjbvr · 2025-07-31T12:10:43Z

crates/matrix-sdk/src/send_queue/mod.rs

            } => {
+                warn!("Saving send queue request for filename {filename:?}");


nit: debug warn! remaining here

bnjbvr · 2025-07-31T12:12:16Z

crates/matrix-sdk/src/test_utils/mocks/mod.rs

@@ -2624,6 +2628,13 @@ impl<'a> MockEndpoint<'a, UploadEndpoint> {
    /// Returns a upload endpoint that emulates success, i.e. the media has been
    /// uploaded to the media server and can be accessed using the given
    /// event has been sent with the given [`MxcUri`].
+    /// Expect the filename query param to match the provided value.
+    pub fn expect_filename(self, filename: &str) -> Self {


I've started to use the match_ prefix to mean "a parameter must match this value", so lightly suggesting to do it here too. (Strictly speaking it's not "expecting" as it's not asserting; if the filename isn't provided, the mock will silently not catch the request, and it may run into a 404 instead)

…but i realize the equivalent for mime type is called expect_mime_type, so it's fun to use expect_ here, or better to rename the other one to match_mime_type as well. Your choice :)

jmartinesp added 4 commits July 22, 2025 09:56

misc: Use filename when uploading unencrypted media

c2338b7

This will rename the uploaded file in the media server so it references the original one.

misc: Use filename for the send queue and gallery too

f072fa7

misc(ffi): Add filename to the FFI bindings too

cecf017

misc: Add filename to the thumbnails too so they can reference the or…

b868702

…iginal file

jmartinesp requested a review from a team as a code owner July 22, 2025 09:05

jmartinesp requested review from stefanceriu and removed request for a team July 22, 2025 09:05

bnjbvr requested review from bnjbvr and removed request for stefanceriu July 22, 2025 09:06

jmartinesp commented Jul 22, 2025

View reviewed changes

bnjbvr requested changes Jul 22, 2025

View reviewed changes

jmartinesp force-pushed the misc/upload-unencrypted-media-with-filename branch from 4c37c86 to ebe9038 Compare July 22, 2025 11:16

jmartinesp added 2 commits July 22, 2025 14:14

refactor: Make SendMediaUploadRequest a builder, allowing to create…

d665ab2

… a media upload request with a more flexible API. Use `SendMediaUploadRequest` in `Media::upload`

refactor: Avoid unnecessary cloning if MediaEventContent.filename i…

713ae36

…s `None`, fix wrong implementation for `ImageMessageEventContent`

jmartinesp force-pushed the misc/upload-unencrypted-media-with-filename branch from 5cc936b to 713ae36 Compare July 22, 2025 12:14

refactor: Remove duplicate check for filename/body in the event content

58d5f6e

jmartinesp changed the title ~~misc: Include file name when uploading unencrypted media~~ refactor: Include file name when uploading unencrypted media Jul 22, 2025

bnjbvr self-requested a review July 22, 2025 12:55

bnjbvr requested changes Jul 31, 2025

View reviewed changes

		@@ -748,8 +757,9 @@ impl Media {

		let (data, content_type, thumbnail_info) = thumbnail.into_parts();

		let filename = filename.map(\|name\| format!("thumbnail-{name}"));

	// If caption is set, use it as body, and filename as the file name; otherwise,
	// body is the filename, and the filename is not set.
	// https://github.com/matrix-org/matrix-spec-proposals/blob/main/proposals/2530-body-as-caption.md
	let (body, filename) = match $caption {
	Some(caption) => (caption, Some($filename)),
	None => ($filename, None),
	};

	/// The name of the file to upload, if known or public.
	/// The name of the file to upload.

		} => {
		warn!("Saving send queue request for filename {filename:?}");

refactor: Include file name when uploading unencrypted media #5430

Are you sure you want to change the base?

refactor: Include file name when uploading unencrypted media #5430

Uh oh!

Conversation

jmartinesp commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnjbvr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmartinesp Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmartinesp commented Jul 22, 2025

Uh oh!

jmartinesp commented Jul 22, 2025

Uh oh!

codecov bot commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bnjbvr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmartinesp commented Jul 22, 2025 •

edited

Loading

jmartinesp Jul 22, 2025 •

edited

Loading

codecov bot commented Jul 22, 2025 •

edited

Loading