Make after_filter halt when before_filter renders or redirects [#5648 state:resolved]

Signed-off-by: José Valim <jose.valim@gmail.com>

2 files changed, 81 insertions, 2 deletions

diff --git a/actionpack/lib/abstract_controller/callbacks.rb b/actionpack/lib/abstract_controller/callbacks.rb
index 7b0d80614d..094e62b9c7 100644
--- a/actionpack/lib/abstract_controller/callbacks.rb
+++ b/actionpack/lib/abstract_controller/callbacks.rb
@@ -83,6 +83,7 @@ module AbstractController
           # for details on the allowed parameters.
           def #{filter}_filter(*names, &blk)                                                    # def before_filter(*names, &blk)
             _insert_callbacks(names, blk) do |name, options|                                    #   _insert_callbacks(names, blk) do |name, options}
+              options[:if]=(Array.wrap(options[:if]) << "!halted") if #{filter == :after}
               set_callback(:process_action, :#{filter}, name, options)                          #     set_callback(:process_action, :before_filter, name, options)
             end                                                                                 #   end
           end                                                                                   # end
@@ -91,6 +92,7 @@ module AbstractController
           # for details on the allowed parameters.
           def prepend_#{filter}_filter(*names, &blk)                                            # def prepend_before_filter(*names, &blk)
             _insert_callbacks(names, blk) do |name, options|                                    #   _insert_callbacks(names, blk) do |name, options|
+              options[:if]=(Array.wrap(options[:if]) << "!halted") if #{filter == :after}
               set_callback(:process_action, :#{filter}, name, options.merge(:prepend => true))  #     set_callback(:process_action, :before, name, options.merge(:prepend => true))
             end                                                                                 #   end
           end                                                                                   # end
diff --git a/actionpack/test/controller/filters_test.rb b/actionpack/test/controller/filters_test.rb
index 3a8a37d967..68febf425d 100644
--- a/actionpack/test/controller/filters_test.rb
+++ b/actionpack/test/controller/filters_test.rb
@@ -78,7 +78,8 @@ class FilterTest < ActionController::TestCase
   end
 
   class RenderingController < ActionController::Base
-    before_filter :render_something_else
+    before_filter :before_filter_rendering
+    after_filter :unreached_after_filter
 
     def show
       @ran_action = true
@@ -86,9 +87,59 @@ class FilterTest < ActionController::TestCase
     end
 
     private
-      def render_something_else
+      def before_filter_rendering
+        @ran_filter ||= []
+        @ran_filter << "before_filter_rendering"
         render :inline => "something else"
       end
+
+      def unreached_after_filter
+        @ran_filter << "unreached_after_filter_after_render"
+      end
+  end
+
+  class RenderingForPrependAfterFilterController < RenderingController
+    prepend_after_filter :unreached_prepend_after_filter
+
+    private
+      def unreached_prepend_after_filter
+        @ran_filter << "unreached_preprend_after_filter_after_render"
+      end
+  end
+
+  class BeforeFilterRedirectionController < ActionController::Base
+    before_filter :before_filter_redirects
+    after_filter :unreached_after_filter
+
+    def show
+      @ran_action = true
+      render :inline => "ran show action"
+    end
+
+    def target_of_redirection
+      @ran_target_of_redirection = true
+      render :inline => "ran target_of_redirection action"
+    end
+
+    private
+      def before_filter_redirects
+        @ran_filter ||= []
+        @ran_filter << "before_filter_redirects"
+        redirect_to(:action => 'target_of_redirection')
+      end
+
+      def unreached_after_filter
+        @ran_filter << "unreached_after_filter_after_redirection"
+      end
+  end
+
+  class BeforeFilterRedirectionForPrependAfterFilterController < BeforeFilterRedirectionController
+    prepend_after_filter :unreached_prepend_after_filter_after_redirection
+
+    private
+      def unreached_prepend_after_filter_after_redirection
+        @ran_filter << "unreached_prepend_after_filter_after_redirection"
+      end
   end
 
   class ConditionalFilterController < ActionController::Base
@@ -625,6 +676,32 @@ class FilterTest < ActionController::TestCase
     assert !assigns["ran_action"]
   end
 
+  def test_before_filter_rendering_breaks_filtering_chain_for_after_filter
+    response = test_process(RenderingController)
+    assert_equal %w( before_filter_rendering ), assigns["ran_filter"]
+    assert !assigns["ran_action"]
+  end
+
+  def test_before_filter_redirects_breaks_filtering_chain_for_after_filter
+    response = test_process(BeforeFilterRedirectionController)
+    assert_response :redirect
+    assert_equal "http://test.host/filter_test/before_filter_redirection/target_of_redirection", redirect_to_url
+    assert_equal %w( before_filter_redirects ), assigns["ran_filter"]
+  end
+
+  def test_before_filter_rendering_breaks_filtering_chain_for_preprend_after_filter
+    response = test_process(RenderingForPrependAfterFilterController)
+    assert_equal %w( before_filter_rendering ), assigns["ran_filter"]
+    assert !assigns["ran_action"]
+  end
+
+  def test_before_filter_redirects_breaks_filtering_chain_for_preprend_after_filter
+    response = test_process(BeforeFilterRedirectionForPrependAfterFilterController)
+    assert_response :redirect
+    assert_equal "http://test.host/filter_test/before_filter_redirection_for_prepend_after_filter/target_of_redirection", redirect_to_url
+    assert_equal %w( before_filter_redirects ), assigns["ran_filter"]
+  end
+
   def test_filters_with_mixed_specialization_run_in_order
     assert_nothing_raised do
       response = test_process(MixedSpecializationController, 'bar')