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

Use strong typing for the response's headers #24

Open
tomaka opened this issue Sep 23, 2016 · 7 comments
Open

Use strong typing for the response's headers #24

tomaka opened this issue Sep 23, 2016 · 7 comments

Comments

@tomaka
Copy link
Owner

tomaka commented Sep 23, 2016

Instead of providing a single member headers: Vec<(String, String)>, the Response should have multiple members each corresponding to a header.

@tomaka
Copy link
Owner Author

tomaka commented Oct 14, 2016

The objectives of that change would be:

  • Avoid typos in the headers names and values.
  • Make it easier to build more complex headers (like cookies, or cache-control, which are not necessarily trivial)
  • Make it easier to modify a header already in the response without having to go through the list of headers (eg. adding a cookie).
  • Prevent headers that should only appear once from appearing twice.
  • Still allow custom headers if required.
  • Implicitly forbid some headers like Connection or Upgrade by making them not appear in the list.

However I really don't like hyper's strong typing system, as it's confusing, difficult to understand, and annoying to use. Rouille's API should remain as idiomatic as possible, and not use meta-programming techniques.

In my opinion the new API should look like this:

pub struct Response {
    pub status_code: u16,
    pub cache_control: ...,
    pub content_type: ...,
    pub cookies: Vec<Cookie>,
    ...
    pub custom_headers: Vec<(String, String)>,
    pub body: ResponseBody,
    private_member_so_that_headers_can_be_added_later_without_a_breaking_change: (),
}

However a good trade-off needs to be found between strong typing and usability. For example in my opinion the status code should remain a u16 and not an enum, because with an enum the user would have to import it in scope. However for cookies it's more acceptable because it's a complex object and it's not frequent to build cookies.

The problem that remains is how to handle headers that are specific to a status code, like Location or WWW-Authenticate.

@tomaka tomaka modified the milestone: Version 1.0 Nov 30, 2016
@tomaka
Copy link
Owner Author

tomaka commented Dec 3, 2016

The other problem is how to handle that with the cgi and reverse proxy features.
Obviously we don't want to parse the headers returned by cgi/reverse-proxy just to build a response and then serialize them again.

@tomaka
Copy link
Owner Author

tomaka commented Dec 3, 2016

I think we need the following changes:

  • Create a new RawResponse type that has the same fields as the current Response.
  • Modify Response to be strongly-typed (two posts above). Implement From<Response> for RawResponse.
  • The closure passed to start_server must return a T where T: Into<RawResponse>.
  • All the helpers in rouille would return a Response except the cgi and reverse-proxy stuff which would return a RawResponse. This seems reasonable as cgi/reverse-proxy are rarely used at the same time as regular responses building.

@tomaka
Copy link
Owner Author

tomaka commented Dec 3, 2016

The custom_headers member can be removed from the Response proposal above, since the user can turn it into a raw response and add their own headers.

@tomaka
Copy link
Owner Author

tomaka commented Dec 13, 2016

PR #65 is now mature enough to give a small idea of how it looks like.
Overall I have mixed feelings.

Things I noticed:

  • It is not really elegant to enforce a correct response with a struct layout. For example with the status code 401 it is mandatory to include a WWW-Authenticate header. But since it's the only status code that contains a mandatory header (not even 301/302/303 require a mandatory location for example) and since a WWW-Authenticate header can also be added to any other status code, I didn't find an appropriate struct design. In the end I decided to make www-authenticate optional but automatically turn a 401 status code into 403 if www-authenticate wasn't included. This is a bit crappy.

  • You probably end up allocating more memory with a strongly-typed response because of all the intermediary strings and containers that must be joined together at the end. With a weakly-typed response you just build one string. If you take for example WWW-Authenticate: Basic realm="foo". With a strongly typed response you need three strings (Basic, realm and foo) and two Vecs (for the list of challenges and list of challenge parameters). With a weakly-typed response you just build Basic realm="foo" immediately and have one string.

  • The split between Response and RawResponse is ok from a usability point of view, I think. Maybe the fact that you need to use into() is a bit confusing, but it's acceptable.

  • Right now I changed the log function to take a Into<RawResponse> and return a RawResponse. This would eventually need to be changed by adding a trait.

  • try_or_400! and try_or_404! needed to be changed to add .into() so that they can return either a Response or a RawResponse.

  • I kinda like the design of rouille right now (with a weakly-typed response) in the sense that it mimics web servers interacting with each other. Each function that takes a &Request and returns a Response is kind-of similar to a mini web server, and returning a weakly-typed response mimics what a real HTTP response would look like.

  • I like the fact that converting from a RawResponse to a Response doesn't have access to the Request. However this is problematic because the headers should be different depending on whether or not the Request uses HTTP 1.0 or 1.1 (for example with cache control). I'm not sure whether this is really a problem, because we could just "ignore" HTTP 1.1. The same problem arises with a weakly-typed response as well anyway, and some methods would need to have access to the request.

@tomaka
Copy link
Owner Author

tomaka commented Dec 13, 2016

For now my conclusion is: negative, but let's postpone and not close it altogether.

@tomaka tomaka removed this from the Version 1.0 milestone Dec 13, 2016
@MHordecki
Copy link

Is hyper's Headers implementation really that hard to use? It seems to me that their approach is the natural end-game for a strongly-typed approach to HTTP headers (modulo some improvements). The metaprogramming use is purely optional, and merely automates away the boilerplate.

At the end, using it boils down to req.headers.get::<Location>(), as opposed to req.location, and in both cases one receives an Option type anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants