From 2d52f537f98d41ba7520b0cb35d346901302876e Mon Sep 17 00:00:00 2001
From: Elliot Winkler <elliot.winkler@gmail.com>
Date: Wed, 16 Apr 2014 02:58:05 -0600
Subject: [PATCH] Fix the `permit` matcher to be less intrusive

Within the `permit` matcher, we need to keep track of when #permit was
called and which arguments it was called with. Previously we did this by
overriding ActionController::Parameters#[] to return an instance of
StubbedParameters (which, when #permit was called on it, would then
track the call).

This has three problems:

* We're overriding something in a global class. We can simply override
  something on the controller that's being tested instead.
* Overriding #[] is kind of magical. How does this even end up doing
  what we want? Really we should just replace the entire params hash.
* Controller tests involve hitting actions on real controllers. Those
  actions may need to access the params hash. Completely changing how
  the params hash works prevents them from being able to do that.

To solve this, we override the params hash on the controller under test
with an instance of StubbedParameters, which decorates the original
params hash. We then override #permit to track the call, but then we let
it do its thing.
---
 NEWS.md                                       |   5 +-
 .../strong_parameters_matcher.rb              | 110 +++++++++---------
 .../strong_parameters_matcher_spec.rb         | 101 ++++++++++------
 spec/support/controller_builder.rb            |  12 +-
 4 files changed, 134 insertions(+), 94 deletions(-)

diff --git a/NEWS.md b/NEWS.md
index c9591792..a06ef17e 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -1,7 +1,7 @@
 # 2.6.1
 
 * Fix `ComparisonMatcher` so that `validate_numericality_of` comparison matchers
-  work with large numbers (#483).
+  work with large numbers.
 
 * Fix so that ActiveRecord matchers aren't included when ActiveRecord
   isn't defined (i.e. if you are using ActiveModel only).
@@ -11,6 +11,9 @@
   `validate_presence_of` when used in conjunction with `has_secure_password`.
   That fix has been updated so that it does not affect `allow_value`.
 
+* Fix `permit` so that it does not interfere with the existing `params` hash of
+  the controller you're testing.
+
 # 2.6.0
 
 * The boolean argument to `have_db_index`'s `unique` option is now optional, for
diff --git a/lib/shoulda/matchers/action_controller/strong_parameters_matcher.rb b/lib/shoulda/matchers/action_controller/strong_parameters_matcher.rb
index 481a386f..019f4c12 100644
--- a/lib/shoulda/matchers/action_controller/strong_parameters_matcher.rb
+++ b/lib/shoulda/matchers/action_controller/strong_parameters_matcher.rb
@@ -1,8 +1,12 @@
+require 'delegate'
+
 begin
   require 'strong_parameters'
 rescue LoadError
 end
 
+require 'active_support/hash_with_indifferent_access'
+
 module Shoulda
   module Matchers
     module ActionController
@@ -11,19 +15,12 @@ module Shoulda
       end
 
       class StrongParametersMatcher
-        def self.stubbed_parameters_class
-          @stubbed_parameters_class ||= build_stubbed_parameters_class
-        end
-
-        def self.build_stubbed_parameters_class
-          Class.new(::ActionController::Parameters) do
-            include StubbedParameters
-          end
-        end
+        attr_writer :stubbed_params
 
         def initialize(context = nil, attributes)
           @attributes = attributes
           @context = context
+          @stubbed_params = NullStubbedParameters.new
         end
 
         def for(action, options = {})
@@ -41,11 +38,13 @@ module Shoulda
           "permit #{verb.upcase} ##{action} to receive parameters #{attributes_as_sentence}"
         end
 
-        def matches?(controller = nil)
+        def matches?(controller)
+          @controller = controller
           simulate_controller_action && parameters_difference.empty?
         end
 
-        def does_not_match?(controller = nil)
+        def does_not_match?(controller)
+          @controller = controller
           simulate_controller_action && parameters_intersection.empty?
         end
 
@@ -61,50 +60,48 @@ module Shoulda
 
         private
 
-        attr_reader :verb, :action, :attributes, :context
+        attr_reader :controller, :verb, :action, :attributes, :context
 
         def simulate_controller_action
           ensure_action_and_verb_present!
-          stub_model_attributes
+          stub_params
 
           begin
             context.send(verb, action)
           ensure
-            unstub_model_attributes
+            unstub_params
           end
 
           verify_permit_call
         end
 
         def verify_permit_call
-          @model_attrs.permit_was_called
+          @stubbed_params.permit_was_called
         end
 
         def parameters_difference
-          attributes - @model_attrs.shoulda_permitted_params
+          attributes - @stubbed_params.shoulda_permitted_params
         end
 
         def parameters_intersection
-          attributes & @model_attrs.shoulda_permitted_params
+          attributes & @stubbed_params.shoulda_permitted_params
         end
 
-        def stub_model_attributes
-          @model_attrs = self.class.stubbed_parameters_class.new(arbitrary_attributes)
+        def stub_params
+          matcher = self
 
-          local_model_attrs = @model_attrs
-          ::ActionController::Parameters.class_eval do
-            alias_method :'shoulda_original_[]', :[]
+          controller.singleton_class.class_eval do
+            alias_method :__shoulda_original_params__, :params
 
-            define_method :[] do |*args|
-              local_model_attrs
+            define_method :params do
+              matcher.stubbed_params = StubbedParameters.new(__shoulda_original_params__)
             end
           end
         end
 
-        def unstub_model_attributes
-          ::ActionController::Parameters.class_eval do
-            alias_method :[], :'shoulda_original_[]'
-            undef_method :'shoulda_original_[]'
+        def unstub_params
+          controller.singleton_class.class_eval do
+            alias_method :params, :__shoulda_original_params__
           end
         end
 
@@ -117,10 +114,6 @@ module Shoulda
           end
         end
 
-        def arbitrary_attributes
-          {any_key: 'any_value'}
-        end
-
         def verb_for_action
           verb_lookup = { create: :post, update: :put }
           verb_lookup[action]
@@ -129,37 +122,44 @@ module Shoulda
         def attributes_as_sentence
           attributes.map(&:inspect).to_sentence
         end
-      end
 
-      module StrongParametersMatcher::StubbedParameters
-        extend ActiveSupport::Concern
+        class StubbedParameters < SimpleDelegator
+          attr_reader :permit_was_called, :shoulda_permitted_params
 
-        included do
-          attr_accessor :permit_was_called, :shoulda_permitted_params
+          def initialize(original_params)
+            super(original_params)
+            @permit_was_called = false
+          end
+
+          def require(*args)
+            self
+          end
+
+          def permit(*args)
+            @shoulda_permitted_params = args
+            @permit_was_called = true
+            super(*args)
+          end
         end
 
-        def initialize(*)
-          @permit_was_called = false
-          super
+        class NullStubbedParameters < ActiveSupport::HashWithIndifferentAccess
+          def permit_was_called; false; end
+          def shoulda_permitted_params; self; end
+          def require(*); self; end
+          def permit(*); self; end
         end
 
-        def permit(*args)
-          self.shoulda_permitted_params = args
-          self.permit_was_called = true
-          nil
+        class ActionNotDefinedError < StandardError
+          def message
+            'You must specify the controller action using the #for method.'
+          end
         end
-      end
 
-      class StrongParametersMatcher::ActionNotDefinedError < StandardError
-        def message
-          'You must specify the controller action using the #for method.'
-        end
-      end
-
-      class StrongParametersMatcher::VerbNotDefinedError < StandardError
-        def message
-          'You must specify an HTTP verb when using a non-RESTful action.' +
-          ' e.g. for(:authorize, verb: :post)'
+        class VerbNotDefinedError < StandardError
+          def message
+            'You must specify an HTTP verb when using a non-RESTful action.' +
+            ' e.g. for(:authorize, verb: :post)'
+          end
         end
       end
     end
diff --git a/spec/shoulda/matchers/action_controller/strong_parameters_matcher_spec.rb b/spec/shoulda/matchers/action_controller/strong_parameters_matcher_spec.rb
index 86a53576..2c7aa168 100644
--- a/spec/shoulda/matchers/action_controller/strong_parameters_matcher_spec.rb
+++ b/spec/shoulda/matchers/action_controller/strong_parameters_matcher_spec.rb
@@ -3,27 +3,27 @@ require 'spec_helper'
 describe Shoulda::Matchers::ActionController do
   describe "#permit" do
     it 'matches when the sent parameter is allowed' do
-      controller_class = controller_for_resource_with_strong_parameters(action: :create) do
+      controller_for_resource_with_strong_parameters(action: :create) do
         params.require(:user).permit(:name)
       end
 
-      expect(controller_class).to permit(:name).for(:create)
+      expect(@controller).to permit(:name).for(:create)
     end
 
     it 'does not match when the sent parameter is not allowed' do
-      controller_class = controller_for_resource_with_strong_parameters(action: :create) do
+      controller_for_resource_with_strong_parameters(action: :create) do
         params.require(:user).permit(:name)
       end
 
-      expect(controller_class).not_to permit(:admin).for(:create)
+      expect(@controller).not_to permit(:admin).for(:create)
     end
 
     it 'matches against multiple attributes' do
-      controller_class = controller_for_resource_with_strong_parameters(action: :create) do
+      controller_for_resource_with_strong_parameters(action: :create) do
         params.require(:user).permit(:name, :age)
       end
 
-      expect(controller_class).to permit(:name, :age).for(:create)
+      expect(@controller).to permit(:name, :age).for(:create)
     end
   end
 end
@@ -63,7 +63,7 @@ describe Shoulda::Matchers::ActionController::StrongParametersMatcher do
       end
 
       matcher = described_class.new([:name]).in_context(self).for(:create)
-      expect(matcher.matches?).to be_true
+      expect(matcher.matches?(@controller)).to be_true
     end
 
     it "is true for all the allowable attributes" do
@@ -72,7 +72,7 @@ describe Shoulda::Matchers::ActionController::StrongParametersMatcher do
       end
 
       matcher = described_class.new([:name, :age]).in_context(self).for(:create)
-      expect(matcher.matches?).to be_true
+      expect(matcher.matches?(@controller)).to be_true
     end
 
     it "is false when any attributes are not allowed" do
@@ -81,76 +81,104 @@ describe Shoulda::Matchers::ActionController::StrongParametersMatcher do
       end
 
       matcher = described_class.new([:name, :admin]).in_context(self).for(:create)
-      expect(matcher.matches?).to be_false
+      expect(matcher.matches?(@controller)).to be_false
     end
 
     it "is false when permit is not called" do
-      controller_for_resource_with_strong_parameters(action: :create) {}
+      controller_for_resource_with_strong_parameters(action: :create)
 
       matcher = described_class.new([:name]).in_context(self).for(:create)
-      expect(matcher.matches?).to be_false
+      expect(matcher.matches?(@controller)).to be_false
     end
 
     it "requires an action" do
+      controller_for_resource_with_strong_parameters
       matcher = described_class.new([:name])
-      expect { matcher.matches? }
+      expect { matcher.matches?(@controller) }
         .to raise_error(Shoulda::Matchers::ActionController::StrongParametersMatcher::ActionNotDefinedError)
     end
 
     it "requires a verb for non-restful action" do
+      controller_for_resource_with_strong_parameters
       matcher = described_class.new([:name]).for(:authorize)
-      expect { matcher.matches? }
+      expect { matcher.matches?(@controller) }
         .to raise_error(Shoulda::Matchers::ActionController::StrongParametersMatcher::VerbNotDefinedError)
     end
 
-    context 'Stubbing ActionController::Parameters#[]' do
-      it "does not permanently stub []" do
+    context 'stubbing params on the controller' do
+      it 'still allows the original params to be set and accessed' do
+        actual_value = nil
+
+        controller_for_resource_with_strong_parameters(action: :create) do
+          params[:foo] = 'bar'
+          actual_value = params[:foo]
+
+          params.require(:user).permit(:name)
+        end
+
+        matcher = described_class.new([:name]).in_context(self).for(:create)
+        matcher.matches?(@controller)
+
+        expect(actual_value).to eq 'bar'
+      end
+
+      it 'stubs the params while the controller action is being run' do
+        params_class = nil
+
+        controller_for_resource_with_strong_parameters(action: :create) do
+          params_class = params.class
+          params.require(:user).permit(:name)
+        end
+
+        matcher = described_class.new([:name]).in_context(self).for(:create)
+        matcher.matches?(@controller)
+
+        expect(params_class).to be described_class::StubbedParameters
+      end
+
+      it 'does not permanently stub params' do
         controller_for_resource_with_strong_parameters(action: :create) do
           params.require(:user).permit(:name)
         end
 
-        described_class.new([:name]).in_context(self).for(:create).matches?
+        matcher = described_class.new([:name]).in_context(self).for(:create)
+        matcher.matches?(@controller)
 
-        param = ActionController::Parameters.new(name: 'Ralph')[:name]
-        expect(param.singleton_class).not_to include(
-          Shoulda::Matchers::ActionController::StrongParametersMatcher::StubbedParameters
-        )
+        expect(@controller.params).to be_a(ActionController::Parameters)
       end
 
-      it 'prevents permanently overwriting [] on error' do
+      it 'prevents permanently stubbing params on error' do
         stub_controller_with_exception
 
         begin
-          described_class.new([:name]).in_context(self).for(:create).matches?
+          matcher = described_class.new([:name]).in_context(self).for(:create)
+          matcher.matches?(@controller)
         rescue SimulatedError
         end
 
-        param = ActionController::Parameters.new(name: 'Ralph')[:name]
-        expect(param.singleton_class).not_to include(
-          Shoulda::Matchers::ActionController::StrongParametersMatcher::StubbedParameters
-        )
+        expect(@controller.params).to be_a(ActionController::Parameters)
       end
     end
   end
 
   describe "failure message" do
     it "includes all missing attributes" do
-      controller_class = controller_for_resource_with_strong_parameters(action: :create) do
+      controller_for_resource_with_strong_parameters(action: :create) do
         params.require(:user).permit(:name, :age)
       end
 
       expect {
-        expect(controller_class).to permit(:name, :age, :city, :country).for(:create)
+        expect(@controller).to permit(:name, :age, :city, :country).for(:create)
       }.to fail_with_message("Expected controller to permit city and country, but it did not.")
     end
 
     it "includes all attributes that should not have been allowed but were" do
-      controller_class = controller_for_resource_with_strong_parameters(action: :create) do
+      controller_for_resource_with_strong_parameters(action: :create) do
         params.require(:user).permit(:name, :age)
       end
 
       expect {
-        expect(controller_class).not_to permit(:name, :age).for(:create)
+        expect(@controller).not_to permit(:name, :age).for(:create)
       }.to fail_with_message("Expected controller not to permit name and age, but it did.")
     end
   end
@@ -158,43 +186,46 @@ describe Shoulda::Matchers::ActionController::StrongParametersMatcher do
   describe "#for" do
     context "when given :create" do
       it "posts to the controller" do
+        controller = ActionController::Base.new
         context = stub('context', post: nil)
         matcher = described_class.new([:name]).in_context(context).for(:create)
 
-        matcher.matches?
+        matcher.matches?(controller)
         expect(context).to have_received(:post).with(:create)
       end
     end
 
     context "when given :update" do
       it "puts to the controller" do
+        controller = ActionController::Base.new
         context = stub('context', put: nil)
         matcher = described_class.new([:name]).in_context(context).for(:update)
 
-        matcher.matches?
+        matcher.matches?(controller)
         expect(context).to have_received(:put).with(:update)
       end
     end
 
     context "when given a custom action and verb" do
       it "deletes to the controller" do
+        controller = ActionController::Base.new
         context = stub('context', delete: nil)
         matcher = described_class.new([:name]).in_context(context).for(:hide, verb: :delete)
 
-        matcher.matches?
+        matcher.matches?(controller)
         expect(context).to have_received(:delete).with(:hide)
       end
     end
   end
 
   def stub_controller_with_exception
-    controller = define_controller('Examples') do
+    controller_class = define_controller('Examples') do
       def create
         raise SimulatedError
       end
     end
 
-    setup_rails_controller_test(controller)
+    setup_rails_controller_test(controller_class)
 
     define_routes do
       get 'examples', to: 'examples#create'
diff --git a/spec/support/controller_builder.rb b/spec/support/controller_builder.rb
index 2c7a7d42..b0ffbba1 100644
--- a/spec/support/controller_builder.rb
+++ b/spec/support/controller_builder.rb
@@ -59,9 +59,12 @@ module ControllerBuilder
   end
 
   def controller_for_resource_with_strong_parameters(options = {}, &block)
+    controller_name = 'Users'
+    block ||= -> { {} }
+
     define_model "User"
-    controller_class = define_controller "Users" do
-      define_method options.fetch(:action) do
+    controller_class = define_controller controller_name do
+      define_method options.fetch(:action, 'some_action') do
         @user = User.create(user_params)
         render nothing: true
       end
@@ -72,7 +75,10 @@ module ControllerBuilder
 
     setup_rails_controller_test(controller_class)
 
-    define_routes { resources :users }
+    collection_name = controller_name.
+      to_s.sub(/Controller$/, '').underscore.
+      to_sym
+    define_routes { resources(collection_name) }
 
     controller_class
   end