Merge pull request #24167 from kamipo/append_sql_mode_instead_of_overwriting_in_strict_mode

Append sql_mode instead of overwriting in strict mode

3 files changed, 44 insertions, 28 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index e315a560c0..3cfebdc2d0 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,10 @@
+*   MySQL: strict mode respects other SQL modes rather than overwriting them.
+    Setting `strict: true` adds `STRICT_ALL_TABLES` to `sql_mode`. Setting
+    `strict: false` removes `STRICT_TRANS_TABLES`, `STRICT_ALL_TABLES`, and
+    `TRADITIONAL` from `sql_mode`.
+
+    *Ryuta Kamizono*
+
 *   Execute default_scope defined by abstract class in the context of subclass.
 
     Fixes #23413.
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 22b71fbaba..0e22062913 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -846,9 +846,18 @@ module ActiveRecord
         # Make MySQL reject illegal values rather than truncating or blanking them, see
         # http://dev.mysql.com/doc/refman/5.7/en/sql-mode.html#sqlmode_strict_all_tables
         # If the user has provided another value for sql_mode, don't replace it.
-        unless variables.has_key?('sql_mode') || defaults.include?(@config[:strict])
-          variables['sql_mode'] = strict_mode? ? 'STRICT_ALL_TABLES' : ''
+        if sql_mode = variables.delete('sql_mode')
+          sql_mode = quote(sql_mode)
+        elsif !defaults.include?(strict_mode?)
+          if strict_mode?
+            sql_mode = "CONCAT(@@sql_mode, ',STRICT_ALL_TABLES')"
+          else
+            sql_mode = "REPLACE(@@sql_mode, 'STRICT_TRANS_TABLES', '')"
+            sql_mode = "REPLACE(#{sql_mode}, 'STRICT_ALL_TABLES', '')"
+            sql_mode = "REPLACE(#{sql_mode}, 'TRADITIONAL', '')"
+          end
         end
+        sql_mode_assignment = "@@SESSION.sql_mode = #{sql_mode}, " if sql_mode
 
         # NAMES does not have an equals sign, see
         # http://dev.mysql.com/doc/refman/5.7/en/set-statement.html#id944430
@@ -870,7 +879,7 @@ module ActiveRecord
         end.compact.join(', ')
 
         # ...and send them all in one query
-        @connection.query  "SET #{encoding} #{variable_assignments}"
+        @connection.query  "SET #{encoding} #{sql_mode_assignment} #{variable_assignments}"
       end
 
       def column_definitions(table_name) # :nodoc:
diff --git a/activerecord/test/cases/adapters/mysql2/connection_test.rb b/activerecord/test/cases/adapters/mysql2/connection_test.rb
index 575138eb2a..c4715393b3 100644
--- a/activerecord/test/cases/adapters/mysql2/connection_test.rb
+++ b/activerecord/test/cases/adapters/mysql2/connection_test.rb
@@ -69,40 +69,48 @@ class Mysql2ConnectionTest < ActiveRecord::Mysql2TestCase
   end
 
   def test_mysql_default_in_strict_mode
-    result = @connection.exec_query "SELECT @@SESSION.sql_mode"
-    assert_equal [["STRICT_ALL_TABLES"]], result.rows
+    result = @connection.select_value("SELECT @@SESSION.sql_mode")
+    assert_match %r(STRICT_ALL_TABLES), result
   end
 
   def test_mysql_strict_mode_disabled
     run_without_connection do |orig_connection|
-      ActiveRecord::Base.establish_connection(orig_connection.merge({:strict => false}))
-      result = ActiveRecord::Base.connection.exec_query "SELECT @@SESSION.sql_mode"
-      assert_equal [['']], result.rows
+      ActiveRecord::Base.establish_connection(orig_connection.merge(strict: false))
+      result = ActiveRecord::Base.connection.select_value("SELECT @@SESSION.sql_mode")
+      assert_no_match %r(STRICT_ALL_TABLES), result
+    end
+  end
+
+  def test_mysql_strict_mode_specified_default
+    run_without_connection do |orig_connection|
+      ActiveRecord::Base.establish_connection(orig_connection.merge(strict: :default))
+      global_sql_mode = ActiveRecord::Base.connection.select_value("SELECT @@GLOBAL.sql_mode")
+      session_sql_mode = ActiveRecord::Base.connection.select_value("SELECT @@SESSION.sql_mode")
+      assert_equal global_sql_mode, session_sql_mode
+    end
+  end
+
+  def test_mysql_sql_mode_variable_overrides_strict_mode
+    run_without_connection do |orig_connection|
+      ActiveRecord::Base.establish_connection(orig_connection.deep_merge(variables: { 'sql_mode' => 'ansi' }))
+      result = ActiveRecord::Base.connection.select_value('SELECT @@SESSION.sql_mode')
+      assert_no_match %r(STRICT_ALL_TABLES), result
     end
   end
 
   def test_passing_arbitary_flags_to_adapter
-    run_without_connection do |orig_connection|             
+    run_without_connection do |orig_connection|
       ActiveRecord::Base.establish_connection(orig_connection.merge({flags: Mysql2::Client::COMPRESS}))
       assert_equal (Mysql2::Client::COMPRESS |  Mysql2::Client::FOUND_ROWS), ActiveRecord::Base.connection.raw_connection.query_options[:flags]
     end
   end
-  
+
   def test_passing_flags_by_array_to_adapter
-    run_without_connection do |orig_connection|             
+    run_without_connection do |orig_connection|
       ActiveRecord::Base.establish_connection(orig_connection.merge({flags: ['COMPRESS'] }))
       assert_equal ["COMPRESS", "FOUND_ROWS"], ActiveRecord::Base.connection.raw_connection.query_options[:flags]
     end
   end
-  
-  def test_mysql_strict_mode_specified_default
-    run_without_connection do |orig_connection|
-      ActiveRecord::Base.establish_connection(orig_connection.merge({strict: :default}))
-      global_sql_mode = ActiveRecord::Base.connection.exec_query "SELECT @@GLOBAL.sql_mode"
-      session_sql_mode = ActiveRecord::Base.connection.exec_query "SELECT @@SESSION.sql_mode"
-      assert_equal global_sql_mode.rows, session_sql_mode.rows
-    end
-  end
 
   def test_mysql_set_session_variable
     run_without_connection do |orig_connection|
@@ -112,14 +120,6 @@ class Mysql2ConnectionTest < ActiveRecord::Mysql2TestCase
     end
   end
 
-  def test_mysql_sql_mode_variable_overrides_strict_mode
-    run_without_connection do |orig_connection|
-      ActiveRecord::Base.establish_connection(orig_connection.deep_merge(variables: { 'sql_mode' => 'ansi' }))
-      result = ActiveRecord::Base.connection.exec_query 'SELECT @@SESSION.sql_mode'
-      assert_not_equal [['STRICT_ALL_TABLES']], result.rows
-    end
-  end
-
   def test_mysql_set_session_variable_to_default
     run_without_connection do |orig_connection|
       ActiveRecord::Base.establish_connection(orig_connection.deep_merge({:variables => {:default_week_format => :default}}))