Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use singular routes when plural routes do not exist #15725

Closed
wants to merge 1 commit into from
Closed

Use singular routes when plural routes do not exist #15725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 10 additions & 6 deletions actionpack/lib/action_dispatch/routing/polymorphic_routes.rb
View file
Open in desktop
Expand Up @@ -208,7 +208,7 @@ def self.polymorphic_method(recipient, record_or_hash_or_array, action, type, op
when nil
raise ArgumentError, "Nil location provided. Can't build URI."
else
method, args = builder.handle_model record_or_hash_or_array
method, args = builder.handle_model record_or_hash_or_array, recipient
end


Expand Down Expand Up @@ -243,24 +243,28 @@ def handle_class_call(target, klass)
target.send get_method_for_class klass
end

def handle_model(record)
def handle_model(record, target, plural = true)
args = []

model = record.to_model
name = if record.persisted?
args << model
name = if record.persisted? || !plural
args << model if plural
model.class.model_name.singular_route_key
else
@key_strategy.call model.class.model_name
end

named_route = prefix + "#{name}_#{suffix}"

[named_route, args]
if !plural || target.respond_to?(named_route)
[named_route, args]
else
handle_model(record, target, false)
end
end

def handle_model_call(target, model)
method, args = handle_model model
method, args = handle_model model, target
target.send(method, *args)
end

Expand Down
9 changes: 9 additions & 0 deletions actionpack/lib/action_dispatch/routing/route_set.rb
View file
Open in desktop
Expand Up @@ -210,6 +210,7 @@ def initialize(route, options)
def call(t, args)
controller_options = t.url_options
options = controller_options.merge @options
ensure_no_model_for_format(args)
hash = handle_positional_args(controller_options, args, options, @segment_keys)
t._routes.url_for(hash)
end
Expand All @@ -229,6 +230,14 @@ def handle_positional_args(controller_options, args, result, path_params)

result.merge!(inner_options)
end

def ensure_no_model_for_format(args)
if format_index = @segment_keys.index(:format)
if args[format_index].respond_to?(:to_model)
args.delete_at(format_index)
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is in a hot path for url generation - have you tested how much it will affect performance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not extensively enough, but I'm thinking this is the wrong way to go about it. Most of the places that we generate a URL, a singular resource is semantically different than a plural resource. e.g. if link_to @user generated '/user', that would be incorrect -- You're not linking to that user, you're linking to "the" user -- And it's not significantly more painful to do link_to :user.

So really the only cases where this matters are form_for and respond_with, where we need the object in addition to the URL that it lives at. The only similar case I can think of here is when we work with nested resources. However in that case, the user is still able to unambiguously give us all of the information required.

I'm not sure we can truly solve this in a way that works consistently and deterministically (especially when you bring nested resources into account). I think realistically, we should do one of these:

  • Decide that this is an uncommon case that's outside of the use case these methods are meant to handle, and that the user should manually pass the url.
  • Add form_for_singular and respond_with_singular, which uses a singular route rather than the plural.

In both cases, we should improve the error message to inform the user why it didn't work, and point them at how to fix it. My preference would be towards the first option. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pixeltrix Would still love your opinions on this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sgrif lets go for the first option and then if we make some kind of breakthrough with refactoring Mapper for 4.2 we can always revert it - thanks.

end

private
Expand Down
17 changes: 17 additions & 0 deletions actionpack/test/controller/redirect_test.rb
View file
Open in desktop
Expand Up @@ -275,6 +275,23 @@ def test_redirect_to_record
end
end

def test_redirect_to_singular_record
with_routing do |set|
set.draw do
resource :workshop
get ':controller/:action'
end

get :redirect_to_existing_record
assert_equal "http://test.host/workshop", redirect_to_url
assert_redirected_to Workshop.new(5)

get :redirect_to_new_record
assert_equal "http://test.host/workshop", redirect_to_url
assert_redirected_to Workshop.new(nil)
end
end

def test_redirect_to_nil
assert_raise(ActionController::ActionControllerError) do
get :redirect_to_nil
Expand Down
14 changes: 14 additions & 0 deletions actionview/test/template/form_helper_test.rb
View file
Open in desktop
Expand Up @@ -95,6 +95,8 @@ def @post.to_param; '123'; end
resources :comments
end

resource :comment

namespace :admin do
resources :posts do
resources :comments
Expand Down Expand Up @@ -3022,6 +3024,18 @@ def test_fields_for_returns_block_result
assert_equal "fields", output
end

def test_form_for_with_new_singular_resource
form_for(Comment.new) {}
expected = whole_form('/comment', 'new_comment', 'new_comment')
assert_equal expected, output_buffer
end

def test_form_for_with_persisted_singular_resource
form_for(Comment.new(123)) {}
expected = whole_form('/comment', 'edit_comment_123', 'edit_comment', method: :patch)
assert_equal expected, output_buffer
end

def test_form_for_only_instantiates_builder_once
initialization_count = 0
builder_class = Class.new(ActionView::Helpers::FormBuilder) do
Expand Down