actix-web icon indicating copy to clipboard operation
actix-web copied to clipboard

Support for multipart form extractors

Open jacob-pro opened this issue 2 years ago • 39 comments

If you have a look on crates.io there are a proliferation of crates for doing multipart form uploads with files in actix.

Including:

I think each of these have some unique improvements and features that the others do not. It seems silly to have so many unofficial crates, and is not very friendly to people who just want to quickly get started with actix.

I am wondering if we should have an effort to consolidate all of these crates into one official one in the actix project?

jacob-pro avatar Aug 15 '22 13:08 jacob-pro

Hey there! As someone mentioned here, I have a requirement for any consolidation work on this front. I haven't looked at each of these options so I can't speak for what features they do or don't have, but I think it is important to be able to support streaming files from a multipart upload into various upstreams (files, object storage, etc). Just aggregating a file upload into a Bytes or a Vec isn't flexible enough, especially if uploaded files are potentially large

That's the primary motivation behind my library, which I use in my image host API pict-rs (example: https://git.asonix.dog/asonix/pict-rs/src/branch/main/src/main.rs#L814-L833)

asonix avatar Aug 15 '22 14:08 asonix

Yep ~~I think all~~ some of these support at least writing files. In my implementation stuff gets written to a NamedTempFile which you can then choose to persist.

What I might do a bit later when I have time is write up a matrix of all the different features these crates have, so we can get an idea of what an official crate would need to include.

jacob-pro avatar Aug 15 '22 14:08 jacob-pro

I also think that maybe moving to just an Extractor would be a better idea than my current api, which is a combination Middleware + Extractor. Maybe I'll play with updating my library to work more like that

edit (more details): Currently, I support multiple different "Form" middlewares that you can register for different routes. To make this work as a more extractor-driven API, I would need to change the Form from a middleware to extension data, then move the stream processing logic to a FromRequest that accesses that extension. This would allow adding a single form as top-level data if you only need one upload endpoint for simplicity, but different scopes would still need different forms to handle multiple endpoints (nothing a couple example programs couldn't demonstrate)

edit 2 (even more details): An alternative to registering different forms at different scopes would be to allow tagging a form implementation with some additional data (e.g. a unit struct) and then referencing that same unit struct in the extractor. This would simplify scoping forms to routes, but would increase the complexity of the extractor type

asonix avatar Aug 15 '22 15:08 asonix

Mine uses serde and does not support streaming. It was mostly just a proof of concept but I've been locally working on something similar to a builder but with a procmacro to auto parse into the struct which is what I think would be best.

JSH32 avatar Aug 15 '22 15:08 JSH32

@JSH32 fwiw the proc-macro "extract into this struct" method is really nice, and ideally that would be the final API. Maybe if there was an annotation you could put on a field like #[stream_with(handler)] to allow custom stream processing while still storing the result in the struct

asonix avatar Aug 15 '22 16:08 asonix

@asonix You could allow both Vec<u8> for getting just the bytes or something which is an AsyncRead (or something like that) to put the stream into the struct.

JSH32 avatar Aug 15 '22 16:08 JSH32

@JSH32 yeah I was impressed with your serde compatible macro - it is a lot better than my one!

jacob-pro avatar Aug 15 '22 16:08 jacob-pro

@JSH32 that doesn't quite work, because you need to process each field in order. If you embed a file stream directly into a struct for processing later, you don't read the Multipart stream any farther and can't "deserialize" fields that come afterwards.

asonix avatar Aug 15 '22 16:08 asonix

@JSH32 that doesn't quite work, because you need to process each field in order. If you embed a file stream directly into a struct for processing later, you don't read the Multipart stream any farther and can't "deserialize" fields that come afterwards.

The problem is that a lot of the time the processor for the field will need to have context of the rest of the data, and reading the pure request is kind of unintuitive. Maybe save the original stream as a temporary file and provide a StreamExt for reading the bytes from temporary?

JSH32 avatar Aug 15 '22 16:08 JSH32

Saving to temporary files solves the problem for a number of use-cases, but prevents creating a pure "file proxy" implementation, which could stream files in-line to some other service. It requires either using tmpfs (which means holding files in memory) or using a filesystem, which adds requirements for deploying the service

I like the ToStream trait, but maybe it could look something like this:

trait FromStream {
    type Error: ResponseError;
    type Future: Future<Output = Result<Self, Self::Error>>;
    
    // implementations must ensure they consume the entire stream if they complete successfully,
    // otherwise the multipart stream will be stuck and no further fields can be processed (leading to a deadlock)
    fn from_stream(req: HttpRequest, stream: actix_multipart::Field /* or maybe some other opaque stream type */) -> Self::Future;
}

struct StreamMetadata {
    object_storage_id: String,
}

impl FromStream for StreamMetadata {
    type Error = actix_web::Error;
    type Future = Pin<Box<dyn Future<Output = Result<Self, Self::Error>>>>;
    
    fn from_stream(req: HttpRequest, stream: Field) -> Self::Future {
        // extract useful things from HttpRequest here
        // ...
        
        Box::pin(async move {
            // upload to minio or s3 or something here
            // ...
            
            Ok(StreamMetadata { object_storage_id: /* blah */ })
        })
    }
}

#[derive(Multipart)]
struct MyForm {
    file_field: StreamMetadata
}

asonix avatar Aug 15 '22 16:08 asonix

That definitely works. Still kind of meh though because you cant get the full metadata of a route for conditionally uploading based on something like a user and will need to do processing of your form data outside of the route but I'm pretty sure its the only way to do it right.

JSH32 avatar Aug 15 '22 16:08 JSH32

This FromStream trait could be implemented for PathBuf (or maybe a custom TemporaryPathbuf) that would stream the files onto a disk and return their path in the deserialized struct

asonix avatar Aug 15 '22 16:08 asonix

Thanks @asonix @JSH32

I've had a think about it and I think these are all the features we will need:

What we need to do with streams of bytes:

  1. Store the bytes in memory, e.g.
    pub struct BytePart {
        pub bytes: Vec<u8>,
        pub filename: Option<String>,
        pub mime: Mime,
    }
    
  2. Temporary files on disk (that can later be persisted)
    pub struct FilePart {
        /// The file data itself stored as a temporary file on disk.
        pub file: NamedTempFile,
        /// The size in bytes of the file.
        pub size: usize,
        /// The `filename` value in the `Content-Disposition` header.
        pub filename: Option<String>,
        /// The Content-Type specified as reported in the uploaded form.
        pub mime: mime::Mime,
    }
    
  3. An arbitrary user implemented future that streams the bytes elsewhere (e.g. cloud object storage)
    pub struct ObjectPart {
        /// The file has been uploaded to an external service
        pub object_id: String,
    }
    

As suggested by @asonix the solution could be to require each field to implement a trait, something like:

pub trait HandlePart {
   // Providing the request context allows pulling from app_data() to read config etc.
   fn handler(req: HttpRequest, stream: Field) -> Future<Output = Result<Self>>;
}

struct Form {
    bytes_in_memory: BytePart,
    file_on_disk: FilePart,
    file_in_s3: ObjectPart
}

What about simple text fields?

~~We could provide a handler for T: FromStr which would cover all of these?~~

~~We could provide a handler for T: Deserialize, and use it with the serde_plain crate which would convert text/plain into primitive rust types and basic enums.~~

Edit: We can provide another part implementation:

/// Deserializes the part in memory as plain text (using serde_plain)
#[derive(Deref)]
struct TextPart<T: DeserializeOwned>(T);

This would allow deserializing a plain text field into a simple type, e.g. strings, ints, floats, and plain enums.

It also does not restrict us to a single deserialization format, because we could introduce additional implementations such as:

/// Deserializes the part in memory as JSON
#[derive(Deref)]
struct JsonPart<T: DeserializeOwned>(T);

How should we handle Lists and Optionals?

~~We would need to provide our own containers that implement the handler trait. (We can't implement directly on Vec/Option because it would conflict with the generic Deserialize implementation).~~

According to the spec:

To match widely deployed implementations, multiple files MUST be sent by supplying each file in a separate part but all with the same "name" parameter.

Using something like compile_type_eq (thanks @JSH32) the macro could detect the use of Vec and Option wrapped fields:

struct Form {
    files: Vec<S3Upload>,
    maybe: Option<String>,
}

A Vec can be used when multiple parts arrive with the same "name". An an option can be used to represent an optional part that may or may not be in the form.

Unknown and Extra fields

Serde allows using #[serde(deny_unknown_fields)] to prevent surplus fields - we should probably have something similar?

A further question is what to do if multiple fields are uploaded with the same name, but we are not expecting a Vector?

My crate has the #[from_multipart(deny_extra_parts)] which errors if:

  • A form is uploaded with an unknown field name
  • A form is uploaded with a duplicate field name (and the type is not a Vector)

But these two cases should really be separately configurable.

The duplicate field case should actually have 3 actions - keep first, keep last or error?

Renaming fields

We need to be able to support something similar to #[serde(rename = "name")] for cases where we can't use the field-name e.g. rust keywords or different casing styles.

Data limits

We need to be able to set a global limit on the amount of data read into memory, for basic DDoS protection, just as the official actix extractors support. It would have to be up to the implementor of the handler to choose whether to respect / consume from the in-memory limits.

But we will also want to provide field level limits as @JSH32 has done, which will be useful for files etc.

pub struct ExampleForm {
    #[multipart(max_size = 5MB)]
    file_field: File,
}

Let me know if you can think of anything I have missed?

jacob-pro avatar Aug 21 '22 18:08 jacob-pro

@robjtede Please can you advise if this sort of extractor would be accepted into acix-extras and what would be an appropriate name for it?

Or actually maybe it should go into the actix-multipart crate itself - which would clear up a lot of the naming confusion?

If so, I am happy to write a lot of the code for this - but would just like to know where to get started from an organizational standpoint? Maybe we can have a WIP/feature branch in the actix repository which we can open PRs against?

jacob-pro avatar Aug 22 '22 15:08 jacob-pro

The handler will already be generated by a derive macro for introspecting field names/options so Vec/Option can just be done there and conditionally code can be generated for that.

JSH32 avatar Aug 22 '22 16:08 JSH32

@JSH32 I think you will find this is easier said than done? - I could be wrong but when I tried doing that with my macro I couldn't get it to work

The problem is how do you tell if something is a std::vec::Vec or std::option::Option - I don't think macros are smart enough to do this - they can't do import resolution. E.g. what if a user is using a type definition etc.

In my macro I came up with a hacky work around which is to define two traits with the same method name, the first trait implemented for T: FromStr and the second trait implemented for Vec/Option. And then using T::method_name() to pull the data (compiler chooses right trait automatically). But this is kind of ugly and I don't want to do it again

jacob-pro avatar Aug 22 '22 16:08 jacob-pro

Is there a reason to use FromStr over an implementation with serde like @JSH32's? I may be misunderstanding the issue, but I'm imagining an interface like this:

/// The resulting struct from an upload.
/// 
/// All fields must either implement `Deserialize` or `HandlePart`
#[derive(Multipart)]
struct Upload {
    image: File,
    stream: Stream<Item = u8>,
    s3_upload: ImplementsS3Upload,
    id: Option<i32>,
    list: Vec<String>,
    metadata: Metadata,
}

#[derive(Deserialize)]
struct Metadata { ... }

struct ImplementsS3Upload { ... }
impl HandlePart for ImplementsS3Upload { ... }

junbl avatar Aug 22 '22 21:08 junbl

@e-rhodes Yeah I did also wonder about that - it would possibly make error handling easier - but my question is which deserializer were you thinking of using? We could use serde_plain - which works for primitive data types and simple enums? https://github.com/mitsuhiko/serde-plain I think this would be the only thing that make sense in the case of text/plain fields at least

jacob-pro avatar Aug 22 '22 21:08 jacob-pro

@e-rhodes I guess a further benefit if we use serde Deserialize as you suggest we could match on the provided mime type of the part, and use that to deserialize appropriately? text/plain -> serde_plain application/json -> serde_json etc.

jacob-pro avatar Aug 22 '22 21:08 jacob-pro

I was mostly thinking about the additional form fields that can be posted with the files, as opposed to deserializing the content of the files. I don't have a ton of experience with the multipart spec, but it seems like other fields will just be multipart/form-data (per this answer).

For these fields, serde_plain may be sufficient---though it doesn't look like it would support deserializing multiple fields into a #[serde(flatten)]ed struct, or all the remaining keys into a HashMap, etc.

Looks like @JSH32's uses serde_json, but it supports the field[]=item syntax for arrays, which is awesome.

I also realized that my example above

    stream: Stream<Item = u8>,
    s3_upload: ImplementsS3Upload,

probably won't work because of the issue @asonix and @JSH32 alluded to above, that you can't deserialize the rest of the fields without consuming the stream first. Would it be possible to successfully deserialize successfully only if the stream is provided last in the form? Would probably be confusing behavior to work with, but a nice interface if it works.

The order is supposedly guaranteed but I'm not sure how that works with e.g. curl or other apis.

junbl avatar Aug 22 '22 21:08 junbl

@e-rhodes There are a few issues here:

First is the question of what syntax do you use for a list/array of parts. According to the spec the recommended way to do it is that the uploader puts each part in with the same name. However this does vary based on implementation...

The reason I am suggesting having a separate VecPart and OptionPart type is because we should be able to upload a list of files (or an optional file) with an arbitrary handler:

#[derive(Multipart)]
struct Upload {
    files: VecPart<S3Upload>
    text: String,
}

So given the following upload:

Content-Type: multipart/form-data; boundary=BOUNDARY

--BOUNDARY
content-disposition: form-data; name="text"

Hello world
--BOUNDARY
content-disposition: form-data; name="files"

....bytes1...
--BOUNDARY
content-disposition: form-data; name="files"

....bytes2...

(Note the filename and content-type headers for each part are optional - it is up to the server to choose how to interpret them)

The multipart comes in as a stream of fields that we have to read one at a time, the order is up to uploader / doesn't have to match our struct. We would read the first part and see that its name is text we would then need to do a lookup (the macro would provide implementation of this lookup), that matches text -> a handler, in this case the one from impl<T: Deserialize> Handler for T

Once each handler is completed we will need to store its output in some sort of state, likely a Map<(String, TypeId), Box<Type>> (similar concept to actix extensions.

Once we have processed all the parts in whatever order they arrived in, saved them into a map, we can then create the output struct Upload by retrieving each of its fields out of the map.

What we can't do is this:

#[derive(Multipart)]
struct Upload {
    files: Vec<S3Upload>
    text: String,
}

First of all S3Upload won't implement Deserialize, so Vec can't implement Deserialize. The compiler (at least until trait specialisation arrives) won't allow us to implement our handler trait for both T: Deserialize and also Vec at the same time, because of the possibility that Vec itself implements Deserialize (which it does!) causing a conflict .

We could compile this:

#[derive(Multipart)]
struct Upload {
    files: OptionalPart<S3Upload>
    text: Vec<String>,
    some_other_complex_structure: HashMap<String, String>,
    plain_text: String,
}

But it would only work where all the data for those fields is uploaded within a single part string. But to do that would require us to know which deserializer to use? One possibility is the user could provide it using the content-type field

Content-Type: multipart/form-data; boundary=BOUNDARY

--BOUNDARY
content-disposition: form-data; name="text"
content-type: application/json

["string 1", "string2"]
--BOUNDARY
content-disposition: form-data; name="some_other_complex_structure"
content-type: application/json

{"key": "value"}
--BOUNDARY
content-disposition: form-data; name="plain_text"
content-type: text/plain

abcdefg

jacob-pro avatar Aug 22 '22 22:08 jacob-pro

Maybe https://github.com/robjtede/actix-web-lab is the place for development?

jacob-pro avatar Aug 23 '22 14:08 jacob-pro

what syntax do you use for a list/array of parts

Seems like we should support both the multiple files with same field name as well as multiple fields with names like field[] (ideally for both the fields and the files).

The compiler (at least until trait specialisation arrives) won't allow us to implement our handler trait for both T: Deserialize and also Vec at the same time, because of the possibility that Vec itself implements Deserialize (which it does!) causing a conflict

Makes sense now. Still, having wrappers for Vec and Option is less that ideal ux. Could we achieve this with attribute macros?

struct Upload {
    all_files: Vec<File>,
    cloud_file: S3Upload,
    #[multipart(deserialize)]
    field: Vec<String>,
}

Alternatively, maybe the library could provide a struct:

struct Multipart<F: HandlePart, T: Deserialize> {
    files: F,
    fields: T,
}
#[derive(HandlePart)]
struct Files { ... }
#[derive(Deserialize)]
struct Fields { ... }
pub async fn handler(upload: multipart::Multipart<Files, Fields>) { ... }

Doesn't seem like there's an easy way to support only having files with this approach, though.

the user could provide it using the content-type field

This seems like it would be frustrating to require, since as far as I can tell there's no way to supply content-type on an HTML form. I know you can do it with curl, but at least for my use case, I need a frontend. I think supporting deserialization of JSON passed as a form field is a low-priority use case.

For me, the most valuable use cases would just involve deserializing from flat form fields to enums (either validating a dropdown or fields that can be multiple types) or (not as useful) collecting multiple fields into a struct/map. E.g.:

Content-Type: multipart/form-data; boundary=BOUNDARY

--BOUNDARY
content-disposition: form-data; name="field1"

dog
--BOUNDARY
content-disposition: form-data; name="multi_type"

1
--BOUNDARY
content-disposition: form-data; name="other_field"

cat
--BOUNDARY
content-disposition: form-data; name="dropdown"

bird
--BOUNDARY
content-disposition: form-data; name="file"

## bytes of file ##
struct Data {
     field1: String
}
#[serde(untagged)]
enum StrOrInt {
    Str(String),
    Int(i32),
}
#[serde(rename_all = "snake_case")]
enum Dropdown {
    Bird,
    Frog,
}

#[derive(Multipart)]
struct Upload {
    #[serde(flatten)]
    data: Data,
    multi_type: StrOrInt, 
    dropdown: Dropdown,
    #[serde(flatten)]
    rest_of_the_fields: HashMap<String, Value>, // grabs other_field
    file: File,
}


junbl avatar Aug 23 '22 20:08 junbl

@e-rhodes

Seems like we should support both the multiple files with same field name as well as multiple fields with names like field[]

Do you mean where multiple parts are uploaded each using the name field[] ? Would the following work?

struct Upload {
    #[multipart(rename="files[]")]
    files: VecPart<File>,
    #[multipart(rename="text[]")]
    texts: VecPart<String>,
}

In the example you've given you wouldn't be able to use those #[serde(flatten)] annotations since as mentioned before the Upload struct itself does not and cannot implement Deserialize itself.

This also doesn't work because of the limitations of serde_plain:

    #[derive(Deserialize, Debug)]
    #[serde(untagged)]
    enum StrOrInt {
        Int(i32),
        Str(String),
    }

    #[test]
    fn test_deserialize() {
        let s1: StrOrInt = serde_plain::from_str("1234").unwrap();
        // Str("1234") - doesn't match to int because plain text doesn't make a distinction unlike JSON
        println!("{:?}", s1);
    }

But the other enum would work, and this is one advantage of implementing for T: Deserialize instead of just T: FromStr:

    #[derive(Deserialize, Debug)]
    #[serde(rename_all = "snake_case")]
    enum Dropdown {
        Bird,
        Frog,
    }

    #[test]
    fn test_deserialize2() {
        let d1: Dropdown = serde_plain::from_str("frog").unwrap();
        // Dropdown::Frog
        println!("{:?}", d1);
    }

I think the fundamental issue here is that you are trying to categorise things into files and non files...

From the perspective of the server we just get a stream of parts, each one has:

  • A field name
  • An optional filename (this doesn't necessarily indicate if it is a file)
  • An optional mime type (but if it is missing, we can default to text/plain)
  • A stream of bytes

The file name isn't mandatory for cases where the file name isn't available or is meaningless or private; this might result, for example, when selection or drag-and- drop is used or when the form data content is streamed directly from a device.

What the proposal above was going to do is map a handler based purely on the field name. If the field name is associated with the S3 uploader type, the data would get uploaded to S3 regardless of whether it is a 10 byte text-plain or a 1 GiB file application/octet-stream

I guess a scenario to imagine is that you have a user uploading a 1GiB .txt file, it would have a mime type of text/plain and the filename may be empty - there is no reliable way for the server to decide if that is "file" or "non-file"...

This seems like it would be frustrating to require, since there's no way to supply content-type on an HTML form.

Yes you are right, and it goes back to my question - that for a given part how do you know what deserializer to use?

My suggestion to use the content-type would work for HTML forms, since the forms either supply a mime type of text/plain or default to text/plain and therefore their fields would get deserialized with serde_plain.

If you wanted to do JSON, we would either need an API client to tell the server via the content type that it is uploading JSON within that part, or put in some sort of annotation on the server side so we know to deserialize that part with JSON instead.

rest_of_the_fields: HashMap<String, T>, // grabs other_field

This is a feature I hadn't really thought about. It would be workable with an anotation similar to this. But it would come with the restriction that every part would have to use the same handler T, e.g. you have a to choose whether you want to attempt to read every unknown field into memory, write to disk or upload to S3, etc.

jacob-pro avatar Aug 23 '22 23:08 jacob-pro

(Sorry for the hiaitus.)

I'm glad someone is giving this problem some thought; it definitely needs it.

would just like to know where to get started from an organizational standpoint

actix-multipart is pre-v1 so I don't mind iterating on the version as quickly as I would in actix-web-lab, hence the issue transfer to this repo. Also if the actix-multipart-derive crate name is needed for this effort that's fine by me.


I've been thinking for a while that might be an easier first goal is some sort of non-macro API similar to awmp or a builder API... idk... might play nicer with the flexibility we're wanting here.

robjtede avatar Aug 25 '22 02:08 robjtede

I wasn't familiar with awmp, looks like it has an API similar to what I was imagining here--

struct Upload {
    all_files: Vec<File>,
    cloud_file: S3Upload,
    #[multipart(deserialize)]
    field: Vec<String>,
}

Alternatively, maybe the library could provide a struct:

struct Multipart<F: HandlePart, T: Deserialize> {
    files: F,
    fields: T,
}

Either of these would lend themselves to using an internal struct similar to awmp::Parts that could eventually be extended into a macro API for custom structs.

@robjtede when you say a builder api, are you imagining something like this?

Multipart::new()
    .file("file_field_name")
    .text("text_field_name")

Which would basically another interface to awmp::PartsConfig::with_{file|text}_fields. This could also lend itself to extending with generics like:

Multipart::new()
    .file::<S3Upload>("file_field_name") // T: Multipart or whatever we call the trait
    .text::<CustomEnum>("text_field_name") // T: Deserialize

Possible extensions to cover the use cases we've discussed:

Multipart::new()
    .files::<File>("name") // Vec<File>
    .into_struct::<FlattenedStruct>() // uses field names in struct? may be possible with serde but probably macros

The biggest issue with this approach is that it requires registering the configuration at the app level, instead of being able to specify the structure just with the struct given in the route (like other extractors e.g. web::Query<DeserializableStruct>). We'd need a way to specify multiple configurations per route, too, which from a glance I can't see how to do in awmp.

@jacob-pro

Do you mean where multiple parts are uploaded each using the name field[] ? Would the following work?

Seems like it would--I'm of the opinion that it should be supported by default instead of having to decorate each field, but that's an unnecessary extension as long as there's some way to do it.

The file name isn't mandatory

Dang lol that was the piece I was missing; I thought that's how it would distinguish them. Regardless, either of the approaches above (two different structs or specified with attribute macros) would solve this issue, since the derive macro can statically determine how to map fields to files/deserializers.

It would be workable with an annotation similar to this

I wasn't imagining supporting this for the file parts, but that's an interesting thought. If we strictly separate them, then we can piggyback off of serde(flatten) for the text fields and at any point add the feature for e.g. multipart(flatten).

junbl avatar Aug 25 '22 22:08 junbl

Here's my proposal:

  1. An internal API similar to awmp.
    • This will support splitting your fields into text fields and file fields, which store a list of pairs (String, String) or (String,TempFile), respectively. (or &str or NamedTempFile or whatever else.)
      • Is this worth storing instead as a HashMap<String, Vec<String|TempFile>>? Probably less overhead to just keep as the Vec, and the less ergonomic interface isn't an issue with 2a.
    • The structure of these (where fields go) should be able to be specified per route. (This might not be feasible without 2a, or not worth reimplementing)
  2. Conversion from the internal representation to user defined structs.
    • A struct Multipart<F, T>, where T: Deserialize and F: MultipartUpload. Multipart will implement FromRequest, which will allow for specifying the structure just in the handler args, like in web::{Query, Json, Form}.
      • The internal representation Map<String,String> can be deserialized into T using e.g. serde_json.
      • Ideally, an interface like Multipart<T, ()> if you don't have any text fields.
    • Implementations of MultipartUpload for common use cases, e.g. TempFile, File, Vec<u8>.

junbl avatar Aug 25 '22 22:08 junbl

@e-rhodes what you have described is very similar to what my existing crate already does.

It has an internal loader API:

let grouped: MultiMap<String, Field> = Loader::builder()
        .file_limit(500 * 1024 * 1024)
        .build()
        .load_grouped(payload)
        .await
        .unwrap();

// Where:
pub enum Field {
    File(File),
    Text(Text),
}

It determines whether a part is "text" or "file" based on this heuristic:

let item = if content_type == mime::TEXT_PLAIN && cd.get_filename().is_none() {
   // Treat as text
} else {
   // Write to tempfile
};

However like I have said many times before - this is a mistake - for multiple reasons:

  • It does not work if someone uploads a very large text file (which is a perfectly reasonable use case)
  • Error messages are confusing - what if someone uploads "text" (with the right field name), when the server is expecting "file"
  • What if your are only expecting small files and want them read directly into memory?
  • What if you want to write data somewhere other than Tempfiles?

The discussion we were having at the start of the thread, is that this isn't good enough as a general purpose solution, because we want to be able to support streaming to arbitrary backends (file, S3, memory) based on the field name.

It would thus allow us to support much broader use-cases, which your suggestion of Vec<String|TempFile> does not allow.

jacob-pro avatar Aug 25 '22 23:08 jacob-pro

@jacob-pro

TempFile in my example was a misnomer---I meant any kind of internal representation representing the stream of bytes, that can eventually be used as any type that implements MultipartUpload. Maybe the easiest way to do this is to just make it generic at this stage, and not try to make some kind of unified stream representation.

Maybe this example will show what I mean more clearly:

#[derive(Deserialize)]
struct TextFields {
    field1: String,
}

#[derive(MultipartUpload)]
struct FileFields {
    field2: Vec<u8>, // in memory, implemented by library
    field3: S3Upload, // custom implementation of `MultipartUpload`
}

async fn handler(upload: Multipart<TextFields, FileFields>) -> impl Responder { ... }

Multipart's implementation of FromRequest decides where to put each field based on the names in the struct. We never interact with the MIME type or the filename, which, as you say, can't be relied upon. The type of the field, combined with which struct it appears in, determines how the bytes from the upload are treated.

junbl avatar Aug 25 '22 23:08 junbl

@e-rhodes the distinction between file and non-file only makes sense in the context of the client / or a web browser.

From the server side it is completely up to us to decide what we want to do with the parts - it is irrelevant whether they came from an actual file on disk or the clients memory - the server can't reliably tell - and even if it could it wouldn't matter:

For example I would often like to submit a multipart like this:

Content-Type: multipart/form-data; boundary=BOUNDARY

--BOUNDARY
content-disposition: form-data; name="manifest"
content-type: application/json

{"title": "Photo title", "category": "holidays"}
--BOUNDARY
content-disposition: form-data; name="image"
content-type: image/jpeg

JPEG_BYTES...

Note that neither of these parts have a filename. The mime types are correct, neither of them are text/plain.

My proposal above would work with this:

#[derive(Deserialize)]
struct Manifest {
   title: String,
   category: String
}

#[derive(MultipartForm)]
struct Form {
    manifest: Manifest, // Because the mime-type is provided we know we can use serde_json instead of serde_plain
    image: S3Upload
}

Or via a builder API:

MultipartForm::new()
    .part::<S3Upload>("image") // implements MultipartHandler or whatever we call the trait
    .part::<Manifest>("manifest") // implements Deserialize

Note there is no distinction between "file" and "text"

jacob-pro avatar Aug 25 '22 23:08 jacob-pro