Merge pull request #1 from rails-sqlserver/master

Merge upstream

Author: smlsml

CHANGELOG.md

@@ -1,3 +1,14 @@
+## v5.2.0
+
+- #686 sql_for_insert set table name in case when pk is not nil
+
+## v5.2.0.rc2
+
+#### Fixed
+
+- #681 change_column_null should not clear other column attributes. Fixes #582.
+- #684 Fix explain with array conditions. Fixes #673.
+
 ## v5.2.0.rc1
 
 #### Fixed

README.md

@@ -1,7 +1,6 @@
 # ActiveRecord SQL Server Adapter. For SQL Server 2012 And Higher.
 
 * [![TravisCI](https://travis-ci.org/rails-sqlserver/activerecord-sqlserver-adapter.svg?branch=master)](https://travis-ci.org/rails-sqlserver/activerecord-sqlserver-adapter) - TravisCI
-* [![CircleCI](https://circleci.com/gh/rails-sqlserver/activerecord-sqlserver-adapter/tree/master.svg?style=svg)](https://circleci.com/gh/rails-sqlserver/activerecord-sqlserver-adapter/tree/master) - CircleCI
 * [![Build Status](https://ci.appveyor.com/api/projects/status/mtgbx8f57vr7k2qa/branch/master?svg=true)](https://ci.appveyor.com/project/rails-sqlserver/activerecord-sqlserver-adapter/branch/master) - Appveyor
 * [![Gem Version](http://img.shields.io/gem/v/activerecord-sqlserver-adapter.svg)](https://rubygems.org/gems/activerecord-sqlserver-adapter) - Gem Version
 * [![Dependency Status](https://dependencyci.com/github/rails-sqlserver/activerecord-sqlserver-adapter/badge)](https://dependencyci.com/github/rails-sqlserver/activerecord-sqlserver-adapter) - Dependency Status

VERSION

@@ -1 +1 @@
-5.2.0.rc1
+5.2.0

circle.yml

@@ -5,12 +5,11 @@ module CoreExt
         module Explain
 
           SQLSERVER_STATEMENT_PREFIX = 'EXEC sp_executesql '.freeze
-          SQLSERVER_PARAM_MATCHER = /@\d+ = (.*)/
-          SQLSERVER_NATIONAL_STRING_MATCHER = /N'(.*)'/m
+          SQLSERVER_STATEMENT_REGEXP = /N'(.+)', N'(.+)', (.+)/
 
           def exec_explain(queries)
             unprepared_queries = queries.map do |(sql, binds)|
-              [unprepare_sqlserver_statement(sql), binds]
+              [unprepare_sqlserver_statement(sql, binds), binds]
             end
             super(unprepared_queries)
           end
@@ -19,22 +18,19 @@ def exec_explain(queries)
 
           # This is somewhat hacky, but it should reliably reformat our prepared sql statment
           # which uses sp_executesql to just the first argument, then unquote it. Likewise our
-          # `sp_executesql` method should substitude the @n args withe the quoted values.
-          def unprepare_sqlserver_statement(sql)
-            if sql.starts_with?(SQLSERVER_STATEMENT_PREFIX)
-              executesql = sql.from(SQLSERVER_STATEMENT_PREFIX.length)
-              args = executesql.split(', ')
-              unprepared_sql = args.shift.strip.match(SQLSERVER_NATIONAL_STRING_MATCHER)[1]
-              unprepared_sql = Utils.unquote_string(unprepared_sql)
-              args = args.from(args.length / 2)
-              args.each_with_index do |arg, index|
-                value = arg.match(SQLSERVER_PARAM_MATCHER)[1]
-                unprepared_sql.sub! "@#{index}", value
-              end
-              unprepared_sql
-            else
-              sql
+          # `sp_executesql` method should substitude the @n args with the quoted values.
+          def unprepare_sqlserver_statement(sql, binds)
+            return sql unless sql.starts_with?(SQLSERVER_STATEMENT_PREFIX)
+
+            executesql = sql.from(SQLSERVER_STATEMENT_PREFIX.length)
+            executesql = executesql.match(SQLSERVER_STATEMENT_REGEXP).to_a[1]
+
+            binds.each_with_index do |bind, index|
+              value = connection.quote(bind)
+              executesql = executesql.sub("@#{index}", value)
             end
+
+            executesql
           end
 
         end

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

@@ -228,6 +228,7 @@ def sql_for_insert(sql, pk, id_value, sequence_name, binds)
           end
           sql = if pk && use_output_inserted? && !database_prefix_remote_server?
                   quoted_pk = SQLServer::Utils.extract_identifiers(pk).quoted
+                  table_name ||= get_table_name(sql)
                   exclude_output_inserted = exclude_output_inserted_table_name?(table_name, sql)
                   if exclude_output_inserted
                     id_sql_type = exclude_output_inserted.is_a?(TrueClass) ? 'bigint' : exclude_output_inserted

lib/active_record/connection_adapters/sqlserver/database_statements.rb

@@ -258,7 +258,7 @@ def update_table_definition(table_name, base)
         def change_column_null(table_name, column_name, allow_null, default = nil)
           table_id = SQLServer::Utils.extract_identifiers(table_name)
           column_id = SQLServer::Utils.extract_identifiers(column_name)
-          column = detect_column_for! table_name, column_name
+          column = column_for(table_name, column_name)
           if !allow_null.nil? && allow_null == false && !default.nil?
             do_execute("UPDATE #{table_id} SET #{column_id}=#{quote(default)} WHERE #{column_id} IS NULL")
           end
@@ -492,13 +492,6 @@ def default_constraint_name(table_name, column_name)
           "DF_#{table_name}_#{column_name}"
         end
 
-        def detect_column_for!(table_name, column_name)
-          unless column = schema_cache.columns(table_name).find { |c| c.name == column_name.to_s }
-            raise ActiveRecordError, "No such column: #{table_name}.#{column_name}"
-          end
-          column
-        end
-
         def lowercase_schema_reflection_sql(node)
           lowercase_schema_reflection ? "LOWER(#{node})" : node
         end

lib/active_record/connection_adapters/sqlserver/schema_statements.rb

@@ -0,0 +1,42 @@
+require 'cases/helper_sqlserver'
+require 'migrations/create_clients_and_change_column_null'
+
+class ChangeColumnNullTestSqlServer < ActiveRecord::TestCase
+  before do
+    @old_verbose = ActiveRecord::Migration.verbose
+    ActiveRecord::Migration.verbose = false
+    CreateClientsAndChangeColumnNull.new.up
+  end
+
+  after do
+    CreateClientsAndChangeColumnNull.new.down
+    ActiveRecord::Migration.verbose = @old_verbose
+  end
+
+  def find_column(table, name)
+    table.find { |column| column.name == name }
+  end
+
+  let(:clients_table) { connection.columns('clients') }
+  let(:name_column) { find_column(clients_table, 'name') }
+  let(:code_column) { find_column(clients_table, 'code') }
+  let(:value_column) { find_column(clients_table, 'value') }
+
+  describe '#change_column_null' do
+    it 'does not change the column limit' do
+      name_column.limit.must_equal 15
+    end
+
+    it 'does not change the column default' do
+      code_column.default.must_equal 'n/a'
+    end
+
+    it 'does not change the column precision' do
+      value_column.precision.must_equal 32
+    end
+
+    it 'does not change the column scale' do
+      value_column.scale.must_equal 8
+    end
+  end
+end

test/cases/change_column_null_test_sqlserver.rb

@@ -26,6 +26,18 @@ class ShowplanTestSQLServer < ActiveRecord::TestCase
       plan.must_include "Clustered Index Scan", 'make sure we do not showplan the sp_executesql'
     end
 
+    it 'from array condition using index' do
+      plan = Car.where(id: [1, 2]).explain
+      plan.must_include " SELECT [cars].* FROM [cars] WHERE [cars].[id] IN (1, 2)"
+      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
+      plan.must_include " SELECT [cars].* FROM [cars] WHERE [cars].[name] IN (N'honda', N'zyke')"
+      plan.must_include "Clustered Index Scan", 'make sure we do not showplan the sp_executesql'
+    end
+
   end
 
   describe 'With SHOWPLAN_TEXT option' do

test/cases/showplan_test_sqlserver.rb

@@ -0,0 +1,23 @@
+class CreateClientsAndChangeColumnNull < ActiveRecord::Migration[5.2]
+  def up
+    create_table :clients do |t|
+      t.string :name
+      t.string :code
+      t.decimal :value
+
+      t.timestamps
+    end
+
+    change_column :clients, :name, :string, limit: 15
+    change_column :clients, :code, :string, default: 'n/a'
+    change_column :clients, :value, :decimal, precision: 32, scale: 8
+
+    change_column_null :clients, :name, false
+    change_column_null :clients, :code, false
+    change_column_null :clients, :value, false
+  end
+
+  def down
+    drop_table :clients
+  end
+end