activerecord: Avoid mysql non-null text column defaulting to empty string #7780
Conversation
…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.
|
I agree with this patch. @jonleighton this is related with your last change could you review? |
|
I will take a proper look after the weekend. Thanks! |
|
This causes a regression if you don't have strict set, though. |
| @@ -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 | |||
There was a problem hiding this comment.
The return keyword isn't necessary here.
Yes. If they explicitly set Here is a commit that would fix the regression if we decide to continue supporting this hack for non-strict mode dylanahsmith@fd1c0af |
|
I am 👍 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. |
| # 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 |
There was a problem hiding this comment.
I think we can just completely rm this test?
I looked into this and noticed that strict mode wasn't used in rails 3.2, and that |
ghost
assigned
|
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 However, I think that in So my suggestion is that:
I think they allows users an opt-out from the busy work (by setting I will try to catch up with @jeremy about this again today. |
|
@dylanahsmith I talked to @jeremy and he was ok with my proposal so I've implemented that, see above commit. Thanks! |
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: falsein 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.