Make use of helper methods in StaticFileHandler
#714
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
straight-shoota
added a commit
to crystal-lang/crystal
that referenced
this pull request
May 10, 2025
`StaticFileHandler#call` was a complex method with > 100 loc. This patch extracts individual aspects into helper methods, which gives it more structure. This also enables inheriting types to inject partial custom behaviour by overriding individual helper methods (see kemalcr/kemal#714 for an example).
syeopite
reviewed
May 17, 2025
src/kemal/static_file_handler.cr
Outdated
| # NOTE: This override opts out of some behaviour from HTTP::StaticFileHandler, | ||
| # such as serving content ranges. | ||
| private def serve_file(context : Server::Context, file_info, file_path : Path, last_modified : Time) | ||
| send_file(context, file_path.to_s) |
There was a problem hiding this comment.
There is quite a lot of duplicated logic between what send_file does verses what is being overridden. Since the primary difference is compression do you think it would make sense to do something like this instead?
private def serve_file(context : Server::Context, file_info, file_path : Path, last_modified : Time)
env.response.headers["X-Content-Type-Options"] = "nosniff"
config = Kemal.config
condition = config.is_a?(Hash) && config["gzip"]? == true && file_info.size > 860 && Kemal::Utils.zip_types(file_path.to_s)
if condition && context.request.headers.includes_word?("Accept-Encoding", "gzip")
context.response.headers["Content-Encoding"] = "gzip"
context.response.output = Compress::Gzip::Writer.new(context.response.output, sync_close: true)
elsif condition && context.request.headers.includes_word?("Accept-Encoding", "deflate")
context.response.headers["Content-Encoding"] = "deflate"
context.response.output = Compress::Deflate::Writer.new(context.response.output, sync_close: true)
end
super
endThere was a problem hiding this comment.
Probably, yes. But I'd like to keep the refactoring of StaticFileHandler as a separate change, and address serve_file in a follow-up.
straight-shoota
commented
May 27, 2025
| @@ -1,88 +1,120 @@ | |||
| module Kemal | |||
| class StaticFileHandler < HTTP::StaticFileHandler | |||
| # ameba:disable Metrics/CyclomaticComplexity | |||
There was a problem hiding this comment.
note: I'm not sure why this needs to be removed. But ameba told me to 🤷
The #call def didn't change its complexity.
hugopl
reviewed
May 28, 2025
| else | ||
| if @fallthrough | ||
| call_next(context) | ||
| {% if compare_versions(Crystal::VERSION, "1.17.0") >= 0 %} |
There was a problem hiding this comment.
If we want to support old versions, better add crystal 1.16.0 to the CI matrix.
sdogruyol
approved these changes
Jun 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch simplifies the implementation of
StaticFileHandlerby inheriting most behaviour from the stdlib implementation. We only need to override the parts where Kemal provides custom behaviour.Following the upstream implementation more closely avoids missing out on improvements (such as the fix for #710 would have been available for a long time).
This requires a
HTTP::StaticFileHandlerwith patch crystal-lang/crystal#15678