diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 931313612cbb8e2db02950af5842c87ee7896a83..de256a59ee534c944c52e918c86817fc96e1288d 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,10 @@ +* Refactor handling of `:action` default in routing so that the request's + `path_parameters` is not mutated when generating a route. + + Fixes #23019. + + *Andrew White* + * Add request encoding and response parsing to integration tests. What previously was: diff --git a/actionpack/lib/action_dispatch/journey/formatter.rb b/actionpack/lib/action_dispatch/journey/formatter.rb index 0323360faa035b23b42d7f235bbe6305f7f1c203..200477b0027a296141a73a83397187191e43fd0b 100644 --- a/actionpack/lib/action_dispatch/journey/formatter.rb +++ b/actionpack/lib/action_dispatch/journey/formatter.rb @@ -32,8 +32,13 @@ def generate(name, options, path_parameters, parameterize = nil) defaults = route.defaults required_parts = route.required_parts - parameterized_parts.keep_if do |key, value| - (defaults[key].nil? && value.present?) || value.to_s != defaults[key].to_s || required_parts.include?(key) + + route.parts.reverse_each do |key| + break if defaults[key].nil? && parameterized_parts[key].present? + break if parameterized_parts[key].to_s != defaults[key].to_s + break if required_parts.include?(key) + + parameterized_parts.delete(key) end return [route.format(parameterized_parts), params] diff --git a/actionpack/lib/action_dispatch/routing/inspector.rb b/actionpack/lib/action_dispatch/routing/inspector.rb index 983f1daeb3d4da1332203d9c71f6e7cb89aa0c99..4ee5717b3ed678b0c3c69acfbe32fd1a7afbde3b 100644 --- a/actionpack/lib/action_dispatch/routing/inspector.rb +++ b/actionpack/lib/action_dispatch/routing/inspector.rb @@ -33,11 +33,11 @@ def reqs end def controller - requirements[:controller] || ':controller' + parts.include?(:controller) ? ':controller' : requirements[:controller] end def action - requirements[:action] || ':action' + parts.include?(:action) ? ':action' : requirements[:action] end def internal? diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index afbaa45d208d9fa3a53f85072b0948c23cba4dca..92205a89f6c3aa499c3a12e1072aa672e63b0747 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -137,6 +137,10 @@ def initialize(set, ast, defaults, controller, default_action, modyoule, to, for @conditions = Hash[conditions] @defaults = formats[:defaults].merge(@defaults).merge(normalize_defaults(options)) + if path_params.include?(:action) && !@requirements.key?(:action) + @defaults[:action] ||= 'index' + end + @required_defaults = (split_options[:required_defaults] || []).map(&:first) end diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 310e98f584e9e53e347fe6a0b0bef22d53b9ef48..de291d249bdc3a52576d6b02fc9e1508e2d30f8a 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -533,12 +533,10 @@ def initialize(named_route, options, recall, set) @recall = recall @set = set - normalize_recall! normalize_options! normalize_controller_action_id! use_relative_controller! normalize_controller! - normalize_action! end def controller @@ -557,11 +555,6 @@ def use_recall_for(key) end end - # Set 'index' as default action for recall - def normalize_recall! - @recall[:action] ||= 'index' - end - def normalize_options! # If an explicit :controller was given, always make :action explicit # too, so that action expiry works as expected for things like @@ -615,13 +608,6 @@ def normalize_controller! end end - # Move 'index' action from options to recall - def normalize_action! - if @options[:action] == 'index'.freeze - @recall[:action] = @options.delete(:action) - end - end - # Generates a path from routes, returns [path, params]. # If no route is generated the formatter will raise ActionController::UrlGenerationError def generate diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb index a39fede5b9e97e24cb5fd89fb54029105b72ae16..9dabd58d69899c242238c8d35f6ed5c8ffd2963d 100644 --- a/actionpack/test/controller/routing_test.rb +++ b/actionpack/test/controller/routing_test.rb @@ -1964,11 +1964,11 @@ def test_generate_extras def test_extras params = {:controller => 'people'} assert_equal [], @routes.extra_keys(params) - assert_equal({:controller => 'people'}, params) + assert_equal({:controller => 'people', :action => 'index'}, params) params = {:controller => 'people', :foo => 'bar'} assert_equal [:foo], @routes.extra_keys(params) - assert_equal({:controller => 'people', :foo => 'bar'}, params) + assert_equal({:controller => 'people', :action => 'index', :foo => 'bar'}, params) params = {:controller => 'people', :action => 'create', :person => { :name => 'Josh'}} assert_equal [:person], @routes.extra_keys(params) diff --git a/actionpack/test/dispatch/routing/inspector_test.rb b/actionpack/test/dispatch/routing/inspector_test.rb index f72a87b9949b4af4d9220b67439698e116d5d31d..642769681bb119c23b4c78c4651b9fe9fca7ba29 100644 --- a/actionpack/test/dispatch/routing/inspector_test.rb +++ b/actionpack/test/dispatch/routing/inspector_test.rb @@ -339,7 +339,7 @@ def test_regression_route_with_controller_regexp end assert_equal ["Prefix Verb URI Pattern Controller#Action", - " GET /:controller(/:action) (?-mix:api\\/[^\\/]+)#:action"], output + " GET /:controller(/:action) :controller#:action"], output end def test_inspect_routes_shows_resources_route_when_assets_disabled diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb index 5ead9357ae7e21e9c8ab63c973ffa25f60d3e006..02cd640fa5b0ebf5b9e314e7bdb2651de8dcf78f 100644 --- a/actionpack/test/dispatch/routing_test.rb +++ b/actionpack/test/dispatch/routing_test.rb @@ -3964,16 +3964,6 @@ def app; APP end end class TestMultipleNestedController < ActionDispatch::IntegrationTest - module ::Foo - module Bar - class BazController < ActionController::Base - def index - render :inline => "<%= url_for :controller => '/pooh', :action => 'index' %>" - end - end - end - end - Routes = ActionDispatch::Routing::RouteSet.new.tap do |app| app.draw do namespace :foo do @@ -3985,7 +3975,18 @@ def index end end - include Routes.url_helpers + module ::Foo + module Bar + class BazController < ActionController::Base + include Routes.url_helpers + + def index + render :inline => "<%= url_for :controller => '/pooh', :action => 'index' %>" + end + end + end + end + APP = build_app Routes def app; APP end @@ -4717,3 +4718,42 @@ def assert_params(params) assert_equal(params, request.path_parameters) end end + +class TestPathParameters < ActionDispatch::IntegrationTest + Routes = ActionDispatch::Routing::RouteSet.new.tap do |app| + app.draw do + scope module: 'test_path_parameters' do + scope ':locale', locale: /en|ar/ do + root to: 'home#index' + get '/about', to: 'pages#about' + end + end + + get ':controller(/:action/(:id))' + end + end + + class HomeController < ActionController::Base + include Routes.url_helpers + + def index + render inline: "<%= root_path %>" + end + end + + class PagesController < ActionController::Base + include Routes.url_helpers + + def about + render inline: "<%= root_path(locale: :ar) %> | <%= url_for(locale: :ar) %>" + end + end + + APP = build_app Routes + def app; APP end + + def test_path_parameters_are_not_mutated + get '/en/about' + assert_equal "/ar | /ar/about", @response.body + end +end