Add a `Request` builder #55

alexcrichton · 2017-07-10T21:43:15Z

This commit adds a new builder for request::Head which will transitively allow
creation of a Request through a builder-like interface.

cc #49

Closes #58
Closes #56
Closes #52
Closes #51
Closes #49
Closes #43

alexcrichton · 2017-07-10T21:50:31Z

I haven't done Response yet but I figured we could mirror this once settled. Some thoughts:

I opted to have a builder for Head rather than Request directly as it helped avoid the generic. That meant the struct itself was named HeadBuilder even though you're likely building a Request. The benefit though is that it gives us a Head builder for free.
Any thoughts about how to create the builder itself? Right now you can choose between Head::builder(), HeadBuilder::new(), or HeadBuilder::default(). It may be prudent to do Request::builder() as well though? Or maybe drop the ::builder methods?
I've opted for &mut builders which guarantee a panic for now if you reuse it after building, we could perhaps make this more flexible though if desired. I figured this was the most conservative route.
I was a little sad that foo.header("foo", "bar") didn't work, I had to import HeaderValue and work with that. Were there any plans to have something like Into<HeaderValue> work in more places?

seanmonstar · 2017-07-13T16:56:05Z

Any thoughts about how to create the builder itself?

I'm a fan of not having to import more types. In my own programs, I'd want to reach for Request::builder().

let req = Request::builder()
    .method(Method::POST)
    .uri(httpbin_org_post)
    .header(CONTENT_TYPE, JSON)
    .build(body);

It does read slightly off to me that the body is supplied to build. The builders in hyper include the generic body, and it typically just works thanks to type inference.

I was a little sad that foo.header("foo", "bar") didn't work, I had to import HeaderValue and work with that. Were there any plans to have something like Into work in more places?

Into<HeaderValue> is fine, but it cannot be implemented for str. You can have invalid characters in a string. hyper has to protect against this indiscriminately by searching for and replacing all \r and \n before writing to the socket. There's other illegal characters, but those are particularly damaging.

alexcrichton · 2017-07-14T15:18:59Z

Yeah it's true that avoiding too many imported types is good, although maybe we could just add Request::builder anyway returning a HeadBuilder? It seems nice to have a builder for Head for free and it seems like the ergonomics wouldn't be all that different?

Yeah I understand that headers can have invalid chars, I just find it sort of unfortnate. You mention that you don't like to import types, but the current construction means that we'll require an import of HeaderValue at least to set a header. I think we may want to explore some degree of fallibility either on the header method directly or maybe in the build method.

We could potentially vendor a custom version of the TryInto trait, add some standard impls (notably including &str), and then have this return a Result which can just be used with ? to propagate it.

seanmonstar · 2017-07-14T21:34:43Z

we could just add Request::builder anyway returning a HeadBuilder? It seems nice to have a builder for Head for free and it seems like the ergonomics wouldn't be all that different?

Oh certainly, I didn't mean to remove the other constructors, just that I'd want Request::builder to exist.

I think we may want to explore some degree of fallibility either on the header method directly or maybe in the build method.

That makes sense! A Builder should always be about convenience. There is headers_mut().insert that is infallible on the Request/Response/Head already.

In the reqwest review, there was wide agreement that a method in a builder that can fail should return the error immediately, not hold on to them and return it in build.

alexcrichton · 2017-07-17T16:06:35Z

Ok I've rebased this on #57 to achieve these ergonomics instead:

let request = Request::builder()
    .method(method::GET)
    .uri("https://www.rust-lang.org/".parse()?)
    .header("X-Custom-Foo", "Bar".parse()?)
    .request(());

I tried out adding a local TryFrom etc trait but it ran into a few issues:

If you already have a HeaderValue or a Uri then you have to deal with a Result, which may not always be desired.
The type signature was getting a little unusual to look at.
Ideally the TryFrom and TryInto traits would be "sealed", but with blanket impls necessary this may not be possible.
We'd have to select some error for TryFrom<T> for T but there's not really a great choice.

After trying that out for a bit I settled on the addition of .parse() if necessary in these cases. I think we'd just elide that .parse() if we baked in TryInto directly, but it didn't seem that worth it in terms of the ergonomic costs.

thoughts?

alexcrichton · 2017-07-18T14:33:04Z

Ok I've pushed another commit which preemptively incorporates #58 and achieves what I'd view as "optimal/desired ergonomics":

let request = Request::builder()
    .method("GET")
    .uri("https://www.rust-lang.org/")
    .header("X-Custom-Foo", "Bar")
    .request(())?;

I believe the only downside of this approach is that if you only specify infallible values like Method, Uri, etc, (e.g. you never use the string parsing) then you get a "spurious error" at the end that's statically impossible to reach. This seems like a pretty rare scenario to me, though, and a .unwrap() wouldn't be so bad in that case. For other use cases I'd imagine that having the ability to defer all errors to the end is basically what you'd want.

I wanted to address this point though:

In the reqwest review, there was wide agreement that a method in a builder that can fail should return the error immediately, not hold on to them and return it in build.

This guideline is intended for builders like TcpBuilder and other operations where particular operations are always fallible such as TcpBuilder::bind which is calling the bind syscall. This is not an applicable guideline to this use case because none of the operations (e.g. setting a method) are fallible, it's just the argument conversions that are themselves fallible. If users desire errors earlier then they can parse the arguments and be guaranteed that an error won't get returned at the end.

seanmonstar · 2017-07-18T18:42:34Z

This is not an applicable guideline to this use case because none of the operations (e.g. setting a method) are fallible, it's just the argument conversions that are themselves fallible.

I admit, determining this feels a little cloudy to me. I could say that in reqwest, none of the operations there can fail either, they're just parsing a URL, or serializing into JSON, before setting the actual property on the internal Request. A user could get the errors earlier by parsing a Url or running serde_json::to_vec themselves, and then passing that to req.body(json_bytes)... I'm not really against the idea, I just want consistency, and less surprise, and I feel like this is surprising to me, considering the recommendation that was given for reqwest.

alexcrichton · 2017-07-18T19:49:51Z

Do you have a reason for favoring error-on-each-operation versus deferred until the end other than it was recommended?

seanmonstar · 2017-07-18T19:53:00Z

I'm not really against the idea, I just want consistency, and less surprise.

Nope, I don't think I favor one or the other. Just worried about consistency.

alexcrichton · 2017-07-18T19:54:34Z

Can you elaborate how you're worried? Do you have reservations about this strategy? Do you have concerns?

seanmonstar · 2017-07-18T19:59:56Z

Well, am I correct then that using the guideline in this PR, and applying it to reqwest, I should shift all the errors (that are eventually just conversions) back to the build method?

alexcrichton · 2017-07-18T20:00:59Z

If following this exactly, yes. Do you think that's not right? Would you prefer a different strategy?

carllerche · 2017-07-18T20:41:16Z

Just to throw in some more opinions...

It seems to me that the builder is more about convenience, so why deal w/ returning Result at all? 😃

The builder could panic on conversions that fail. If the user wants to be explicit about handling errors, they can convert from &str -> HeaderValue explicitly before passing it into the builder?

carllerche

Some initial thoughts on the PR. I'll try to look in more depth when I get home.

carllerche · 2017-07-18T20:47:26Z

src/request.rs

+/// This type can be used to construct an instance of `Head` through a
+/// builder-like pattern.
+#[derive(Debug)]
+pub struct HeadBuilder {


IMO this builder can build either Head or Request values, so why not name it Builder? 😄

Yeah I think now especially that we've got Request::builder it makes more sense to call this Builder

carllerche · 2017-07-18T20:50:28Z

src/request.rs

+    ///     .build()
+    ///     .unwrap();
+    /// ```
+    pub fn headers_clear(&mut self) -> &mut HeadBuilder {


Could you say more about the motivation for this specific fn? Would the use case be satisfied by a clear fn that resets the entire builder?

Oh this was mostly just following in the footsteps of Command::env_clear, but in retrospect it may not fit here. On Command you inherit everything by default so it makes sense to sometimes need to clear the environment, but here you start with nothing so I don't think it makes as much sense. Will remove.

carllerche · 2017-07-18T20:51:00Z

src/request.rs

+    ///     .request(())
+    ///     .unwrap();
+    /// ```
+    pub fn request<T>(&mut self, body: T) -> Result<Request<T>> {


Maybe name this body as it takes the body argument? It just happens to also perform the final building :)

carllerche · 2017-07-18T20:57:25Z

Not necessary for this PR, but I'd also like to see some more ergonomic builders for common paths. For example:

Request::get("/")
    .header("foo", "bar")
    .body(()); // Yeah, this is a bit weird
    // .into(); maybe that could work instead for requests w/o a body.

Request::post("/")
    .header("content-type", "application/json")
    .body("{}");

alexcrichton · 2017-07-18T21:13:54Z

@carllerche

It seems to me that the builder is more about convenience, so why deal w/ returning Result at all? 😃

That's a possibility, yeah, but I personally feel at least that it's still best to go through Result as much as possible. These builders may not always be pure convenience if they're much nicer to use when constructing a Request or Response, so I could imagine these being inside the guts of an application which ends up taking headers from random sources that'd otherwise run a risk of causing a panic. In general as well I figured one unwrap wouldn't be too bad.

Do you think though that having any result is too much here?

Not necessary for this PR, but I'd also like to see some more ergonomic builders for common paths.

Interesting ideas! I wonder if this'd be odd though with Request::get(..) -> Builder as opposed to returning a Request? In some sense the ergonomics seem much nicer so I wouldn't be too opposed. (just maybe not a lot of precedence for this)

alexcrichton · 2017-07-18T21:28:03Z

Ok I've pushed up a version which also includes a Response builder as well.

withoutboats · 2017-07-23T22:34:33Z

What I'd like to do is not treat these as builders for Head, but as builders for Request and Response. That entails:

The ::build constructor is moved from Head to Request & Response.
The methods which currently return Head instead return Request<()> or Response<()>.

Head would still be public, but only constructable through Request::into_parts and Response::into_parts. It exists as an intermediate for deconstructing the message types into their components.

alexcrichton · 2017-07-24T14:20:08Z

Seems reasonable to me!

carllerche · 2017-07-24T20:21:32Z

@alexcrichton

so I could imagine these being inside the guts of an application which ends up taking headers from random sources that'd otherwise run a risk of causing a panic

So, even if the Builder fns unwrap internally, you can always perform the conversion outside of the builder in order to get the result:

Request::builder()
    .method(try!(Method::try_from("123")))
    // stuff

Do you think though that having any result is too much here?

Really, only builder ergonomics. So, if avoiding all panics in the builder is more important than avoiding the unwrap in apps, then we don't need to do this.

I personally would rather not have to call unwrap() in my app, but I don't have a very strong opinion either way.

withoutboats · 2017-07-24T20:29:52Z

Re returning errors: a big part of the justification for ? was that it made it easier for builders to return errors; rather than save errors til the end I think any methods that can fail should just return Result<&mut Builder>.

It might also be a good idea to have equivalents that don't return results, either because they panic or because they take HeaderValue etc and don't perform the conversion.

carllerche · 2017-07-24T20:34:19Z

Should Builder::head(&mut self) -> Request<()>.

Aka, use the head fn on Builder to construct a request / response w/o a body.

carllerche · 2017-07-24T20:25:38Z

src/convert.rs

+use uri::{self, Uri};
+
+/// dox
+pub trait HttpTryFrom<T>: Sized + Sealed {


Is the plan to alias this to std TryFrom when that becomes stable?

alexcrichton · 2017-07-24T23:19:01Z

@carllerche

you can always perform the conversion outside of the builder in order to get the result:

Indeed! I found this to be pretty unergonomic though because it involves more imports and a lot more ?, whereas if the builders are optimized almost entirely for ergonomics it seemed better to defer until the end to avoid the need for imports as well as cut down on the ? traffic.

I personally would rather not have to call unwrap() in my app, but I don't have a very strong opinion either way.

Oh to clarify I don't imagine apps almost ever wanting to call unwrap() here, the last error on the consumption of the builder seems like it would frequently want to be handled gracefully with ?.

@withoutboats

Re returning errors: a big part of the justification for ? was that it made it easier for builders to return errors; rather than save errors til the end I think any methods that can fail should just return Result<&mut Builder>.

Ah it looks like you came to the same conclusion as I after this which is that this doesn't work great if you have a HeaderValue on hand (or some other concrete type). Builders like a TCP builder would still use this Result<&mut Builder> pattern but here the only fallibility is in the arguments, not the operation, so it's less necessary to return a Result at each stage (you can still learn when the precise error happens).

Oh looking back at @withoutboats's comment above, mind taking another look @withoutboats?

Right now we've got both Request::builder and Head::builder, and we've got Builder::head and Builder::body returning the request/response. That I think is ok enough to de-emphasize Head, but do you think we should remove Head::builder and Builder::head?

seanmonstar · 2017-07-25T01:22:50Z

src/request.rs

+#[derive(Debug)]
+pub struct Builder {
+    head: Option<Head>,
+    err: Option<Error>,


Hm, by holding on to the error here, we allow a user to do a bunch of method calls, even though it's possible the first one failed. I know I've commented about this before, but seeing the implementation, I again don't really feel that this is probably how it should be done...

That's sort of the intention though? Can you elaborate on your worries here?

I've been assuming that we want to optimize for ergonomics on the Builder as that's sort of the point of having a builder, rather than optimizing for the implementation or the nitty gritty semantics per se.

carllerche · 2017-07-25T16:25:34Z

Right now we've got both Request::builder and Head::builder, and we've got Builder::head and Builder::body returning the request/response. That I think is ok enough to de-emphasize Head, but do you think we should remove Head::builder and Builder::head?

Yes, I think the suggestion is that you can't construct a Head w/o having to go through Request, i.e. deemphasize Head to only a return value from into_parts. So, I think Head::builder and Builder::head (as it is) should be removed.

I had two other ideas assuming the above changes are made.

Have a default of T -> () (no body) -- struct Request<T = ()>.
Builder::head(&mut self) -> Request<()>.

I'm not sure how good either of them are, but it's worth a thought.

seanmonstar · 2017-07-26T20:09:23Z

src/error.rs

+    Uri(uri::InvalidUri),
+    HeaderName(header::InvalidHeaderName),
+    HeaderValue(header::InvalidHeaderValue),
+    Io(io::Error),


I'd lean towards "if people really wish http::Error has this, we can always add it later".

This commit adds a new builder for `request::Head` which will transitively allow creation of a `Request` through a builder-like interface. cc #49

All parsing errors are named `Invalid{Type}`, where Type is the type being parsed into (e.g. `InvalidMethod`). The same error is returned regardless of what type the source being parsed was.

Also add `Request::new` and privatize all `Parts` constructors

alexcrichton · 2017-07-26T22:57:20Z

I've now updated the OP as well with a list of closed issues

withoutboats · 2017-07-26T23:28:28Z

👍 looks good to me

alexcrichton mentioned this pull request Jul 10, 2017

Provide request / response builders. #49

Closed

carllerche reviewed Jul 18, 2017

View reviewed changes

alexcrichton mentioned this pull request Jul 19, 2017

would be nice to remove * steveklabnik/simple-server#3

Closed

withoutboats mentioned this pull request Jul 22, 2017

Should the Head type be public? #56

Closed

carllerche reviewed Jul 24, 2017

View reviewed changes

seanmonstar reviewed Jul 25, 2017

View reviewed changes

seanmonstar mentioned this pull request Jul 25, 2017

Add HeaderName/HeaderValue <-> Bytes conversions #61

Closed

seanmonstar reviewed Jul 26, 2017

View reviewed changes

alexcrichton and others added 23 commits July 26, 2017 14:01

Add a Request builder

5806aca

This commit adds a new builder for `request::Head` which will transitively allow creation of a `Request` through a builder-like interface. cc #49

Defer errors until build

4bc29fb

Drop head methods and add `extension

27a4eca

Rename and coallesce errors.

85a0113

All parsing errors are named `Invalid{Type}`, where Type is the type being parsed into (e.g. `InvalidMethod`). The same error is returned regardless of what type the source being parsed was.

InvalidStatus -> InvalidStatusCode

94a4076

Fix merge with errors

971fd01

Add convert::Into impls for HeaderName and HeaderValue

a90dae9

Quick test for StandardHeader::into<Bytes>()

befc3c0

Move convert::Into impls to where all the other trait impls are

8fe256d

Add convert::From<Bytes> impl to HeaderName

820ea81

Move convert::Into<Bytes> for HeaderValue impl to where other impls are

dfc0fa6

Add convert::From<Bytes> impl for HeaderValue

b8c3303

Rewrite fallible conversions to use HttpTryFrom polyfill

1f04392

Convert from convert::Into into convert::From

78bb5dd

Fix compile errors with Bytes conversion errors

e1d7076

Move HttpTryFrom impls next to types

87403c3

Add more HttpTryFrom for HeaderName

e1c2d83

Rename Head to Parts

df77f28

Also add `Request::new` and privatize all `Parts` constructors

Add some explicit conversion methods

890b2d7

Fix a test

24f89b2

Panic on reuse of builders

280f2b4

Remove suport for io::Error from http::Error

7de27af

Rename Builder::head

c266c25

carllerche self-requested a review July 27, 2017 17:15

alexcrichton merged commit 96f250e into hyperium:master Jul 27, 2017

alexcrichton deleted the builders branch July 27, 2017 17:15

alexcrichton mentioned this pull request Jul 27, 2017

Rename and coallesce errors. #62

Closed

Add a Request builder #55

Add a Request builder #55

Conversation

alexcrichton commented Jul 10, 2017 • edited Loading

alexcrichton commented Jul 10, 2017

seanmonstar commented Jul 13, 2017

alexcrichton commented Jul 14, 2017

seanmonstar commented Jul 14, 2017

alexcrichton commented Jul 17, 2017

alexcrichton commented Jul 18, 2017

seanmonstar commented Jul 18, 2017

alexcrichton commented Jul 18, 2017

seanmonstar commented Jul 18, 2017

alexcrichton commented Jul 18, 2017

seanmonstar commented Jul 18, 2017

alexcrichton commented Jul 18, 2017

carllerche commented Jul 18, 2017

carllerche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carllerche commented Jul 18, 2017

alexcrichton commented Jul 18, 2017

alexcrichton commented Jul 18, 2017

withoutboats commented Jul 23, 2017

alexcrichton commented Jul 24, 2017

carllerche commented Jul 24, 2017

withoutboats commented Jul 24, 2017

carllerche commented Jul 24, 2017

Choose a reason for hiding this comment

alexcrichton commented Jul 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carllerche commented Jul 25, 2017

Choose a reason for hiding this comment

alexcrichton commented Jul 26, 2017

withoutboats commented Jul 26, 2017

Add a `Request` builder #55

Add a `Request` builder #55

alexcrichton commented Jul 10, 2017 •

edited

Loading