Design suggestion on `State` system.

[thedodd]

Greetings all. Excellent work on this system so far. Wanted to share an opinion to generate some discussion &c.

IMHO, it may be a nice improvement to have the State system — as it relates to Handlers — reworked a little. So, instead of Handlers (and middleware) taking a State instance, what if they took a new type instance, say Request, which looked something like this:

/// This type simply lifts the standard request components out of `State` (so that
/// you don't have to go through the `BorrowBag` interface). Now, instead of `State`
/// holding all of the fundamental components related to the request, `State` itself will
/// be one of those fundamental request components inside of the `Request` instance.
pub struct Request {
    pub body: hyper::Body,
    pub method: hyper::Method,
    pub uri: hyper::Uri,
    pub http_version: hyper::HttpVersion,
    pub headers: hyper::Headers,
    pub handler: tokio_core::reactor::Handle,
    pub state: gotham::state::State,
}

Why? This may help to mitigate some confusion as to how those fundamental request components are accessed. This approach would make it much more explicit.

Thoughts? I'm happy to put some work in on this as well.

[smangelsdorf]

Thanks for proposing this @thedodd. Definitely some sensible points made in here. In the spirit of opinions and discussions I'm going to put this one against the 0.3 milestone so we remember to revisit after 0.2 is released, and consider what course to take.

The release of 0.2 is likely to elicit some more opinions on Gotham's API, and hopefully some of them will land in here too. 😁

[ParisLiakos]

This is a good suggestion, it will definitely help newcomers and the initial learning curve. But maybe we should not create yet another request object.. Hyper already has one and having two of them would confuse people that write integration libraries (and not only).

I think we should just provide the hyper request as a separate argument to handlers...thinking something along this signature;

use state: gotham::state::State;
use hyper::{Request, Response};

fn my_handler(state: State,  request: Request) -> (State, Response) {
    // Use request directly to grab common stuff.
    // Use state to retrieve things like diesel connections.
}

Standardizing on hyper request/response objects (and then, the separate http crate) would be very good for interoperability in the long-term.

[nyarly]

I tend to agree that two things called Request would be confusing. I was just reviewing the State setup code and notice that it makes use of Request.deconstruct() to put all the parts of a Request into the State. For the 90% case, getting the parts of the Request from the State should be sufficient (IMO).

The exception (raised in #186) is in wrapping Hyper Services in Gotham Handlers. For that case, it'd be nice to have the inverse operation: constructing a new-but-equivalent hyper::Request from the State.

[nyarly]

Also, I'm not sure where to anchor this, but the BorrowBag nature of State is such that it's unclear what items it contains by default. A reference for that in the rustdocs somewhere might forstall a lot of this discussion.

[whitfin]

@nyarly what do you think about being able to replace State with an instance of http's Request? For anything we use that doesn't fit in that API, we can place it inside the extensions.

[nyarly]

Looking at the extensions in Request I'm concerned that we'd be overloading its intended purpose. I'm only about 30% sure, but it seems like it's meant to support something like https://www.w3.org/TR/WD-http-pep-971121.html#_Toc404743937

What if State had a request method? The two approaches seem roughly equivalent - the only benefit I can see to this one would be avoiding the possibly-maybe overlap and confusion of using the extensions.

[msrd0]

I just looked at this issue and trying to understand the rationale behind it. Is this just about a more convenient way to retrieve headers/cookies/... from the state? If so, I don't think a complete redesign of the State struct is necessary and we could just provide convenient access methods like state.headers() or state.cookies() which just hide borrow call.

Also, as said in #186, http::Request has a from_parts methods, so as long as we put all parts into the State, it seems to be quite easy to reconstruct the hyper request that way.

[nyarly]

This lingered for 2 years without real update. I think convenience methods to get Requests from State would be helpful (maybe TryFrom<State> for Request?) I'd be happy to see issues raised with that kind of approach, but State has been a part of how Gotham works for a long time now.