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.

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

cause rails to correctly place optional path parameter booleans #42283

Merged
merged 1 commit into from Jun 1, 2021

Conversation

@HParker
Copy link
Contributor

@HParker HParker commented May 24, 2021

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`
@rails-bot rails-bot bot added the actionpack label May 24, 2021
@HParker HParker force-pushed the named-routes-identifies-false branch from 1f39826 to 5f57ea5 May 24, 2021
@zzak
Copy link
Member

@zzak zzak commented May 25, 2021

@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 HParker force-pushed the named-routes-identifies-false branch from 5f57ea5 to 1033b6a May 25, 2021
@HParker
Copy link
Contributor Author

@HParker HParker commented May 25, 2021

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.

zzak
zzak approved these changes May 25, 2021
@HParker 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
@bogdan
Copy link
Contributor

@bogdan bogdan commented May 25, 2021

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
Copy link
Member

@zzak zzak commented May 25, 2021

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

@HParker HParker force-pushed the named-routes-identifies-false branch from 44313bd to a8390e4 May 25, 2021
@HParker
Copy link
Contributor Author

@HParker HParker commented May 25, 2021

Added tests for positional arguments & rebased to one commit.

@HParker HParker force-pushed the named-routes-identifies-false branch 2 times, most recently from d03c499 to 0c64d8d May 25, 2021
@@ -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? }
Copy link
Contributor

@SkipKayhil SkipKayhil May 27, 2021

Could we use compact here?

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

Copy link
Contributor Author

@HParker HParker May 27, 2021

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

Copy link
Contributor

@SkipKayhil SkipKayhil May 27, 2021

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

Copy link
Contributor Author

@HParker HParker May 28, 2021

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.

@HParker HParker force-pushed the named-routes-identifies-false branch 2 times, most recently from 1717221 to 9805b2b May 27, 2021
@zzak
Copy link
Member

@zzak zzak commented May 27, 2021

@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>
@HParker HParker force-pushed the named-routes-identifies-false branch from 9805b2b to 98ed240 May 28, 2021
@eileencodes eileencodes merged commit 0d18b43 into rails:main Jun 1, 2021
4 checks passed
eileencodes added a commit that referenced this issue Jun 2, 2021
cause rails to correctly place optional path parameter booleans
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants