(feat) Add body parsing for POST requests.

[michaelsproul]

I've added a way to create middleware that parses either the url or body of a request for urlencoded data.

use urlencoded::UrlEncodedParser;

// For GET requests
server.chain.link(UrlEncodedParser::for_url());

// For POST requests
server.chain.link(UrlEncodedParser::for_body());

I've also removed use of soon to be deprecated liburl in favour of servo/rust-url, as per rust-lang/rust#15874.

My approach intentionally doesn't permit parsing of requests with data in both the URL and the body, as this is a violation of HTTP convention (as far as I can ascertain, I'm willing to be wrong).

Close #21.

[reem]

This is really good work! Thank you very much. I'm going to leave a few comments in the source also, but here's some overall feedback.

Ideally I could enable both behaviors (i.e. you check the url and then check the body or vise-versa) with a single UrlEncodedParser::new(). If I wanted only one behavior, I should do UrlEncodedParser::only_url() or UrlEncodedParser::only_body(), rather than having to opt-in. We want to ensure that we are optimizing for the common use case.

[reem]

I worry that this is pretty fragile for parsing. Using a more heavy-duty parser, if available, from the servo url crate would be better.

If you have an argument for why this won't break unexpectedly, then I'm fine with it.

[michaelsproul]

I agree that it's a bit dodgy, yeah. Servo's URL library has a function for parsing a URL and extracting the 'query' field, but to use it we would have to transform Request::url into a full URL. We could do this by adding a .url() function to Request which is guaranteed to return a full, valid URL. Alternatively, we could construct a complete URL by adding a bogus domain, although I think this is just as dodgy as the current approach. I think the .url() option is vastly better, and could be useful elsewhere (plus it insulates us from rust-http).

[reem]

@michaelsproul Thanks for your contribution. I left you a few comments, if you don't mind taking a look. If you don't want to/don't have time to make the changes, just let me know and we can take care of it.

[theptrk]

This is a really great addition to urlencoded that I overlooked, thanks

[michaelsproul]

@reem: Regarding the overall structure, I'm very open to change. This is the other structure I was considering:

Allow parsing of the URL and body independently by inserting values with different types into the Alloy. Something like UrlEncodedUrl (awkward) and UrlEncodedBody. The UrlEncodedParser could look something like this:

struct UrlEncodedParser {
    parse_url: bool,
    parse_body: bool
}

[reem]

@michaelsproul I sort of like the way you only parse either the url or body but it may be too opinionated. How about storing:

struct UrlEncodedParsed {
    querystring: Option<HashMap<String, String>,
    body: Option<HashMap<String, String>
}

or something similar.

The recent changes to Request and Response allow me to add new fields to them, and I'm thinking of moving Alloy to instead be a field of Request, meaning that you could instead write a mixin Trait and just add methods to Request, which better hides all of this complexity from the user.

[michaelsproul]

Yeah, looking into it a bit further I think we should allow users to parse parameters from both the URL and the body.

Your idea about mixin traits for Requests is interesting. Adding methods like get_query_params() and get_data_params() would make this sort of thing really simple. My only concern is that this approach would detract from the focus on Middleware, which isn't necessarily bad, just different. One thing to keep in mind is the somewhat hidden nature of traits in Rust, people would only have to use urlencoded::UrlEncodedParser; to have methods added to their requests (which could be a bit strange). Although the more I think about it, the more I like it...

[reem]

It's a tiny bit of conceptual overhead (methods come in scope with their traits) for a huge payoff in usability - no more mucking about in with match alloy.find::<SomeSuperLongType>() { ... }.

If you could make the change we described in the last few posts (allow both with one declaration, store a type containing two Options) I'll merge this in.

If you'd like to make later changes, I'll @mention you in an issue about it when I finish cleaning up the Request API.

[michaelsproul]

Ok, all done. I'm pretty happy with it now.

Let me know if there's anything I can do on the main bit of Iron. I'd love to get involved.

My timezone is UTC + 10h, but I'll be on IRC tomorrow morning (afternoon for you guys).

[michaelsproul]

Typo fixed.

[reem]

This looks good. Thank you again for contributing.

If you're up for it, providing a nicer interface for Request::url (i.e. returning a Url struct that is a parsed version of the url with host, uri, port, querystring, etc.) would be awesome. Just go ahead and start hacking on it on a PR in iron/iron if you want to.

[reem]

@michaelsproul It looks like extern crate url = "url_" is failing on travis. Changing it to extern crate url; makes it compile but gives a warning about multiple versions of url being used.

[michaelsproul]

Yeah, rust-url changed its package name 7 hours ago. According to the commit, it's ok to use the new liburl as url, so long as the old liburl isn't required in the same crate. The clash occurs with the main Iron package, which I suppose we'll be updating over the next couple of days.

See: servo/rust-url@092c4e6

[cburgdorf]

@reem that's essentially what we do in nickel.rs as well (putting the anymap on the request) and I think it pays of quite well.