From d3fd4e4ed9bae3775969c5c913b15bbd927e9ad9 Mon Sep 17 00:00:00 2001 From: David Stosik Date: Mon, 19 Mar 2018 17:42:23 +0000 Subject: [PATCH 1/4] Expose foreign key name ignore pattern in configuration When dumping the database schema, Rails will dump foreign key names only if those names were not generate by Rails. Currently this is determined by checking if the foreign key name is `fk_rails_` followed by a 10-character hash. At [Cookpad](https://github.com/cookpad), we use [Departure](https://github.com/departurerb/departure) (Percona's pt-online-schema-change runner for ActiveRecord migrations) to run migrations. Often, `pt-osc` will make a copy of a table in order to run a long migration without blocking it. In this copy process, foreign keys are copied too, but [their name is prefixed with an underscore to prevent name collision ](https://www.percona.com/doc/percona-toolkit/LATEST/pt-online-schema-change.html#cmdoption-pt-online-schema-change-alter-foreign-keys-method). In the process described above, we often end up with a development database that contains foreign keys which name starts with `_fk_rails_`. That name does not match the ignore pattern, so next time Rails dumps the database schema (eg. when running `rake db:migrate`), our `db/schema.rb` file ends up containing those unwanted foreign key names. This also produces an unwanted git diff that we'd prefer not to commit. In this PR, I'd like to suggest a way to expose the foreign key name ignore pattern to the Rails configuration, so that individual projects can decide on a different pattern of foreign keys that will not get their names dumped in `schema.rb`. --- activerecord/lib/active_record.rb | 1 + .../abstract/schema_definitions.rb | 4 ++++ .../abstract/schema_statements.rb | 2 +- activerecord/lib/active_record/core.rb | 2 ++ activerecord/lib/active_record/foreign_keys.rb | 12 ++++++++++++ activerecord/lib/active_record/railtie.rb | 1 + activerecord/lib/active_record/schema_dumper.rb | 2 +- 7 files changed, 22 insertions(+), 2 deletions(-) create mode 100644 activerecord/lib/active_record/foreign_keys.rb diff --git a/activerecord/lib/active_record.rb b/activerecord/lib/active_record.rb index d43378c64f..66084e9b1c 100644 --- a/activerecord/lib/active_record.rb +++ b/activerecord/lib/active_record.rb @@ -42,6 +42,7 @@ module ActiveRecord autoload :CounterCache autoload :DynamicMatchers autoload :Enum + autoload :ForeignKeys autoload :InternalMetadata autoload :Explain autoload :Inheritance diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb index 584a86da21..b91f591e1a 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb @@ -101,6 +101,10 @@ module ActiveRecord end alias validated? validate? + def export_name_on_schema_dump? + name !~ ActiveRecord::Base.fk_ignore_pattern + end + def defined_for?(to_table_ord = nil, to_table: nil, **options) if to_table_ord self.to_table == to_table_ord.to_s diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb index e2147b7fcf..ef45fff9d2 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -1324,7 +1324,7 @@ module ActiveRecord identifier = "#{table_name}_#{options.fetch(:column)}_fk" hashed_identifier = Digest::SHA256.hexdigest(identifier).first(10) - "fk_rails_#{hashed_identifier}" + "#{ActiveRecord::ForeignKeys::PREFIX}_#{hashed_identifier}" end end diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index e1a0b2ecf8..cb0be9787f 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -125,6 +125,8 @@ module ActiveRecord mattr_accessor :belongs_to_required_by_default, instance_accessor: false + mattr_accessor :fk_ignore_pattern, instance_accessor: false + class_attribute :default_connection_handler, instance_writer: false def self.connection_handler diff --git a/activerecord/lib/active_record/foreign_keys.rb b/activerecord/lib/active_record/foreign_keys.rb new file mode 100644 index 0000000000..87ce3ace20 --- /dev/null +++ b/activerecord/lib/active_record/foreign_keys.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module ActiveRecord + module ForeignKeys + # The prefix used by Rails to name unnamed foreign keys. + PREFIX = "fk_rails" + + # Default regular expression used by Rails to determine if a foreign key + # name was generated. + DEFAULT_IGNORE_PATTERN = /^#{PREFIX}_[0-9a-f]{10}$/ + end +end diff --git a/activerecord/lib/active_record/railtie.rb b/activerecord/lib/active_record/railtie.rb index 6ab80a654d..7f58780fdf 100644 --- a/activerecord/lib/active_record/railtie.rb +++ b/activerecord/lib/active_record/railtie.rb @@ -27,6 +27,7 @@ module ActiveRecord config.active_record.use_schema_cache_dump = true config.active_record.maintain_test_schema = true + config.active_record.fk_ignore_pattern = ActiveRecord::ForeignKeys::DEFAULT_IGNORE_PATTERN config.active_record.sqlite3 = ActiveSupport::OrderedOptions.new config.active_record.sqlite3.represent_boolean_as_integer = nil diff --git a/activerecord/lib/active_record/schema_dumper.rb b/activerecord/lib/active_record/schema_dumper.rb index b8d848b999..72b7460342 100644 --- a/activerecord/lib/active_record/schema_dumper.rb +++ b/activerecord/lib/active_record/schema_dumper.rb @@ -210,7 +210,7 @@ HEADER parts << "primary_key: #{foreign_key.primary_key.inspect}" end - if foreign_key.name !~ /^fk_rails_[0-9a-f]{10}$/ + if foreign_key.export_name_on_schema_dump? parts << "name: #{foreign_key.name.inspect}" end From 864e500817e7bcda4753f001ba862f7adbdb1c15 Mon Sep 17 00:00:00 2001 From: David Stosik Date: Tue, 20 Mar 2018 18:08:11 +0000 Subject: [PATCH 2/4] Document config.active_record.fk_ignore_pattern --- activerecord/lib/active_record/core.rb | 4 ++++ guides/source/configuring.md | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index cb0be9787f..4e63d1a273 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -125,6 +125,10 @@ module ActiveRecord mattr_accessor :belongs_to_required_by_default, instance_accessor: false + ## + # :singleton-method: + # Specify a custom regular expression matching foreign keys which name + # should not be dumped to db/schema.rb. mattr_accessor :fk_ignore_pattern, instance_accessor: false class_attribute :default_connection_handler, instance_writer: false diff --git a/guides/source/configuring.md b/guides/source/configuring.md index 368b74f708..8c328f46c7 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -374,6 +374,12 @@ All these configuration options are delegated to the `I18n` library. having to send a query to the database to get this information. Defaults to `true`. +* `config.active_record.fk_ignore_pattern` allows setting a different regular + expression that will be used to decide whether a foreign key's name should be + dumped to db/schema.rb or not. By default, foreign key names starting with + `fk_rails_` are not exported to the database schema dump. + Defaults to `/^fk_rails_[0-9a-f]{10}$/`. + 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`. From 87194b736ef767bf31c997b799d032eb09516aac Mon Sep 17 00:00:00 2001 From: David Stosik Date: Tue, 20 Mar 2018 19:12:36 +0000 Subject: [PATCH 3/4] Test config.active_record.fk_ignore_pattern --- activerecord/test/cases/migration/foreign_key_test.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/activerecord/test/cases/migration/foreign_key_test.rb b/activerecord/test/cases/migration/foreign_key_test.rb index de37215e80..d4a7afac14 100644 --- a/activerecord/test/cases/migration/foreign_key_test.rb +++ b/activerecord/test/cases/migration/foreign_key_test.rb @@ -306,6 +306,14 @@ if ActiveRecord::Base.connection.supports_foreign_keys? assert_match %r{\s+add_foreign_key "fk_test_has_fk", "fk_test_has_pk", column: "fk_id", primary_key: "pk_id", name: "fk_name"$}, output end + def test_schema_dumping_with_custom_fk_ignore_pattern + ActiveRecord::Base.fk_ignore_pattern = /^ignored_/ + @connection.add_foreign_key :astronauts, :rockets, name: :ignored_fk_astronauts_rockets + + output = dump_table_schema "astronauts" + assert_match %r{\s+add_foreign_key "astronauts", "rockets"$}, output + end + def test_schema_dumping_on_delete_and_on_update_options @connection.add_foreign_key :astronauts, :rockets, column: "rocket_id", on_delete: :nullify, on_update: :cascade From f6e612b2723ca7ae730ba3e98267c19356b386b3 Mon Sep 17 00:00:00 2001 From: David Stosik Date: Thu, 22 Mar 2018 10:12:58 +0000 Subject: [PATCH 4/4] Move fk_ignore_pattern from config.active_record to SchemaDumper This makes more sense, as the foreign key ignore pattern is only used by the schema dumper. --- .../abstract/schema_definitions.rb | 2 +- activerecord/lib/active_record/core.rb | 6 ------ activerecord/lib/active_record/railtie.rb | 1 - activerecord/lib/active_record/schema_dumper.rb | 6 ++++++ .../test/cases/migration/foreign_key_test.rb | 5 ++++- guides/source/configuring.md | 14 +++++++------- 6 files changed, 18 insertions(+), 16 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb index b91f591e1a..6a498b353c 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb @@ -102,7 +102,7 @@ module ActiveRecord alias validated? validate? def export_name_on_schema_dump? - name !~ ActiveRecord::Base.fk_ignore_pattern + name !~ ActiveRecord::SchemaDumper.fk_ignore_pattern end def defined_for?(to_table_ord = nil, to_table: nil, **options) diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index 4e63d1a273..e1a0b2ecf8 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -125,12 +125,6 @@ module ActiveRecord mattr_accessor :belongs_to_required_by_default, instance_accessor: false - ## - # :singleton-method: - # Specify a custom regular expression matching foreign keys which name - # should not be dumped to db/schema.rb. - mattr_accessor :fk_ignore_pattern, instance_accessor: false - class_attribute :default_connection_handler, instance_writer: false def self.connection_handler diff --git a/activerecord/lib/active_record/railtie.rb b/activerecord/lib/active_record/railtie.rb index 7f58780fdf..6ab80a654d 100644 --- a/activerecord/lib/active_record/railtie.rb +++ b/activerecord/lib/active_record/railtie.rb @@ -27,7 +27,6 @@ module ActiveRecord config.active_record.use_schema_cache_dump = true config.active_record.maintain_test_schema = true - config.active_record.fk_ignore_pattern = ActiveRecord::ForeignKeys::DEFAULT_IGNORE_PATTERN config.active_record.sqlite3 = ActiveSupport::OrderedOptions.new config.active_record.sqlite3.represent_boolean_as_integer = nil diff --git a/activerecord/lib/active_record/schema_dumper.rb b/activerecord/lib/active_record/schema_dumper.rb index 72b7460342..8fc2752f0c 100644 --- a/activerecord/lib/active_record/schema_dumper.rb +++ b/activerecord/lib/active_record/schema_dumper.rb @@ -17,6 +17,12 @@ module ActiveRecord # Only strings are accepted if ActiveRecord::Base.schema_format == :sql. cattr_accessor :ignore_tables, default: [] + ## + # :singleton-method: + # Specify a custom regular expression matching foreign keys which name + # should not be dumped to db/schema.rb. + cattr_accessor :fk_ignore_pattern, default: ActiveRecord::ForeignKeys::DEFAULT_IGNORE_PATTERN + class << self def dump(connection = ActiveRecord::Base.connection, stream = STDOUT, config = ActiveRecord::Base) connection.create_schema_dumper(generate_options(config)).dump(stream) diff --git a/activerecord/test/cases/migration/foreign_key_test.rb b/activerecord/test/cases/migration/foreign_key_test.rb index d4a7afac14..50f5696ad1 100644 --- a/activerecord/test/cases/migration/foreign_key_test.rb +++ b/activerecord/test/cases/migration/foreign_key_test.rb @@ -307,11 +307,14 @@ if ActiveRecord::Base.connection.supports_foreign_keys? end def test_schema_dumping_with_custom_fk_ignore_pattern - ActiveRecord::Base.fk_ignore_pattern = /^ignored_/ + original_pattern = ActiveRecord::SchemaDumper.fk_ignore_pattern + ActiveRecord::SchemaDumper.fk_ignore_pattern = /^ignored_/ @connection.add_foreign_key :astronauts, :rockets, name: :ignored_fk_astronauts_rockets output = dump_table_schema "astronauts" assert_match %r{\s+add_foreign_key "astronauts", "rockets"$}, output + + ActiveRecord::SchemaDumper.fk_ignore_pattern = original_pattern end def test_schema_dumping_on_delete_and_on_update_options diff --git a/guides/source/configuring.md b/guides/source/configuring.md index 8c328f46c7..8bdba4b3de 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -374,12 +374,6 @@ All these configuration options are delegated to the `I18n` library. having to send a query to the database to get this information. Defaults to `true`. -* `config.active_record.fk_ignore_pattern` allows setting a different regular - expression that will be used to decide whether a foreign key's name should be - dumped to db/schema.rb or not. By default, foreign key names starting with - `fk_rails_` are not exported to the database schema dump. - Defaults to `/^fk_rails_[0-9a-f]{10}$/`. - 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`. @@ -406,10 +400,16 @@ by adding the following to your `application.rb` file: Rails.application.config.active_record.sqlite3.represent_boolean_as_integer = true ``` -The schema dumper adds one additional configuration option: +The schema dumper adds two additional configuration options: * `ActiveRecord::SchemaDumper.ignore_tables` accepts an array of tables that should _not_ be included in any generated schema file. +* `ActiveRecord::SchemaDumper.fk_ignore_pattern` allows setting a different regular + expression that will be used to decide whether a foreign key's name should be + dumped to db/schema.rb or not. By default, foreign key names starting with + `fk_rails_` are not exported to the database schema dump. + Defaults to `/^fk_rails_[0-9a-f]{10}$/`. + ### Configuring Action Controller `config.action_controller` includes a number of configuration settings: