Fix HTTP request builder setHeader case sensitivity

[findepi]

No description provided.

[brybacki]

good catch!
Does addHeader and addHeaders behave correctly?

[findepi]

add are supposed to accumulate

[brybacki]

Right, it accumulates so not a problem, but there is still a difference and results in names not normalized. But maybe deduplication and normalization is a responsibility of a different class.

So it depends if we want:

ListMultimap<String, String> headers = ArrayListMultimap.create();
        headers.put("Key-One", "v1");
        headers.put("Key-One", "v1");
        headers.put("Key-One", "v2");

        headers.put("key-one", "v1");
        
        System.out.println(headers);

To result in:

{Key-One=[v1, v1, v2], key-one=[v1]}

or in:

{key-one=[v1, v1, v1, v2]}

[wendigo]

Custom multimap does that:

ListMultimap<String, String> headers = Multimaps.newListMultimap(new TreeMap<>(CASE_INSENSITIVE_ORDER), ArrayList::new);
headers.put("Key-One", "v1");
headers.put("Key-One", "v1");
headers.put("Key-One", "v2");
headers.put("key-one", "v1");
System.out.println(headers);

{Key-One=[v1, v1, v2, v1]}

[findepi]

added lowercasing and a test

[brybacki]

Error:    TestBasicAuthRequestFilter.testBasicAuthentication:39 expected [Basic QWxhZGRpbjpvcGVuIHNlc2FtZQ==] but found [null]
Error:    TestTraceTokenRequestFilter.testBasic:46 lists don't have the same size expected [1] but found [0]

[findepi]

replaced lowercasing with case-preserving, per @losipiuk suggestion

[findepi]

the code is not simplest possible, because it preserves the case of headers. the simplest would be to just lowercase.
preserving header case is not important from http spec perspective, but we've seen cases where not preservice headers breaks some buggy applications

cc @losipiuk @electrum @wendigo

[wendigo]

Alternative #1166

[findepi]

With lowercasing, apps using airlift may need to change too. For example https://github.com/trinodb/trino/blob/a27e5af2d7389a7bc0e906210eafb906f01e4365/core/trino-main/src/test/java/io/trino/operator/MockExchangeRequestProcessor.java#L103

[electrum]

We should probably make Request.getHeaders() return a case-insensitive map

[findepi]

happy to apply once we know for sure (without "probably"). there was quite a bit back and forth already on this PR. It doesn't cost much because it is small, but it's not cool nonetheless

@electrum @martint can you explicitly decide on the behavior you want to see implemented? happy to apply whatever is needed

[findepi]

added lowercasing again.

ptal

[findepi]

@electrum @wendigo @losipiuk can you please review?

[losipiuk]

Non preserving hurts my aesthetic soul but I hope I can live with that.

[martint]

We shouldn’t lower case header names. What problem are we trying to solve?

[findepi]

What problem are we trying to solve?

setHeader was incorrectly replacing headers only if they were given in exact same case. See tests.

Of course the simplest implementation would be to just change setHeader:

--- http-client/src/main/java/io/airlift/http/client/Request.java
+++ http-client/src/main/java/io/airlift/http/client/Request.java
@@ -232,7 +232,7 @@ public final class Request
 
         public Builder setHeader(String name, String value)
         {
-            this.headers.removeAll(name);
+            this.headers.keySet().removeIf(key -> key.equalsIgnoreCase(name));
             this.headers.put(name, value);
             return this;
         }

in fact this was the first version of this PR.
However i realized that keeping io.airlift.http.client.Request.Builder#headers as ListMultimap would no longer make sense, if we always search it linearly.

then the PR had case-preserving version (see "before" in this diff https://github.com/airlift/airlift/compare/257a0490937b4f1e23ddbd44e0bf47612c81704a..08e57c46f089d59d97cf29481e2b2d69edcce336)

Then @electrum suggested case-preserving isn't worth it.

BTW once headers gets thru airlift request builder, their case is later changed in jetty client. It looks like the case normalization in the jetty client happens for all headers registered with org.eclipse.jetty.http.HttpHeader enum.

We shouldn’t lower case header names.

Why?

[findepi]

@martint @electrum hope you can converge on what this should do (lowercase vs case-preserving vs something else).
Happy to implement whatever we want to do. If this helps, i can create separate PRs for every option considered, so that we don't need to use this PR push history.

[wendigo]

Jersey is using follow implementation: https://github.com/eclipse-ee4j/jersey/blob/14c5c43ff227ba34bc27aa41c81f84d5f1f3080d/core-common/src/main/java/org/glassfish/jersey/internal/util/collection/StringKeyIgnoreCaseMultivaluedMap.java#L28

[electrum]

Given the HTTP/2 is always lowercase, it's not clear that there is value in preserving header case. This seems like a bad assumption in applications that can appear to work, but will break depending on the protocol version used at runtime. Lowercasing headers makes code simpler and avoids the surprise.

[findepi]

Hopefully we have agreement that the current behavior of setHeader is wrong. It caused an incident internally at Starburst.

@electrum @martint how would you like the code to behave?

• preserve case vs lower-case in the builder?
• getHeader to stay as is OR match case-insensitively in request getHeader()
• case-sensitive OR case-insensitive map keys in request getHeaders(), OR something else (like removing this method altogether)
  • case-insensitive has the problem similar to identity-based collections -- taking defensive immutable copy changes semantics; so i'd actually prefer to have dedicated wrapper class to represent header name (either lowercasing or preserving case but comparing case insensitively; depending on the answer to the first question)