Catchall error block fails to catch direct decendants of Exception

[thesecretmaster]

Because the conditional on this line checks for strictly less than instead of less than or equal to, error blocks are unable to catch any thing which is a direct descendant of Exception.

sinatra/lib/sinatra/base.rb

Line 1153 in eee711b

return false unless key.respond_to? :superclass and key.superclass < Exception

Here's a quick (untested) example:

require 'sinatra'

error do
  puts "Error block called!"
end

get "/" do
  raise StandardError
end

After visiting /, the error block will not be called. If this is by design, then this seems like a strange design decision. If it isn't, then I'd be happy to open a PR for this.

[namusyaka]

@thesecretmaster Thanks for filing an issue.

What's your motivation to use StandardError? Even the default raise (IOW raise with no argument) raises RuntimeError (it's a subclass of StandardError) instead of raw StandardError.

If you have a good justification regarding your motivation and have a good patch (this meant the patch must not break other tested behavior), please submit a PR. Let's discuss based on the patch.

P.S.

1. I wouldn't like to break the compatibility, so even if your proposal goes well, I may not merge that until v3.0.0.
2. Your example needs to set :show_exceptions, false

[dentarg]

With set :show_exceptions, false (or RACK_ENV=production), I can see the error block being called, even with raise StandardError using Sinatra 2.0.5.

[jkowens]

I'm closing unless more information can be provided as I am seeing the same thing mentioned by @dentarg.

[carlwiedemann]

I'd suggest re-opening this issue. The problem here is that the behavior contracts the documentation.

In the README, it shows the following example, acting as a catch-all for any error.

error do
  'Sorry there was a nasty error - ' + env['sinatra.error'].message
end

However, this does not work as described.

Consider the following app:

require 'sinatra'

set :raise_errors, true
set :show_exceptions, false

class FooError < RuntimeError; end

get '/' do
  raise FooError, 'wat'
end

error do
  "Sorry there was a nasty error"
end

We expect the "Sorry there was a nasty error" message to appear, but it does not.

In order for the message to appear we change the error do line to instead use a specific error class such as:

• error FooError do
• error RuntimeError do
• error StandardError do

I will attempt to write a failing test that demonstrates this behavior.

[carlwiedemann]

I should be more specific: The behavior depends on :raise_errors config option.

If :raise_errors is true, then the catch-all works as described. Otherwise, it does not work as described.

[carlwiedemann]

I added #1914 as an example

[dentarg]

If :raise_errors is true, then the catch-all works as described.

Isn't #1914 showing the opposite?

sinatra/test/mapped_error_test.rb

Lines 121 to 130 in 70151c9

	it "uses the Exception handler before raising errors even when raise_errors is set" do
	mock_app do
	set :raise_errors, true
	error(Exception) { "Parent handler" }
	get('/') { raise FooError }
	end
	get '/'
	assert_equal 500, status
	assert_equal "Parent handler", body
	end

[dentarg]

Just noting a thing here as I looked it up:

Because the conditional on this line checks for strictly less than instead of less than or equal to, error blocks are unable to catch any thing which is a direct descendant of Exception.

sinatra/lib/sinatra/base.rb

Line 1153 in eee711b

return false unless key.respond_to? :superclass and key.superclass < Exception

Looks like that comes from 2e0030f

[dentarg]

Is the bug that any error handlers are called when raise_errors are true? 🤔

[carlwiedemann]

Isn't #1914 showing the opposite?

Sorry, let me explain what I meant. I was trying to say that when raise_errors is true, that the problem as originally described by @thesecretmaster would happen.

Is the bug that any error handlers are called when raise_errors are true? 🤔

That's difficult to say. Here's why: Consider the test added in 4e71959, which now appears on line 110 of mapped_error_test.rb

That test, which has been in the codebase for 13 years, seems to suggest that error handlers should be invoked when raise_errors is true, and that the error handler text should appear in the HTTP response body.

Based on that information, I added an additional test, which demonstrates that the error handler is not invoked if the handler is declared for Exception, or is a catch-all.

So, I would expect the test that I added in #1914 to also pass.

If you compare the original test that I mentioned above from 4e71959, with the test that I added, you can see the only material difference is the class that the error handler is invoked for -- the original test uses FooError, while my test uses Exception.

Here they are together (as they appear in the test file):

    it "calls error handlers before raising errors even when raise_errors is set" do
      mock_app do
        set :raise_errors, true
        error(FooError) { "she's there." }
        get('/') { raise FooError }
      end
      get '/'
      assert_equal 500, status
      assert_equal "she's there.", body
    end

    it "uses the Exception handler before raising errors even when raise_errors is set" do
      mock_app do
        set :raise_errors, true
        error(Exception) { "Parent handler" }
        get('/') { raise FooError }
      end
      get '/'
      assert_equal 500, status
      assert_equal "Parent handler", body
    end

It seems to me that both of these tests should pass. Right now only the first test passes, while the second test fails. What do you think?

If you agree, I am happy to work on #1914 to achieve that result.

[dentarg]

Thanks for clarifying and providing more background info.

I guess it depends on how you view your use of the catch-all (error do ... end), is it only there to provide a default response when something went wrong (unhandled exceptions) and you want errors to be raised when raise_errors is true (the default for tests).

I'm starting to think the existing behaviour is the intentional behaviour and not a bug (you want your tests to raise errors even if you have a catch all defined).

[carlwiedemann]

I'm starting to think the existing behaviour is the intentional behaviour and not a bug (you want your tests to raise errors even if you have a catch all defined).

Ok. If I am writing controller tests for an application that had a default catch-all error handler, and I want to write a test for the default catch-all error handler, how would I do that?

It seems to me that I cannot write that test, if raise_errors is supposed to be true, and the default catch-all error handler is never invoked, as you describe.

But that default catch-all error handler is application behavior, and application behavior should be subject to testing, no?

How would you suggest testing that implementation?

[dentarg]

I guess you would set raise_errors to false for that test?

[carlwiedemann]

If that is the official case, then it seems worth documenting. Do you agree?

Reading more about the initial change that I posted above it says the following in 4e71959:

• Exception error handlers always override the raise_errors option now.
  Previously, all exceptions would be raised outside of the application
  when the raise_errors option was enabled, even if an error handler was
  defined for that exception. The raise_errors option now controls
  whether unhandled exceptions are raised (enabled) or if a generic 500
  error is returned (disabled). (Ryan Tomayko)

I think I am now understanding a potential difference between a catch-all error handler that is declared using error vs undeclared where no error expression exists for Exception nor with an empty argument.

So if you are suggesting that in order to test a declared catch-all error handler, that raise_errors must be false, why should that not be the case for all error handlers? What makes a declared catch-all error handler special in your view? If I have declared it in my application, I would consider these errors to be “handled”, no?

So I could see a distinction made between a declared catch-all error handler, and an undeclared catch-all error handler. I believe the test on line 102 is testing something undeclared. But the test that I have added is testing something declared.

I would offer any of the following suggested options to improve the current situation:

1. Allow any explicitly declared catch-all error handler to behave as other error handlers do, which overrides raise_errors, while undeclared catch-all error handling does not override raise_errors. The test I wrote would pass. This is a breaking change.
2. Revert the changes that were added in 4e71959, which then means that all errors are raised, and raise_errors needs to be explicitly set to false to test any error handler. This is a breaking change.
3. Rename raise_errors to be more specific and accurate, e.g. raise_unhandled_errors or raise_unhandled_default_errors. This is a breaking change.
4. Change the documentation about raise_errors to accurately describe its behavior, as denoted in the comments that I quoted above. Its current description in the README is inadequate, in my view. This is a non-breaking change.

Option 4 is the least intrusive and difficult, so perhaps it would be most attractive. I am happy to open a separate PR to make that sort of change.

I will remark that, speaking for myself, even if option 4 would be pursued, I still view the application behavior as inconsistent and confusing. I quite value consistency because I believe it is less cognitive load than having to recall edge-cases for things like raise_errors, and less cognitive load leads to fewer mistakes.

While this comment suggests the changes in 4e71959 are the “best of both worlds” I would say it is the quite the opposite, since it introduced inconsistency and arbitrary definition to what raise_errors actually means — the name of the option is now quite meaningless, in my view.

[dentarg]

Thanks for that thorough comment @carlwiedemann. I also like consistency.

I think 4. is what we can do short term. For a new major version we could consider the other options I think.

I'm interested to hear some thoughts from the rest of @sinatra/sinatras-helpers

[carlwiedemann]

Ok, I opened #1917 for a potential doc update.

[dentarg]

@epergo @jkowens what are you thoughts here? Should we consider any implementation change?