Skip to content
Browse files

Merge branch 'constraints'

* constraints:
  rm reset_parameters because we automatically do it from 9ca4839
  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
  • Loading branch information...
2 parents babcd7d + 406b1b6 commit cfdab77d1fd39a887e9d94342e4e85f915a5b00b @tenderlove tenderlove committed May 27, 2014
View
11 actionpack/lib/action_controller/metal.rb
@@ -221,13 +221,18 @@ def self.middleware
# 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
View
4 actionpack/lib/action_dispatch/http/parameters.rb
@@ -43,10 +43,6 @@ def path_parameters
@env[Routing::RouteSet::PARAMETERS_KEY] ||= {}
end
- def reset_parameters #:nodoc:
- @env.delete("action_dispatch.request.parameters")
- end
-
private
# Convert nested Hash to HashWithIndifferentAccess
View
11 actionpack/lib/action_dispatch/http/request.rb
@@ -53,6 +53,17 @@ def initialize(env)
@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
View
10 actionpack/lib/action_dispatch/journey/route.rb
@@ -16,14 +16,6 @@ def initialize(name, app, path, constraints, defaults = {})
@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 @@ def glob?
end
def dispatcher?
- @dispatcher
+ @app.dispatcher?
end
def matches?(request)
View
2 actionpack/lib/action_dispatch/journey/router.rb
@@ -39,7 +39,7 @@ def serve(req)
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
View
10 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
View
15 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 @@ def internal?
end
def engine?
- rack_app && rack_app.respond_to?(:routes)
+ rack_app.respond_to?(:routes)
end
end
View
43 actionpack/lib/action_dispatch/routing/mapper.rb
@@ -6,6 +6,7 @@
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 @@ class Mapper
: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 @@ def normalize_conditions!
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 @@ def strexp
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
View
18 actionpack/lib/action_dispatch/routing/redirection.rb
@@ -3,28 +3,26 @@
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)
@status = status
@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
View
28 actionpack/lib/action_dispatch/routing/route_set.rb
@@ -8,6 +8,7 @@
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 @@ class RouteSet #:nodoc:
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 @@ def call(env)
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 @@ def recognize_path(path, environment = {})
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
View
8 actionpack/test/journey/router_test.rb
@@ -9,6 +9,7 @@ class StubDispatcher < Routing::RouteSet::Dispatcher
def initialize
super({})
end
+ def dispatcher?; true; end
end
attr_reader :routes
@@ -217,13 +218,16 @@ def test_X_Cascade
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

0 comments on commit cfdab77

Please sign in to comment.
Something went wrong with that request. Please try again.