Merge pull request #34208 from yskkin/inspect_with_parameter_filter

Implement AR#inspect using ParameterFilter

4 files changed, 90 insertions, 32 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index f56551beca..0d9bc3cec7 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -161,13 +161,13 @@
     specify sensitive attributes to specific model.
 
     ```
-    Rails.application.config.filter_parameters += [:credit_card_number]
-    Account.last.inspect # => #<Account id: 123, name: "DHH", credit_card_number: [FILTERED] ...>
+    Rails.application.config.filter_parameters += [:credit_card_number, /phone/]
+    Account.last.inspect # => #<Account id: 123, name: "DHH", credit_card_number: [FILTERED], telephone_number: [FILTERED] ...>
     SecureAccount.filter_attributes += [:name]
     SecureAccount.last.inspect # => #<SecureAccount id: 42, name: [FILTERED], credit_card_number: [FILTERED] ...>
     ```
 
-    *Zhang Kang*
+    *Zhang Kang*, *Yoshiyuki Kinjo*
 
 *   Deprecate `column_name_length`, `table_name_length`, `columns_per_table`,
     `indexes_per_table`, `columns_per_multicolumn_index`, `sql_query_length`,
diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb
index eeb88e4b7a..1e92ee3b96 100644
--- a/activerecord/lib/active_record/attribute_methods.rb
+++ b/activerecord/lib/active_record/attribute_methods.rb
@@ -329,14 +329,7 @@ module ActiveRecord
     #   # => "[1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11]"
     def attribute_for_inspect(attr_name)
       value = read_attribute(attr_name)
-
-      if value.is_a?(String) && value.length > 50
-        "#{value[0, 50]}...".inspect
-      elsif value.is_a?(Date) || value.is_a?(Time)
-        %("#{value.to_s(:db)}")
-      else
-        value.inspect
-      end
+      format_for_inspect(value)
     end
 
     # Returns +true+ if the specified +attribute+ has been set by the user or by a
@@ -456,6 +449,16 @@ module ActiveRecord
         end
       end
 
+      def format_for_inspect(value)
+        if value.is_a?(String) && value.length > 50
+          "#{value[0, 50]}...".inspect
+        elsif value.is_a?(Date) || value.is_a?(Time)
+          %("#{value.to_s(:db)}")
+        else
+          value.inspect
+        end
+      end
+
       def readonly_attribute?(name)
         self.class.readonly_attributes.include?(name)
       end
diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb
index 0718688863..da3e2549a2 100644
--- a/activerecord/lib/active_record/core.rb
+++ b/activerecord/lib/active_record/core.rb
@@ -2,15 +2,13 @@
 
 require "active_support/core_ext/hash/indifferent_access"
 require "active_support/core_ext/string/filters"
+require "active_support/parameter_filter"
 require "concurrent/map"
-require "set"
 
 module ActiveRecord
   module Core
     extend ActiveSupport::Concern
 
-    FILTERED = "[FILTERED]" # :nodoc:
-
     included do
       ##
       # :singleton-method:
@@ -239,9 +237,7 @@ module ActiveRecord
       end
 
       # Specifies columns which shouldn't be exposed while calling +#inspect+.
-      def filter_attributes=(attributes_names)
-        @filter_attributes = attributes_names.map(&:to_s).to_set
-      end
+      attr_writer :filter_attributes
 
       # Returns a string like 'Post(id:integer, title:string, body:text)'
       def inspect # :nodoc:
@@ -502,11 +498,14 @@ module ActiveRecord
       inspection = if defined?(@attributes) && @attributes
         self.class.attribute_names.collect do |name|
           if has_attribute?(name)
-            if filter_attribute?(name)
-              "#{name}: #{ActiveRecord::Core::FILTERED}"
+            attr = read_attribute(name)
+            value = if attr.nil?
+              attr.inspect
             else
-              "#{name}: #{attribute_for_inspect(name)}"
+              attr = format_for_inspect(attr)
+              inspection_filter.filter_param(name, attr)
             end
+            "#{name}: #{value}"
           end
         end.compact.join(", ")
       else
@@ -522,18 +521,16 @@ module ActiveRecord
       return super if custom_inspect_method_defined?
       pp.object_address_group(self) do
         if defined?(@attributes) && @attributes
-          column_names = self.class.column_names.select { |name| has_attribute?(name) || new_record? }
-          pp.seplist(column_names, proc { pp.text "," }) do |column_name|
+          attr_names = self.class.attribute_names.select { |name| has_attribute?(name) }
+          pp.seplist(attr_names, proc { pp.text "," }) do |attr_name|
             pp.breakable " "
             pp.group(1) do
-              pp.text column_name
+              pp.text attr_name
               pp.text ":"
               pp.breakable
-              if filter_attribute?(column_name)
-                pp.text ActiveRecord::Core::FILTERED
-              else
-                pp.pp read_attribute(column_name)
-              end
+              value = read_attribute(attr_name)
+              value = inspection_filter.filter_param(attr_name, value) unless value.nil?
+              pp.pp value
             end
           end
         else
@@ -585,8 +582,14 @@ module ActiveRecord
         self.class.instance_method(:inspect).owner != ActiveRecord::Base.instance_method(:inspect).owner
       end
 
-      def filter_attribute?(attribute_name)
-        self.class.filter_attributes.include?(attribute_name) && !read_attribute(attribute_name).nil?
+      def inspection_filter
+        @inspection_filter ||= begin
+          mask = DelegateClass(::String).new(ActiveSupport::ParameterFilter::FILTERED)
+          def mask.pretty_print(pp)
+            pp.text __getobj__
+          end
+          ActiveSupport::ParameterFilter.new(self.class.filter_attributes, mask: mask)
+        end
       end
   end
 end
diff --git a/activerecord/test/cases/filter_attributes_test.rb b/activerecord/test/cases/filter_attributes_test.rb
index af5badd87d..47161a547a 100644
--- a/activerecord/test/cases/filter_attributes_test.rb
+++ b/activerecord/test/cases/filter_attributes_test.rb
@@ -4,6 +4,7 @@ require "cases/helper"
 require "models/admin"
 require "models/admin/user"
 require "models/admin/account"
+require "models/user"
 require "pp"
 
 class FilterAttributesTest < ActiveRecord::TestCase
@@ -30,6 +31,32 @@ class FilterAttributesTest < ActiveRecord::TestCase
     end
   end
 
+  test "string filter_attributes perform pertial match" do
+    ActiveRecord::Base.filter_attributes = ["n"]
+    Admin::Account.all.each do |account|
+      assert_includes account.inspect, "name: [FILTERED]"
+      assert_equal 1, account.inspect.scan("[FILTERED]").length
+    end
+  end
+
+  test "regex filter_attributes are accepted" do
+    ActiveRecord::Base.filter_attributes = [/\An\z/]
+    account = Admin::Account.find_by(name: "37signals")
+    assert_includes account.inspect, 'name: "37signals"'
+    assert_equal 0, account.inspect.scan("[FILTERED]").length
+
+    ActiveRecord::Base.filter_attributes = [/\An/]
+    account = Admin::Account.find_by(name: "37signals")
+    assert_includes account.reload.inspect, "name: [FILTERED]"
+    assert_equal 1, account.inspect.scan("[FILTERED]").length
+  end
+
+  test "proc filter_attributes are accepted" do
+    ActiveRecord::Base.filter_attributes = [ lambda { |key, value| value.reverse! if key == "name" } ]
+    account = Admin::Account.find_by(name: "37signals")
+    assert_includes account.inspect, 'name: "slangis73"'
+  end
+
   test "filter_attributes could be overwritten by models" do
     Admin::Account.all.each do |account|
       assert_includes account.inspect, "name: [FILTERED]"
@@ -37,7 +64,6 @@ class FilterAttributesTest < ActiveRecord::TestCase
     end
 
     begin
-      previous_account_filter_attributes = Admin::Account.filter_attributes
       Admin::Account.filter_attributes = []
 
       # Above changes should not impact other models
@@ -51,7 +77,7 @@ class FilterAttributesTest < ActiveRecord::TestCase
         assert_equal 0, account.inspect.scan("[FILTERED]").length
       end
     ensure
-      Admin::Account.filter_attributes = previous_account_filter_attributes
+      Admin::Account.remove_instance_variable(:@filter_attributes)
     end
   end
 
@@ -63,6 +89,18 @@ class FilterAttributesTest < ActiveRecord::TestCase
     assert_equal 0, account.inspect.scan("[FILTERED]").length
   end
 
+  test "filter_attributes should handle [FILTERED] value properly" do
+    begin
+      User.filter_attributes = ["auth"]
+      user = User.new(token: "[FILTERED]", auth_token: "[FILTERED]")
+
+      assert_includes user.inspect, "auth_token: [FILTERED]"
+      assert_includes user.inspect, 'token: "[FILTERED]"'
+    ensure
+      User.remove_instance_variable(:@filter_attributes)
+    end
+  end
+
   test "filter_attributes on pretty_print" do
     user = admin_users(:david)
     actual = "".dup
@@ -81,4 +119,18 @@ class FilterAttributesTest < ActiveRecord::TestCase
     assert_not_includes actual, "name: [FILTERED]"
     assert_equal 0, actual.scan("[FILTERED]").length
   end
+
+  test "filter_attributes on pretty_print should handle [FILTERED] value properly" do
+    begin
+      User.filter_attributes = ["auth"]
+      user = User.new(token: "[FILTERED]", auth_token: "[FILTERED]")
+      actual = "".dup
+      PP.pp(user, StringIO.new(actual))
+
+      assert_includes actual, "auth_token: [FILTERED]"
+      assert_includes actual, 'token: "[FILTERED]"'
+    ensure
+      User.remove_instance_variable(:@filter_attributes)
+    end
+  end
 end