Merge pull request rails#46452 from jonathanhefner/parameter_filter-precompile_filters

Add `AS::ParameterFilter.precompile_filters`

Author: jonathanhefner

activesupport/lib/active_support/parameter_filter.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require "active_support/core_ext/object/duplicable"
+require "active_support/core_ext/array/extract"
 
 module ActiveSupport
   # +ParameterFilter+ allows you to specify keys for sensitive data from
@@ -32,6 +33,34 @@ module ActiveSupport
   class ParameterFilter
     FILTERED = "[FILTERED]" # :nodoc:
 
+    # Precompiles an array of filters that otherwise would be passed directly to
+    # #initialize. Depending on the quantity and types of filters,
+    # precompilation can improve filtering performance, especially in the case
+    # where the ParameterFilter instance itself cannot be retained (but the
+    # precompiled filters can be retained).
+    #
+    #   filters = [/foo/, :bar, "nested.baz", /nested\.qux/]
+    #
+    #   precompiled = ActiveSupport::ParameterFilter.precompile_filters(filters)
+    #   # => [/(?-mix:foo)|(?i:bar)/, /(?i:nested\.baz)|(?-mix:nested\.qux)/]
+    #
+    #   ActiveSupport::ParameterFilter.new(precompiled)
+    #
+    def self.precompile_filters(filters)
+      filters, patterns = filters.partition { |filter| filter.is_a?(Proc) }
+
+      patterns.map! do |pattern|
+        pattern.is_a?(Regexp) ? pattern : "(?i:#{Regexp.escape pattern.to_s})"
+      end
+
+      deep_patterns = patterns.extract! { |pattern| pattern.to_s.include?("\\.") }
+
+      filters << Regexp.new(patterns.join("|")) if patterns.any?
+      filters << Regexp.new(deep_patterns.join("|")) if deep_patterns.any?
+
+      filters
+    end
+
     # Create instance with given filters. Supported type of filters are +String+, +Regexp+, and +Proc+.
     # Other types of filters are treated as +String+ using +to_s+.
     # For +Proc+ filters, key, value, and optional original hash is passed to block arguments.

activesupport/test/parameter_filter_test.rb

@@ -121,4 +121,28 @@ class ParameterFilterTest < ActiveSupport::TestCase
       assert_equal after_filter, parameter_filter.filter(before_filter)
     end
   end
+
+  test "precompile_filters" do
+    patterns = [/A.a/, /b.B/i, "ccC", :ddD]
+    keys = ["Aaa", "Bbb", "Ccc", "Ddd"]
+    deep_patterns = [/A\.a/, /b\.B/i, "c.C", :"d.D"]
+    deep_keys = ["A.a", "B.b", "C.c", "D.d"]
+    procs = [proc { }, proc { }]
+
+    precompiled = ActiveSupport::ParameterFilter.precompile_filters([*patterns, *deep_patterns, *procs])
+
+    assert_equal 2, precompiled.grep(Regexp).length
+    assert_equal 2 + procs.length, precompiled.length
+
+    regexp = precompiled.find { |filter| filter.to_s.include?(patterns.first.to_s) }
+    keys.each { |key| assert_match regexp, key }
+    assert_no_match regexp, keys.first.swapcase
+
+    deep_regexp = precompiled.find { |filter| filter.to_s.include?(deep_patterns.first.to_s) }
+    deep_keys.each { |deep_key| assert_match deep_regexp, deep_key }
+    assert_no_match deep_regexp, deep_keys.first.swapcase
+
+    assert_not_equal regexp, deep_regexp
+    assert_equal procs, precompiled & procs
+  end
 end

guides/source/configuring.md

@@ -74,6 +74,7 @@ Below are the default values associated with each target version. In cases of co
 - [`config.active_support.raise_on_invalid_cache_expiration_time`](#config-active-support-raise-on-invalid-cache-expiration-time): `true`
 - [`config.add_autoload_paths_to_load_path`](#config-add-autoload-paths-to-load-path): `false`
 - [`config.log_file_size`](#config-log-file-size): `100 * 1024 * 1024`
+- [`config.precompile_filter_parameters`](#config-precompile-filter-parameters): `true`
 
 #### Default Values for Target Version 7.0
 
@@ -387,6 +388,20 @@ config.logger      = ActiveSupport::TaggedLogging.new(mylogger)
 
 Allows you to configure the application's middleware. This is covered in depth in the [Configuring Middleware](#configuring-middleware) section below.
 
+#### `config.precompile_filter_parameters`
+
+When `true`, will precompile [`config.filter_parameters`](#config-filter-parameters)
+using [`ActiveSupport::ParameterFilter.precompile_filters`][].
+
+The default value depends on the `config.load_defaults` target version:
+
+| Starting with version | The default value is |
+| --------------------- | -------------------- |
+| (original)            | `false`              |
+| 7.1                   | `true`               |
+
+[`ActiveSupport::ParameterFilter.precompile_filters`]: https://api.rubyonrails.org/classes/ActiveSupport/ParameterFilter.html#method-c-precompile_filters
+
 #### `config.public_file_server.enabled`
 
 Configures Rails to serve static files from the public directory. This option defaults to `true`, but in the production environment it is set to `false` because the server software (e.g. NGINX or Apache) used to run the application should serve static files instead. If you are running or testing your app in production using WEBrick (it is not recommended to use WEBrick in production), set the option to `true`. Otherwise, you won't be able to use page caching and request for files that exist under the public directory.

railties/CHANGELOG.md

@@ -1,3 +1,13 @@
+*   Add `config.precompile_filter_parameters`, which enables precompilation of
+    `config.filter_parameters` using `ActiveSupport::ParameterFilter.precompile_filters`.
+    Precompilation can improve filtering performance, depending on the quantity
+    and types of filters.
+
+    `config.precompile_filter_parameters` defaults to `true` for
+    `config.load_defaults 7.1` and above.
+
+    *Jonathan Hefner*
+
 *   Add `after_routes_loaded` hook to `Rails::Railtie::Configuration` for
     engines to add a hook to be called after application routes have been
     loaded.

railties/lib/rails/application.rb

@@ -301,7 +301,7 @@ def config_for(name, env: Rails.env)
     # will be used by middlewares and engines to configure themselves.
     def env_config
       @app_env_config ||= super.merge(
-          "action_dispatch.parameter_filter" => config.filter_parameters,
+          "action_dispatch.parameter_filter" => filter_parameters,
           "action_dispatch.redirect_filter" => config.filter_redirect,
           "action_dispatch.secret_key_base" => secret_key_base,
           "action_dispatch.show_exceptions" => config.action_dispatch.show_exceptions,
@@ -675,5 +675,13 @@ def build_middleware
       def coerce_same_site_protection(protection)
         protection.respond_to?(:call) ? protection : proc { protection }
       end
+
+      def filter_parameters
+        if config.precompile_filter_parameters
+          ActiveSupport::ParameterFilter.precompile_filters(config.filter_parameters)
+        else
+          config.filter_parameters
+        end
+      end
   end
 end

railties/lib/rails/application/configuration.rb

@@ -12,7 +12,7 @@ class Application
     class Configuration < ::Rails::Engine::Configuration
       attr_accessor :allow_concurrency, :asset_host, :autoflush_log,
                     :cache_classes, :cache_store, :consider_all_requests_local, :console,
-                    :eager_load, :exceptions_app, :file_watcher, :filter_parameters,
+                    :eager_load, :exceptions_app, :file_watcher, :filter_parameters, :precompile_filter_parameters,
                     :force_ssl, :helpers_paths, :hosts, :host_authorization, :logger, :log_formatter,
                     :log_tags, :railties_order, :relative_url_root, :secret_key_base,
                     :ssl_options, :public_file_server,
@@ -278,6 +278,7 @@ def load_defaults(target_version)
           load_defaults "7.0"
 
           self.add_autoload_paths_to_load_path = false
+          self.precompile_filter_parameters = true
 
           if Rails.env.development? || Rails.env.test?
             self.log_file_size = 100 * 1024 * 1024

railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt

@@ -117,3 +117,7 @@
 # The previous behavior was to validate the presence of the parent record, which performed an extra query
 # to get the parent every time the child record was updated, even when parent has not changed.
 # Rails.application.config.active_record.belongs_to_required_validates_foreign_key = false
+
+# Enable precompilation of `config.filter_parameters`. Precompilation can
+# improve filtering performance, depending on the quantity and types of filters.
+# Rails.application.config.precompile_filter_parameters = true

railties/test/application/configuration_test.rb

@@ -521,6 +521,7 @@ class Comment < ActiveRecord::Base
     end
 
     test "filter_parameters should be able to set via config.filter_parameters in an initializer" do
+      remove_from_config '.*config\.load_defaults.*\n'
       app_file "config/initializers/filter_parameters_logging.rb", <<-RUBY
         Rails.application.config.filter_parameters += [ :password, :foo, 'bar' ]
       RUBY
@@ -530,6 +531,46 @@ class Comment < ActiveRecord::Base
       assert_equal [:password, :foo, "bar"], Rails.application.env_config["action_dispatch.parameter_filter"]
     end
 
+    test "filter_parameters is precompiled when config.precompile_filter_parameters is true" do
+      filters = [/foo/, :bar, "baz.qux"]
+
+      add_to_config <<~RUBY
+        config.filter_parameters += #{filters.inspect}
+        config.precompile_filter_parameters = true
+      RUBY
+
+      app "development"
+
+      assert_equal ActiveSupport::ParameterFilter.precompile_filters(filters), Rails.application.env_config["action_dispatch.parameter_filter"]
+    end
+
+    test "filter_parameters is not precompiled when config.precompile_filter_parameters is false" do
+      filters = [/foo/, :bar, "baz.qux"]
+
+      add_to_config <<~RUBY
+        config.filter_parameters += #{filters.inspect}
+        config.precompile_filter_parameters = false
+      RUBY
+
+      app "development"
+
+      assert_equal filters, Rails.application.env_config["action_dispatch.parameter_filter"]
+    end
+
+    test "config.precompile_filter_parameters is true by default for new apps" do
+      app "development"
+
+      assert Rails.application.config.precompile_filter_parameters
+    end
+
+    test "config.precompile_filter_parameters is false by default for upgraded apps" do
+      remove_from_config '.*config\.load_defaults.*\n'
+      add_to_config 'config.load_defaults "7.0"'
+      app "development"
+
+      assert_not Rails.application.config.precompile_filter_parameters
+    end
+
     test "config.to_prepare is forwarded to ActionDispatch" do
       $prepared = false