missing_foreign_keys: use belongs_to to identify foreign keys

Author: hatsu38

lib/active_record_doctor/config/default.rb

@@ -36,7 +36,7 @@
 
   detector :missing_foreign_keys,
     enabled: true,
-    ignore_tables: [],
+    ignore_models: [],
     ignore_columns: []
 
   detector :missing_non_null_constraint,

lib/active_record_doctor/detectors/missing_foreign_keys.rb

@@ -7,8 +7,8 @@ module Detectors
     class MissingForeignKeys < Base # :nodoc:
       @description = "detect foreign-key-like columns lacking an actual foreign key constraint"
       @config = {
-        ignore_tables: {
-          description: "tables whose columns should not be checked",
+        ignore_models: {
+          description: "models whose columns should not be checked",
           global: true
         },
         ignore_columns: {
@@ -23,42 +23,18 @@ def message(table:, column:)
       end
 
       def detect
-        each_table(except: config(:ignore_tables)) do |table|
-          each_column(table, except: config(:ignore_columns)) do |column|
-            # We need to skip polymorphic associations as they can reference
-            # multiple tables but a foreign key constraint can reference
-            # a single predefined table.
-            next unless looks_like_foreign_key?(column)
-            next if foreign_key?(table, column)
-            next if polymorphic_foreign_key?(table, column)
-            next if model_destroyed_async?(table, column)
+        each_model(except: config(:ignore_models), existing_tables_only: true) do |model|
+          foreign_keys = connection.foreign_keys(model.table_name)
+          foreign_key_columns = foreign_keys.map { |key| key.options[:column] }
 
-            problem!(table: table, column: column.name)
-          end
-        end
-      end
-
-      def foreign_key?(table, column)
-        connection.foreign_keys(table).any? do |foreign_key|
-          foreign_key.options[:column] == column.name
-        end
-      end
+          each_association(model, type: :belongs_to) do |association|
+            next if ignored?(association.foreign_key, config(:ignore_columns))
+            next if association.options[:polymorphic]
 
-      def polymorphic_foreign_key?(table, column)
-        type_column_name = column.name.sub(/_id\Z/, "_type")
-        connection.columns(table).any? do |another_column|
-          another_column.name == type_column_name
-        end
-      end
+            has_foreign_key = foreign_key_columns.include?(association.foreign_key)
+            next if has_foreign_key
 
-      def model_destroyed_async?(table, column)
-        # Check if there are any models having `has_many ..., dependent: :destroy_async`
-        # referencing the specified table.
-        models.any? do |model|
-          model.reflect_on_all_associations(:has_many).any? do |reflection|
-            reflection.options[:dependent] == :destroy_async &&
-              reflection.foreign_key == column.name &&
-              reflection.klass.table_name == table
+            problem!(table: model.table_name, column: association.foreign_key)
           end
         end
       end

test/active_record_doctor/detectors/missing_foreign_keys_test.rb

@@ -5,6 +5,8 @@ def test_missing_foreign_key_is_reported
     Context.create_table(:companies)
     Context.create_table(:users) do |t|
       t.references :company, foreign_key: false
+    end.define_model do
+      belongs_to :company
     end
 
     assert_problems(<<~OUTPUT)
@@ -16,14 +18,29 @@ def test_present_foreign_key_is_not_reported
     Context.create_table(:companies)
     Context.create_table(:users) do |t|
       t.references :company, foreign_key: true
+    end.define_model do
+      belongs_to :company
     end
 
     refute_problems
   end
 
-  def test_non_integer_missing_foreign_key_is_reported
+  def test_missing_foreign_key_on_abstract_class_is_not_reported
+    Context.create_table(:companies)
+    Context.create_table(:users) do |t|
+      t.references :company, foreign_key: false
+    end.define_model do
+      self.abstract_class = true
+      belongs_to :company
+    end
+
+    refute_problems
+  end
+
+  def test_missing_foreign_key_without_belongs_to_is_not_reported
+    Context.create_table(:companies)
     Context.create_table(:users) do |t|
-      t.string :external_id
+      t.references :company, foreign_key: false
     end
 
     refute_problems
@@ -32,8 +49,13 @@ def test_non_integer_missing_foreign_key_is_reported
   def test_uuid_missing_foreign_key_is_reported
     require_uuid_column_type!
 
+    Context.create_table(:companies) do |t|
+      t.uuid :id, default: "gen_random_uuid()"
+    end
     Context.create_table(:users) do |t|
-      t.uuid :company_id
+      t.references :company, type: :uuid, foreign_key: false
+    end.define_model do
+      belongs_to :company
     end
 
     assert_problems(<<~OUTPUT)
@@ -60,12 +82,14 @@ def test_config_ignore_models
     Context.create_table(:companies)
     Context.create_table(:users) do |t|
       t.references :company, foreign_key: false
+    end.define_model do
+      belongs_to :company
     end
 
     config_file(<<-CONFIG)
       ActiveRecordDoctor.configure do |config|
         config.detector :missing_foreign_keys,
-          ignore_tables: ["users"]
+          ignore_models: [Context::User]
       end
     CONFIG
 
@@ -76,11 +100,13 @@ def test_global_ignore_models
     Context.create_table(:companies)
     Context.create_table(:users) do |t|
       t.references :company, foreign_key: false
+    end.define_model do
+      belongs_to :company
     end
 
     config_file(<<-CONFIG)
       ActiveRecordDoctor.configure do |config|
-        config.global :ignore_tables, ["users"]
+        config.global :ignore_models, [Context::User]
       end
     CONFIG
 
@@ -102,4 +128,15 @@ def test_config_ignore_columns
 
     refute_problems
   end
+
+  def test_polymorphic_association_is_not_reported
+    Context.create_table(:companies)
+    Context.create_table(:users) do |t|
+      t.references :commentable, polymorphic: true, foreign_key: false
+    end.define_model do
+      belongs_to :commentable, polymorphic: true
+    end
+
+    refute_problems
+  end
 end