Skip to content

Commit 9a8158a

Browse files
committed
missing_presence_validation: recognize explicit foreign key validators
This commit allows a foreign key column to be explicitly validated via a standalone validator. Without this change a false positive errors would be reported. Additionally, only `belongs_to` associations are considered for marking as optional, as `has_*` don't support the `optional` nor `required` options.
1 parent 8d0edf9 commit 9a8158a

File tree

2 files changed

+45
-6
lines changed

2 files changed

+45
-6
lines changed

lib/active_record_doctor/detectors/missing_presence_validation.rb

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -76,28 +76,30 @@ def detect
7676
# that are validated directly or via an association name.
7777
model.validators.each do |validator|
7878
problematic_columns.reject! do |column|
79-
# Translate a foreign key or type to the association name.
80-
attribute = column_name_to_association_name[column.name] || column.name.to_sym
79+
attribute_names = [
80+
column.name.to_sym,
81+
column_name_to_association_name[column.name]
82+
].compact
8183

8284
case validator
8385

8486
# A regular presence validator is enough if the column name is
8587
# listed among the attributes it's validating.
8688
when ActiveRecord::Validations::PresenceValidator
87-
validator.attributes.include?(attribute)
89+
(validator.attributes & attribute_names).present?
8890

8991
# An inclusion validator ensures the column is not nil if it covers
9092
# the column and nil is NOT one of the values it allows.
9193
when ActiveModel::Validations::InclusionValidator
9294
validator_items = inclusion_or_exclusion_validator_items(validator)
93-
validator.attributes.include?(attribute) &&
95+
(validator.attributes & attribute_names).present? &&
9496
(validator_items.is_a?(Proc) || validator_items.exclude?(nil))
9597

9698
# An exclusion validator ensures the column is not nil if it covers
9799
# the column and excludes nil as an allowed value explicitly.
98100
when ActiveModel::Validations::ExclusionValidator
99101
validator_items = inclusion_or_exclusion_validator_items(validator)
100-
validator.attributes.include?(attribute) &&
102+
(validator.attributes & attribute_names).present? &&
101103
(validator_items.is_a?(Proc) || validator_items.include?(nil))
102104

103105
end
@@ -109,7 +111,7 @@ def detect
109111
problematic_associations = []
110112
problematic_polymorphic_associations = []
111113

112-
model.reflect_on_all_associations.each do |reflection|
114+
model.reflect_on_all_associations(:belongs_to).each do |reflection|
113115
foreign_key_column = problematic_columns.find { |column| column.name == reflection.foreign_key }
114116
if reflection.polymorphic?
115117
# If the foreign key and type are not one of the columns that lack

test/active_record_doctor/detectors/missing_presence_validation_test.rb

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,43 @@ def test_abstract_class
261261
refute_problems
262262
end
263263

264+
def test_both_foreign_key_column_and_association_validators_are_checked
265+
Context.create_table(:registrations) do |t|
266+
t.string :email, null: false
267+
end.define_model do
268+
# email is NOT NULL, but its presence is validated via the following
269+
# required association, no complaints about the column should be set.
270+
belongs_to :user, primary_key: :email, foreign_key: :email, inverse_of: :registrations, optional: false
271+
end
272+
273+
Context.create_table(:users) do |t|
274+
t.string :email, null: false
275+
end.define_model do
276+
has_many :registrations, primary_key: :email, foreign_key: :email, inverse_of: :user, dependent: nil
277+
validates :email, presence: true
278+
end
279+
280+
refute_problems
281+
end
282+
283+
def test_explicit_foreign_key_validator_allows_optional_belongs_to
284+
Context.create_table(:registrations) do |t|
285+
t.string :email, null: false
286+
end.define_model do
287+
# The association below is optional, but email has an explicit presence
288+
# validator so no errors should be reported.
289+
belongs_to :user, primary_key: :email, foreign_key: :email, inverse_of: :registrations, optional: true
290+
validates :email, presence: true
291+
end
292+
293+
Context.create_table(:users) do
294+
end.define_model do
295+
has_many :registrations, primary_key: :email, foreign_key: :email, inverse_of: :user, dependent: nil
296+
end
297+
298+
refute_problems
299+
end
300+
264301
def test_config_ignore_models
265302
Context.create_table(:users) do |t|
266303
t.string :name, null: false

0 commit comments

Comments
 (0)