Adding multipart/form-data support for Reqwest #143
Conversation
src/client.rs
Outdated
| timeout: None, | ||
| tls: tls_connector_builder, | ||
| }), | ||
| }) |
There was a problem hiding this comment.
A previous rustfmt patch just moves that into the 'block' style. Can you revert this?
src/client.rs
Outdated
| redirect_policy: config.redirect_policy, | ||
| referer: config.referer, | ||
| }), | ||
| }) |
src/client.rs
Outdated
| headers.set(AcceptEncoding(vec![qitem(Encoding::Gzip)])); | ||
| } | ||
|
|
||
| println!("{:?}", body); |
There was a problem hiding this comment.
Stray print probably for debugging snuck in.
src/client.rs
Outdated
| redirect::Action::TooManyRedirects => { | ||
| return Err(::error::too_many_redirects(res.url.clone())); | ||
| } | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
Can we revert all the formatting changes in this method?
src/request.rs
Outdated
| })) | ||
| username: username.into(), | ||
| password: password.map(|p| p.into()), | ||
| })) |
There was a problem hiding this comment.
Can this format be reverted please?
| path.file_name().unwrap().to_str().unwrap()); | ||
| write_bytes!(body, "\r\nContent-type: {}\r\n\r\n", mime); | ||
| let mut content = try_!(fs::File::open(path)); | ||
| content.read_to_end(&mut body).unwrap(); |
There was a problem hiding this comment.
The files could be very big, so instead of copying all of them into memory, it'd probably be better to create an internal MultipartBody that implements Read, and pushes the multipart bytes onto the read buf, and then forwards to File::read.
There was a problem hiding this comment.
Could you give me an example on how to do that? I haven't worked with files in Rust that much actually.
There was a problem hiding this comment.
struct MultipartBody {
cursor: SomethingToRecordWhatParamOrFile,
params: Params,
files: Vec<File>,
}
impl Read for MultipartBody {
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
let mut n = 0;
if self.params_bytes_remaining() {
// take &mut [u8], use (&mut [u8]).write(other_bytes))
n += copy_param_bytes(buf);
}
while !buf.is_empty() && !self.files.is_empty() {
// basically, self.files[self.file_pos].read(buf)
// and then when files is EOF, self.file_pos += 1
n += self.copy_file(buf)?
}
Ok(n)
}
}The point here is that instead of pushing into a Vec that might grow to huge sizes, bytes are only pushed into the read slice. After a read from the body, that slice is pushed onto the socket, and only afterwards is another read done, so the amount of memory used stays low.
| write_bytes!(body, "Content-Disposition: form-data; name=\"{}\"", name); | ||
| write_bytes!(body, | ||
| "; filename=\"{}\"", | ||
| path.file_name().unwrap().to_str().unwrap()); |
There was a problem hiding this comment.
It'd be better to remove these unwraps, so it doesn't panic. If the name is not valid UTF-8, the RFC suggests to use percent-encoding of the bytes.
src/request.rs
Outdated
| if let Some(p) = self.params.clone() { | ||
| for (name, value) in p { | ||
| write_bytes!(body, "\r\n--{}\r\n", boundary); | ||
| write_bytes!(body, "Content-Disposition: form-data; name=\"{}\"\n", name); |
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| pub fn header<H>(&mut self, header: H) -> &mut MultipartRequestBuilder |
There was a problem hiding this comment.
I wonder if perhaps it's better to keep this builder focused, so as to not duplicate methods. Add headers on the regular builder, and then call multipart?
There was a problem hiding this comment.
Well, you'd still be able to call the json, form and others like the post method if you want to keep things in the regular builder. I could potentially fix that by emptying the body in the MultipartRequestBuilder.build method and then writing everything to it, but it just seems less elegant to me for the user to allow methods to be called while building a multipart/form-data request while these do not belong in the construction of one.
| let multipart_mime = ContentType(format!{"multipart/form-data; boundary={}", boundary} | ||
| .parse::<mime::Mime>() | ||
| .unwrap()); | ||
| if let Some(p) = self.params.clone() { |
There was a problem hiding this comment.
It'd be best to not clone the params and files fields in here, and instead use some sort of take_multipart method, like the RequestBuilder does.
|
I would love to have this functionality in reqwest. Are you still working on the requested changes @voider1 ? If not I might be able to help. |
|
@e00E Hey, I'm still working on it, but currently I don't have much time. Lots of things going on, I'll be working on it again in a week or 3. If you want to help out you're more than welcome to! |
This PR adds multipart/form-data support for Reqwest, I have a
MutipartRequestBuilderwhich has 2 extra chainable methods;files()andparams(), which obviously add files and parameters to the multipart/form-data request. This PR also closes issue #4.