simplify the Middleware constructor

We should do the hard work outside the constructor.  Also fix the tests
to not directly construct middleware objects, but to go through the
stack object.

3 files changed, 54 insertions, 57 deletions

diff --git a/actionpack/lib/action_controller/metal.rb b/actionpack/lib/action_controller/metal.rb
index dc88557e3a..817339dadf 100644
--- a/actionpack/lib/action_controller/metal.rb
+++ b/actionpack/lib/action_controller/metal.rb
@@ -11,18 +11,16 @@ module ActionController
   #
   class MiddlewareStack < ActionDispatch::MiddlewareStack #:nodoc:
     class Middleware < ActionDispatch::MiddlewareStack::Middleware #:nodoc:
-      def initialize(klass, *args, &block)
-        options = args.extract_options!
-        @only   = Array(options.delete(:only)).map(&:to_s)
-        @except = Array(options.delete(:except)).map(&:to_s)
-        args << options unless options.empty?
-        super
+      def initialize(klass, args, only, except, block)
+        @only = only
+        @except = except
+        super(klass, args, block)
       end
 
       def valid?(action)
-        if @only.present?
+        if @only.any?
           @only.include?(action)
-        elsif @except.present?
+        elsif @except.any?
           !@except.include?(action)
         else
           true
@@ -37,6 +35,17 @@ module ActionController
         middleware.valid?(action) ? middleware.build(a) : a
       end
     end
+
+    private
+
+    def build_middleware(klass, args, block)
+      options = args.extract_options!
+      only   = Array(options.delete(:only)).map(&:to_s)
+      except = Array(options.delete(:except)).map(&:to_s)
+      args << options unless options.empty?
+
+      Middleware.new(klass, args, only, except, block)
+    end
   end
 
   # <tt>ActionController::Metal</tt> is the simplest possible controller, providing a
diff --git a/actionpack/lib/action_dispatch/middleware/stack.rb b/actionpack/lib/action_dispatch/middleware/stack.rb
index 87e627319e..b39a502d77 100644
--- a/actionpack/lib/action_dispatch/middleware/stack.rb
+++ b/actionpack/lib/action_dispatch/middleware/stack.rb
@@ -6,7 +6,7 @@ module ActionDispatch
     class Middleware
       attr_reader :args, :block, :name, :classcache
 
-      def initialize(klass_or_name, *args, &block)
+      def initialize(klass_or_name, args, block)
         @klass = nil
 
         if klass_or_name.respond_to?(:name)
@@ -75,19 +75,17 @@ module ActionDispatch
       middlewares[i]
     end
 
-    def unshift(*args, &block)
-      middleware = self.class::Middleware.new(*args, &block)
-      middlewares.unshift(middleware)
+    def unshift(klass, *args, &block)
+      middlewares.unshift(build_middleware(klass, args, block))
     end
 
     def initialize_copy(other)
       self.middlewares = other.middlewares.dup
     end
 
-    def insert(index, *args, &block)
+    def insert(index, klass, *args, &block)
       index = assert_index(index, :before)
-      middleware = self.class::Middleware.new(*args, &block)
-      middlewares.insert(index, middleware)
+      middlewares.insert(index, build_middleware(klass, args, block))
     end
 
     alias_method :insert_before, :insert
@@ -107,9 +105,8 @@ module ActionDispatch
       middlewares.delete target
     end
 
-    def use(*args, &block)
-      middleware = self.class::Middleware.new(*args, &block)
-      middlewares.push(middleware)
+    def use(klass, *args, &block)
+      middlewares.push(build_middleware(klass, args, block))
     end
 
     def build(app = Proc.new)
@@ -123,5 +120,11 @@ module ActionDispatch
       raise "No such middleware to insert #{where}: #{index.inspect}" unless i
       i
     end
+
+    private
+
+    def build_middleware(klass, args, block)
+      Middleware.new(klass, args, block)
+    end
   end
 end
diff --git a/actionpack/test/dispatch/middleware_stack/middleware_test.rb b/actionpack/test/dispatch/middleware_stack/middleware_test.rb
index 9607f026db..6366d37aa4 100644
--- a/actionpack/test/dispatch/middleware_stack/middleware_test.rb
+++ b/actionpack/test/dispatch/middleware_stack/middleware_test.rb
@@ -12,65 +12,50 @@ module ActionDispatch
       }.each do |name, klass|
 
         define_method("test_#{name}_klass") do
-          mw = Middleware.new klass
-          assert_equal klass, mw.klass
+          stack = ActionDispatch::MiddlewareStack.new
+          stack.use klass
+          assert_equal klass, stack.first.klass
         end
 
         define_method("test_#{name}_==") do
-          mw1 = Middleware.new klass
-          mw2 = Middleware.new klass
-          assert_equal mw1, mw2
+          stack = ActionDispatch::MiddlewareStack.new
+          stack.use klass
+          stack.use klass
+          assert_equal 2, stack.size
+          assert_equal stack.first, stack.last
         end
 
       end
 
+      attr_reader :stack
+
+      def setup
+        @stack = ActionDispatch::MiddlewareStack.new
+      end
+
       def test_string_class
-        mw = Middleware.new Omg.name
-        assert_equal Omg, mw.klass
+        stack = ActionDispatch::MiddlewareStack.new
+        stack.use Omg.name
+        assert_equal Omg, stack.first.klass
       end
 
       def test_double_equal_works_with_classes
         k = Class.new
-        mw = Middleware.new k
-        assert_operator mw, :==, k
+        stack.use k
+        assert_operator stack.first, :==, k
 
-        result = mw != Class.new
+        result = stack.first != Class.new
         assert result, 'middleware should not equal other anon class'
       end
 
       def test_double_equal_works_with_strings
-        mw = Middleware.new Omg
-        assert_operator mw, :==, Omg.name
+        stack.use Omg
+        assert_operator stack.first, :==, Omg.name
       end
 
       def test_double_equal_normalizes_strings
-        mw = Middleware.new Omg
-        assert_operator mw, :==, "::#{Omg.name}"
-      end
-
-      def test_middleware_loads_classnames_from_cache
-        mw = Class.new(Middleware) {
-          attr_accessor :classcache
-        }.new(Omg.name)
-
-        fake_cache    = { mw.name => Omg }
-        mw.classcache = fake_cache
-
-        assert_equal Omg, mw.klass
-
-        fake_cache[mw.name] = Middleware
-        assert_equal Middleware, mw.klass
-      end
-
-      def test_middleware_always_returns_class
-        mw = Class.new(Middleware) {
-          attr_accessor :classcache
-        }.new(Omg)
-
-        fake_cache    = { mw.name => Middleware }
-        mw.classcache = fake_cache
-
-        assert_equal Omg, mw.klass
+        stack.use Omg
+        assert_operator stack.first, :==, "::#{Omg.name}"
       end
     end
   end