From 9931ab68222196d04efae606afad050f15e1761b Mon Sep 17 00:00:00 2001 From: Matt Taylor Date: Mon, 27 Oct 2025 11:24:04 -0500 Subject: [PATCH 1/4] Apply parameterization to sql query after it has been logged by ActiveRecord Prevents sensitive values from by-passing ActiveRecord filtered attributes. --- .../sqlserver/database_statements.rb | 10 +- test/cases/coerced_tests.rb | 95 ++++++++----------- test/cases/optimizer_hints_test_sqlserver.rb | 2 +- test/cases/showplan_test_sqlserver.rb | 4 +- test/cases/specific_schema_test_sqlserver.rb | 13 +-- test/support/query_assertions.rb | 22 +++++ 6 files changed, 77 insertions(+), 69 deletions(-) diff --git a/lib/active_record/connection_adapters/sqlserver/database_statements.rb b/lib/active_record/connection_adapters/sqlserver/database_statements.rb index 32a0791fc..a53f90e11 100644 --- a/lib/active_record/connection_adapters/sqlserver/database_statements.rb +++ b/lib/active_record/connection_adapters/sqlserver/database_statements.rb @@ -14,6 +14,11 @@ def write_query?(sql) # :nodoc: 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) + sql = sp_executesql_sql(sql, types, params, notification_payload[:name]) + 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) @@ -41,11 +46,6 @@ def affected_rows(raw_result) 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 diff --git a/test/cases/coerced_tests.rb b/test/cases/coerced_tests.rb index ba07f96c0..fef2a2e6d 100644 --- a/test/cases/coerced_tests.rb +++ b/test/cases/coerced_tests.rb @@ -248,7 +248,7 @@ def test_belongs_to_with_primary_key_joins_on_correct_column_coerced def test_belongs_to_coerced client = Client.find(3) first_firm = companies(:first_firm) - assert_queries_match(/FETCH NEXT @(\d) ROWS ONLY(.)*@\1 = 1/) do + assert_queries_and_values_match(/FETCH NEXT @3 ROWS ONLY/, ['Firm', 'Agency', 1, 1]) do assert_equal first_firm, client.firm assert_equal first_firm.name, client.firm.name end @@ -257,21 +257,6 @@ def test_belongs_to_coerced module ActiveRecord class BindParameterTest < ActiveRecord::TestCase - # Same as original coerced test except log is found using `EXEC sp_executesql` wrapper. - coerce_tests! :test_binds_are_logged - def test_binds_are_logged_coerced - sub = Arel::Nodes::BindParam.new(1) - binds = [Relation::QueryAttribute.new("id", 1, Type::Value.new)] - sql = "select * from topics where id = #{sub.to_sql}" - - @connection.exec_query(sql, "SQL", binds) - - logged_sql = "EXEC sp_executesql N'#{sql}', N'#{sub.to_sql} int', #{sub.to_sql} = 1" - message = @subscriber.calls.find { |args| args[4][:sql] == logged_sql } - - assert_equal binds, message[4][:binds] - end - # SQL Server adapter does not use a statement cache as query plans are already reused using `EXEC sp_executesql`. coerce_tests! :test_statement_cache coerce_tests! :test_statement_cache_with_query_cache @@ -307,7 +292,7 @@ def assert_bind_params_to_sql_coerced(prepared:) # SELECT [authors].* FROM [authors] WHERE ([authors].[id] IN (1, 2, 3) OR [authors].[id] IS NULL) # sql_unprepared = "SELECT #{table}.* FROM #{table} WHERE (#{pk} IN (#{bind_params(1..3)}) OR #{pk} IS NULL)" - sql_prepared = "EXEC sp_executesql N'SELECT #{table}.* FROM #{table} WHERE (#{pk} IN (#{bind_params(1..3)}) OR #{pk} IS NULL)', N'@0 bigint, @1 bigint, @2 bigint', @0 = 1, @1 = 2, @2 = 3" + sql_prepared = "SELECT #{table}.* FROM #{table} WHERE (#{pk} IN (#{bind_params(1..3)}) OR #{pk} IS NULL)" authors = Author.where(id: [1, 2, 3, nil]) assert_equal sql_unprepared, @connection.to_sql(authors.arel) @@ -322,7 +307,7 @@ def assert_bind_params_to_sql_coerced(prepared:) # SELECT [authors].* FROM [authors] WHERE [authors].[id] IN (1, 2, 3) # sql_unprepared = "SELECT #{table}.* FROM #{table} WHERE #{pk} IN (#{bind_params(1..3)})" - sql_prepared = "EXEC sp_executesql N'SELECT #{table}.* FROM #{table} WHERE #{pk} IN (#{bind_params(1..3)})', N'@0 bigint, @1 bigint, @2 bigint', @0 = 1, @1 = 2, @2 = 3" + sql_prepared = "SELECT #{table}.* FROM #{table} WHERE #{pk} IN (#{bind_params(1..3)})" authors = Author.where(id: [1, 2, 3, 9223372036854775808]) assert_equal sql_unprepared, @connection.to_sql(authors.arel) @@ -391,7 +376,7 @@ def test_should_count_with_group_by_qualified_name_on_loaded_coerced assert_predicate accounts, :loaded? assert_equal expected, accounts.count(:id) end - + # Fix randomly failing test. The loading of the model's schema was affecting the test. coerce_tests! :test_offset_is_kept def test_offset_is_kept_coerced @@ -512,7 +497,7 @@ def test_select_avg_with_joins_and_group_by_as_virtual_attribute_with_ar_coerced def test_limit_is_kept_coerced queries = capture_sql { Account.limit(1).count } assert_equal 1, queries.length - assert_match(/ORDER BY \[accounts\]\.\[id\] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY.*@0 = 1/, queries.first) + assert_match(/ORDER BY \[accounts\]\.\[id\] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY/, queries.first) end # Match SQL Server limit implementation @@ -520,7 +505,7 @@ def test_limit_is_kept_coerced def test_limit_with_offset_is_kept_coerced queries = capture_sql { Account.limit(1).offset(1).count } assert_equal 1, queries.length - assert_match(/ORDER BY \[accounts\]\.\[id\] ASC OFFSET @0 ROWS FETCH NEXT @1 ROWS ONLY.*@0 = 1, @1 = 1/, queries.first) + assert_match(/ORDER BY \[accounts\]\.\[id\] ASC OFFSET @0 ROWS FETCH NEXT @1 ROWS ONLY/, queries.first) end # SQL Server needs an alias for the calculated column @@ -987,9 +972,9 @@ class FinderTest < ActiveRecord::TestCase # Assert SQL Server limit implementation coerce_tests! :test_take_and_first_and_last_with_integer_should_use_sql_limit def test_take_and_first_and_last_with_integer_should_use_sql_limit_coerced - assert_queries_match(/OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY.* @0 = 3/) { Topic.take(3).entries } - assert_queries_match(/OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY.* @0 = 2/) { Topic.first(2).entries } - assert_queries_match(/OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY.* @0 = 5/) { Topic.last(5).entries } + assert_queries_and_values_match(/OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY/, [3]) { Topic.take(3).entries } + assert_queries_and_values_match(/OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY/, [2]) { Topic.first(2).entries } + assert_queries_and_values_match(/OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY/, [5]) { Topic.last(5).entries } end # This fails only when run in the full test suite task. Just taking it out of the mix. @@ -1020,7 +1005,7 @@ def test_condition_local_time_interpolation_with_default_timezone_utc_coerced # Check for `FETCH NEXT x ROWS` rather then `LIMIT`. coerce_tests! :test_include_on_unloaded_relation_with_match def test_include_on_unloaded_relation_with_match_coerced - assert_queries_match(/1 AS one.*FETCH NEXT @2 ROWS ONLY.*@2 = 1/) do + assert_queries_match(/1 AS one.*FETCH NEXT @2 ROWS ONLY/) do assert_equal true, Customer.where(name: "David").include?(customers(:david)) end end @@ -1028,7 +1013,7 @@ def test_include_on_unloaded_relation_with_match_coerced # Check for `FETCH NEXT x ROWS` rather then `LIMIT`. coerce_tests! :test_include_on_unloaded_relation_without_match def test_include_on_unloaded_relation_without_match_coerced - assert_queries_match(/1 AS one.*FETCH NEXT @2 ROWS ONLY.*@2 = 1/) do + assert_queries_match(/1 AS one.*FETCH NEXT @2 ROWS ONLY/) do assert_equal false, Customer.where(name: "David").include?(customers(:mary)) end end @@ -1036,7 +1021,7 @@ def test_include_on_unloaded_relation_without_match_coerced # Check for `FETCH NEXT x ROWS` rather then `LIMIT`. coerce_tests! :test_member_on_unloaded_relation_with_match def test_member_on_unloaded_relation_with_match_coerced - assert_queries_match(/1 AS one.*FETCH NEXT @2 ROWS ONLY.*@2 = 1/) do + assert_queries_match(/1 AS one.*FETCH NEXT @2 ROWS ONLY/) do assert_equal true, Customer.where(name: "David").member?(customers(:david)) end end @@ -1044,7 +1029,7 @@ def test_member_on_unloaded_relation_with_match_coerced # Check for `FETCH NEXT x ROWS` rather then `LIMIT`. coerce_tests! :test_member_on_unloaded_relation_without_match def test_member_on_unloaded_relation_without_match_coerced - assert_queries_match(/1 AS one.*FETCH NEXT @2 ROWS ONLY.*@2 = 1/) do + assert_queries_match(/1 AS one.*FETCH NEXT @2 ROWS ONLY/) do assert_equal false, Customer.where(name: "David").member?(customers(:mary)) end end @@ -1059,7 +1044,7 @@ def test_implicit_order_column_is_configurable_coerced assert_equal topics(:third), Topic.last c = Topic.lease_connection - assert_queries_match(/ORDER BY #{Regexp.escape(c.quote_table_name("topics.title"))} DESC, #{Regexp.escape(c.quote_table_name("topics.id"))} DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY.*@0 = 1/i) { + assert_queries_and_values_match(/ORDER BY #{Regexp.escape(c.quote_table_name("topics.title"))} DESC, #{Regexp.escape(c.quote_table_name("topics.id"))} DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY/i, [1]) { Topic.last } ensure @@ -1073,7 +1058,7 @@ def test_implicit_order_set_to_primary_key_coerced Topic.implicit_order_column = "id" c = Topic.lease_connection - assert_queries_match(/ORDER BY #{Regexp.escape(c.quote_table_name("topics.id"))} DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY.*@0 = 1/i) { + assert_queries_and_values_match(/ORDER BY #{Regexp.escape(c.quote_table_name("topics.id"))} DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY/i, [1]) { Topic.last } ensure @@ -1088,7 +1073,7 @@ def test_implicit_order_for_model_without_primary_key_coerced c = NonPrimaryKey.lease_connection - assert_queries_match(/ORDER BY #{Regexp.escape(c.quote_table_name("non_primary_keys.created_at"))} DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY.*@0 = 1/i) { + assert_queries_and_values_match(/ORDER BY #{Regexp.escape(c.quote_table_name("non_primary_keys.created_at"))} DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY/i, [1]) { NonPrimaryKey.last } ensure @@ -1098,7 +1083,7 @@ def test_implicit_order_for_model_without_primary_key_coerced # Check for `FETCH NEXT x ROWS` rather then `LIMIT`. coerce_tests! :test_member_on_unloaded_relation_with_composite_primary_key def test_member_on_unloaded_relation_with_composite_primary_key_coerced - assert_queries_match(/1 AS one.* FETCH NEXT @(\d) ROWS ONLY.*@\1 = 1/) do + assert_queries_match(/1 AS one.* FETCH NEXT @3 ROWS ONLY/) do book = cpk_books(:cpk_great_author_first_book) assert Cpk::Book.where(title: "The first book").member?(book) end @@ -1113,7 +1098,7 @@ def test_implicit_order_column_prepends_query_constraints_coerced quoted_color = Regexp.escape(c.quote_table_name("clothing_items.color")) quoted_descrption = Regexp.escape(c.quote_table_name("clothing_items.description")) - assert_queries_match(/ORDER BY #{quoted_descrption} ASC, #{quoted_type} ASC, #{quoted_color} ASC OFFSET 0 ROWS FETCH NEXT @(\d) ROWS ONLY.*@\1 = 1/i) do + assert_queries_match(/ORDER BY #{quoted_descrption} ASC, #{quoted_type} ASC, #{quoted_color} ASC OFFSET 0 ROWS FETCH NEXT @(\d) ROWS ONLY/i) do assert_kind_of ClothingItem, ClothingItem.first end ensure @@ -1127,7 +1112,7 @@ def test_implicit_order_column_prepends_query_constraints_coerced quoted_type = Regexp.escape(c.quote_table_name("clothing_items.clothing_type")) quoted_color = Regexp.escape(c.quote_table_name("clothing_items.color")) - assert_queries_match(/ORDER BY #{quoted_type} DESC, #{quoted_color} DESC OFFSET 0 ROWS FETCH NEXT @(\d) ROWS ONLY.*@\1 = 1/i) do + assert_queries_match(/ORDER BY #{quoted_type} DESC, #{quoted_color} DESC OFFSET 0 ROWS FETCH NEXT @(\d) ROWS ONLY/i) do assert_kind_of ClothingItem, ClothingItem.last end end @@ -1139,7 +1124,7 @@ def test_implicit_order_column_prepends_query_constraints_coerced quoted_type = Regexp.escape(c.quote_table_name("clothing_items.clothing_type")) quoted_color = Regexp.escape(c.quote_table_name("clothing_items.color")) - assert_queries_match(/ORDER BY #{quoted_type} ASC, #{quoted_color} ASC OFFSET 0 ROWS FETCH NEXT @(\d) ROWS ONLY.*@\1 = 1/i) do + assert_queries_match(/ORDER BY #{quoted_type} ASC, #{quoted_color} ASC OFFSET 0 ROWS FETCH NEXT @(\d) ROWS ONLY/i) do assert_kind_of ClothingItem, ClothingItem.first end end @@ -1152,7 +1137,7 @@ def test_implicit_order_column_reorders_query_constraints_coerced quoted_type = Regexp.escape(c.quote_table_name("clothing_items.clothing_type")) quoted_color = Regexp.escape(c.quote_table_name("clothing_items.color")) - assert_queries_match(/ORDER BY #{quoted_color} ASC, #{quoted_type} ASC OFFSET 0 ROWS FETCH NEXT @(\d) ROWS ONLY.*@\1 = 1/i) do + assert_queries_match(/ORDER BY #{quoted_color} ASC, #{quoted_type} ASC OFFSET 0 ROWS FETCH NEXT @(\d) ROWS ONLY/i) do assert_kind_of ClothingItem, ClothingItem.first end ensure @@ -1162,7 +1147,7 @@ def test_implicit_order_column_reorders_query_constraints_coerced # Check for `FETCH NEXT x ROWS` rather then `LIMIT`. coerce_tests! :test_include_on_unloaded_relation_with_composite_primary_key def test_include_on_unloaded_relation_with_composite_primary_key_coerced - assert_queries_match(/1 AS one.*OFFSET 0 ROWS FETCH NEXT @(\d) ROWS ONLY.*@\1 = 1/) do + assert_queries_match(/1 AS one.*OFFSET 0 ROWS FETCH NEXT @(\d) ROWS ONLY/) do book = cpk_books(:cpk_great_author_first_book) assert Cpk::Book.where(title: "The first book").include?(book) end @@ -1172,11 +1157,11 @@ def test_include_on_unloaded_relation_with_composite_primary_key_coerced coerce_tests! :test_nth_to_last_with_order_uses_limit def test_nth_to_last_with_order_uses_limit_coerced c = Topic.lease_connection - assert_queries_match(/ORDER BY #{Regexp.escape(c.quote_table_name("topics.id"))} DESC OFFSET @(\d) ROWS FETCH NEXT @(\d) ROWS ONLY.*@\1 = 1.*@\2 = 1/i) do + assert_queries_match(/ORDER BY #{Regexp.escape(c.quote_table_name("topics.id"))} DESC OFFSET @(\d) ROWS FETCH NEXT @(\d) ROWS ONLY/i) do Topic.second_to_last end - assert_queries_match(/ORDER BY #{Regexp.escape(c.quote_table_name("topics.updated_at"))} DESC OFFSET @(\d) ROWS FETCH NEXT @(\d) ROWS ONLY.*@\1 = 1.*@\2 = 1/i) do + assert_queries_match(/ORDER BY #{Regexp.escape(c.quote_table_name("topics.updated_at"))} DESC OFFSET @(\d) ROWS FETCH NEXT @(\d) ROWS ONLY/i) do Topic.order(:updated_at).second_to_last end end @@ -1227,7 +1212,7 @@ class HasOneAssociationsTest < ActiveRecord::TestCase def test_has_one_coerced firm = companies(:first_firm) first_account = Account.find(1) - assert_queries_match(/FETCH NEXT @(\d) ROWS ONLY(.)*@\1 = 1/) do + assert_queries_match(/FETCH NEXT @(\d) ROWS ONLY/) do assert_equal first_account, firm.account assert_equal first_account.credit_limit, firm.account.credit_limit end @@ -1239,7 +1224,7 @@ class HasOneThroughAssociationsTest < ActiveRecord::TestCase coerce_tests! :test_has_one_through_executes_limited_query def test_has_one_through_executes_limited_query_coerced boring_club = clubs(:boring_club) - assert_queries_match(/FETCH NEXT @(\d) ROWS ONLY(.)*@\1 = 1/) do + assert_queries_match(/FETCH NEXT @(\d) ROWS ONLY/) do assert_equal boring_club, @member.general_club end end @@ -1460,7 +1445,7 @@ def test_having_with_binds_for_both_where_and_having # Find any limit via our expression. coerce_tests! %r{relations don't load all records in #inspect} def test_relations_dont_load_all_records_in_inspect_coerced - assert_queries_match(/NEXT @0 ROWS.*@0 = \d+/) do + assert_queries_match(/NEXT @0 ROWS/) do Post.all.inspect end end @@ -1568,10 +1553,10 @@ def test_dump_schema_information_outputs_lexically_reverse_ordered_versions_rega schema_info = ActiveRecord::Base.lease_connection.dump_schema_information expected = <<~STR - INSERT INTO #{ActiveRecord::Base.lease_connection.quote_table_name("schema_migrations")} (version) VALUES - (N'20100301010101'), - (N'20100201010101'), - (N'20100101010101'); + INSERT INTO #{ActiveRecord::Base.lease_connection.quote_table_name("schema_migrations")} (version) VALUES + (N'20100301010101'), + (N'20100201010101'), + (N'20100101010101'); STR assert_equal expected.strip, schema_info ensure @@ -2148,7 +2133,7 @@ def test_merge_doesnt_duplicate_same_clauses_coerced non_mary_and_bob = Author.where.not(id: [mary, bob]) author_id = Author.lease_connection.quote_table_name("authors.id") - assert_queries_match(/WHERE #{Regexp.escape(author_id)} NOT IN \((@\d), \g<1>\)'/) do + assert_queries_match(/WHERE #{Regexp.escape(author_id)} NOT IN \((@\d), \g<1>\)/) do assert_equal [david], non_mary_and_bob.merge(non_mary_and_bob) end @@ -2313,7 +2298,7 @@ def test_preloads_has_many_on_model_with_a_composite_primary_key_through_id_attr c = Cpk::OrderAgreement.lease_connection order_id_column = Regexp.escape(c.quote_table_name("cpk_order_agreements.order_id")) - order_id_constraint = /#{order_id_column} = @0.*@0 = \d+$/ + order_id_constraint = /#{order_id_column} = @0$/ expectation = /SELECT.*WHERE.* #{order_id_constraint}/ assert_match(expectation, preload_sql) @@ -2337,7 +2322,7 @@ def test_preloads_belongs_to_a_composite_primary_key_model_through_id_attribute_ c = Cpk::Order.lease_connection order_id = Regexp.escape(c.quote_table_name("cpk_orders.id")) - order_constraint = /#{order_id} = @0.*@0 = \d+$/ + order_constraint = /#{order_id} = @0$/ expectation = /SELECT.*WHERE.* #{order_constraint}/ assert_match(expectation, preload_sql) @@ -2410,7 +2395,7 @@ def test_sql_commenter_format_coerced ActiveRecord::QueryLogs.tags_formatter = :sqlcommenter ActiveRecord::QueryLogs.tags = [:application] - assert_queries_match(%r{/\*application=''active_record''\*/}) do + assert_queries_match(%r{/\*application='active_record'\*/}) do Dashboard.first end end @@ -2425,7 +2410,7 @@ def test_sqlcommenter_format_value_coerced { tracestate: "congo=t61rcWkgMzE,rojo=00f067aa0ba902b7", custom_proc: -> { "Joe's Shack" } }, ] - assert_queries_match(%r{custom_proc=''Joe%27s%20Shack'',tracestate=''congo%3Dt61rcWkgMzE%2Crojo%3D00f067aa0ba902b7''\*/}) do + assert_queries_match(%r{custom_proc='Joe%27s%20Shack',tracestate='congo%3Dt61rcWkgMzE%2Crojo%3D00f067aa0ba902b7'\*/}) do Dashboard.first end end @@ -2440,7 +2425,7 @@ def test_sqlcommenter_format_value_string_coercible_coerced { custom_proc: -> { 1234 } }, ] - assert_queries_match(%r{custom_proc=''1234''\*/}) do + assert_queries_match(%r{custom_proc='1234'\*/}) do Dashboard.first end end @@ -2459,7 +2444,7 @@ def test_sqlcommenter_format_allows_string_keys_coerced }, ] - assert_queries_match(%r{custom_proc=''Joe%27s%20Shack'',string=''value'',tracestate=''congo%3Dt61rcWkgMzE%2Crojo%3D00f067aa0ba902b7''\*/}) do + assert_queries_match(%r{custom_proc='Joe%27s%20Shack',string='value',tracestate='congo%3Dt61rcWkgMzE%2Crojo%3D00f067aa0ba902b7'\*/}) do Dashboard.first end end @@ -2719,7 +2704,7 @@ class ExplainTest < ActiveRecord::TestCase def test_relation_explain_with_first_coerced expected_query = capture_sql { Car.all.first - }.first[/EXEC sp_executesql N'(.*?) NEXT/, 1] + }.first[/(.*?) NEXT/, 1] message = Car.all.explain.first assert_match(/^EXPLAIN/, message) assert_match(expected_query, message) @@ -2730,7 +2715,7 @@ def test_relation_explain_with_first_coerced def test_relation_explain_with_last_coerced expected_query = capture_sql { Car.all.last - }.first[/EXEC sp_executesql N'(.*?) NEXT/, 1] + }.first[/(.*?) NEXT/, 1] expected_query = expected_query message = Car.all.explain.last diff --git a/test/cases/optimizer_hints_test_sqlserver.rb b/test/cases/optimizer_hints_test_sqlserver.rb index b1aab6820..fde5e9144 100644 --- a/test/cases/optimizer_hints_test_sqlserver.rb +++ b/test/cases/optimizer_hints_test_sqlserver.rb @@ -29,7 +29,7 @@ class OptimizerHitsTestSQLServer < ActiveRecord::TestCase end it "support subqueries" do - assert_queries_match(%r{.*'SELECT COUNT\(count_column\) FROM \(SELECT .*\) subquery_for_count OPTION \(MAXDOP 2\)'.*}) do + assert_queries_match(%r{SELECT COUNT\(count_column\) FROM \(SELECT .*\) subquery_for_count OPTION \(MAXDOP 2\)}) do companies = Company.optimizer_hints("MAXDOP 2") companies = companies.select(:id).where(firm_id: [0, 1]).limit(3) assert_equal 3, companies.count diff --git a/test/cases/showplan_test_sqlserver.rb b/test/cases/showplan_test_sqlserver.rb index 3bf9e1538..5b01fb9d1 100644 --- a/test/cases/showplan_test_sqlserver.rb +++ b/test/cases/showplan_test_sqlserver.rb @@ -28,13 +28,13 @@ class ShowplanTestSQLServer < ActiveRecord::TestCase it "from array condition using index" do plan = Car.where(id: [1, 2]).explain.inspect - _(plan).must_include "SELECT [cars].* FROM [cars] WHERE [cars].[id] IN (1, 2)" + _(plan).must_include "SELECT [cars].* FROM [cars] WHERE [cars].[id]" _(plan).must_include "Clustered Index Seek", "make sure we do not showplan the sp_executesql" end it "from array condition" do plan = Car.where(name: ["honda", "zyke"]).explain.inspect - _(plan).must_include " SELECT [cars].* FROM [cars] WHERE [cars].[name] IN (N'honda', N'zyke')" + _(plan).must_include " SELECT [cars].* FROM [cars] WHERE [cars].[name]" _(plan).must_include "Clustered Index Scan", "make sure we do not showplan the sp_executesql" end end diff --git a/test/cases/specific_schema_test_sqlserver.rb b/test/cases/specific_schema_test_sqlserver.rb index bfdce3617..cd0da18a6 100644 --- a/test/cases/specific_schema_test_sqlserver.rb +++ b/test/cases/specific_schema_test_sqlserver.rb @@ -115,17 +115,18 @@ def quoted_id "'T'" end end + value = value.new # Using ActiveRecord's quoted_id feature for objects. - assert_queries_match(/@0 = 'T'/) { SSTestDatatypeMigration.where(char_col: value.new).first } - assert_queries_match(/@0 = 'T'/) { SSTestDatatypeMigration.where(varchar_col: value.new).first } + assert_queries_and_values_match(/.*/, ["'T'", 1]) { SSTestDatatypeMigration.where(char_col: value).first } + assert_queries_and_values_match(/.*/, ["'T'", 1]) { SSTestDatatypeMigration.where(varchar_col: value).first } # Using our custom char type data. type = ActiveRecord::Type::SQLServer::Char data = ActiveRecord::Type::SQLServer::Data - assert_queries_match(/@0 = 'T'/) { SSTestDatatypeMigration.where(char_col: data.new("T", type.new)).first } - assert_queries_match(/@0 = 'T'/) { SSTestDatatypeMigration.where(varchar_col: data.new("T", type.new)).first } + assert_queries_and_values_match(/.*/, ["'T'", 1]) { SSTestDatatypeMigration.where(char_col: data.new("T", type.new)).first } + assert_queries_and_values_match(/.*/, ["'T'", 1]) { SSTestDatatypeMigration.where(varchar_col: data.new("T", type.new)).first } # Taking care of everything. - assert_queries_match(/@0 = 'T'/) { SSTestDatatypeMigration.where(char_col: "T").first } - assert_queries_match(/@0 = 'T'/) { SSTestDatatypeMigration.where(varchar_col: "T").first } + assert_queries_and_values_match(/.*/, ["'T'", 1]) { SSTestDatatypeMigration.where(char_col: "T").first } + assert_queries_and_values_match(/.*/, ["'T'", 1]) { SSTestDatatypeMigration.where(varchar_col: "T").first } end it "can update and hence properly quoted non-national char/varchar columns" do diff --git a/test/support/query_assertions.rb b/test/support/query_assertions.rb index accb11f09..8d5a46221 100644 --- a/test/support/query_assertions.rb +++ b/test/support/query_assertions.rb @@ -22,6 +22,28 @@ def assert_queries_count(count = nil, include_schema: false, &block) end end + def assert_queries_and_values_match(match, bound_values=[], count: nil, &block) + ActiveRecord::Base.lease_connection.materialize_transactions + + counter = ActiveRecord::Assertions::QueryAssertions::SQLCounter.new + ActiveSupport::Notifications.subscribed(counter, "sql.active_record") do + result = _assert_nothing_raised_or_warn("assert_queries_match", &block) + queries = counter.log_full + matched_queries = queries.select do |query, values| + values = values.map { |v| v.respond_to?(:quoted) ? v.quoted : v } + match === query && bound_values === values + end + + if count + assert_equal count, matched_queries.size, "#{matched_queries.size} instead of #{count} queries were executed.#{count.log.empty? ? '' : "\nQueries:\n#{counter.log.join("\n")}"}" + else + assert_operator matched_queries.size, :>=, 1, "1 or more queries expected, but none were executed.#{counter.log.empty? ? '' : "\nQueries:\n#{counter.log.join("\n")}"}" + end + + result + end + end + private # Rails tests expect a save-point to be created and released. SQL Server does not release From cf3da5703dbbea17fafc5ecad26f8b73fa6d062b Mon Sep 17 00:00:00 2001 From: Matt Taylor Date: Mon, 27 Oct 2025 11:38:43 -0500 Subject: [PATCH 2/4] Remove unnecessary method override in DatabaseStatements module --- .../connection_adapters/sqlserver/database_statements.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/active_record/connection_adapters/sqlserver/database_statements.rb b/lib/active_record/connection_adapters/sqlserver/database_statements.rb index a53f90e11..27967f9c1 100644 --- a/lib/active_record/connection_adapters/sqlserver/database_statements.rb +++ b/lib/active_record/connection_adapters/sqlserver/database_statements.rb @@ -45,10 +45,6 @@ def affected_rows(raw_result) raw_result.first[column_name] end - def raw_execute(sql, name = nil, binds = [], prepare: false, async: false, allow_retry: false, materialize_transactions: true, batch: false) - super - end - def internal_exec_sql_query(sql, conn) handle = internal_raw_execute(sql, conn) handle_to_names_and_values(handle, ar_result: true) From bdef07491646231134a02c7c96eb725104a6d321 Mon Sep 17 00:00:00 2001 From: Matt Taylor Date: Mon, 27 Oct 2025 12:00:20 -0500 Subject: [PATCH 3/4] Test cleanup Remove unnecessary duplication Remove some lines added during debugging --- test/cases/coerced_tests.rb | 18 ++++++++++-------- test/cases/specific_schema_test_sqlserver.rb | 5 ++--- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/test/cases/coerced_tests.rb b/test/cases/coerced_tests.rb index fef2a2e6d..37a48d21d 100644 --- a/test/cases/coerced_tests.rb +++ b/test/cases/coerced_tests.rb @@ -285,33 +285,35 @@ def assert_bind_params_to_sql_coerced(prepared:) # prepared_statements: true # + # SQL to database: # EXEC sp_executesql N'SELECT [authors].* FROM [authors] WHERE [authors].[id] IN (@0, @1, @2) OR [authors].[id] IS NULL)', N'@0 bigint, @1 bigint, @2 bigint', @0 = 1, @1 = 2, @2 = 3 # # prepared_statements: false # + # SQL to database: # SELECT [authors].* FROM [authors] WHERE ([authors].[id] IN (1, 2, 3) OR [authors].[id] IS NULL) # - sql_unprepared = "SELECT #{table}.* FROM #{table} WHERE (#{pk} IN (#{bind_params(1..3)}) OR #{pk} IS NULL)" - sql_prepared = "SELECT #{table}.* FROM #{table} WHERE (#{pk} IN (#{bind_params(1..3)}) OR #{pk} IS NULL)" + expected_logged_sql = "SELECT #{table}.* FROM #{table} WHERE (#{pk} IN (#{bind_params(1..3)}) OR #{pk} IS NULL)" authors = Author.where(id: [1, 2, 3, nil]) - assert_equal sql_unprepared, @connection.to_sql(authors.arel) - assert_queries_match(prepared ? sql_prepared : sql_unprepared) { assert_equal 3, authors.length } + assert_equal expected_logged_sql, @connection.to_sql(authors.arel) + assert_queries_match(expected_logged_sql) { assert_equal 3, authors.length } # prepared_statements: true # + # SQL to database: # EXEC sp_executesql N'SELECT [authors].* FROM [authors] WHERE [authors].[id] IN (@0, @1, @2)', N'@0 bigint, @1 bigint, @2 bigint', @0 = 1, @1 = 2, @2 = 3 # # prepared_statements: false # + # SQL to database: # SELECT [authors].* FROM [authors] WHERE [authors].[id] IN (1, 2, 3) # - sql_unprepared = "SELECT #{table}.* FROM #{table} WHERE #{pk} IN (#{bind_params(1..3)})" - sql_prepared = "SELECT #{table}.* FROM #{table} WHERE #{pk} IN (#{bind_params(1..3)})" + expected_logged_sql = "SELECT #{table}.* FROM #{table} WHERE #{pk} IN (#{bind_params(1..3)})" authors = Author.where(id: [1, 2, 3, 9223372036854775808]) - assert_equal sql_unprepared, @connection.to_sql(authors.arel) - assert_queries_match(prepared ? sql_prepared : sql_unprepared) { assert_equal 3, authors.length } + assert_equal expected_logged_sql, @connection.to_sql(authors.arel) + assert_queries_match(expected_logged_sql) { assert_equal 3, authors.length } end end end diff --git a/test/cases/specific_schema_test_sqlserver.rb b/test/cases/specific_schema_test_sqlserver.rb index cd0da18a6..0829012e5 100644 --- a/test/cases/specific_schema_test_sqlserver.rb +++ b/test/cases/specific_schema_test_sqlserver.rb @@ -115,10 +115,9 @@ def quoted_id "'T'" end end - value = value.new # Using ActiveRecord's quoted_id feature for objects. - assert_queries_and_values_match(/.*/, ["'T'", 1]) { SSTestDatatypeMigration.where(char_col: value).first } - assert_queries_and_values_match(/.*/, ["'T'", 1]) { SSTestDatatypeMigration.where(varchar_col: value).first } + assert_queries_and_values_match(/.*/, ["'T'", 1]) { SSTestDatatypeMigration.where(char_col: value.new).first } + assert_queries_and_values_match(/.*/, ["'T'", 1]) { SSTestDatatypeMigration.where(varchar_col: value.new).first } # Using our custom char type data. type = ActiveRecord::Type::SQLServer::Char data = ActiveRecord::Type::SQLServer::Data From 429a441d1119ac599823bfb6d53ca638aa7c307d Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Thu, 6 Nov 2025 19:33:10 +0000 Subject: [PATCH 4/4] Removed tests that no longer need to be coerced. Added changelog. --- CHANGELOG.md | 6 ++ test/cases/coerced_tests.rb | 138 -------------------------- test/cases/showplan_test_sqlserver.rb | 4 +- 3 files changed, 8 insertions(+), 140 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 97d6e8f93..2a68fe895 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +## Unreleased + +#### Fixed + +- [#1370](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1370) Fixed query logging so that filter parameters are respected. + ## v8.0.9 #### Fixed diff --git a/test/cases/coerced_tests.rb b/test/cases/coerced_tests.rb index 37a48d21d..421c7ea2a 100644 --- a/test/cases/coerced_tests.rb +++ b/test/cases/coerced_tests.rb @@ -264,57 +264,6 @@ class BindParameterTest < ActiveRecord::TestCase coerce_tests! :test_statement_cache_with_find_by coerce_tests! :test_statement_cache_with_in_clause coerce_tests! :test_statement_cache_with_sql_string_literal - - # Same as original coerced test except prepared statements include `EXEC sp_executesql` wrapper. - coerce_tests! :test_bind_params_to_sql_with_prepared_statements, :test_bind_params_to_sql_with_unprepared_statements - def test_bind_params_to_sql_with_prepared_statements_coerced - assert_bind_params_to_sql_coerced(prepared: true) - end - - def test_bind_params_to_sql_with_unprepared_statements_coerced - @connection.unprepared_statement do - assert_bind_params_to_sql_coerced(prepared: false) - end - end - - private - - def assert_bind_params_to_sql_coerced(prepared:) - table = Author.quoted_table_name - pk = "#{table}.#{Author.quoted_primary_key}" - - # prepared_statements: true - # - # SQL to database: - # EXEC sp_executesql N'SELECT [authors].* FROM [authors] WHERE [authors].[id] IN (@0, @1, @2) OR [authors].[id] IS NULL)', N'@0 bigint, @1 bigint, @2 bigint', @0 = 1, @1 = 2, @2 = 3 - # - # prepared_statements: false - # - # SQL to database: - # SELECT [authors].* FROM [authors] WHERE ([authors].[id] IN (1, 2, 3) OR [authors].[id] IS NULL) - # - expected_logged_sql = "SELECT #{table}.* FROM #{table} WHERE (#{pk} IN (#{bind_params(1..3)}) OR #{pk} IS NULL)" - - authors = Author.where(id: [1, 2, 3, nil]) - assert_equal expected_logged_sql, @connection.to_sql(authors.arel) - assert_queries_match(expected_logged_sql) { assert_equal 3, authors.length } - - # prepared_statements: true - # - # SQL to database: - # EXEC sp_executesql N'SELECT [authors].* FROM [authors] WHERE [authors].[id] IN (@0, @1, @2)', N'@0 bigint, @1 bigint, @2 bigint', @0 = 1, @1 = 2, @2 = 3 - # - # prepared_statements: false - # - # SQL to database: - # SELECT [authors].* FROM [authors] WHERE [authors].[id] IN (1, 2, 3) - # - expected_logged_sql = "SELECT #{table}.* FROM #{table} WHERE #{pk} IN (#{bind_params(1..3)})" - - authors = Author.where(id: [1, 2, 3, 9223372036854775808]) - assert_equal expected_logged_sql, @connection.to_sql(authors.arel) - assert_queries_match(expected_logged_sql) { assert_equal 3, authors.length } - end end end @@ -2391,66 +2340,6 @@ def test_in_order_of_with_nil_coerced require "models/dashboard" class QueryLogsTest < ActiveRecord::TestCase - # SQL requires double single-quotes. - coerce_tests! :test_sql_commenter_format - def test_sql_commenter_format_coerced - ActiveRecord::QueryLogs.tags_formatter = :sqlcommenter - ActiveRecord::QueryLogs.tags = [:application] - - assert_queries_match(%r{/\*application='active_record'\*/}) do - Dashboard.first - end - end - - # SQL requires double single-quotes. - coerce_tests! :test_sqlcommenter_format_value - def test_sqlcommenter_format_value_coerced - ActiveRecord::QueryLogs.tags_formatter = :sqlcommenter - - ActiveRecord::QueryLogs.tags = [ - :application, - { tracestate: "congo=t61rcWkgMzE,rojo=00f067aa0ba902b7", custom_proc: -> { "Joe's Shack" } }, - ] - - assert_queries_match(%r{custom_proc='Joe%27s%20Shack',tracestate='congo%3Dt61rcWkgMzE%2Crojo%3D00f067aa0ba902b7'\*/}) do - Dashboard.first - end - end - - # SQL requires double single-quotes. - coerce_tests! :test_sqlcommenter_format_value_string_coercible - def test_sqlcommenter_format_value_string_coercible_coerced - ActiveRecord::QueryLogs.tags_formatter = :sqlcommenter - - ActiveRecord::QueryLogs.tags = [ - :application, - { custom_proc: -> { 1234 } }, - ] - - assert_queries_match(%r{custom_proc='1234'\*/}) do - Dashboard.first - end - end - - # SQL requires double single-quotes. - coerce_tests! :test_sqlcommenter_format_allows_string_keys - def test_sqlcommenter_format_allows_string_keys_coerced - ActiveRecord::QueryLogs.tags_formatter = :sqlcommenter - - ActiveRecord::QueryLogs.tags = [ - :application, - { - "string" => "value", - tracestate: "congo=t61rcWkgMzE,rojo=00f067aa0ba902b7", - custom_proc: -> { "Joe's Shack" } - }, - ] - - assert_queries_match(%r{custom_proc='Joe%27s%20Shack',string='value',tracestate='congo%3Dt61rcWkgMzE%2Crojo%3D00f067aa0ba902b7'\*/}) do - Dashboard.first - end - end - # Invalid character encoding causes `ActiveRecord::StatementInvalid` error similar to Postgres. coerce_tests! :test_invalid_encoding_query def test_invalid_encoding_query_coerced @@ -2699,33 +2588,6 @@ def type_for_attribute_is_not_aware_of_custom_types_coerced end end -require "models/car" -class ExplainTest < ActiveRecord::TestCase - # Expected query slightly different from because of 'sp_executesql' and query parameters. - coerce_tests! :test_relation_explain_with_first - def test_relation_explain_with_first_coerced - expected_query = capture_sql { - Car.all.first - }.first[/(.*?) NEXT/, 1] - message = Car.all.explain.first - assert_match(/^EXPLAIN/, message) - assert_match(expected_query, message) - end - - # Expected query slightly different from because of 'sp_executesql' and query parameters. - coerce_tests! :test_relation_explain_with_last - def test_relation_explain_with_last_coerced - expected_query = capture_sql { - Car.all.last - }.first[/(.*?) NEXT/, 1] - expected_query = expected_query - message = Car.all.explain.last - - assert_match(/^EXPLAIN/, message) - assert_match(expected_query, message) - end -end - module ActiveRecord module Assertions class QueryAssertionsTest < ActiveSupport::TestCase diff --git a/test/cases/showplan_test_sqlserver.rb b/test/cases/showplan_test_sqlserver.rb index 5b01fb9d1..6dfde5386 100644 --- a/test/cases/showplan_test_sqlserver.rb +++ b/test/cases/showplan_test_sqlserver.rb @@ -28,13 +28,13 @@ class ShowplanTestSQLServer < ActiveRecord::TestCase it "from array condition using index" do plan = Car.where(id: [1, 2]).explain.inspect - _(plan).must_include "SELECT [cars].* FROM [cars] WHERE [cars].[id]" + _(plan).must_include "SELECT [cars].* FROM [cars] WHERE [cars].[id] IN (@0, @1)" _(plan).must_include "Clustered Index Seek", "make sure we do not showplan the sp_executesql" end it "from array condition" do plan = Car.where(name: ["honda", "zyke"]).explain.inspect - _(plan).must_include " SELECT [cars].* FROM [cars] WHERE [cars].[name]" + _(plan).must_include " SELECT [cars].* FROM [cars] WHERE [cars].[name] IN (@0, @1)" _(plan).must_include "Clustered Index Scan", "make sure we do not showplan the sp_executesql" end end