From ad0bde58d4e5dd3686fdbdc21c7e4b6d71e371e4 Mon Sep 17 00:00:00 2001 From: Dennis Taylor Date: Wed, 5 Jul 2017 15:21:25 -0700 Subject: Don't translate non-database exceptions. The AbstractAdapter will translate all StandardErrors generated during the course of a query into ActiveRecord::StatementInvalids. Unfortunately, it'll also mangle non-database-related errors generated in ActiveSupport::Notification callbacks after the query has successfully completed. This should prevent it from translating errors from ActiveSupport::Notifications. --- activerecord/CHANGELOG.md | 5 ++++ .../connection_adapters/abstract_adapter.rb | 6 +++-- activerecord/test/cases/adapter_test.rb | 30 ++++++++++++++++++++++ 3 files changed, 39 insertions(+), 2 deletions(-) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 8d900d9669..b5457e389b 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,8 @@ +* Prevent AbstractAdapter from converting exceptions from ActiveSupport::Notification + callbacks into ActiveRecord::StatementInvalids. + + *Dennis Taylor* + * Fix eager loading/preloading association with scope including joins. Fixes #28324. diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index cfe1892d78..30b29e7007 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -576,12 +576,14 @@ module ActiveRecord type_casted_binds: type_casted_binds, statement_name: statement_name, connection_id: object_id) do + begin @lock.synchronize do yield end + rescue => e + raise translate_exception_class(e, sql) end - rescue => e - raise translate_exception_class(e, sql) + end end def translate_exception(exception, message) diff --git a/activerecord/test/cases/adapter_test.rb b/activerecord/test/cases/adapter_test.rb index a1fb6427f9..6d211d5768 100644 --- a/activerecord/test/cases/adapter_test.rb +++ b/activerecord/test/cases/adapter_test.rb @@ -211,6 +211,28 @@ module ActiveRecord end end + def test_exceptions_from_notifications_are_not_translated + original_error = RuntimeError.new("This RuntimeError shouldn't get translated") + subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") { raise original_error } + actual_error = assert_raises(RuntimeError) do + @connection.execute("SELECT * FROM posts") + end + + assert_equal original_error, actual_error + + ensure + ActiveSupport::Notifications.unsubscribe(subscriber) if subscriber + end + + def test_other_exceptions_are_translated_to_statement_invalid + error = assert_raises(ActiveRecord::StatementInvalid) do + @connection.execute("This is a syntax error") + end + + assert_instance_of ActiveRecord::StatementInvalid, error + assert_instance_of syntax_error_exception_class, error.cause + end + def test_select_all_always_return_activerecord_result result = @connection.select_all "SELECT * FROM posts" assert result.is_a?(ActiveRecord::Result) @@ -261,6 +283,14 @@ module ActiveRecord assert_not_nil error.message end end + + private + + def syntax_error_exception_class + return Mysql2::Error if defined?(Mysql2) + return PG::SyntaxError if defined?(PG) + return SQLite3::SQLException if defined?(SQLite3) + end end class AdapterForeignKeyTest < ActiveRecord::TestCase -- cgit v1.2.3 From 06e89c922c770f81c98c68eed9a4bfd96e6770c7 Mon Sep 17 00:00:00 2001 From: Dennis Taylor Date: Wed, 5 Jul 2017 17:13:37 -0700 Subject: Fix indentation style for private method. --- activerecord/test/cases/adapter_test.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'activerecord') diff --git a/activerecord/test/cases/adapter_test.rb b/activerecord/test/cases/adapter_test.rb index 6d211d5768..2236039ff7 100644 --- a/activerecord/test/cases/adapter_test.rb +++ b/activerecord/test/cases/adapter_test.rb @@ -286,11 +286,11 @@ module ActiveRecord private - def syntax_error_exception_class - return Mysql2::Error if defined?(Mysql2) - return PG::SyntaxError if defined?(PG) - return SQLite3::SQLException if defined?(SQLite3) - end + def syntax_error_exception_class + return Mysql2::Error if defined?(Mysql2) + return PG::SyntaxError if defined?(PG) + return SQLite3::SQLException if defined?(SQLite3) + end end class AdapterForeignKeyTest < ActiveRecord::TestCase -- cgit v1.2.3 From 6b7347b124064bbbaf3a20eadee7fbec977eed4c Mon Sep 17 00:00:00 2001 From: Dennis Taylor Date: Thu, 6 Jul 2017 10:33:36 -0700 Subject: Use StandardError instead of RuntimeError. Whoops. --- activerecord/test/cases/adapter_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'activerecord') diff --git a/activerecord/test/cases/adapter_test.rb b/activerecord/test/cases/adapter_test.rb index 2236039ff7..3827e87d35 100644 --- a/activerecord/test/cases/adapter_test.rb +++ b/activerecord/test/cases/adapter_test.rb @@ -212,9 +212,9 @@ module ActiveRecord end def test_exceptions_from_notifications_are_not_translated - original_error = RuntimeError.new("This RuntimeError shouldn't get translated") + original_error = StandardError.new("This StandardError shouldn't get translated") subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") { raise original_error } - actual_error = assert_raises(RuntimeError) do + actual_error = assert_raises(StandardError) do @connection.execute("SELECT * FROM posts") end -- cgit v1.2.3 From e45226dd899ebb39d4ae0fae1dda4a2a507eaddd Mon Sep 17 00:00:00 2001 From: Dennis Taylor Date: Fri, 7 Jul 2017 11:27:46 -0700 Subject: Fix changelog wording as suggested. --- activerecord/CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index b5457e389b..53b9752867 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,5 +1,5 @@ -* Prevent AbstractAdapter from converting exceptions from ActiveSupport::Notification - callbacks into ActiveRecord::StatementInvalids. +* Prevent errors raised by sql.active_record notification subscribers from being converted into + ActiveRecord::StatementInvalid exceptions. *Dennis Taylor* -- cgit v1.2.3 From 79c8fa7c643831d0f731937d1b7e082df2e82885 Mon Sep 17 00:00:00 2001 From: Dennis Taylor Date: Fri, 7 Jul 2017 11:28:17 -0700 Subject: Rename the StatementInvalid test method for clarity. --- activerecord/test/cases/adapter_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/test/cases/adapter_test.rb b/activerecord/test/cases/adapter_test.rb index 3827e87d35..5ec15aff06 100644 --- a/activerecord/test/cases/adapter_test.rb +++ b/activerecord/test/cases/adapter_test.rb @@ -224,7 +224,7 @@ module ActiveRecord ActiveSupport::Notifications.unsubscribe(subscriber) if subscriber end - def test_other_exceptions_are_translated_to_statement_invalid + def test_database_related_exceptions_are_translated_to_statement_invalid error = assert_raises(ActiveRecord::StatementInvalid) do @connection.execute("This is a syntax error") end -- cgit v1.2.3 From 8219e177633e3c3a77a50c2342b8fe6c4956914b Mon Sep 17 00:00:00 2001 From: Dennis Taylor Date: Fri, 7 Jul 2017 11:28:47 -0700 Subject: Remove driver-specific hard-coding in the tests. --- activerecord/test/cases/adapter_test.rb | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) (limited to 'activerecord') diff --git a/activerecord/test/cases/adapter_test.rb b/activerecord/test/cases/adapter_test.rb index 5ec15aff06..827bcba121 100644 --- a/activerecord/test/cases/adapter_test.rb +++ b/activerecord/test/cases/adapter_test.rb @@ -230,7 +230,7 @@ module ActiveRecord end assert_instance_of ActiveRecord::StatementInvalid, error - assert_instance_of syntax_error_exception_class, error.cause + assert_kind_of Exception, error.cause end def test_select_all_always_return_activerecord_result @@ -283,14 +283,6 @@ module ActiveRecord assert_not_nil error.message end end - - private - - def syntax_error_exception_class - return Mysql2::Error if defined?(Mysql2) - return PG::SyntaxError if defined?(PG) - return SQLite3::SQLException if defined?(SQLite3) - end end class AdapterForeignKeyTest < ActiveRecord::TestCase -- cgit v1.2.3