response header rework

[robjtede]

PR Type

Feature

PR Checklist

• Tests for the changes have been added / updated.
• Documentation comments have been added / updated.
• A changelog entry has been made for the appropriate packages.
• Format code with the latest stable rustfmt

Overview

This PR normalizes header methods used throughout the crates and enables header K/V pairs to use the same methods on Response as the typed headers (and give more attention to typed headers in the process).

In general, the signatures have changed from insert_header(key: K, value: V) to insert_header(header: H), where H implements a new trait IntoHeaderPair.

The easiest migration is simply to use insert_header((K, V)), noting the tuple which implements the new trait for all previously accepted types.

[fakeshadow]

LGTM

[jplatte]

I don't really like .insert_header((name, value)) but I guess it's better than having separate methods for typed and untyped headers and having to come up with distinct names for them. The only other solution I could think of is having the same method on two different traits and users importing the one they prefer (I doubt many people are using both typed and untyped headers in the same module).

Also, I see actix-http has its own typed headers; do these predate the headers crate or is there some other reason they are separate?

[robjtede]

@jplatte

I don't really like .insert_header((name, value)) but

In my view, an increased focus on typed headers alleviates this enough to make it worth doing. In most cases you're adding common headers to responses, if we don't have the header someone's after we can add it. Plus typed headers are theoretically cheaper than (string, V) since the HeaderName conversion is free. Do you agree that this trade off becomes more worth it when typed headers are used more liberally?

You've pointed out the primary issue with the trait approach; we already have folks getting confused when trying to use extensions_mut for example.

There's some foreign trait rules things to workaround to use an external crate. It's come up and we're looking into the viability of it. Either way this PR would be largely similar.

[jplatte]

In my view, an increased focus on typed headers alleviates this enough to make it worth doing. In most cases you're adding common headers to responses, if we don't have the header someone's after we can add it. Plus typed headers are theoretically cheaper than (string, V) since the HeaderName conversion is free. Do you agree that this trade off becomes more worth it when typed headers are used more liberally?

Yeah, I agree. Still it will be some annoying churn that has to be handled as part of transitioning to actix-web 4.0. Maybe you should consider having these new methods before the next major version and deprecate the old ones so upgrading can be done in independent steps. If it's too much work to backport these, maybe you could keep the old ones around as deprecated for the next major version.

[robjtede]

I'll have a think about keeping old ones, deprecated, to make migration easier. Maybe just on ResponseBuilder where most churn would occur.

[therealprof]

So much more ergonomic. I like it.

[robjtede]

I've added back the response builder methods set_header and header as deprecated.

[omid]

Sorry for this false alarm, it's fixed by adding to Cargo.toml:

[patch.crates-io]
actix-http = { path = "../actix-web/actix-http" }
awc = { path = "../actix-web/awc" }