Merge pull request #17362 from bronzle/fix_debug_exceptions_app

Show the user’s application in the source window and select the correct ...

4 files changed, 76 insertions, 26 deletions

diff --git a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
index 274f6f2f22..ff72592b94 100644
--- a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
+++ b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
@@ -35,10 +35,23 @@ module ActionDispatch
 
       if env['action_dispatch.show_detailed_exceptions']
         request = Request.new(env)
+        traces = traces_from_wrapper(wrapper)
+
+        trace_to_show = 'Application Trace'
+        if traces[trace_to_show].empty?
+          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 = ActionView::Base.new([RESCUES_TEMPLATE_PATH],
           request: request,
           exception: wrapper.exception,
-          traces: traces_from_wrapper(wrapper),
+          traces: traces,
+          show_source_idx: source_to_show_id,
+          trace_to_show: trace_to_show,
           routes_inspector: routes_inspector(exception),
           source_extract: wrapper.source_extract,
           line_number: wrapper.line_number,
@@ -97,31 +110,28 @@ module ActionDispatch
     # Augment the exception traces by providing ids for all unique stack frame
     def traces_from_wrapper(wrapper)
       application_trace = wrapper.application_trace
-      framework_trace = wrapper.framework_trace
-      full_trace = wrapper.full_trace
-
-      if application_trace && framework_trace
-        id_counter = 0
-
-        application_trace = application_trace.map do |trace|
-          prev = id_counter
-          id_counter += 1
-          { id: prev, trace: trace }
-        end
-
-        framework_trace = framework_trace.map do |trace|
-          prev = id_counter
-          id_counter += 1
-          { id: prev, trace: trace }
+      framework_trace   = wrapper.framework_trace
+      full_trace        = wrapper.full_trace
+
+      appplication_trace_with_ids = []
+      framework_trace_with_ids = []
+      full_trace_with_ids = []
+
+      if full_trace
+        full_trace.each_with_index do |trace, idx|
+          id_trace = {
+            id: idx,
+            trace: trace
+          }
+          appplication_trace_with_ids << id_trace if application_trace.include? trace
+          framework_trace_with_ids << id_trace if framework_trace.include? trace
+          full_trace_with_ids << id_trace
         end
-
-        full_trace = application_trace + framework_trace
       end
-
       {
-        "Application Trace" => application_trace,
-        "Framework Trace" => framework_trace,
-        "Full Trace" => full_trace
+        "Application Trace" => appplication_trace_with_ids,
+        "Framework Trace" => framework_trace_with_ids,
+        "Full Trace" => full_trace_with_ids
       }
     end
   end
diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.erb
index ba29e26930..eabac3a9d2 100644
--- a/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.erb
+++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.erb
@@ -1,7 +1,7 @@
 <% if @source_extract %>
   <% @source_extract.each_with_index do |extract_source, index| %>
     <% if extract_source[:code] %>
-      <div class="source <%="hidden" if index != 0%>" id="frame-source-<%=index%>">
+      <div class="source <%="hidden" if @show_source_idx != index%>" id="frame-source-<%=index%>">
         <div class="info">
           Extracted source (around line <strong>#<%= extract_source[:line_number] %></strong>):
         </div>
diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/_trace.html.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/_trace.html.erb
index f62caf51d7..ab57b11c7d 100644
--- a/actionpack/lib/action_dispatch/middleware/templates/rescues/_trace.html.erb
+++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/_trace.html.erb
@@ -12,7 +12,7 @@
   <% end %>
 
   <% @traces.each do |name, trace| %>
-    <div id="<%= name.gsub(/\s/, '-') %>" style="display: <%= (name == "Application Trace") ? 'block' : 'none' %>;">
+    <div id="<%= name.gsub(/\s/, '-') %>" style="display: <%= (name == @trace_to_show) ? 'block' : 'none' %>;">
       <pre><code><% trace.each do |frame| %><a class="trace-frames" data-frame-id="<%= frame[:id] %>" href="#"><%= frame[:trace] %></a><br><% end %></code></pre>
     </div>
   <% end %>
diff --git a/actionpack/test/dispatch/debug_exceptions_test.rb b/actionpack/test/dispatch/debug_exceptions_test.rb
index 0add9fa3b0..f8851f0152 100644
--- a/actionpack/test/dispatch/debug_exceptions_test.rb
+++ b/actionpack/test/dispatch/debug_exceptions_test.rb
@@ -19,6 +19,10 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest
       @closed = true
     end
 
+    def method_that_raises
+      raise StandardError.new 'error in framework'
+    end
+
     def call(env)
       env['action_dispatch.show_detailed_exceptions'] = @detailed
       req = ActionDispatch::Request.new(env)
@@ -57,7 +61,8 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest
                                               {})
           raise ActionView::Template::Error.new(template, e)
         end
-
+      when "/framework_raises"
+        method_that_raises
       else
         raise "puke!"
       end
@@ -280,4 +285,39 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest
       assert_select 'pre code', /\(eval\):1: syntax error, unexpected/
     end
   end
+
+  test 'debug exceptions app shows user code that caused the error in source view' do
+    @app = DevelopmentApp
+    Rails.stubs(:root).returns(Pathname.new('.'))
+    cleaner = ActiveSupport::BacktraceCleaner.new.tap do |bc|
+      bc.add_silencer { |line| line =~ /method_that_raises/ }
+      bc.add_silencer { |line| line !~ %r{test/dispatch/debug_exceptions_test.rb} }
+    end
+
+    get '/framework_raises', {}, {'action_dispatch.backtrace_cleaner' => cleaner}
+
+    # Assert correct error
+    assert_response 500
+    assert_select 'h2', /error in framework/
+
+    # assert source view line is the call to method_that_raises
+    assert_select 'div.source:not(.hidden)' do
+      assert_select 'pre .line.active', /method_that_raises/
+    end
+
+    # assert first source view (hidden) that throws the error
+    assert_select 'div.source:first' do
+      assert_select 'pre .line.active', /raise StandardError\.new/
+    end
+
+    # assert application trace refers to line that calls method_that_raises is first
+    assert_select '#Application-Trace' do
+      assert_select 'pre code a:first', %r{test/dispatch/debug_exceptions_test\.rb:\d+:in `call}
+    end
+
+    # assert framework trace that that threw the error is first
+    assert_select '#Framework-Trace' do
+      assert_select 'pre code a:first', /method_that_raises/
+    end
+  end
 end