http-rs / http-types Goto Github PK

This would touch tide and possibly async-h1 as well. The idea proposed by @jdortiz in discord was for tide to support connection cancellation in order to early-return from the middleware-response handler stack

mime::ParamValue has a private tuple struct constructor and no accessor methods, so there's no way to actually get the value out of them.

With #102 implementing Body::from_file we should probably remove the magic-bytes based mime sniffing from the other streams, and only keep it for Body::from_file.

Additionally we should expand our sniffing capabilities similar to http-rs/tide#461, but probably using a manual mapping instead of using mime_guess. We already cover most AV formats through the magic bytes, all we'd need to do is add some basic mappings for e.g. JS, HTML, CSS, SVG.

In Surf creating new requests and responses is really straightforward. It'd be nice if we were able to expose shorthands from http-types as well.

// current
let req = Request::new(Method::Get, Url::parse("https://foo.biz").unwrap());

// with this proposal
let req = Request::get(Url::parse("https://foo.biz").unwrap());

// once the TryFrom patch on the url crate is published
let req = Request::get("https://foo.biz");

The downside is that this will add more methods to the Request type. But we already have many methods, and these feel fairly natural as constructors. I would propose we add them.

Marking as "good first issue" because this should be fairly straightforward. The bounds should be copied from the Request::new method, forwarding to Request::new internally. Thanks!

We should enable the following:

assert_eq!(req.header("name"), Some("value"));

For future reference: the design of combining Error, ErrorKind and BoxError into a single catch-all type might not be the right way to go about it.

An alternative would be to only have a single catch-all error type that stores a message + StatusCode, so that it can wrap any other error type. That way we don't have to worry about all the error kind stuff and can simplify error construction throughout.

@dignifiedquire mentioned yesterday that error construction takes a lot of parameters, and I agree. This could be a way to improve that.

As per https://github.com/dignifiedquire/tide/pull/1#discussion_r373361446 we should provide docs on what the header-related Iterator impls in the crate provide.

Request::recv_trailers and Response::recv_trailers should return non-clone channels, matching their Sender counterparts. I'm currently integrating this, and because we have an anonymous future calling it from a poll context means we need to box it, which isn't great.

This request type is quite common for file upload APIs, so it would be nice to have some ergonomic helpers to construct them. Support of streaming uploads is essential.

Examples of multipart APIs:

• https://docs.rs/reqwest/0.10.4/reqwest/multipart/index.html
• https://docs.rs/crate/multipart/0.16.1

A request / response is a combination of "headers" + "body" inside a single type. We should explain how that works, and why it's a single type (rather than multiple).

This is after a conversation with @rylev where he expressed he was somewhat confused about it, but after chatting more it made more sense. Documentation would be a way of pre-empting people having questions about it, and explain the model we're using here. Thanks!

Because it doesn't, this does not compile:

async fn get(addr: &str) -> anyhow::Result<Response> {
    // ...
    let resp = async_h1::connect(stream, req).await?;
    Ok(resp)
}

It's not obvious what to do, so I'm using a hack like this:

let resp = async_h1::connect(stream, req).await.map_err(anyhow::Error::msg)?;

In any case, all error types should implement std::error::Error in order to work seamlessly with the ? operator.

Ref #61, we should do the same for more methods.

#47 introduces a cookies method which returns a Result<Vec<Cookie>>. But all of our plural methods (header, header_names, header_values) return iterators, making this method somewhat inconsistent.

Would it make sense to make this return an iterator of cookies? Thoughts?

After #119 we're still missing Response::host, the only method left from #117. This should be modeled after https://docs.rs/actix-web/3.0.0-alpha.2/actix_web/dev/struct.ConnectionInfo.html#method.host. Thanks!

cc/ @jbr

After having reviewed http-rs/tide#380, I'm now wondering if we should have a different approach for cookies than what we currently have. This supersedes #48.

Current API

We currently provide APIs for both getting and setting cookies in the request and response headers. Each call to e.g. Response::cookie decodes the full set of cookies, and returns the corresponding cookie:

https://github.com/yoshuawuyts/http-types/blob/b1623854102986b151d07406b6949d985787d1fd/src/response.rs#L305-L314

https://github.com/yoshuawuyts/http-types/blob/b1623854102986b151d07406b6949d985787d1fd/src/response.rs#L331-L335

This has the downside of being rather slow, as each access of a cookie ends up going through a full decode stage.

Proposed API

In http-rs/tide#380 Request has cookie,set_cookie, and remove_cookie methods that only parse cookies once. And even better: it automatically sets the correct Set-Cookie headers on the Response by calculating the delta of the cookies that have been set before, and which need to be set again.

To make that API easier to write, I'd like to propose we change the current cookies API in http-types to primarily operate on CookieJar instead.

impl Request {
    /// Parse the cookies from the headers into a `CookieJar`
    pub fn cookies(&self) -> CookieJar;

    /// Encodes the delta of the cookies in the `CookieJar` as the corresponding HTTP headers.
    pub fn set_cookies(&mut self, jar: &CookieJar);
}

impl Response {
    /// Parse the cookies from the headers into a `CookieJar`
    pub fn cookies(&self) -> CookieJar;

    /// Encodes the delta of the cookies in the `CookieJar` as the corresponding HTTP headers.
    pub fn set_cookies(&mut self, jar: &CookieJar);
}

This way http-types can operate on cookie jars, rather than on individual cookies. And as we can see in Tide it would then be possible to at the framework layer enable operating on individual cookies instead.

cc/ @tirr-c @rylev

All of our methods should be documented and have doctests. We don't need to go overboard; but having code examples would go a long way. Thanks!

Getting a value from Headers and comparing it to a string is quite involved. The current way to do it is:

if let Some(encoding) = res.header(&TRANSFER_ENCODING) {
    if !encoding.is_empty() && encoding.last().unwrap().as_str() == "chunked" {
       // proceed
    }
}

Even though we implemented more convenient comparisons in #80, this is still quite involved. What we'd like to be able to do is:

if res.header(&TRANSFER_ENCODING) == "chunked" {
    // proceed
}

Note on Option<Vec<HeaderValue>>

We currently return Option<Vec<HeaderValue>> from various of our methods. This is because HTTP by design can have multiple values for a single key. However there is some overlap between a vec of 0 elements, and a None value. The idea we've been talking about offline is to perhaps introduce a new type: HeaderValues which implements Iterator<Item = HeaderValue>> and guarantees to never be empty.

This would provide a convenient location to implement equality checks. Examples include:

• PartialEq<[str]> for HeaderValues
• PartialEq<str> for HeaderValues
• PartialEq<String> for HeaderValues
• PartialEq<HeaderValue> for HeaderValues

Returning HeaderValues from existing methods would be a breaking change, so we shouldn't do this right away. But this provides a cleaner path forward than implementing equality for Vec<HeaderValue>.

Currently, the Body's length is usize - which implies that a 32-bit web server could only respond with 4GB of data without using chunked encoding, even if they're streaming data which could be much larger. This could be resolved by using a u64 instead.

http/2 introduced a mechanism for sending back multiple responses for a single request (PUSH frames). This is often used as an optimization for requesting static files in the browser, so for when example a user requests index.html we send back bundle.css and bundle.wasm as well because we know that those will be requested next.

This mechanism is not limited to http/2, http/3 makes use of it as well. And as such it makes sense to introduce a mechanism for sending back multiple responses into http-types since it's part of the HTTP semantics now.

API example

In #126 (comment) I authored an example for multipart responses, which are "multiple bodies sent in response to a single request". HTTP push is "multiple responses sent in response to a single request". So I was thinking we could make it a similar-ish API, but multiplex on the response level rather than on the request level. This is a sketch

let mut res = Response::new(200);
res.set_body(Body::from_file("assets/index.html").await?);

// provide a way to do feature detection. Knowing the http version probably works well enough.
if req.version() >= http_types::Version::2_0 {
    // we should send over fully-qualified HTTP responses.
    let mut css = Response::new(200);
    css.set_body(Body::from_file("assets/bundle.css").await?);
    // in order to push we need to share which URL this resource would live at.
    res.push("bundle.css", css);

    let mut wasm = Response::new(200);
    wasm.set_body(Body::from_file("assets/bundle.wasm").await?);
    res.push("bundle.wasm", wasm);
}

// provide some way to iterate over the pushed responses
for pushed in res.pushed() {
    println!("{}", pushed.url());
}

alternatives

Something that's been brought up before is to use a channel based approach provided by request instead. I think the right mental model for push is: "multiple responses to a single request" and we should create an API that matches that. And actually sending back multiple responses in response to a request is the clearest way to model that.

Similar to http-rs/async-h1#16; I think it would make sense if the majority of the logic would live in this crate, so that when it's integrated in async-h1 it just works.

This is my mistake; should've filed it here 😓

There should be a way to apply random things to either of the Request or Response structs when chaining. I'd like to propose we introduce a new method apply for this purpose:

let res = Response::new(200)
    .apply(Policy::new);

Apply would take a closure that is then passed &mut self. Policy would implement Fn so it can actually apply that.

Extra nice would be if Policy had an apply function that did the inverse. So it can apply itself to a Request of Response.

We probably need to create a shared trait for this to work somehow. We can figure out the details later.

In http-rs/tide#537 we couldn't update the Request::query method to http_types' version because tests were failing. We should port Tide's tests to http-types and fix the query method to match the behavior. This means correctly parsing empty query strings, and setting a 400 status code.

This should be fairly straight forward, so marking as a good first issue!

Refs

• Tide tests: https://github.com/http-rs/tide/blob/master/tests/querystring.rs
• Request::query: https://docs.rs/http-types/2.0.1/http_types/struct.Request.html#method.query

@zkat mentioned this: we should have a way to pass multiple headers of the same name into the headermap.

http does this through the append call, which seems like the right name for the operation. And values can be extracted through the get_all call.

Also we should have an iterator for HashMap that returns all k-v pairs.

Proposed Methods

• Headers::append -- append multiple entries of the same name
• Headers::get_all -- get an iterator of header name entries
• Headers::remove -- remove a single entry, returns an iterator over all entries
• Headers::iter -- create an iterator over all header entries

Similar to http-rs/surf#101 and http-rs/surf#108 we should make string parsing and encoding aware of the Content-Type header.

cc/ @goto-bus-stop; pinging you here since you contributed the patch to Surf; we're planning to move Tide and Surf both over to http-types, so figured it'd make sense to include the work you've done here.

This is a tracking issue for typed headers. I looked over the
HTTP headers page and categorized which headers would need to be ported.

There are probably some nuances in how we want to implement them, but this provides an overview of how far along we are in constructing typed constructors for all header types.

edit: I realize now that this might read a bit dated. I drafted this text back in December but forgot to post it. Some things may seem a bit outdated or different; the most important part is that we can track whether we can construct headers .

Authentication (auth)

• WWW-Authenticate (auth::Authenticate)
• Authorization (auth::Authorization)
• Proxy-Authenticate (auth::ProxyAuthenticate)
• Proxy-Authorization (auth::ProxyAuthorization)

Caching (cache)

• Age (cache::Age)
• Cache-Control (cache::Control)
• Clear-Site-Data (cache::ClearSite)
• Expires (cache::Expires)
• Pragma (cache::Pragma) (HTTP/1.0 only)
• Warning (cache::Warning) (soon to be deprecated)

Client Hints (client_hints)

• Accept-CH (ch::Accept) (experimental, implementation postponed)
• Accept-CH-Lifetime (ch::Lifetime) (experimental, implementation postponed)
• Early-Data (ch::EarlyData) (experimental, implementation postponed)
• Content-DPR (ch::ContentDpr) (experimental, implementation postponed)
• DPR (ch::Dpr) (experimental, implementation postponed)
• Device-Memory (ch::DeviceMemory) (experimental, implementation postponed)
• Save-Data (ch::SaveData) (experimental, implementation postponed)
• Viewport-Width (ch::ViewportWidth) (experimental, implementation postponed)
• Width (ch::Width) (experimental, implementation postponed)

Conditionals (conditionals)

• Last-Modified (conditionals::LastModified)
• Etag (conditionals::Etag)
• If-Match (conditionals::IfMatch)
• If-Modified-Since (conditionals::IfModifiedSince)
• If-Unmodified-Since (conditionals::IfUnmodifiedSince)
• Vary (conditionals::Vary)

Content Negotiation (content)

• Accept (content::Accept)
• Accept-Charset (content::Charset) (no longer supported by any browser)
• Accept-Encoding (content::Encoding)
• Accept-Language (content::Language)

Controls (controls)

• Expect (controls::Expect)

Cookies

I think this one is special, and we should re-export the
cookie crate, and just expose get /
set cookies as methods on the Request and Response types. Let's just treat
them as built-ins to evade the name collision.

CORS (cors)

• Access-Control-Allow-Origin (cors::AllowOrigin)
• Access-Control-Allow-Credentials (cors::AllowCredentials)
• Access-Control-Allow-Headers (cors::AllowHeaders)
• Access-Control-Allow-Methods (cors::AllowMethods)
• Access-Control-Expose-Headers (cors::ExposeHeaders)
• Access-Control-Max-Age (cors::MaxAge)
• Access-Control-Request-Headers (cors::RequestHeaders)
• Access-Control-Request-Method (cors::RequestMethod)
• Access-Control-Origin (cors::Origin)
• Access-Control-Timing-Allow-Origin (cors::TimingAllowOrigin)

Do Not Track (privacy)

• DNT (privacy::DoNotTrack)
• Tk (privacy::TrackingStatus)

Content Information (content)

• Content-Disposition (downloads::Disposition)
• Content-Length (content::Length)
• Content-Type (content::Type)
• Content-Encoding (content::Encoding)
• Content-Language (content::Language)
• Content-Location (content::Location)

Proxies (proxies)

• Forwarded (proxies::Forwarded)
• Via (proxies::Via)

Response (response)

• Allow (response::Allow)

Range (content)

• Accept-Ranges (range::Accept)
• Range (range::Range)
• If-Range (range::If)
• Content-Range (range::Content)

Security (security)

• Cross-Origin-Opener-Policy (security::Coop)
• Cross-Origin-Resource-Policy (security::Cors)
• Content-Security-Policy (security::Csp)
• Content-Security-Policy-Report-Only (security::Cspro)
• Expect-CT (security::Cspro)
• Feature-Policy (security::FeaturePolicy)
• Public-Key-Pins (security::HPKP) (deprecated)

Other

• Alt-Svc
• Date
• Large-Allocation (unspecced; firefox only)
• Link
• Referer
• Retry-After
• SourceMap
• Upgrade
• User-Agent
• Keep-Alive

Was going through Tide, and we rely on it there. We should probably enable it by default.

I'm currently working on a demo project using tide to demo JWT authorization. It's not open source yet but it probably will be.

Looking at Mozilla's documentation, the structure of the Authorization header consists of <type> <credentials>, so in my app, I modeled this with the following struct.

pub enum AuthorizationKind<'a> {
    Basic,
    // ...
    Unknown(&'a str), // borrowed reference to the underlying `HeaderValue`
}

pub struct AuthorizationData<'a> {
    pub kind: Option<AuthorizationKind<'a>>,
    pub data: &'a str,
}

This model allows most use-cases that I can think of, even outside of what is recommended, <type> can be omitted such that there's no authorization type present in the header, e.g. Authorization: BUGU93VSw-mUi5-d0WNPq0JjXdMt2Ls7unnX4GuLNtg.

All of this can be done zero-copy as far as I can tell by just referencing ranges in the internal HeaderValue.

I'm implementing this in my own demo codebase and I realized this is probably useful to a lot of people beyond me, so I'm wondering if I can contribute this to this library.

Would such an addition be considered, or is this outside of the range of what http-types wants to be?

There's a spec for the terminology and parsing of mime types that we would likely want to follow. We already use WhatWG spec-compliant URLs, so having the mime types match up too seems like it'd be a great match.

This will enable the authoring of the following methods:

impl Request {
    fn mime(&self) -> Option<&Mime>;
    fn set_mime(&mut self, mime: Mime) -> Option<&Mime>;
}

impl Response {
    fn mime(&self) -> Option<&Mime>;
    fn set_mime(&mut self, mime: Mime) -> Option<&Mime>;
}

impl TryFrom<HeaderValue> for Mime;

note: the set_mime methods already exist, but they return a HeaderValue as we currently can't guarantee they're valid mime types.

Links

• https://mimesniff.spec.whatwg.org/#parsing-a-mime-type (parsing spec)
• https://github.com/jsdom/whatwg-mimetype (package by spec author)
• https://mimesniff.spec.whatwg.org/#mime-type-representation (mime type representation)

_req.take_body().into_json().await?

on using above syntax while parsing body gives "EOF while parsing a value at line 1 column 0 error".
Please look into it and fix the error.

The Debug output for Headers is currently pretty hard to parse. This is the output containing a single header:

    status: Ok,
    headers: Headers {
        headers: {
            HeaderName(
                "traceparent",
            ): HeaderValues {
                inner: [
                    HeaderValue {
                        inner: "00-14576827793038113322513871894673895836-202c105b7cb49aa1-01",
                    },
                ],
            },
        },
    },
    version: None,
    trailers_sender: Some(
        Sender { .. },
    ),
    trailers_receiver: Some(
        Receiver { .. },
    ),
    body: Body {
        reader: "<hidden>",
        length: Some(
            0,
        ),
    },
    ext: Extensions,
    local_addr: None,
    peer_addr: None,
}

We could probably do better here. For example HashMap does much better here:

[src/main.rs:5] m = {
    "hello world": "world",
}

[src/main.rs:5] m = {
    "hello world": "world",
}

For example something like this would be much nicer already:

status: Ok,
headers: {
   "traceparent": [
        "00-14576827793038113322513871894673895836-202c105b7cb49aa1-01"
    ],
}

And even then we could probably special-case it further so single-header items (the most common case) wouldn't be displayed as arrays, which reduces nesting further:

status: Ok,
headers: {
   "traceparent": "00-14576827793038113322513871894673895836-202c105b7cb49aa1-01",
}

We're applying breaking changes to them anyway. We should reserve the right to make changes to them before until we've fully integrated them in async-h1.

From talking with @dignifiedquire offline about #51, the biggest constraining feature we have currently is not being able to initialize mime types with attributes as constants. E.g.

const TEXT: Mime = "text/plain; encoding=utf-8"

We should figure out how to enable it, probably looking to hyperium/mime as inspiration as their impl does allow for this.

And the methods to ext and ext_mut. Terminology around "local state" that returns a TypeMap indexed by TypeId has been hard to follow.

Instead talking about "here's a way to store more values in request and response" feels like a better fit. This is similar to Hyper's http crate, but with a different explainer.

HTTP/1.1, HTTP/2.0, and HTTP/3.0 all support trailing headers. http-rs/async-h1#44 includes a .trailer method for ChunkedDecoder. However it seems we could probably generalize this. The hyperium/http-body crate includes a Body::trailers method as well.

Design Sketch

impl Request {
    async fn set_trailers(&self, headers: Headers);
    async fn trailers(&self) -> io::Result<Option<Headers>>;
}

impl Response {
    async fn set_trailers(&self, headers: Headers);
    async fn trailers(&self) -> io::Result<Option<Headers>>;
}

notes

• This design should work with &self because async-std's has an impl AsyncRead for &TcpStream so we don't need to borrow mutably.
• Errors returned by trailers should be of kind io::Error. Possibly even http_types::Error if we can extend it to wrap io::Error (through thiserror's wrapping methods).
• Ok(None) should be returned in HTTP/1.1 if Content-Type was not Chunked.
• Ok(None) should be returned in all versions of HTTP if there were no trailing headers, as opposed to sometimes returning an empty Headers instance.
• set_trailers should be called at the end of an HTTP parsing run, but should not consume the Request / Response instance so that trailers can still be called. We should include the intent of this API in the documentation.
• This API makes use of an internal async_std::sync::channel, similar to the PR linked above.
• This is the only instance where a Headers instance is separate from Request and Response. This means Headers needs to expose Headers::new for the purpose of initializing trailers.
  • Alternatively we could create a new struct, Trailers just for the purpose of setting and receiving Trailers. Just like Request and Response this would impl Deref<Headers> and impl DerefMut<Headers> and forward the methods from Headers.

When writing code quickly it's nice to be able to quickly jot down statuscodes for errors. It'd be nice if the Status trait was able to accept a wider range of input.

// current
let bar = foo.status(StatusCode::NotFound)?;

// proposed
let bar = foo.status(404)?;

We already do this in the Response constructor as well. This should provide a template for how to go about it. Marking as a good first issue!

Related to #29, we currently assume all streams by default are octet streams. By using the MIME type sniffing algorithm we can do better and try and determine the mime type of a stream when it is first passed, if it hasn't been set already.

Impl

When a stream is passed as the body:

• check whether a mime type has been set already
• if no mime type has been set, peek at the first 4 bytes of the stream
• determine the mime type using a crate such as infer
• if no mime type matches, fall back to mime::BYTE_STREAM
• set the Content-Encoding header accordingly

We should have an optional "hyperium-http" feature for taking an instance of hyperium/http::{Request,Response} and converting it to and from http_types::{Request,Response}.

We could probably literally do this with Into/From implementations.

Similar to actix's ConnectionInfo::host struct we should expose peer_addr, local_addr and host for responses, and remote for requests.

This is a requirement for implementing http-rs/tide#462 and http-rs/async-h1#99. Thanks!

Body should resolve to be Body::empty

The ascii part is kind of implied; there are no differences in how we parse. The most important part is whether we're converting from a vec or from a string. This is a breaking change.

It seems that so far we are only duplicating the mime crate, splitting the ecosystem unnecesarrily. Why not just reuse that?

There should be a way to wipe a request + response headers, and set a new {url,statuscode,method}. That allows request allocations to be reused between runs without going through the allocator.

We're currently using io::Error in several places as a placeholder for our own error types. We should probably migrate before we can release.

Hi, I am not a user of this crate, but I saw the announcement post. Please feel free to close this if you disagree.

In the post, you listed use of the url::Url type as an advantage of this crate. But I think it is not a good fit for such a library.

Semantically, the thing that goes in an HTTP request is not a url::Url (== WHATWG URL). It is a "Request Target" as defined by RFC 7230. They are of course related, but not interchangeable.

Some URLs are not valid Request Targets. For example, ftp://foo/bar is a valid url::Url, but obviously should not reach HTTP. But Request::new() accepts all urls without any verification. (There are many other examples).

Some Request Targets are not valid URLs. For example, * is a valid Request Target (see RFC) that is not a URL. (There are many other examples). But async-h1::server::decode() does let uri = url::Url::parse(&format!("{}{}", addr, uri))?; (where uri is httparse::Request.path) which will fail for such.

Also, the hyperium::http interop module has conversions between url and http::Uri but this will also run into trouble as described above.

I suspect this will cause you a lot of headache. My suggestion is to use a type which models a Request Target as described in the RFC and not use WHATWG URL which is really meant for browsers, not the wire. The http::Uri type does this correctly IMO, except for the name -- I would call it RequestTarget to avoid any and all confusion.

When converting an http::Request from hyper to an http_types::Request the url fails to convert with an error:

relative URL without a base

small example problem showing the failure:

use std::convert::TryFrom;

fn main() {
    let request = http::Request::builder()
        .method("GET")
        .uri("/")
        .header("Host", "localhost")
        .body(http_types::Body::empty())
        .unwrap();

    http_types::Request::try_from(request).unwrap();
}

This might a bit controversial but I'd like to bring it up. I want to suggest to remove the generic From<StdError>.

The reason being the following:

As long as this generic implementation exists it is not possible for errors as defined by users of this library to implement their own Into implementations, this makes it hard to use when you need non-generic 500 error codes.

Similar to http-rs/async-h1#16; I think it would make sense if the majority of the logic would live in this crate, so that when it's integrated in async-h1 it just works.

This is my mistake; should've filed it here 😓

Example

Request::new("/hello")
    .body_with_length(my_body, 1024);

I was thinking we should do the following:

• Rename the main methods should be renamed to Server::respond and Client::request.
• The argument passed to both methods should be impl Into<Request>.

That should set us up better for using Surf to construct requests that can be passed into Tide.