From 8037d7eacd14376d2c093d27f5b7d18fed085916 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Mon, 8 Jun 2015 17:18:06 -0700 Subject: extract required_defaults from the conditions hash before constructing the route this way we can remove the strange "respond_to?" conditional in the `matches?` loop --- actionpack/lib/action_dispatch/journey/route.rb | 7 ++- actionpack/lib/action_dispatch/journey/routes.rb | 4 +- .../lib/action_dispatch/routing/route_set.rb | 5 ++- actionpack/test/journey/route_test.rb | 26 +++++------ actionpack/test/journey/router_test.rb | 52 +++++++++++----------- actionpack/test/journey/routes_test.rb | 18 ++++---- 6 files changed, 56 insertions(+), 56 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/journey/route.rb b/actionpack/lib/action_dispatch/journey/route.rb index 4698ff8cc7..cbc985640a 100644 --- a/actionpack/lib/action_dispatch/journey/route.rb +++ b/actionpack/lib/action_dispatch/journey/route.rb @@ -11,7 +11,7 @@ module ActionDispatch ## # +path+ is a path constraint. # +constraints+ is a hash of constraints to be applied to this route. - def initialize(name, app, path, constraints, defaults = {}) + def initialize(name, app, path, constraints, required_defaults, defaults) @name = name @app = app @path = path @@ -19,6 +19,7 @@ module ActionDispatch @constraints = constraints @defaults = defaults @required_defaults = nil + @_required_defaults = required_defaults || [] @required_parts = nil @parts = nil @decorated_ast = nil @@ -73,7 +74,7 @@ module ActionDispatch end def required_default?(key) - (constraints[:required_defaults] || []).include?(key) + @_required_defaults.include?(key) end def required_defaults @@ -92,8 +93,6 @@ module ActionDispatch def matches?(request) constraints.all? do |method, value| - next true unless request.respond_to?(method) - case value when Regexp, String value === request.send(method).to_s diff --git a/actionpack/lib/action_dispatch/journey/routes.rb b/actionpack/lib/action_dispatch/journey/routes.rb index 6325dfa3dd..5990964b57 100644 --- a/actionpack/lib/action_dispatch/journey/routes.rb +++ b/actionpack/lib/action_dispatch/journey/routes.rb @@ -63,8 +63,8 @@ module ActionDispatch end # Add a route to the routing table. - def add_route(app, path, conditions, defaults, name = nil) - route = Route.new(name, app, path, conditions, defaults) + def add_route(app, path, conditions, required_defaults, defaults, name = nil) + route = Route.new(name, app, path, conditions, required_defaults, defaults) route.precedence = routes.length routes << route diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index d0d8ded515..1e950d13f8 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -511,10 +511,11 @@ module ActionDispatch path = conditions.delete :path_info ast = conditions.delete :parsed_path_info + required_defaults = conditions.delete :required_defaults path = build_path(path, ast, requirements, anchor) conditions = build_conditions(conditions, path.names.map(&:to_sym)) - route = @set.add_route(app, path, conditions, defaults, name) + route = @set.add_route(app, path, conditions, required_defaults, defaults, name) named_routes[name] = route if name route end @@ -563,7 +564,7 @@ module ActionDispatch end conditions.keep_if do |k, _| - k == :action || k == :controller || k == :required_defaults || + k == :action || k == :controller || request_class.public_method_defined?(k) || path_values.include?(k) end end diff --git a/actionpack/test/journey/route_test.rb b/actionpack/test/journey/route_test.rb index 0980df06e8..eff96a0abc 100644 --- a/actionpack/test/journey/route_test.rb +++ b/actionpack/test/journey/route_test.rb @@ -7,7 +7,7 @@ module ActionDispatch app = Object.new path = Path::Pattern.from_string '/:controller(/:action(/:id(.:format)))' defaults = {} - route = Route.new("name", app, path, {}, defaults) + route = Route.new("name", app, path, {}, [], defaults) assert_equal app, route.app assert_equal path, route.path @@ -18,7 +18,7 @@ module ActionDispatch app = Object.new path = Path::Pattern.from_string '/:controller(/:action(/:id(.:format)))' defaults = {} - route = Route.new("name", app, path, {}, defaults) + route = Route.new("name", app, path, {}, [], defaults) route.ast.grep(Nodes::Terminal).each do |node| assert_equal route, node.memo @@ -29,27 +29,27 @@ module ActionDispatch strexp = Router::Strexp.build(':name', { name: /love/ }, ['/']) path = Path::Pattern.new strexp defaults = { name: 'tender' } - route = Route.new('name', nil, path, nil, defaults) + route = Route.new('name', nil, path, nil, [], defaults) assert_equal(/love/, route.requirements[:name]) end def test_ip_address path = Path::Pattern.from_string '/messages/:id(.:format)' - route = Route.new("name", nil, path, {:ip => '192.168.1.1'}, + route = Route.new("name", nil, path, {:ip => '192.168.1.1'}, [], { :controller => 'foo', :action => 'bar' }) assert_equal '192.168.1.1', route.ip end def test_default_ip path = Path::Pattern.from_string '/messages/:id(.:format)' - route = Route.new("name", nil, path, {}, + route = Route.new("name", nil, path, {}, [], { :controller => 'foo', :action => 'bar' }) assert_equal(//, route.ip) end def test_format_with_star path = Path::Pattern.from_string '/:controller/*extra' - route = Route.new("name", nil, path, {}, + route = Route.new("name", nil, path, {}, [], { :controller => 'foo', :action => 'bar' }) assert_equal '/foo/himom', route.format({ :controller => 'foo', @@ -59,7 +59,7 @@ module ActionDispatch def test_connects_all_match path = Path::Pattern.from_string '/:controller(/:action(/:id(.:format)))' - route = Route.new("name", nil, path, {:action => 'bar'}, { :controller => 'foo' }) + route = Route.new("name", nil, path, {:action => 'bar'}, [], { :controller => 'foo' }) assert_equal '/foo/bar/10', route.format({ :controller => 'foo', @@ -70,34 +70,34 @@ module ActionDispatch def test_extras_are_not_included_if_optional path = Path::Pattern.from_string '/page/:id(/:action)' - route = Route.new("name", nil, path, { }, { :action => 'show' }) + route = Route.new("name", nil, path, { }, [], { :action => 'show' }) assert_equal '/page/10', route.format({ :id => 10 }) end def test_extras_are_not_included_if_optional_with_parameter path = Path::Pattern.from_string '(/sections/:section)/pages/:id' - route = Route.new("name", nil, path, { }, { :action => 'show' }) + route = Route.new("name", nil, path, { }, [], { :action => 'show' }) assert_equal '/pages/10', route.format({:id => 10}) end def test_extras_are_not_included_if_optional_parameter_is_nil path = Path::Pattern.from_string '(/sections/:section)/pages/:id' - route = Route.new("name", nil, path, { }, { :action => 'show' }) + route = Route.new("name", nil, path, { }, [], { :action => 'show' }) assert_equal '/pages/10', route.format({:id => 10, :section => nil}) end def test_score - constraints = {:required_defaults => [:controller, :action]} + constraints = {} defaults = {:controller=>"pages", :action=>"show"} path = Path::Pattern.from_string "/page/:id(/:action)(.:format)" - specific = Route.new "name", nil, path, constraints, defaults + specific = Route.new "name", nil, path, constraints, [:controller, :action], defaults path = Path::Pattern.from_string "/:controller(/:action(/:id))(.:format)" - generic = Route.new "name", nil, path, constraints + generic = Route.new "name", nil, path, constraints, [], {} knowledge = {:id=>20, :controller=>"pages", :action=>"show"} diff --git a/actionpack/test/journey/router_test.rb b/actionpack/test/journey/router_test.rb index a134e343cc..802fb93c69 100644 --- a/actionpack/test/journey/router_test.rb +++ b/actionpack/test/journey/router_test.rb @@ -35,7 +35,7 @@ module ActionDispatch exp = Router::Strexp.build '/foo-bar-baz', {}, ['/.?'] path = Path::Pattern.new exp - routes.add_route nil, path, {}, {:id => nil}, {} + routes.add_route nil, path, {}, [], {:id => nil}, {} env = rails_env 'PATH_INFO' => '/foo-bar-baz' called = false @@ -52,7 +52,7 @@ module ActionDispatch exp = Router::Strexp.build '/%E3%81%BB%E3%81%92', {}, ['/.?'] path = Path::Pattern.new exp - routes.add_route nil, path, {}, {:id => nil}, {} + routes.add_route nil, path, {}, [], {:id => nil}, {} env = rails_env 'PATH_INFO' => '/%E3%81%BB%E3%81%92' called = false @@ -71,7 +71,7 @@ module ActionDispatch exp = Router::Strexp.build '/foo(/:id)', {}, ['/.?'] path = Path::Pattern.new exp - routes.add_route nil, path, requirements, {:id => nil}, {} + routes.add_route nil, path, requirements, [], {:id => nil}, {} env = rails_env({'PATH_INFO' => '/foo/10'}, klass) router.recognize(env) do |r, params| @@ -91,7 +91,7 @@ module ActionDispatch exp = Router::Strexp.build '/foo(/:id)', {}, ['/.?'] path = Path::Pattern.new exp - router.routes.add_route nil, path, requirements, {:id => nil}, {} + router.routes.add_route nil, path, requirements, [], {:id => nil}, {} env = rails_env({'PATH_INFO' => '/foo/10'}, klass) router.recognize(env) do |r, params| @@ -118,7 +118,7 @@ module ActionDispatch exp = Router::Strexp.build '/bar', {}, ['/.?'] path = Path::Pattern.new exp - routes.add_route nil, path, {}, {}, {} + routes.add_route nil, path, {}, [], {}, {} env = rails_env({'PATH_INFO' => '/foo', 'custom.path_info' => '/bar'}, CustomPathRequest) @@ -192,7 +192,7 @@ module ActionDispatch route_name = "gorby_thunderhorse" pattern = Router::Strexp.build("/foo/:id", { :id => /\d+/ }, ['/', '.', '?'], false) path = Path::Pattern.new pattern - @router.routes.add_route nil, path, {}, {}, route_name + @router.routes.add_route nil, path, {}, [], {}, route_name error = assert_raises(ActionController::UrlGenerationError) do @formatter.generate(route_name, { }, { }) @@ -234,7 +234,7 @@ module ActionDispatch def test_defaults_merge_correctly path = Path::Pattern.from_string '/foo(/:id)' - @router.routes.add_route nil, path, {}, {:id => nil}, {} + @router.routes.add_route nil, path, {}, [], {:id => nil}, {} env = rails_env 'PATH_INFO' => '/foo/10' @router.recognize(env) do |r, params| @@ -317,7 +317,7 @@ module ActionDispatch def test_nil_path_parts_are_ignored path = Path::Pattern.from_string "/:controller(/:action(.:format))" - @router.routes.add_route @app, path, {}, {}, {} + @router.routes.add_route @app, path, {}, [], {}, {} params = { :controller => "tasks", :format => nil } extras = { :action => 'lol' } @@ -332,7 +332,7 @@ module ActionDispatch str = Router::Strexp.build("/", Hash[params], ['/', '.', '?'], true) path = Path::Pattern.new str - @router.routes.add_route @app, path, {}, {}, {} + @router.routes.add_route @app, path, {}, [], {}, {} path, _ = @formatter.generate(nil, Hash[params], {}) assert_equal '/', path @@ -340,7 +340,7 @@ module ActionDispatch def test_generate_calls_param_proc path = Path::Pattern.from_string '/:controller(/:action)' - @router.routes.add_route @app, path, {}, {}, {} + @router.routes.add_route @app, path, {}, [], {}, {} parameterized = [] params = [ [:controller, "tasks"], @@ -357,7 +357,7 @@ module ActionDispatch def test_generate_id path = Path::Pattern.from_string '/:controller(/:action)' - @router.routes.add_route @app, path, {}, {}, {} + @router.routes.add_route @app, path, {}, [], {}, {} path, params = @formatter.generate( nil, {:id=>1, :controller=>"tasks", :action=>"show"}, {}) @@ -367,7 +367,7 @@ module ActionDispatch def test_generate_escapes path = Path::Pattern.from_string '/:controller(/:action)' - @router.routes.add_route @app, path, {}, {}, {} + @router.routes.add_route @app, path, {}, [], {}, {} path, _ = @formatter.generate(nil, { :controller => "tasks", @@ -378,7 +378,7 @@ module ActionDispatch def test_generate_escapes_with_namespaced_controller path = Path::Pattern.from_string '/:controller(/:action)' - @router.routes.add_route @app, path, {}, {}, {} + @router.routes.add_route @app, path, {}, [], {}, {} path, _ = @formatter.generate( nil, { :controller => "admin/tasks", @@ -389,7 +389,7 @@ module ActionDispatch def test_generate_extra_params path = Path::Pattern.from_string '/:controller(/:action)' - @router.routes.add_route @app, path, {}, {}, {} + @router.routes.add_route @app, path, {}, [], {}, {} path, params = @formatter.generate( nil, { :id => 1, @@ -403,7 +403,7 @@ module ActionDispatch def test_generate_missing_keys_no_matches_different_format_keys path = Path::Pattern.from_string '/:controller/:action/:name' - @router.routes.add_route @app, path, {}, {}, {} + @router.routes.add_route @app, path, {}, [], {}, {} primarty_parameters = { :id => 1, :controller => "tasks", @@ -430,7 +430,7 @@ module ActionDispatch def test_generate_uses_recall_if_needed path = Path::Pattern.from_string '/:controller(/:action(/:id))' - @router.routes.add_route @app, path, {}, {}, {} + @router.routes.add_route @app, path, {}, [], {}, {} path, params = @formatter.generate( nil, @@ -442,7 +442,7 @@ module ActionDispatch def test_generate_with_name path = Path::Pattern.from_string '/:controller(/:action)' - @router.routes.add_route @app, path, {}, {}, "tasks" + @router.routes.add_route @app, path, {}, [], {}, "tasks" path, params = @formatter.generate( "tasks", @@ -460,7 +460,7 @@ module ActionDispatch define_method("test_recognize_#{expected.keys.map(&:to_s).join('_')}") do path = Path::Pattern.from_string "/:controller(/:action(/:id))" app = Object.new - route = @router.routes.add_route(app, path, {}, {}, {}) + route = @router.routes.add_route(app, path, {}, [], {}, {}) env = rails_env 'PATH_INFO' => request_path called = false @@ -482,7 +482,7 @@ module ActionDispatch define_method("test_recognize_#{name}") do path = Path::Pattern.from_string '/:segment/*splat' app = Object.new - route = @router.routes.add_route(app, path, {}, {}, {}) + route = @router.routes.add_route(app, path, {}, [], {}, {}) env = rails_env 'PATH_INFO' => request_path called = false @@ -505,7 +505,7 @@ module ActionDispatch ) path = Path::Pattern.new strexp app = Object.new - route = @router.routes.add_route(app, path, {}, {}, {}) + route = @router.routes.add_route(app, path, {}, [], {}, {}) env = rails_env 'PATH_INFO' => '/admin/users/show/10' called = false @@ -526,7 +526,7 @@ module ActionDispatch def test_recognize_literal path = Path::Pattern.from_string "/books(/:action(.:format))" app = Object.new - route = @router.routes.add_route(app, path, {}, {:controller => 'books'}) + route = @router.routes.add_route(app, path, {}, [], {:controller => 'books'}) env = rails_env 'PATH_INFO' => '/books/list.rss' expected = { :controller => 'books', :action => 'list', :format => 'rss' } @@ -544,7 +544,7 @@ module ActionDispatch path = Path::Pattern.from_string "/books(/:action(.:format))" app = Object.new conditions = { request_method: 'HEAD' } - @router.routes.add_route(app, path, conditions, {}) + @router.routes.add_route(app, path, conditions, [], {}) env = rails_env( 'PATH_INFO' => '/books/list.rss', @@ -565,7 +565,7 @@ module ActionDispatch conditions = { :request_method => 'GET' } - @router.routes.add_route(app, path, conditions, {}) + @router.routes.add_route(app, path, conditions, [], {}) env = rails_env 'PATH_INFO' => '/books/list.rss', "REQUEST_METHOD" => "HEAD" @@ -582,7 +582,7 @@ module ActionDispatch path = Path::Pattern.from_string "/books(/:action(.:format))" app = Object.new conditions = { request_method: 'GET' } - @router.routes.add_route(app, path, conditions, {}) + @router.routes.add_route(app, path, conditions, [], {}) env = rails_env 'PATH_INFO' => '/books/list.rss', "REQUEST_METHOD" => "POST" @@ -597,7 +597,7 @@ module ActionDispatch conditions = conditions.dup conditions[:request_method] = 'POST' - post = @router.routes.add_route(app, path, conditions, {}) + post = @router.routes.add_route(app, path, conditions, [], {}) called = false @router.recognize(env) do |r, params| @@ -617,7 +617,7 @@ module ActionDispatch else path = Path::Pattern.new path end - router.routes.add_route @app, path, {}, {}, {} + router.routes.add_route @app, path, {}, [], {}, {} end end diff --git a/actionpack/test/journey/routes_test.rb b/actionpack/test/journey/routes_test.rb index 2a1c1c337e..b9dac8751c 100644 --- a/actionpack/test/journey/routes_test.rb +++ b/actionpack/test/journey/routes_test.rb @@ -13,7 +13,7 @@ module ActionDispatch path = Path::Pattern.new exp requirements = { :hello => /world/ } - routes.add_route nil, path, requirements, {:id => nil}, {} + routes.add_route nil, path, requirements, [], {:id => nil}, {} assert_not routes.empty? assert_equal 1, routes.length @@ -26,9 +26,9 @@ module ActionDispatch routes = Routes.new path = Path::Pattern.from_string '/hello' - routes.add_route nil, path, {}, {}, {} + routes.add_route nil, path, {}, [], {}, {} ast = routes.ast - routes.add_route nil, path, {}, {}, {} + routes.add_route nil, path, {}, [], {}, {} assert_not_equal ast, routes.ast end @@ -36,16 +36,16 @@ module ActionDispatch routes = Routes.new path = Path::Pattern.from_string '/hello' - routes.add_route nil, path, {}, {}, {} + routes.add_route nil, path, {}, [], {}, {} sim = routes.simulator - routes.add_route nil, path, {}, {}, {} + routes.add_route nil, path, {}, [], {}, {} assert_not_equal sim, routes.simulator end def test_partition_route path = Path::Pattern.from_string '/hello' - anchored_route = @routes.add_route nil, path, {}, {}, {} + anchored_route = @routes.add_route nil, path, {}, [], {}, {} assert_equal [anchored_route], @routes.anchored_routes assert_equal [], @routes.custom_routes @@ -54,7 +54,7 @@ module ActionDispatch ) path = Path::Pattern.new strexp - custom_route = @routes.add_route nil, path, {}, {}, {} + custom_route = @routes.add_route nil, path, {}, [], {}, {} assert_equal [custom_route], @routes.custom_routes assert_equal [anchored_route], @routes.anchored_routes end @@ -65,8 +65,8 @@ module ActionDispatch one = Path::Pattern.from_string '/hello' two = Path::Pattern.from_string '/aaron' - routes.add_route nil, one, {}, {}, 'aaron' - routes.add_route nil, two, {}, {}, 'aaron' + routes.add_route nil, one, {}, [], {}, 'aaron' + routes.add_route nil, two, {}, [], {}, 'aaron' assert_equal '/hello', routes.named_routes['aaron'].path.spec.to_s end -- cgit v1.2.3