Error handling improvements

* Capture bodies of raised NotFound and BadRequest errors * Rework handle_exception! to use &&/||/! instead of and/or/not * Use Forwardable to delegate methods to the Request and Response objects
sinatra · Dec 27, 2016 · 1d3d9ef · 1d3d9ef
1 parent bf2f65a
commit 1d3d9ef
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 49 deletions.
diff --git a/lib/sinatra/base.rb b/lib/sinatra/base.rb
@@ -10,6 +10,7 @@
 require 'mustermann/regular'
 
 # stdlib dependencies
+require 'forwardable'
 require 'thread'
 require 'time'
 require 'uri'
@@ -242,6 +243,8 @@ def http_status; 404 end
 
   # Methods available to routes, before/after filters, and views.
   module Helpers
+    extend Forwardable
+
     # Set or retrieve the response status code.
     def status(value = nil)
       response.status = Rack::Utils.status_code(value) if value
@@ -319,16 +322,6 @@ def headers(hash = nil)
       response.headers
     end
 
-    # Access the underlying Rack session.
-    def session
-      request.session
-    end
-
-    # Access shared logger object.
-    def logger
-      request.logger
-    end
-
     # Look up a media type by file extension in Rack's mime registry.
     def mime_type(type)
       Base.mime_type(type)
@@ -574,40 +567,16 @@ def etag(value, options = {})
       end
     end
 
-    # Sugar for redirect (example:  redirect back)
-    def back
-      request.referer
-    end
-
-    # whether or not the status is set to 1xx
-    def informational?
-      status.between? 100, 199
-    end
-
-    # whether or not the status is set to 2xx
-    def success?
-      status.between? 200, 299
-    end
+    # Delegate methods to the request object.
+    def_delegators :request, :session, :logger
 
-    # whether or not the status is set to 3xx
-    def redirect?
-      status.between? 300, 399
-    end
-
-    # whether or not the status is set to 4xx
-    def client_error?
-      status.between? 400, 499
-    end
-
-    # whether or not the status is set to 5xx
-    def server_error?
-      status.between? 500, 599
-    end
+    # Sugar for redirect (example:  redirect back)
+    def_delegator :request, :referer, :back
 
-    # whether or not the status is set to 404
-    def not_found?
-      status == 404
-    end
+    # Delegate methods that inspect the status code to the response object.
+    def_delegators :response, :bad_request?, :client_error?, :informational?, :not_found?, :server_error?
+    def_delegator :response, :successful?, :success?
+    def_delegator :response, :redirection?, :redirect?
 
     # Generates a Time object from the given value.
     # Used by #expires and #last_modified.
@@ -1123,27 +1092,30 @@ def handle_exception!(boom)
       end
       @env['sinatra.error'] = boom
 
-      if boom.respond_to? :http_status
+      if boom.respond_to?(:http_status)
         status(boom.http_status)
-      elsif settings.use_code? and boom.respond_to? :code and boom.code.between? 400, 599
+      elsif settings.use_code? && boom.respond_to?(:code) && boom.code.between?(400, 599)
         status(boom.code)
       else
         status(500)
       end
 
-      status(500) unless status.between? 400, 599
+      status(500) unless status.between?(400, 599)
 
+      boom_body = boom.message if boom.message && boom.message != boom.class.name
       if server_error?
         dump_errors! boom if settings.dump_errors?
-        raise boom if settings.show_exceptions? and settings.show_exceptions != :after_handler
+        raise boom if settings.show_exceptions? && settings.show_exceptions != :after_handler
       elsif not_found?
         headers['X-Cascade'] = 'pass' if settings.x_cascade?
-        body '<h1>Not Found</h1>'
+        body boom_body || '<h1>Not Found</h1>'
+      elsif bad_request?
+        body boom_body || '<h1>Bad Request</h1>'
       end
 
       res = error_block!(boom.class, boom) || error_block!(status, boom)
-      return res if res or not server_error?
-      raise boom if settings.raise_errors? or settings.show_exceptions?
+      return res if res || !server_error?
+      raise boom if settings.raise_errors? || settings.show_exceptions?
       error_block! Exception, boom
     end
 

diff --git a/test/routing_test.rb b/test/routing_test.rb
@@ -175,6 +175,32 @@ class RoutingTest < Minitest::Test
     assert_equal 404, status
   end
 
+  it "captures the body of a raised NotFound" do
+    mock_app {
+      get '/' do
+        raise Sinatra::NotFound, "This is not a drill"
+      end
+    }
+
+    get "/"
+    assert_equal "19", response["Content-Length"]
+    assert_equal 404, status
+    assert_equal "This is not a drill", response.body
+  end
+
+  it "captures the body of a raised BadRequest" do
+    mock_app {
+      get '/' do
+        raise Sinatra::BadRequest, "This is not a drill either"
+      end
+    }
+
+    get "/"
+    assert_equal "26", response["Content-Length"]
+    assert_equal 400, status
+    assert_equal "This is not a drill either", response.body
+  end
+
   it "uses 404 error handler for not matching route" do
     mock_app {
       not_found do