From 5d870c929157b7c7949c9356d4f88b97c8848ec3 Mon Sep 17 00:00:00 2001 From: Doug Barth Date: Tue, 5 Nov 2013 10:04:00 -0800 Subject: Don't swallow exceptions in transctional statements The MySQL connection adapater swallows all StandardError exceptions, which includes Mysql::Error and Mysql2::Error. The comment in the exception clause claims errors thrown here indicate that transactions aren't supported by the server but that isn't necessarily true. It's possible the MySQL server has gone away and swallowing a failed commit may let the application return a successful response when the data has not been saved. Also, replication libraries like Galera require that the application handle exceptions thrown at BEGIN/COMMIT. I'm unable to determine what version of MySQL threw an exception for transactional statements. I tried as far back as 3.23.49 with InnoDB disabled but BEGIN & COMMIT statements do not throw an error. If there's a real case for this logic to continue, we could instead push this behavior into a configuration setting. The exception swallowing has been there since the beginning: db045dbbf60b53dbe013ef25554fd013baf88134 --- .../connection_adapters/abstract_mysql_adapter.rb | 8 ------- .../connection_adapters/mysql_adapter.rb | 2 -- .../test/cases/adapters/mysql/connection_test.rb | 26 ++++++++++++++++++++++ .../test/cases/adapters/mysql2/connection_test.rb | 26 ++++++++++++++++++++++ 4 files changed, 52 insertions(+), 10 deletions(-) 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 138ab811dc..9a88be5219 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -314,27 +314,19 @@ module ActiveRecord def begin_db_transaction execute "BEGIN" - rescue - # Transactions aren't supported end def begin_isolated_db_transaction(isolation) execute "SET TRANSACTION ISOLATION LEVEL #{transaction_isolation_levels.fetch(isolation)}" begin_db_transaction - rescue - # Transactions aren't supported end def commit_db_transaction #:nodoc: execute "COMMIT" - rescue - # Transactions aren't supported end def rollback_db_transaction #:nodoc: execute "ROLLBACK" - rescue - # Transactions aren't supported end # In the simple case, MySQL allows us to place JOINs directly into the UPDATE diff --git a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb index 88c9494fc6..d61847506d 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb @@ -468,8 +468,6 @@ module ActiveRecord def begin_db_transaction #:nodoc: exec_query "BEGIN" - rescue Mysql::Error - # Transactions aren't supported end private diff --git a/activerecord/test/cases/adapters/mysql/connection_test.rb b/activerecord/test/cases/adapters/mysql/connection_test.rb index 1844a2e0dc..ece0ee77fb 100644 --- a/activerecord/test/cases/adapters/mysql/connection_test.rb +++ b/activerecord/test/cases/adapters/mysql/connection_test.rb @@ -40,6 +40,11 @@ class MysqlConnectionTest < ActiveRecord::TestCase @connection.update('set @@wait_timeout=1') sleep 2 assert !@connection.active? + + # Repair all fixture connections so other tests won't break. + @fixture_connections.each do |c| + c.verify! + end end def test_successful_reconnection_after_timeout_with_manual_reconnect @@ -159,6 +164,27 @@ class MysqlConnectionTest < ActiveRecord::TestCase end end + def test_mysql_begin_db_transaction_can_throw_an_exception + @connection.expects(:exec_query).with('BEGIN').raises('OH NOES') + assert_raise RuntimeError do + @connection.begin_db_transaction + end + end + + def test_mysql_commit_db_transaction_can_throw_an_exception + @connection.expects(:execute).with('COMMIT').raises('OH NOES') + assert_raise RuntimeError do + @connection.commit_db_transaction + end + end + + def test_mysql_rollback_db_transaction_can_throw_an_exception + @connection.expects(:execute).with('ROLLBACK').raises('OH NOES') + assert_raise RuntimeError do + @connection.rollback_db_transaction + end + end + private def run_without_connection diff --git a/activerecord/test/cases/adapters/mysql2/connection_test.rb b/activerecord/test/cases/adapters/mysql2/connection_test.rb index 679c515e8c..943ca9d552 100644 --- a/activerecord/test/cases/adapters/mysql2/connection_test.rb +++ b/activerecord/test/cases/adapters/mysql2/connection_test.rb @@ -18,6 +18,11 @@ class MysqlConnectionTest < ActiveRecord::TestCase @connection.update('set @@wait_timeout=1') sleep 2 assert !@connection.active? + + # Repair all fixture connections so other tests won't break. + @fixture_connections.each do |c| + c.verify! + end end def test_successful_reconnection_after_timeout_with_manual_reconnect @@ -84,6 +89,27 @@ class MysqlConnectionTest < ActiveRecord::TestCase @connection.execute "DROP TABLE `bar_baz`" end + def test_mysql_begin_db_transaction_can_throw_an_exception + @connection.expects(:execute).with('BEGIN').raises('OH NOES') + assert_raise RuntimeError do + @connection.begin_db_transaction + end + end + + def test_mysql_commit_db_transaction_can_throw_an_exception + @connection.expects(:execute).with('COMMIT').raises('OH NOES') + assert_raise RuntimeError do + @connection.commit_db_transaction + end + end + + def test_mysql_rollback_db_transaction_can_throw_an_exception + @connection.expects(:execute).with('ROLLBACK').raises('OH NOES') + assert_raise RuntimeError do + @connection.rollback_db_transaction + end + end + private def run_without_connection -- cgit v1.2.3 From 2b0406cedb61c4c2f74ecca61fc07771e911fd35 Mon Sep 17 00:00:00 2001 From: Doug Barth Date: Fri, 15 Nov 2013 10:29:30 -0800 Subject: Remove tests for not swallowing exceptions. From PR, @tenderlove would prefer to not maintain these tests. --- .../test/cases/adapters/mysql/connection_test.rb | 21 --------------------- .../test/cases/adapters/mysql2/connection_test.rb | 21 --------------------- 2 files changed, 42 deletions(-) diff --git a/activerecord/test/cases/adapters/mysql/connection_test.rb b/activerecord/test/cases/adapters/mysql/connection_test.rb index ece0ee77fb..7cc71c8929 100644 --- a/activerecord/test/cases/adapters/mysql/connection_test.rb +++ b/activerecord/test/cases/adapters/mysql/connection_test.rb @@ -164,27 +164,6 @@ class MysqlConnectionTest < ActiveRecord::TestCase end end - def test_mysql_begin_db_transaction_can_throw_an_exception - @connection.expects(:exec_query).with('BEGIN').raises('OH NOES') - assert_raise RuntimeError do - @connection.begin_db_transaction - end - end - - def test_mysql_commit_db_transaction_can_throw_an_exception - @connection.expects(:execute).with('COMMIT').raises('OH NOES') - assert_raise RuntimeError do - @connection.commit_db_transaction - end - end - - def test_mysql_rollback_db_transaction_can_throw_an_exception - @connection.expects(:execute).with('ROLLBACK').raises('OH NOES') - assert_raise RuntimeError do - @connection.rollback_db_transaction - end - end - private def run_without_connection diff --git a/activerecord/test/cases/adapters/mysql2/connection_test.rb b/activerecord/test/cases/adapters/mysql2/connection_test.rb index 943ca9d552..8dc1df1851 100644 --- a/activerecord/test/cases/adapters/mysql2/connection_test.rb +++ b/activerecord/test/cases/adapters/mysql2/connection_test.rb @@ -89,27 +89,6 @@ class MysqlConnectionTest < ActiveRecord::TestCase @connection.execute "DROP TABLE `bar_baz`" end - def test_mysql_begin_db_transaction_can_throw_an_exception - @connection.expects(:execute).with('BEGIN').raises('OH NOES') - assert_raise RuntimeError do - @connection.begin_db_transaction - end - end - - def test_mysql_commit_db_transaction_can_throw_an_exception - @connection.expects(:execute).with('COMMIT').raises('OH NOES') - assert_raise RuntimeError do - @connection.commit_db_transaction - end - end - - def test_mysql_rollback_db_transaction_can_throw_an_exception - @connection.expects(:execute).with('ROLLBACK').raises('OH NOES') - assert_raise RuntimeError do - @connection.rollback_db_transaction - end - end - private def run_without_connection -- cgit v1.2.3