Merge pull request rails#47990 from ghiculescu/verify-fk-reason

Show reason for foreign key error when loading fixtures

Author: byroot

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

@@ -636,7 +636,17 @@ def disable_referential_integrity
 
       # Override to check all foreign key constraints in a database.
       def all_foreign_keys_valid?
+        check_all_foreign_keys_valid!
         true
+      rescue ActiveRecord::StatementInvalid
+        false
+      end
+      deprecate :all_foreign_keys_valid?, deprecator: ActiveRecord.deprecator
+
+      # Override to check all foreign key constraints in a database.
+      # The adapter should raise a +ActiveRecord::StatementInvalid+ if foreign key
+      # constraints are not met.
+      def check_all_foreign_keys_valid!
       end
 
       # CONNECTION MANAGEMENT ====================================

activerecord/lib/active_record/connection_adapters/postgresql/referential_integrity.rb

@@ -38,7 +38,7 @@ def disable_referential_integrity # :nodoc:
           end
         end
 
-        def all_foreign_keys_valid? # :nodoc:
+        def check_all_foreign_keys_valid! # :nodoc:
           sql = <<~SQL
             do $$
               declare r record;
@@ -61,14 +61,8 @@ def all_foreign_keys_valid? # :nodoc:
             $$;
           SQL
 
-          begin
-            transaction(requires_new: true) do
-              execute(sql)
-            end
-
-            true
-          rescue ActiveRecord::StatementInvalid
-            false
+          transaction(requires_new: true) do
+            execute(sql)
           end
         end
       end

activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb

@@ -241,8 +241,14 @@ def disable_referential_integrity # :nodoc:
         end
       end
 
-      def all_foreign_keys_valid? # :nodoc:
-        execute("PRAGMA foreign_key_check").blank?
+      def check_all_foreign_keys_valid! # :nodoc:
+        sql = "PRAGMA foreign_key_check"
+        result = execute(sql)
+
+        unless result.blank?
+          tables = result.map { |row| row["table"] }
+          raise ActiveRecord::StatementInvalid.new("Foreign key violations found: #{tables.join(", ")}", sql: sql)
+        end
       end
 
       # SCHEMA STATEMENTS ========================================

activerecord/lib/active_record/fixtures.rb

@@ -646,9 +646,7 @@ def insert(fixture_sets, connection) # :nodoc:
 
             conn.insert_fixtures_set(table_rows_for_connection, table_rows_for_connection.keys)
 
-            if ActiveRecord.verify_foreign_keys_for_fixtures && !conn.all_foreign_keys_valid?
-              raise "Foreign key violations found in your fixture data. Ensure you aren't referring to labels that don't exist on associations."
-            end
+            check_all_foreign_keys_valid!(conn)
 
             # Cap primary key sequences to max(pk).
             if conn.respond_to?(:reset_pk_sequence!)
@@ -657,6 +655,16 @@ def insert(fixture_sets, connection) # :nodoc:
           end
         end
 
+        def check_all_foreign_keys_valid!(conn)
+          return unless ActiveRecord.verify_foreign_keys_for_fixtures
+
+          begin
+            conn.check_all_foreign_keys_valid!
+          rescue ActiveRecord::StatementInvalid => e
+            raise "Foreign key violations found in your fixture data. Ensure you aren't referring to labels that don't exist on associations. Error from database:\n\n#{e.message}"
+          end
+        end
+
         def update_all_loaded_fixtures(fixtures_map) # :nodoc:
           all_loaded_fixtures.update(fixtures_map)
         end

activerecord/test/cases/adapter_test.rb

@@ -445,6 +445,12 @@ def test_truncate_tables_with_query_cache
       @connection.disable_query_cache!
     end
 
+    def test_all_foreign_keys_valid_is_deprecated
+      assert_deprecated(ActiveRecord.deprecator) do
+        @connection.all_foreign_keys_valid?
+      end
+    end
+
     # test resetting sequences in odd tables in PostgreSQL
     if ActiveRecord::Base.connection.respond_to?(:reset_pk_sequence!)
       require "models/movie"

activerecord/test/cases/fixtures_test.rb

@@ -850,20 +850,24 @@ def setup
     # those violations can cause false positives in these tests. since they aren't related to these tests we
     # delete the irrelevant records here (this test is transactional so it's fine).
     Parrot.all.each(&:destroy)
+
+    @path = "/fk_pointing_to_non_existent_object.yml"
   end
 
   def test_raises_fk_violations
     fk_pointing_to_non_existent_object = <<~FIXTURE
     first:
       fk_object_to_point_to: one
     FIXTURE
-    File.write(FIXTURES_ROOT + "/fk_pointing_to_non_existent_object.yml", fk_pointing_to_non_existent_object)
+    File.write(FIXTURES_ROOT + @path, fk_pointing_to_non_existent_object)
 
     with_verify_foreign_keys_for_fixtures do
       if current_adapter?(:SQLite3Adapter, :PostgreSQLAdapter)
-        assert_raise RuntimeError do
+        error = assert_raise RuntimeError do
           ActiveRecord::FixtureSet.create_fixtures(FIXTURES_ROOT, ["fk_pointing_to_non_existent_object"])
         end
+        assert_includes error.message, "Foreign key violations found in your fixture data. Ensure you aren't referring to labels that don't exist on associations."
+        assert_includes error.message, "fk_pointing_to_non_existent_objects"
       else
         assert_nothing_raised do
           ActiveRecord::FixtureSet.create_fixtures(FIXTURES_ROOT, ["fk_pointing_to_non_existent_object"])
@@ -872,7 +876,7 @@ def test_raises_fk_violations
     end
 
   ensure
-    File.delete(FIXTURES_ROOT + "/fk_pointing_to_non_existent_object.yml")
+    File.delete(FIXTURES_ROOT + @path)
     ActiveRecord::FixtureSet.reset_cache
   end
 
@@ -881,7 +885,7 @@ def test_does_not_raise_if_no_fk_violations
     first:
       fk_object_to_point_to_id: 1
     FIXTURE
-    File.write(FIXTURES_ROOT + "/fk_pointing_to_non_existent_object.yml", fk_pointing_to_valid_object)
+    File.write(FIXTURES_ROOT + @path, fk_pointing_to_valid_object)
 
     with_verify_foreign_keys_for_fixtures do
       assert_nothing_raised do
@@ -890,7 +894,7 @@ def test_does_not_raise_if_no_fk_violations
     end
 
   ensure
-    File.delete(FIXTURES_ROOT + "/fk_pointing_to_non_existent_object.yml")
+    File.delete(FIXTURES_ROOT + @path)
     ActiveRecord::FixtureSet.reset_cache
   end