diff --git a/NEWS.md b/NEWS.md index 49fbe28c..d496f7e5 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,4 +1,4 @@ -# 2.6.1 +# HEAD * Fix `ComparisonMatcher` so that `validate_numericality_of` comparison matchers work with large numbers. @@ -13,8 +13,21 @@ * Fix callback matchers and correct test coverage. -* Fix `permit` so that it does not interfere with the existing `params` hash of - the controller you're testing. +* Fix `permit` so that it does not interfere with different usages of `params` + in your controller action. Specifically, this will not raise an error: + `params.fetch(:foo, {}).permit(:bar, :baz)` (the `permit` will have no + problems recognizing that :bar and :baz are permitted params). + +* Fix `permit` on Rails 4.1 to use PATCH by default for #update instead of PUT. + Previously you had to specify this manually. + +* Fix `permit` so that it track multiple calls to #permit in your controller + action. Previously only the last usage of #permit would be considered in + determining whether the matcher matched. + +* Fix `permit` so that if the route for your action requires params (such as id) + then you can now specify those params: + `permit(:first_name, :last_name).for(:update, params: { id: 42 })`. # 2.6.0 diff --git a/lib/shoulda/matchers/action_controller/strong_parameters_matcher.rb b/lib/shoulda/matchers/action_controller/strong_parameters_matcher.rb index 019f4c12..7fd442a1 100644 --- a/lib/shoulda/matchers/action_controller/strong_parameters_matcher.rb +++ b/lib/shoulda/matchers/action_controller/strong_parameters_matcher.rb @@ -10,22 +10,25 @@ require 'active_support/hash_with_indifferent_access' module Shoulda module Matchers module ActionController - def permit(*attributes) - StrongParametersMatcher.new(self, attributes) + def permit(*params) + StrongParametersMatcher.new(params).in_context(self) end class StrongParametersMatcher attr_writer :stubbed_params - def initialize(context = nil, attributes) - @attributes = attributes - @context = context - @stubbed_params = NullStubbedParameters.new + def initialize(expected_permitted_params) + @action = nil + @verb = nil + @request_params = {} + @expected_permitted_params = expected_permitted_params + set_double_collection end def for(action, options = {}) @action = action - @verb = options[:verb] || verb_for_action + @verb = options.fetch(:verb, default_verb) + @request_params = options.fetch(:params, {}) self end @@ -35,118 +38,80 @@ module Shoulda end def description - "permit #{verb.upcase} ##{action} to receive parameters #{attributes_as_sentence}" + "permit #{verb.upcase} ##{action} to receive parameters #{param_names_as_sentence}" end def matches?(controller) @controller = controller - simulate_controller_action && parameters_difference.empty? - end + ensure_action_and_verb_present! - def does_not_match?(controller) - @controller = controller - simulate_controller_action && parameters_intersection.empty? + Doublespeak.with_doubles_activated do + context.__send__(verb, action, request_params) + end + + unpermitted_params.empty? end def failure_message - "Expected controller to permit #{parameters_difference.to_sentence}, but it did not." + "Expected controller to permit #{unpermitted_params.to_sentence}, but it did not." end alias failure_message_for_should failure_message def failure_message_when_negated - "Expected controller not to permit #{parameters_intersection.to_sentence}, but it did." + "Expected controller not to permit #{verified_permitted_params.to_sentence}, but it did." end alias failure_message_for_should_not failure_message_when_negated private - attr_reader :controller, :verb, :action, :attributes, :context + attr_reader :controller, :double_collection, :action, :verb, + :request_params, :expected_permitted_params, :context - def simulate_controller_action - ensure_action_and_verb_present! - stub_params + def set_double_collection + @double_collection = + Doublespeak.register_double_collection(::ActionController::Parameters) - begin - context.send(verb, action) - ensure - unstub_params - end - - verify_permit_call + @double_collection.register_stub(:require).to_return { |params| params } + @double_collection.register_proxy(:permit) end - def verify_permit_call - @stubbed_params.permit_was_called + def actual_permitted_params + double_collection.calls_to(:permit).inject([]) do |all_param_names, call| + all_param_names + call.args + end.flatten end - def parameters_difference - attributes - @stubbed_params.shoulda_permitted_params + def permit_called? + actual_permitted_params.any? end - def parameters_intersection - attributes & @stubbed_params.shoulda_permitted_params + def unpermitted_params + expected_permitted_params - actual_permitted_params end - def stub_params - matcher = self - - controller.singleton_class.class_eval do - alias_method :__shoulda_original_params__, :params - - define_method :params do - matcher.stubbed_params = StubbedParameters.new(__shoulda_original_params__) - end - end - end - - def unstub_params - controller.singleton_class.class_eval do - alias_method :params, :__shoulda_original_params__ - end + def verified_permitted_params + expected_permitted_params & actual_permitted_params end def ensure_action_and_verb_present! if action.blank? raise ActionNotDefinedError end + if verb.blank? raise VerbNotDefinedError end end - def verb_for_action - verb_lookup = { create: :post, update: :put } - verb_lookup[action] - end - - def attributes_as_sentence - attributes.map(&:inspect).to_sentence - end - - class StubbedParameters < SimpleDelegator - attr_reader :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) + def default_verb + case action + when :create then :post + when :update then RailsShim.verb_for_update end end - class NullStubbedParameters < ActiveSupport::HashWithIndifferentAccess - def permit_was_called; false; end - def shoulda_permitted_params; self; end - def require(*); self; end - def permit(*); self; end + def param_names_as_sentence + expected_permitted_params.map(&:inspect).to_sentence end class ActionNotDefinedError < StandardError @@ -157,8 +122,7 @@ module Shoulda class VerbNotDefinedError < StandardError def message - 'You must specify an HTTP verb when using a non-RESTful action.' + - ' e.g. for(:authorize, verb: :post)' + 'You must specify an HTTP verb when using a non-RESTful action. For example: for(:authorize, verb: :post)' end end end diff --git a/lib/shoulda/matchers/rails_shim.rb b/lib/shoulda/matchers/rails_shim.rb index 286bab17..cddca1fb 100644 --- a/lib/shoulda/matchers/rails_shim.rb +++ b/lib/shoulda/matchers/rails_shim.rb @@ -33,6 +33,14 @@ module Shoulda # :nodoc: end end + def self.verb_for_update + if action_pack_gte_4_1? + :patch + else + :put + end + end + def self.active_record_major_version ::ActiveRecord::VERSION::MAJOR end @@ -44,6 +52,14 @@ module Shoulda # :nodoc: def self.action_pack_major_version ::ActionPack::VERSION::MAJOR end + + def self.action_pack_gte_4_1? + Gem::Requirement.new('>= 4.1').satisfied_by?(action_pack_version) + end + + def self.action_pack_version + Gem::Version.new(::ActionPack::VERSION::STRING) + 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 2c7aa168..50feaa1f 100644 --- a/spec/shoulda/matchers/action_controller/strong_parameters_matcher_spec.rb +++ b/spec/shoulda/matchers/action_controller/strong_parameters_matcher_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Shoulda::Matchers::ActionController do - describe "#permit" do + describe '#permit' do it 'matches when the sent parameter is allowed' do controller_for_resource_with_strong_parameters(action: :create) do params.require(:user).permit(:name) @@ -56,8 +56,8 @@ describe Shoulda::Matchers::ActionController::StrongParametersMatcher do end end - describe "#matches?" do - it "is true for a subset of the allowable attributes" do + describe '#matches?' do + it 'is true for a subset of the allowable attributes' do controller_for_resource_with_strong_parameters(action: :create) do params.require(:user).permit(:name) end @@ -66,7 +66,7 @@ describe Shoulda::Matchers::ActionController::StrongParametersMatcher do expect(matcher.matches?(@controller)).to be_true end - it "is true for all the allowable attributes" do + it 'is true for all the allowable attributes' do controller_for_resource_with_strong_parameters(action: :create) do params.require(:user).permit(:name, :age) end @@ -75,7 +75,7 @@ describe Shoulda::Matchers::ActionController::StrongParametersMatcher do expect(matcher.matches?(@controller)).to be_true end - it "is false when any attributes are not allowed" do + it 'is false when any attributes are not allowed' do controller_for_resource_with_strong_parameters(action: :create) do params.require(:user).permit(:name) end @@ -84,67 +84,130 @@ describe Shoulda::Matchers::ActionController::StrongParametersMatcher do expect(matcher.matches?(@controller)).to be_false end - it "is false when permit is not called" do + it 'is false when permit is not called' do controller_for_resource_with_strong_parameters(action: :create) matcher = described_class.new([:name]).in_context(self).for(:create) expect(matcher.matches?(@controller)).to be_false end - it "requires an action" do + it 'requires an action' do controller_for_resource_with_strong_parameters matcher = described_class.new([:name]) - expect { matcher.matches?(@controller) } - .to raise_error(Shoulda::Matchers::ActionController::StrongParametersMatcher::ActionNotDefinedError) + expect { matcher.matches?(@controller) }. + to raise_error(described_class::ActionNotDefinedError) end - it "requires a verb for non-restful action" do + 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?(@controller) } - .to raise_error(Shoulda::Matchers::ActionController::StrongParametersMatcher::VerbNotDefinedError) + expect { matcher.matches?(@controller) }. + to raise_error(described_class::VerbNotDefinedError) + end + + it 'works with routes that require extra params' do + options = { + controller_name: 'Posts', + action: :show, + routes: -> { + get '/posts/:slug', to: 'posts#show' + } + } + controller_for_resource_with_strong_parameters(options) do + params.require(:user).permit(:name) + end + + matcher = described_class.new([:name]). + in_context(self). + for(:show, verb: :get, params: { slug: 'foo' }) + expect(matcher.matches?(@controller)).to be_true + end + + it 'works with #update specifically' do + controller_for_resource_with_strong_parameters(action: :update) do + params.require(:user).permit(:name) + end + + matcher = described_class.new([:name]). + in_context(self). + for(:update, params: { id: 1 }) + expect(matcher.matches?(@controller)).to be_true + end + + it 'does not raise an error when #fetch was used instead of #require (issue #495)' do + controller_for_resource_with_strong_parameters(action: :create) do + params.fetch(:order, {}).permit(:eta, :diner_id) + end + + matcher = described_class.new([:eta, :diner_id]). + in_context(self). + for(:create) + expect(matcher.matches?(@controller)).to be_true + end + + it 'tracks multiple calls to #permit' do + sets_of_attributes = [ + [:eta, :diner_id], + [:phone_number, :address_1, :address_2, :city, :state, :zip] + ] + controller_for_resource_with_strong_parameters(action: :create) do + params.require(:order).permit(sets_of_attributes[0]) + params.require(:diner).permit(sets_of_attributes[1]) + end + + matcher = described_class.new(sets_of_attributes[0]). + in_context(self). + for(:create) + expect(matcher.matches?(@controller)).to be_true + + matcher = described_class.new(sets_of_attributes[1]). + in_context(self). + for(:create) + expect(matcher.matches?(@controller)).to be_true end context 'stubbing params on the controller' do it 'still allows the original params to be set and accessed' do - actual_value = nil + actual_user_params = nil + actual_foo_param = nil controller_for_resource_with_strong_parameters(action: :create) do params[:foo] = 'bar' - actual_value = params[:foo] + actual_foo_param = params[:foo] + + actual_user_params = params[:user] params.require(:user).permit(:name) end - matcher = described_class.new([:name]).in_context(self).for(:create) + matcher = described_class.new([:name]). + in_context(self). + for(:create, params: { user: { some: 'params' } }) matcher.matches?(@controller) - expect(actual_value).to eq 'bar' + expect(actual_user_params).to eq('some' => 'params') + expect(actual_foo_param).to eq 'bar' end - it 'stubs the params while the controller action is being run' do - params_class = nil - + it 'stubs the params during the controller action' do controller_for_resource_with_strong_parameters(action: :create) do - params_class = params.class - params.require(:user).permit(:name) + params.require(:user) end matcher = described_class.new([:name]).in_context(self).for(:create) - matcher.matches?(@controller) - expect(params_class).to be described_class::StubbedParameters + expect { matcher.matches?(@controller) }.not_to raise_error end it 'does not permanently stub params' do - controller_for_resource_with_strong_parameters(action: :create) do - params.require(:user).permit(:name) - end + controller_for_resource_with_strong_parameters(action: :create) matcher = described_class.new([:name]).in_context(self).for(:create) matcher.matches?(@controller) - expect(@controller.params).to be_a(ActionController::Parameters) + expect { + @controller.params.require(:user) + }.to raise_error(::ActionController::ParameterMissing) end it 'prevents permanently stubbing params on error' do @@ -156,64 +219,79 @@ describe Shoulda::Matchers::ActionController::StrongParametersMatcher do rescue SimulatedError end - expect(@controller.params).to be_a(ActionController::Parameters) + expect { + @controller.params.require(:user) + }.to raise_error(::ActionController::ParameterMissing) end end end - describe "failure message" do - it "includes all missing attributes" do + describe 'failure message' do + it 'includes all missing attributes' do controller_for_resource_with_strong_parameters(action: :create) do params.require(:user).permit(:name, :age) end expect { 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.") + }.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 + it 'includes all attributes that should not have been allowed but were' do controller_for_resource_with_strong_parameters(action: :create) do params.require(:user).permit(:name, :age) end expect { expect(@controller).not_to permit(:name, :age).for(:create) - }.to fail_with_message("Expected controller not to permit name and age, but it did.") + }.to fail_with_message('Expected controller not to permit name and age, but it did.') end end - describe "#for" do - context "when given :create" do - it "posts to the controller" do + describe '#for' do + context 'when given :create' do + it 'POSTs to the controller' do controller = ActionController::Base.new - context = stub('context', post: nil) + context = mock() + context.expects(:post).with(:create, {}) matcher = described_class.new([:name]).in_context(context).for(:create) 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) + context 'when given :update' do + if rails_gte_41? + it 'PATCHes to the controller' do + controller = ActionController::Base.new + context = mock() + context.expects(:patch).with(:update, {}) + matcher = described_class.new([:name]).in_context(context).for(:update) - matcher.matches?(controller) - expect(context).to have_received(:put).with(:update) + matcher.matches?(controller) + end + else + it 'PUTs to the controller' do + controller = ActionController::Base.new + context = mock() + context.expects(:put).with(:update, {}) + matcher = described_class.new([:name]).in_context(context).for(:update) + + matcher.matches?(controller) + end end end - context "when given a custom action and verb" do - it "deletes to the controller" do + context 'when given a custom action and verb' do + it 'calls the action with the verb' do controller = ActionController::Base.new - context = stub('context', delete: nil) - matcher = described_class.new([:name]).in_context(context).for(:hide, verb: :delete) + context = mock() + context.expects(:delete).with(:hide, {}) + matcher = described_class.new([:name]). + in_context(context). + for(:hide, verb: :delete) matcher.matches?(controller) - expect(context).to have_received(:delete).with(:hide) end end end diff --git a/spec/support/controller_builder.rb b/spec/support/controller_builder.rb index b0ffbba1..11207c65 100644 --- a/spec/support/controller_builder.rb +++ b/spec/support/controller_builder.rb @@ -58,27 +58,30 @@ module ControllerBuilder $test_app.create_temp_view(path, contents) end - def controller_for_resource_with_strong_parameters(options = {}, &block) - controller_name = 'Users' - block ||= -> { {} } + def controller_for_resource_with_strong_parameters(options = {}, &action_body) + model_name = options.fetch(:model_name, 'User') + controller_name = options.fetch(:controller_name, 'UsersController') + collection_name = controller_name. + to_s.sub(/Controller$/, '').underscore. + to_sym + action_name = options.fetch(:action, :some_action) + routes ||= options.fetch(:routes, -> { resources collection_name }) + + define_model(model_name) + + controller_class = define_controller(controller_name) do + define_method action_name do + if action_body + instance_eval(&action_body) + end - define_model "User" - controller_class = define_controller controller_name do - define_method options.fetch(:action, 'some_action') do - @user = User.create(user_params) render nothing: true end - - private - define_method :user_params, &block end setup_rails_controller_test(controller_class) - collection_name = controller_name. - to_s.sub(/Controller$/, '').underscore. - to_sym - define_routes { resources(collection_name) } + define_routes(&routes) controller_class end diff --git a/spec/support/rails_versions.rb b/spec/support/rails_versions.rb index 459ef598..ac93c654 100644 --- a/spec/support/rails_versions.rb +++ b/spec/support/rails_versions.rb @@ -10,6 +10,10 @@ module RailsVersions def rails_4_x? Gem::Requirement.new('~> 4.0').satisfied_by?(rails_version) end + + def rails_gte_41? + Gem::Requirement.new('>= 4.1').satisfied_by?(rails_version) + end end RSpec.configure do |config|