search

Ogeon / rustful

[OUTDATED] A light HTTP framework for Rust
https://docs.rs/rustful
Apache License 2.0
862 stars 51 forks source link

Implement support for multipart requests #50

Closed Ogeon closed 9 years ago

Ogeon commented 9 years ago

This adds support for requests with the type multipart/form-data to the BodyReader. The solution has also been discussed in #49 and (mainly) consists of adding an as_multipart method to BodyReader and a multipart cargo feature that toggles it.

The changes here breaks the from_reader method on BodyReader, which is really an internal method. I would like to consider it highly unstable for now, so this is still a minor change.

Closes #49

abonander commented 9 years ago

This looks pretty good, the only recommendation I would give in general is maybe consider putting the multipart stuff behind a feature so people can opt-out if they want. It can be on by default, I just think people would appreciate having the option.

Ogeon commented 9 years ago

Good point. Its not really intrusive, but it does come with a small runtime overhead. Being able to turn it off sounds good. Thank you for your advice :smile:

I'll look into this and why it breaks on Windows (possibly a Rust version issue) as soon as possible.

abonander commented 9 years ago

Hyper added SSL support and it's a bit broken cause it needs libopenssl. I have it installed but it still doesn't work. Check my Cargo.toml to see how I made it work without disabling SSL entirely.

Ogeon commented 9 years ago

I have set it up to download and install OpenSSL, so that's no problem (except for the occasional version change). This break is caused by winapi not finding std::os::raw. I have been a bit conservative when it comes to what Rust version I test with, and decided to stay at 1.0.0 until I'm forced to upgrade.

abonander commented 9 years ago

Oh, wow. I use a nightly and upgrade whenever I think it's prudent. It's okay to use nightlies because as long as you don't have any #![feature(...)] annotations in your crate, it is basically guaranteed to work on the latest stable release. And you can always configure Travis CI to check against both the nightly and release branches (and beta too if you care that much).

Ogeon commented 9 years ago

I'm usually following stable, but I thought that I should make this build as far back as possible without sacrificing functionality and so far so good, until today. It wasn't really a promise, but I didn't want to break it unless I had to. Seems like it's finally time to bump the lowest supported version for Windows.

Ogeon commented 9 years ago

I think I managed to include all the suggested changes. The only thing left, then, is a proper description of as_multipart with an example (unless something else comes up).

Ogeon commented 9 years ago

I think this is it, unless I have been too distracted and missed something... What do you think @cybergeek94?

abonander commented 9 years ago

I have a PR on multipart waiting for Travis CI checks to pass: https://github.com/cybergeek94/multipart/pull/19.

It doesn't break the server API but it introduces some much-needed fixes. I recommend testing against it once I merge it.

Additionally, I might just go ahead and refactor out HttpRequest::is_multipart(), and rename boundary() to multipart_boundary() to make the semantics clearer. Since this would be a breaking change, it'd necessitate bumping the version to 0.3. I'd also take this opportunity to fix a nit I've had with the client API for a while now.

Ogeon commented 9 years ago

Sounds good. Just notify me when 0.3 is ready and I'll make the change.

abonander commented 9 years ago

0.3.0 is uploaded to crates. PR: https://github.com/cybergeek94/multipart/pull/20

Ogeon commented 9 years ago

Oh, that was quick :smile: I'll update it in a moment...

Ogeon commented 9 years ago

...and there it is!

Ogeon commented 9 years ago

Ok, that's it. This is done. Thank you, @cybergeek94, for helping me reviewing it! It has been really helpful :smile:

@homu r+

homu commented 9 years ago

:pushpin: Commit e8acf52 has been approved by Ogeon

homu commented 9 years ago

:hourglass: Testing commit e8acf52 with merge b014b4f...

homu commented 9 years ago

:sunny: Test successful - status

abonander commented 9 years ago

:+1: