Bubble up to the error_formatter the original exception and the backtrace #1652

dcsg · 2017-07-04T00:18:43Z

When a custom parser is not able to parse the json input body, it raises an exception that is rescued in the Grape::Middleware::Formatter:111. However, the throw statement does not bubble up the root cause by not setting the backtrace. This makes it harder for the error_formatter to understand what caused the exception and to properly handle it.

I tried to add tests to this but was not that easy, any suggestions?

Before the fix

grape/middleware/formatter.rb:

my_custom_error_formatter.rb:

After the fix

my_custom_error_formatter.rb:

dblock · 2017-07-04T15:44:32Z

lib/grape/middleware/formatter.rb

@@ -110,7 +110,7 @@ def read_rack_input(body)
          rescue Grape::Exceptions::Base => e
            raise e
          rescue StandardError => e
-            throw :error, status: 400, message: e.message
+            throw :error, status: 400, message: e.message, backtrace: e


Is this a standard pattern somehow in Ruby? I mean I would expect backtrace to be an actual backtrace and not the original exception. Is anyone doing it like this elsewhere?

Are there other places we throw :error like this that also need fixing?

Is this a standard pattern somehow in Ruby? I mean I would expect backtrace to be an actual backtrace and not the original exception. Is anyone doing it like this elsewhere?

I am quite new in the Ruby world, so TBH I don't know if this is a standard pattern but I would say no. However the Grape library as a "contract" for the formatter: formatter.call(message, backtrace, options, env).

# lib/grape/middleware/error.rb:90 def error_response(error = {}) status = error[:status] || options[:default_status] message = error[:message] || options[:default_message] headers = { Grape::Http::Headers::CONTENT_TYPE => content_type } headers.merge!(error[:headers]) if error[:headers].is_a?(Hash) backtrace = error[:backtrace] || [] rack_response(format_message(message, backtrace), status, headers) end # lib/grape/middleware/error.rb:103 def format_message(message, backtrace) format = env[Grape::Env::API_FORMAT] || options[:format] formatter = Grape::ErrorFormatter.formatter_for(format, options) throw :error, status: 406, message: "The requested format '#{format}' is not supported." unless formatter formatter.call(message, backtrace, options, env) end

This method is called here:

# lib/grape/middleware/error.rb:29 def call!(env) @env = env begin error_response(catch(:error) do return @app.call(@env) end) rescue StandardError => e # ... end

Are there other places we throw :error like this that also need fixing?

Probably yes, but I didn't investigate any other use cases. This one affected us.

While it works, backtrace is meant to be a backtrace. So I think either adding an original exception into the contract or passing the actual backtrace is the right thing to do.

I can do both, what would be your advice on how to test it?

dblock · 2017-07-05T19:10:21Z

Danger is right, this also needs tests.

dcsg · 2017-07-05T23:58:27Z

@dblock would you care to review the code again? I already did the changes you requested (added both backtrace and original exception) and added a test.

I will update the changelog as soon as I have some feedback regarding the implementation.

dblock · 2017-07-06T16:33:29Z

Thanks for hanging in here @dcsg.

I am not loving this at all :( I think we're going the wrong way. The following passes all kinds of things here which are all properties of e.

throw :error, status: 400, message: e.message, backtrace: e.backtrace, exception: e

I think what we want to write is this:

throw :error, status: 400, original_exception: e

Does this look feasible? It might mean breaking some backwards compat which I am OK with, but maybe we don't have to?

dcsg · 2017-07-06T21:38:06Z

@dblock did the changes, what do you think about them?

dcsg · 2017-07-06T21:45:11Z

lib/grape/middleware/error.rb

@@ -92,19 +95,24 @@ def error_response(error = {})
        message = error[:message] || options[:default_message]
        headers = { Grape::Http::Headers::CONTENT_TYPE => content_type }
        headers.merge!(error[:headers]) if error[:headers].is_a?(Hash)
-        backtrace = error[:backtrace] || []
-        rack_response(format_message(message, backtrace), status, headers)
+        backtrace = error[:backtrace] || error[:original_exception] && error[:original_exception].backtrace || []


@dblock here I could replace this with the following:

backtrace = error[:backtrace] || error[:original_exception]&.backtrace || []

However, rubocop complains because of Ruby 2.1.*. Does Grape still support that Ruby version?

Grape supports whatever is in https://github.com/ruby-grape/grape/blob/master/.travis.yml, so 2.2+, so we can tame Rubocop accordingly. Since we don't use &. anywhere else maybe just expanding it is better.

This is fine.

dblock

I have some minor comments left and I am OK merging this because it does what it does. However I'd like us to go further and get rid of passing backtrace through, maybe in a future PR?

Basically I want

throw :error, status: 400, message: e.message, backtrace: e.backtrace, exception: e

to become

throw :error, status: 400, original_exception: e

dblock · 2017-07-07T15:06:14Z

lib/grape/error_formatter/json.rb

@@ -4,12 +4,15 @@ module Json
      extend Base

      class << self
-        def call(message, backtrace, options = {}, env = nil)
+        def call(message, backtrace, options = {}, env = nil, original_exception = nil)
          result = wrap_message(present(message, env))

          if (options[:rescue_options] || {})[:backtrace] && backtrace && !backtrace.empty?


The if (options[:rescue_options] || {}) is dup, so unwrap it.

rescue_options = options[:rescue_options] if (rescue_options) if ... end

dblock · 2017-07-07T15:07:05Z

lib/grape/middleware/error.rb

@@ -77,13 +80,13 @@ def exec_handler(e, &handler)
        end
      end

-      def error!(message, status = options[:default_status], headers = {}, backtrace = [])
+      def error!(message, status = options[:default_status], headers = {}, backtrace = [], original_exception = '')


Here original_exception should be defaulted to nil.

dblock · 2017-07-07T15:07:24Z

lib/grape/middleware/error.rb

@@ -92,19 +95,24 @@ def error_response(error = {})
        message = error[:message] || options[:default_message]
        headers = { Grape::Http::Headers::CONTENT_TYPE => content_type }
        headers.merge!(error[:headers]) if error[:headers].is_a?(Hash)
-        backtrace = error[:backtrace] || []
-        rack_response(format_message(message, backtrace), status, headers)
+        backtrace = error[:backtrace] || error[:original_exception] && error[:original_exception].backtrace || []


This is fine.

dblock · 2017-07-07T15:08:35Z

lib/grape/middleware/formatter.rb

@@ -45,7 +45,7 @@ def build_formatted_response(status, headers, bodies)
          Rack::Response.new(bodymap, status, headers)
        end
      rescue Grape::Exceptions::InvalidFormatter => e
-        throw :error, status: 500, message: e.message
+        throw :error, status: 500, message: e.message, backtrace: e.backtrace, exception: e


Sholdn't this be original_exception: e?

dblock · 2017-07-07T15:09:37Z

CHANGELOG.md

@@ -6,7 +6,7 @@

 #### Fixes

-* Your contribution here.
+* [#1652](https://github.com/ruby-grape/grape/pull/1652): Bubble up to the error_formatter the root exception - [@dcsg](https://github.com/dcsg).


root -> original

dcsg · 2017-07-07T15:36:20Z

I have some minor comments left and I am OK merging this because it does what it does. However I'd like us to go further and get rid of passing backtrace through, maybe in a future PR?

Doing this change it will break backwards compatibility. Do you want to do it now?

dcsg · 2017-07-07T15:43:10Z

@dblock did the changes.

dblock · 2017-07-07T17:36:29Z

lib/grape/middleware/formatter.rb

@@ -110,7 +110,7 @@ def read_rack_input(body)
          rescue Grape::Exceptions::Base => e
            raise e
          rescue StandardError => e
-            throw :error, status: 400, message: e.message
+            throw :error, status: 400, message: e.message, backtrace: e.backtrace, exception: e


This is still exception: ..., which tells me there's no tests for it cause this doesn't work.

Yeah, I didn't find a way to test these use cases. Do you have any suggestion?

Use a trivial custom parser that raises an exception, it will throw on parser.call and hit this code.

dblock · 2017-07-07T17:36:55Z

CHANGELOG.md

@@ -6,7 +6,7 @@

 #### Fixes

-* Your contribution here.
+* [#1652](https://github.com/ruby-grape/grape/pull/1652): Bubble up to the error_formatter the original exception - [@dcsg](https://github.com/dcsg).


Oh and this is a feature, not a fix, right? Should be above.

IMO is both a new feature (the original_exception) and a fix (the backtrace).

dcsg · 2017-07-11T00:00:19Z

add to run rubocop --auto-gen-config because of this:

spec/grape/middleware/formatter_spec.rb:330:19: C: Avoid using {...} for multi-line blocks.
    let(:options) {
                  ^
spec/grape/middleware/formatter_spec.rb:339:29: C: Avoid using {...} for multi-line blocks.
      error = catch(:error) {
                            ^

dcsg · 2017-07-11T00:01:21Z

@dblock added the missing test!

dcsg · 2017-07-11T00:27:09Z

quite odd the tests that are failing. Any ideas? I didn't change at all anything related to those tests that are failing in the past 2 commits

dblock · 2017-07-11T17:18:07Z

Found the problem with the build, #1655. I'll rebase/merge this one next.

dblock · 2017-07-11T18:00:40Z

You can rebase on master, otherwise I'll get to this soon.

…rser is not able to parse a json input body.

Add backtrace in missing `throw :error` Add tests

dcsg · 2017-07-11T19:54:18Z

@dblock rebase done

dblock · 2017-07-12T15:15:27Z

Merged, thank you.

dblock · 2017-07-12T15:15:53Z

Want to try to make the backward (in)compatible change to reduce the number of things we pass into the raise on top of this?

dcsg · 2017-07-12T15:39:44Z

@dblock yes, I can do it

dcsg · 2017-07-12T15:40:34Z

BTW when do you expect to release a new tag with this fix?

dcsg · 2017-07-13T22:38:29Z

@dblock I guess it will be kind of hard or almost impossible to do the refactor you want.

# lib/grape/dsl/inside_route.rb:103

      # End the request and display an error to the
      # end user with the specified message.
      #
      # @param message [String] The message to display.
      # @param status [Integer] the HTTP Status Code. Defaults to default_error_status, 500 if not set.
      def error!(message, status = nil, headers = nil)
        self.status(status || namespace_inheritable(:default_error_status))
        throw :error, message: message, status: self.status, headers: headers
      end

What do you think?

dblock · 2017-07-14T12:57:52Z

We'll do a release soon.

Give the refactor a shot and lets see if we can come up with something!

Updates README according to ruby-grape#1652 Prevents `ArgumentError: wrong number of arguments (given 5, expected 4)` from happening when following the README.

dblock reviewed Jul 4, 2017

View reviewed changes

dcsg force-pushed the bubble-up-exception branch from 9db6988 to b6f30b1 Compare July 5, 2017 23:55

dcsg changed the title ~~Bubble up to the error_formatter the root exception~~ Bubble up to the error_formatter the root exception and the backtrace Jul 6, 2017

ruby-grape deleted a comment from dcsg Jul 6, 2017

dcsg commented Jul 6, 2017

View reviewed changes

dblock requested changes Jul 7, 2017

View reviewed changes

dcsg changed the title ~~Bubble up to the error_formatter the root exception and the backtrace~~ Bubble up to the error_formatter the original exception and the backtrace Jul 7, 2017

dblock reviewed Jul 7, 2017

View reviewed changes

dcsg force-pushed the bubble-up-exception branch 5 times, most recently from e850d37 to 261de9e Compare July 7, 2017 20:49

dcsg force-pushed the bubble-up-exception branch from 6b08b4f to 4521544 Compare July 11, 2017 00:17

dcsg force-pushed the bubble-up-exception branch from ef0c192 to 264aee1 Compare July 11, 2017 00:49

dcsg added 3 commits July 11, 2017 20:53

Bubbles up to the error_formatter the root exception when a custom pa…

05bff0b

…rser is not able to parse a json input body.

update the changelog

25b1dcd

Add exception to the formatter.call contract

93670da

Add backtrace in missing `throw :error` Add tests

dcsg added 5 commits July 11, 2017 20:53

Refactor exception to original_exception.

2c90e6f

Code review changes

35fcf6f

add missing test

d9f5e51

rubocop fixes

0476231

trying to fix random test failures

469b05d

dcsg force-pushed the bubble-up-exception branch from 264aee1 to 469b05d Compare July 11, 2017 19:54

dblock approved these changes Jul 12, 2017

View reviewed changes

dblock merged commit 3c0be5e into ruby-grape:master Jul 12, 2017

dcsg deleted the bubble-up-exception branch July 12, 2017 15:39

vrinek added a commit to vrinek/grape that referenced this pull request Sep 27, 2017

Update README

0c096e3

Updates README according to ruby-grape#1652 Prevents `ArgumentError: wrong number of arguments (given 5, expected 4)` from happening when following the README.

milap-neupane mentioned this pull request Oct 11, 2017

Include original_exception in Readme for custom error formatter #1692

Merged

Timunas mentioned this pull request Sep 10, 2019

Error formatter new argument change not documented #1905

Open

Bubble up to the error_formatter the original exception and the backtrace #1652

Bubble up to the error_formatter the original exception and the backtrace #1652

Conversation

dcsg commented Jul 4, 2017 • edited Loading

Before the fix

After the fix

Choose a reason for hiding this comment

dcsg Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcsg Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

dblock commented Jul 5, 2017

dcsg commented Jul 5, 2017 • edited Loading

dblock commented Jul 6, 2017 • edited Loading

dcsg commented Jul 6, 2017

dcsg Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcsg commented Jul 7, 2017

dcsg commented Jul 7, 2017

Choose a reason for hiding this comment

dcsg Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcsg commented Jul 11, 2017

dcsg commented Jul 11, 2017

dcsg commented Jul 11, 2017

dblock commented Jul 11, 2017

dblock commented Jul 11, 2017

dcsg commented Jul 11, 2017

dblock commented Jul 12, 2017

dblock commented Jul 12, 2017

dcsg commented Jul 12, 2017 • edited Loading

dcsg commented Jul 12, 2017

dcsg commented Jul 13, 2017 • edited Loading

dblock commented Jul 14, 2017

dcsg commented Jul 4, 2017 •

edited

Loading

dcsg Jul 4, 2017 •

edited

Loading

dcsg Jul 5, 2017 •

edited

Loading

dcsg commented Jul 5, 2017 •

edited

Loading

dblock commented Jul 6, 2017 •

edited

Loading

dcsg Jul 6, 2017 •

edited

Loading

dcsg Jul 7, 2017 •

edited

Loading

dcsg commented Jul 12, 2017 •

edited

Loading

dcsg commented Jul 13, 2017 •

edited

Loading