Skip to content

Commit

Permalink
cause rails to correctly place optional path parameters
Browse files Browse the repository at this point in the history
previously, if you specify a url parameter that is part of the path as false it would include that part of the path as parameter at the end of the url instead of in the path for example:
`get "(/optional/:optional_id)/things" => "foo#foo", as: :things`
`things_path(optional_id: false)  # => /things?optional_id=false`

this is not the case for empty string,
`things_path(optional_id: '')  # => "/things"

this is due to a quark in how path parameters get removed from the parameters.

we where doing `(paramter || recall).nil?` which returns nil if both values are nil however it also return nil if one value is false and the other value is nil. i.e. `(false || nil).nil # => nil` which is confusing.

After this change, `true` and `false` will be treated the same when used as optional path parameters. meaning now,

```
get '(this/:my_bool)/that' as: :that

that_path(my_bool: true) # => `/this/true/that`
that_path(my_bool: false) # => `/this/false/that`
```
fixes: #42280

Co-authored-by: Ryuta Kamizono <kamipo@gmail.com>
  • Loading branch information
HParker and kamipo committed May 28, 2021
1 parent a8461b7 commit 98ed240
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 3 deletions.
4 changes: 2 additions & 2 deletions actionpack/lib/action_dispatch/journey/formatter.rb
Expand Up @@ -103,7 +103,7 @@ def extract_parameterized_parts(route, options, recall)
parameterized_parts = recall.merge(options)

keys_to_keep = route.parts.reverse_each.drop_while { |part|
!(options.key?(part) || route.scope_options.key?(part)) || (options[part] || recall[part]).nil?
!(options.key?(part) || route.scope_options.key?(part)) || (options[part].nil? && recall[part].nil?)
} | route.required_parts

parameterized_parts.delete_if do |bad_key, _|
Expand All @@ -118,7 +118,7 @@ def extract_parameterized_parts(route, options, recall)
end
end

parameterized_parts.keep_if { |_, v| v }
parameterized_parts.compact!
parameterized_parts
end

Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_dispatch/journey/visitors.rb
Expand Up @@ -40,7 +40,7 @@ def evaluate(hash)
@parameters.each do |index|
param = parts[index]
value = hash[param.name]
return "" unless value
return "" if value.nil?
parts[index] = param.escape value
end

Expand Down
29 changes: 29 additions & 0 deletions actionpack/test/dispatch/url_generation_test.rb
Expand Up @@ -16,6 +16,7 @@ def index

Routes.draw do
get "/foo", to: "my_route_generating#index", as: :foo
get "(/optional/:optional_id)/baz", to: "my_route_generating#index", as: :baz

resources :bars

Expand Down Expand Up @@ -127,6 +128,34 @@ def app
assert_equal "http://example.com/foo", foo_url(subdomain: "")
end

test "keep optional path parameter when given" do
assert_equal "http://www.example.com/optional/123/baz", baz_url(optional_id: 123)
end

test "keep optional path parameter when true" do
assert_equal "http://www.example.com/optional/true/baz", baz_url(optional_id: true)
end

test "omit optional path parameter when false" do
assert_equal "http://www.example.com/optional/false/baz", baz_url(optional_id: false)
end

test "omit optional path parameter when blank" do
assert_equal "http://www.example.com/baz", baz_url(optional_id: "")
end

test "keep positional path parameter when true" do
assert_equal "http://www.example.com/optional/true/baz", baz_url(true)
end

test "omit positional path parameter when false" do
assert_equal "http://www.example.com/optional/false/baz", baz_url(false)
end

test "omit positional path parameter when blank" do
assert_equal "http://www.example.com/baz", baz_url("")
end

test "generating URLs with trailing slashes" do
assert_equal "/bars.json", bars_path(
trailing_slash: true,
Expand Down

0 comments on commit 98ed240

Please sign in to comment.