Merge pull request #749 from rails-sqlserver/5-2-backport

Backport master to 5-2-stable

Author: wpolicarpo

lib/active_record/connection_adapters/sqlserver/core_ext/calculations.rb

@@ -6,6 +6,25 @@ module ConnectionAdapters
     module SQLServer
       module CoreExt
         module Calculations
+
+          # Same as original except we don't perform PostgreSQL hack that removes ordering.
+          def calculate(operation, column_name)
+            if has_include?(column_name)
+              relation = apply_join_dependency
+
+              if operation.to_s.downcase == "count"
+                unless distinct_value || distinct_select?(column_name || select_for_count)
+                  relation.distinct!
+                  relation.select_values = [ klass.primary_key || table[Arel.star] ]
+                end
+              end
+
+              relation.calculate(operation, column_name)
+            else
+              perform_calculation(operation, column_name)
+            end
+          end
+
           private
 
           def build_count_subquery(relation, column_name, distinct)

lib/active_record/connection_adapters/sqlserver/core_ext/finder_methods.rb

@@ -0,0 +1,43 @@
+require 'active_record/relation'
+require 'active_record/version'
+
+module ActiveRecord
+  module ConnectionAdapters
+    module SQLServer
+      module CoreExt
+        module FinderMethods
+
+          private
+
+          # Same as original except we order by values in distinct select if present.
+          def construct_relation_for_exists(conditions)
+            if distinct_value && offset_value
+              relation = limit!(1)
+
+              if select_values.present?
+                relation = relation.order(*select_values)
+              else
+                relation = relation.except(:order)
+              end
+            else
+              relation = except(:select, :distinct, :order)._select!(::ActiveRecord::FinderMethods::ONE_AS_ONE).limit!(1)
+            end
+
+            case conditions
+            when Array, Hash
+              relation.where!(conditions) unless conditions.empty?
+            else
+              relation.where!(primary_key => conditions) unless conditions == :none
+            end
+
+            relation
+          end
+        end
+      end
+    end
+  end
+end
+
+ActiveSupport.on_load(:active_record) do
+  ActiveRecord::Relation.include(ActiveRecord::ConnectionAdapters::SQLServer::CoreExt::FinderMethods)
+end

lib/active_record/connection_adapters/sqlserver/core_ext/query_methods.rb

@@ -0,0 +1,26 @@
+require 'active_record/relation'
+require 'active_record/version'
+
+module ActiveRecord
+  module ConnectionAdapters
+    module SQLServer
+      module CoreExt
+        module QueryMethods
+
+          private
+
+          # Copy of original from Rails master. This patch can be removed when adapter supports Rails 6.
+          def table_name_matches?(from)
+            table_name = Regexp.escape(table.name)
+            quoted_table_name = Regexp.escape(connection.quote_table_name(table.name))
+            /(?:\A|(?<!FROM)\s)(?:\b#{table_name}\b|#{quoted_table_name})(?!\.)/i.match?(from.to_s)
+          end
+        end
+      end
+    end
+  end
+end
+
+ActiveSupport.on_load(:active_record) do
+  ActiveRecord::Relation.include(ActiveRecord::ConnectionAdapters::SQLServer::CoreExt::QueryMethods)
+end

lib/active_record/connection_adapters/sqlserver_adapter.rb

@@ -7,6 +7,8 @@
 require 'active_record/connection_adapters/sqlserver/core_ext/explain'
 require 'active_record/connection_adapters/sqlserver/core_ext/explain_subscriber'
 require 'active_record/connection_adapters/sqlserver/core_ext/attribute_methods'
+require 'active_record/connection_adapters/sqlserver/core_ext/finder_methods'
+require 'active_record/connection_adapters/sqlserver/core_ext/query_methods'
 require 'active_record/connection_adapters/sqlserver/version'
 require 'active_record/connection_adapters/sqlserver/type'
 require 'active_record/connection_adapters/sqlserver/database_limits'
@@ -40,6 +42,9 @@ class SQLServerAdapter < AbstractAdapter
 
       ADAPTER_NAME = 'SQLServer'.freeze
 
+      # Default precision for 'time' (See https://docs.microsoft.com/en-us/sql/t-sql/data-types/time-transact-sql)
+      DEFAULT_TIME_PRECISION = 7
+
       attr_reader :spid
 
       cattr_accessor :cs_equality_operator, instance_accessor: false
@@ -296,8 +301,7 @@ def initialize_type_map(m = type_map)
         end
         m.register_type              'smalldatetime',     SQLServer::Type::SmallDateTime.new
         m.register_type              %r{\Atime}i do |sql_type|
-          scale = extract_scale(sql_type)
-          precision = extract_precision(sql_type)
+          precision = extract_precision(sql_type) || DEFAULT_TIME_PRECISION
           SQLServer::Type::Time.new precision: precision
         end
         # Character Strings

test/cases/adapter_test_sqlserver.rb

@@ -15,14 +15,14 @@ class AdapterTestSQLServer < ActiveRecord::TestCase
 
   it 'has basic and non-senstive information in the adpaters inspect method' do
     string = connection.inspect
-    string.must_match %r{ActiveRecord::ConnectionAdapters::SQLServerAdapter}
-    string.must_match %r{version\: \d.\d}
-    string.must_match %r{mode: dblib}
-    string.must_match %r{azure: (true|false)}
-    string.wont_match %r{host}
-    string.wont_match %r{password}
-    string.wont_match %r{username}
-    string.wont_match %r{port}
+    _(string).must_match %r{ActiveRecord::ConnectionAdapters::SQLServerAdapter}
+    _(string).must_match %r{version\: \d.\d}
+    _(string).must_match %r{mode: dblib}
+    _(string).must_match %r{azure: (true|false)}
+    _(string).wont_match %r{host}
+    _(string).wont_match %r{password}
+    _(string).wont_match %r{username}
+    _(string).wont_match %r{port}
   end
 
   it 'has a 128 max #table_alias_length' do
@@ -161,7 +161,7 @@ class AdapterTestSQLServer < ActiveRecord::TestCase
     end
 
     it 'return an empty array when calling #identity_columns for a table_name with no identity' do
-      connection.send(:identity_columns, Subscriber.table_name).must_equal []
+      _(connection.send(:identity_columns, Subscriber.table_name)).must_equal []
     end
 
   end
@@ -303,7 +303,7 @@ class AdapterTestSQLServer < ActiveRecord::TestCase
     end
 
     it 'find SSTestCustomersView table name' do
-      connection.views.must_include 'sst_customers_view'
+      _(connection.views).must_include 'sst_customers_view'
     end
 
     it 'work with dynamic finders' do
@@ -344,9 +344,9 @@ class AdapterTestSQLServer < ActiveRecord::TestCase
     end
 
     it 'find identity column' do
-      SSTestCustomersView.primary_key.must_equal 'id'
-      connection.primary_key(SSTestCustomersView.table_name).must_equal 'id'
-      SSTestCustomersView.columns_hash['id'].must_be :is_identity?
+      _(SSTestCustomersView.primary_key).must_equal 'id'
+      _(connection.primary_key(SSTestCustomersView.table_name)).must_equal 'id'
+      _(SSTestCustomersView.columns_hash['id']).must_be :is_identity?
     end
 
     it 'find default values' do
@@ -371,9 +371,9 @@ class AdapterTestSQLServer < ActiveRecord::TestCase
     end
 
     it 'find identity column' do
-      SSTestStringDefaultsView.primary_key.must_equal 'id'
-      connection.primary_key(SSTestStringDefaultsView.table_name).must_equal 'id'
-      SSTestStringDefaultsView.columns_hash['id'].must_be :is_identity?
+      _(SSTestStringDefaultsView.primary_key).must_equal 'id'
+      _(connection.primary_key(SSTestStringDefaultsView.table_name)).must_equal 'id'
+      _(SSTestStringDefaultsView.columns_hash['id']).must_be :is_identity?
     end
 
     it 'find default values' do
@@ -422,8 +422,8 @@ class AdapterTestSQLServer < ActiveRecord::TestCase
 
   it 'in_memory_oltp' do
     if ENV['IN_MEMORY_OLTP'] && connection.supports_in_memory_oltp?
-      SSTMemory.primary_key.must_equal 'id'
-      SSTMemory.columns_hash['id'].must_be :is_identity?
+      _(SSTMemory.primary_key).must_equal 'id'
+      _(SSTMemory.columns_hash['id']).must_be :is_identity?
     else
       skip 'supports_in_memory_oltp? => false'
     end

test/cases/change_column_null_test_sqlserver.rb

@@ -24,19 +24,19 @@ def find_column(table, name)
 
   describe '#change_column_null' do
     it 'does not change the column limit' do
-      name_column.limit.must_equal 15
+      _(name_column.limit).must_equal 15
     end
 
     it 'does not change the column default' do
-      code_column.default.must_equal 'n/a'
+      _(code_column.default).must_equal 'n/a'
     end
 
     it 'does not change the column precision' do
-      value_column.precision.must_equal 32
+      _(value_column.precision).must_equal 32
     end
 
     it 'does not change the column scale' do
-      value_column.scale.must_equal 8
+      _(value_column.scale).must_equal 8
     end
   end
 end

test/cases/coerced_tests.rb

@@ -145,8 +145,27 @@ def test_belongs_to_with_primary_key_joins_on_correct_column_coerced
 
 module ActiveRecord
   class BindParameterTest < ActiveRecord::TestCase
-    # Never finds `sql` since we use `EXEC sp_executesql` wrappers.
+    # 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
+    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
   end
 end
 
@@ -192,14 +211,14 @@ def test_should_return_decimal_average_of_integer_field_coerced
   def test_limit_is_kept_coerced
     queries = capture_sql_ss { Account.limit(1).count }
     assert_equal 1, queries.length
-    queries.first.must_match %r{ORDER BY \[accounts\]\.\[id\] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY.*@0 = 1}
+    _(queries.first).must_match %r{ORDER BY \[accounts\]\.\[id\] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY.*@0 = 1}
   end
 
   coerce_tests! :test_limit_with_offset_is_kept
   def test_limit_with_offset_is_kept_coerced
     queries = capture_sql_ss { Account.limit(1).offset(1).count }
     assert_equal 1, queries.length
-    queries.first.must_match %r{ORDER BY \[accounts\]\.\[id\] ASC OFFSET @0 ROWS FETCH NEXT @1 ROWS ONLY.*@0 = 1, @1 = 1}
+    _(queries.first).must_match %r{ORDER BY \[accounts\]\.\[id\] ASC OFFSET @0 ROWS FETCH NEXT @1 ROWS ONLY.*@0 = 1, @1 = 1}
   end
 
   # SQL Server needs an alias for the calculated column
@@ -265,7 +284,7 @@ class ColumnAttributesTest < ActiveRecord::TestCase
       def test_add_column_without_limit_coerced
         add_column :test_models, :description, :string, limit: nil
         TestModel.reset_column_information
-        TestModel.columns_hash["description"].limit.must_equal 4000
+        _(TestModel.columns_hash["description"].limit).must_equal 4000
       end
     end
   end
@@ -615,14 +634,14 @@ class PersistenceTest < ActiveRecord::TestCase
   coerce_tests! :test_update_all_doesnt_ignore_order
   def test_update_all_doesnt_ignore_order_coerced
     david, mary = authors(:david), authors(:mary)
-    david.id.must_equal 1
-    mary.id.must_equal 2
-    david.name.wont_equal mary.name
+    _(david.id).must_equal 1
+    _(mary.id).must_equal 2
+    _(david.name).wont_equal mary.name
     assert_sql(/UPDATE.*\(SELECT \[authors\].\[id\] FROM \[authors\].*ORDER BY \[authors\].\[id\]/i) do
       Author.where('[id] > 1').order(:id).update_all(name: 'Test')
     end
-    david.reload.name.must_equal 'David'
-    mary.reload.name.must_equal 'Test'
+    _(david.reload.name).must_equal 'David'
+    _(mary.reload.name).must_equal 'Test'
   end
 
   # We can not UPDATE identity columns.