Don't raise ActionController::UnknownHttpMethod from ActionDispatch::Static #25798
Conversation
|
r? @sgrif (@rails-bot has picked a reviewer for you, use r? to override) |
|
Can we use |
|
We use We could do the following, if it's preferable? def call(env)
req = Rack::Request.new env
if req.get? || req.head?
path = req.path_info.chomp('/'.freeze)
if match = @file_handler.match?(path)
req.path_info = match
return @file_handler.serve(ActionDispatch::Request.new(env))
end
end
@app.call(req.env)
end |
|
@greysteil ya, lets go with |
|
@matthewd @tenderlove - updated. |
| @@ -160,6 +160,9 @@ def test_serves_gzip_files_when_header_set | |||
| response = get(file_name, 'HTTP_ACCEPT_ENCODING' => 'GZIP') | |||
| assert_gzip file_name, response | |||
|
|
|||
| response = get(file_name, 'HTTP_ACCEPT_ENCODING' => 'compress;q=0.5, gzip;q=1.0') | |||
There was a problem hiding this comment.
We didn't have any tests where gzip wasn't the first content-coding, but now we're using the parsed accept encoding (rather than the raw string) it feels valuable to check this case.
…Static The `ActionDispatch::Static` middleware is used low down in the stack to serve static assets before doing much processing. Since it's called from so low in the stack, we don't have access to the request ID at this point, and generally won't have any exception handling defined (by default `ShowExceptions` is added to the stack quite a bit higher and relies on logging and request ID). Before 8f27d60 this middleware would ignore unknown HTTP methods, and an exception about these would be raised higher in the stack. After that commit, however, that exception will be raised here. If we want to keep `ActionDispatch::Static` so low in the stack (I think we do) we should suppress the `ActionController::UnknownHttpMethod` exception here, and instead let it be raised higher up the stack, once we've had a chance to define exception handling behaviour. This PR updates `ActionDispatch::Static` so it passes `Rack::Request` objects to `ActionDispatch::FileHandler`, which won't raise an `ActionController::UnknownHttpMethod` error. If an unknown method is passed, it should exception higher in the stack instead, once we've had a chance to define exception handling behaviour.`
|
Just rebased off master to fix a changelog conflict, and specs are 💚 . Hopefully this is good to go now. |
|
r? @matthewd |
|
❤️ - thanks guys. |
…od-low-in-stack Don't raise ActionController::UnknownHttpMethod from ActionDispatch::Static
|
Backported in ffa13e9 |
The
ActionDispatch::Staticmiddleware is used low down in the stack to serve static assets before doing much processing. Since it's called from so low in the stack, we don't have access to the request ID at this point, and generally won't have any exception handling defined (by defaultShowExceptionsis added to the stack quite a bit higher and relies on logging and request ID).Before 8f27d60 this middleware would ignore unknown HTTP methods, and an exception about these would be raised higher in the stack. After that commit, however, that exception will be raised here.
If we want to keep
ActionDispatch::Static so low in the stack (I think we do) we should suppress theActionController::UnknownHttpMethod` exception here, and instead let it be raised higher up the stack, once we've had a chance to define exception handling behaviour.