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: https://github.com/rails/rails/issues/42280
Co-authored-by: Ryuta Kamizono <kamipo@gmail.com>
This PR converts the route params array into an object and moves the
error generation code into its own object as well. This is a refactoring
of internal code to make it easier to change and simplier for internals
to interface with. We have two new objects:
1) `RouteWithParams`
Previously `#generate` returned an array of parameterized parts for a
route. We have now turned this into an object with a `path` and a
`params methods` to be able to access the parts we need.
2) `MissingRoute`
`#generate` was also responsible for constructing the message for the
error. We've moved this code into the new `MissingRoute` object so it
does that work instead.
This change makes these methods reusable throughout the code and easier
for future refactoring we plan to do.
Co-authored-by: Aaron Patterson <aaron.patterson@gmail.com>
This commit is a PoC for adding did you mean support to Url Generation
errors. I think we can do much better than this, but I wanted to try
something out as a first pass.
Journey is a private API inside Rails now, so lets start cleaning up
some of its signatures. In this case, we always wan to parameterize
things the same way, so lets just do that in the code (rather than
passing some strategy object in).
When the route definition has parameters, we can supply a regex for
validation purposes:
get "/a/:b" => "test#index", constraints: { b: /abc/ }, as: test
This regex is going to be used to check the supplied values during
link generation:
test_path("abc") # check "abc" against /abc/ regex
The link generation code checks each parameter. To properly validate the
parameter, it creates a new regex with start and end of string modifiers:
/\A#{original_regex}\Z/
This means for each link generation the code stringifies the existing
regex and creates a new one. When a new regex is created, it needs to be
compiled, for large regexes this can take quite a bit of time.
This change memoizes the generated regex for each route when constrains
are given. It also removes the RegexCaseComparator class since it is not
in use anymore.
This reverts commit 4e105385d046e2aeab16955943df97c5eefa3a6f, reversing
changes made to 62b43839098bbbbfc4be789128d33dc0612f1ab3.
The change in Ruby that made those changes required was reverted in
8852fa8760
When a route was defined within an optional scope, if that route didn't
take parameters the scope was lost when using path helpers. This patch
ensures scope is kept both when the route takes parameters or when it
doesn't.
Fixes#33219
We want to avoid terminating the whole loop here, because it will cause
parameters that should be removed to not be removed, since we are
terminating early. In this specific case, `param2` is processed before
`param1` due to the reversing of `route.parts`, and since `param2` fails
the check on this line, it would previously cause the whole loop to
fail, and `param1` would still be in `parameterized_parts`. Now, we are
simply calling `next`, which is the intended behavior.
Introduced by 8ca8a2d773b942c4ea76baabe2df502a339d05b1.
Fixes#27454.
Scoring routes based on constraints repeated many type conversions that
could be performed in the outer loop. Determinations of score and
fitness also used Array operations that required allocations. Against
my benchmark with a large routeset, this reduced object allocations by
over 30x and wall time by over 3x.
Currently a misleading "missing required keys" error is thrown when a param
fails to match the constraints of a particular route. This commit ensures that
these params are recognised as unmatching rather than missing.
Note: this means that a different error message will be provided between
optimized and non-optimized path helpers, due to the fact that the former does
not check constraints when matching routes.
Fixes#26470.
The longstanding convention in Rails is that if the :action parameter
is missing or nil then it defaults to 'index'. Up until Rails 5.0.0.beta1
this was handled slightly differently than other routing defaults by
deleting it from the route options and adding it to the recall parameters.
With the recent focus of removing unnecessary duplications this has
exposed a problem in this strategy - we are now mutating the request's
path parameters and causing problems for later url generation. This will
typically affect url_for rather a named url helper since the latter
explicitly pass :controller, :action, etc.
The fix is to add a default for :action in the route class if the path
contains an :action segment and no default is passed. This change also
revealed an issue with the parameterized part expiry in that it doesn't
follow a right to left order - as soon as a dynamic segment is required
then all other segments become required.
Fixes#23019.
Commit bff61ba, while reducing allocations, caused a regression when an empty
format is passed to a route.
This can happen in cases where you're using an anchor tag, for example:
`https://example.com/parent/575256966.#child_1032289285`.
Because of this change `format` was getting sent in
`parameterized_parts` when previously it was not included. This resulted
in blank `format`'s being returned as `.` when if there was an extension
included it would be `.extension`. Since there was no extension this
caused incorrect URL's.
The test shows this would result in `/posts/show/1.` instead of
`/posts/show/1` which causes bad urls since the format is not present.
We don't always need an array when generating a url with the formatter. We can be lazy about allocating the `missing_keys` array. This saves us:
35,606 bytes and 889 objects per request
THe only reason we were allocating an array is to get the "missing_keys" variable in scope of the error message generator. Guess what? Arrays kinda take up a lot of memory, so by replacing that with a nil, we save:
35,303 bytes and 886 objects per request
When `defaults[key]` in `generate` in the journey formatter is called, it often returns a `nil` when we call `to_s` on a nil, it allocates an empty string. We can skip this check when the default value is nil.
This change buys us 35,431 bytes of memory and 887 fewer objects per request.
Thanks to @matthewd for help with the readability
Most routes have a `route.path.requirements[key]` of `/[-_.a-zA-Z0-9]+\/[-_.a-zA-Z0-9]+/` yet every time this method is called a new regex is generated on the fly with `/\A#{DEFAULT_INPUT}\Z/`. OBJECT ALLOCATIONS BLERG!
This change uses a special module that implements `===` so it can be used in a case statement to pull out the default input. When this happens, we use a pre-generated regex.
This change buys us 1,643,465 bytes of memory and 7,990 fewer objects per request.
Micro optimization: `reverse.drop_while` is slower than `reverse_each.drop_while`. This doesn't save any object allocations.
Second, `keys_to_keep` is typically a very small array. The operation `parameterized_parts.keys - keys_to_keep` actually allocates two arrays. It is quicker (I benchmarked) to iterate over each and check inclusion in array manually.
This change buys us 1774 fewer objects per request
If the route set is empty, or if none of the routes matches with a score > 0,
there is no point showing the deprecation message because we are already be
raising the `ActionController::UrlGenerationError` mentioned in the warning.
In this case it is the expected behavior and the user wouldn't have to take any
actions.
The internal tests that (incorrectly) relied on this were already fixed in
938d130. However, we cannot simply fix this bug because the guides prior to
b7b9e92 recommended a workaround that relies on this buggy behavior.
Reference #17453
"recall" is a terrible name. This variable contains the parameters that
we got from the path (e.g. for "/posts/1" it has :controller => "posts",
:id => "1"). Since it contains the parameters we got from the path,
"path_parameters" is a better name. We always pass path_parameters to
`generate`, so lets make it required.
