Change ForceSSLHandler#secure? to use == instead of regex #1662
Conversation
|
Putting this into draft because, while the tests pass, the private method is not exactly the same. I'm going to run benchmarks and base my next approach based on those benchmarks. |
There was a problem hiding this comment.
I think it will be fine to do ==. Looking at rails, they delegate to Rack::Request#ssl? which if you look through the code branches does the same thing https://github.com/rack/rack/blob/a61c0b4fe7162df76cee617922c332b9b138d425/lib/rack/request.rb#L366-L368
|
Cool, asserting equality is faster and easier anyway Unless @env is auto-downcased. |
src/lucky/force_ssl_handler.cr
Outdated
| @@ -48,7 +48,7 @@ class Lucky::ForceSSLHandler | |||
| end | |||
|
|
|||
| private def secure?(context) : Bool | |||
| !!(context.request.headers["X-Forwarded-Proto"]? =~ /https/i) | |||
| context.request.headers["X-Forwarded-Proto"]?.try &.downcase == "https" | |||
There was a problem hiding this comment.
Do we need to downcase this?
It seems like other frameworks don't
I think it's like a 60x increase or so from the original regex if we can drop the case insensitivity altogether
There was a problem hiding this comment.
Yep, i believe it's fine to remove the downcase 👍
There was a problem hiding this comment.
Looks like HTTP::Headers already uses ==
There was a problem hiding this comment.
oh wait.. that's just comparing two headers, and not their value... Ignore me, I'm still waking up lol
|
The benchmarks are hard to read because you have both the match and mismatch tests in the same group. Can you separate out the two match runs from the mismatch runs so that the |
|
Re downcasing, I know http header names are supposed to be case insensitive but I don't think I know that about the values. The Rack tests only seem to test that x-forwarded-proto works correctly when it's all lowercase. The internet wisdom about configuring haproxy seems to have decided on using a lowercase I think it's fine to remove the downcase. |
It's more clear and 52x faster in majority of scenarios (matches) and 24x faster on redirects (mismatches) It also does remove case insensitive searching, but it discussion on the PR luckyframework#1662 stated this should be okay functionality to change Benchmark: ```crystal require "benchmark" puts "Matches" Benchmark.ips do |x| x.report("String ==") do "https" == "https" end x.report("!! String =~ /https/i") do !!("https" =~ /https/i) end end puts "\n\nMismatches" Benchmark.ips do |x| x.report("String == mismatch") do "http" == "https" end x.report("!! String =~ /https/i mismatch") do !!("http" =~ /https/i) end end ``` ```plaintext ➜ tmp crystal build --release downcase-includes-vs-regex.cr && ./downcase-includes-vs-regex Matches String == 897.53M ( 1.11ns) (± 6.76%) 0.0B/op fastest !! String =~ /https/i 17.13M ( 58.36ns) (±16.32%) 16.0B/op 52.38× slower Mismatches String == mismatch 881.32M ( 1.13ns) (± 8.78%) 0.0B/op fastest !! String =~ /https/i mismatch 36.42M ( 27.46ns) (± 1.47%) 16.0B/op 24.20× slower ```
grepsedawk
force-pushed
the
34-ns-is-34-ns
branch
from
95c5a1c
to
b0ae752
Compare
grepsedawk
changed the title
It's more clear and way faster
Latest benchmark in commit message