Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement AR#inspect using ParameterFilter #34208

Merged
merged 1 commit into from Oct 26, 2018

Conversation

@yskkin
Copy link
Contributor

@yskkin yskkin commented Oct 13, 2018

Summary

AR instance support filter_parameters since #33756.
Though Regex or Proc is valid as filter_parameters, they are not supported as AR#inspect.
#33756 (comment)

This PR tries to fix this issue.

@rails-bot
Copy link

@rails-bot rails-bot commented Oct 13, 2018

r? @rafaelfranca

(@rails-bot has picked a reviewer for you, use r? to override)

@rails-bot rails-bot bot added the railties label Oct 13, 2018
@@ -240,7 +238,7 @@ def filter_attributes

# Specifies columns which shouldn't be exposed while calling +#inspect+.
def filter_attributes=(attributes_names)
@filter_attributes = attributes_names.map(&:to_s).to_set
@filter_attributes = attributes_names.to_set
Copy link
Member

@kamipo kamipo Oct 13, 2018

Now .to_set has been useless since self.class.filter_attributes.include?(attribute_name) is replaced.

Copy link
Member

@jeremy jeremy Oct 13, 2018

We could save the cost of recompiling a ParameterFilter per #inspect call by instantiating it on the class when #filter_attributes= is called. For example:

 
       # Returns columns which shouldn't be exposed while calling +#inspect+.
       def filter_attributes
-        if defined?(@filter_attributes)
-          @filter_attributes
-        else
-          superclass.filter_attributes
-        end
+        inspect_filter.filters # needs a new ParameterFilter.attr_reader :filters
       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
+        self.inspect_filter = ActiveSupport::ParameterFilter.new(attributes_names)
+      end
+
+      # Or provide your own…
+      attr_writer :inspect_filter #:nodoc:
+
+      # Returns columns which shouldn't be exposed while calling +#inspect+.
+      def inspect_filter #:nodoc:
+        if defined?(@inspect_filter)
+          @inspect_filter
+        else
+          superclass.inspect_filter
+        end
       end
 
       # Returns a string like 'Post(id:integer, title:string, body:text)'

Copy link
Contributor Author

@yskkin yskkin Oct 14, 2018

I think inspect or pretty_print is not called in production for typical application.
So I will go like this to defer allocation.

      def inspection_filter
        @inspection_filter ||= ActiveSupport::ParameterFilter.new(self.class.filter_attributes, expose: [nil])
      end

end
o
end
filter.filter(hash, filter_nil: false).transform_values! { |v| v.nil? ? v.inspect : v }
Copy link
Member

@kamipo kamipo Oct 13, 2018

I think that anyone probably have no strong opinion to filter a value except nil.
So how about always filtering values whether or not considered as sensitive value?

Copy link
Contributor Author

@yskkin yskkin Oct 13, 2018

It's @matthewd's suggestion and also have a test case.
#33756 (comment)

test "filter_attributes should not filter nil value" do

Can we change this?

# Mask sensitive data and format value for inspection.
# +nil+ is not filtered even if it matches +filter_parameter+.
def attributes_for_inspect(filter_parameter)
filter = ActiveSupport::ParameterFilter.new(filter_parameter)
Copy link
Member

@kamipo kamipo Oct 13, 2018

The attributes_for_inspect is internal API and is always passed self.class.filter_attributes, so we could just use ActiveSupport::ParameterFilter.new(self.class.filter_attributes) here instead of passing as a method argument.

end
end
attributes_for_inspect(self.class.filter_attributes).map do |name, value|
"#{name}: #{value}"
end.compact.join(", ")
Copy link
Member

@kamipo kamipo Oct 13, 2018

.compact is no longer needed.

Copy link
Member

@jeremy jeremy left a comment

This is a great change. It exposes new demands on ParameterFilter that we should probably satisfy. I left some review comments as well as suggestions for refactoring and API naming for you to consider.

In summary:

  • we can store one ParameterFilter instance rather than the list of filtered attributes
  • ParameterFilter should allow us to filter a single name, value pair rather than a full params hash
  • we needn't introduce new attributes_for_inspect public API, but we can extract a private API for attribute_names_for_inspect that's used by #inspect and #pretty_print
  • we need API to format an attribute value for #inspect, but attribute_for_inspect takes a name argument only

@@ -463,6 +464,20 @@ def attributes_for_create(attribute_names)
end
end

# Mask sensitive data and format value for inspection.
# +nil+ is not filtered even if it matches +filter_parameter+.
def attributes_for_inspect(filter_parameter)
Copy link
Member

@jeremy jeremy Oct 13, 2018

Rather than expose public API now, let's start with a private method in Core that supports #inspect and #pretty_print. Public API may have different expectations; for example, #attribute_for_inspect doesn't filter.

Copy link
Member

@jeremy jeremy Oct 13, 2018

Furthermore, if we move this to a private #attributes_for_inspect then we could rely on self.class.inspect_filter (see comment below) to provide the ParameterFilter instance, and we needn't take an attribute list argument.

@@ -240,7 +238,7 @@ def filter_attributes

# Specifies columns which shouldn't be exposed while calling +#inspect+.
def filter_attributes=(attributes_names)
@filter_attributes = attributes_names.map(&:to_s).to_set
@filter_attributes = attributes_names.to_set
Copy link
Member

@jeremy jeremy Oct 13, 2018

We could save the cost of recompiling a ParameterFilter per #inspect call by instantiating it on the class when #filter_attributes= is called. For example:

 
       # Returns columns which shouldn't be exposed while calling +#inspect+.
       def filter_attributes
-        if defined?(@filter_attributes)
-          @filter_attributes
-        else
-          superclass.filter_attributes
-        end
+        inspect_filter.filters # needs a new ParameterFilter.attr_reader :filters
       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
+        self.inspect_filter = ActiveSupport::ParameterFilter.new(attributes_names)
+      end
+
+      # Or provide your own…
+      attr_writer :inspect_filter #:nodoc:
+
+      # Returns columns which shouldn't be exposed while calling +#inspect+.
+      def inspect_filter #:nodoc:
+        if defined?(@inspect_filter)
+          @inspect_filter
+        else
+          superclass.inspect_filter
+        end
       end
 
       # Returns a string like 'Post(id:integer, title:string, body:text)'

value = read_attribute(attr_name)
o[attr_name] = value.nil? ? value : attribute_for_inspect(attr_name)
end
o
Copy link
Member

@jeremy jeremy Oct 13, 2018

We can switch #inspect to #each_with_object and omit the return value.

#
# * <tt>:filter_nil</tt> - Filter value even if it is +nil+ for +String+ or +Regex+ filters. +Proc+ filters are not affedted by this value. Defaults to +true+.
def filter(params, filter_nil: true)
compiled_filter.call(params, filter_nil: filter_nil)
Copy link
Member

@jeremy jeremy Oct 13, 2018

If a ParameterFilter is always meant to allow nil values, let's pass the option to #initialize.

Since our filters language always means filtering parameter keys, a new filter_nil option would suggest that we filter nil keys rather than nil values. Is there other language that suggests that we bypass the filters for certain values? Some possibilities:

# Too clunky? "Match keys to these filters, excepting nil values"
ParameterFilter.new(filters, except_nil_values: true)

# Too generic? "Match keys to these filters, with these exceptions"
ParameterFilter.new(filters, except: ->(key, value) { value.nil? ||  })

# Too particular? "Match keys to these filters, but treat these values as exceptions"
ParameterFilter.new(filters, except_values: [ nil, "" ])

# Too unclear? "Match keys to these filters, but expose these values anyway"
ParameterFilter.new(filters, expose: [ nil, "" ])

end
o
end
filter.filter(hash, filter_nil: false).transform_values! { |v| v.nil? ? v.inspect : v }
Copy link
Member

@jeremy jeremy Oct 13, 2018

This usage suggests that a ParameterFilter#filter_param(key, value) API would be desirable so we don't have to preprocess, filter, then postprocess an attributes hash. Instead we could enumerate attributes directly:

def attributes_for_inspect
  attributes.each_with_object({}) do |(attr_name, value), filtered|
    filtered[attr_name] = attribute_value_for_inspect(self.class.inspect_filter.filter_param(attr_name, value)))
  end
end

# change implementation to delegate to new value-formatter method…
def attribute_for_inspect(attr_name)
  attribute_value_for_inspect(read_attribute(attr_name))
end

# Extract implementation from current attribute_for_inspect
def attribute_value_for_inspect(value)
  
end

Copy link
Contributor Author

@yskkin yskkin Oct 13, 2018

I also considered that path, but gave up since proc filters with arity of 3 need an entire hash.

blocks.each { |b| b.arity == 2 ? b.call(key, value) : b.call(key, value, original_params) }

What should filter_param(key, value) API do if proc filter which takes three args is given?
Passing nil as third block parameter may be imcompatible since filters do not expect to get nil

Copy link
Contributor Author

@yskkin yskkin Oct 14, 2018

filter_param is new API so backward compatibility may not be severe than I thought.
I will add it.

end
end
attributes_for_inspect(self.class.filter_attributes).map do |name, value|
"#{name}: #{value}"
Copy link
Member

@jeremy jeremy Oct 13, 2018

Only #inspect wants truncated strings and formatted Date values, so perhaps it should be calling #attribute_value_for_inspect here, rather than expecting #attributes_for_inspect to provide them:

attributes_for_inspect.map do |name, value|
  "#{name}: #{attribute_value_for_inspect(value)}"
end.join(", ")

@@ -534,17 +526,17 @@ def pretty_print(pp)
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|
inspect_hash = attributes_for_inspect(self.class.filter_attributes)
Copy link
Member

@jeremy jeremy Oct 13, 2018

Both #inspect and #pretty_print need a list of attribute names to display. They don't share handling of the filtered values, however. That suggests extracting #attribute_names_for_inspect rather than an #attributes_for_inspect.

Then we may use the new single-parameter ParameterFilter API below instead of using a conditional:

pp.pp self.class.inspect_filter.filter_param(attr_name, value)

@@ -534,17 +533,18 @@ def pretty_print(pp)
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? }
Copy link
Contributor Author

@yskkin yskkin Oct 14, 2018

I think column_names here was left over from 8ab9daf so changed to attribute_names.

@@ -463,6 +457,16 @@ def attributes_for_create(attribute_names)
end
end

def attribute_value_for_inspect(value)
Copy link
Contributor

@albertoalmagro albertoalmagro Oct 14, 2018

I'm not sure about the name of this method... I think something like format_for_inspect(value) reveals better the method's intention. Having said this... I also dislike the public API's method name attribute_for_inspect, but that isn't in the scope of this PR.

@@ -28,12 +28,27 @@ module ActiveSupport
class ParameterFilter
FILTERED = "[FILTERED]" # :nodoc:

def initialize(filters = [])
# Create instance with given filters. Supported type of filter is +String+, +Regexp+, and +Proc+.
Copy link
Contributor

@albertoalmagro albertoalmagro Oct 14, 2018

Typo:

Supported type of filter is +String+, +Regexp+, and +Proc+. ->
Supported types of filters are +String+, +Regexp+, and +Proc+.

@@ -28,12 +28,27 @@ module ActiveSupport
class ParameterFilter
FILTERED = "[FILTERED]" # :nodoc:

def initialize(filters = [])
# Create instance with given filters. Supported type of filter is +String+, +Regexp+, and +Proc+.
# Other type of filters are treated as +String+ using +to_s+.
Copy link
Contributor

@albertoalmagro albertoalmagro Oct 14, 2018

Typo: type -> types

jeremy
jeremy approved these changes Oct 15, 2018
Copy link
Member

@jeremy jeremy left a comment

Looking good for merge. Great work. Please squash commits and add a changelog entry 👍

#
# * <tt>:expose</tt> - Values that will not be masked even if key matches one of +String+ or +Regex+ filters.
# +Proc+ filters are not affected by this value. Defaults to <tt>[]</tt>.
def initialize(filters = [], expose: [])
Copy link
Member

@kamipo kamipo Oct 15, 2018

The :expose option is only implemented a string filter, not implemented yet regex and proc filters, how this option should be implemented is out of scope in this PR though.

The reason that this option is needed is to make filter_attributes behave differently than http params filter.

My preference is consistent behavior and less code, so the extra code for the purpose makes less sense to me.

Copy link
Member

@kamipo kamipo Oct 15, 2018

ah I've misreading the doc, the :expose option behave filtering keys by filters first, then excluding values of the matched keys if values exactly matches in the expose collection.

Copy link
Member

@kamipo kamipo Oct 15, 2018

At least we could implement more less code for the purpose even without this option.

diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb
index b83facf4b7..47fc382d70 100644
--- a/activerecord/lib/active_record/core.rb
+++ b/activerecord/lib/active_record/core.rb
@@ -540,7 +540,8 @@ def pretty_print(pp)
               pp.text attr_name
               pp.text ":"
               pp.breakable
-              value = inspection_filter.filter_param(attr_name, read_attribute(attr_name))
+              value = read_attribute(attr_name)
+              value = inspection_filter.filter_param(attr_name, value) unless value.nil?
               if value == ActiveSupport::ParameterFilter::FILTERED
                 pp.text value
               else
@@ -598,7 +599,7 @@ def custom_inspect_method_defined?
       end
 
       def inspection_filter
-        @inspection_filter ||= ActiveSupport::ParameterFilter.new(self.class.filter_attributes, expose: [nil])
+        @inspection_filter ||= ActiveSupport::ParameterFilter.new(self.class.filter_attributes)
       end
   end
 end

if filter_attribute?(column_name)
pp.text ActiveRecord::Core::FILTERED
value = inspection_filter.filter_param(attr_name, read_attribute(attr_name))
if value == ActiveSupport::ParameterFilter::FILTERED
Copy link
Member

@kamipo kamipo Oct 15, 2018

If read_attribute(attr_name) returns "[FILTERED]", this condition would be true too even if the attribute is not filtered.

@yskkin
Copy link
Contributor Author

@yskkin yskkin commented Oct 16, 2018

I forgot to address method name of attribute_value_for_inspect.
I'll change it in next push/squash.

@yskkin yskkin force-pushed the inspect_with_parameter_filter branch from e1b020a to 924cc4e Oct 17, 2018
@yskkin
Copy link
Contributor Author

@yskkin yskkin commented Oct 18, 2018

I sqashed all the changes so far.
Do you have any other comment?
Thanks in advance ❤️

@matthewd
Copy link
Member

@matthewd matthewd commented Oct 18, 2018

I don't immediately have a better suggestion, and it doesn't need to block us merging this, but I'm a little uncomfortable with the fact it's called ParameterFilter now that it's being used to filter things that.. aren't parameters (except in the broadest possible "input value for code" sense of the word).

Does anyone else have feelings on that?

def to_s
ActiveSupport::ParameterFilter::FILTERED
end
alias inspect to_s
Copy link
Member

@kamipo kamipo Oct 18, 2018

Is this alias necessary? mask.inspect isn't called in our test cases.

And also, I'm not fond to reference ActiveSupport::ParameterFilter::FILTERED repeatedly.
How about defining pretty_print only?

diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb
index 34bde3fbe9..9780258302 100644
--- a/activerecord/lib/active_record/core.rb
+++ b/activerecord/lib/active_record/core.rb
@@ -596,16 +596,10 @@ def custom_inspect_method_defined?
 
       def inspection_filter
         @inspection_filter ||= begin
-          mask = Class.new do
-            def to_s
-              ActiveSupport::ParameterFilter::FILTERED
-            end
-            alias inspect to_s
-
-            def pretty_print(pp)
-              pp.text ActiveSupport::ParameterFilter::FILTERED
-            end
-          end.new.freeze
+          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

Copy link
Contributor Author

@yskkin yskkin Oct 19, 2018

I added alias for completeness but that is not necessary.
I'll update as you suggested.

AR instance support `filter_parameters` since rails#33756.
Though Regex or Proc is valid as `filter_parameters`,
they are not supported as AR#inspect.

I also add :mask option and #filter_params to
`ActiveSupport::ParameterFilter#new` to implement this.
@yskkin yskkin force-pushed the inspect_with_parameter_filter branch from 924cc4e to 32b03b4 Oct 19, 2018
kamipo
kamipo approved these changes Oct 19, 2018
Copy link
Member

@kamipo kamipo left a comment

👍

@yskkin
Copy link
Contributor Author

@yskkin yskkin commented Oct 26, 2018

As for the naming of ParameterFilter, I don't have strong opinion on it, but open to change it.

I came up with candidate like

  • SecurityFilter
  • HashFilter
  • ValueMask

That said, I feel changing class name is not the scope of this PR and inclined to just merge this and open new PR about discussing naming.

@kamipo
Copy link
Member

@kamipo kamipo commented Oct 26, 2018

Yeah, I think that changing the class name is not the scope of this PR too, it open to change though.

I think what to do naming the class depends on what filter is passed.

# Mask value of +params+ if key matches one of filters.
def filter(params)
compiled_filter.call(params)
end

Anyway, I'm going to merge this PR.

@kamipo kamipo merged commit 6149080 into rails:master Oct 26, 2018
2 checks passed
@yskkin yskkin deleted the inspect_with_parameter_filter branch Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants