Skip to content
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

BUGFIX: Fix bug where 'inspect_with_backtrace' raises an IndexError i… #625

Merged
merged 18 commits into from
Mar 2, 2018
Merged
Changes from 11 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
6e1d7ef
BUGFIX: Fix bug where 'inspect_with_backtrace' raises an IndexError i…
marksiemers Feb 3, 2018
6e3240a
Merge branch 'master' into ms/fix-internal_server_error-error
elorest Feb 3, 2018
97c6439
Merge branch 'master' into ms/fix-internal_server_error-error
marksiemers Feb 3, 2018
0250a5d
Switch puts statement to code comment about exception
marksiemers Feb 3, 2018
e63610c
Merge branch 'ms/fix-internal_server_error-error' of github.com:amber…
marksiemers Feb 3, 2018
3fd3848
Simplify 'internal_error_message' and add catch-all
marksiemers Feb 5, 2018
15ad9b4
Merge branch 'master' into ms/fix-internal_server_error-error
marksiemers Feb 5, 2018
843fb47
Merge branch 'master' into ms/fix-internal_server_error-error
faustinoaq Feb 8, 2018
ce6719f
Merge branch 'master' into ms/fix-internal_server_error-error
elorest Feb 9, 2018
1d6cb4a
Merge branch 'master' into ms/fix-internal_server_error-error
faustinoaq Feb 14, 2018
0cbe400
Add comment to prevent future refactor, report messages from unknown …
marksiemers Feb 18, 2018
394f9f6
Merge branch 'master' into ms/fix-internal_server_error-error
eliasjpr Feb 19, 2018
c725862
Merge branch 'master' into ms/fix-internal_server_error-error
faustinoaq Feb 20, 2018
bdafef6
Merge branch 'master' into ms/fix-internal_server_error-error
eliasjpr Feb 20, 2018
fe085e7
Merge branch 'master' into ms/fix-internal_server_error-error
eliasjpr Feb 21, 2018
d8e64a6
Merge branch 'master' into ms/fix-internal_server_error-error
robacarp Feb 24, 2018
17131e8
Merge branch 'master' into ms/fix-internal_server_error-error
robacarp Feb 27, 2018
3246f03
Merge branch 'master' into ms/fix-internal_server_error-error
eliasjpr Mar 2, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions src/amber/controller/error.cr
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,7 @@ module Amber::Controller
end

def internal_server_error
response_format("ERROR: #{@ex.inspect_with_backtrace}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should leave these comments here or else someone will refactor it again because they won't understand why there's added complexity here.

# IMPORTANT: #inspect_with_backtrace will fail in some situtions which breaks the tests.
# Even if you call @ex.callstack you'll notice that backtrace is nil.
# #backtrace? is supposed to be safe but it exceptions anyway.
# Please don't remove this without verifying that crystal core has been fixed first.
rescue ex : IndexError
response_format("ERROR: #{@ex.message}")
response_format("ERROR: #{internal_server_error_message}")
end

def forbidden
Expand All @@ -33,6 +27,21 @@ module Amber::Controller
end
end

private def internal_server_error_message
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with moving this into a private method especially since this only gets called if there's an error.

Personally I think we abstract code into methods too much though. In a lot of cases it's just as readable either way and slows down the code minutely every branch we add to the AST.

# IMPORTANT: #inspect_with_backtrace will fail in some situtions which breaks the tests.
# Even if you call @ex.callstack you'll notice that backtrace is nil.
# #backtrace? is supposed to be safe but it exceptions anyway.
# Please don't remove this without verifying that crystal core has been fixed first.
@ex.inspect_with_backtrace
rescue ex : IndexError
@ex.message
rescue ex
<<-ERROR
Original Error: #{@ex.message}
Error during 'inspect_with_backtrace': #{ex.message}
ERROR
end

private def response_format(message)
case content_type
when "application/json"
Expand Down