cause rails to correctly place optional path parameter booleans #42283

HParker · 2021-05-24T18:30:13Z

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 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`

zzak · 2021-05-25T02:06:05Z

@HParker Thank you for your patch!

This is interesting because we're changing the behavior and some apps may be indirectly depending upon that.

What if you had an optional parameter that you expected to be a boolean?

Correct me if I'm wrong but the current behavior would still pass that parameter as false but we'd be changing it to nil. I'm not sure the impact of this but just wondering out loud. 🤔

HParker · 2021-05-25T04:10:11Z

Thanks for looking at it @zzak!

What if you had an optional parameter that you expected to be a boolean?

Yeah, that is a good point. Until reading your comment I didn't know this was possible, but you can have a route like,

get "this/:my_bool/that", as: :this # etc.

this_path(my_bool: true) # => "this/true/that

I updated my branch to make true and false behave the same in urls. So if you have an optional path parameter that is a boolean, it can be true or false.

This still prevents adding the parameter at the end of the url and makes behavior more consistent.

actionpack/lib/action_dispatch/journey/formatter.rb

actionpack/lib/action_dispatch/journey/visitors.rb

actionpack/test/dispatch/url_generation_test.rb

bogdan · 2021-05-25T11:07:23Z

Rails also allows to put an optional parameter as a positional argument:

that_path(true)
that_path(false)
that_path('')
that_path(nil)

Can you make sure it is also consistent and add tests for that?

zzak · 2021-05-25T11:33:05Z

@bogdan Good call, I did not consider positional args 🤦

HParker · 2021-05-25T15:05:41Z

Added tests for positional arguments & rebased to one commit.

skipkayhil · 2021-05-27T17:24:45Z

actionpack/lib/action_dispatch/journey/formatter.rb

@@ -118,7 +118,7 @@ def extract_parameterized_parts(route, options, recall)
            end
          end

-          parameterized_parts.keep_if { |_, v| v }
+          parameterized_parts.delete_if { |_, v| v.nil? }


Could we use compact here?

Suggested change

parameterized_parts.delete_if { |_, v| v.nil? }

parameterized_parts.compact!

Ah, compact works, but compact! does not. Thanks for the suggestion! either way it is nicer then delete_if :)

Right, compact! only works if you leave the parameterize_parts line after (because it returns nil if nothing is removed). compact will always return the new value because it creates a copy

I see! I suppose delete_if doesn't create a copy so maybe it would be better to use compact! and leave the parameterized_parts at the end. like you originally suggested.

zzak · 2021-05-27T23:26:17Z

@kamipo I think this is ready for review if you wouldn't mind taking another look? 🙇

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: rails#42280 Co-authored-by: Ryuta Kamizono <kamipo@gmail.com>

cause rails to correctly place optional path parameter booleans

rails-bot bot added the actionpack label May 24, 2021

HParker force-pushed the named-routes-identifies-false branch from 1f39826 to 5f57ea5 Compare May 24, 2021 18:33

HParker force-pushed the named-routes-identifies-false branch from 5f57ea5 to 1033b6a Compare May 25, 2021 04:09

zzak approved these changes May 25, 2021

View reviewed changes

HParker changed the title ~~cause rails to correctly remove unused optional path parameters~~ cause rails to correctly place optional path parameter booleans May 25, 2021

kamipo reviewed May 25, 2021

View reviewed changes

actionpack/lib/action_dispatch/journey/formatter.rb Outdated Show resolved Hide resolved

actionpack/lib/action_dispatch/journey/visitors.rb Outdated Show resolved Hide resolved

actionpack/test/dispatch/url_generation_test.rb Show resolved Hide resolved

HParker force-pushed the named-routes-identifies-false branch from 44313bd to a8390e4 Compare May 25, 2021 15:04

HParker force-pushed the named-routes-identifies-false branch 2 times, most recently from d03c499 to 0c64d8d Compare May 25, 2021 17:20

skipkayhil reviewed May 27, 2021

View reviewed changes

HParker force-pushed the named-routes-identifies-false branch 2 times, most recently from 1717221 to 9805b2b Compare May 27, 2021 21:53

HParker force-pushed the named-routes-identifies-false branch from 9805b2b to 98ed240 Compare May 28, 2021 01:21

eileencodes merged commit 0d18b43 into rails:main Jun 1, 2021

eileencodes added a commit that referenced this pull request Jun 2, 2021

Merge pull request #42283 from HParker/named-routes-identifies-false

d83a318

cause rails to correctly place optional path parameter booleans

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cause rails to correctly place optional path parameter booleans #42283

cause rails to correctly place optional path parameter booleans #42283

HParker commented May 24, 2021 •

edited

zzak commented May 25, 2021

HParker commented May 25, 2021

bogdan commented May 25, 2021

zzak commented May 25, 2021

HParker commented May 25, 2021

skipkayhil May 27, 2021

HParker May 27, 2021

skipkayhil May 27, 2021

HParker May 28, 2021

zzak commented May 27, 2021

	parameterized_parts.delete_if { \|_, v\| v.nil? }
	parameterized_parts.compact!

cause rails to correctly place optional path parameter booleans #42283

cause rails to correctly place optional path parameter booleans #42283

Conversation

HParker commented May 24, 2021 • edited

zzak commented May 25, 2021

HParker commented May 25, 2021

bogdan commented May 25, 2021

zzak commented May 25, 2021

HParker commented May 25, 2021

skipkayhil May 27, 2021

Choose a reason for hiding this comment

HParker May 27, 2021

Choose a reason for hiding this comment

skipkayhil May 27, 2021

Choose a reason for hiding this comment

HParker May 28, 2021

Choose a reason for hiding this comment

zzak commented May 27, 2021

HParker commented May 24, 2021 •

edited