[Breaking Change Request] HttpHeaders allows cased header fields

[zichangg]

The intended change in behavior

Current implementation of HttpHeaders will always force all header fields to be lower-cased. An optional named parameters will be added to following methods to loosen this restriction.

Methods in abstract Class HttpHeaders will be changed:

void add(String name, Object value);
// change to 
void add(String name, Object value, {bool preserveHeaderCase: false});

add will be functionally the same as before. What's new?

Header names will be always converted to lower-case unless preserveHeaderCase is set to true. If two header names are the same when converted to lower-case, they are considered to be the same header, with one set of values. The current case of a header is the name assigned by the last set or add call for that header.

void set(String name, Object value);
// change to 
void set(String name, Object value, {bool preserveHeaderCase: false});

set will remove the entity that has the same lower-cased header field and perform an add.

The implementation class _HttpHeaders in dart:io will be modified accordingly.

The justification

Header fields of HttpHeaders are case-insensitive according to specification. Implementation class _HttpHeaders will convert all header fields into lowercases by default. This is expected behavior. However, some servers do rely on cased header fields.

Relative issues:
#33501
#25120

Expected impact

Code that has classes extends/implements the HttpHeaders will now get compile time error.

Steps for mitigation

Code that doesn't have classes extends/implements abstract HttpHeaders will not need to make any change. Otherwise, update add() and set() as showed above.

Proposed implementation

https://dart-review.googlesource.com/c/sdk/+/119100

@lrhn @mit-mit @a-siva

[zichangg]

@sortie

[mit-mit]

cc @Hixie @matanlurey @dgrove any concerns?

[cdm2012]

Hey guys (@sstrickl & @leafpetersen), I saw that you worked on a previous breaking change request. I was wondering if there could be a suggestion on how to get this cross platform fix accepted.

I'm currently trying to use Flutter with my Nginx/Laravel installations, and I am having issues with the server not accepting my headers (please see other comment from last week for details).

Is it possible that the intent is that the Dart SDK only allows lowercase http headers @aartbik?

[Hixie]

I guess I'm fine with this but we should document that if this affects server behaviour it is a standards violation. Maybe call the argument "supportBrokenServersByPreservingCase" or some such.

[shaxxx]

@Hixie One can say that Dart's HttpClient also breaks the standard.
If standard says that headers are case insensitive why force it to lowercase?
Any developer familiar with the fact that headers are case insensitive will expect it to be passed AS IS from HttpClient. There is no single reason to mangle with headers in the first place.

I believe this change is going in the wrong direction to begin with. Instead of making breaking changes why not refactor HttpClient internals so anyone can overload/re-implement parts they want to change. This would allow people to relatively easy change default behavior and would make everyone happy.

[seblaa]

Perhaps an immediate workaround

I guess I'm fine with this but we should document that if this affects server behaviour it is a standards violation. Maybe call the argument "supportBrokenServersByPreservingCase" or some such.

Forcing lowercase is also, by definition, breaking the standard. At the very least, allow the user to implement on override. I am in a position where an API, outside of our control, is rejecting the call due to this.

[zichangg]

A friendly ping. Is there any other concern except argument naming?

[seblaa]

A friendly ping. Is there any other concern except argument naming?

Just a note below on the comment from @Hixie

I guess I'm fine with this but we should document that if this affects server behaviour it is a standards violation. Maybe call the argument "supportBrokenServersByPreservingCase" or some such.

There ideally should not be a name that is indicative of calling out "support ... broken" or anything like that. Since the standard does not impose case, any party could implement a solution where it is forced server-side and fails a request (unfortunately) as an indirect consequence of an internal byproduct, misconfig, or legacy solution. In Ngenix, this can be controlled for example.

Something simple and clear such as "lowercaseHeaderOverride" - which simply and succinctly states what's being done.

@zichangg it would be great to have this implemented as soon as possible. Perhaps even considered a "hotfix"? As the impact is zero, since you have to explicitly override to enable - and forcing lowercase kills many API calls (there is a third-party package that does this, which is unnecessary).

[mit-mit]

I suggest we name the argument nonStandardHeaderCasePreservation. @Hixie would that address your concern? @seblaa what do you think?

[seblaa]

@mit-mit That would work, sure. Descriptive and need to set it to invoke, perfect.

[Hixie]

@mit-mit nonStandardHeaderCasePreservation is ok. It's not entirely accurate (the non-standardness is not the case preservation, it's just that this is a workaround for broken servers).

@seblaa

Since the standard does not impose case, any party could implement a solution where it is forced server-side and fails a request (unfortunately) as an indirect consequence of an internal byproduct, misconfig, or legacy solution.

I don't understand. Per the standard, there can be no difference in behavior whether the header is uppercase or lowercase.

Forcing lowercase is also, by definition, breaking the standard.

By what definition? There's nothing in the HTTP specification that says a client API can't lowercase the headers. It makes no difference, per HTTP.

I am in a position where an API, outside of our control, is rejecting the call due to this.

That's why we're considering a workaround for broken servers. It doesn't mean that the server isn't broken though.

@shaxxx

One can say that Dart's HttpClient also breaks the standard.

Why?

[shaxxx]

@Hixie by the same logic that using uppercase is breaking the standard.

[Hixie]

Using uppercase isn't breaking the standard. What the standard says is that the server must act the same whether you use uppercase or lowercase or a mixture:

4.2 Message Headers

   HTTP header fields, which include general-header (section 4.5),
   request-header (section 5.3), response-header (section 6.2), and
   entity-header (section 7.1) fields, follow the same generic format as
   that given in Section 3.1 of RFC 822 [9]. Each header field consists
   of a name followed by a colon (":") and the field value. Field names
   are case-insensitive.

-- https://tools.ietf.org/html/rfc2616#section-4.2

[mit-mit]

This breaking change is approved, with the argument name nonStandardHeaderCasePreservation

[sortie]

I prefer preserveHeaderCase (which we settled on in the code review) because:

1. It describes what it does and it's much shorter than than nonStandardHeaderCasePreservation.
2. As Hixie points out, the standard allow us to send whatever case, meaning that whatever we do is allowed by the standard, but the name suggests it's a weird non-standard mode (which it isn't).

[zichangg]

Agree with what sortie suggests.
A short and descriptive name is more user-friendly in my opinion and nonStandard doesn't help to explain what this argument is doing.
@mit-mit @Hixie What do you think?

[seblaa]

@zichangg @mit-mit @Hixie @shaxxx @sortie

To be blunt, there should be no override needed. This was an unfortunate implementation, since casing does not belong at this level. It should be a holistic approach at the framework level and allow the implementer to decide if it is required between endpoints. That would be the true definition of carrying forward the case insensitive "source of truth" from the spec, and allow the developer to decide if and when it matters.

The name of the parameter is inconsequential in the overall picture if that is the direction. Just have it short and descriptive.

EDIT: As the casing is irrelevant, there should be no issue in removing the current lowercase force correct? That would be more in line with the spec and remove unnecessary workarounds. Simplest approach might be the best here.

[Hixie]

@sortie I don't have strong opinions on the name, so long as we document that if this affects server behaviour it is a standards violation on the part of the server. If it was me I'd call it something like "supportBrokenServersByPreservingHeaderCase".

[shaxxx]

What's the current status of this? Are you still discussing the name of optional parameter or something else?

[a-siva]

I beleive @zichangg is in the process of landing the change.

[zichangg]

I'm working on places that affected by this breaking change. I need to make sure landing this change won't break testing bots. After this, there should be nothing blocking.

[mit-mit]

@zichangg may we have an update on when this might land, please?

[zichangg]

It landed! It should be in next stable release if there is no more regression.

The current status is that this change has not been merged into internal codebase, because rolling is dead for a couple of day.

[mit-mit]

Thanks; closing this issue to reflect that this is in master. CL was d39cdf0

[seblaa]

@zichangg perfect, thanks for working this in quickly!