Skip to content

Commit 47467fe

Browse files
committed
Verify foreign keys after loading fixtures
When writing fixtures, it's currently possible to define associations that don't exist, even if a foreign key exists. For example: ```yml george: name: "Curious George" pirate: redbeard blackbeard: name: "Blackbeard" ``` When the fixtures are created, `parrots(:george).pirate` will be nil, but it's not immediately clear why. This can make it hard to debug tests and can give false confidence in passing ones. This can happen because Rails [disables referential integrity](https://github.com/rails/rails/blob/f263530bf709f611920b5f663882e5113b4f984b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb#L407) when inserting fixtures. This makes the fixtures algorithm much simpler - it can just create the fixtures in alphabetical order and assume that the other side of a foreign key constraint will *eventually* be added. Ideally we would check foreign keys once all fixtures have been loaded, so that we can be sure that the foreign key constraints were met. This PR introduces that. To enable it: ```ruby config.active_record.verify_foreign_keys_for_fixtures = true ``` I'm proposing we enable this in 7.0 for new apps and have added it to new framework defaults. When run against our app, it found 3 fixture files with unmet FK constraints - turns out all those fixtures weren't being used and were safe to delete.
1 parent 0fc31fe commit 47467fe

File tree

12 files changed

+177
-5
lines changed

12 files changed

+177
-5
lines changed

activerecord/lib/active_record.rb

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -313,6 +313,15 @@ def self.global_executor_concurrency # :nodoc:
313313
singleton_class.attr_accessor :suppress_multiple_database_warning
314314
self.suppress_multiple_database_warning = false
315315

316+
##
317+
# :singleton-method:
318+
# If true, Rails will verify all foreign keys in the database after loading fixtures.
319+
# An error will be raised if there are any foreign key violations, indicating incorrectly
320+
# written fixtures.
321+
# Supported by PostgreSQL and SQLite.
322+
singleton_class.attr_accessor :verify_foreign_keys_for_fixtures
323+
self.verify_foreign_keys_for_fixtures = false
324+
316325
def self.eager_load!
317326
super
318327
ActiveRecord::Locking.eager_load!

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -489,6 +489,11 @@ def disable_referential_integrity
489489
yield
490490
end
491491

492+
# Override to check all foreign key constraints in a database.
493+
def all_foreign_keys_valid?
494+
true
495+
end
496+
492497
# CONNECTION MANAGEMENT ====================================
493498

494499
# Checks whether the connection to the database is still active. This includes

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,38 @@ def disable_referential_integrity # :nodoc:
3737
rescue ActiveRecord::ActiveRecordError
3838
end
3939
end
40+
41+
def all_foreign_keys_valid? # :nodoc:
42+
sql = <<~SQL
43+
do $$
44+
declare r record;
45+
BEGIN
46+
FOR r IN (
47+
SELECT FORMAT(
48+
'UPDATE pg_constraint SET convalidated=false WHERE conname = ''%I''; ALTER TABLE %I VALIDATE CONSTRAINT %I;',
49+
constraint_name,
50+
table_name,
51+
constraint_name
52+
) AS constraint_check
53+
FROM information_schema.table_constraints WHERE constraint_type = 'FOREIGN KEY'
54+
)
55+
LOOP
56+
EXECUTE (r.constraint_check);
57+
END LOOP;
58+
END;
59+
$$;
60+
SQL
61+
62+
begin
63+
transaction(requires_new: true) do
64+
execute(sql)
65+
end
66+
67+
true
68+
rescue ActiveRecord::StatementInvalid
69+
false
70+
end
71+
end
4072
end
4173
end
4274
end

activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,10 @@ def disable_referential_integrity # :nodoc:
211211
end
212212
end
213213

214+
def all_foreign_keys_valid? # :nodoc:
215+
execute("PRAGMA foreign_key_check").blank?
216+
end
217+
214218
# SCHEMA STATEMENTS ========================================
215219

216220
def primary_keys(table_name) # :nodoc:

activerecord/lib/active_record/fixtures.rb

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -637,6 +637,10 @@ def insert(fixture_sets, connection) # :nodoc:
637637

638638
conn.insert_fixtures_set(table_rows_for_connection, table_rows_for_connection.keys)
639639

640+
if ActiveRecord.verify_foreign_keys_for_fixtures && !conn.all_foreign_keys_valid?
641+
raise "Foreign key violations found in your fixture data. Ensure you aren't referring to labels that don't exist on associations."
642+
end
643+
640644
# Cap primary key sequences to max(pk).
641645
if conn.respond_to?(:reset_pk_sequence!)
642646
set.each { |fs| conn.reset_pk_sequence!(fs.table_name) }

activerecord/test/cases/fixtures_test.rb

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -790,6 +790,76 @@ def test_number2
790790
end
791791
end
792792

793+
class FkObjectToPointTo < ActiveRecord::Base
794+
has_many :fk_pointing_to_non_existent_objects
795+
end
796+
class FkPointingToNonExistentObject < ActiveRecord::Base
797+
belongs_to :fk_object_to_point_to
798+
end
799+
800+
class FixturesWithForeignKeyViolationsTest < ActiveRecord::TestCase
801+
fixtures :fk_object_to_point_to
802+
803+
def setup
804+
# other tests in this file load the parrots fixture but not the treasure one (see `test_create_fixtures`).
805+
# this creates FK violations since Parrot and ParrotTreasure records are created.
806+
# those violations can cause false positives in these tests. since they aren't related to these tests we
807+
# delete the irrelevant records here (this test is transactional so it's fine).
808+
Parrot.all.each(&:destroy)
809+
end
810+
811+
def test_raises_fk_violations
812+
fk_pointing_to_non_existent_object = <<~FIXTURE
813+
first:
814+
fk_object_to_point_to: one
815+
FIXTURE
816+
File.write(FIXTURES_ROOT + "/fk_pointing_to_non_existent_object.yml", fk_pointing_to_non_existent_object)
817+
818+
with_verify_foreign_keys_for_fixtures do
819+
if current_adapter?(:SQLite3Adapter, :PostgreSQLAdapter)
820+
assert_raise RuntimeError do
821+
ActiveRecord::FixtureSet.create_fixtures(FIXTURES_ROOT, ["fk_pointing_to_non_existent_object"])
822+
end
823+
else
824+
assert_nothing_raised do
825+
ActiveRecord::FixtureSet.create_fixtures(FIXTURES_ROOT, ["fk_pointing_to_non_existent_object"])
826+
end
827+
end
828+
end
829+
830+
ensure
831+
File.delete(FIXTURES_ROOT + "/fk_pointing_to_non_existent_object.yml")
832+
ActiveRecord::FixtureSet.reset_cache
833+
end
834+
835+
def test_does_not_raise_if_no_fk_violations
836+
fk_pointing_to_valid_object = <<~FIXTURE
837+
first:
838+
fk_object_to_point_to_id: 1
839+
FIXTURE
840+
File.write(FIXTURES_ROOT + "/fk_pointing_to_non_existent_object.yml", fk_pointing_to_valid_object)
841+
842+
with_verify_foreign_keys_for_fixtures do
843+
assert_nothing_raised do
844+
ActiveRecord::FixtureSet.create_fixtures(FIXTURES_ROOT, ["fk_pointing_to_non_existent_object"])
845+
end
846+
end
847+
848+
ensure
849+
File.delete(FIXTURES_ROOT + "/fk_pointing_to_non_existent_object.yml")
850+
ActiveRecord::FixtureSet.reset_cache
851+
end
852+
853+
private
854+
def with_verify_foreign_keys_for_fixtures
855+
setting_was = ActiveRecord.verify_foreign_keys_for_fixtures
856+
ActiveRecord.verify_foreign_keys_for_fixtures = true
857+
yield
858+
ensure
859+
ActiveRecord.verify_foreign_keys_for_fixtures = setting_was
860+
end
861+
end
862+
793863
class OverRideFixtureMethodTest < ActiveRecord::TestCase
794864
fixtures :topics
795865

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
first:
2+
id: 1

activerecord/test/schema/schema.rb

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1241,6 +1241,16 @@
12411241
end
12421242
end
12431243

1244+
disable_referential_integrity do
1245+
create_table :fk_object_to_point_tos, force: :cascade do |t|
1246+
end
1247+
1248+
create_table :fk_pointing_to_non_existent_objects, force: true do |t|
1249+
t.references :fk_object_to_point_to, null: false, index: false
1250+
t.foreign_key :fk_object_to_point_tos, column: "fk_object_to_point_to_id", name: "fk_that_will_be_broken"
1251+
end
1252+
end
1253+
12441254
create_table :overloaded_types, force: true do |t|
12451255
t.float :overloaded_float, default: 500
12461256
t.float :unoverloaded_float

guides/source/configuring.md

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -481,6 +481,13 @@ in controllers and views. This defaults to `false`.
481481

482482
* `config.active_record.enumerate_columns_in_select_statements` when true, will always include column names in `SELECT` statements, and avoid wildcard `SELECT * FROM ...` queries. This avoids prepared statement cache errors when adding columns to a PostgreSQL database for example. Defaults to `false`.
483483

484+
* `config.active_record.destroy_all_in_batches` ensures
485+
ActiveRecord::Relation#destroy_all to perform the record's deletion in batches.
486+
ActiveRecord::Relation#destroy_all won't longer return the collection of the deleted
487+
records after enabling this option.
488+
489+
* `config.active_record.verify_foreign_keys_for_fixtures` ensures all foreign key constraints are valid after fixtures are loaded in tests. Supported by PostgreSQL and SQLite only. Defaults to `false`.
490+
484491
The MySQL adapter adds one additional configuration option:
485492

486493
* `ActiveRecord::ConnectionAdapters::Mysql2Adapter.emulate_booleans` controls whether Active Record will consider all `tinyint(1)` columns as booleans. Defaults to `true`.
@@ -511,11 +518,6 @@ The schema dumper adds two additional configuration options:
511518
`fk_rails_` are not exported to the database schema dump.
512519
Defaults to `/^fk_rails_[0-9a-f]{10}$/`.
513520
514-
* `config.active_record.destroy_all_in_batches` ensures
515-
ActiveRecord::Relation#destroy_all to perform the record's deletion in batches.
516-
ActiveRecord::Relation#destroy_all won't longer return the collection of the deleted
517-
records after enabling this option.
518-
519521
### Configuring Action Controller
520522
521523
`config.action_controller` includes a number of configuration settings:
@@ -1103,6 +1105,7 @@ text/javascript image/svg+xml application/postscript application/x-shockwave-fla
11031105
- `config.action_controller.silence_disabled_session_errors` : `false`
11041106
- `config.action_mailer.smtp_timeout`: `5`
11051107
- `config.active_storage.video_preview_arguments`: `"-vf 'select=eq(n\\,0)+eq(key\\,1)+gt(scene\\,0.015),loop=loop=-1:size=2,trim=start_frame=1' -frames:v 1 -f image2"`
1108+
- `config.active_record.verify_foreign_keys_for_fixtures`: `true`
11061109

11071110
#### For '6.1', defaults from previous versions below and:
11081111

railties/lib/rails/application/configuration.rb

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,10 @@ def load_defaults(target_version)
226226
"-vf 'select=eq(n\\,0)+eq(key\\,1)+gt(scene\\,0.015),loop=loop=-1:size=2,trim=start_frame=1'" \
227227
" -frames:v 1 -f image2"
228228
end
229+
230+
if respond_to?(:active_record)
231+
active_record.verify_foreign_keys_for_fixtures = true
232+
end
229233
else
230234
raise "Unknown version #{target_version.to_s.inspect}"
231235
end

0 commit comments

Comments
 (0)