MultipartData is not composable due to lifetime restrictions

[SergioBenitez]

Hello!

I'm considering using your library in Rocket, but the lifetime restrictions imposed by MultipartData make any kind of meaningful composition or abstraction impossible. For instance, it is not possible to call a function that creates a Multipart structure, reads a MultipartData entry from a stream, and returns a MultipartFile. This same restriction means that MultipartData cannot be abstracted away in any meaningful manner.

The issue is caused by the lifetime in MultipartData being derived from the read_entry method in Multipart, making it impossible for a MultipartData object to outlive its Multipart parent object. There are two references in MultipartData:

• A reference to a str in the Text variant.
• A reference to a BoundaryReader<B> in MultipartFile in the File variant.

The first can be easily removed by using a String instead of an &str. The &str seems wholly unnecessary, as does line_buf in Multipart.

The reference to the reader is more interesting. There are many ways to avoid this reference, but one particularly viable solution is to have an alternate API that moves the stream into the MultipartField. You can accomplish this by having a MultipartField::next() method that consumes the current object and moves the internal stream to an optionally newly created one. Using this API to iterate through all fields would look something like:

let mut multipart = Multipart::with_body(data, boundary);

let mut field = match multipart.field();
loop {
    /* use `field` here */

    match field.next() {
        Some(field) => field = field,
        None => break
    }
}

This is easy to use and is significantly more flexible. This allows for an extract_reader method to extract the underlying reader, which can be necessary.

Please consider this API or other solutions that allow your library to be used in more contexts.

[abonander]

Do you mind showing me what you're trying to do? The borrowing has generally not been an issue before, and I'm hesitant to make such a sweeping change for one framework. There is Multipart::save_all() which provides owned copies of all fields (saving streams to temporary files). However, I'm wondering if there's not otherwise a way to adapt what you're trying to do to the existing API.

[SergioBenitez]

I mentioned my use case in the original post:

call a function that creates a Multipart structure, reads a MultipartData entry from a stream, and returns a MultipartFile.

This isn't about changing your library to work with one framework, this is about ensuring your library can meet any applicable use case. At present, you're imposing a rather strong restriction; your library is literally impossible to use under certain scenarios, and the restriction is unnecessary. As far as I can tell, your library is the most complete one of its kind, and the only one that doesn't parse eagerly. It would be a shame if there needed to exist yet another library to meet a use case this one can easily meet.

[abonander]

Does the save_all() method work or do you find going to the filesystem undesirable? It doesn't use the filenames supplied in the request.

[SergioBenitez]

I don't want to save to the file system unnecessarily. I want the caller of the function to determine what to do with the file stream. For instance, it should be trivial to gzip the stream without copying the contents to the file system unnecessarily. I want a stream end-to-end.

[abonander]

And allowing the user to consume the Multipart directly isn't acceptable?

[SergioBenitez]

That's right. That's the "abstraction" part.

[SergioBenitez]

Following up on this. Have you given this any more thought, @abonander?

[abonander]

I have some ideas on how to make this work without breaking existing usage. I'll get back to you.

[abonander]

@SergioBenitez

• https://docs.rs/multipart/0.10.0-alpha.1/multipart/server/struct.Multipart.html#method.into_entry

• https://docs.rs/multipart/0.10.0-alpha.1/multipart/server/struct.MultipartField.html#method.next_entry

• https://docs.rs/multipart/0.10.0-alpha.1/multipart/server/struct.MultipartField.html#method.next_entry_inplace

[abonander]

I ended up making the multipart: M fields private because I also wanted to implement nested multipart fields (e.g. a file field with more than one file selected) and the way I wanted that to work necessitated more control over the inner Multipart. The changes are on 0.10 branch, I have a couple more changes to make before I publish another alpha.

[SergioBenitez]

Awesome! This looks like it'll do what I need.

Am completely inundated with work for the next week or so but will play with this afterwards.

[SergioBenitez]

Finally had a chance to give this a try. While the API seems like it will work (though I do find it a bit clumsy to work with, though maybe that's unavoidable), I've hit a bug. Here's what I'm doing, in pseudocode:

let multipart = Multipart::with_body(body, boundary);
let mut field = multipart.into_entry();
let outcome = if let Some(file) = field.data.as_file() {
    let mut temp_file = tempfile();
    file.save_to_limited(&mut temp_file, 1 << 20).expect("ok.");
    Success(temp_file)
} else {
    Failure
};

// There should only be one field.
match field.next_entry() {
    Ok(None) => { outcome }
    Ok(Some(_)) => { Failure }
    Err(e) => { Failure }
}

The issue is that the program hangs on the call to next_entry(). My guess is that there's something wrong with whatever is reading from the body. This happens every time, so this should be easy to add a test for (re: #59).

Another bug is that calling save_to_limited with a limit that is in-fact enforced (IE, the file is larger than the limit) results in parsing the next entry incorrectly. This is contrary to what the documentation says: "If the previously returned entry had contents of type MultipartField::File, calling this again will discard any unread contents of that entry."

[SergioBenitez]

@abonander Have you had a chance to mull over my last comment? This is a showstopper.

[abonander]

I'm working on it.

[SergioBenitez]

Awesome! Thanks. :)

[abonander]

I've published 0.10.0-alpha.2 which addresses the hang and I think the problem with save_to_limited() as well. If the latter is still an issue, can I see the part of the file or stream that's corrupted, and also logs for multipart=trace. Otherwise I'm just left guessing. I'm working out how to add file saving to the integration test in a way that's reproducible.

In the process of fixing the hang I also added the entry API to local_test.rs in combination with both the lazy and the regular client APIs.