Skip to content

Commit 998780b

Browse files
gregnavisgregnavis
authored
Rewrite missing_presence_validation to handle polymorphic associations (#230)
This commits rewrites missing_presence_validation in a more imperative style with extensive commenting to make it clear why certain actions are performed. The new structure made it simple to correctly handle polymorphic associations: if a polymorphic foreign key is NOT NULL then the detector will suggest making the association as required, instead of suggesting adding validators for the _id and _type column separately.
1 parent f226d2c commit 998780b

File tree

3 files changed

+178
-56
lines changed

3 files changed

+178
-56
lines changed

‎lib/active_record_doctor/detectors/missing_presence_validation.rb‎

Lines changed: 135 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -21,78 +21,157 @@ class MissingPresenceValidation < Base # :nodoc:
2121

2222
private
2323

24-
def message(column:, model:)
25-
"add a `presence` validator to #{model}.#{column} - it's NOT NULL but lacks a validator"
24+
def message(type:, column_or_association:, model:)
25+
case type
26+
when :missing_validator
27+
"add a `presence` validator to #{model}.#{column_or_association} - it's NOT NULL but lacks a validator"
28+
when :optional_association
29+
"add `optional: false` to #{model}.#{column_or_association} - the foreign key #{column_or_association}_id is NOT NULL" # rubocop:disable Layout/LineLength
30+
when :optional_polymorphic_association
31+
"add `optional: false` to #{model}.#{column_or_association} - the foreign key #{column_or_association}_id or type #{column_or_association}_type are NOT NULL" # rubocop:disable Layout/LineLength
32+
end
2633
end
2734

2835
def detect
2936
each_model(except: config(:ignore_models), existing_tables_only: true) do |model|
30-
each_attribute(model, except: config(:ignore_attributes)) do |column|
31-
next unless validator_needed?(model, column)
32-
next if validator_present?(model, column)
33-
next if counter_cache_column?(model, column)
34-
35-
problem!(column: column.name, model: model.name)
37+
# List all columns and then remove those that don't need or don't have
38+
# a missing validator.
39+
problematic_columns = connection.columns(model.table_name)
40+
problematic_columns.reject! do |column|
41+
# The primary key, timestamps, and counter caches are special
42+
# columns that are automatically managed by Rails and don't need
43+
# an explicit presence validator.
44+
column.name == model.primary_key ||
45+
["created_at", "updated_at", "created_on", "updated_on"].include?(column.name) ||
46+
counter_cache_column?(model, column) ||
47+
48+
# NULL-able columns don't need a presence validator as they can be
49+
# set to NULL after all. A check constraint (column IS NOT NULL)
50+
# is an alternative approach and the absence of such constraint is
51+
# tested below.
52+
(column.null && !not_null_check_constraint_exists?(model.table_name, column)) ||
53+
54+
# If requested, columns with a default value don't need presence
55+
# validation as they'd have the default value substituted automatically.
56+
(config(:ignore_columns_with_default) && (column.default || column.default_function)) ||
57+
58+
# Explicitly ignored columns should be skipped.
59+
config(:ignore_attributes).include?("#{model.name}.#{column.name}")
3660
end
37-
end
38-
end
3961

40-
def validator_needed?(model, column)
41-
![model.primary_key, "created_at", "updated_at", "created_on", "updated_on"].include?(column.name) &&
42-
(!column.null || not_null_check_constraint_exists?(model.table_name, column)) &&
43-
!default_value_instead_of_validation?(column)
44-
end
45-
46-
def default_value_instead_of_validation?(column)
47-
(!column.default.nil? || column.default_function) && config(:ignore_columns_with_default)
48-
end
49-
50-
def validator_present?(model, column)
51-
inclusion_validator_present?(model, column) ||
52-
exclusion_validator_present?(model, column) ||
53-
presence_validator_present?(model, column)
54-
end
55-
56-
def inclusion_validator_present?(model, column)
57-
model.validators.any? do |validator|
58-
validator_items = inclusion_validator_items(validator)
59-
return true if validator_items.is_a?(Proc)
60-
61-
validator.is_a?(ActiveModel::Validations::InclusionValidator) &&
62-
validator.attributes.map(&:to_s).include?(column.name) &&
63-
!validator_items.include?(nil)
64-
end
65-
end
66-
67-
def exclusion_validator_present?(model, column)
68-
model.validators.any? do |validator|
69-
validator_items = inclusion_validator_items(validator)
70-
return true if validator_items.is_a?(Proc)
62+
# At this point the only columns that are left are those that DO
63+
# need presence validation in the model. Let's iterate over all
64+
# validators to see which columns are actually validated, but before
65+
# we do that let's define a map for quickly translating foreign key
66+
# names to belongs_to association names.
67+
column_name_to_association_name = {}
68+
model.reflect_on_all_associations(:belongs_to).each do |reflection|
69+
column_name_to_association_name[reflection.foreign_key] = reflection.name
70+
if reflection.polymorphic?
71+
column_name_to_association_name[reflection.foreign_type] = reflection.name
72+
end
73+
end
7174

72-
validator.is_a?(ActiveModel::Validations::ExclusionValidator) &&
73-
validator.attributes.include?(column.name.to_sym) &&
74-
validator_items.include?(nil)
75-
end
76-
end
75+
# We're now ready to iterate over the validators and remove columns
76+
# that are validated directly or via an association name.
77+
model.validators.each do |validator|
78+
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
81+
82+
case validator
83+
84+
# A regular presence validator is enough if the column name is
85+
# listed among the attributes it's validating.
86+
when ActiveRecord::Validations::PresenceValidator
87+
validator.attributes.include?(attribute)
88+
89+
# An inclusion validator ensures the column is not nil if it covers
90+
# the column and nil is NOT one of the values it allows.
91+
when ActiveModel::Validations::InclusionValidator
92+
validator_items = inclusion_or_exclusion_validator_items(validator)
93+
validator.attributes.include?(attribute) &&
94+
(validator_items.is_a?(Proc) || validator_items.exclude?(nil))
95+
96+
# An exclusion validator ensures the column is not nil if it covers
97+
# the column and excludes nil as an allowed value explicitly.
98+
when ActiveModel::Validations::ExclusionValidator
99+
validator_items = inclusion_or_exclusion_validator_items(validator)
100+
validator.attributes.include?(attribute) &&
101+
(validator_items.is_a?(Proc) || validator_items.include?(nil))
102+
103+
end
104+
end
105+
end
77106

78-
def presence_validator_present?(model, column)
79-
allowed_attributes = [column.name.to_sym]
107+
# Associations need to be checked whether they're marked optional
108+
# while the underlying foreign key or type columns are marked NOT NULL.
109+
problematic_associations = []
110+
problematic_polymorphic_associations = []
111+
112+
model.reflect_on_all_associations.each do |reflection|
113+
foreign_key_column = problematic_columns.find { |column| column.name == reflection.foreign_key }
114+
if reflection.polymorphic?
115+
# If the foreign key and type are not one of the columns that lack
116+
# a validator then it means the association added a validator and
117+
# is configured correctly.
118+
foreign_type_column = problematic_columns.find { |column| column.name == reflection.foreign_type }
119+
next if foreign_key_column.nil? && foreign_type_column.nil?
120+
121+
# Otherwise, don't report errors about missing validators on the
122+
# foreign key or type, but instead ...
123+
problematic_columns.delete(foreign_key_column)
124+
problematic_columns.delete(foreign_type_column)
125+
126+
# ... report an error about an incorrectly configured polymorphic
127+
# association.
128+
problematic_polymorphic_associations << reflection.name
129+
else
130+
# If the foreign key is not one of the columns that lack a
131+
# validator then it means the association added a validator and is
132+
# configured correctly.
133+
next if foreign_key_column.nil?
134+
135+
# Otherwise, don't report an error about a missing validator on
136+
# the foreign key, but instead ...
137+
problematic_columns.delete(foreign_key_column)
138+
139+
# ... report an error about an incorrectly configured association.
140+
problematic_associations << reflection.name
141+
end
142+
end
80143

81-
belongs_to = model.reflect_on_all_associations(:belongs_to).find do |reflection|
82-
reflection.foreign_key == column.name
83-
end
84-
allowed_attributes << belongs_to.name.to_sym if belongs_to
144+
# Finally, regular and polymorphic associations that are explicitly
145+
# ignored should be removed from the output. It's NOT enough to skip
146+
# processing them in the loop above because their underlying foreign
147+
# key and type columns must be removed from output, too.
148+
problematic_associations.reject! do |name|
149+
config(:ignore_attributes).include?("#{model.name}.#{name}")
150+
end
151+
problematic_polymorphic_associations.reject! do |name|
152+
config(:ignore_attributes).include?("#{model.name}.#{name}")
153+
end
85154

86-
model.validators.any? do |validator|
87-
validator.is_a?(ActiveRecord::Validations::PresenceValidator) &&
88-
(validator.attributes & allowed_attributes).present?
155+
# Job is done and all accumulated errors can be reported.
156+
problematic_polymorphic_associations.each do |name|
157+
problem!(type: :optional_polymorphic_association, column_or_association: name, model: model.name)
158+
end
159+
problematic_associations.each do |name|
160+
problem!(type: :optional_association, column_or_association: name, model: model.name)
161+
end
162+
problematic_columns.each do |column|
163+
problem!(type: :missing_validator, column_or_association: column.name, model: model.name)
164+
end
89165
end
90166
end
91167

92-
def inclusion_validator_items(validator)
168+
# Normalizes the list of values passed to an inclusion or exclusion validator.
169+
def inclusion_or_exclusion_validator_items(validator)
93170
validator.options[:in] || validator.options[:within] || []
94171
end
95172

173+
# Determines whether the given column is used as a counter cache column by
174+
# a has_many association on the model.
96175
def counter_cache_column?(model, column)
97176
model.reflect_on_all_associations(:has_many).any? do |reflection|
98177
reflection.has_cached_counter? && reflection.counter_cache_column == column.name

‎test/active_record_doctor/detectors/missing_presence_validation_test.rb‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,28 @@ def test_non_null_column_is_not_reported_if_association_validation_present
4242
refute_problems
4343
end
4444

45+
def test_association_type_non_null_column_is_not_reported_if_polymorphic_association_validation_present
46+
Context.create_table(:comments) do |t|
47+
t.references :commentable, null: false, polymorphic: true
48+
end.define_model do
49+
belongs_to :commentable, polymorphic: true, optional: false
50+
end
51+
52+
refute_problems
53+
end
54+
55+
def test_association_type_non_null_column_is_reported_if_polymorphic_association_validation_absent
56+
Context.create_table(:comments) do |t|
57+
t.references :commentable, null: false, polymorphic: true
58+
end.define_model do
59+
belongs_to :commentable, polymorphic: true, optional: true
60+
end
61+
62+
assert_problems(<<~OUTPUT)
63+
add `optional: false` to Context::Comment.commentable - the foreign key commentable_id or type commentable_type are NOT NULL
64+
OUTPUT
65+
end
66+
4567
def test_not_null_column_is_not_reported_if_habtm_association
4668
Context.create_table(:users).define_model do
4769
has_and_belongs_to_many :projects, class_name: "Context::Project"
@@ -286,6 +308,23 @@ def test_config_ignore_attributes
286308
refute_problems
287309
end
288310

311+
def test_config_ignore_polymorphic_associations_via_relationship_name
312+
Context.create_table(:comments) do |t|
313+
t.references :commentable, null: false, polymorphic: true
314+
end.define_model do
315+
belongs_to :commentable, optional: true, polymorphic: true
316+
end
317+
318+
config_file(<<-CONFIG)
319+
ActiveRecordDoctor.configure do |config|
320+
config.detector :missing_presence_validation,
321+
ignore_attributes: ["Context::Comment.commentable"]
322+
end
323+
CONFIG
324+
325+
refute_problems
326+
end
327+
289328
def test_config_ignore_columns_with_default_columns_are_not_ignored_by_default
290329
Context.create_table(:users) do |t|
291330
t.integer :posts_count, null: false, default: 0

‎test/setup.rb‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,10 @@ class ApplicationRecord < ActiveRecord::Base
7575

7676
ActiveRecord::Base.establish_connection :primary
7777

78+
# After connecting to the database, ensure default settings are compatible with
79+
# what Rails uses out of the box.
80+
ActiveRecord::Base.belongs_to_required_by_default = true
81+
7882
# Transient Record contexts used by the test class below.
7983
Context = TransientRecord.context_for ApplicationRecord
8084

0 commit comments

Comments
 (0)