Split StaticFileHandler#call into structured components

[straight-shoota]

StaticFileHandler#call is a complex method with > 100 loc. This patch extracts individual aspects into helper methods which gives it more structure.
This also makes it easy for inheriting types to inject partial custom behaviour by overriding individual helper methods (see kemalcr/kemal#714 for an example).

[ysbaddaden]

suggestion: I'd expect a bang method to raise an exception, not return a boolean. Maybe a name such as #allowed_request_path? would be more explicit to the intent? Then we'd have nice guards:

return unless allowed_request_method?(context)
return unless allowed_request_path?(context, request_path)
return unless valid_redirect_to_expanded_path?(context, request_path, expanded_path, file_info)

[straight-shoota]

I don't like that because these methods are not just predicates. They directly affect the behaviour (e.g. the HTTP response) in case of an error.
The purpose of the exclamation mark is to indicate that the method handles errors itself and just returns a boolean that indicates whether the caller should continue or stop execution.

[ysbaddaden]

I'm not fond of the extracted methods still having some implicit expectations and intermixed behavior: validation + action + implicit out-of-scope control-flow. Maybe split again? Granted, it would be much more verbose but the helpers would do one thing.

Note: it could be interesting to wrap it the context + methods into a struct, and then we could have a nice public API! maybe not, a struct would mean it's hard to customize, and a class would mean an allocation (but maybe not a bad idea).

[straight-shoota]

The combination of validation + action forms a self-contained component. The only tricky thing about it is handling control flow in the caller scope.

If we want more granular components, we can split the bodies of the check_ methods into separate helpers, of course. But I don't see much benefit from that.

[straight-shoota]

Would it be better if we pronounce the action more in the method name?

• check_request_method! || return -> respond_on_invalid_request_method && return
• check_request_path! || return -> respond_on_invalid_request_path && return
• check_redirect_to_expanded_path! || return -> redirect_to_expanded_path && return

[ysbaddaden]

Maybe? I still prefer braindead flow-control, and do one thing helpers, but it's prolly just my taste.

[straight-shoota]

Just to be clear, you mean something like this:

unless allowed_request_method?(context)
  return redirect_invalid_request_method(context)
end

unless allowed_request_path?(context, request_path)
  return redirect_invalid_request_path(context, request_path)
end

# EDIT: This has been refactored, see following comment.
# if expanded_path_needs_redirect?(context, request_path, expanded_path, file_info)
#  return redirect_to_normalized_path(context, request_path, expanded_path, file_info)
# end

Especially the latter one would be a bit unfortunate because the redirect needs to check the same conditions again as the validation.
But that's similar for the other as well. The action and condition are strongly connected together. For example, both must consider the same request methods as allowed. If separate methods can be overridden individually, they can get out of sync.

[straight-shoota]

I suppose the path redirect could easily be refactored to this:

if normalized_path = normalize_request_path(context, request_path, expanded_path, file_info)
  return redirect_to context, normalized_path
end

EDIT: Amended in afe80ec

[ysbaddaden]

You may just want to configure the accepted HTTP methods or paths, or overload the checks and call previous_def, but you may also just want to render a custom response, independently from the checks.

[straight-shoota]

issue: I'm not sure what to do about this. It doesn't make sense to set this header always. Some implementations might not support ranges for serving files (Kemal, for example, currently doesn't), and we don't support ranges on directory index pages either.

So this should go somewhere in the serve_file method, maybe in the branch that doesn't handle ranges, i.e. directly above the call to serve_file_full?

[sdogruyol]

Is there anything blocking? This would be a great improvement for Kemal

[straight-shoota]

I don't think so. Waiting for another review.

[Sija]

I believe this PR introduced a regression - gathering from the stacktrace of a build error of Kemal 1.6.0 with Crystal 1.17.0.

In src/server.cr:21:7

 21 | Kemal.run
            ^--
Error: instantiating 'Kemal.run()'


In lib/kemal/src/kemal.cr:16:10

 16 | self.run(nil, args: args, trap_signal: trap_signal)
           ^--
Error: instantiating 'Kemal.run(Nil, args: Array(String), trap_signal: Bool)'


In lib/kemal/src/kemal.cr:11:10

 11 | self.run(port, args, trap_signal) { }
           ^--
Error: instantiating 'Kemal.run(Nil, Array(String), Bool)'


In lib/kemal/src/kemal.cr:65:12

 65 | server.listen unless config.env == "test"
             ^-----
Error: instantiating 'HTTP::Server#listen()'


In /opt/homebrew/Cellar/crystal/1.17.0/share/crystal/src/http/server.cr:465:9

 465 | loop do
       ^---
Error: instantiating 'loop()'


In /opt/homebrew/Cellar/crystal/1.17.0/share/crystal/src/http/server.cr:465:9

 465 | loop do
       ^---
Error: instantiating 'loop()'


In /opt/homebrew/Cellar/crystal/1.17.0/share/crystal/src/http/server.cr:474:13

 474 | dispatch(io)
       ^-------
Error: instantiating 'dispatch(IO+)'


In /opt/homebrew/Cellar/crystal/1.17.0/share/crystal/src/http/server.cr:451:5

 451 | spawn handle_client(io)
       ^
Error: expanding macro


There was a problem expanding macro 'spawn'

Called macro defined in /opt/homebrew/Cellar/crystal/1.17.0/share/crystal/src/concurrent.cr:121:1

 121 | macro spawn(call, *, name = nil, same_thread = false, &block)

Which expanded to:

    1 |
    2 |
    3 |
    4 |     ->(
    5 |
    6 |         __arg0 : typeof(io),
    7 |
    8 |
    9 |       ) {
   10 |       spawn(name: nil, same_thread: false) do
 > 11 |         handle_client(
   12 |
   13 |             __arg0,
   14 |
   15 |
   16 |         )
   17 |       end
   18 |       }.call(
   19 |
   20 |           io,
   21 |
   22 |
   23 |       )
   24 |
Error: instantiating 'handle_client(IO+)'


In /opt/homebrew/Cellar/crystal/1.17.0/share/crystal/src/http/server.cr:521:16

 521 | @processor.process(io, io)
                  ^------
Error: instantiating 'HTTP::Server::RequestProcessor#process(IO+, IO+)'


In /opt/homebrew/Cellar/crystal/1.17.0/share/crystal/src/http/server/request_processor.cr:50:13

 50 | Log.with_context do
          ^-----------
Error: instantiating 'Log#with_context()'


In /opt/homebrew/Cellar/crystal/1.17.0/share/crystal/src/log/main.cr:124:16

 124 | self.class.with_context(**kwargs) do
                  ^-----------
Error: instantiating 'Log.with_context()'


In /opt/homebrew/Cellar/crystal/1.17.0/share/crystal/src/log/main.cr:124:16

 124 | self.class.with_context(**kwargs) do
                  ^-----------
Error: instantiating 'Log.with_context()'


In /opt/homebrew/Cellar/crystal/1.17.0/share/crystal/src/http/server/request_processor.cr:50:13

 50 | Log.with_context do
          ^-----------
Error: instantiating 'Log#with_context()'


In /opt/homebrew/Cellar/crystal/1.17.0/share/crystal/src/http/server/request_processor.cr:51:20

 51 | @handler.call(context)
               ^---
Error: instantiating '(HTTP::Handler | Proc(HTTP::Server::Context, Nil))#call(HTTP::Server::Context)'


In /opt/homebrew/Cellar/crystal/1.17.0/share/crystal/src/http/server/handlers/compress_handler.cr:14:5

 14 | {% if flag?(:without_zlib) %}
      ^
Error: expanding macro


There was a problem expanding macro 'macro_5284479696'

Called macro defined in /opt/homebrew/Cellar/crystal/1.17.0/share/crystal/src/http/server/handlers/compress_handler.cr:14:5

 14 | {% if flag?(:without_zlib) %}

Which expanded to:

   1 |
   2 |       context.response.output = CompressIO.new(context.response.output, context)
 > 3 |       call_next(context)
   4 |
Error: instantiating 'call_next(HTTP::Server::Context)'


In /opt/homebrew/Cellar/crystal/1.17.0/share/crystal/src/http/server/handler.cr:30:20

 30 | next_handler.call(context)
                   ^---
Error: instantiating '(HTTP::Handler | Proc(HTTP::Server::Context, Nil))#call(HTTP::Server::Context)'


In /opt/homebrew/Cellar/crystal/1.17.0/share/crystal/src/http/server/handlers/error_handler.cr:18:5

 18 | call_next(context)
      ^--------
Error: instantiating 'call_next(HTTP::Server::Context)'


In /opt/homebrew/Cellar/crystal/1.17.0/share/crystal/src/http/server/handler.cr:30:20

 30 | next_handler.call(context)
                   ^---
Error: instantiating '(HTTP::Handler | Proc(HTTP::Server::Context, Nil))#call(HTTP::Server::Context)'


In /opt/homebrew/Cellar/crystal/1.17.0/share/crystal/src/http/server/handlers/log_handler.cr:17:7

 17 | call_next(context)
      ^--------
Error: instantiating 'call_next(HTTP::Server::Context)'


In /opt/homebrew/Cellar/crystal/1.17.0/share/crystal/src/http/server/handler.cr:30:20

 30 | next_handler.call(context)
                   ^---
Error: instantiating '(HTTP::Handler | Proc(HTTP::Server::Context, Nil))#call(HTTP::Server::Context)'


In lib/kemal/src/kemal/static_file_handler.cr:61:47

 61 | directory_listing(context.response, request_path, file_path)
                                          ^-----------
Error: expected argument #2 to 'Kemal::StaticFileHandler#directory_listing' to be Path, not String

Overloads are:
 - HTTP::StaticFileHandler#directory_listing(io : IO, request_path : Path, path : Path)

[straight-shoota]

Yes, directory_listing now ensures the path parameter is a Path in order to keep the interface small. It was never intended to be used with string. But Kemal did that and it just happened to still work.
This is fixed in Kemal 1.7.2 (kemalcr/kemal/pull/714).

I wouldn't consider this a regression. It's a parameter of a private internal method, not a public API. And the reason for this inconsistency is Kemal chosing a different type than the stdlib implementation intended.