Improve architecture for permit matcher

Why:

* There were architectural issues with how the permit matcher kept track
  of params instances on which doubles had been placed. Previously we
  were starting off by taking the ActionController::Parameters class and
  stubbing the #permit and #require instance method on it -- in other
  words, we were stubbing #require for all instances of
  ActionController::Parameters -- then we would stub #permit on a
  particular instance of ActionController::Parameters that #require
  returned. What this means is that if for some reason the #permit stub
  on an individual instance isn't working properly, then the #permit
  stub on ActionController::Parameters will respond to the invocation.
  This is exactly what happened for the issue we recently fixed --
  if the stubbing were done a different way we wouldn't have run into
  that issue.
* Also, there's no reason to have both ParametersDoubles and
  SliceOfParametersDoubles classes around. While it's nice that we have
  a simpler option to use if we don't need the more complex one, we
  actually don't need a distinction here, and we can afford one class
  that does both.

To satisfy the above:

* When stubbing #permit or #require, always do so on an instance of
  ActionController::Parameters and not the whole class. This way we know
  exactly which methods are being doubled and it's easier to debug things
  in the future.
* This means that we now stub ActionController::Parameters.new and then
  place stubs on the returned instance.
* Refactor ParametersDoubles and SliceOfParametersDoubles: combine them
  into a ParametersDoubleRegistry class, but extract the code that stubs
  ActionController::Parameters.new into
  a CompositeParametersDoubleRegistry class.
* Since this broke one of the tests, modify DoubleCollection so that a
  method cannot be doubled more than once -- if the method is already
  doubled then `register_stub` or `register_proxy` does nothing and
  returns the original Double.

lib/shoulda/matchers/action_controller/permit_matcher.rb

@@ -205,13 +205,13 @@ module Shoulda
       class PermitMatcher
         attr_writer :stubbed_params
 
-        def initialize(expected_permitted_params)
+        def initialize(expected_permitted_parameter_names)
-          @expected_permitted_params = expected_permitted_params
+          @expected_permitted_parameter_names = expected_permitted_parameter_names
           @action = nil
           @verb = nil
           @request_params = {}
-          @subparameter = nil
+          @subparameter_name = nil
-          @parameters_doubles = ParametersDoubles.new
+          @parameters_double_registry = CompositeParametersDoubleRegistry.new
         end
 
         def for(action, options = {})
@@ -226,9 +226,8 @@ module Shoulda
           self
         end
 
-        def on(subparameter)
+        def on(subparameter_name)
-          @subparameter = subparameter
+          @subparameter_name = subparameter_name
-          @parameters_doubles = SliceOfParametersDoubles.new(subparameter)
           self
         end
 
@@ -245,13 +244,13 @@ module Shoulda
           @controller = controller
           ensure_action_and_verb_present!
 
-          parameters_doubles.register
+          parameters_double_registry.register
 
           Doublespeak.with_doubles_activated do
             context.__send__(verb, action, request_params)
           end
 
-          unpermitted_params.empty?
+          unpermitted_parameter_names.empty?
         end
 
         def failure_message
@@ -264,50 +263,58 @@ module Shoulda
 
         protected
 
-        attr_reader :controller, :double_collections_by_param, :action, :verb,
+        attr_reader :controller, :double_collections_by_parameter_name, :action, :verb,
-          :request_params, :expected_permitted_params, :context, :subparameter,
+          :request_params, :expected_permitted_parameter_names, :context, :subparameter_name,
-          :parameters_doubles
+          :parameters_double_registry
 
         def expectation
           message = 'restrict parameters '
 
-          if subparameter
+          if subparameter_name
-            message << "on #{subparameter.inspect} "
+            message << "on #{subparameter_name.inspect} "
           end
 
-          message << 'to ' + format_param_names(expected_permitted_params)
+          message << 'to ' + format_parameter_names(expected_permitted_parameter_names)
 
           message
         end
 
         def reality
-          if actual_permitted_params.empty?
+          if actual_permitted_parameter_names.empty?
             'it did not restrict any parameters'
           else
             'the restricted parameters were ' +
-              format_param_names(actual_permitted_params) +
+              format_parameter_names(actual_permitted_parameter_names) +
               ' instead'
           end
         end
 
-        def format_param_names(param_names)
+        def format_parameter_names(parameter_names)
-          param_names.map(&:inspect).to_sentence
+          parameter_names.map(&:inspect).to_sentence
         end
 
-        def actual_permitted_params
+        def actual_permitted_parameter_names
-          parameters_doubles.permitted_params
+          @_actual_permitted_parameter_names ||= begin
+            if subparameter_name
+              options = { for: subparameter_name }
+            else
+              options = {}
+            end
+
+            parameters_double_registry.permitted_parameter_names(options)
+          end
         end
 
         def permit_called?
-          actual_permitted_params.any?
+          actual_permitted_parameter_names.any?
         end
 
-        def unpermitted_params
+        def unpermitted_parameter_names
-          expected_permitted_params - actual_permitted_params
+          expected_permitted_parameter_names - actual_permitted_parameter_names
         end
 
-        def verified_permitted_params
+        def verified_permitted_parameter_names
-          expected_permitted_params & actual_permitted_params
+          expected_permitted_parameter_names & actual_permitted_parameter_names
         end
 
         def ensure_action_and_verb_present!
@@ -327,57 +334,63 @@ module Shoulda
           end
         end
 
-        def param_names_as_sentence
+        def parameter_names_as_sentence
-          expected_permitted_params.map(&:inspect).to_sentence
+          expected_permitted_parameter_names.map(&:inspect).to_sentence
         end
 
         # @private
-        class ParametersDoubles
+        class CompositeParametersDoubleRegistry
-          def self.permitted_params_within(double_collection)
-            double_collection.calls_to(:permit).map(&:args).flatten
-          end
-
           def initialize
-            klass = ::ActionController::Parameters
+            @parameters_double_registries_by_params = {}
-            @double_collection = Doublespeak.double_collection_for(klass)
           end
 
           def register
-            double_collection.register_proxy(:permit)
+            double_collection = Doublespeak.double_collection_for(
+              ::ActionController::Parameters.singleton_class
+            )
+            double_collection.register_proxy(:new).to_return do |call|
+              params = call.return_value
+              parameters_double_registry = ParametersDoubleRegistry.new(params)
+              parameters_double_registry.register
+              parameters_double_registries_by_params[params] =
+                parameters_double_registry
+            end
           end
 
-          def permitted_params
+          def permitted_parameter_names(options = {})
-            ParametersDoubles.permitted_params_within(double_collection)
+            parameters_double_registries_by_params.flat_map do |params, double_registry|
+              double_registry.permitted_parameter_names(options)
+            end
           end
 
           protected
 
-          attr_reader :double_collection
+          attr_reader :parameters_double_registries_by_params
         end
 
         # @private
-        class SliceOfParametersDoubles
+        class ParametersDoubleRegistry
           TOP_LEVEL = Object.new
 
-          def initialize(subparameter)
+          def self.permitted_parameter_names_within(double_collection)
-            klass = ::ActionController::Parameters
+            double_collection.calls_to(:permit).map(&:args).flatten
+          end
 
-            @subparameter = subparameter
+          def initialize(params)
-            @double_collections_by_param = {
+            @params = params
-              TOP_LEVEL => Doublespeak.double_collection_for(klass)
+            @double_collections_by_parameter_name = {}
-            }
           end
 
           def register
-            top_level_collection = double_collections_by_param[TOP_LEVEL]
+            register_double_for_permit_against(params, TOP_LEVEL)
-            double_permit_on(top_level_collection)
-            double_require_on(top_level_collection)
           end
 
-          def permitted_params
+          def permitted_parameter_names(args = {})
-            if double_collections_by_param.key?(subparameter)
+            subparameter_name = args.fetch(:for, TOP_LEVEL)
-              ParametersDoubles.permitted_params_within(
+
-                double_collections_by_param[subparameter]
+            if double_collections_by_parameter_name.key?(subparameter_name)
+              self.class.permitted_parameter_names_within(
+                double_collections_by_parameter_name[subparameter_name]
               )
             else
               []
@@ -386,31 +399,30 @@ module Shoulda
 
           protected
 
-          attr_reader :subparameter, :double_collections_by_param
+          attr_reader :params, :double_collections_by_parameter_name
 
           private
 
-          def double_permit_on(double_collection)
+          def register_double_for_permit_against(params, subparameter_name)
+            klass = params.singleton_class
+
+            double_collection = Doublespeak.double_collection_for(klass)
+            register_double_for_permit_on(double_collection)
+            register_double_for_require_on(double_collection)
+
+            double_collections_by_parameter_name[subparameter_name] =
+              double_collection
+          end
+
+          def register_double_for_permit_on(double_collection)
             double_collection.register_proxy(:permit)
           end
 
-          def double_require_on(double_collection)
+          def register_double_for_require_on(double_collection)
-            double_collections_by_param = @double_collections_by_param
+            double_collection.register_proxy(:require).to_return do |call|
-            require_double = double_collection.register_proxy(:require)
-
-            require_double.to_return do |call|
-              param_name = call.args.first
               params = call.return_value
-              double_collections_by_param[param_name] ||=
+              subparameter_name = call.args.first
-                double_permit_against(params)
+              register_double_for_permit_against(params, subparameter_name)
-            end
-          end
-
-          def double_permit_against(params)
-            klass = params.singleton_class
-
-            Doublespeak.double_collection_for(klass).tap do |double_collection|
-              double_permit_on(double_collection)
             end
           end
         end

lib/shoulda/matchers/doublespeak/double_collection.rb

@@ -50,6 +50,7 @@ module Shoulda
         attr_reader :world, :klass, :doubles_by_method_name
 
         def register_double(method_name, implementation_type)
+          doubles_by_method_name.fetch(method_name) do
             implementation =
               DoubleImplementationRegistry.find(implementation_type)
             double = Double.new(world, klass, method_name, implementation)
@@ -59,4 +60,5 @@ module Shoulda
         end
       end
     end
+  end
 end

spec/unit/shoulda/matchers/doublespeak/double_collection_spec.rb

@@ -26,6 +26,36 @@ module Shoulda::Matchers::Doublespeak
           to have_received(:new).
           with(world, :klass, :a_method, :implementation)
       end
+
+      context 'if a double has already been registered for the method' do
+        it 'does not call Double.new again' do
+          world = build_world
+          allow(DoubleImplementationRegistry).
+            to receive(:find).
+            and_return(:implementation)
+          allow(Double).to receive(:new)
+          double_collection = described_class.new(world, :klass)
+
+          double_collection.register_stub(:a_method)
+          double_collection.register_stub(:a_method)
+
+          expect(Double).to have_received(:new).once
+        end
+
+        it 'returns the same Double' do
+          world = build_world
+          allow(DoubleImplementationRegistry).
+            to receive(:find).
+            and_return(:implementation)
+          allow(Double).to receive(:new)
+          double_collection = described_class.new(world, :klass)
+
+          double1 = double_collection.register_stub(:a_method)
+          double2 = double_collection.register_stub(:a_method)
+
+          expect(double1).to equal(double2)
+        end
+      end
     end
 
     describe '#register_proxy' do
@@ -54,6 +84,36 @@ module Shoulda::Matchers::Doublespeak
           to have_received(:new).
           with(world, :klass, :a_method, :implementation)
       end
+
+      context 'if a double has already been registered for the method' do
+        it 'does not call Double.new again' do
+          world = build_world
+          allow(DoubleImplementationRegistry).
+            to receive(:find).
+            and_return(:implementation)
+          allow(Double).to receive(:new)
+          double_collection = described_class.new(world, :klass)
+
+          double_collection.register_proxy(:a_method)
+          double_collection.register_proxy(:a_method)
+
+          expect(Double).to have_received(:new).once
+        end
+
+        it 'returns the same Double' do
+          world = build_world
+          allow(DoubleImplementationRegistry).
+            to receive(:find).
+            and_return(:implementation)
+          allow(Double).to receive(:new)
+          double_collection = described_class.new(world, :klass)
+
+          double1 = double_collection.register_proxy(:a_method)
+          double2 = double_collection.register_proxy(:a_method)
+
+          expect(double1).to equal(double2)
+        end
+      end
     end
 
     describe '#activate' do