search

luckyframework / lucky

A full-featured Crystal web framework that catches bugs for you, runs incredibly fast, and helps you write code that lasts.
https://luckyframework.org
MIT License
2.59k stars 156 forks source link

Change ForceSSLHandler#secure? to use == instead of regex #1662

Closed grepsedawk closed 2 years ago

grepsedawk commented 2 years ago

It's more clear and way faster

Latest benchmark in commit message

grepsedawk commented 2 years ago

Putting this into draft because, while the tests pass, the private method is not exactly the same. The private method previous was a "#includes?" via regex not ==.

I'm going to run benchmarks and base my next approach based on those benchmarks.

grepsedawk commented 2 years ago

Cool, asserting equality is faster and easier anyway Rack also doesn't seem to downcase the header ever, it boils down to calling: https://github.com/rack/rack/blob/a61c0b4fe7162df76cee617922c332b9b138d425/lib/rack/request.rb#L79 via https://github.com/rack/rack/blob/a61c0b4fe7162df76cee617922c332b9b138d425/lib/rack/request.rb#L643

Unless @env is auto-downcased.

robacarp commented 2 years ago

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 fastest and nn x slower stuff is meaningful?

robacarp commented 2 years ago

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 https but there's no reason for that to be necessarily true.

I think it's fine to remove the downcase.