-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validates response processed by exception handler #1776
Changes from 1 commit
9e945a6
429295f
3f95eb2
dfffd7f
96d3138
c32c977
d2bc1b6
641e746
553bf07
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -127,7 +127,13 @@ def run_rescue_handler(handler, error) | |
handler = public_method(handler) | ||
end | ||
|
||
handler.arity.zero? ? instance_exec(&handler) : instance_exec(error, &handler) | ||
response = handler.arity.zero? ? instance_exec(&handler) : instance_exec(error, &handler) | ||
valid_response?(response) ? response : error!('Internal Server Error(Invalid Response)') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we re-raise the original error instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. I will change the code to use default_rescue_handler to handle the original error. That means, if custom handlers failed to do their job (return valid response), default_rescue_handler will handle it. |
||
end | ||
|
||
def valid_response?(response) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we do anything similar elsewhere? Seems a bit fishy, possibly belongs in Rack (maybe there's a method somewhere to check that the response is valid)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After searching, I don't find anything similar. I know this is a bad smell. The better solution is let exception handler return a What do you think? @dblock |
||
# Rack::Response.new(...).finish generates an array with size 3 | ||
response.is_a?(Array) && response.size == 3 | ||
end | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We released 1.1.0, so this belongs in the next release above.