Clean up parameter logging some

2 files changed, 32 insertions, 51 deletions

diff --git a/actionpack/lib/action_controller/metal/filter_parameter_logging.rb b/actionpack/lib/action_controller/metal/filter_parameter_logging.rb
index 4259d9de19..a53c052075 100644
--- a/actionpack/lib/action_controller/metal/filter_parameter_logging.rb
+++ b/actionpack/lib/action_controller/metal/filter_parameter_logging.rb
@@ -4,10 +4,6 @@ module ActionController
 
     include AbstractController::Logger
 
-    included do
-      include InstanceMethodsForNewBase
-    end
-
     module ClassMethods
       # Replace sensitive parameter data from the request log.
       # Filters parameters that have any of the arguments as a substring.
@@ -17,8 +13,6 @@ module ActionController
       # can be replaced using String#replace or similar method.
       #
       # Examples:
-      #   filter_parameter_logging
-      #   => Does nothing, just slows the logging process down
       #
       #   filter_parameter_logging :password
       #   => replaces the value to all keys matching /password/i with "[FILTERED]"
@@ -33,64 +27,51 @@ module ActionController
       #   => reverses the value to all keys matching /secret/i, and
       #      replaces the value to all keys matching /foo|bar/i with "[FILTERED]"
       def filter_parameter_logging(*filter_words, &block)
-        parameter_filter = Regexp.new(filter_words.collect{ |s| s.to_s }.join('|'), true) if filter_words.length > 0
+        raise "You must filter at least one word from logging" if filter_words.empty?
+
+        parameter_filter = Regexp.new(filter_words.join('|'), true)
 
-        define_method(:filter_parameters) do |unfiltered_parameters|
-          filtered_parameters = {}
+        define_method(:filter_parameters) do |original_params|
+          filtered_params = {}
 
-          unfiltered_parameters.each do |key, value|
+          original_params.each do |key, value|
             if key =~ parameter_filter
-              filtered_parameters[key] = '[FILTERED]'
+              value = '[FILTERED]'
             elsif value.is_a?(Hash)
-              filtered_parameters[key] = filter_parameters(value)
+              value = filter_parameters(value)
             elsif value.is_a?(Array)
-              filtered_parameters[key] = value.collect do |item|
-                filter_parameters(item)
-              end
+              value = value.map { |item| filter_parameters(item) }
             elsif block_given?
               key = key.dup
               value = value.dup if value.duplicable?
               yield key, value
-              filtered_parameters[key] = value
-            else
-              filtered_parameters[key] = value
             end
+
+            filtered_params[key] = value
           end
 
-          filtered_parameters
+          filtered_params
         end
         protected :filter_parameters
       end
     end
 
-    module InstanceMethodsForNewBase
-      # TODO : Fix the order of information inside such that it's exactly same as the old base
-      def process(*)
-        ret = super
-
-        if logger
-          parameters = respond_to?(:filter_parameters) ? filter_parameters(params) : params.dup
-          parameters = parameters.except!(:controller, :action, :format, :_method, :only_path)
+    INTERNAL_PARAMS = [:controller, :action, :format, :_method, :only_path]
 
-          unless parameters.empty?
-            # TODO : Move DelayedLog to AS
-            log = AbstractController::Logger::DelayedLog.new { "  Parameters: #{parameters.inspect}" }
-            logger.info(log)
-          end
-        end
-
-        ret
+    def process(*)
+      response = super
+      if logger
+        parameters = filter_parameters(params).except!(*INTERNAL_PARAMS)
+        logger.info { "  Parameters: #{parameters.inspect}" } unless parameters.empty?
       end
+      response
     end
 
-    private
+  protected
 
-    # TODO : This method is not needed for the new base
-    def log_processing_for_parameters
-      parameters = respond_to?(:filter_parameters) ? filter_parameters(params) : params.dup
-      parameters = parameters.except!(:controller, :action, :format, :_method)
-
-      logger.info "  Parameters: #{parameters.inspect}" unless parameters.empty?
+    def filter_parameters(params)
+      params.dup
     end
+
   end
 end
diff --git a/actionpack/test/controller/filter_params_test.rb b/actionpack/test/controller/filter_params_test.rb
index 19232c6bc9..43bef34885 100644
--- a/actionpack/test/controller/filter_params_test.rb
+++ b/actionpack/test/controller/filter_params_test.rb
@@ -19,23 +19,23 @@ class FilterParamTest < ActionController::TestCase
     
     def method_missing(method, *args)
       @logged ||= []
-      @logged << args.first
+      @logged << args.first unless block_given?
+      @logged << yield if block_given?
     end
   end
 
   setup :set_logger
 
+  def test_filter_parameters_must_have_one_word
+    assert_raises RuntimeError do
+      FilterParamController.filter_parameter_logging
+    end
+  end
+
   def test_filter_parameters
     assert FilterParamController.respond_to?(:filter_parameter_logging)
-    assert !@controller.respond_to?(:filter_parameters)
-
-    FilterParamController.filter_parameter_logging
-    assert @controller.respond_to?(:filter_parameters)
 
-    test_hashes = [[{},{},[]],
-    [{'foo'=>nil},{'foo'=>nil},[]],
-    [{'foo'=>'bar'},{'foo'=>'bar'},[]],
-    [{'foo'=>1},{'foo'=>1},[]],
+    test_hashes = [
     [{'foo'=>'bar'},{'foo'=>'bar'},%w'food'],
     [{'foo'=>'bar'},{'foo'=>'[FILTERED]'},%w'foo'],
     [{'foo'=>'bar', 'bar'=>'foo'},{'foo'=>'[FILTERED]', 'bar'=>'foo'},%w'foo baz'],