Repository navigation
Use hash lookup for exact-match regexp filters in ParameterFilter - #57166
Conversation
|
|
||
| literal = source.delete_prefix("^").delete_prefix("\\A") | ||
| .delete_suffix("$").delete_suffix("\\z") | ||
| literal unless literal.match?(/[.\[\]\\()|*+?{}]/) |
There was a problem hiding this comment.
Parsing regexps with regexps makes me uncorfortable. It's very hard to be sure we got this perfectly right.
At the very least, this should error on the side of caution, and only return the literal if we match a safe regexp, not skip returning it unless we match one.
So something like:
literal if literal.match?(/\A[a-zA-Z0-9_]\z/)There was a problem hiding this comment.
Good feedback. I made that exact change.
When ParameterFilter is initialized with anchored regexp filters like
/^code$/ or /\Atoken\z/, extract the literal string and store it in a
Hash for O(1) lookup instead of iterating all regexps with .any?.
This is a common pattern in Rails apps that want to filter specific
parameter names without matching substrings.
Benchmark script:
require "bundler/inline"
gemfile(true) do
source "https://rubygems.org"
gem "rails", path: "."
gem "benchmark-ips"
end
require "active_support/parameter_filter"
PARAMS = {
"controller" => "users", "action" => "index", "format" => "html",
"page" => "1", "per_page" => "25", "sort" => "name", "direction" => "asc",
"locale" => "en", "currency" => "USD", "shipping_id" => "ship_123",
"variant_id" => "var_456", "quantity" => "2", "discount" => "10",
"referrer" => "google", "utm_source" => "email", "utm_medium" => "cpc",
"request_id" => "req_789", "trace_id" => "tr_012", "span_id" => "sp_345",
"version" => "2.1"
}.freeze
EXACT_FILTERS = [
/^code$/, /^token$/, /^secret$/, /^password$/, /^session_id$/,
/^state$/, /^cookie$/, /^email$/, /^evidence$/, /^nonce$/
].freeze
PARTIAL_FILTERS = [
/password/, /token/, /secret/, /authorization/, /certificate/,
/ssn/, /nonce/, /email/, /session/, /credential/
].freeze
MIXED_FILTERS = [
/^code$/, /^token$/, /^state$/, /^email$/, /^cookie$/,
/password/, /secret/, /authorization/, /certificate/, /ssn/
].freeze
exact_pf = ActiveSupport::ParameterFilter.new(EXACT_FILTERS)
partial_pf = ActiveSupport::ParameterFilter.new(PARTIAL_FILTERS)
mixed_pf = ActiveSupport::ParameterFilter.new(MIXED_FILTERS)
Benchmark.ips do |x|
x.report("all exact-match filters") { exact_pf.filter(PARAMS) }
x.report("all partial-match filters") { partial_pf.filter(PARAMS) }
x.report("mixed filters") { mixed_pf.filter(PARAMS) }
x.compare!
end
Results (Ruby 3.4, Apple M1):
Before After
all exact-match filters 58.2k i/s 258.8k i/s (4.5x faster)
all partial-match filters57.5k i/s 65.4k i/s (no regression)
mixed filters 58.3k i/s 103.0k i/s (1.8x faster)
Co-Authored-By: Claude Opus 4.6 <[email protected]>
df0991e to
c6191da
Compare
|
What about the performance for apps which don't only do exact matches? Rails.application.config.filter_parameters += [
:passw, :email, :secret, :token, :_key, :crypt, :salt, :certificate, :otp, :ssn, :cvv, :cvc
]which is not exact matches. I would also expect this optimization to be counter-productive if there is a mix of exact and substring matches, as My concern specifically is the added |
|
I let Claude (Fable) check my concerns and this is its summary: I benchmarked this to quantify my concern above. Setup: CRuby 4.0.4, benchmark-ips, a realistic 17-key nested params hash, comparing the parent of this PR's merge vs current main. Both sides include dd1728e ( 1. Per-request cost is real:
2. And the optimization never activates on this default path. 3. Where it does activate, it works — including in the mixed case. With 12 default substring filters + 6 exact regexps, filtering the params hash:
So contrary to my expectation, hash + smaller regexp beat the single folded union by ~16% here (exact hits skip the regexp entirely; misses scan a smaller union). Defaults-only filtering (no exact matches) is unchanged, within error. Given 2 and 3, the way to actually get this win on the request path would be for |
3 participants
Motivation / Background
I was looking at one of our applications at Shopify where a number of the ParameterFilter regexes are just exact matches -- e.g.,
/^email$/.We can optimize ParameterFilter for cases like this and get a meaningful speedup in ParameterFilter. And because we call ParameterFilter so often, it can be meaningful.
Detail
When ParameterFilter is initialized with anchored regexp filters like
/^code$/or/\Atoken\z/, extract the literal string and store it in a Hash for O(1) lookup instead of iterating all regexps with.any?.This is a common pattern in Rails apps that want to filter specific parameter names without matching substrings. I've included a benchmark script in the commit; if all filters are exact matches, it's 4.5x faster to use the hash to check for which parameters to exclude.
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]