Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add multipart support to RequestBuilder #4

Closed
seanmonstar opened this issue Oct 16, 2016 · 20 comments
Closed

Add multipart support to RequestBuilder #4

seanmonstar opened this issue Oct 16, 2016 · 20 comments
Labels
B-rfc Blocked: Request for comments. More discussion would help move this along.

Comments

@seanmonstar
Copy link
Owner

This can probably use the multipart crate, unsure the most ergonomic API to actually expose.

@seanmonstar seanmonstar changed the title Add multipart method to RequestBuilder Add multipart support to RequestBuilder Oct 29, 2016
@seanmonstar
Copy link
Owner Author

My current thinking is to add a form<T: Serialize>(val: T) method, which will form urlencode. To allow multipart, I'm thinking that a file()method could also exist, which would set the same internally that the form and files are multipart.

@seanmonstar
Copy link
Owner Author

A form<T: Serialize>(val: T) method now exists. To add multipart support, a possibility is as described before, adding a file(name: &str, f: File) method, and if form() and file() are both called, then internally the RequestBuilder makes the request multipart.

@KeenS
Copy link

KeenS commented Feb 24, 2017

What the status of this issue? I'd like to use multipart with reqwest.

@seanmonstar
Copy link
Owner Author

The status is all here in the issue. The question is on how should the API look for adding it.

@KeenS
Copy link

KeenS commented Feb 25, 2017

I see. So, could I suggest a new API? I'd like to have a explicit multipart() call and then accept form<T: Serialize>(val: T) and file<P: AsRef<Path>>(name: &str, p: P) . multipart() may return a new builder, say MultpartRequestBuilder or phantom typed RequestBuilder<Multipart>.
I think your option works well but I prefer explicit control over the format like json() and form.

@abonander
Copy link

@seanmonstar Any blockers from multipart on this? We're blocked on upgrading Hyper because of Nickel but I'm thinking of just dropping support or moving it to an external crate because they move so slowly.

@seanmonstar
Copy link
Owner Author

@abonander if there is a blocker, I'm not aware of it. I admit I haven't had much time to look into the this issue yet.


As for the suggested API comment, having multipart() -> MultipartRequestBuilder sounds like an excellent idea.

@dbrgn
Copy link
Contributor

dbrgn commented Apr 20, 2017

There are already a few multipart related crates around: https://crates.io/search?q=multipart Maybe that could help, or be an inspiration.

@voider1
Copy link

voider1 commented May 8, 2017

How can I send multipart requests with this library now? I can't get it to work, sadly.

@seanmonstar
Copy link
Owner Author

@voider1 this issue is still open, no one has been motivated enough to implement it.

@dbrgn
Copy link
Contributor

dbrgn commented May 8, 2017

How can I send multipart requests with this library now? I can't get it to work, sadly.

What you could do as a workaround is to use one of the crates here https://crates.io/search?q=multipart and simply encode the request body yourself. Once you have the encoded bytes, you can send them using reqwest.

@voider1
Copy link

voider1 commented May 11, 2017

@seanmonstar I'll give it a try, if you have any suggestions I'd like to hear them. I will be taking the suggested approach from this issue to implement it.

@voider1
Copy link

voider1 commented May 28, 2017

I think I basically finished the multipart method (didn't have a lot of time on my hands), but how should I go about implementing the send method for both RequestBuilder and MultipartRequestBuilder?

For now I have made a macro called impl_send which implements the send method for the struct which gets passed to it.

@seanmonstar
Copy link
Owner Author

@voider1 it'd be better if there was only 1 function that was ultimately called. There's some other work to introduce a Request type, and probably some sort of client.execute(Request), so there's several different entry points to actually sending a request over the wire, and they should all converge on a single function. It may be clearer with the proposed RequestBuilder::send() internally just doing client.execute(builder.build()).

@abonander
Copy link

abonander commented May 31, 2017

If you're using multipart::client::lazy::Multipart, there is a bug with the internal buffering that I need to sort out before this gets merged (abonander/multipart#82). If you're using a different crate entirely then obviously it's a non-issue.

@voider1
Copy link

voider1 commented Jun 1, 2017

@seanmonstar Should I just implement the MultipartRequestBuilder as I normally would do? I could adopt the Request type if you could expand on how it should be implemented. As of now, the MultipartRequestBuilder is a wrapper around RequestBuilder, I can make the RequestBuilder public so people could call multipartrequestbuilder.requestbuilder.send() or I could make a method on the MultipartRequestBuilder called send() which would call self.requestbuilder.send(). That's what I'm thinking about now, but I could adopt the Request type and the other mentioned methods and changes in my fork.

@abonander Thanks for mentioning it, but I am not using a crate for this. Implementing multipart/form-data requests myself seems easier and is trivial. I didn't take much time to look at multipart crates and when I did I couldn't get it working with Reqwest.

@seanmonstar
Copy link
Owner Author

@voider1 I've merged the Request and RequestBuilder changes into master, so maybe it's easier to understand what I was referring to. I suspect having send() and build() methods on a multipart builder makes sense.

I wonder though, if having made all the builder methods return &mut Self will have made the chaining into a multipart builder impossible...

@seanmonstar
Copy link
Owner Author

Thought some more, I retract what I said, even though the builder uses &mut Self, it should still be perfectly fine to chain it together. You can see this is already possible since you can chain using the ClientBuilder and RequestBuilder together in a single statement.

@voider1
Copy link

voider1 commented Jun 9, 2017

@seanmonstar Yes, I've implemented them in such a way that params() and files can be chained. Thanks for your input! I will make a PR soon.

@simmons
Copy link

simmons commented Jun 28, 2017

On the off chance it's useful for anyone, I threw together a quick-and-dirty example of manually using the multipart crate with reqwest 0.6.2, based on dbrgn's suggestion:
https://gist.github.com/simmons/6c2e6eb3ade5bfcb962250603dd667cf

@seanmonstar seanmonstar added the B-rfc Blocked: Request for comments. More discussion would help move this along. label Aug 19, 2017
repi pushed a commit to EmbarkStudios/reqwest that referenced this issue Dec 20, 2019
make event fields work a little better
repi pushed a commit to EmbarkStudios/reqwest that referenced this issue Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-rfc Blocked: Request for comments. More discussion would help move this along.
Projects
None yet
Development

No branches or pull requests

6 participants