elli-lib / elli

Simple, robust and performant Erlang web server
https://github.com/elli-lib/elli/blob/develop/doc/README.md
MIT License
314 stars 37 forks source link

treat headers as case-insensitive, as they should be #92

Closed tsloughter closed 3 years ago

tsloughter commented 3 years ago

I don't really like the name "parsed", maybe better if I redo the naming to be headers and original_headers? Assuming we need to keep the headers that haven't been casefold'ed?

coveralls commented 3 years ago

Coverage Status

Coverage increased (+0.1%) to 75.676% when pulling eea39dc309fa753fca24564c23c2cb4616fe7439 on tsloughter:case-insensitive-headers into 581ca7ad81b198ab5dcc30a81ac075ba30baced5 on elli-lib:main.

tsloughter commented 3 years ago

Fails on 19 and 18.

Do we need to support those or is 23, 22, 21, and 20 enough?

paulo-ferraz-oliveira commented 3 years ago

I'd support 19.3. What's failing?

paulo-ferraz-oliveira commented 3 years ago

redo the naming to be headers and original_headers

Won't this break interface for developers relying on name headers, or is this opaque? I'd prefer not to break interface, but it's ultimately not my call.

tsloughter commented 3 years ago

@paulo-ferraz-oliveira just internal and not different from the API in this PR.

But note that this will need to be elli 4.0 anyway, since get_header now returns different results than before.

paulo-ferraz-oliveira commented 3 years ago

@tsloughter, sure. I'm OK with breaking stuff when it's justifiable :) I just thought headers wouldn't be, but since it's internal it's no prob.

paulo-ferraz-oliveira commented 3 years ago

How about support for 19, though? We'll surely move to 20 or 21 when rebar3 does, but that might still take a while.

yurrriq commented 3 years ago

:+1: to headers and original_headers. I should be able to review properly, and maybe fix the OTP 19 issue tomorrow.

paulo-ferraz-oliveira commented 3 years ago

fix the OTP 19 issue tomorrow

This would be much appreciated. 👍

tsloughter commented 3 years ago

Supporting 19 is pretty bad.. forgot string:lowercase won't take a binary in 19. I've updated the PR now to convert the binary to a list and then back to a binary, but that is obviously inefficient. We can keep 19 if people find that really important but I'd argue supporting 4 major versions is adequate and 19 is worth dropping.

paulo-ferraz-oliveira commented 3 years ago

Supporting 19 is pretty bad.. forgot string:lowercase won't take a binary in 19. I've updated the PR now to convert the binary to a list and then back to a binary, but that is obviously inefficient. We can keep 19 if people find that really important but I'd argue supporting 4 major versions is adequate and 19 is worth dropping.

Hm... I don't actually mind you dropping it. It just means we'll get stuck with the latest (as-is = 3.3.0) for a while longer, but we've not experienced issues with it, either, so that might be fine.

tsloughter commented 3 years ago

@paulo-ferraz-oliveira ah, ok. I'd like to hear other's thoughts but I'd be fine with doing any future backports to the 3.3.0 if there are fixes we get into 4.x later that you need and are still stuck on 19.0.

yurrriq commented 3 years ago

future backports to the 3.3.0

That sounds good to me. I wonder how many users are still on 19. Does hex.pm get any useful metadata from clients?

tsloughter commented 3 years ago

That would be useful, but I don't think Hex does.

tsloughter commented 3 years ago

Removed support for 19.

codecov-io commented 3 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (main@fbec0ce). Click here to learn what that means. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #92   +/-   ##
=======================================
  Coverage        ?   75.42%           
=======================================
  Files           ?       12           
  Lines           ?      765           
  Branches        ?        0           
=======================================
  Hits            ?      577           
  Misses          ?      188           
  Partials        ?        0           

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 fbec0ce...f42e56f. Read the comment docs.