Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

activerecord: Avoid mysql non-null text column defaulting to empty string #7780

Closed
wants to merge 3 commits into from

5 participants

@dylanahsmith

The mysql database adaptor assumes that non-null text columns behave as if the default value is an empty string. This is the default behaviour for mysql connections, but putting the connection in strict mode with SET SQL_MODE='STRICT_ALL_TABLES' makes like other databases, where the default value is NULL so the value must be specified.

The mysql adaptor defaults to using this strict mode, although it can be disabled with setting strict: false in the database configuration. However, the mysql adapter has a hack based on non-strict mode behaviour to detect an empty string default value. So even though strict mode is used, this hack is continuing non-strict mode behaviour within strict mode.

I propose just removing this hack to mimic the non-strict behaviour to make its behaviour consistent with other databases. This is what this pull request does.

@dylanahsmith dylanahsmith activerecord: Avoid mysql non-null text column defaulting to empty st…
…ring.

With the :strict config, which defaults to true, the non-null text
columns behave as expected. So the mysql adapter hack to make it behave as
if the default is an empty string is continuing non-strict mode behaviour
within strict mode.
e2b1daf
@rafaelfranca

I agree with this patch.

@jonleighton this is related with your last change could you review?

@jonleighton
Collaborator

I will take a proper look after the weekend. Thanks!

@jeremy
Owner

This causes a regression if you don't have strict set, though.

..._record/connection_adapters/abstract_mysql_adapter.rb
@@ -14,7 +14,7 @@ def initialize(name, default, sql_type = nil, null = true, collation = nil)
def extract_default(default)
if sql_type =~ /blob/i || type == :text
if default.blank?
- return null ? nil : ''
+ return nil

The return keyword isn't necessary here.

removed it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dylanahsmith

This causes a regression if you don't have strict set, though.

Yes. If they explicitly set strict: false in their database.yml, then it will be a regression. However, by setting this config they likely expect quirky behaviour like this. Do we really want to support this hack to work around this quirk when that is the point strict mode? If we want to remove it, then the best time to do it is before a major release (e.g. 4.0).

Here is a commit that would fix the regression if we decide to continue supporting this hack for non-strict mode dylanahsmith@fd1c0af

@jonleighton
Collaborator

I am :+1: on this. I don't think the regression for non-strict is severe enough to justify the pain of having this workaround in our code base given that strict is the default. I agree that the 4.0 release is a good time to introduce this. However, please add a clear explanation to the CHANGELOG so that people have something to refer to when upgrading.

activerecord/test/cases/defaults_test.rb
@@ -51,10 +51,6 @@ class DefaultsTestWithoutTransactionalFixtures < ActiveRecord::TestCase
# 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
@jonleighton Collaborator

I think we can just completely rm this test?

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dylanahsmith

However, please add a clear explanation to the CHANGELOG so that people have something to refer to when upgrading.

I looked into this and noticed that strict mode wasn't used in rails 3.2, and that :strict configuration value wasn't available. It was added afterwards. So no changelog is needed for this change.

@jonleighton jonleighton was assigned
@jonleighton
Collaborator

I discussed with the @jeremy the other day and he was concerned that we would be pushing busy work onto users by merging this, because if a user has strict: false then the default is reported by mysql as null but it is in fact an empty string. (By which I mean insert into foo values () will place an empty string in the field, but show full fields from foo will report null.)

However, I think that in strict: true mode, if a user has chosen to make the field a not null, they don't actually want it to be magically populated with an empty string, as this then changes the expected behaviour of strict mode.

So my suggestion is that:

  • We keep the hack which makes Column#default report '', but only for non-strict mode
  • We get rid of the keys.concat thing in dirty.rb. If the user is in strict mode, they should get an error if they haven't set the field's value. If they're in non-strict mode, the database will set the field's value to '' automatically.

I think they allows users an opt-out from the busy work (by setting strict: false) but keeps the semantics of strict mode consistent and allows us to avoid a horrible hack in dirty.rb.

I will try to catch up with @jeremy about this again today.

@jonleighton jonleighton closed this pull request from a commit
@jonleighton jonleighton 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
af8c8b4
@jonleighton
Collaborator

@dylanahsmith I talked to @jeremy and he was ok with my proposal so I've implemented that, see above commit. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 28, 2012
  1. @dylanahsmith

    activerecord: Avoid mysql non-null text column defaulting to empty st…

    dylanahsmith authored
    …ring.
    
    With the :strict config, which defaults to true, the non-null text
    columns behave as expected. So the mysql adapter hack to make it behave as
    if the default is an empty string is continuing non-strict mode behaviour
    within strict mode.
  2. @dylanahsmith
Commits on Oct 9, 2012
  1. @dylanahsmith
This page is out of date. Refresh to see the latest.
View
12 activerecord/lib/active_record/attribute_methods/dirty.rb
@@ -68,17 +68,7 @@ def update(*)
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
View
6 activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -14,7 +14,7 @@ def initialize(name, default, sql_type = nil, null = true, collation = nil)
def extract_default(default)
if sql_type =~ /blob/i || type == :text
if default.blank?
- return null ? nil : ''
+ nil
else
raise ArgumentError, "#{type} columns cannot have a default value: #{default.inspect}"
end
@@ -30,10 +30,6 @@ def has_default?
super
end
- def explicit_default?
- !null && (sql_type =~ /blob/i || type == :text)
- end
-
# Must return the relevant concrete adapter
def adapter
raise NotImplementedError
View
4 activerecord/lib/active_record/connection_adapters/column.rb
@@ -53,10 +53,6 @@ def has_default?
!default.nil?
end
- def explicit_default?
- false
- end
-
# Returns the Ruby class that corresponds to the abstract data type.
def klass
case type
View
4 activerecord/test/cases/column_definition_test.rb
@@ -78,7 +78,7 @@ def test_should_not_set_default_for_blob_and_text_data_types
assert_equal nil, text_column.default
not_null_text_column = MysqlAdapter::Column.new("title", nil, "text", false)
- assert_equal "", not_null_text_column.default
+ assert_equal nil, not_null_text_column.default
end
def test_has_default_should_return_false_for_blog_and_test_data_types
@@ -112,7 +112,7 @@ def test_should_not_set_default_for_blob_and_text_data_types
assert_equal nil, text_column.default
not_null_text_column = Mysql2Adapter::Column.new("title", nil, "text", false)
- assert_equal "", not_null_text_column.default
+ assert_equal nil, not_null_text_column.default
end
def test_has_default_should_return_false_for_blog_and_test_data_types
View
30 activerecord/test/cases/defaults_test.rb
@@ -51,36 +51,6 @@ class DefaultsTestWithoutTransactionalFixtures < ActiveRecord::TestCase
# 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
- klass = Class.new(ActiveRecord::Base)
- klass.table_name = 'test_mysql_text_not_null_defaults'
- klass.connection.create_table klass.table_name do |t|
- t.column :non_null_text, :text, :null => false
- t.column :non_null_blob, :blob, :null => false
- 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
- ensure
- klass.connection.drop_table(klass.table_name) rescue nil
- end
-
# MySQL uses an implicit default 0 rather than NULL unless in strict mode.
# We use an implicit NULL so schema.rb is compatible with other databases.
def test_mysql_integer_not_null_defaults
View
7 activerecord/test/cases/migration_test.rb
@@ -343,12 +343,7 @@ def test_create_table_with_binary_column
columns = Person.connection.columns(:binary_testings)
data_column = columns.detect { |c| c.name == "data" }
-
- if current_adapter?(:MysqlAdapter) or current_adapter?(:Mysql2Adapter)
- assert_equal '', data_column.default
- else
- assert_nil data_column.default
- end
+ assert_nil data_column.default
Person.connection.drop_table :binary_testings rescue nil
end
Something went wrong with that request. Please try again.