From a465e796e88c8e73be67b9b739473e67707674ef Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Wed, 12 Sep 2018 18:54:29 -0600 Subject: [PATCH] Fix default behavior of belong_to under Rails 4.2 Rails 5 introduced a change where `belongs_to` would default to adding a presence validation along with the association. However, it also introduced a configuration option, `belongs_to_required_by_default`, to emulate the old behavior prior to Rails 5. For Rails 4.2 projects as well as Rails 5 which were migrated from 4, this setting is false, so that existing apps do not break. To mimic this, a change was made to the `belong_to` matcher to check for the presence of the presence validator if `belongs_to_required_by_default` is true and check for the absence of the presence validator if it is false. However, this last bit of the logic actually causes problems. Take this case, for example: ActiveRecord::Base.belongs_to_required_by_default = false class Post < ActiveRecord::Base belongs_to :user validates :user, presence: true end RSpec.describe Post, type: :model do it { is_expected.to belong_to(:user) } end In this example, the developer has chosen to place a presence validation on the association manually. `belong_to` doesn't know this, however, and will check to make sure that `user` can be nil, which of course it can't. Therefore, this test will fail. In addition, the failure message that `belong_to` generates is confusing: Expected Post to have a belongs_to association called user (the association should have been defined with `optional: true`, but was not) The reason why the test fails is that when `belongs_to_required_by_default` is false, belong_to` will place an implicit `optional` qualifier on itself. In other words, these two tests are equivalent: it { is_expected.to belong_to(:user) } it { is_expected.to belong_to(:user).optional } However, this is not only wrong, but the `belongs_to` macro in Rails 4.2 doesn't have an `optional` option (it has `required` instead), so the failure message that `belong_to` generates is confusing. This commit fixes this by modifying `belong_to` so that under Rails 4.2, the matcher will have not have any qualifiers on it by default. --- .../active_record/association_matcher.rb | 8 +- .../unit/helpers/active_record_versions.rb | 2 +- .../active_record/association_matcher_spec.rb | 155 ++++++++++++++---- 3 files changed, 124 insertions(+), 41 deletions(-) diff --git a/lib/shoulda/matchers/active_record/association_matcher.rb b/lib/shoulda/matchers/active_record/association_matcher.rb index 527dc57d..f046a97b 100644 --- a/lib/shoulda/matchers/active_record/association_matcher.rb +++ b/lib/shoulda/matchers/active_record/association_matcher.rb @@ -968,12 +968,8 @@ module Shoulda @submatchers = [] @missing = '' - if macro == :belongs_to - if RailsShim.active_record_gte_5? && belongs_to_required_by_default? - required - else - optional - end + if macro == :belongs_to && RailsShim.active_record_gte_5? + required(belongs_to_required_by_default?) end end diff --git a/spec/support/unit/helpers/active_record_versions.rb b/spec/support/unit/helpers/active_record_versions.rb index 313c7b99..44eee019 100644 --- a/spec/support/unit/helpers/active_record_versions.rb +++ b/spec/support/unit/helpers/active_record_versions.rb @@ -37,7 +37,7 @@ module UnitTests active_record_version < 5 end - def active_record_supports_required_for_associations? + def active_record_supports_optional_for_associations? active_record_version >= 5 end end diff --git a/spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb b/spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb index 361e011c..a5e52a51 100644 --- a/spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb +++ b/spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb @@ -281,7 +281,7 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do context 'given the association is neither configured to be required nor optional' do context 'when qualified with required(true)' do - if active_record_supports_required_for_associations? + if active_record_supports_optional_for_associations? context 'when belongs_to is configured to be required by default' do it 'passes' do configuring_default_belongs_to_requiredness(true) do @@ -311,13 +311,14 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do else it 'fails with an appropriate message' do assertion = lambda do - expect(belonging_to_parent).to belong_to(:parent).required(true) + expect(belonging_to_parent). + to belong_to(:parent).required(true) end message = format_message(<<-MESSAGE, one_line: true) - Expected Child to have a belongs_to association called parent (the - association should have been defined with `required: true`, but - was not) + Expected Child to have a belongs_to association called parent + (the association should have been defined with `required: + true`, but was not) MESSAGE expect(&assertion).to fail_with_message(message) @@ -326,7 +327,7 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do end context 'when qualified with required(false)' do - if active_record_supports_required_for_associations? + if active_record_supports_optional_for_associations? context 'when belongs_to is configured to be required by default' do it 'fails with an appropriate message' do configuring_default_belongs_to_requiredness(true) do @@ -361,7 +362,7 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do end context 'when qualified with optional' do - if active_record_supports_required_for_associations? + if active_record_supports_optional_for_associations? context 'when belongs_to is configured to be required by default' do it 'fails with an appropriate message' do configuring_default_belongs_to_requiredness(true) do @@ -395,8 +396,8 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do end end - if active_record_supports_required_for_associations? - context 'when qualified with nothing' do + context 'when qualified with nothing' do + if active_record_supports_optional_for_associations? context 'when belongs_to is configured to be required by default' do it 'passes' do configuring_default_belongs_to_requiredness(true) do @@ -411,6 +412,92 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do expect(belonging_to_parent).to belong_to(:parent) end end + + context 'and a presence validation is on the attribute instead of using required: true' do + it 'passes' do + configuring_default_belongs_to_requiredness(false) do + record = belonging_to_parent do + validates_presence_of :parent + end + + expect(record).to belong_to(:parent) + end + end + end + + context 'and a presence validation is on the attribute with a condition' do + context 'and the condition is true' do + it 'passes' do + configuring_default_belongs_to_requiredness(false) do + child_model = create_child_model_belonging_to_parent do + attr_accessor :condition + validates_presence_of :parent, if: :condition + end + + record = child_model.new(condition: true) + + expect(record).to belong_to(:parent) + end + end + end + + context 'and the condition is false' do + it 'passes' do + configuring_default_belongs_to_requiredness(false) do + child_model = create_child_model_belonging_to_parent do + attr_accessor :condition + validates_presence_of :parent, if: :condition + end + + record = child_model.new(condition: false) + + expect(record).to belong_to(:parent) + end + end + end + end + end + else + it 'passes' do + expect(belonging_to_parent).to belong_to(:parent) + end + + context 'and a presence validation is on the attribute instead of using required: true' do + it 'passes' do + record = belonging_to_parent do + validates_presence_of :parent + end + + expect(record).to belong_to(:parent) + end + end + + context 'and a presence validation is on the attribute with a condition' do + context 'and the condition is true' do + it 'passes' do + child_model = create_child_model_belonging_to_parent do + attr_accessor :condition + validates_presence_of :parent, if: :condition + end + + record = child_model.new(condition: true) + + expect(record).to belong_to(:parent) + end + end + + context 'and the condition is false' do + it 'passes' do + child_model = create_child_model_belonging_to_parent do + attr_accessor :condition + validates_presence_of :parent, if: :condition + end + + record = child_model.new(condition: false) + + expect(record).to belong_to(:parent) + end + end end end end @@ -459,31 +546,13 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do end context 'when qualified with nothing' do - if active_record_supports_required_for_associations? - it 'passes' do - expect(belonging_to_parent(required: true)). - to belong_to(:parent) - end - else - it 'fails with an appropriate message' do - assertion = lambda do - expect(belonging_to_parent(required: true)). - to belong_to(:parent) - end - - message = format_message(<<-MESSAGE, one_line: true) - Expected Child to have a belongs_to association called parent (the - association should have been defined with `optional: true`, but - was not) - MESSAGE - - expect(&assertion).to fail_with_message(message) - end + it 'passes' do + expect(belonging_to_parent(required: true)).to belong_to(:parent) end end end - if active_record_supports_required_for_associations? + if active_record_supports_optional_for_associations? context 'given the association is configured as optional: true' do context 'when qualified with required(true)' do it 'fails with an appropriate message' do @@ -535,11 +604,29 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do end end - def belonging_to_parent(options = {}, parent_options = {}) - define_model :parent, parent_options + def belonging_to_parent(options = {}, parent_options = {}, &block) + child_model = create_child_model_belonging_to_parent( + options, + parent_options, + &block + ) + child_model.new + end + + def create_child_model_belonging_to_parent( + options = {}, + parent_options = {}, + &block + ) + define_model(:parent, parent_options) + define_model :child, parent_id: :integer do belongs_to :parent, options - end.new + + if block + class_eval(&block) + end + end end def belonging_to_with_inverse(association, inverse_association) @@ -1128,7 +1215,7 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do end end - if active_record_supports_required_for_associations? + if active_record_supports_optional_for_associations? context 'given an association with a matching :required option' do it 'passes' do expect(having_one_detail(required: true)).