Replace the current block/continuation filter chain handling by an implementation based on a simple loop. #8226 [Stefan Kaes]

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@6649 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

3 files changed, 189 insertions, 88 deletions

diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index f4fdeaa76d..631e328b03 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Replace the current block/continuation filter chain handling by an implementation based on a simple loop.  #8226 [Stefan Kaes]
+
 * Update UrlWriter to accept :anchor parameter. Closes #6771. [octopod]
 
 * Added RecordTagHelper for using RecordIdentifier conventions on divs and other container elements [DHH]. Example:
diff --git a/actionpack/lib/action_controller/filters.rb b/actionpack/lib/action_controller/filters.rb
index ec5bb759df..3543c80ec9 100644
--- a/actionpack/lib/action_controller/filters.rb
+++ b/actionpack/lib/action_controller/filters.rb
@@ -263,13 +263,13 @@ module ActionController #:nodoc:
       # The passed <tt>filters</tt> will be appended to the array of filters
       # that run _after_ actions on this controller are performed.
       def append_after_filter(*filters, &block)
-        prepend_filter_to_chain(filters, :after, &block)
+        append_filter_to_chain(filters, :after, &block)
       end
 
       # The passed <tt>filters</tt> will be prepended to the array of filters
       # that run _after_ actions on this controller are performed.
       def prepend_after_filter(*filters, &block)
-        append_filter_to_chain(filters, :after, &block)
+        prepend_filter_to_chain(filters, :after, &block)
       end
 
       # Shorthand for append_after_filter since it's the most common.
@@ -362,12 +362,12 @@ module ActionController #:nodoc:
 
       # Returns a mapping between filters and the actions that may run them.
       def included_actions #:nodoc:
-        read_inheritable_attribute("included_actions") || {}
+        @included_actions ||= read_inheritable_attribute("included_actions") || {}
       end
 
       # Returns a mapping between filters and actions that may not run them.
       def excluded_actions #:nodoc:
-        read_inheritable_attribute("excluded_actions") || {}
+        @excluded_actions ||= read_inheritable_attribute("excluded_actions") || {}
       end
 
       # Find a filter in the filter_chain where the filter method matches the _filter_ param
@@ -381,10 +381,11 @@ module ActionController #:nodoc:
 
       # Returns true if the filter is excluded from the given action
       def filter_excluded_from_action?(filter,action) #:nodoc:
-        if (ia = included_actions[filter]) && !ia.empty?
+        case
+        when ia = included_actions[filter]
           !ia.include?(action)
-        else
-          (excluded_actions[filter] || []).include?(action)
+        when ea = excluded_actions[filter]
+          ea.include?(action)
         end
       end
 
@@ -397,20 +398,28 @@ module ActionController #:nodoc:
           @filter = filter
         end
 
+        def type
+          :around
+        end
+
         def before?
-          false
+          type == :before
         end
 
         def after?
-          false
+          type == :after
         end
 
         def around?
-          true
+          type == :around
+        end
+
+        def run(controller)
+          raise ActionControllerError, 'No filter type: Nothing to do here.'
         end
 
         def call(controller, &block)
-          raise(ActionControllerError, 'No filter type: Nothing to do here.')
+          run(controller)
         end
       end
 
@@ -420,35 +429,38 @@ module ActionController #:nodoc:
         def filter
           @filter.filter
         end
-
-        def around?
-          false
-        end
       end
 
       class BeforeFilterProxy < FilterProxy #:nodoc:
-        def before?
-          true
+        def type
+          :before
         end
 
-        def call(controller, &block)
-          if false == @filter.call(controller) # must only stop if equal to false. only filters returning false are halted.
-            controller.halt_filter_chain(@filter, :returned_false)
-          else
-            yield
+        def run(controller)
+          # only filters returning false are halted.
+          if false == @filter.call(controller)
+            controller.halt_filter_chain(@filter)
           end
         end
+
+        def call(controller)
+          yield unless run(controller)
+        end
       end
 
       class AfterFilterProxy < FilterProxy #:nodoc:
-        def after?
-          true
+        def type
+          :after
         end
 
-        def call(controller, &block)
-          yield
+        def run(controller)
           @filter.call(controller)
         end
+
+        def call(controller)
+          yield
+          run(controller)
+        end
       end
 
       class SymbolFilter < Filter #:nodoc:
@@ -485,29 +497,67 @@ module ActionController #:nodoc:
         end
       end
 
+      class ClassBeforeFilter < Filter #:nodoc:
+        def call(controller, &block)
+          @filter.before(controller)
+        end
+      end
+
+      class ClassAfterFilter < Filter #:nodoc:
+        def call(controller, &block)
+          @filter.after(controller)
+        end
+      end
+
       protected
-        def append_filter_to_chain(filters, position = :around, &block)
-          write_inheritable_array('filter_chain', create_filters(filters, position, &block) )
+        def append_filter_to_chain(filters, filter_type = :around, &block)
+          pos = find_filter_append_position(filters, filter_type)
+          update_filter_chain(filters, filter_type, pos, &block)
+        end
+
+        def prepend_filter_to_chain(filters, filter_type = :around, &block)
+          pos = find_filter_prepend_position(filters, filter_type)
+          update_filter_chain(filters, filter_type, pos, &block)
+        end
+
+        def update_filter_chain(filters, filter_type, pos, &block)
+          new_filters = create_filters(filters, filter_type, &block)
+          new_chain = filter_chain.insert(pos, new_filters).flatten
+          write_inheritable_attribute('filter_chain', new_chain)
         end
 
-        def prepend_filter_to_chain(filters, position = :around, &block)
-          write_inheritable_attribute('filter_chain', create_filters(filters, position, &block) + filter_chain)
+        def find_filter_append_position(filters, filter_type)
+          unless filter_type == :after
+            filter_chain.each_with_index do |f,i|
+              return i if f.after?
+            end
+          end
+          return -1
+        end
+
+        def find_filter_prepend_position(filters, filter_type)
+          if filter_type == :after
+            filter_chain.each_with_index do |f,i|
+              return i if f.after?
+            end
+          end
+          return 0
         end
 
-        def create_filters(filters, position, &block) #:nodoc:
+        def create_filters(filters, filter_type, &block) #:nodoc:
           filters, conditions = extract_conditions(filters, &block)
-          filters.map! { |filter| find_or_create_filter(filter,position) }
+          filters.map! { |filter| find_or_create_filter(filter, filter_type) }
           update_conditions(filters, conditions)
           filters
         end
 
-        def find_or_create_filter(filter,position)
-          if found_filter = find_filter(filter) { |f| f.send("#{position}?") }
+        def find_or_create_filter(filter, filter_type)
+          if found_filter = find_filter(filter) { |f| f.type == filter_type }
             found_filter
           else
-            f = class_for_filter(filter).new(filter)
+            f = class_for_filter(filter, filter_type).new(filter)
             # apply proxy to filter if necessary
-            case position
+            case filter_type
             when :before
               BeforeFilterProxy.new(f)
             when :after
@@ -520,7 +570,7 @@ module ActionController #:nodoc:
 
         # The determination of the filter type was once done at run time.
         # This method is here to extract as much logic from the filter run time as possible
-        def class_for_filter(filter) #:nodoc:
+        def class_for_filter(filter, filter_type) #:nodoc:
           case
           when filter.is_a?(Symbol)
             SymbolFilter
@@ -534,8 +584,12 @@ module ActionController #:nodoc:
             end
           when filter.respond_to?(:filter)
             ClassFilter
+          when filter.respond_to?(:before) && filter_type == :before
+            ClassBeforeFilter
+          when filter.respond_to?(:after) && filter_type == :after
+            ClassAfterFilter
           else
-            raise(ActionControllerError, 'A filters must be a Symbol, Proc, Method, or object responding to filter.')
+            raise(ActionControllerError, 'A filter must be a Symbol, Proc, Method, or object responding to filter, after or before.')
           end
         end
 
@@ -550,8 +604,8 @@ module ActionController #:nodoc:
           return if conditions.empty?
           if conditions[:only]
             write_inheritable_hash('included_actions', condition_hash(filters, conditions[:only]))
-          else
-            write_inheritable_hash('excluded_actions', condition_hash(filters, conditions[:except])) if conditions[:except]
+          elsif conditions[:except]
+            write_inheritable_hash('excluded_actions', condition_hash(filters, conditions[:except]))
           end
         end
 
@@ -576,9 +630,9 @@ module ActionController #:nodoc:
 
         def remove_actions_from_included_actions!(filters,*actions)
           actions = actions.flatten.map(&:to_s)
-          updated_hash = filters.inject(included_actions) do |hash,filter|
+          updated_hash = filters.inject(read_inheritable_attribute('included_actions')||{}) do |hash,filter|
             ia = (hash[filter] || []) - actions
-            ia.blank? ? hash.delete(filter) : hash[filter] = ia
+            ia.empty? ? hash.delete(filter) : hash[filter] = ia
             hash
           end
           write_inheritable_attribute('included_actions', updated_hash)
@@ -595,7 +649,9 @@ module ActionController #:nodoc:
         def proxy_before_and_after_filter(filter) #:nodoc:
           return filter unless filter_responds_to_before_and_after(filter)
           Proc.new do |controller, action|
-            unless filter.before(controller) == false
+            if filter.before(controller) == false
+              controller.send :halt_filter_chain, filter
+            else
               begin
                 action.call
               ensure
@@ -619,30 +675,59 @@ module ActionController #:nodoc:
         self.class.filter_chain
       end
 
-      def call_filter(chain, index)
-        return (performed? || perform_action_without_filters) if index >= chain.size
-        filter = chain[index]
-        return call_filter(chain, index.next) if self.class.filter_excluded_from_action?(filter,action_name)
-
-        halted = false
-        filter.call(self) do
-          halted = call_filter(chain, index.next)
+      def skip_excluded_filters(chain, index)
+        while (filter = chain[index]) && self.class.filter_excluded_from_action?(filter, action_name)
+          index = index.next
         end
-        halt_filter_chain(filter.filter, :no_yield) if halted == false unless @before_filter_chain_aborted
-        halted
+        [filter, index]
       end
 
-      def halt_filter_chain(filter, reason)
-        if logger
-          case reason
-          when :no_yield
-            logger.info "Filter chain halted as [#{filter.inspect}] did not yield."
-          when :returned_false
-            logger.info "Filter chain halted as [#{filter.inspect}] returned false."
+      def call_filters(chain, index, nesting)
+        # run before filters until we find an after filter or around filter
+        while true
+          filter, index = skip_excluded_filters(chain, index)
+          break unless filter
+          case filter.type
+          when :before
+            # invoke before filter
+            filter.run(self)
+            index = index.next
+            break if @before_filter_chain_aborted
+          when :around
+            filter.call(self) do
+              # all remaining before and around filters will be run in this call
+              index = call_filters(chain, index.next, nesting.next)
+            end
+            break
+          else
+            # no before or around filters left
+            break
           end
         end
+
+        aborted = @before_filter_chain_aborted
+        perform_action_without_filters unless performed? || aborted
+        return index if aborted || nesting != 0
+
+        # run after filters, if any
+        while filter = chain[index]
+          filter, index = skip_excluded_filters(chain, index)
+          case filter.type
+          when :after
+            filter.run(self)
+            index = index.next
+          else
+            raise ActionControllerError, "filter #{filter.inspect} was in the wrong place!"
+          end
+        end
+
+        index.next
+      end
+
+      def halt_filter_chain(filter)
+        logger.info "Filter chain halted as [#{filter.inspect}] returned false." if logger
         @before_filter_chain_aborted = true
-        return false
+        false
       end
 
       protected
@@ -654,7 +739,7 @@ module ActionController #:nodoc:
 
       private
         def perform_action_with_filters
-          call_filter(self.class.filter_chain, 0)
+          call_filters(self.class.filter_chain, 0, 0)
         end
 
         def process_cleanup_with_filters
diff --git a/actionpack/test/controller/filters_test.rb b/actionpack/test/controller/filters_test.rb
index 3a74eba04a..f250b24f00 100644
--- a/actionpack/test/controller/filters_test.rb
+++ b/actionpack/test/controller/filters_test.rb
@@ -14,7 +14,7 @@ class FilterTest < Test::Unit::TestCase
         @ran_filter ||= []
         @ran_filter << "ensure_login"
       end
-      
+
       def clean_up
         @ran_after_filter ||= []
         @ran_after_filter << "clean_up"
@@ -62,7 +62,7 @@ class FilterTest < Test::Unit::TestCase
         render :inline => "something else"
       end
   end
-  
+
   class ConditionalFilterController < ActionController::Base
     def show
       render :inline => "ran action"
@@ -86,7 +86,7 @@ class FilterTest < Test::Unit::TestCase
         @ran_filter ||= []
         @ran_filter << "clean_up_tmp"
       end
-      
+
       def rescue_action(e) raise(e) end
   end
 
@@ -94,7 +94,7 @@ class FilterTest < Test::Unit::TestCase
     before_filter :ensure_login, :except => [ :show_without_filter, :another_action ]
   end
 
-  class OnlyConditionSymController < ConditionalFilterController 
+  class OnlyConditionSymController < ConditionalFilterController
     before_filter :ensure_login, :only => :show
   end
 
@@ -104,10 +104,10 @@ class FilterTest < Test::Unit::TestCase
 
   class BeforeAndAfterConditionController < ConditionalFilterController
     before_filter :ensure_login, :only => :show
-    after_filter  :clean_up_tmp, :only => :show 
+    after_filter  :clean_up_tmp, :only => :show
   end
-  
-  class OnlyConditionProcController < ConditionalFilterController 
+
+  class OnlyConditionProcController < ConditionalFilterController
     before_filter(:only => :show) {|c| c.assigns["ran_proc_filter"] = true }
   end
 
@@ -145,7 +145,7 @@ class FilterTest < Test::Unit::TestCase
   class ConditionalSkippingController < TestController
     skip_before_filter :ensure_login, :only => [ :login ]
     skip_after_filter  :clean_up,     :only => [ :login ]
-    
+
     before_filter :find_user, :only => [ :change_password ]
 
     def login
@@ -155,7 +155,7 @@ class FilterTest < Test::Unit::TestCase
     def change_password
       render :inline => "ran action"
     end
-    
+
     protected
       def find_user
         @ran_filter ||= []
@@ -166,15 +166,15 @@ class FilterTest < Test::Unit::TestCase
   class ConditionalParentOfConditionalSkippingController < ConditionalFilterController
     before_filter :conditional_in_parent, :only => [:show, :another_action]
     after_filter  :conditional_in_parent, :only => [:show, :another_action]
-    
+
     private
-      
+
       def conditional_in_parent
         @ran_filter ||= []
         @ran_filter << 'conditional_in_parent'
       end
   end
-  
+
   class ChildOfConditionalParentController < ConditionalParentOfConditionalSkippingController
     skip_before_filter :conditional_in_parent, :only => :another_action
     skip_after_filter  :conditional_in_parent, :only => :another_action
@@ -197,7 +197,7 @@ class FilterTest < Test::Unit::TestCase
       controller.assigns["was_audited"] = true
     end
   end
-  
+
   class AroundFilter
     def before(controller)
       @execution_log = "before"
@@ -209,7 +209,7 @@ class FilterTest < Test::Unit::TestCase
       controller.assigns["execution_log"] = @execution_log + " and after"
       controller.assigns["after_ran"] = true
       controller.class.execution_log << " after aroundfilter " if controller.respond_to? :execution_log
-    end    
+    end
   end
 
   class AppendedAroundFilter
@@ -219,12 +219,12 @@ class FilterTest < Test::Unit::TestCase
 
     def after(controller)
       controller.class.execution_log << " after appended aroundfilter "
-    end    
-  end  
-  
+    end
+  end
+
   class AuditController < ActionController::Base
     before_filter(AuditFilter)
-    
+
     def show
       render_text "hello"
     end
@@ -234,6 +234,14 @@ class FilterTest < Test::Unit::TestCase
     around_filter AroundFilter.new
   end
 
+  class BeforeAfterClassFilterController < PrependingController
+    begin
+      filter = AroundFilter.new
+      before_filter filter
+      after_filter filter
+    end
+  end
+
   class MixedFilterController < PrependingController
     cattr_accessor :execution_log
 
@@ -247,7 +255,7 @@ class FilterTest < Test::Unit::TestCase
     after_filter  { |c| c.class.execution_log << " after procfilter " }
     append_around_filter AppendedAroundFilter.new
   end
-  
+
   class MixedSpecializationController < ActionController::Base
     class OutOfOrder < StandardError; end
 
@@ -292,11 +300,11 @@ class FilterTest < Test::Unit::TestCase
   def test_base_class_in_isolation
     assert_equal [ ], ActionController::Base.before_filters
   end
-  
+
   def test_prepending_filter
     assert_equal [ :wonderful_life, :ensure_login ], PrependingController.before_filters
   end
-  
+
   def test_running_filters
     assert_equal %w( wonderful_life ensure_login ), test_process(PrependingController).template.assigns["ran_filter"]
   end
@@ -304,11 +312,11 @@ class FilterTest < Test::Unit::TestCase
   def test_running_filters_with_proc
     assert test_process(ProcController).template.assigns["ran_proc_filter"]
   end
-  
+
   def test_running_filters_with_implicit_proc
     assert test_process(ImplicitProcController).template.assigns["ran_proc_filter"]
   end
-  
+
   def test_running_filters_with_class
     assert test_process(AuditController).template.assigns["was_audited"]
   end
@@ -319,7 +327,7 @@ class FilterTest < Test::Unit::TestCase
     assert response.template.assigns["ran_class_filter"]
     assert response.template.assigns["ran_proc_filter1"]
     assert response.template.assigns["ran_proc_filter2"]
-    
+
     response = test_process(AnomolousYetValidConditionController, "show_without_filter")
     assert_equal nil, response.template.assigns["ran_filter"]
     assert !response.template.assigns["ran_class_filter"]
@@ -373,6 +381,12 @@ class FilterTest < Test::Unit::TestCase
     assert controller.template.assigns["after_ran"]
   end
 
+  def test_before_after_class_filter
+    controller = test_process(BeforeAfterClassFilterController)
+    assert controller.template.assigns["before_ran"]
+    assert controller.template.assigns["after_ran"]
+  end
+
   def test_having_properties_in_around_filter
     controller = test_process(AroundFilterController)
     assert_equal "before and after", controller.template.assigns["execution_log"]
@@ -381,10 +395,10 @@ class FilterTest < Test::Unit::TestCase
   def test_prepending_and_appending_around_filter
     controller = test_process(MixedFilterController)
     assert_equal " before aroundfilter  before procfilter  before appended aroundfilter " +
-                 " after appended aroundfilter  after aroundfilter  after procfilter ", 
+                 " after appended aroundfilter  after aroundfilter  after procfilter ",
                  MixedFilterController.execution_log
   end
-  
+
   def test_rendering_breaks_filtering_chain
     response = test_process(RenderingController)
     assert_equal "something else", response.body