From 5228be296ce459c12d12efc5b8d410c6486387be Mon Sep 17 00:00:00 2001 From: Matias Grunberg Date: Wed, 21 Apr 2021 00:59:35 -0300 Subject: [PATCH 01/10] raise ActiveRecord::ConnectionNotEstablished instead of ActiveRecord::StatementInvalid when call execute and connection is disconnected --- .../connection_adapters/sqlserver/database_statements.rb | 6 ++++++ lib/active_record/connection_adapters/sqlserver_adapter.rb | 2 ++ 2 files changed, 8 insertions(+) diff --git a/lib/active_record/connection_adapters/sqlserver/database_statements.rb b/lib/active_record/connection_adapters/sqlserver/database_statements.rb index 194b0acd9..6ad1b81d4 100644 --- a/lib/active_record/connection_adapters/sqlserver/database_statements.rb +++ b/lib/active_record/connection_adapters/sqlserver/database_statements.rb @@ -167,6 +167,8 @@ def execute_procedure(proc_name, *variables) log(sql, name) do case @connection_options[:mode] when :dblib + raise ActiveRecord::ConnectionNotEstablished if @connection.nil? + result = @connection.execute(sql) options = { as: :hash, cache_rows: true, timezone: ActiveRecord::Base.default_timezone || :utc } result.each(options) do |row| @@ -357,6 +359,8 @@ def sp_executesql_sql(sql, types, params, name) def raw_connection_do(sql) case @connection_options[:mode] when :dblib + raise ActiveRecord::ConnectionNotEstablished if @connection.nil? + result = @connection.execute(sql) # TinyTDS returns false instead of raising an exception if connection fails. @@ -428,6 +432,8 @@ def _raw_select(sql, options = {}) def raw_connection_run(sql) case @connection_options[:mode] when :dblib + raise ActiveRecord::ConnectionNotEstablished if @connection.nil? + @connection.execute(sql) end end diff --git a/lib/active_record/connection_adapters/sqlserver_adapter.rb b/lib/active_record/connection_adapters/sqlserver_adapter.rb index 92ecc2248..6cedc2285 100644 --- a/lib/active_record/connection_adapters/sqlserver_adapter.rb +++ b/lib/active_record/connection_adapters/sqlserver_adapter.rb @@ -374,6 +374,8 @@ def initialize_type_map(m = type_map) end def translate_exception(e, message:, sql:, binds:) + return ActiveRecord::ConnectionNotEstablished.new("SQLServer client is not connected") if e.is_a?(ActiveRecord::ConnectionNotEstablished) + case message when /(cannot insert duplicate key .* with unique index) | (violation of unique key constraint)/i RecordNotUnique.new(message, sql: sql, binds: binds) From 5f1f8a33757a8bf8bb7aae284cffaddc3cd2baff Mon Sep 17 00:00:00 2001 From: Matias Grunberg Date: Wed, 21 Apr 2021 01:00:14 -0300 Subject: [PATCH 02/10] add test of other calls to connection.execute that should fail --- test/cases/disconnected_test_sqlserver.rb | 39 +++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 test/cases/disconnected_test_sqlserver.rb diff --git a/test/cases/disconnected_test_sqlserver.rb b/test/cases/disconnected_test_sqlserver.rb new file mode 100644 index 000000000..7529ee864 --- /dev/null +++ b/test/cases/disconnected_test_sqlserver.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require "cases/helper_sqlserver" + +class TestDisconnectedAdapter < ActiveRecord::TestCase + self.use_transactional_tests = false + + def setup + @connection = ActiveRecord::Base.connection + end + + teardown do + return if in_memory_db? + db_config = ActiveRecord::Base.connection_db_config + ActiveRecord::Base.establish_connection(db_config) + end + + test "can't execute procuderes while disconnected" do + @connection.execute_procedure :sp_tables, "sst_datatypes" + @connection.disconnect! + assert_raises(ActiveRecord::ConnectionNotEstablished) do + @connection.execute_procedure :sp_tables, "sst_datatypes" + end + end + + test "can't execute query while disconnected" do + sql = "SELECT count(*) from products WHERE id IN(@0, @1)" + binds = [ + ActiveRecord::Relation::QueryAttribute.new("id", 2, ActiveRecord::Type::BigInteger.new), + ActiveRecord::Relation::QueryAttribute.new("id", 2, ActiveRecord::Type::BigInteger.new) + ] + + @connection.exec_query sql, "TEST", binds + @connection.disconnect! + assert_raises(ActiveRecord::ConnectionNotEstablished) do + @connection.exec_query sql, "TEST", binds + end + end +end From e4eb20303e2dc9c4ce6a45a3831fa0ebef6afad2 Mon Sep 17 00:00:00 2001 From: Matias Grunberg Date: Wed, 21 Apr 2021 01:07:10 -0300 Subject: [PATCH 03/10] add changelog item --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cc54be65a..15feb3678 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ - [#892](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/892) Add support for if_exists on remove_column - [#883](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/885) Fix quoting of ActiveRecord::Relation::QueryAttribute and ActiveModel::Attributes - [#893](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/893) Add Active Record Marshal forward compatibility tests +- [#903](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/903) Raise ActiveRecord::ConnectionNotEstablished on calls to execute with a disconnected connection #### Changed From 9f18dd9925de6421a269530992a509352f921c6f Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Wed, 21 Apr 2021 15:47:39 +0100 Subject: [PATCH 04/10] Refactor --- .../sqlserver/database_statements.rb | 18 +++++++++--------- .../connection_adapters/sqlserver_adapter.rb | 2 +- test/cases/disconnected_test_sqlserver.rb | 4 ++-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/active_record/connection_adapters/sqlserver/database_statements.rb b/lib/active_record/connection_adapters/sqlserver/database_statements.rb index 6ad1b81d4..1ef2dbca0 100644 --- a/lib/active_record/connection_adapters/sqlserver/database_statements.rb +++ b/lib/active_record/connection_adapters/sqlserver/database_statements.rb @@ -167,9 +167,7 @@ def execute_procedure(proc_name, *variables) log(sql, name) do case @connection_options[:mode] when :dblib - raise ActiveRecord::ConnectionNotEstablished if @connection.nil? - - result = @connection.execute(sql) + result = ensure_established_connection! { @connection.execute(sql) } options = { as: :hash, cache_rows: true, timezone: ActiveRecord::Base.default_timezone || :utc } result.each(options) do |row| r = row.with_indifferent_access @@ -359,9 +357,7 @@ def sp_executesql_sql(sql, types, params, name) def raw_connection_do(sql) case @connection_options[:mode] when :dblib - raise ActiveRecord::ConnectionNotEstablished if @connection.nil? - - result = @connection.execute(sql) + result = ensure_established_connection! { @connection.execute(sql) } # TinyTDS returns false instead of raising an exception if connection fails. # Getting around this by raising an exception ourselves while this PR @@ -432,9 +428,7 @@ def _raw_select(sql, options = {}) def raw_connection_run(sql) case @connection_options[:mode] when :dblib - raise ActiveRecord::ConnectionNotEstablished if @connection.nil? - - @connection.execute(sql) + ensure_established_connection! { @connection.execute(sql) } end end @@ -468,6 +462,12 @@ def finish_statement_handle(handle) end handle end + + def ensure_established_connection! + raise ActiveRecord::ConnectionNotEstablished, 'SQL Server client is not connected' unless @connection + + yield + end end end end diff --git a/lib/active_record/connection_adapters/sqlserver_adapter.rb b/lib/active_record/connection_adapters/sqlserver_adapter.rb index 6cedc2285..75988dced 100644 --- a/lib/active_record/connection_adapters/sqlserver_adapter.rb +++ b/lib/active_record/connection_adapters/sqlserver_adapter.rb @@ -374,7 +374,7 @@ def initialize_type_map(m = type_map) end def translate_exception(e, message:, sql:, binds:) - return ActiveRecord::ConnectionNotEstablished.new("SQLServer client is not connected") if e.is_a?(ActiveRecord::ConnectionNotEstablished) + return e if e.is_a?(ActiveRecord::ConnectionNotEstablished) case message when /(cannot insert duplicate key .* with unique index) | (violation of unique key constraint)/i diff --git a/test/cases/disconnected_test_sqlserver.rb b/test/cases/disconnected_test_sqlserver.rb index 7529ee864..207605d4e 100644 --- a/test/cases/disconnected_test_sqlserver.rb +++ b/test/cases/disconnected_test_sqlserver.rb @@ -18,7 +18,7 @@ def setup test "can't execute procuderes while disconnected" do @connection.execute_procedure :sp_tables, "sst_datatypes" @connection.disconnect! - assert_raises(ActiveRecord::ConnectionNotEstablished) do + assert_raises(ActiveRecord::ConnectionNotEstablished, 'SQL Server client is not connected') do @connection.execute_procedure :sp_tables, "sst_datatypes" end end @@ -32,7 +32,7 @@ def setup @connection.exec_query sql, "TEST", binds @connection.disconnect! - assert_raises(ActiveRecord::ConnectionNotEstablished) do + assert_raises(ActiveRecord::ConnectionNotEstablished, 'SQL Server client is not connected') do @connection.exec_query sql, "TEST", binds end end From 9ddca724a4788701bf5546ee5a9ec184cbc893ae Mon Sep 17 00:00:00 2001 From: Matias Grunberg Date: Fri, 23 Apr 2021 10:37:30 -0300 Subject: [PATCH 05/10] fix typo --- test/cases/disconnected_test_sqlserver.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cases/disconnected_test_sqlserver.rb b/test/cases/disconnected_test_sqlserver.rb index 207605d4e..d7ccd33f1 100644 --- a/test/cases/disconnected_test_sqlserver.rb +++ b/test/cases/disconnected_test_sqlserver.rb @@ -15,7 +15,7 @@ def setup ActiveRecord::Base.establish_connection(db_config) end - test "can't execute procuderes while disconnected" do + test "can't execute procedures while disconnected" do @connection.execute_procedure :sp_tables, "sst_datatypes" @connection.disconnect! assert_raises(ActiveRecord::ConnectionNotEstablished, 'SQL Server client is not connected') do From 686176340cea77337ebb2e6579a752bfc3344ef8 Mon Sep 17 00:00:00 2001 From: Matias Grunberg Date: Fri, 23 Apr 2021 10:42:06 -0300 Subject: [PATCH 06/10] check connection active to prevent usage of dead/closed connection --- .../connection_adapters/sqlserver/database_statements.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/active_record/connection_adapters/sqlserver/database_statements.rb b/lib/active_record/connection_adapters/sqlserver/database_statements.rb index 1ef2dbca0..1ba6843af 100644 --- a/lib/active_record/connection_adapters/sqlserver/database_statements.rb +++ b/lib/active_record/connection_adapters/sqlserver/database_statements.rb @@ -464,7 +464,7 @@ def finish_statement_handle(handle) end def ensure_established_connection! - raise ActiveRecord::ConnectionNotEstablished, 'SQL Server client is not connected' unless @connection + raise ActiveRecord::ConnectionNotEstablished, 'SQL Server client is not connected' if @connection.nil? || !@connection.active? yield end From 21a196288444a876f9c1fe9cd89a8993c4197207 Mon Sep 17 00:00:00 2001 From: Matias Grunberg Date: Fri, 23 Apr 2021 11:23:05 -0300 Subject: [PATCH 07/10] raise TinyTds::Error and translate it. 'active?' method is rescuing just those --- .../connection_adapters/sqlserver/database_statements.rb | 2 +- lib/active_record/connection_adapters/sqlserver_adapter.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/active_record/connection_adapters/sqlserver/database_statements.rb b/lib/active_record/connection_adapters/sqlserver/database_statements.rb index 1ba6843af..aecf55ba2 100644 --- a/lib/active_record/connection_adapters/sqlserver/database_statements.rb +++ b/lib/active_record/connection_adapters/sqlserver/database_statements.rb @@ -464,7 +464,7 @@ def finish_statement_handle(handle) end def ensure_established_connection! - raise ActiveRecord::ConnectionNotEstablished, 'SQL Server client is not connected' if @connection.nil? || !@connection.active? + raise TinyTds::Error, 'SQL Server client is not connected' if @connection.nil? || !@connection.active? yield end diff --git a/lib/active_record/connection_adapters/sqlserver_adapter.rb b/lib/active_record/connection_adapters/sqlserver_adapter.rb index 75988dced..9a94f40c9 100644 --- a/lib/active_record/connection_adapters/sqlserver_adapter.rb +++ b/lib/active_record/connection_adapters/sqlserver_adapter.rb @@ -374,9 +374,9 @@ def initialize_type_map(m = type_map) end def translate_exception(e, message:, sql:, binds:) - return e if e.is_a?(ActiveRecord::ConnectionNotEstablished) - case message + when /SQL Server client is not connected/ + ConnectionNotEstablished.new(message) when /(cannot insert duplicate key .* with unique index) | (violation of unique key constraint)/i RecordNotUnique.new(message, sql: sql, binds: binds) when /(conflicted with the foreign key constraint) | (The DELETE statement conflicted with the REFERENCE constraint)/i From 91701a633c1361cfd858cc99c8f73c88b769f4d9 Mon Sep 17 00:00:00 2001 From: Matias Grunberg Date: Mon, 26 Apr 2021 21:20:06 -0300 Subject: [PATCH 08/10] Revert "check connection active to prevent usage of dead/closed connection" This reverts commit 686176340cea77337ebb2e6579a752bfc3344ef8. --- .../connection_adapters/sqlserver/database_statements.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/active_record/connection_adapters/sqlserver/database_statements.rb b/lib/active_record/connection_adapters/sqlserver/database_statements.rb index aecf55ba2..5a18df620 100644 --- a/lib/active_record/connection_adapters/sqlserver/database_statements.rb +++ b/lib/active_record/connection_adapters/sqlserver/database_statements.rb @@ -464,7 +464,7 @@ def finish_statement_handle(handle) end def ensure_established_connection! - raise TinyTds::Error, 'SQL Server client is not connected' if @connection.nil? || !@connection.active? + raise TinyTds::Error, 'SQL Server client is not connected' unless @connection yield end From 470c569d7c6514fee61f91e8a36b91ae42afe1dd Mon Sep 17 00:00:00 2001 From: Matias Grunberg Date: Tue, 27 Apr 2021 11:54:47 -0300 Subject: [PATCH 09/10] check TinyTds FalseClass response in execute_procedure and raw_connection_run --- .../sqlserver/database_statements.rb | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/active_record/connection_adapters/sqlserver/database_statements.rb b/lib/active_record/connection_adapters/sqlserver/database_statements.rb index 5a18df620..fbab539be 100644 --- a/lib/active_record/connection_adapters/sqlserver/database_statements.rb +++ b/lib/active_record/connection_adapters/sqlserver/database_statements.rb @@ -167,7 +167,7 @@ def execute_procedure(proc_name, *variables) log(sql, name) do case @connection_options[:mode] when :dblib - result = ensure_established_connection! { @connection.execute(sql) } + result = ensure_established_connection! { dblib_execute(sql) } options = { as: :hash, cache_rows: true, timezone: ActiveRecord::Base.default_timezone || :utc } result.each(options) do |row| r = row.with_indifferent_access @@ -357,13 +357,7 @@ def sp_executesql_sql(sql, types, params, name) def raw_connection_do(sql) case @connection_options[:mode] when :dblib - result = ensure_established_connection! { @connection.execute(sql) } - - # TinyTDS returns false instead of raising an exception if connection fails. - # Getting around this by raising an exception ourselves while this PR - # https://github.com/rails-sqlserver/tiny_tds/pull/469 is not released. - raise TinyTds::Error, "failed to execute statement" if result.is_a?(FalseClass) - + result = ensure_established_connection! { dblib_execute(sql) } result.do end ensure @@ -428,7 +422,7 @@ def _raw_select(sql, options = {}) def raw_connection_run(sql) case @connection_options[:mode] when :dblib - ensure_established_connection! { @connection.execute(sql) } + ensure_established_connection! { dblib_execute(sql) } end end @@ -463,6 +457,15 @@ def finish_statement_handle(handle) handle end + def dblib_execute(sql) + @connection.execute(sql).tap do |result| + # TinyTDS returns false instead of raising an exception if connection fails. + # Getting around this by raising an exception ourselves while this PR + # https://github.com/rails-sqlserver/tiny_tds/pull/469 is not released. + raise TinyTds::Error, "failed to execute statement" if result.is_a?(FalseClass) + end + end + def ensure_established_connection! raise TinyTds::Error, 'SQL Server client is not connected' unless @connection From 6b73a4ec697b02cdf314769d79f996341afc6ba0 Mon Sep 17 00:00:00 2001 From: Matias Grunberg Date: Tue, 27 Apr 2021 11:57:02 -0300 Subject: [PATCH 10/10] raise ConnectionNotEstabilished when TinyTDS returns false --- lib/active_record/connection_adapters/sqlserver_adapter.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/active_record/connection_adapters/sqlserver_adapter.rb b/lib/active_record/connection_adapters/sqlserver_adapter.rb index 9a94f40c9..e9e9e6324 100644 --- a/lib/active_record/connection_adapters/sqlserver_adapter.rb +++ b/lib/active_record/connection_adapters/sqlserver_adapter.rb @@ -375,7 +375,7 @@ def initialize_type_map(m = type_map) def translate_exception(e, message:, sql:, binds:) case message - when /SQL Server client is not connected/ + when /(SQL Server client is not connected)|(failed to execute statement)/i ConnectionNotEstablished.new(message) when /(cannot insert duplicate key .* with unique index) | (violation of unique key constraint)/i RecordNotUnique.new(message, sql: sql, binds: binds)