Fix some edge cases in AD::DebugExceptions in rails api apps

2 files changed, 126 insertions, 52 deletions

diff --git a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
index 972410d806..c81f52ec88 100644
--- a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
+++ b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
@@ -67,55 +67,78 @@ module ActionDispatch
       log_error(request, wrapper)
 
       if request.get_header('action_dispatch.show_detailed_exceptions')
-        traces = wrapper.traces
-
-        trace_to_show = 'Application Trace'
-        if traces[trace_to_show].empty? && wrapper.rescue_template != 'routing_error'
-          trace_to_show = 'Full Trace'
-        end
-
-        if source_to_show = traces[trace_to_show].first
-          source_to_show_id = source_to_show[:id]
-        end
-
-        template = DebugView.new([RESCUES_TEMPLATE_PATH],
-          request: request,
-          exception: wrapper.exception,
-          traces: traces,
-          show_source_idx: source_to_show_id,
-          trace_to_show: trace_to_show,
-          routes_inspector: routes_inspector(exception),
-          source_extracts: wrapper.source_extracts,
-          line_number: wrapper.line_number,
-          file: wrapper.file
-        )
-        file = "rescues/#{wrapper.rescue_template}"
-
         if @api_only
-          body = {
-            :status => wrapper.status_code,
-            :error => Rack::Utils::HTTP_STATUS_CODES.fetch(wrapper.status_code, Rack::Utils::HTTP_STATUS_CODES[500]),
-            :exception => wrapper.exception.inspect,
-            :traces => traces
-          }
-          if content_type = request.formats.first
-            to_format = "to_#{content_type.to_sym}"
-            body = body.public_send(to_format)
-          end
-          format = "application/json"
-        elsif request.xhr?
-          body = template.render(template: file, layout: false, formats: [:text])
-          format = "text/plain"
+          render_for_api_application(request, wrapper)
         else
-          body = template.render(template: file, layout: 'rescues/layout')
-          format = "text/html"
+          render_for_non_api_application(request, wrapper)
         end
-        render(wrapper.status_code, body, format)
       else
         raise exception
       end
     end
 
+    def render_for_non_api_application(request, wrapper)
+      template = create_template(request, wrapper)
+
+      file = "rescues/#{wrapper.rescue_template}"
+
+      if request.xhr?
+        body = template.render(template: file, layout: false, formats: [:text])
+        format = "text/plain"
+      else
+        body = template.render(template: file, layout: 'rescues/layout')
+        format = "text/html"
+      end
+      render(wrapper.status_code, body, format)
+    end
+
+    def render_for_api_application(request, wrapper)
+      body = {
+        :status => wrapper.status_code,
+        :error => Rack::Utils::HTTP_STATUS_CODES.fetch(wrapper.status_code, Rack::Utils::HTTP_STATUS_CODES[500]),
+        :exception => wrapper.exception.inspect,
+        :traces => wrapper.traces
+      }
+
+      content_type = request.formats.first
+      to_format = "to_#{content_type.to_sym}"
+
+      if content_type && body.respond_to?(to_format)
+        body = body.public_send(to_format)
+        format = content_type
+      else
+        body = body.to_json
+        format = Mime::JSON
+      end
+
+      render(wrapper.status_code, body, format)
+    end
+
+    def create_template(request, wrapper)
+      traces = wrapper.traces
+
+      trace_to_show = 'Application Trace'
+      if traces[trace_to_show].empty? && wrapper.rescue_template != 'routing_error'
+        trace_to_show = 'Full Trace'
+      end
+
+      if source_to_show = traces[trace_to_show].first
+        source_to_show_id = source_to_show[:id]
+      end
+
+      DebugView.new([RESCUES_TEMPLATE_PATH],
+        request: request,
+        exception: wrapper.exception,
+        traces: traces,
+        show_source_idx: source_to_show_id,
+        trace_to_show: trace_to_show,
+        routes_inspector: routes_inspector(wrapper.exception),
+        source_extracts: wrapper.source_extracts,
+        line_number: wrapper.line_number,
+        file: wrapper.file
+      )
+    end
+
     def render(status, body, format)
       [status, {'Content-Type' => "#{format}; charset=#{Response.default_charset}", 'Content-Length' => body.bytesize.to_s}, [body]]
     end
diff --git a/actionpack/test/dispatch/debug_exceptions_test.rb b/actionpack/test/dispatch/debug_exceptions_test.rb
index 48eff700ce..d9c6233511 100644
--- a/actionpack/test/dispatch/debug_exceptions_test.rb
+++ b/actionpack/test/dispatch/debug_exceptions_test.rb
@@ -162,17 +162,6 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest
     assert_match(/ActionController::ParameterMissing/, body)
   end
 
-  test "rescue with json on API request" do
-    @app = ActionDispatch::DebugExceptions.new(BoomerAPI.new(true), RoutesApp, true)
-
-    get "/index.json", headers: { 'action_dispatch.show_exceptions' => true }
-    assert_response 500
-    assert_no_match(/<header>/, body)
-    assert_no_match(/<body>/, body)
-    assert_equal "application/json", response.content_type
-    assert_match(/RuntimeError: puke/, body)
-  end
-
   test "rescue with text error for xhr request" do
     @app = DevelopmentApp
     xhr_request_env = {'action_dispatch.show_exceptions' => true, 'HTTP_X_REQUESTED_WITH' => 'XMLHttpRequest'}
@@ -223,6 +212,68 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest
     assert_match(/ActionController::ParameterMissing/, body)
   end
 
+  test "rescue with json error for API request" do
+    @app = ActionDispatch::DebugExceptions.new(Boomer.new(true), RoutesApp, true)
+
+    get "/", headers: { 'action_dispatch.show_exceptions' => true }
+    assert_response 500
+    assert_no_match(/<header>/, body)
+    assert_no_match(/<body>/, body)
+    assert_equal "application/json", response.content_type
+    assert_match(/RuntimeError: puke/, body)
+
+    get "/not_found", headers: { 'action_dispatch.show_exceptions' => true }
+    assert_response 404
+    assert_no_match(/<body>/, body)
+    assert_equal "application/json", response.content_type
+    assert_match(/#{AbstractController::ActionNotFound.name}/, body)
+
+    get "/method_not_allowed", headers: { 'action_dispatch.show_exceptions' => true }
+    assert_response 405
+    assert_no_match(/<body>/, body)
+    assert_equal "application/json", response.content_type
+    assert_match(/ActionController::MethodNotAllowed/, body)
+
+    get "/unknown_http_method", headers: { 'action_dispatch.show_exceptions' => true }
+    assert_response 405
+    assert_no_match(/<body>/, body)
+    assert_equal "application/json", response.content_type
+    assert_match(/ActionController::UnknownHttpMethod/, body)
+
+    get "/bad_request", headers: { 'action_dispatch.show_exceptions' => true }
+    assert_response 400
+    assert_no_match(/<body>/, body)
+    assert_equal "application/json", response.content_type
+    assert_match(/ActionController::BadRequest/, body)
+
+    get "/parameter_missing", headers: { 'action_dispatch.show_exceptions' => true }
+    assert_response 400
+    assert_no_match(/<body>/, body)
+    assert_equal "application/json", response.content_type
+    assert_match(/ActionController::ParameterMissing/, body)
+  end
+
+  test "rescue with json on API request returns only allowed formats or json as a fallback" do
+    @app = ActionDispatch::DebugExceptions.new(Boomer.new(true), RoutesApp, true)
+
+    get "/index.json", headers: { 'action_dispatch.show_exceptions' => true }
+    assert_response 500
+    assert_equal "application/json", response.content_type
+    assert_match(/RuntimeError: puke/, body)
+
+    get "/index.html", headers: { 'action_dispatch.show_exceptions' => true }
+    assert_response 500
+    assert_no_match(/<header>/, body)
+    assert_no_match(/<body>/, body)
+    assert_equal "application/json", response.content_type
+    assert_match(/RuntimeError: puke/, body)
+
+    get "/index.xml", headers: { 'action_dispatch.show_exceptions' => true }
+    assert_response 500
+    assert_equal "application/xml", response.content_type
+    assert_match(/RuntimeError: puke/, body)
+  end
+
   test "does not show filtered parameters" do
     @app = DevelopmentApp