From 3c6be5e502e203be17373f30336d59a7511ef31b Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Sun, 17 May 2020 07:11:56 +0900 Subject: [PATCH] Default engine `ENGINE=InnoDB` is no longer dumped to make schema more agnostic 5 years ago, I made dumping full table options at #17569, especially to dump `ENGINE=InnoDB ROW_FORMAT=DYNAMIC` to use utf8mb4 with large key prefix. In that time, omitting the default engine `ENGINE=InnoDB` was not useful since `ROW_FORMAT=DYNAMIC` always remains as long as using utf8mb4 with large key prefix. But now, MySQL 5.7.9 has finally changed the default row format to DYNAMIC, utf8mb4 with large key prefix can be used without dumping the default engine and the row format explicitly. So now is a good time to make the default engine is omitted. Before: ```ruby create_table "accounts", options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci", force: :cascade do |t| end ``` After: ```ruby create_table "accounts", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| end ``` To entirely omit `:options` option to make schema agnostic, I've added `:charset` and `:collation` table options to exclude `CHARSET` and `COLLATE` from `:options`. Fixes #26209. Closes #29472. See also #33608, #33853, and #34742. --- activerecord/CHANGELOG.md | 20 ++++- .../abstract/schema_creation.rb | 15 +--- .../abstract/schema_definitions.rb | 3 +- .../abstract/schema_statements.rb | 12 +-- .../abstract_mysql_adapter.rb | 15 +++- .../mysql/schema_creation.rb | 7 +- .../mysql/schema_definitions.rb | 8 ++ .../mysql/schema_statements.rb | 4 +- .../postgresql/schema_statements.rb | 4 +- .../sqlite3/schema_statements.rb | 4 +- .../adapters/mysql2/table_options_test.rb | 73 +++++++------------ 11 files changed, 87 insertions(+), 78 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 0118ddaca5..1aa24618be 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,6 +1,24 @@ +* Default engine `ENGINE=InnoDB` is no longer dumped to make schema more agnostic. + + Before: + + ```ruby + create_table "accounts", options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci", force: :cascade do |t| + end + ``` + + After: + + ```ruby + create_table "accounts", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| + end + ``` + + *Ryuta Kamizono* + * Added delegated type as an alternative to single-table inheritance for representing class hierarchies. See ActiveRecord::DelegatedType for the full description. - + *DHH* * Deprecate aggregations with group by duplicated fields. diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_creation.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_creation.rb index 759d8f523b..0d3cffd586 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_creation.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_creation.rb @@ -53,7 +53,7 @@ module ActiveRecord end create_sql << "(#{statements.join(', ')})" if statements.present? - add_table_options!(create_sql, table_options(o)) + add_table_options!(create_sql, o) create_sql << " AS #{to_sql(o.as)}" if o.as create_sql end @@ -106,17 +106,8 @@ module ActiveRecord true end - def table_options(o) - table_options = {} - table_options[:comment] = o.comment - table_options[:options] = o.options - table_options - end - - def add_table_options!(create_sql, options) - if options_sql = options[:options] - create_sql << " #{options_sql}" - end + def add_table_options!(create_sql, o) + create_sql << " #{o.options}" if o.options create_sql end 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 45242488f8..7e3b225c91 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb @@ -277,7 +277,8 @@ module ActiveRecord if_not_exists: false, options: nil, as: nil, - comment: nil + comment: nil, + ** ) @conn = conn @columns_hash = {} 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 2634bc5b2b..44b15f7780 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -295,9 +295,7 @@ module ActiveRecord # # See also TableDefinition#column for details on how to create columns. def create_table(table_name, id: :primary_key, primary_key: nil, force: nil, **options) - td = create_table_definition( - table_name, **options.extract!(:temporary, :if_not_exists, :options, :as, :comment) - ) + td = create_table_definition(table_name, **extract_table_options!(options)) if id && !td.as pk = primary_key || Base.get_primary_key(table_name.to_s.singularize) @@ -1379,14 +1377,18 @@ module ActiveRecord SchemaCreation.new(self) end - def create_table_definition(*args, **options) - TableDefinition.new(self, *args, **options) + def create_table_definition(name, **options) + TableDefinition.new(self, name, **options) end def create_alter_table(name) AlterTable.new create_table_definition(name) end + def extract_table_options!(options) + options.extract!(:temporary, :if_not_exists, :options, :as, :comment, :charset, :collation) + end + def fetch_type_metadata(sql_type) cast_type = lookup_cast_type(sql_type) SqlTypeMetadata.new( diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb index 8925a96b8a..3435e2884e 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -414,24 +414,31 @@ module ActiveRecord end def table_options(table_name) # :nodoc: - table_options = {} - create_table_info = create_table_info(table_name) # strip create_definitions and partition_options # Be aware that `create_table_info` might not include any table options due to `NO_TABLE_OPTIONS` sql mode. raw_table_options = create_table_info.sub(/\A.*\n\) ?/m, "").sub(/\n\/\*!.*\*\/\n\z/m, "").strip + return if raw_table_options.empty? + + table_options = {} + + if / DEFAULT CHARSET=(?\w+)(?: COLLATE=(?\w+))?/ =~ raw_table_options + raw_table_options = $` + $' # before part + after part + table_options[:charset] = charset + table_options[:collation] = collation if collation + end + # strip AUTO_INCREMENT raw_table_options.sub!(/(ENGINE=\w+)(?: AUTO_INCREMENT=\d+)/, '\1') - table_options[:options] = raw_table_options unless raw_table_options.blank? - # strip COMMENT if raw_table_options.sub!(/ COMMENT='.+'/, "") table_options[:comment] = table_comment(table_name) end + table_options[:options] = raw_table_options unless raw_table_options == "ENGINE=InnoDB" table_options end diff --git a/activerecord/lib/active_record/connection_adapters/mysql/schema_creation.rb b/activerecord/lib/active_record/connection_adapters/mysql/schema_creation.rb index 8e5351d14b..3b91e15b2c 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql/schema_creation.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql/schema_creation.rb @@ -40,8 +40,11 @@ module ActiveRecord add_sql_comment!(sql.join(" "), o.comment) end - def add_table_options!(create_sql, options) - add_sql_comment!(super, options[:comment]) + def add_table_options!(create_sql, o) + create_sql = super + create_sql << " DEFAULT CHARSET=#{o.charset}" if o.charset + create_sql << " COLLATE=#{o.collation}" if o.collation + add_sql_comment!(create_sql, o.comment) end def add_column_options!(sql, options) diff --git a/activerecord/lib/active_record/connection_adapters/mysql/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/mysql/schema_definitions.rb index d21535a709..52a8a0b97d 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql/schema_definitions.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql/schema_definitions.rb @@ -60,6 +60,14 @@ module ActiveRecord class TableDefinition < ActiveRecord::ConnectionAdapters::TableDefinition include ColumnMethods + attr_reader :charset, :collation + + def initialize(conn, name, charset: nil, collation: nil, **) + super + @charset = charset + @collation = collation + end + def new_column_definition(name, type, **options) # :nodoc: case type when :virtual diff --git a/activerecord/lib/active_record/connection_adapters/mysql/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/mysql/schema_statements.rb index 9447e2e8d8..af5683d23f 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql/schema_statements.rb @@ -154,8 +154,8 @@ module ActiveRecord MySQL::SchemaCreation.new(self) end - def create_table_definition(*args, **options) - MySQL::TableDefinition.new(self, *args, **options) + def create_table_definition(name, **options) + MySQL::TableDefinition.new(self, name, **options) end def new_column_from_field(table_name, field) diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb index 1a6b9b04fe..ba60ab1d67 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb @@ -621,8 +621,8 @@ module ActiveRecord PostgreSQL::SchemaCreation.new(self) end - def create_table_definition(*args, **options) - PostgreSQL::TableDefinition.new(self, *args, **options) + def create_table_definition(name, **options) + PostgreSQL::TableDefinition.new(self, name, **options) end def create_alter_table(name) diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb index 824e9372b9..558cd649ef 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb @@ -87,8 +87,8 @@ module ActiveRecord SQLite3::SchemaCreation.new(self) end - def create_table_definition(*args, **options) - SQLite3::TableDefinition.new(self, *args, **options) + def create_table_definition(name, **options) + SQLite3::TableDefinition.new(self, name, **options) end def validate_index_length!(table_name, new_name, internal = false) diff --git a/activerecord/test/cases/adapters/mysql2/table_options_test.rb b/activerecord/test/cases/adapters/mysql2/table_options_test.rb index b91f4ef137..a47413da38 100644 --- a/activerecord/test/cases/adapters/mysql2/table_options_test.rb +++ b/activerecord/test/cases/adapters/mysql2/table_options_test.rb @@ -5,6 +5,7 @@ require "support/schema_dumping_helper" class Mysql2TableOptionsTest < ActiveRecord::Mysql2TestCase include SchemaDumpingHelper + self.use_transactional_tests = false def setup @connection = ActiveRecord::Base.connection @@ -17,29 +18,36 @@ class Mysql2TableOptionsTest < ActiveRecord::Mysql2TestCase test "table options with ENGINE" do @connection.create_table "mysql_table_options", force: true, options: "ENGINE=MyISAM" output = dump_table_schema("mysql_table_options") - options = %r{create_table "mysql_table_options", options: "(?.*)"}.match(output)[:options] - assert_match %r{ENGINE=MyISAM}, options + expected = /create_table "mysql_table_options", charset: "utf8mb4"(?:, collation: "\w+")?, options: "ENGINE=MyISAM", force: :cascade/ + assert_match expected, output end test "table options with ROW_FORMAT" do @connection.create_table "mysql_table_options", force: true, options: "ROW_FORMAT=REDUNDANT" output = dump_table_schema("mysql_table_options") - options = %r{create_table "mysql_table_options", options: "(?.*)"}.match(output)[:options] - assert_match %r{ROW_FORMAT=REDUNDANT}, options + expected = /create_table "mysql_table_options", charset: "utf8mb4"(?:, collation: "\w+")?, options: "ENGINE=InnoDB ROW_FORMAT=REDUNDANT", force: :cascade/ + assert_match expected, output end test "table options with CHARSET" do - @connection.create_table "mysql_table_options", force: true, options: "CHARSET=utf8mb4" + @connection.create_table "mysql_table_options", force: true, options: "CHARSET=latin1" output = dump_table_schema("mysql_table_options") - options = %r{create_table "mysql_table_options", options: "(?.*)"}.match(output)[:options] - assert_match %r{CHARSET=utf8mb4}, options + expected = /create_table "mysql_table_options", charset: "latin1", force: :cascade/ + assert_match expected, output end test "table options with COLLATE" do @connection.create_table "mysql_table_options", force: true, options: "COLLATE=utf8mb4_bin" output = dump_table_schema("mysql_table_options") - options = %r{create_table "mysql_table_options", options: "(?.*)"}.match(output)[:options] - assert_match %r{COLLATE=utf8mb4_bin}, options + expected = /create_table "mysql_table_options", charset: "utf8mb4", collation: "utf8mb4_bin", force: :cascade/ + assert_match expected, output + end + + test "charset and collation options" do + @connection.create_table "mysql_table_options", force: true, charset: "utf8mb4", collation: "utf8mb4_bin" + output = dump_table_schema("mysql_table_options") + expected = /create_table "mysql_table_options", charset: "utf8mb4", collation: "utf8mb4_bin", force: :cascade/ + assert_match expected, output end test "schema dump works with NO_TABLE_OPTIONS sql mode" do @@ -60,47 +68,10 @@ class Mysql2TableOptionsTest < ActiveRecord::Mysql2TestCase end end -class Mysql2DefaultEngineOptionSchemaDumpTest < ActiveRecord::Mysql2TestCase +class Mysql2DefaultEngineOptionTest < ActiveRecord::Mysql2TestCase include SchemaDumpingHelper self.use_transactional_tests = false - def setup - @verbose_was = ActiveRecord::Migration.verbose - ActiveRecord::Migration.verbose = false - end - - def teardown - ActiveRecord::Base.connection.drop_table "mysql_table_options", if_exists: true - ActiveRecord::Migration.verbose = @verbose_was - ActiveRecord::SchemaMigration.delete_all rescue nil - end - - test "schema dump includes ENGINE=InnoDB if not provided" do - ActiveRecord::Base.connection.create_table "mysql_table_options", force: true - - output = dump_table_schema("mysql_table_options") - options = %r{create_table "mysql_table_options", options: "(?.*)"}.match(output)[:options] - assert_match %r{ENGINE=InnoDB}, options - end - - test "schema dump includes ENGINE=InnoDB in legacy migrations" do - migration = Class.new(ActiveRecord::Migration[5.1]) do - def migrate(x) - create_table "mysql_table_options", force: true - end - end.new - - ActiveRecord::Migrator.new(:up, [migration], ActiveRecord::Base.connection.schema_migration).migrate - - output = dump_table_schema("mysql_table_options") - options = %r{create_table "mysql_table_options", options: "(?.*)"}.match(output)[:options] - assert_match %r{ENGINE=InnoDB}, options - end -end - -class Mysql2DefaultEngineOptionSqlOutputTest < ActiveRecord::Mysql2TestCase - self.use_transactional_tests = false - def setup @logger_was = ActiveRecord::Base.logger @log = StringIO.new @@ -120,6 +91,10 @@ class Mysql2DefaultEngineOptionSqlOutputTest < ActiveRecord::Mysql2TestCase ActiveRecord::Base.connection.create_table "mysql_table_options", force: true assert_no_match %r{ENGINE=InnoDB}, @log.string + + output = dump_table_schema("mysql_table_options") + expected = /create_table "mysql_table_options", charset: "utf8mb4"(?:, collation: "\w+")?, force: :cascade/ + assert_match expected, output end test "legacy migrations contain default ENGINE=InnoDB option" do @@ -132,5 +107,9 @@ class Mysql2DefaultEngineOptionSqlOutputTest < ActiveRecord::Mysql2TestCase ActiveRecord::Migrator.new(:up, [migration], ActiveRecord::Base.connection.schema_migration).migrate assert_match %r{ENGINE=InnoDB}, @log.string + + output = dump_table_schema("mysql_table_options") + expected = /create_table "mysql_table_options", charset: "utf8mb4"(?:, collation: "\w+")?, force: :cascade/ + assert_match expected, output end end