Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Setting status code to 404 in error handler triggers not_found handler #541

Closed
chancancode opened this Issue · 3 comments

3 participants

@chancancode

Something like

  # ...

  not_found do
    puts "not found!"
  end

  error do
    puts "error!"
    halt 404
  end

  # ...

Raising an error in this case would cause the error handler to be triggered, followed by the not_found handler. This seems wrong, as setting the status code to 5xx inside the error handler doesn't cause the error handler to fire (otherwise it would result in an infinite loop).

A real-world use case might be that your database adapter would throw an error when a record cannot be found, and you might want to catch that error inside an error handler, set the status code to 404 and provide a descriptive message. If the not_found handler is also triggered, it might overwrite the body set by the error handler.

@rkh
Owner

Agreed, I'll look into it.

@rkh rkh was assigned
@zzak
Collaborator

@rkh I've tried to write a test against this, not sure I fully understand the bug.

I'm getting a unexpected 500 status from this test, though.

diff --git a/test/helpers_test.rb b/test/helpers_test.rb
index 09ba8d5..7a593dd 100644
--- a/test/helpers_test.rb
+++ b/test/helpers_test.rb
@@ -281,6 +281,21 @@ class HelpersTest < Test::Unit::TestCase
       assert_equal 500, status
       assert_equal 'FAIL', body
     end
+
+    it 'uses the status code from a block' do
+      mock_app do
+        get('/') do
+          error 'FAIL' do
+            puts 'errors!'
+            halt 404
+          end
+        end
+      end
+
+      get '/'
+      assert_equal 404, status
+      assert_equal 'errors!', body
+    end
   end

   describe 'not_found' do
@chancancode

@zzak The appropriate test that describes the issue would be:

it 'should not invoke error handler when halting with 500 inside an error handler' do
  mock_app do
    not_found do
      body "not_found handler"
      halt 404
    end

    error do
      body "error handler"
      halt 404
    end

    get '/' do
      raise
    end
  end

  get '/'
  assert_equal 404, status
  assert_equal 'error handler', body
end

it 'should not invoke not_found handler when halting with 404 inside a not found handler' do
  mock_app do
    not_found do
      body "not_found handler"
      halt 500
    end

    error do
      body "error handler"
      halt 500
    end
  end

  get '/'
  assert_equal 500, status
  assert_equal 'not_found handler', body
end
@rkh rkh closed this in 86bc9ac
@senekis senekis referenced this issue in padrino/padrino-contrib
Merged

Use the not_found handler instead of error 404 #14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.