From af8c8b432abfe478f478fee35c5624c8c04d65e4 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Fri, 19 Oct 2012 15:08:58 +0100 Subject: The default value of a text/blob in mysql strict mode should be nil In non-strict mode it is '', but if someone is in strict mode then we should honour the strict semantics. Also, this removes the need for a completely horrible hack in dirty.rb. Closes #7780 --- .../lib/active_record/attribute_methods/dirty.rb | 12 +--- .../connection_adapters/abstract_mysql_adapter.rb | 18 +++--- .../active_record/connection_adapters/column.rb | 4 -- .../connection_adapters/mysql2_adapter.rb | 6 +- .../connection_adapters/mysql_adapter.rb | 6 +- activerecord/test/cases/defaults_test.rb | 72 +++++++++++++++++----- 6 files changed, 70 insertions(+), 48 deletions(-) diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb index 7a5bb9e863..3417e3af17 100644 --- a/activerecord/lib/active_record/attribute_methods/dirty.rb +++ b/activerecord/lib/active_record/attribute_methods/dirty.rb @@ -68,17 +68,7 @@ module ActiveRecord end def create(*) - if partial_updates? - keys = keys_for_partial_update - - # This is an extremely bloody annoying necessity to work around mysql being crap. - # See test_mysql_text_not_null_defaults - keys.concat self.class.columns.select(&:explicit_default?).map(&:name) - - super keys - else - super - end + partial_updates? ? super(keys_for_partial_update) : super end # Serialized attributes should always be written in case they've been 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 8c83c4f5db..b0e7bd7e82 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -4,17 +4,19 @@ module ActiveRecord module ConnectionAdapters class AbstractMysqlAdapter < AbstractAdapter class Column < ConnectionAdapters::Column # :nodoc: - attr_reader :collation + attr_reader :collation, :strict - def initialize(name, default, sql_type = nil, null = true, collation = nil) - super(name, default, sql_type, null) + def initialize(name, default, sql_type = nil, null = true, collation = nil, strict = false) + @strict = strict @collation = collation + + super(name, default, sql_type, null) end def extract_default(default) if sql_type =~ /blob/i || type == :text if default.blank? - return null ? nil : '' + null || strict ? nil : '' else raise ArgumentError, "#{type} columns cannot have a default value: #{default.inspect}" end @@ -30,10 +32,6 @@ module ActiveRecord super end - def explicit_default? - !null && (sql_type =~ /blob/i || type == :text) - end - # Must return the relevant concrete adapter def adapter raise NotImplementedError @@ -571,6 +569,10 @@ module ActiveRecord where_sql end + def strict_mode? + @config.fetch(:strict, true) + end + protected # MySQL is too stupid to create a temporary table for use subquery, so we have diff --git a/activerecord/lib/active_record/connection_adapters/column.rb b/activerecord/lib/active_record/connection_adapters/column.rb index 2028abf6f0..816b5e17c1 100644 --- a/activerecord/lib/active_record/connection_adapters/column.rb +++ b/activerecord/lib/active_record/connection_adapters/column.rb @@ -53,10 +53,6 @@ module ActiveRecord !default.nil? end - def explicit_default? - false - end - # Returns the Ruby class that corresponds to the abstract data type. def klass case type diff --git a/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb index 328d080687..879eec7fcf 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb @@ -53,7 +53,7 @@ module ActiveRecord end def new_column(field, default, type, null, collation) # :nodoc: - Column.new(field, default, type, null, collation) + Column.new(field, default, type, null, collation, strict_mode?) end def error_number(exception) @@ -259,9 +259,7 @@ module ActiveRecord # Make MySQL reject illegal values rather than truncating or # blanking them. See # http://dev.mysql.com/doc/refman/5.5/en/server-sql-mode.html#sqlmode_strict_all_tables - if @config.fetch(:strict, true) - variable_assignments << "SQL_MODE='STRICT_ALL_TABLES'" - end + variable_assignments << "SQL_MODE='STRICT_ALL_TABLES'" if strict_mode? encoding = @config[:encoding] diff --git a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb index 0b936bbf39..76667616a1 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb @@ -150,7 +150,7 @@ module ActiveRecord end def new_column(field, default, type, null, collation) # :nodoc: - Column.new(field, default, type, null, collation) + Column.new(field, default, type, null, collation, strict_mode?) end def error_number(exception) # :nodoc: @@ -546,9 +546,7 @@ module ActiveRecord # Make MySQL reject illegal values rather than truncating or # blanking them. See # http://dev.mysql.com/doc/refman/5.5/en/server-sql-mode.html#sqlmode_strict_all_tables - if @config.fetch(:strict, true) - execute("SET SQL_MODE='STRICT_ALL_TABLES'", :skip_logging) - end + execute("SET SQL_MODE='STRICT_ALL_TABLES'", :skip_logging) if strict_mode? end def select(sql, name = nil, binds = []) diff --git a/activerecord/test/cases/defaults_test.rb b/activerecord/test/cases/defaults_test.rb index 72f1c99ca0..0df872ff10 100644 --- a/activerecord/test/cases/defaults_test.rb +++ b/activerecord/test/cases/defaults_test.rb @@ -51,11 +51,60 @@ if current_adapter?(:MysqlAdapter) or current_adapter?(:Mysql2Adapter) # We don't want that to happen, so we disable transactional fixtures here. self.use_transactional_fixtures = false - # MySQL 5 and higher is quirky with not null text/blob columns. - # With MySQL Text/blob columns cannot have defaults. If the column is not - # null MySQL will report that the column has a null default - # but it behaves as though the column had a default of '' - def test_mysql_text_not_null_defaults + def using_strict(strict) + connection = ActiveRecord::Model.remove_connection + ActiveRecord::Model.establish_connection connection.merge(strict: strict) + yield + ensure + ActiveRecord::Model.remove_connection + ActiveRecord::Model.establish_connection connection + end + + # MySQL cannot have defaults on text/blob columns. It reports the + # default value as null. + # + # Despite this, in non-strict mode, MySQL will use an empty string + # as the default value of the field, if no other value is + # specified. + # + # Therefore, in non-strict mode, we want column.default to report + # an empty string as its default, to be consistent with that. + # + # In strict mode, column.default should be nil. + def test_mysql_text_not_null_defaults_non_strict + using_strict(false) do + with_text_blob_not_null_table do |klass| + assert_equal '', klass.columns_hash['non_null_blob'].default + assert_equal '', klass.columns_hash['non_null_text'].default + + assert_nil klass.columns_hash['null_blob'].default + assert_nil klass.columns_hash['null_text'].default + + instance = klass.create! + + assert_equal '', instance.non_null_text + assert_equal '', instance.non_null_blob + + assert_nil instance.null_text + assert_nil instance.null_blob + end + end + end + + def test_mysql_text_not_null_defaults_strict + using_strict(true) do + with_text_blob_not_null_table do |klass| + assert_nil klass.columns_hash['non_null_blob'].default + assert_nil klass.columns_hash['non_null_text'].default + assert_nil klass.columns_hash['null_blob'].default + assert_nil klass.columns_hash['null_text'].default + + assert_raises(ActiveRecord::StatementInvalid) { klass.create } + end + end + end + + def with_text_blob_not_null_table klass = Class.new(ActiveRecord::Base) klass.table_name = 'test_mysql_text_not_null_defaults' klass.connection.create_table klass.table_name do |t| @@ -64,19 +113,8 @@ if current_adapter?(:MysqlAdapter) or current_adapter?(:Mysql2Adapter) t.column :null_text, :text, :null => true t.column :null_blob, :blob, :null => true end - assert_equal '', klass.columns_hash['non_null_blob'].default - assert_equal '', klass.columns_hash['non_null_text'].default - - assert_nil klass.columns_hash['null_blob'].default - assert_nil klass.columns_hash['null_text'].default - assert_nothing_raised do - instance = klass.create! - assert_equal '', instance.non_null_text - assert_equal '', instance.non_null_blob - assert_nil instance.null_text - assert_nil instance.null_blob - end + yield klass ensure klass.connection.drop_table(klass.table_name) rescue nil end -- cgit v1.2.3