Skip to content

Commit

Permalink
Fail have_many :through when inverse is invalid
Browse files Browse the repository at this point in the history
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/<USER>/.rvm/gems/ruby-2.2.0/gems/activerecord-4.2.0/lib/active_record/reflection.rb:871:in `derive_class_name'
         # /Users/<USER>/.rvm/gems/ruby-2.2.0/gems/activerecord-4.2.0/lib/active_record/reflection.rb:147:in `class_name'
         # /Users/<USER>/.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/<USER>/.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/<USER>/.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 <top (required)>'

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 <elliot.winkler@gmail.com>
  • Loading branch information
maurogeorge and mcmire committed Jul 11, 2020
1 parent f50f70e commit c0a1578
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 8 deletions.
18 changes: 17 additions & 1 deletion lib/shoulda/matchers/active_record/association_matcher.rb
Expand Up @@ -1144,6 +1144,7 @@ def matches?(subject)
@subject = subject
association_exists? &&
macro_correct? &&
validate_inverse_of_through_association &&
(polymorphic? || class_exists?) &&
foreign_key_exists? &&
primary_key_exists? &&
Expand Down Expand Up @@ -1198,7 +1199,14 @@ def macro_description
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
Expand Down Expand Up @@ -1241,6 +1249,14 @@ def macro_correct?
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?)
Expand Down
Expand Up @@ -65,16 +65,22 @@ def association_foreign_key
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
Expand Down
Expand Up @@ -4,9 +4,23 @@ module ActiveRecord
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
Expand Down
Expand Up @@ -847,6 +847,15 @@ def belonging_to_non_existent_class(model_name, assoc_name, options = {})
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,
Expand All @@ -860,6 +869,21 @@ def belonging_to_non_existent_class(model_name, assoc_name, options = {})
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 => <name>'. 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
Expand Down

0 comments on commit c0a1578

Please sign in to comment.