From c0a1578435f66d6fbf0db1164205bd8d99f6aa2f Mon Sep 17 00:00:00 2001 From: Mauro George Date: Thu, 7 May 2015 20:01:57 -0300 Subject: [PATCH] Fail have_many :through when inverse is invalid When used with a `:through` association where a corresponding `belongs_to` association has not been set up on the inverse model, the `have_many` matcher raises this error: Failures: 1) User should have many projects through clients Failure/Error: it { should have_many(:projects).through :clients } NoMethodError: undefined method `class_name' for nil:NilClass # /Users//.rvm/gems/ruby-2.2.0/gems/activerecord-4.2.0/lib/active_record/reflection.rb:871:in `derive_class_name' # /Users//.rvm/gems/ruby-2.2.0/gems/activerecord-4.2.0/lib/active_record/reflection.rb:147:in `class_name' # /Users//.rvm/gems/ruby-2.2.0/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_record/association_matcher.rb:1067:in `rescue in class_exists?' # /Users//.rvm/gems/ruby-2.2.0/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_record/association_matcher.rb:1064:in `class_exists?' # /Users//.rvm/gems/ruby-2.2.0/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_record/association_matcher.rb:928:in `matches?' # ./spec/models/user_spec.rb:5:in `block (2 levels) in ' Fortunately, ActiveRecord has a `check_validity!` method we can call on an association. For `:through` associations, this will run through a litany of checks, one of which is to check for the inverse association, which we want in this case. We rescue the error that is produced and include this in the failure message. Co-authored-by: Elliot Winkler --- .../active_record/association_matcher.rb | 18 +++++++++++++- .../association_matchers/model_reflection.rb | 14 +++++++---- .../association_matchers/model_reflector.rb | 20 +++++++++++++--- .../active_record/association_matcher_spec.rb | 24 +++++++++++++++++++ 4 files changed, 68 insertions(+), 8 deletions(-) diff --git a/lib/shoulda/matchers/active_record/association_matcher.rb b/lib/shoulda/matchers/active_record/association_matcher.rb index 5b122dec..a4fd6a07 100644 --- a/lib/shoulda/matchers/active_record/association_matcher.rb +++ b/lib/shoulda/matchers/active_record/association_matcher.rb @@ -1144,6 +1144,7 @@ module Shoulda @subject = subject association_exists? && macro_correct? && + validate_inverse_of_through_association && (polymorphic? || class_exists?) && foreign_key_exists? && primary_key_exists? && @@ -1198,7 +1199,14 @@ module Shoulda end def expectation - "#{model_class.name} to have a #{macro} association called #{name}" + expectation = + "#{model_class.name} to have a #{macro} association called #{name}" + + if through? + expectation << " through #{reflector.has_and_belongs_to_many_name}" + end + + expectation end def missing_options @@ -1241,6 +1249,14 @@ module Shoulda end end + def validate_inverse_of_through_association + reflector.validate_inverse_of_through_association! + true + rescue ::ActiveRecord::ActiveRecordError => error + @missing = error.message + false + end + def macro_supports_primary_key? macro == :belongs_to || ([:has_many, :has_one].include?(macro) && !through?) diff --git a/lib/shoulda/matchers/active_record/association_matchers/model_reflection.rb b/lib/shoulda/matchers/active_record/association_matchers/model_reflection.rb index 882808f4..7a27ccae 100644 --- a/lib/shoulda/matchers/active_record/association_matchers/model_reflection.rb +++ b/lib/shoulda/matchers/active_record/association_matchers/model_reflection.rb @@ -65,16 +65,22 @@ module Shoulda end end + def validate_inverse_of_through_association! + if through? + reflection.check_validity! + end + end + + def has_and_belongs_to_many_name + reflection.options[:through] + end + protected attr_reader :reflection, :subject private - def has_and_belongs_to_many_name - reflection.options[:through] - end - def has_and_belongs_to_many_name_table_name if has_and_belongs_to_many_reflection has_and_belongs_to_many_reflection.table_name diff --git a/lib/shoulda/matchers/active_record/association_matchers/model_reflector.rb b/lib/shoulda/matchers/active_record/association_matchers/model_reflector.rb index 0d0503c9..8927bc32 100644 --- a/lib/shoulda/matchers/active_record/association_matchers/model_reflector.rb +++ b/lib/shoulda/matchers/active_record/association_matchers/model_reflector.rb @@ -4,9 +4,23 @@ module Shoulda module AssociationMatchers # @private class ModelReflector - delegate :associated_class, :through?, :join_table_name, - :association_relation, :polymorphic?, :foreign_key, - :association_foreign_key, to: :reflection + delegate( + :associated_class, + :association_foreign_key, + :association_relation, + :foreign_key, + :has_and_belongs_to_many_name, + :join_table_name, + :polymorphic?, + :validate_inverse_of_through_association!, + to: :reflection + ) + + delegate( + :through?, + to: :reflection, + allow_nil: true + ) def initialize(subject, name) @subject = subject 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 29f5a20a..eb32d265 100644 --- a/spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb +++ b/spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb @@ -847,6 +847,15 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do expect(having_many_children).to have_many(:children) end + it 'does not reject a non-:through association where there is no belongs_to in the inverse model' do + define_model :Child, parent_id: :integer + parent_class = define_model :Parent do + has_many :children + end + + expect { have_many(:children) }.to match_against(parent_class.new) + end + it 'accepts a valid association with a :through option' do define_model :child define_model :conception, child_id: :integer, @@ -860,6 +869,21 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do expect(Parent.new).to have_many(:children) end + it 'rejects a :through association where there is no belongs_to in the inverse model' do + define_model :Child + define_model :Conception, child_id: :integer, parent_id: :integer + parent_class = define_model :Parent do + has_many :conceptions + has_many :children, through: :conceptions + end + + expect { have_many(:children) } + .not_to match_against(parent_class.new) + .and_fail_with(<<-MESSAGE) +Expected Parent to have a has_many association called children through conceptions (Could not find the source association(s) "child" or :children in model Conception. Try 'has_many :children, :through => :conceptions, :source => '. Is it one of ?) + MESSAGE + end + it 'accepts a valid association with an :as option' do define_model :child, guardian_type: :string, guardian_id: :integer define_model :parent do