diff --git a/lib/active_record_doctor/detectors/missing_presence_validation.rb b/lib/active_record_doctor/detectors/missing_presence_validation.rb index e1a5a2c..bcc3f3d 100644 --- a/lib/active_record_doctor/detectors/missing_presence_validation.rb +++ b/lib/active_record_doctor/detectors/missing_presence_validation.rb @@ -21,78 +21,157 @@ class MissingPresenceValidation < Base # :nodoc: private - def message(column:, model:) - "add a `presence` validator to #{model}.#{column} - it's NOT NULL but lacks a validator" + def message(type:, column_or_association:, model:) + case type + when :missing_validator + "add a `presence` validator to #{model}.#{column_or_association} - it's NOT NULL but lacks a validator" + when :optional_association + "add `optional: false` to #{model}.#{column_or_association} - the foreign key #{column_or_association}_id is NOT NULL" # rubocop:disable Layout/LineLength + when :optional_polymorphic_association + "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 + end end def detect each_model(except: config(:ignore_models), existing_tables_only: true) do |model| - each_attribute(model, except: config(:ignore_attributes)) do |column| - next unless validator_needed?(model, column) - next if validator_present?(model, column) - next if counter_cache_column?(model, column) - - problem!(column: column.name, model: model.name) + # List all columns and then remove those that don't need or don't have + # a missing validator. + problematic_columns = connection.columns(model.table_name) + problematic_columns.reject! do |column| + # The primary key, timestamps, and counter caches are special + # columns that are automatically managed by Rails and don't need + # an explicit presence validator. + column.name == model.primary_key || + ["created_at", "updated_at", "created_on", "updated_on"].include?(column.name) || + counter_cache_column?(model, column) || + + # NULL-able columns don't need a presence validator as they can be + # set to NULL after all. A check constraint (column IS NOT NULL) + # is an alternative approach and the absence of such constraint is + # tested below. + (column.null && !not_null_check_constraint_exists?(model.table_name, column)) || + + # If requested, columns with a default value don't need presence + # validation as they'd have the default value substituted automatically. + (config(:ignore_columns_with_default) && (column.default || column.default_function)) || + + # Explicitly ignored columns should be skipped. + config(:ignore_attributes).include?("#{model.name}.#{column.name}") end - end - end - def validator_needed?(model, column) - ![model.primary_key, "created_at", "updated_at", "created_on", "updated_on"].include?(column.name) && - (!column.null || not_null_check_constraint_exists?(model.table_name, column)) && - !default_value_instead_of_validation?(column) - end - - def default_value_instead_of_validation?(column) - (!column.default.nil? || column.default_function) && config(:ignore_columns_with_default) - end - - def validator_present?(model, column) - inclusion_validator_present?(model, column) || - exclusion_validator_present?(model, column) || - presence_validator_present?(model, column) - end - - def inclusion_validator_present?(model, column) - model.validators.any? do |validator| - validator_items = inclusion_validator_items(validator) - return true if validator_items.is_a?(Proc) - - validator.is_a?(ActiveModel::Validations::InclusionValidator) && - validator.attributes.map(&:to_s).include?(column.name) && - !validator_items.include?(nil) - end - end - - def exclusion_validator_present?(model, column) - model.validators.any? do |validator| - validator_items = inclusion_validator_items(validator) - return true if validator_items.is_a?(Proc) + # At this point the only columns that are left are those that DO + # need presence validation in the model. Let's iterate over all + # validators to see which columns are actually validated, but before + # we do that let's define a map for quickly translating foreign key + # names to belongs_to association names. + column_name_to_association_name = {} + model.reflect_on_all_associations(:belongs_to).each do |reflection| + column_name_to_association_name[reflection.foreign_key] = reflection.name + if reflection.polymorphic? + column_name_to_association_name[reflection.foreign_type] = reflection.name + end + end - validator.is_a?(ActiveModel::Validations::ExclusionValidator) && - validator.attributes.include?(column.name.to_sym) && - validator_items.include?(nil) - end - end + # We're now ready to iterate over the validators and remove columns + # that are validated directly or via an association name. + model.validators.each do |validator| + problematic_columns.reject! do |column| + # Translate a foreign key or type to the association name. + attribute = column_name_to_association_name[column.name] || column.name.to_sym + + case validator + + # A regular presence validator is enough if the column name is + # listed among the attributes it's validating. + when ActiveRecord::Validations::PresenceValidator + validator.attributes.include?(attribute) + + # An inclusion validator ensures the column is not nil if it covers + # the column and nil is NOT one of the values it allows. + when ActiveModel::Validations::InclusionValidator + validator_items = inclusion_or_exclusion_validator_items(validator) + validator.attributes.include?(attribute) && + (validator_items.is_a?(Proc) || validator_items.exclude?(nil)) + + # An exclusion validator ensures the column is not nil if it covers + # the column and excludes nil as an allowed value explicitly. + when ActiveModel::Validations::ExclusionValidator + validator_items = inclusion_or_exclusion_validator_items(validator) + validator.attributes.include?(attribute) && + (validator_items.is_a?(Proc) || validator_items.include?(nil)) + + end + end + end - def presence_validator_present?(model, column) - allowed_attributes = [column.name.to_sym] + # Associations need to be checked whether they're marked optional + # while the underlying foreign key or type columns are marked NOT NULL. + problematic_associations = [] + problematic_polymorphic_associations = [] + + model.reflect_on_all_associations.each do |reflection| + foreign_key_column = problematic_columns.find { |column| column.name == reflection.foreign_key } + if reflection.polymorphic? + # If the foreign key and type are not one of the columns that lack + # a validator then it means the association added a validator and + # is configured correctly. + foreign_type_column = problematic_columns.find { |column| column.name == reflection.foreign_type } + next if foreign_key_column.nil? && foreign_type_column.nil? + + # Otherwise, don't report errors about missing validators on the + # foreign key or type, but instead ... + problematic_columns.delete(foreign_key_column) + problematic_columns.delete(foreign_type_column) + + # ... report an error about an incorrectly configured polymorphic + # association. + problematic_polymorphic_associations << reflection.name + else + # If the foreign key is not one of the columns that lack a + # validator then it means the association added a validator and is + # configured correctly. + next if foreign_key_column.nil? + + # Otherwise, don't report an error about a missing validator on + # the foreign key, but instead ... + problematic_columns.delete(foreign_key_column) + + # ... report an error about an incorrectly configured association. + problematic_associations << reflection.name + end + end - belongs_to = model.reflect_on_all_associations(:belongs_to).find do |reflection| - reflection.foreign_key == column.name - end - allowed_attributes << belongs_to.name.to_sym if belongs_to + # Finally, regular and polymorphic associations that are explicitly + # ignored should be removed from the output. It's NOT enough to skip + # processing them in the loop above because their underlying foreign + # key and type columns must be removed from output, too. + problematic_associations.reject! do |name| + config(:ignore_attributes).include?("#{model.name}.#{name}") + end + problematic_polymorphic_associations.reject! do |name| + config(:ignore_attributes).include?("#{model.name}.#{name}") + end - model.validators.any? do |validator| - validator.is_a?(ActiveRecord::Validations::PresenceValidator) && - (validator.attributes & allowed_attributes).present? + # Job is done and all accumulated errors can be reported. + problematic_polymorphic_associations.each do |name| + problem!(type: :optional_polymorphic_association, column_or_association: name, model: model.name) + end + problematic_associations.each do |name| + problem!(type: :optional_association, column_or_association: name, model: model.name) + end + problematic_columns.each do |column| + problem!(type: :missing_validator, column_or_association: column.name, model: model.name) + end end end - def inclusion_validator_items(validator) + # Normalizes the list of values passed to an inclusion or exclusion validator. + def inclusion_or_exclusion_validator_items(validator) validator.options[:in] || validator.options[:within] || [] end + # Determines whether the given column is used as a counter cache column by + # a has_many association on the model. def counter_cache_column?(model, column) model.reflect_on_all_associations(:has_many).any? do |reflection| reflection.has_cached_counter? && reflection.counter_cache_column == column.name diff --git a/test/active_record_doctor/detectors/missing_presence_validation_test.rb b/test/active_record_doctor/detectors/missing_presence_validation_test.rb index 77c7ef8..4596076 100644 --- a/test/active_record_doctor/detectors/missing_presence_validation_test.rb +++ b/test/active_record_doctor/detectors/missing_presence_validation_test.rb @@ -42,6 +42,28 @@ def test_non_null_column_is_not_reported_if_association_validation_present refute_problems end + def test_association_type_non_null_column_is_not_reported_if_polymorphic_association_validation_present + Context.create_table(:comments) do |t| + t.references :commentable, null: false, polymorphic: true + end.define_model do + belongs_to :commentable, polymorphic: true, optional: false + end + + refute_problems + end + + def test_association_type_non_null_column_is_reported_if_polymorphic_association_validation_absent + Context.create_table(:comments) do |t| + t.references :commentable, null: false, polymorphic: true + end.define_model do + belongs_to :commentable, polymorphic: true, optional: true + end + + assert_problems(<<~OUTPUT) + add `optional: false` to Context::Comment.commentable - the foreign key commentable_id or type commentable_type are NOT NULL + OUTPUT + end + def test_not_null_column_is_not_reported_if_habtm_association Context.create_table(:users).define_model do has_and_belongs_to_many :projects, class_name: "Context::Project" @@ -286,6 +308,23 @@ def test_config_ignore_attributes refute_problems end + def test_config_ignore_polymorphic_associations_via_relationship_name + Context.create_table(:comments) do |t| + t.references :commentable, null: false, polymorphic: true + end.define_model do + belongs_to :commentable, optional: true, polymorphic: true + end + + config_file(<<-CONFIG) + ActiveRecordDoctor.configure do |config| + config.detector :missing_presence_validation, + ignore_attributes: ["Context::Comment.commentable"] + end + CONFIG + + refute_problems + end + def test_config_ignore_columns_with_default_columns_are_not_ignored_by_default Context.create_table(:users) do |t| t.integer :posts_count, null: false, default: 0 diff --git a/test/setup.rb b/test/setup.rb index 34fc21e..6be9323 100644 --- a/test/setup.rb +++ b/test/setup.rb @@ -75,6 +75,10 @@ class ApplicationRecord < ActiveRecord::Base ActiveRecord::Base.establish_connection :primary +# After connecting to the database, ensure default settings are compatible with +# what Rails uses out of the box. +ActiveRecord::Base.belongs_to_required_by_default = true + # Transient Record contexts used by the test class below. Context = TransientRecord.context_for ApplicationRecord