`Pattern#from_sting` was introduced in bb207ea7 to support creating
patterns from strings in tests (removing a conditional in the initialize
method that had been there only for the sake of those tests).
`Pattern#build` was introduced in 947ebe9a, and was similarly used only
in tests.
My understanding is that [`Mapping#path`][path] is the only place where
we initialize a `Path::Pattern` in library code.
Since these two methods appear to be used only in tests, this
commit moves them out of the class and into a test helper.
My reasoning for doing this is that I am doing some performance work
that may involve a modification to how `Path::Pattern`s get initialized,
and I will have more confidence in that work if I know for sure that
these methods are test helpers that can be modified freely.
[path]: https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/routing/mapper.rb#L192-L194
Before this commit we initialized all Symbols with the default regexp,
then later on reassigned any symbols descending from stars with either
their regexp from `@requirements` or the default greedy regexp.
With this commit we initialize all Symbols descending from Stars with
the greedy regexp at parse time. This allows us to get rid of the star
branch in path/pattern, since any regexps from `@requirements` will
already have been set in the symbol branch of this code.
This is essentially an alternate version of #38901. Getting rid of the
extra branch makes some performance work I am doing a bit easier, plus
it saves us a few method calls. Also the constant saves us from creating
the same regexp multiple times, and it is nice to give that regexp a
name.
Awhile back tenderlove and I worked to improve performance of
integration tests and remove controller tests. The second never
happened, we ended up soft deprecating them but never did that
completely.
Now that we're revisting that work we need a way to override these two
methods in tests so that we can convert tests in Rails to be integration
tests instead of controller tests. Splitting these two concerns into two
methods allows us to overwrite them to work for our needs while
refactoring the test harness code.
These methods are private because they should not be used by an
application or gems, they will be removed when the refactoring has been
completed.
Co-authored-by: Aaron Patterson <tenderlove@ruby-lang.org>
Along the same lines as #38901, #39903, and #39914, this commit removes
one pass through the journey ast in an attempt to improve application
boot time.
Before this commit, `Mapper#path` would iterate through the ast to alter
the regex for custom routes. With this commit we move the regex
alterations to initialize, where we were already iterating through the
ast.
The Benchmark for this change is similar to what we have been seeing in
the other PRs.
```
Warming up --------------------------------------
after 13.121k i/100ms
before 7.416k i/100ms
Calculating -------------------------------------
after 128.469k (± 3.1%) i/s - 642.929k in 5.009391s
before 76.561k (± 1.8%) i/s - 385.632k in 5.038677s
Comparison:
after: 128469.4 i/s
before: 76560.8 i/s - 1.68x (± 0.00) slower
Calculating -------------------------------------
after 160.000 memsize ( 0.000 retained)
3.000 objects ( 0.000 retained)
0.000 strings ( 0.000 retained)
before 360.000 memsize ( 0.000 retained)
6.000 objects ( 0.000 retained)
0.000 strings ( 0.000 retained)
Comparison:
after: 160 allocated
before: 360 allocated - 2.25x more
```
Along the same lines as https://github.com/rails/rails/pull/38901,
this commit makes a small performance enhancement by iterating through
the ast once instead of twice when initializing a Mapping.
This stemmed from work to improve the boot time of an application with
3500+ routes. This patch only shaves off about 70ms from our boot time,
and about 25000 allocations, but every little bit counts!
Benchmark
---
```rb
require "bundler/inline"
gemfile(true) do
source "https://rubygems.org"
git_source(:github) { |repo| "https://github.com/#{repo}.git" }
# Activate the gem you are reporting the issue against.
gem "rails", path: "/Users/daniel/Desktop/oss/rails/rails"
gem "benchmark-ips"
gem "benchmark-memory", require: "benchmark/memory"
end
require "action_controller/railtie"
class TestApp < Rails::Application
config.root = __dir__
config.hosts << "example.org"
config.session_store :cookie_store, key: "cookie_store_key"
secrets.secret_key_base = "secret_key_base"
config.logger = Logger.new($stdout)
Rails.logger = config.logger
routes.draw do
get("/*wildcard", to: "controller#index")
end
end
```
With the benchmarking code inserted into Mapper#initialize:
```rb
after = -> {
path_params = []
wildcard_options = {}
ast.each do |node|
if node.symbol?
path_params << node.to_sym
elsif node.star? && formatted != false
# Add a constraint for wildcard route to make it non-greedy and match the
# optional format part of the route by default.
wildcard_options[node.name.to_sym] ||= /.+?/
end
end
wildcard_options.merge(options)
}
before = -> {
path_params = ast.find_all(&:symbol?).map(&:to_sym)
add_wildcard_options(options, formatted, ast)
}
puts "IPS"
Benchmark.ips do |x|
x.report("before") { before.call }
x.report("after") { after.call }
x.compare!
end
puts "MEMORY"
Benchmark.memory do |x|
x.report("before") { before.call }
x.report("after") { after.call }
x.compare!
end
```
The results are:
```
IPS
Warming up --------------------------------------
before 14.352k i/100ms
after 30.852k i/100ms
Calculating -------------------------------------
before 135.675k (± 3.7%) i/s - 688.896k in 5.084368s
after 288.126k (± 3.3%) i/s - 1.450M in 5.038072s
Comparison:
after: 288126.4 i/s
before: 135675.1 i/s - 2.12x (± 0.00) slower
MEMORY
Calculating -------------------------------------
before 360.000 memsize ( 0.000 retained)
7.000 objects ( 0.000 retained)
0.000 strings ( 0.000 retained)
after 200.000 memsize ( 0.000 retained)
4.000 objects ( 0.000 retained)
0.000 strings ( 0.000 retained)
comparison:
after: 200 allocated
before: 360 allocated - 1.80x more end
```
This change ensures we're no longer using a singleton for routes because
we want to change the routing table based on the controller we're
testing. We don't want the routing table to be global for the Action
Pack tests.
Co-authored-by: Aaron Patterson <tenderlove@ruby-lang.org>
308 status code introduced in https://tools.ietf.org/html/rfc7538
preserves the request method unlike 301 status code which would convert
POST requests to GET.
The controller test case is trying to encode json parameters in the same
way it encodes query parameters. However we need to encode json
parameters as json.
This was broken in #39534 because `generate_extras` is encoding
everything into query parameters. But json parameters are in the post
body and need to be treated differently than the query parameters.
Followup to #39651
Aaron Patterson <tenderlove@ruby-lang.org>
- In #39534, `Routing::RouteSet#generate_extras` (which get called)
by ActionController::TestCase now go through `path_for` which strips
out all key in the options hash that have the same name as the ones
defined in `RESERVED_OPTIONS`.
Follow up to c07dff72278fb7f2a3c4c71212a0773a2b25c790.
Actually it is not the cop's fault, but we mistakenly use `^`, `$`, and
`\Z` in much places, the cop doesn't correct those conservatively.
I've checked all those usage and replaced all safe ones.
Prior to this commit, when multiple cookie domains were specified, the
first domain that was a substring of the request host was chosen. This
allowed, for example, the "example.com" domain to be chosen when the
request host was "example.com.au" or even "myexample.com".
This commit ensures a domain is chosen only if it is equal to or is a
superdomain of the request host.
Fixes#37760.
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 reverts commit ea791e53f9edbe1e2d2ca779f5ce20b27320581f.
reason: these ivars are modified within other synchronize blocks,
and so need to be blocked from other threads for ensuring therad safety.
Co-authored-by: Ryuta Kamizono <kamipo@gmail.com>
The previous example disables CSRF protection for JSON format requests,
which allows any request to bypass it by adding the right Accept header.
Accept is one of the few headers that doesn't trigger a CORS preflight:
https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#Examples_of_access_control_scenarios
Instead of disabling it entirely, the simple case where authentication
is handled without a session (e.g. when using API keys) can be solved by
switching to the `:null_session` strategy.
For more complex cases (e.g. an API that uses session authentication),
I don't think a copy-and-paste-able example is appropriate; the user
needs to understand CSRF protection in order to make an informed choice.
When using an external build process (webpack, grunt) it's helpful for
rails to be able to serve those assets. Brotli has better compression
than gzip and should eventually replace it for static assets.
When using an external build process (webpack, grunt) it's helpful for
rails to be able to serve those assets. Brotli has better compression
than gzip and will eventually replace it for static assets.
