hyperium / http

Rust HTTP types
Apache License 2.0
1.16k stars 289 forks source link

feat: add support for the digest headers #539

Closed npmccallum closed 2 years ago

npmccallum commented 2 years ago

This commit adds support for the following headers:

These headers are defined in the following upcoming RFC:

https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-digest-headers-08

Signed-off-by: Nathaniel McCallum nathaniel@profian.com

npmccallum commented 2 years ago

@rvolosatovs FYI

jeddenlea commented 2 years ago

As written, this is incomplete. Any predefined, const HeaderName needs to be included in the internal header::name::parse_hdr function.

As is, this would fail:

    let h = http::header::HeaderName::from_static("want-repr-digest");
    assert_eq!(h, http::header::WANT_REPR_DIGEST);

with,

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `"want-repr-digest"`,
 right: `"want-repr-digest"`', src/main.rs:4:5

...which is an unfortunately misleading error message, but is due to the internal representation of the const headers being different than ad hoc headers.

I do not speak on behalf of the crate owners, but I suspect these particular headers may not rise to the level of universal inclusion. If you would like to argue otherwise, I would humbly suggest you rebase this on top of my branch in #499, as it automates the inclusion of the const header names in parse_hdr. When that is finally merged, if it is decided that these headers belong in this crate, this change alone would work. But, if not, it will make dealing with non-standard headers easier in general.

seanmonstar commented 2 years ago

Yes, my initial feeling is that we wouldn't include these header names. The included constants tend to be very common. When the referenced pull request is merged (very soon), a user can make these constants in their own code.

npmccallum commented 2 years ago

@seanmonstar @jeddenlea I actually concur with you that these headers probably don't warrant inclusion just yet. However...

We have a chicken and egg problem. In the headers crate, Header::name() -> &'static HeaderName requires a return value of a static reference. Further, HeaderName has no public const constructor. Therefore, while http regards these headers as extensible, only http-defined headers can be used with the headers::Header trait. So I see three options:

  1. Merge the headers into http. I think we agree this isn't the optimal solution.
  2. Give HeaderName a public const constructor. I think #499 does this. But it isn't clear when this can be merged.
  3. Modify Header::name() to return HeaderName rather than &'static HeaderName. This is a breaking change.

Thoughts?

seanmonstar commented 2 years ago

The Header::name() was made a function specifically to allow for static header names that aren't constants. Otherwise it was going to be an associated constant. As a function, a user can return something from a lazy_static or similar.

But yes, the const constructor should be merged shortly. I have time next week for the review.

npmccallum commented 2 years ago

@seanmonstar I had attempted this with once_cell and failed. I might need to retry.

jeddenlea commented 2 years ago

@npmccallum There is a good chance the problem you ran into with once_cell (or lazy_static) is that most of the places within the http API that want a header name express that via traits that don't align with the structs that either once_cell or lazy_static produces. If that was the issue, the answer is just to force a dereference, e.g., https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=fd83935a06a6d659f97a1dd4b9aa125a

robjtede commented 2 years ago

Would be surprised if these landed in http, at least before the standard is out of draft.

Like Jed said, you can achieve this with once_cell easily.