Rewrite missing_presence_validation to handle polymorphic associations

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

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

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