diff options
Diffstat (limited to 'actionpack')
| -rw-r--r-- | actionpack/CHANGELOG.md | 12 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/metal.rb | 4 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/http/url.rb | 20 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/journey/formatter.rb | 5 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/journey/route.rb | 2 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/journey/router.rb | 67 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/journey/visualizer/index.html.erb | 4 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/routing/inspector.rb | 2 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/routing/mapper.rb | 58 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/routing/route_set.rb | 21 | ||||
| -rw-r--r-- | actionpack/test/dispatch/routing_test.rb | 9 | ||||
| -rw-r--r-- | actionpack/test/dispatch/url_generation_test.rb | 18 | ||||
| -rw-r--r-- | actionpack/test/journey/router_test.rb | 45 |
13 files changed, 126 insertions, 141 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 2b4ff7f03d..86e98c011c 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,9 +1,14 @@ +* Fix URL generation with `:trailing_slash` such that it does not add + a trailing slash after `.:format` + + *Dan Langevin* + * Build full URI as string when processing path in integration tests for performance reasons. *Guo Xiang Tan* -* Fix 'Stack level too deep' when rendering `head :ok` in an action method +* Fix `'Stack level too deep'` when rendering `head :ok` in an action method called 'status' in a controller. Fixes #13905. @@ -71,7 +76,7 @@ 4. Use `escape_segment` rather than `escape_path` in URL generation For point 4 there are two exceptions. Firstly, when a route uses wildcard segments - (e.g. *foo) then we use `escape_path` as the value may contain '/' characters. This + (e.g. `*foo`) then we use `escape_path` as the value may contain '/' characters. This means that wildcard routes can't be optimized. Secondly, if a `:controller` segment is used in the path then this uses `escape_path` as the controller may be namespaced. @@ -101,7 +106,7 @@ *Andrew White*, *James Coglan* -* Append link to bad code to backtrace when exception is SyntaxError. +* Append link to bad code to backtrace when exception is `SyntaxError`. *Boris Kuznetsov* @@ -131,4 +136,5 @@ *Tony Wooster* + Please check [4-1-stable](https://github.com/rails/rails/blob/4-1-stable/actionpack/CHANGELOG.md) for previous changes. diff --git a/actionpack/lib/action_controller/metal.rb b/actionpack/lib/action_controller/metal.rb index 696fbf6e09..70ca99f01c 100644 --- a/actionpack/lib/action_controller/metal.rb +++ b/actionpack/lib/action_controller/metal.rb @@ -30,10 +30,8 @@ module ActionController end end - def build(action, app=nil, &block) - app ||= block + def build(action, app = Proc.new) action = action.to_s - raise "MiddlewareStack#build requires an app" unless app middlewares.reverse.inject(app) do |a, middleware| middleware.valid?(action) ? middleware.build(a) : a diff --git a/actionpack/lib/action_dispatch/http/url.rb b/actionpack/lib/action_dispatch/http/url.rb index a6c17f50a5..4cba4f5f37 100644 --- a/actionpack/lib/action_dispatch/http/url.rb +++ b/actionpack/lib/action_dispatch/http/url.rb @@ -37,13 +37,7 @@ module ActionDispatch path = options[:script_name].to_s.chomp("/") path << options[:path].to_s - if options[:trailing_slash] - if path.include?('?') - path.sub!(/\?/, '/\&') - else - path.sub!(/[^\/]\z|\A\z/, '\&/') - end - end + add_trailing_slash(path) if options[:trailing_slash] result = path @@ -66,6 +60,18 @@ module ActionDispatch private + def add_trailing_slash(path) + # includes querysting + if path.include?('?') + path.sub!(/\?/, '/\&') + # does not have a .format + elsif !path.include?(".") + path.sub!(/[^\/]\z|\A\z/, '\&/') + end + + path + end + def build_host_url(options) if match = options[:host].match(HOST_REGEXP) options[:protocol] ||= match[1] unless options[:protocol] == false diff --git a/actionpack/lib/action_dispatch/journey/formatter.rb b/actionpack/lib/action_dispatch/journey/formatter.rb index 8b3081d8fe..6d58323789 100644 --- a/actionpack/lib/action_dispatch/journey/formatter.rb +++ b/actionpack/lib/action_dispatch/journey/formatter.rb @@ -132,11 +132,6 @@ module ActionDispatch } end - # Returns +true+ if no missing keys are present, otherwise +false+. - def verify_required_parts!(route, parts) - missing_keys(route, parts).empty? - end - def build_cache root = { ___routes: [] } routes.each_with_index do |route, i| diff --git a/actionpack/lib/action_dispatch/journey/route.rb b/actionpack/lib/action_dispatch/journey/route.rb index cc3c7f20cb..1ba91d548e 100644 --- a/actionpack/lib/action_dispatch/journey/route.rb +++ b/actionpack/lib/action_dispatch/journey/route.rb @@ -19,7 +19,7 @@ module ActionDispatch # 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. - while app.is_a?(Routing::Mapper::Constraints) do + if app.is_a?(Routing::Mapper::Constraints) app = app.app end @dispatcher = app.is_a?(Routing::RouteSet::Dispatcher) diff --git a/actionpack/lib/action_dispatch/journey/router.rb b/actionpack/lib/action_dispatch/journey/router.rb index c34b44409e..2ead6a4eb3 100644 --- a/actionpack/lib/action_dispatch/journey/router.rb +++ b/actionpack/lib/action_dispatch/journey/router.rb @@ -20,61 +20,30 @@ module ActionDispatch # :nodoc: VERSION = '2.0.0' - class NullReq # :nodoc: - attr_reader :env - attr_accessor :path_parameters - def initialize(env) - @env = env - @path_parameters = {} - end - - def request_method - env['REQUEST_METHOD'] - end - - def path_info - env['PATH_INFO'] - end - - def ip - env['REMOTE_ADDR'] - end - - def [](k) - env[k] - end - end - - attr_reader :request_class, :formatter attr_accessor :routes - def initialize(routes, options) - @options = options - @request_class = options[:request_class] || NullReq - @routes = routes + def initialize(routes) + @routes = routes end - def call(env) - env['PATH_INFO'] = Utils.normalize_path(env['PATH_INFO']) - - req = request_class.new(env) - - find_routes(env, req).each do |match, parameters, route| - set_params = req.path_parameters - script_name, path_info = env.values_at('SCRIPT_NAME', 'PATH_INFO') + def serve(req) + find_routes(req).each do |match, parameters, route| + set_params = req.path_parameters + path_info = req.path_info + script_name = req.script_name unless route.path.anchored - env['SCRIPT_NAME'] = (script_name.to_s + match.to_s).chomp('/') - env['PATH_INFO'] = match.post_match + req.script_name = (script_name.to_s + match.to_s).chomp('/') + req.path_info = match.post_match end req.path_parameters = set_params.merge parameters - status, headers, body = route.app.call(env) + status, headers, body = route.app.call(req.env) if 'pass' == headers['X-Cascade'] - env['SCRIPT_NAME'] = script_name - env['PATH_INFO'] = path_info + req.script_name = script_name + req.path_info = path_info req.path_parameters = set_params next end @@ -85,13 +54,11 @@ module ActionDispatch return [404, {'X-Cascade' => 'pass'}, ['Not Found']] end - def recognize(req) - rails_req = request_class.new(req.env) - - find_routes(req.env, rails_req).each do |match, parameters, route| + def recognize(rails_req) + find_routes(rails_req).each do |match, parameters, route| unless route.path.anchored - req.env['SCRIPT_NAME'] = match.to_s - req.env['PATH_INFO'] = match.post_match.sub(/^([^\/])/, '/\1') + rails_req.script_name = match.to_s + rails_req.path_info = match.post_match.sub(/^([^\/])/, '/\1') end yield(route, parameters) @@ -128,7 +95,7 @@ module ActionDispatch simulator.memos(path) { [] } end - def find_routes env, req + def find_routes req routes = filter_routes(req.path_info).concat custom_routes.find_all { |r| r.path.match(req.path_info) } diff --git a/actionpack/lib/action_dispatch/journey/visualizer/index.html.erb b/actionpack/lib/action_dispatch/journey/visualizer/index.html.erb index 6aff10956a..9b28a65200 100644 --- a/actionpack/lib/action_dispatch/journey/visualizer/index.html.erb +++ b/actionpack/lib/action_dispatch/journey/visualizer/index.html.erb @@ -2,13 +2,13 @@ <html> <head> <title><%= title %></title> - <link rel="stylesheet" href="https://raw.github.com/gist/1706081/af944401f75ea20515a02ddb3fb43d23ecb8c662/reset.css" type="text/css"> + <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/meyer-reset/2.0/reset.css" type="text/css"> <style> <% stylesheets.each do |style| %> <%= style %> <% end %> </style> - <script src="https://raw.github.com/gist/1706081/df464722a05c3c2bec450b7b5c8240d9c31fa52d/d3.min.js" type="text/javascript"></script> + <script src="https://cdnjs.cloudflare.com/ajax/libs/d3/3.4.8/d3.min.js" type="text/javascript"></script> </head> <body> <div id="wrapper"> diff --git a/actionpack/lib/action_dispatch/routing/inspector.rb b/actionpack/lib/action_dispatch/routing/inspector.rb index 71a0c5e826..2135b280da 100644 --- a/actionpack/lib/action_dispatch/routing/inspector.rb +++ b/actionpack/lib/action_dispatch/routing/inspector.rb @@ -16,7 +16,7 @@ module ActionDispatch @rack_app ||= begin class_name = app.class.name.to_s if class_name == "ActionDispatch::Routing::Mapper::Constraints" - rack_app(app.app) + app.app elsif ActionDispatch::Routing::Redirect === app || class_name !~ /^ActionDispatch::Routing/ app end diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 7e78b417fa..b33c5e0dfd 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -16,17 +16,18 @@ module ActionDispatch :shallow, :blocks, :defaults, :options] class Constraints #:nodoc: - def self.new(app, constraints, request = Rack::Request) - if constraints.any? - super(app, constraints, request) - else - app - end - end - attr_reader :app, :constraints def initialize(app, constraints, request) + # 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) + constraints += app.constraints + app = app.app + end + @app, @constraints, @request = app, constraints, request end @@ -57,12 +58,18 @@ module ActionDispatch WILDCARD_PATH = %r{\*([^/\)]+)\)?$} attr_reader :scope, :path, :options, :requirements, :conditions, :defaults + attr_reader :to, :default_controller, :default_action def initialize(set, scope, path, options) - @set, @scope, @path, @options = set, scope, path, options + @set, @scope, @path = set, scope, path @requirements, @conditions, @defaults = {}, {}, {} - normalize_options! + options = scope[:options].merge(options) if scope[:options] + @to = options[:to] + @default_controller = options[:controller] || scope[:controller] + @default_action = options[:action] || scope[:action] + + @options = normalize_options!(options) normalize_path! normalize_requirements! normalize_conditions! @@ -94,14 +101,13 @@ module ActionDispatch options[:format] != false && !path.include?(':format') && !path.end_with?('/') end - def normalize_options! - @options.reverse_merge!(scope[:options]) if scope[:options] + def normalize_options!(options) path_without_format = path.sub(/\(\.:format\)$/, '') # Add a constraint for wildcard route to make it non-greedy and match the # optional format part of the route by default - if path_without_format.match(WILDCARD_PATH) && @options[:format] != false - @options[$1.to_sym] ||= /.+?/ + if path_without_format.match(WILDCARD_PATH) && options[:format] != false + options[$1.to_sym] ||= /.+?/ end if path_without_format.match(':controller') @@ -111,10 +117,10 @@ module ActionDispatch # controllers with default routes like :controller/:action/:id(.:format), e.g: # GET /admin/products/show/1 # => { controller: 'admin/products', action: 'show', id: '1' } - @options[:controller] ||= /.+?/ + options[:controller] ||= /.+?/ end - @options.merge!(default_controller_and_action) + options.merge!(default_controller_and_action) end def normalize_requirements! @@ -210,7 +216,11 @@ module ActionDispatch end def app - Constraints.new(endpoint, blocks, @set.request_class) + if blocks.any? + Constraints.new(endpoint, blocks, @set.request_class) + else + endpoint + end end def default_controller_and_action @@ -301,19 +311,7 @@ module ActionDispatch end def dispatcher - Routing::RouteSet::Dispatcher.new(:defaults => defaults) - end - - def to - options[:to] - end - - def default_controller - options[:controller] || scope[:controller] - end - - def default_action - options[:action] || scope[:action] + Routing::RouteSet::Dispatcher.new(defaults) end end diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 8b4fd26ce2..924455bce2 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -21,9 +21,8 @@ module ActionDispatch PARAMETERS_KEY = 'action_dispatch.request.path_parameters' class Dispatcher #:nodoc: - def initialize(options={}) - @defaults = options[:defaults] - @glob_param = options.delete(:glob) + def initialize(defaults) + @defaults = defaults @controller_class_names = ThreadSafe::Cache.new end @@ -53,7 +52,6 @@ module ActionDispatch def prepare_params!(params) normalize_controller!(params) merge_default_action!(params) - split_glob_param!(params) if @glob_param end # If this is a default_controller (i.e. a controller specified by the user) @@ -89,10 +87,6 @@ module ActionDispatch def merge_default_action!(params) params[:action] ||= 'index' end - - def split_glob_param!(params) - params[@glob_param] = params[@glob_param].split('/').map { |v| URI.parser.unescape(v) } - end end # A NamedRouteCollection instance is a collection of named routes, and also @@ -302,8 +296,7 @@ module ActionDispatch @finalized = false @set = Journey::Routes.new - @router = Journey::Router.new(@set, { - :request_class => request_class}) + @router = Journey::Router.new @set @formatter = Journey::Formatter.new @set end @@ -683,7 +676,9 @@ module ActionDispatch end def call(env) - @router.call(env) + req = request_class.new(env) + req.path_info = Journey::Router::Utils.normalize_path(req.path_info) + @router.serve(req) end def recognize_path(path, environment = {}) @@ -697,7 +692,7 @@ module ActionDispatch raise ActionController::RoutingError, e.message end - req = @request_class.new(env) + req = request_class.new(env) @router.recognize(req) do |route, params| params.merge!(extras) params.each do |key, value| @@ -709,7 +704,7 @@ module ActionDispatch old_params = req.path_parameters req.path_parameters = old_params.merge params dispatcher = route.app - while dispatcher.is_a?(Mapper::Constraints) && dispatcher.matches?(env) do + if dispatcher.is_a?(Mapper::Constraints) && dispatcher.matches?(env) dispatcher = dispatcher.app end diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb index 051431ce26..a427113763 100644 --- a/actionpack/test/dispatch/routing_test.rb +++ b/actionpack/test/dispatch/routing_test.rb @@ -3368,15 +3368,14 @@ end class TestAltApp < ActionDispatch::IntegrationTest class AltRequest - attr_accessor :path_parameters + attr_accessor :path_parameters, :path_info, :script_name + attr_reader :env def initialize(env) @path_parameters = {} @env = env - end - - def path_info - "/" + @path_info = "/" + @script_name = "" end def request_method diff --git a/actionpack/test/dispatch/url_generation_test.rb b/actionpack/test/dispatch/url_generation_test.rb index 910ff8a80f..a4dfd0a63d 100644 --- a/actionpack/test/dispatch/url_generation_test.rb +++ b/actionpack/test/dispatch/url_generation_test.rb @@ -15,6 +15,8 @@ module TestUrlGeneration Routes.draw do get "/foo", :to => "my_route_generating#index", :as => :foo + resources :bars + mount MyRouteGeneratingController.action(:index), at: '/bar' end @@ -109,6 +111,22 @@ module TestUrlGeneration test "omit subdomain when key is blank" do assert_equal "http://example.com/foo", foo_url(subdomain: "") end + + test "generating URLs with trailing slashes" do + assert_equal "/bars.json", bars_path( + trailing_slash: true, + format: 'json' + ) + end + + test "generating URLS with querystring and trailing slashes" do + assert_equal "/bars.json?a=b", bars_path( + trailing_slash: true, + a: 'b', + format: 'json' + ) + end + end end diff --git a/actionpack/test/journey/router_test.rb b/actionpack/test/journey/router_test.rb index 9a8d644f7b..db2d3bc10d 100644 --- a/actionpack/test/journey/router_test.rb +++ b/actionpack/test/journey/router_test.rb @@ -5,23 +5,21 @@ module ActionDispatch module Journey class TestRouter < ActiveSupport::TestCase # TODO : clean up routing tests so we don't need this hack - class StubDispatcher < Routing::RouteSet::Dispatcher; end + class StubDispatcher < Routing::RouteSet::Dispatcher + def initialize + super({}) + end + end attr_reader :routes def setup @app = StubDispatcher.new @routes = Routes.new - @router = Router.new(@routes, {}) + @router = Router.new(@routes) @formatter = Formatter.new(@routes) end - def test_request_class_reader - klass = Object.new - router = Router.new(routes, :request_class => klass) - assert_equal klass, router.request_class - end - class FakeRequestFeeler < Struct.new(:env, :called) def new env self.env = env @@ -39,7 +37,7 @@ module ActionDispatch end def test_dashes - router = Router.new(routes, {}) + router = Router.new(routes) exp = Router::Strexp.new '/foo-bar-baz', {}, ['/.?'] path = Path::Pattern.new exp @@ -55,7 +53,7 @@ module ActionDispatch end def test_unicode - router = Router.new(routes, {}) + router = Router.new(routes) #match the escaped version of /ほげ exp = Router::Strexp.new '/%E3%81%BB%E3%81%92', {}, ['/.?'] @@ -73,7 +71,7 @@ module ActionDispatch def test_request_class_and_requirements_success klass = FakeRequestFeeler.new nil - router = Router.new(routes, {:request_class => klass }) + router = Router.new(routes) requirements = { :hello => /world/ } @@ -82,7 +80,7 @@ module ActionDispatch routes.add_route nil, path, requirements, {:id => nil}, {} - env = rails_env 'PATH_INFO' => '/foo/10' + env = rails_env({'PATH_INFO' => '/foo/10'}, klass) router.recognize(env) do |r, params| assert_equal({:id => '10'}, params) end @@ -93,7 +91,7 @@ module ActionDispatch def test_request_class_and_requirements_fail klass = FakeRequestFeeler.new nil - router = Router.new(routes, {:request_class => klass }) + router = Router.new(routes) requirements = { :hello => /mom/ } @@ -102,7 +100,7 @@ module ActionDispatch router.routes.add_route nil, path, requirements, {:id => nil}, {} - env = rails_env 'PATH_INFO' => '/foo/10' + env = rails_env({'PATH_INFO' => '/foo/10'}, klass) router.recognize(env) do |r, params| flunk 'route should not be found' end @@ -111,21 +109,26 @@ module ActionDispatch assert_equal env.env, klass.env end - class CustomPathRequest < Router::NullReq + class CustomPathRequest < ActionDispatch::Request def path_info env['custom.path_info'] end + + def path_info=(x) + env['custom.path_info'] = x + end end def test_request_class_overrides_path_info - router = Router.new(routes, {:request_class => CustomPathRequest }) + router = Router.new(routes) exp = Router::Strexp.new '/bar', {}, ['/.?'] path = Path::Pattern.new exp routes.add_route nil, path, {}, {}, {} - env = rails_env 'PATH_INFO' => '/foo', 'custom.path_info' => '/bar' + env = rails_env({'PATH_INFO' => '/foo', + 'custom.path_info' => '/bar'}, CustomPathRequest) recognized = false router.recognize(env) do |r, params| @@ -207,7 +210,7 @@ module ActionDispatch def test_X_Cascade add_routes @router, [ "/messages(.:format)" ] - resp = @router.call({ 'REQUEST_METHOD' => 'GET', 'PATH_INFO' => '/lol' }) + resp = @router.serve(rails_env({ 'REQUEST_METHOD' => 'GET', 'PATH_INFO' => '/lol' })) assert_equal ['Not Found'], resp.last assert_equal 'pass', resp[1]['X-Cascade'] assert_equal 404, resp.first @@ -220,7 +223,7 @@ module ActionDispatch @router.routes.add_route(app, path, {}, {}, {}) env = rack_env('SCRIPT_NAME' => '', 'PATH_INFO' => '/weblog') - resp = @router.call(env) + resp = @router.serve rails_env env assert_equal ['success!'], resp.last assert_equal '', env['SCRIPT_NAME'] end @@ -561,8 +564,8 @@ module ActionDispatch RailsEnv = Struct.new(:env) - def rails_env env - RailsEnv.new rack_env env + def rails_env env, klass = ActionDispatch::Request + klass.new env end def rack_env env |