From 47467fe33d0c94ba46d70c530a438a4efb8864ff Mon Sep 17 00:00:00 2001 From: Alex Ghiculescu Date: Thu, 1 Jul 2021 16:10:41 -0500 Subject: [PATCH] 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. --- activerecord/lib/active_record.rb | 9 +++ .../connection_adapters/abstract_adapter.rb | 5 ++ .../postgresql/referential_integrity.rb | 32 +++++++++ .../connection_adapters/sqlite3_adapter.rb | 4 ++ activerecord/lib/active_record/fixtures.rb | 4 ++ activerecord/test/cases/fixtures_test.rb | 70 +++++++++++++++++++ .../test/fixtures/fk_object_to_point_to.yml | 2 + activerecord/test/schema/schema.rb | 10 +++ guides/source/configuring.md | 13 ++-- .../lib/rails/application/configuration.rb | 4 ++ .../new_framework_defaults_7_0.rb.tt | 3 + .../test/application/configuration_test.rb | 26 +++++++ 12 files changed, 177 insertions(+), 5 deletions(-) create mode 100644 activerecord/test/fixtures/fk_object_to_point_to.yml diff --git a/activerecord/lib/active_record.rb b/activerecord/lib/active_record.rb index dc7d94335b..de8b86c9ad 100644 --- a/activerecord/lib/active_record.rb +++ b/activerecord/lib/active_record.rb @@ -313,6 +313,15 @@ module ActiveRecord singleton_class.attr_accessor :suppress_multiple_database_warning self.suppress_multiple_database_warning = false + ## + # :singleton-method: + # If true, Rails will verify all foreign keys in the database after loading fixtures. + # An error will be raised if there are any foreign key violations, indicating incorrectly + # written fixtures. + # Supported by PostgreSQL and SQLite. + singleton_class.attr_accessor :verify_foreign_keys_for_fixtures + self.verify_foreign_keys_for_fixtures = false + def self.eager_load! super ActiveRecord::Locking.eager_load! diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index 7f4366f0b5..361a49183b 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -489,6 +489,11 @@ module ActiveRecord yield end + # Override to check all foreign key constraints in a database. + def all_foreign_keys_valid? + true + end + # CONNECTION MANAGEMENT ==================================== # Checks whether the connection to the database is still active. This includes diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/referential_integrity.rb b/activerecord/lib/active_record/connection_adapters/postgresql/referential_integrity.rb index dfb0029daf..e332e36ef9 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/referential_integrity.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/referential_integrity.rb @@ -37,6 +37,38 @@ Rails needs superuser privileges to disable referential integrity. rescue ActiveRecord::ActiveRecordError end end + + def all_foreign_keys_valid? # :nodoc: + sql = <<~SQL + do $$ + declare r record; + BEGIN + FOR r IN ( + SELECT FORMAT( + 'UPDATE pg_constraint SET convalidated=false WHERE conname = ''%I''; ALTER TABLE %I VALIDATE CONSTRAINT %I;', + constraint_name, + table_name, + constraint_name + ) AS constraint_check + FROM information_schema.table_constraints WHERE constraint_type = 'FOREIGN KEY' + ) + LOOP + EXECUTE (r.constraint_check); + END LOOP; + END; + $$; + SQL + + begin + transaction(requires_new: true) do + execute(sql) + end + + true + rescue ActiveRecord::StatementInvalid + false + end + end end end end diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb index c8107c940f..439bfd266f 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb @@ -211,6 +211,10 @@ module ActiveRecord end end + def all_foreign_keys_valid? # :nodoc: + execute("PRAGMA foreign_key_check").blank? + end + # SCHEMA STATEMENTS ======================================== def primary_keys(table_name) # :nodoc: diff --git a/activerecord/lib/active_record/fixtures.rb b/activerecord/lib/active_record/fixtures.rb index d8c4ff6bf4..7162b033d7 100644 --- a/activerecord/lib/active_record/fixtures.rb +++ b/activerecord/lib/active_record/fixtures.rb @@ -637,6 +637,10 @@ module ActiveRecord 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 + # Cap primary key sequences to max(pk). if conn.respond_to?(:reset_pk_sequence!) set.each { |fs| conn.reset_pk_sequence!(fs.table_name) } diff --git a/activerecord/test/cases/fixtures_test.rb b/activerecord/test/cases/fixtures_test.rb index ba4cb88f38..a466cccac9 100644 --- a/activerecord/test/cases/fixtures_test.rb +++ b/activerecord/test/cases/fixtures_test.rb @@ -790,6 +790,76 @@ class ForeignKeyFixturesTest < ActiveRecord::TestCase end end +class FkObjectToPointTo < ActiveRecord::Base + has_many :fk_pointing_to_non_existent_objects +end +class FkPointingToNonExistentObject < ActiveRecord::Base + belongs_to :fk_object_to_point_to +end + +class FixturesWithForeignKeyViolationsTest < ActiveRecord::TestCase + fixtures :fk_object_to_point_to + + def setup + # other tests in this file load the parrots fixture but not the treasure one (see `test_create_fixtures`). + # this creates FK violations since Parrot and ParrotTreasure records are created. + # 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) + 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) + + with_verify_foreign_keys_for_fixtures do + if current_adapter?(:SQLite3Adapter, :PostgreSQLAdapter) + assert_raise RuntimeError do + ActiveRecord::FixtureSet.create_fixtures(FIXTURES_ROOT, ["fk_pointing_to_non_existent_object"]) + end + else + assert_nothing_raised do + ActiveRecord::FixtureSet.create_fixtures(FIXTURES_ROOT, ["fk_pointing_to_non_existent_object"]) + end + end + end + + ensure + File.delete(FIXTURES_ROOT + "/fk_pointing_to_non_existent_object.yml") + ActiveRecord::FixtureSet.reset_cache + end + + def test_does_not_raise_if_no_fk_violations + fk_pointing_to_valid_object = <<~FIXTURE + 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) + + with_verify_foreign_keys_for_fixtures do + assert_nothing_raised do + ActiveRecord::FixtureSet.create_fixtures(FIXTURES_ROOT, ["fk_pointing_to_non_existent_object"]) + end + end + + ensure + File.delete(FIXTURES_ROOT + "/fk_pointing_to_non_existent_object.yml") + ActiveRecord::FixtureSet.reset_cache + end + + private + def with_verify_foreign_keys_for_fixtures + setting_was = ActiveRecord.verify_foreign_keys_for_fixtures + ActiveRecord.verify_foreign_keys_for_fixtures = true + yield + ensure + ActiveRecord.verify_foreign_keys_for_fixtures = setting_was + end +end + class OverRideFixtureMethodTest < ActiveRecord::TestCase fixtures :topics diff --git a/activerecord/test/fixtures/fk_object_to_point_to.yml b/activerecord/test/fixtures/fk_object_to_point_to.yml new file mode 100644 index 0000000000..c3ec546209 --- /dev/null +++ b/activerecord/test/fixtures/fk_object_to_point_to.yml @@ -0,0 +1,2 @@ +first: + id: 1 diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index ae2709725e..87bdf91ba4 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -1241,6 +1241,16 @@ ActiveRecord::Schema.define do end end + disable_referential_integrity do + create_table :fk_object_to_point_tos, force: :cascade do |t| + end + + create_table :fk_pointing_to_non_existent_objects, force: true do |t| + t.references :fk_object_to_point_to, null: false, index: false + t.foreign_key :fk_object_to_point_tos, column: "fk_object_to_point_to_id", name: "fk_that_will_be_broken" + end + end + create_table :overloaded_types, force: true do |t| t.float :overloaded_float, default: 500 t.float :unoverloaded_float diff --git a/guides/source/configuring.md b/guides/source/configuring.md index 3dd928dc2a..07c50f65e8 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -481,6 +481,13 @@ in controllers and views. This defaults to `false`. * `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`. +* `config.active_record.destroy_all_in_batches` ensures + ActiveRecord::Relation#destroy_all to perform the record's deletion in batches. + ActiveRecord::Relation#destroy_all won't longer return the collection of the deleted + records after enabling this option. + +* `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`. + The MySQL adapter adds one additional configuration option: * `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: `fk_rails_` are not exported to the database schema dump. Defaults to `/^fk_rails_[0-9a-f]{10}$/`. -* `config.active_record.destroy_all_in_batches` ensures - ActiveRecord::Relation#destroy_all to perform the record's deletion in batches. - ActiveRecord::Relation#destroy_all won't longer return the collection of the deleted - records after enabling this option. - ### Configuring Action Controller `config.action_controller` includes a number of configuration settings: @@ -1103,6 +1105,7 @@ text/javascript image/svg+xml application/postscript application/x-shockwave-fla - `config.action_controller.silence_disabled_session_errors` : `false` - `config.action_mailer.smtp_timeout`: `5` - `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"` +- `config.active_record.verify_foreign_keys_for_fixtures`: `true` #### For '6.1', defaults from previous versions below and: diff --git a/railties/lib/rails/application/configuration.rb b/railties/lib/rails/application/configuration.rb index e3c742e2af..41b5711b64 100644 --- a/railties/lib/rails/application/configuration.rb +++ b/railties/lib/rails/application/configuration.rb @@ -226,6 +226,10 @@ module Rails "-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" end + + if respond_to?(:active_record) + active_record.verify_foreign_keys_for_fixtures = true + end else raise "Unknown version #{target_version.to_s.inspect}" end diff --git a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt index 434d33f673..b7a14076a8 100644 --- a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt +++ b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt @@ -55,3 +55,6 @@ # Enforce destroy in batches when calling ActiveRecord::Relation#destroy_all # Rails.application.config.active_record.destroy_all_in_batches = true + +# Raise when running tests if fixtures contained foreign key violations +# Rails.application.config.active_record.verify_foreign_keys_for_fixtures = true diff --git a/railties/test/application/configuration_test.rb b/railties/test/application/configuration_test.rb index 1f53d2b53f..91251d35cd 100644 --- a/railties/test/application/configuration_test.rb +++ b/railties/test/application/configuration_test.rb @@ -2305,6 +2305,32 @@ module ApplicationTests assert_equal true, ActiveRecord::Base.has_many_inversing end + test "ActiveRecord.verify_foreign_keys_for_fixtures is true by default for new apps" do + app "development" + + assert_equal true, ActiveRecord.verify_foreign_keys_for_fixtures + end + + test "ActiveRecord.verify_foreign_keys_for_fixtures is false by default for upgraded apps" do + remove_from_config '.*config\.load_defaults.*\n' + + app "development" + + assert_equal false, ActiveRecord.verify_foreign_keys_for_fixtures + end + + test "ActiveRecord.verify_foreign_keys_for_fixtures can be configured via config.active_record.verify_foreign_keys_for_fixtures" do + remove_from_config '.*config\.load_defaults.*\n' + + app_file "config/initializers/new_framework_defaults_7_0.rb", <<-RUBY + Rails.application.config.active_record.verify_foreign_keys_for_fixtures = true + RUBY + + app "development" + + assert_equal true, ActiveRecord.verify_foreign_keys_for_fixtures + end + test "ActiveSupport::MessageEncryptor.use_authenticated_message_encryption is true by default for new apps" do app "development"