Look up headers in a case insensitive way #199

martina-if · 2017-04-18T12:31:47Z

In this first step apollo will look up headers in a case insensitive way but it will still preserve the original letter case of header fields when propagated. This makes services compatible with services that send lowercase headers (like those who propagate headers from an HTTP 2 connection).

I recommend looking at the commits separately.

@pettermahlen @rouzwawi

codecov-io · 2017-04-18T12:40:00Z

Codecov Report

Merging #199 into 1.x will decrease coverage by 0.05%.
The diff coverage is 70.68%.

@@             Coverage Diff              @@
##                1.x     #199      +/-   ##
============================================
- Coverage     69.57%   69.51%   -0.06%     
- Complexity        0      651     +651     
============================================
  Files           138      139       +1     
  Lines          2879     2916      +37     
  Branches        226      172      -54     
============================================
+ Hits           2003     2027      +24     
- Misses          833      846      +13     
  Partials         43       43

Impacted Files	Coverage Δ	Complexity Δ
...api/src/main/java/com/spotify/apollo/Response.java	`20% <0%> (-30%)`	`2 <0> (+2)`
...-api/src/main/java/com/spotify/apollo/Request.java	`35.71% <0%> (-30.96%)`	`3 <0> (+3)`
...ava/com/spotify/apollo/http/client/HttpClient.java	`93.1% <100%> (ø)`	`9 <1> (+9)`	⬆️
...-api/src/main/java/com/spotify/apollo/Headers.java	`100% <100%> (ø)`	`11 <11> (?)`
...apollo/http/server/AsyncContextOngoingRequest.java	`85.29% <50%> (-2.59%)`	`9 <0> (+9)`
...com/spotify/apollo/test/unit/ResponseMatchers.java	`41.46% <60%> (ø)`	`6 <0> (+6)`	⬆️
...src/main/java/com/spotify/apollo/ResponseImpl.java	`81.81% <83.33%> (+1.81%)`	`11 <3> (+11)`	⬆️
...src/main/java/com/spotify/apollo/RequestValue.java	`94.44% <90.9%> (+0.69%)`	`14 <9> (+14)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d9e653...2975a92. Read the comment docs.

The headers are still sorted by insertion order and preserve their original letter casing when read.

martina-if · 2017-04-20T16:41:32Z

Maybe it's not worth having Headers being an AutoValue type, what do you think?

pettermahlen

Some questions/comments. I think it's best to leave Headers as an AutoValue, it saves some implementation+conceptual overhead wrt especially equality and such.

pettermahlen · 2017-04-21T06:46:31Z

apollo-api/src/main/java/com/spotify/apollo/Headers.java

+  }
+
+  public Optional<String> get(String name) {
+    Preconditions.checkArgument(name != null, "Header names cannot be null");


I think it's better to use Objects.requireNonNull in this case - generally null arguments lead to NPEs, and it's better to use the JDK classes than Guava where feasible.

Thanks! Fixed

pettermahlen · 2017-04-21T06:48:39Z

apollo-api/src/main/java/com/spotify/apollo/Request.java

   */
  default Optional<String> header(String name) {
-    return Optional.ofNullable(headers().get(name));
+    Preconditions.checkArgument(name != null, "Header names cannot be null");


Would it be better to permit nulls here, and reverse the comparison below so that it would just fail to find something, rather than throw an exception for a null? I'm not sure. Either way, if nulls shouldn't be permitted, then I think we should use Objects.requireNonNull for the argument check.

Talked offline about this. I will leave the check with Objects.requireNonNull

pettermahlen · 2017-04-21T06:51:23Z

apollo-api/src/main/java/com/spotify/apollo/Response.java

  Map<String, String> headers();

  /**
+   * The response headers
+   */
+  Iterable<Map.Entry<String, String>> headerEntries();


Why Iterable here? Since we're spending some effort on retaining order of headers, would it not make more sense to return a List?

I was trying to be conservative and restrictive, conveying the message that this is only for iterating over all the headers. I want to prevent users from looking up keys from whatever object this method returns. Also, I didn't want to lock a specific implementation (we could change the internal implementation for a Map, etc). But I see now that I chose the wrong interface and it should have been Iterator, but maybe it's too annoying, specially since it does not have size(). What do you think would be better: Iterator or List?

Iterator is generally wrong, since they are one-use only. Iterable is better in that case.

I would still prefer List; it's still quite generic, and I feel like we have committed to retaining order. We could even specify that in the javadocs.

Fair enough. Changed to List.

pettermahlen · 2017-04-21T06:51:42Z

apollo-api/src/main/java/com/spotify/apollo/Response.java

@@ -119,4 +144,5 @@
  static <T> Response<T> of(StatusType statusCode, T payload) {
    return ResponseImpl.create(statusCode, payload);
  }
+


Probably remove this empty line

martina-if · 2017-04-21T08:24:18Z

apollo-api/src/main/java/com/spotify/apollo/Headers.java

+    return Optional.empty();
+  }
+
+  public Map<String, String> asMap() {


This is used a lot in apollo. Should I memoized it already in the constructor?

Not sure; doing so would increase the memory footprint - you'd need some measurements to be sure, I guess.

I'll leave the optimizations for later then.

kouKatsumi · 2017-04-24T11:27:49Z

apollo-api/src/main/java/com/spotify/apollo/Headers.java

+
+  public abstract ImmutableList<Map.Entry<String, String>> entries();
+
+  private static void insertIfAbsent(List<Map.Entry<String, String>> headerList,


I'm confused here. Method name suggest that the value would not be overwritten (as it will be entered only if it's absent), but that's what the method does?

Right. I fixed the name to insertOrReplace(). Is this better?

kouKatsumi · 2017-04-24T13:15:59Z

apollo-api/src/test/java/com/spotify/apollo/HeadersTest.java

+  private static final Map<String, String> TEST_BIG_MAP = createBigMap();
+
+  private static Map<String, String> createDuplicateKeysMap() {
+    LinkedHashMap<String, String> testMap = new LinkedHashMap<>(3);


Maybe declare test map asMap instead of LinkedHashMap ?

kouKatsumi · 2017-04-24T13:30:42Z

apollo-api/src/test/java/com/spotify/apollo/ResponseTest.java

+  }
+
+  @Test
+  public void shouldReturnHeaderCaseInsensitive() throws Exception {


Do we want to test it again as we already do in HeadersTest.java?

Yes, I think the test should be implementation agnostic. At least the most important parts.

martina-if added 2 commits April 20, 2017 18:27

Make header lookups case insensitive in Request/Response

1681da2

The headers are still sorted by insertion order and preserve their original letter casing when read.

Stop using deprecated method headers()

1dbf28c

martina-if force-pushed the case-insensitive-headers branch from 2352198 to 1dbf28c Compare April 20, 2017 16:29

pettermahlen suggested changes Apr 21, 2017

View reviewed changes

martina-if commented Apr 21, 2017

View reviewed changes

kouKatsumi reviewed Apr 24, 2017

View reviewed changes

Address PR comments

2975a92

pettermahlen approved these changes Apr 25, 2017

View reviewed changes

martina-if merged commit ba73aa9 into spotify:1.x Apr 25, 2017

pettermahlen mentioned this pull request Oct 27, 2017

upgrade to okhttp3 #224

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Look up headers in a case insensitive way #199

Look up headers in a case insensitive way #199

martina-if commented Apr 18, 2017 •

edited

codecov-io commented Apr 18, 2017 •

edited

martina-if commented Apr 20, 2017

pettermahlen left a comment

pettermahlen Apr 21, 2017

martina-if Apr 24, 2017

pettermahlen Apr 21, 2017

martina-if Apr 24, 2017

pettermahlen Apr 21, 2017

martina-if Apr 21, 2017

pettermahlen Apr 21, 2017

martina-if Apr 24, 2017

pettermahlen Apr 21, 2017

martina-if Apr 24, 2017

martina-if Apr 21, 2017

pettermahlen Apr 21, 2017

martina-if Apr 24, 2017

kouKatsumi Apr 24, 2017

martina-if Apr 24, 2017

kouKatsumi Apr 24, 2017

martina-if Apr 24, 2017

kouKatsumi Apr 24, 2017

martina-if Apr 24, 2017


		public abstract ImmutableList<Map.Entry<String, String>> entries();

		private static void insertIfAbsent(List<Map.Entry<String, String>> headerList,

Look up headers in a case insensitive way #199

Look up headers in a case insensitive way #199

Conversation

martina-if commented Apr 18, 2017 • edited

codecov-io commented Apr 18, 2017 • edited

Codecov Report

martina-if commented Apr 20, 2017

pettermahlen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martina-if commented Apr 18, 2017 •

edited

codecov-io commented Apr 18, 2017 •

edited