From e6a05b2634d264d5376f45d2c9dfdb5a7be2c4bd Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Sat, 31 Aug 2024 10:17:09 +0100 Subject: [PATCH 1/7] Refactor adapter interface to match abstract adapter --- .../sqlserver/database_statements.rb | 162 ++++++++++++------ 1 file changed, 109 insertions(+), 53 deletions(-) diff --git a/lib/active_record/connection_adapters/sqlserver/database_statements.rb b/lib/active_record/connection_adapters/sqlserver/database_statements.rb index 741acb21e..3ae176e88 100644 --- a/lib/active_record/connection_adapters/sqlserver/database_statements.rb +++ b/lib/active_record/connection_adapters/sqlserver/database_statements.rb @@ -7,55 +7,110 @@ module DatabaseStatements READ_QUERY = ActiveRecord::ConnectionAdapters::AbstractAdapter.build_read_query_regexp(:begin, :commit, :dbcc, :explain, :save, :select, :set, :rollback, :waitfor, :use) # :nodoc: private_constant :READ_QUERY - def write_query?(sql) # :nodoc: - !READ_QUERY.match?(sql) - rescue ArgumentError # Invalid encoding - !READ_QUERY.match?(sql.b) - end + # TODO: replace `internal_exec_query` by `perform_query`. - def raw_execute(sql, name, async: false, allow_retry: false, materialize_transactions: true) - log(sql, name, async: async) do |notification_payload| - with_raw_connection(allow_retry: allow_retry, materialize_transactions: materialize_transactions) do |conn| - result = if id_insert_table_name = query_requires_identity_insert?(sql) - with_identity_insert_enabled(id_insert_table_name, conn) { internal_raw_execute(sql, conn, perform_do: true) } - else - internal_raw_execute(sql, conn, perform_do: true) - end - verified! - notification_payload[:row_count] = result - result - end - end - end + def perform_query(raw_connection, sql, binds, type_casted_binds, prepare:, notification_payload:, batch:) + unless binds.nil? || binds.empty? + types, params = sp_executesql_types_and_parameters(binds) - def internal_exec_query(sql, name = "SQL", binds = [], prepare: false, async: false, allow_retry: false) - sql = transform_query(sql) + # TODO: `name` parameter does not exist. + sql = sp_executesql_sql(sql, types, params) + end - check_if_write_query(sql) - mark_transaction_written_if_write(sql) - unless without_prepared_statement?(binds) - types, params = sp_executesql_types_and_parameters(binds) - sql = sp_executesql_sql(sql, types, params, name) + result = if id_insert_table_name = query_requires_identity_insert?(sql) + with_identity_insert_enabled(id_insert_table_name, raw_connection) do + internal_exec_sql_query(sql, raw_connection) + end + else + internal_exec_sql_query(sql, raw_connection) + end + + verified! + notification_payload[:row_count] = result.count + result + end + + # + # def internal_exec_query(sql, name = "SQL", binds = [], prepare: false, async: false, allow_retry: false) + # sql = transform_query(sql) + # + # check_if_write_query(sql) + # mark_transaction_written_if_write(sql) + # + # unless without_prepared_statement?(binds) + # types, params = sp_executesql_types_and_parameters(binds) + # sql = sp_executesql_sql(sql, types, params, name) + # end + # + # log(sql, name, binds, async: async) do |notification_payload| + # with_raw_connection do |conn| + # result = if id_insert_table_name = query_requires_identity_insert?(sql) + # with_identity_insert_enabled(id_insert_table_name, conn) do + # internal_exec_sql_query(sql, conn) + # end + # else + # internal_exec_sql_query(sql, conn) + # end + # + # verified! + # notification_payload[:row_count] = result.count + # result + # end + # end + # end + + + # Receive a native adapter result object and returns an ActiveRecord::Result object. + def cast_result(raw_result) + if raw_result.columns.empty? + ActiveRecord::Result.empty + else + ActiveRecord::Result.new(raw_result.columns, raw_result.rows) end - log(sql, name, binds, async: async) do |notification_payload| - with_raw_connection do |conn| - result = if id_insert_table_name = query_requires_identity_insert?(sql) - with_identity_insert_enabled(id_insert_table_name, conn) do - internal_exec_sql_query(sql, conn) - end - else - internal_exec_sql_query(sql, conn) - end + # rescue => e + # binding.pry + end - verified! - notification_payload[:row_count] = result.count - result - end + def affected_rows(raw_result) + + if raw_result.count == 1 && raw_result.first.key?('AffectedRows') + raw_result.first['AffectedRows'] + else + raw_result.count end + + rescue => e + binding.pry end + + + def write_query?(sql) # :nodoc: + !READ_QUERY.match?(sql) + rescue ArgumentError # Invalid encoding + !READ_QUERY.match?(sql.b) + end + + # TODO: This method implemented in Rails. + # def raw_execute(sql, name, async: false, allow_retry: false, materialize_transactions: true) + # log(sql, name, async: async) do |notification_payload| + # with_raw_connection(allow_retry: allow_retry, materialize_transactions: materialize_transactions) do |conn| + # result = if id_insert_table_name = query_requires_identity_insert?(sql) + # with_identity_insert_enabled(id_insert_table_name, conn) { internal_raw_execute(sql, conn, perform_do: true) } + # else + # internal_raw_execute(sql, conn, perform_do: true) + # end + # verified! + # notification_payload[:row_count] = result + # result + # end + # end + # end + + + def internal_exec_sql_query(sql, conn) handle = internal_raw_execute(sql, conn) handle_to_names_and_values(handle, ar_result: true) @@ -65,12 +120,12 @@ def internal_exec_sql_query(sql, conn) def exec_delete(sql, name, binds) sql = sql.dup << "; SELECT @@ROWCOUNT AS AffectedRows" - super(sql, name, binds).rows.first.first + super(sql, name, binds) end def exec_update(sql, name, binds) sql = sql.dup << "; SELECT @@ROWCOUNT AS AffectedRows" - super(sql, name, binds).rows.first.first + super(sql, name, binds) end def begin_db_transaction @@ -366,18 +421,19 @@ def basic_attribute_type?(type) type.is_a?(NilClass) end - def sp_executesql_sql(sql, types, params, name) - if name == "EXPLAIN" - params.each.with_index do |param, index| - substitute_at_finder = /(@#{index})(?=(?:[^']|'[^']*')*$)/ # Finds unquoted @n values. - sql = sql.sub substitute_at_finder, param.to_s - end - else + # TODO: `name` was removed from the method signature. + def sp_executesql_sql(sql, types, params) + # if name == "EXPLAIN" + # params.each.with_index do |param, index| + # substitute_at_finder = /(@#{index})(?=(?:[^']|'[^']*')*$)/ # Finds unquoted @n values. + # sql = sql.sub substitute_at_finder, param.to_s + # end + # else types = quote(types.join(", ")) params = params.map.with_index { |p, i| "@#{i} = #{p}" }.join(", ") # Only p is needed, but with @i helps explain regexp. sql = "EXEC sp_executesql #{quote(sql)}" sql += ", #{types}, #{params}" unless params.empty? - end + # end sql.freeze end @@ -455,10 +511,10 @@ def finish_statement_handle(handle) # TinyTDS returns false instead of raising an exception if connection fails. # Getting around this by raising an exception ourselves while PR # https://github.com/rails-sqlserver/tiny_tds/pull/469 is not released. - def internal_raw_execute(sql, conn, perform_do: false) - result = conn.execute(sql).tap do |_result| - raise TinyTds::Error, "failed to execute statement" if _result.is_a?(FalseClass) - end + # TODO: Check if `perform_do` is needed. + def internal_raw_execute(sql, raw_connection, perform_do: false) + result = raw_connection.execute(sql) + raise TinyTds::Error, "failed to execute statement" if result.is_a?(FalseClass) perform_do ? result.do : result end From aedf993fc07593bb64da4eb2965ea68bb1387c82 Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Sat, 31 Aug 2024 20:29:17 +0100 Subject: [PATCH 2/7] Cleanup affected row --- .../sqlserver/database_statements.rb | 22 +++++++++++-------- 1 file changed, 13 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 3ae176e88..6516ab007 100644 --- a/lib/active_record/connection_adapters/sqlserver/database_statements.rb +++ b/lib/active_record/connection_adapters/sqlserver/database_statements.rb @@ -75,14 +75,16 @@ def cast_result(raw_result) def affected_rows(raw_result) - if raw_result.count == 1 && raw_result.first.key?('AffectedRows') - raw_result.first['AffectedRows'] - else - raw_result.count - end + raw_result.first['AffectedRows'] - rescue => e - binding.pry + # if raw_result.count == 1 && raw_result.first.key?('AffectedRows') + # raw_result.first['AffectedRows'] + # else + # raw_result.count + # end + + # rescue => e + # binding.pry end @@ -118,14 +120,16 @@ def internal_exec_sql_query(sql, conn) finish_statement_handle(handle) end - def exec_delete(sql, name, binds) + def exec_delete(sql, name = nil, binds = []) sql = sql.dup << "; SELECT @@ROWCOUNT AS AffectedRows" super(sql, name, binds) + # internal_execute(sql, name, binds).first['AffectedRows'] end - def exec_update(sql, name, binds) + def exec_update(sql, name = nil, binds = []) sql = sql.dup << "; SELECT @@ROWCOUNT AS AffectedRows" super(sql, name, binds) + # internal_execute(sql, name, binds).first['AffectedRows'] end def begin_db_transaction From 5a6bb451adccf377d1360fe17c1c9746806916e2 Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Sat, 31 Aug 2024 20:51:00 +0100 Subject: [PATCH 3/7] Do not wrap EXPLAIN SQL in sp_executesql --- .../sqlserver/database_statements.rb | 46 +++++++++++++------ .../connection_adapters/sqlserver/showplan.rb | 5 +- 2 files changed, 34 insertions(+), 17 deletions(-) diff --git a/lib/active_record/connection_adapters/sqlserver/database_statements.rb b/lib/active_record/connection_adapters/sqlserver/database_statements.rb index 6516ab007..71b1ce450 100644 --- a/lib/active_record/connection_adapters/sqlserver/database_statements.rb +++ b/lib/active_record/connection_adapters/sqlserver/database_statements.rb @@ -10,12 +10,12 @@ module DatabaseStatements # TODO: replace `internal_exec_query` by `perform_query`. def perform_query(raw_connection, sql, binds, type_casted_binds, prepare:, notification_payload:, batch:) - unless binds.nil? || binds.empty? - types, params = sp_executesql_types_and_parameters(binds) - - # TODO: `name` parameter does not exist. - sql = sp_executesql_sql(sql, types, params) - end + # unless binds.nil? || binds.empty? + # types, params = sp_executesql_types_and_parameters(binds) + # + # # TODO: `name` parameter does not exist. + # sql = sp_executesql_sql(sql, types, params) + # end result = if id_insert_table_name = query_requires_identity_insert?(sql) @@ -73,6 +73,8 @@ def cast_result(raw_result) # binding.pry end + + def affected_rows(raw_result) raw_result.first['AffectedRows'] @@ -112,6 +114,19 @@ def write_query?(sql) # :nodoc: # end + def raw_execute(sql, name = nil, binds = [], prepare: false, async: false, allow_retry: false, materialize_transactions: true, batch: false) + + unless binds.nil? || binds.empty? + types, params = sp_executesql_types_and_parameters(binds) + + + sql = sp_executesql_sql(sql, types, params, name) + end + + + super + end + def internal_exec_sql_query(sql, conn) handle = internal_raw_execute(sql, conn) @@ -425,19 +440,20 @@ def basic_attribute_type?(type) type.is_a?(NilClass) end - # TODO: `name` was removed from the method signature. - def sp_executesql_sql(sql, types, params) - # if name == "EXPLAIN" - # params.each.with_index do |param, index| - # substitute_at_finder = /(@#{index})(?=(?:[^']|'[^']*')*$)/ # Finds unquoted @n values. - # sql = sql.sub substitute_at_finder, param.to_s - # end - # else + + def sp_executesql_sql(sql, types, params, name) + if name == "EXPLAIN" + params.each.with_index do |param, index| + substitute_at_finder = /(@#{index})(?=(?:[^']|'[^']*')*$)/ # Finds unquoted @n values. + sql = sql.sub substitute_at_finder, param.to_s + end + else types = quote(types.join(", ")) params = params.map.with_index { |p, i| "@#{i} = #{p}" }.join(", ") # Only p is needed, but with @i helps explain regexp. sql = "EXEC sp_executesql #{quote(sql)}" sql += ", #{types}, #{params}" unless params.empty? - # end + end + sql.freeze end diff --git a/lib/active_record/connection_adapters/sqlserver/showplan.rb b/lib/active_record/connection_adapters/sqlserver/showplan.rb index 734495f8a..8ee07e14d 100644 --- a/lib/active_record/connection_adapters/sqlserver/showplan.rb +++ b/lib/active_record/connection_adapters/sqlserver/showplan.rb @@ -13,9 +13,10 @@ module Showplan OPTIONS = [OPTION_ALL, OPTION_TEXT, OPTION_XML] def explain(arel, binds = [], options = []) - sql = to_sql(arel) - result = with_showplan_on { internal_exec_query(sql, "EXPLAIN", binds) } + sql = to_sql(arel) + result = with_showplan_on { internal_exec_query(sql, "EXPLAIN", binds) } printer = showplan_printer.new(result) + printer.pp end From 5e30f4e98832cbd99d5ac51bfb15b6ad771eb132 Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Sat, 31 Aug 2024 21:10:43 +0100 Subject: [PATCH 4/7] Cleanup --- .../sqlserver/database_statements.rb | 93 +------------------ .../connection_adapters/sqlserver/showplan.rb | 4 +- 2 files changed, 7 insertions(+), 90 deletions(-) diff --git a/lib/active_record/connection_adapters/sqlserver/database_statements.rb b/lib/active_record/connection_adapters/sqlserver/database_statements.rb index 71b1ce450..8af3da584 100644 --- a/lib/active_record/connection_adapters/sqlserver/database_statements.rb +++ b/lib/active_record/connection_adapters/sqlserver/database_statements.rb @@ -7,17 +7,13 @@ module DatabaseStatements READ_QUERY = ActiveRecord::ConnectionAdapters::AbstractAdapter.build_read_query_regexp(:begin, :commit, :dbcc, :explain, :save, :select, :set, :rollback, :waitfor, :use) # :nodoc: private_constant :READ_QUERY - # TODO: replace `internal_exec_query` by `perform_query`. + def write_query?(sql) # :nodoc: + !READ_QUERY.match?(sql) + rescue ArgumentError # Invalid encoding + !READ_QUERY.match?(sql.b) + end def perform_query(raw_connection, sql, binds, type_casted_binds, prepare:, notification_payload:, batch:) - # unless binds.nil? || binds.empty? - # types, params = sp_executesql_types_and_parameters(binds) - # - # # TODO: `name` parameter does not exist. - # sql = sp_executesql_sql(sql, types, params) - # end - - result = if id_insert_table_name = query_requires_identity_insert?(sql) with_identity_insert_enabled(id_insert_table_name, raw_connection) do internal_exec_sql_query(sql, raw_connection) @@ -31,103 +27,27 @@ def perform_query(raw_connection, sql, binds, type_casted_binds, prepare:, notif result end - # - # def internal_exec_query(sql, name = "SQL", binds = [], prepare: false, async: false, allow_retry: false) - # sql = transform_query(sql) - # - # check_if_write_query(sql) - # mark_transaction_written_if_write(sql) - # - # unless without_prepared_statement?(binds) - # types, params = sp_executesql_types_and_parameters(binds) - # sql = sp_executesql_sql(sql, types, params, name) - # end - # - # log(sql, name, binds, async: async) do |notification_payload| - # with_raw_connection do |conn| - # result = if id_insert_table_name = query_requires_identity_insert?(sql) - # with_identity_insert_enabled(id_insert_table_name, conn) do - # internal_exec_sql_query(sql, conn) - # end - # else - # internal_exec_sql_query(sql, conn) - # end - # - # verified! - # notification_payload[:row_count] = result.count - # result - # end - # end - # end - - - # Receive a native adapter result object and returns an ActiveRecord::Result object. def cast_result(raw_result) if raw_result.columns.empty? ActiveRecord::Result.empty else ActiveRecord::Result.new(raw_result.columns, raw_result.rows) end - - # rescue => e - # binding.pry end - - def affected_rows(raw_result) - raw_result.first['AffectedRows'] - - # if raw_result.count == 1 && raw_result.first.key?('AffectedRows') - # raw_result.first['AffectedRows'] - # else - # raw_result.count - # end - - # rescue => e - # binding.pry - end - - - - def write_query?(sql) # :nodoc: - !READ_QUERY.match?(sql) - rescue ArgumentError # Invalid encoding - !READ_QUERY.match?(sql.b) end - # TODO: This method implemented in Rails. - # def raw_execute(sql, name, async: false, allow_retry: false, materialize_transactions: true) - # log(sql, name, async: async) do |notification_payload| - # with_raw_connection(allow_retry: allow_retry, materialize_transactions: materialize_transactions) do |conn| - # result = if id_insert_table_name = query_requires_identity_insert?(sql) - # with_identity_insert_enabled(id_insert_table_name, conn) { internal_raw_execute(sql, conn, perform_do: true) } - # else - # internal_raw_execute(sql, conn, perform_do: true) - # end - # verified! - # notification_payload[:row_count] = result - # result - # end - # end - # end - - def raw_execute(sql, name = nil, binds = [], prepare: false, async: false, allow_retry: false, materialize_transactions: true, batch: false) - unless binds.nil? || binds.empty? types, params = sp_executesql_types_and_parameters(binds) - - sql = sp_executesql_sql(sql, types, params, name) end - super end - def internal_exec_sql_query(sql, conn) handle = internal_raw_execute(sql, conn) handle_to_names_and_values(handle, ar_result: true) @@ -138,13 +58,11 @@ def internal_exec_sql_query(sql, conn) def exec_delete(sql, name = nil, binds = []) sql = sql.dup << "; SELECT @@ROWCOUNT AS AffectedRows" super(sql, name, binds) - # internal_execute(sql, name, binds).first['AffectedRows'] end def exec_update(sql, name = nil, binds = []) sql = sql.dup << "; SELECT @@ROWCOUNT AS AffectedRows" super(sql, name, binds) - # internal_execute(sql, name, binds).first['AffectedRows'] end def begin_db_transaction @@ -531,7 +449,6 @@ def finish_statement_handle(handle) # TinyTDS returns false instead of raising an exception if connection fails. # Getting around this by raising an exception ourselves while PR # https://github.com/rails-sqlserver/tiny_tds/pull/469 is not released. - # TODO: Check if `perform_do` is needed. def internal_raw_execute(sql, raw_connection, perform_do: false) result = raw_connection.execute(sql) raise TinyTds::Error, "failed to execute statement" if result.is_a?(FalseClass) diff --git a/lib/active_record/connection_adapters/sqlserver/showplan.rb b/lib/active_record/connection_adapters/sqlserver/showplan.rb index 8ee07e14d..a93a839f4 100644 --- a/lib/active_record/connection_adapters/sqlserver/showplan.rb +++ b/lib/active_record/connection_adapters/sqlserver/showplan.rb @@ -13,8 +13,8 @@ module Showplan OPTIONS = [OPTION_ALL, OPTION_TEXT, OPTION_XML] def explain(arel, binds = [], options = []) - sql = to_sql(arel) - result = with_showplan_on { internal_exec_query(sql, "EXPLAIN", binds) } + sql = to_sql(arel) + result = with_showplan_on { internal_exec_query(sql, "EXPLAIN", binds) } printer = showplan_printer.new(result) printer.pp From a6b53feaee9e0ec9f0f3d8e75afaa79981558298 Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Sat, 31 Aug 2024 21:20:17 +0100 Subject: [PATCH 5/7] Typo --- .../connection_adapters/sqlserver/database_statements.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/active_record/connection_adapters/sqlserver/database_statements.rb b/lib/active_record/connection_adapters/sqlserver/database_statements.rb index 8af3da584..d3dbd47b6 100644 --- a/lib/active_record/connection_adapters/sqlserver/database_statements.rb +++ b/lib/active_record/connection_adapters/sqlserver/database_statements.rb @@ -357,8 +357,7 @@ def basic_attribute_type?(type) type.is_a?(FalseClass) || type.is_a?(NilClass) end - - + def sp_executesql_sql(sql, types, params, name) if name == "EXPLAIN" params.each.with_index do |param, index| From 570d4e8695582e5778364a0700c61cc26d666815 Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Sat, 31 Aug 2024 21:21:57 +0100 Subject: [PATCH 6/7] Typo --- .../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 d3dbd47b6..38bd54525 100644 --- a/lib/active_record/connection_adapters/sqlserver/database_statements.rb +++ b/lib/active_record/connection_adapters/sqlserver/database_statements.rb @@ -357,7 +357,7 @@ def basic_attribute_type?(type) type.is_a?(FalseClass) || type.is_a?(NilClass) end - + def sp_executesql_sql(sql, types, params, name) if name == "EXPLAIN" params.each.with_index do |param, index| From 2daa98e163727eab514017878d02eee80fe79b2c Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Sat, 31 Aug 2024 21:35:37 +0100 Subject: [PATCH 7/7] Remove coerced test --- CHANGELOG.md | 1 + test/cases/coerced_tests.rb | 8 -------- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99be32efb..e257b0dff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - [#1153](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1153) Only support Ruby v3.1+ - [#1196](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1196) Use default inspect for database adapter +- [#1216](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1216) Refactor adapter interface to match abstract adapter #### Fixed diff --git a/test/cases/coerced_tests.rb b/test/cases/coerced_tests.rb index 44c7d2211..26ff4d15a 100644 --- a/test/cases/coerced_tests.rb +++ b/test/cases/coerced_tests.rb @@ -2250,14 +2250,6 @@ class LogSubscriberTest < ActiveRecord::TestCase def test_verbose_query_logs_coerced original_test_verbose_query_logs end - - # Bindings logged slightly differently. - coerce_tests! :test_where_in_binds_logging_include_attribute_names - def test_where_in_binds_logging_include_attribute_names_coerced - Developer.where(id: [1, 2, 3, 4, 5]).load - wait - assert_match(%{@0 = 1, @1 = 2, @2 = 3, @3 = 4, @4 = 5 [["id", nil], ["id", nil], ["id", nil], ["id", nil], ["id", nil]]}, @logger.logged(:debug).last) - end end class ReloadModelsTest < ActiveRecord::TestCase