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

feat: add support for the digest headers #539

Closed
wants to merge 1 commit into from

Conversation

npmccallum
Copy link

This commit adds support for the following headers:

  • Repr-Digest
  • Content-Digest
  • Want-ReprDigest
  • Want-Content-Digest

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

This commit adds support for the following headers:
  * Repr-Digest
  * Content-Digest
  * Want-ReprDigest
  * Want-Content-Digest

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
Copy link
Author

@rvolosatovs FYI

@jeddenlea
Copy link
Contributor

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
Copy link
Member

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
Copy link
Author

npmccallum commented Apr 2, 2022

@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 Make HeaderName::from_static const #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
Copy link
Member

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
Copy link
Author

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

@jeddenlea
Copy link
Contributor

@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
Copy link

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.

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

Successfully merging this pull request may close these issues.

4 participants