Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for foreign_type #1425

Closed
wants to merge 3 commits into from
+169 −3
Closed

Add support for foreign_type #1425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ed00c46
Add support for foreign_type
mswiszcz Mar 5, 2021
7484513
Rubocop fix
mswiszcz Mar 5, 2021
1cf931c
Fix documentation
mswiszcz Mar 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 117 additions & 0 deletions lib/shoulda/matchers/active_record/association_matcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,28 @@ module ActiveRecord
# with_foreign_key('country_id')
# end
#
# ##### with_foreign_type
#
# Use `with_foreign_type` to test usage of the `:foreign_type` option.
#
# class Visitor < ActiveRecord::Base
# belongs_to :location, foreign_type: 'facility_type', polymorphic: true
# end
#
# # RSpec
# RSpec.describe Visitor, type: :model do
# it do
# should belong_to(:location).
# with_foreign_type('facility_type')
# end
# end
#
# # Minitest (Shoulda)
# class VisitorTest < ActiveSupport::TestCase
# should belong_to(:location).
# with_foreign_type('facility_type')
# end
#
# ##### dependent
#
# Use `dependent` to assert that the `:dependent` option was specified.
Expand Down Expand Up @@ -455,6 +477,24 @@ def belong_to(name)
# should have_many(:worries).with_foreign_key('worrier_id')
# end
#
# ##### with_foreign_type
#
# Use `with_foreign_type` to test usage of the `:foreign_type` option.
#
# class Hotel < ActiveRecord::Base
# has_many :visitors, foreign_key: 'facility_type', as: :location
# end
#
# # RSpec
# RSpec.describe Hotel, type: :model do
# it { should have_many(:visitors).with_foreign_type('facility_type') }
# end
#
# # Minitest (Shoulda)
# class HotelTest < ActiveSupport::TestCase
# should have_many(:visitors).with_foreign_type('facility_type')
# end
#
# ##### dependent
#
# Use `dependent` to assert that the `:dependent` option was specified.
Expand Down Expand Up @@ -726,6 +766,24 @@ def have_many(name)
# should have_one(:job).with_foreign_key('worker_id')
# end
#
# ##### with_foreign_type
#
# Use `with_foreign_type` to test usage of the `:foreign_type` option.
#
# class Hotel < ActiveRecord::Base
# has_one :special_guest, foreign_type: 'facility_type', as: :location
# end
#
# # RSpec
# RSpec.describe Hotel, type: :model do
# it { should have_one(:special_guest).with_foreign_type('facility_type') }
# end
#
# # Minitest (Shoulda)
# class HotelTest < ActiveSupport::TestCase
# should have_one(:special_guest).with_foreign_type('facility_type')
# end
#
# ##### through
#
# Use `through` to test usage of the `:through` option. This asserts that
Expand Down Expand Up @@ -1088,6 +1146,11 @@ def with_foreign_key(foreign_key)
self
end

def with_foreign_type(foreign_type)
@options[:foreign_type] = foreign_type
self
end

def with_primary_key(primary_key)
@options[:primary_key] = primary_key
self
Expand Down Expand Up @@ -1155,6 +1218,7 @@ def matches?(subject)
macro_correct? &&
validate_inverse_of_through_association &&
(polymorphic? || class_exists?) &&
foreign_type_matches? &&
foreign_key_exists? &&
primary_key_exists? &&
class_name_correct? &&
Expand Down Expand Up @@ -1267,6 +1331,12 @@ def foreign_key_exists?
!(belongs_foreign_key_missing? || has_foreign_key_missing?)
end

def foreign_type_matches?
return true unless options.key?(:foreign_type)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't typically return early or use the modifier form of unless. Since the end result is a boolean, what do you think about combining these into one conditional?

!options.key?(:foreign_type) || (
  !belongs_foreign_type_missing? &&
  !has_foreign_type_missing?
)


!(belongs_foreign_type_missing? || has_foreign_type_missing?)
end

def primary_key_exists?
!macro_supports_primary_key? || primary_key_correct?(model_class)
end
Expand All @@ -1275,12 +1345,22 @@ def belongs_foreign_key_missing?
macro == :belongs_to && !class_has_foreign_key?(model_class)
end

def belongs_foreign_type_missing?
macro == :belongs_to && !class_has_foreign_type?(model_class)
end

def has_foreign_key_missing?
[:has_many, :has_one].include?(macro) &&
!through? &&
!class_has_foreign_key?(associated_class)
end

def has_foreign_type_missing?
[:has_many, :has_one].include?(macro) &&
!through? &&
!class_has_foreign_type?(associated_class)
end

def class_name_correct?
if options.key?(:class_name)
if option_verifier.correct_for_constant?(
Expand Down Expand Up @@ -1405,6 +1485,22 @@ def class_has_foreign_key?(klass)
end
end

def class_has_foreign_type?(klass)
if options.key?(:foreign_type) && !foreign_type_correct?
@missing = foreign_type_failure_message(
klass,
options[:foreign_type],
)

false
elsif !has_column?(klass, foreign_type)
@missing = foreign_type_failure_message(klass, foreign_type)
false
else
true
end
end

def has_column?(klass, column)
case column
when Array
Expand All @@ -1421,10 +1517,21 @@ def foreign_key_correct?
)
end

def foreign_type_correct?
option_verifier.correct_for_string?(
:foreign_type,
options[:foreign_type],
)
end

def foreign_key_failure_message(klass, foreign_key)
"#{klass} does not have a #{foreign_key} foreign key."
end

def foreign_type_failure_message(klass, foreign_type)
"#{klass} does not have a #{foreign_type} foreign type."
end

def primary_key_correct?(klass)
if options.key?(:primary_key)
if option_verifier.correct_for_string?(
Expand Down Expand Up @@ -1472,6 +1579,16 @@ def foreign_key_reflection
end
end

def foreign_type
type = if [:has_one, :has_many].include?(macro)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To simplify this expression and remove the temporary assignment, what do you think about moving the .to_s to has_column? at line 1509?

reflection.type
else
reflection.foreign_type
end

type.to_s
end

def submatchers_match?
failing_submatchers.empty?
end
Expand Down
1 change: 1 addition & 0 deletions lib/shoulda/matchers/active_record/association_matchers/model_reflector.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ class ModelReflector
:associated_class,
:association_foreign_key,
:foreign_key,
:foreign_type,
:has_and_belongs_to_many_name,
:join_table_name,
:polymorphic?,
Expand Down
54 changes: 51 additions & 3 deletions spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,17 @@
expect(Child.new).to belong_to(:parent)
end

it 'accepts an association with an existing custom foreign key and type' do
define_model :parent
define_model :child, ancestor_id: :integer, ancestor_type: :string do
belongs_to :parent, polymorphic: true, foreign_key: 'ancestor_id', foreign_type: 'ancestor_type'
end

expect(Child.new).to belong_to(:parent).
with_foreign_key(:ancestor_id).
with_foreign_type(:ancestor_type)
end

it 'accepts an association using an existing custom primary key' do
define_model :parent
define_model :child, parent_id: :integer, custom_primary_key: :integer do
Expand Down Expand Up @@ -1226,6 +1237,20 @@ def belonging_to_non_existent_class(model_name, assoc_name, options = {})
expect(Parent.new).to have_many(:children)
end

it 'accepts an association with a nonstandard reverse foreign type, using :inverse_of' do
define_model :visitor, location_id: :integer, facility_type: :string do
belongs_to :location, foreign_type: :facility_type,
inverse_of: :visitors,
polymorphic: true
end

define_model :hotel do
has_many :visitors, inverse_of: :location, foreign_type: :facility_type, as: :location
end

expect(Hotel.new).to have_many(:visitors).with_foreign_type(:facility_type)
end

it 'rejects an association with a nonstandard reverse foreign key, if :inverse_of is not correct' do
define_model :child, mother_id: :integer do
belongs_to :mother, inverse_of: :children, class_name: :Parent
Expand All @@ -1239,16 +1264,24 @@ def belonging_to_non_existent_class(model_name, assoc_name, options = {})
end

it 'accepts an association with a nonstandard foreign key, with reverse association turned off' do
define_model :child, ancestor_id: :integer do
end

define_model :child, ancestor_id: :integer
define_model :parent do
has_many :children, foreign_key: :ancestor_id, inverse_of: false
end

expect(Parent.new).to have_many(:children)
end

it 'accepts an association with a nonstandard type, with reverse association turned off' do
define_model :visitor, location_id: :integer, facitility_type: :string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be facility_type?


define_model :hotel do
has_many :visitors, foreign_type: :facitility_type, inverse_of: false, as: :location
end

expect(Hotel.new).to have_many(:visitors).with_foreign_type(:facitility_type)
end

def having_many_children(options = {})
define_model :child, parent_id: :integer
define_model(:parent).tap do |model|
Expand Down Expand Up @@ -1300,6 +1333,21 @@ def having_many_non_existent_class(model_name, assoc_name, options = {})
expect(Person.new).to have_one(:detail).with_foreign_key(:detailed_person_id)
end

it 'accepts an association with an existing custom foreign type' do
define_model :profile, user_id: :integer, related_user_type: :string

define_model :admin do
has_one :profile, foreign_type: :related_user_type, as: :user
end

define_model :moderator do
has_one :profile, foreign_type: :related_user_type, as: :user
end

expect(Admin.new).to have_one(:profile).with_foreign_type(:related_user_type)
expect(Moderator.new).to have_one(:profile).with_foreign_type(:related_user_type)
end

it 'accepts an association using an existing custom primary key' do
define_model :detail, person_id: :integer
define_model :person, custom_primary_key: :integer do
Expand Down