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