search

puniverse / comsat

Fibers and actors for web development
docs.paralleluniverse.co/comsat
Other
598 stars 103 forks source link

HttpMessage header name case normalization #66

Closed roded closed 8 years ago

roded commented 8 years ago

https://github.com/puniverse/comsat/issues/58 Not entirely happy with this. A case-insensitive data structure to keep the headers would have been preferable. Though, a case-insensitive immutable multimap (probably backed by a TreeMap) seems a bit too much. Also, I wasn't sure how important it is to keep the interface unchanged; specifically the getHeaders() return type. Comments welcome.

circlespainter commented 8 years ago

Pre-1.0 interfaces can be changed if there's a compelling reason to but I think ListMultiMap represents HTTP headers well: the values order is relevant and there can even be duplicates. This means that a set-based multi-map (like TreeMap) is not an option and unfortunately a new list-based implementation using comparators would be needed (Guava 19 doesn’t seem to include a public one and whatever the implementation the additional overhead should be evaluated as well).

Overall I think it doesn't pay out though, because the previous behavior that “what you put is what you get back” just can’t be enforced: different key casings would just collapse into a single one and at most one would be preserved. With case normalization we don’t respect that previous behavior even in the (maybe, somewhat) common “one key, one value” situation but that's not a big loss compared to the code simplification and near-0-overhead we get with your approach.

As for the behavior change I think that documenting the case normalization in JavaDocs and citing the behavior change in in relnotes will be enough. Could you add a mention in the interface JavaDocs about the key case (which is case-insensitive according to HTTP) not being necessarily preserved, and in the implementations about the lowerCase strategy? Then I'll merge.

Thanks!

roded commented 8 years ago

Updated. What commenting did you mean exactly in the implementations? And where's the relnotes?

circlespainter commented 8 years ago

Not really needed, it's fine as it is. We'll build the release notes upon release and cite the change.

circlespainter commented 8 years ago

One last thing: could you base the PR on the release branch instead? We usually commit fixes there and since master has diverged with new features it can be troublesome for me to pick it manually. I'll then merge into master.