mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
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](f263530bf7/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.
This commit is contained in:
parent
0fc31fe28a
commit
47467fe33d
12 changed files with 177 additions and 5 deletions
|
@ -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!
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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:
|
||||
|
|
|
@ -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) }
|
||||
|
|
|
@ -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
|
||||
|
||||
|
|
2
activerecord/test/fixtures/fk_object_to_point_to.yml
vendored
Normal file
2
activerecord/test/fixtures/fk_object_to_point_to.yml
vendored
Normal file
|
@ -0,0 +1,2 @@
|
|||
first:
|
||||
id: 1
|
|
@ -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
|
||||
|
|
|
@ -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:
|
||||
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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"
|
||||
|
||||
|
|
Loading…
Reference in a new issue