-
Notifications
You must be signed in to change notification settings - Fork 22k
Fix retrieving default value for text column for MariaDB #40822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
72b6e50
to
d08c5bf
Compare
283ffb5
to
9e9e11f
Compare
9e9e11f
to
5595d7b
Compare
@fatkodima @kamipo I didn't see your PR and accidentally created a similar one #41726 🤦♂️ I'll close my PR buy if you want you can port the extra test that I've added for the SchemaDumper: rails/activerecord/test/cases/schema_dumper_test.rb Lines 622 to 626 in f6453e2
|
Would love to see this one merged 🤘. Currently this issue is preventing us from setting default values for serialized JSON columns on MariaDB. |
👀 |
@fatkodima since you're the author could you please fix the conflicts? Also please check #40822 (comment) incase you'd like to pull in the extra tests. If you can't work on this I don't mind opening a new PR. |
5595d7b
to
d836faf
Compare
Rebased and pulled the test case. |
@@ -167,6 +167,9 @@ def new_column_from_field(table_name, field) | |||
elsif type_metadata.extra == "DEFAULT_GENERATED" | |||
default = +"(#{default})" unless default.start_with?("(") | |||
default, default_function = nil, default | |||
elsif type_metadata.type == :text && default | |||
# strip and unescape quotes | |||
default = default[1...-1].gsub("\\'", "'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is gsub
the right operation here? I wonder if this might be costly 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean replace it with gsub!
? text
columns with default are not popular enough, I think, and default values in the db most of the time have small values in them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fatkodima Sorry I was misconnecting the dots here. This code probably isn't being called during a request so maybe we don't have to worry about the performance of gsub
. Please correct me if I'm wrong! 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used in
rails/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
Lines 114 to 119 in 8a7a9d0
def columns(table_name) | |
table_name = table_name.to_s | |
column_definitions(table_name).map do |field| | |
new_column_from_field(table_name, field) | |
end | |
end |
which runs once for each table, when columns list for the first time is needed, and then cached in
SchemaCache
-rails/activerecord/lib/active_record/connection_adapters/schema_cache.rb
Lines 110 to 114 in 8a7a9d0
def columns(table_name) | |
@columns.fetch(table_name) do | |
@columns[deep_deduplicate(table_name)] = deep_deduplicate(connection.columns(table_name)) | |
end | |
end |
So, no need to worry about the performance.
Fix retrieving default value for text column for MariaDB
This was backported to Rails 6.1 in rails/rails#40822 and the tests require it now.
Why Mysql2 not supporting this? Mysql2::Error: BLOB, TEXT, GEOMETRY or JSON column 'my_field_name' can't have a default value |
This error is returned by the MySQL server. You won't get an answer on this thread. |
Fixes #38218
Fixes #41240
All adapters support
text
columns with defaults, except MySQL (but MariaDB >= 10.2.1 supports).