TypedHeader shouldn't reject missing headers by default

[RReverser]

• I have looked for existing issues (including closed) about this

Feature Request

TypedHeader should allow specific Header type to handle missing headers instead of immediately rejecting the request.

Motivation

Header trait already allows to decode headers from arbitrary iterators, including iterators with zero, one or more header value repetitions.

Some Header implementations require their header value to be always present, in which case Header::decode will return an error anyway, while many other headers might be optional by design, in which case their Header::decode will correctly handle empty iterator and return some default parsed value (e.g. empty collection).

However, right now axum's TypedHeader checks for header name's presence in the request and, if it's not there, immediately rejects the request, not giving those Header::decode implementation a chance to gracefully handle the empty case.

Proposal

Forward empty iterator to the <H as Header>::decode and use its result instead of immediately rejecting the request. This will correctly handle both mandatory headers (as those will still return an error) as well as optional ones.

Alternatives

If this is considered undesirable for some reason, another alternative could be to implement a custom extractor for Option<TypedHeader<H>>. It won't be ideal as the user will have to manually invoke H::decode again to get the correct default value, but might be less of a breaking change.

[jplatte]

Seems reasonable!

[davidpdrsn]

axum just calls typed_try_get. Could the issue be there?

[jplatte]

Seems like it, that optimization (?) of returning Ok(None) when the size hint is 0 seems wrong according to the original issue description.

[RReverser]

Traced it down through git blame to hyperium/headers@1150ca8.

Looks like the original signatures of those extension methods were returning Option<H>, and when decode was changed to return not Option<H> but the more descriptive Result<H, Error>, then for some reason those helper methods weren't changed in the same way from Option<H> to Result<H, Error>, but to Result<Option<H>, Error>.

Not sure why - maybe a refactoring oversight, or maybe indeed an optimisation, but either way I don't think that Option::None is supposed to be treated as an error when H::decode is perfectly capable of deciding what to do with empty iterators on its own.

[davidpdrsn]

Not sure why - maybe a refactoring oversight, or maybe indeed an optimisation, but either way I don't think that Option::None is supposed to be treated as an error when H::decode is perfectly capable of deciding what to do with empty iterators on its own.

That makes sense. Do you wanna try and make a PR to headers? Or at least an issue?

[RReverser]

Yeah I can, I suppose it would have to a feature request for a new method though since Option<H> to H would be a breaking change.

[davidpdrsn]

cc @seanmonstar

[RReverser]

Created upstream feature request at hyperium/headers#133.

[davidpdrsn]

Should we close this? Or is there something we should do in axum?

[jplatte]

Well, we still need to use the new functionality from the headers crate. Unless it's shipped as a breaking change to the existing function, in which case we still need to bump the dependency and note it in the changelog.

[RReverser]

It should be pretty easy to do this even without waiting for headers crate update FWIW. If that's something you're open to, I'm happy to make a PR.

[davidpdrsn]

Sure! Would it be a breaking change? Or does that depend on the specific Header type you're using?

[RReverser]

Or does that depend on the specific Header type you're using?

Yeah, this. Also worth noting it's breaking in the sense that "something that used to reject the request before now might succeed", which might be potentially problematic in some scenarios where someone was counting on that error, but seems low-risk.

[davidpdrsn]

Right. Let's merge it into the 0.7.0 branch for now then. Hyper 1.0 hopefully isn't too far off anyway.

[RReverser]

Okay. I guess if we can do breaking changes, then I can also remove the TypedHeaderRejectionReason::Missing branch and unwrap that rejection enum. Or do you think it's worth keeping that case as separate?

[davidpdrsn]

Yeah we could make other changes as well, though I'm not quite sure what the implications of that would be. We obviously want to avoid panics.

[RReverser]

Oh yeah I'm just thinking whether it's worth the extra effort to continue reporting missing headers as a special case... I guess let me prototype it in a PR and we can discuss further over there.

[RReverser]

Ok opened a PR with a quick fix. In retrospect, I'm tempted to say this is not a breaking change after all, but rather a bugfix (with the usual caveat that someone might have started relying on a bug, but I don't think that constitutes a breaking change?).

Up to you though.

[FlixCoder]

Hmm, this change now means that
Option<TypedHeader<IfMatch>> now always returns Some, even if the header is not present. This makes it look like there is a IfMatch header wanting to match the empty value. I opened an issue in headers about IfMatch specifically, but I think this is an issue in axum now generally, as there is no way to know whether the header was actually provided. (Except for checking manually via HeaderMap of course)

[jplatte]

FWIW, TypedHeader is moving into axum-extra with the next axum release. Maybe the unclear expectations are a reason to also experiment with different header extractors (though I'm not sure how much better we can do).

[RReverser]

but I think this is an issue in axum now generally, as there is no way to know whether the header was actually provided

Hm yeah I'd say you'd then want Option itself to implement FromRequest differently (check presence first, parse later).

That said, in pretty much all real-world scenarios you shouldn't distinguish between the header's default value and the missing header if the header is specified in such a way that missing header is equivalent to some default value.