Merge branch 'constraints'

* constraints:
  rm reset_parameters because we automatically do it from 9ca4839a
  move path_parameter encoding check to the request object
  dispatcher doesn't need `call` anymore
  call `serve` with the request on dispatchers
  constraints class does not need the request class anymore
  give all endpoints a superclass
  skip the build business if the stack is empty
  stop hardcoding path_parameters and get it from the request
  we do not need to cache rack_app
  a redirect is not a dispatcher by definition, so eliminate test
  push is_a check up to where the Constraints object is allocated
  pass the request object to the application
  pass a request to `matches?` so we can avoid creating excess requests
  nothing is passed to `rack_app` anymore, so rm the params
  one fewer is_a check
  Constraints#app should never return another Constraints object, so switch to if statement
  eliminate dispatcher is_a checks
  push is_a?(Dispatcher) check in to one place
  Always construct route objects with Constraint objects

Conflicts:
	actionpack/lib/action_controller/metal.rb

11 files changed, 85 insertions, 75 deletions

diff --git a/actionpack/lib/action_controller/metal.rb b/actionpack/lib/action_controller/metal.rb
index 3bfc8d6460..9a427ebfdb 100644
--- a/actionpack/lib/action_controller/metal.rb
+++ b/actionpack/lib/action_controller/metal.rb
@@ -221,13 +221,18 @@ module ActionController
     # Makes the controller a Rack endpoint that runs the action in the given
     # +env+'s +action_dispatch.request.path_parameters+ key.
     def self.call(env)
-      action(env['action_dispatch.request.path_parameters'][:action]).call(env)
+      req = ActionDispatch::Request.new env
+      action(req.path_parameters[:action]).call(env)
     end
 
     # Returns a Rack endpoint for the given action name.
     def self.action(name, klass = ActionDispatch::Request)
-      middleware_stack.build(name) do |env|
-        new.dispatch(name, klass.new(env))
+      if middleware_stack.any?
+        middleware_stack.build(name) do |env|
+          new.dispatch(name, klass.new(env))
+        end
+      else
+        lambda { |env| new.dispatch(name, klass.new(env)) }
       end
     end
 
diff --git a/actionpack/lib/action_dispatch/http/parameters.rb b/actionpack/lib/action_dispatch/http/parameters.rb
index 5b22cd1fcd..ff555f93a8 100644
--- a/actionpack/lib/action_dispatch/http/parameters.rb
+++ b/actionpack/lib/action_dispatch/http/parameters.rb
@@ -43,10 +43,6 @@ module ActionDispatch
         @env[Routing::RouteSet::PARAMETERS_KEY] ||= {}
       end
 
-      def reset_parameters #:nodoc:
-        @env.delete("action_dispatch.request.parameters")
-      end
-
     private
 
       # Convert nested Hash to HashWithIndifferentAccess
diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb
index cdb3e44b3a..dfe258e463 100644
--- a/actionpack/lib/action_dispatch/http/request.rb
+++ b/actionpack/lib/action_dispatch/http/request.rb
@@ -53,6 +53,17 @@ module ActionDispatch
       @uuid              = nil
     end
 
+    def check_path_parameters!
+      # If any of the path parameters has an invalid encoding then
+      # raise since it's likely to trigger errors further on.
+      path_parameters.each do |key, value|
+        next unless value.respond_to?(:valid_encoding?)
+        unless value.valid_encoding?
+          raise ActionController::BadRequest, "Invalid parameter: #{key} => #{value}"
+        end
+      end
+    end
+
     def key?(key)
       @env.key?(key)
     end
diff --git a/actionpack/lib/action_dispatch/journey/route.rb b/actionpack/lib/action_dispatch/journey/route.rb
index 1ba91d548e..9f0a3af902 100644
--- a/actionpack/lib/action_dispatch/journey/route.rb
+++ b/actionpack/lib/action_dispatch/journey/route.rb
@@ -16,14 +16,6 @@ module ActionDispatch
         @app         = app
         @path        = path
 
-        # Unwrap any constraints so we can see what's inside for route generation.
-        # This allows the formatter to skip over any mounted applications or redirects
-        # that shouldn't be matched when using a url_for without a route name.
-        if app.is_a?(Routing::Mapper::Constraints)
-          app = app.app
-        end
-        @dispatcher  = app.is_a?(Routing::RouteSet::Dispatcher)
-
         @constraints = constraints
         @defaults    = defaults
         @required_defaults = nil
@@ -99,7 +91,7 @@ module ActionDispatch
       end
 
       def dispatcher?
-        @dispatcher
+        @app.dispatcher?
       end
 
       def matches?(request)
diff --git a/actionpack/lib/action_dispatch/journey/router.rb b/actionpack/lib/action_dispatch/journey/router.rb
index 7e525d04d4..74fa9ee3a2 100644
--- a/actionpack/lib/action_dispatch/journey/router.rb
+++ b/actionpack/lib/action_dispatch/journey/router.rb
@@ -39,7 +39,7 @@ module ActionDispatch
 
           req.path_parameters = set_params.merge parameters
 
-          status, headers, body = route.app.call(req.env)
+          status, headers, body = route.app.serve(req)
 
           if 'pass' == headers['X-Cascade']
             req.script_name     = script_name
diff --git a/actionpack/lib/action_dispatch/routing/endpoint.rb b/actionpack/lib/action_dispatch/routing/endpoint.rb
new file mode 100644
index 0000000000..88aa13c3e8
--- /dev/null
+++ b/actionpack/lib/action_dispatch/routing/endpoint.rb
@@ -0,0 +1,10 @@
+module ActionDispatch
+  module Routing
+    class Endpoint # :nodoc:
+      def dispatcher?;   false; end
+      def redirect?;     false; end
+      def matches?(req); true;  end
+      def app;           self;  end
+    end
+  end
+end
diff --git a/actionpack/lib/action_dispatch/routing/inspector.rb b/actionpack/lib/action_dispatch/routing/inspector.rb
index 2135b280da..ea3b2f419d 100644
--- a/actionpack/lib/action_dispatch/routing/inspector.rb
+++ b/actionpack/lib/action_dispatch/routing/inspector.rb
@@ -5,22 +5,15 @@ module ActionDispatch
   module Routing
     class RouteWrapper < SimpleDelegator
       def endpoint
-        rack_app ? rack_app.inspect : "#{controller}##{action}"
+        app.dispatcher? ? "#{controller}##{action}" : rack_app.inspect
       end
 
       def constraints
         requirements.except(:controller, :action)
       end
 
-      def rack_app(app = self.app)
-        @rack_app ||= begin
-          class_name = app.class.name.to_s
-          if class_name == "ActionDispatch::Routing::Mapper::Constraints"
-            app.app
-          elsif ActionDispatch::Routing::Redirect === app || class_name !~ /^ActionDispatch::Routing/
-            app
-          end
-        end
+      def rack_app
+        app.app
       end
 
       def verb
@@ -73,7 +66,7 @@ module ActionDispatch
       end
 
       def engine?
-        rack_app && rack_app.respond_to?(:routes)
+        rack_app.respond_to?(:routes)
       end
     end
 
diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
index b33c5e0dfd..84a08770f5 100644
--- a/actionpack/lib/action_dispatch/routing/mapper.rb
+++ b/actionpack/lib/action_dispatch/routing/mapper.rb
@@ -6,6 +6,7 @@ require 'active_support/core_ext/array/extract_options'
 require 'active_support/core_ext/module/remove_method'
 require 'active_support/inflector'
 require 'action_dispatch/routing/redirection'
+require 'action_dispatch/routing/endpoint'
 
 module ActionDispatch
   module Routing
@@ -15,35 +16,41 @@ module ActionDispatch
                        :controller, :action, :path_names, :constraints,
                        :shallow, :blocks, :defaults, :options]
 
-      class Constraints #:nodoc:
+      class Constraints < Endpoint #:nodoc:
         attr_reader :app, :constraints
 
-        def initialize(app, constraints, request)
+        def initialize(app, constraints, dispatcher_p)
           # Unwrap Constraints objects.  I don't actually think it's possible
           # to pass a Constraints object to this constructor, but there were
           # multiple places that kept testing children of this object.  I
           # *think* they were just being defensive, but I have no idea.
-          while app.is_a?(self.class)
+          if app.is_a?(self.class)
             constraints += app.constraints
             app = app.app
           end
 
-          @app, @constraints, @request = app, constraints, request
+          @dispatcher = dispatcher_p
+
+          @app, @constraints, = app, constraints
         end
 
-        def matches?(env)
-          req = @request.new(env)
+        def dispatcher?; @dispatcher; end
 
+        def matches?(req)
           @constraints.all? do |constraint|
             (constraint.respond_to?(:matches?) && constraint.matches?(req)) ||
               (constraint.respond_to?(:call) && constraint.call(*constraint_args(constraint, req)))
           end
-        ensure
-          req.reset_parameters
         end
 
-        def call(env)
-          matches?(env) ? @app.call(env) : [ 404, {'X-Cascade' => 'pass'}, [] ]
+        def serve(req)
+          return [ 404, {'X-Cascade' => 'pass'}, [] ] unless matches?(req)
+
+          if dispatcher?
+            @app.serve req
+          else
+            @app.call req.env
+          end
         end
 
         private
@@ -216,10 +223,16 @@ module ActionDispatch
           end
 
           def app
-            if blocks.any?
-              Constraints.new(endpoint, blocks, @set.request_class)
+            return to if Redirect === to
+
+            if to.respond_to?(:call)
+              Constraints.new(to, blocks, false)
             else
-              endpoint
+              if blocks.any?
+                Constraints.new(dispatcher, blocks, true)
+              else
+                dispatcher
+              end
             end
           end
 
@@ -306,10 +319,6 @@ module ActionDispatch
             Journey::Router::Strexp.compile(path, requirements, SEPARATORS)
           end
 
-          def endpoint
-            to.respond_to?(:call) ? to : dispatcher
-          end
-
           def dispatcher
             Routing::RouteSet::Dispatcher.new(defaults)
           end
diff --git a/actionpack/lib/action_dispatch/routing/redirection.rb b/actionpack/lib/action_dispatch/routing/redirection.rb
index f8ed0cbe6a..b1b39a5496 100644
--- a/actionpack/lib/action_dispatch/routing/redirection.rb
+++ b/actionpack/lib/action_dispatch/routing/redirection.rb
@@ -3,10 +3,11 @@ require 'active_support/core_ext/uri'
 require 'active_support/core_ext/array/extract_options'
 require 'rack/utils'
 require 'action_controller/metal/exceptions'
+require 'action_dispatch/routing/endpoint'
 
 module ActionDispatch
   module Routing
-    class Redirect # :nodoc:
+    class Redirect < Endpoint # :nodoc:
       attr_reader :status, :block
 
       def initialize(status, block)
@@ -14,17 +15,14 @@ module ActionDispatch
         @block  = block
       end
 
-      def call(env)
-        req = Request.new(env)
+      def redirect?; true; end
 
-        # If any of the path parameters has an invalid encoding then
-        # raise since it's likely to trigger errors further on.
-        req.path_parameters.each do |key, value|
-          unless value.valid_encoding?
-            raise ActionController::BadRequest, "Invalid parameter: #{key} => #{value}"
-          end
-        end
+      def call(env)
+        serve Request.new env
+      end
 
+      def serve(req)
+        req.check_path_parameters!
         uri = URI.parse(path(req.path_parameters, req))
         
         unless uri.host
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index 924455bce2..8777a9cd71 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -8,6 +8,7 @@ require 'active_support/core_ext/module/remove_method'
 require 'active_support/core_ext/array/extract_options'
 require 'action_controller/metal/exceptions'
 require 'action_dispatch/http/request'
+require 'action_dispatch/routing/endpoint'
 
 module ActionDispatch
   module Routing
@@ -20,24 +21,17 @@ module ActionDispatch
 
       PARAMETERS_KEY = 'action_dispatch.request.path_parameters'
 
-      class Dispatcher #:nodoc:
+      class Dispatcher < Routing::Endpoint #:nodoc:
         def initialize(defaults)
           @defaults = defaults
           @controller_class_names = ThreadSafe::Cache.new
         end
 
-        def call(env)
-          params = env[PARAMETERS_KEY]
+        def dispatcher?; true; end
 
-          # If any of the path parameters has an invalid encoding then
-          # raise since it's likely to trigger errors further on.
-          params.each do |key, value|
-            next unless value.respond_to?(:valid_encoding?)
-
-            unless value.valid_encoding?
-              raise ActionController::BadRequest, "Invalid parameter: #{key} => #{value}"
-            end
-          end
+        def serve(req)
+          req.check_path_parameters!
+          params = req.path_parameters
 
           prepare_params!(params)
 
@@ -46,7 +40,7 @@ module ActionDispatch
             return [404, {'X-Cascade' => 'pass'}, []]
           end
 
-          dispatch(controller, params[:action], env)
+          dispatch(controller, params[:action], req.env)
         end
 
         def prepare_params!(params)
@@ -703,12 +697,10 @@ module ActionDispatch
           end
           old_params = req.path_parameters
           req.path_parameters = old_params.merge params
-          dispatcher = route.app
-          if dispatcher.is_a?(Mapper::Constraints) && dispatcher.matches?(env)
-            dispatcher = dispatcher.app
-          end
+          app = route.app
+          if app.matches?(req) && app.dispatcher?
+            dispatcher = app.app
 
-          if dispatcher.is_a?(Dispatcher)
             if dispatcher.controller(params, false)
               dispatcher.prepare_params!(params)
               return params
diff --git a/actionpack/test/journey/router_test.rb b/actionpack/test/journey/router_test.rb
index db2d3bc10d..1a2106a3c5 100644
--- a/actionpack/test/journey/router_test.rb
+++ b/actionpack/test/journey/router_test.rb
@@ -9,6 +9,7 @@ module ActionDispatch
         def initialize
           super({})
         end
+        def dispatcher?; true; end
       end
 
       attr_reader :routes
@@ -217,13 +218,16 @@ module ActionDispatch
       end
 
       def test_clear_trailing_slash_from_script_name_on_root_unanchored_routes
+        route_set = Routing::RouteSet.new
+        mapper = Routing::Mapper.new route_set
+
         strexp = Router::Strexp.new("/", {}, ['/', '.', '?'], false)
         path   = Path::Pattern.new strexp
         app    = lambda { |env| [200, {}, ['success!']] }
-        @router.routes.add_route(app, path, {}, {}, {})
+        mapper.get '/weblog', :to => app
 
         env  = rack_env('SCRIPT_NAME' => '', 'PATH_INFO' => '/weblog')
-        resp = @router.serve rails_env env
+        resp = route_set.call env
         assert_equal ['success!'], resp.last
         assert_equal '', env['SCRIPT_NAME']
       end