Better bind types & params for sp_executesql. Fixes #239.

Fixed: RuntimeError: Unknown bind columns. We can account for this.

* HasManyAssociationsTest#test_with_polymorphic_has_many_with_custom_columns_name:
* HasOneAssociationsTest#test_with_polymorphic_has_one_with_custom_columns_name:

12 failures, 6 errors, 2 skips

Author: metaskills

CHANGELOG.md

@@ -49,6 +49,7 @@
 #### Fixed
 
 * Default lock is now "WITH(UPDLOCK)". Fixes #368
+* Better bind types & params for `sp_executesql`. Fixes #239.
 
 #### Security
 

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

@@ -16,7 +16,7 @@ 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
-          # do_exec_query method should substitude the @n args withe the quoted values.
+          # `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)

lib/active_record/connection_adapters/sqlserver/database_statements.rb

@@ -4,7 +4,7 @@ module SQLServer
       module DatabaseStatements
 
         def select_rows(sql, name = nil, binds = [])
-          do_exec_query sql, name, binds, fetch: :rows
+          sp_executesql sql, name, binds, fetch: :rows
         end
 
         def execute(sql, name = nil)
@@ -18,9 +18,9 @@ def execute(sql, name = nil)
         def exec_query(sql, name = 'SQL', binds = [], sqlserver_options = {})
           if update_sql?(sql)
             sql = strip_ident_from_update(sql)
-            do_exec_query(sql, name, binds)
+            sp_executesql(sql, name, binds)
           else
-            do_exec_query(sql, name, binds)
+            sp_executesql(sql, name, binds)
           end
         end
 
@@ -307,42 +307,45 @@ def do_execute(sql, name = 'SQL')
           log(sql, name) { raw_connection_do(sql) }
         end
 
-        def do_exec_query(sql, name, binds, options = {})
-          # This allows non-AR code to utilize the binds
-          # handling code, e.g. select_rows()
-          if options[:fetch] != :rows
-            options[:ar_result] = true
-          end
-          explaining = name == 'EXPLAIN'
-          names_and_types = []
-          params = []
+        def sp_executesql(sql, name, binds, options = {})
+          options[:ar_result] = true if options[:fetch] != :rows
+          types, params = sp_executesql_types_and_parameters(binds)
+          sql = sp_executesql_sql(sql, types, params, name)
+          raw_select sql, name, binds, options
+        end
+
+        def sp_executesql_types_and_parameters(binds)
+          types, params = [], []
           binds.each_with_index do |(column, value), index|
-            ar_column = column.is_a?(ActiveRecord::ConnectionAdapters::Column)
-            next if ar_column && column.sql_type == 'timestamp'
-            v = value
-            names_and_types << if ar_column
-                                 "@#{index} #{column.sql_type_for_statement}"
-                               elsif column.acts_like?(:string)
-                                 "@#{index} nvarchar(max)"
-                               elsif column.is_a?(Fixnum)
-                                 v = value.to_i
-                                 "@#{index} int"
-                               else
-                                 raise 'Unknown bind columns. We can account for this.'
-                               end
-            quoted_value = ar_column ? quote(v, column) : quote(v, nil)
-            params << (explaining ? quoted_value : "@#{index} = #{quoted_value}")
+            types << "@#{index} #{sp_executesql_sql_type(column, value)}"
+            params << quote(value, column)
           end
-          if explaining
-            params.each_with_index do |param, index|
+          [types, params]
+        end
+
+        def sp_executesql_sql_type(column, value)
+          return column.sql_type_for_statement if SQLServerColumn === column
+          if value.is_a?(Numeric)
+            'int'
+          # We can do more here later.
+          else
+            'nvarchar(max)'
+          end
+        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.sub! substitute_at_finder, param
             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 << ", #{quote(names_and_types.join(', '))}, #{params.join(', ')}" unless binds.empty?
+            sql << ", #{types}, #{params}" unless params.empty?
           end
-          raw_select sql, name, binds, options
+          sql
         end
 
         def raw_connection_do(sql)

lib/active_record/connection_adapters/sqlserver/schema_statements.rb

@@ -244,9 +244,9 @@ def column_definitions(table_name)
               AND columns.TABLE_SCHEMA = #{identifier.schema.blank? ? 'schema_name()' : '@1'}
             ORDER BY columns.ordinal_position
           }.gsub(/[ \t\r\n]+/, ' ')
-          binds = [['table_name', identifier.object]]
-          binds << ['table_schema', identifier.schema] unless identifier.schema.blank?
-          results = do_exec_query(sql, 'SCHEMA', binds)
+          binds = [[info_schema_table_name_column, identifier.object]]
+          binds << [info_schema_table_schema_column, identifier.schema] unless identifier.schema.blank?
+          results = sp_executesql(sql, 'SCHEMA', binds)
           results.map do |ci|
             ci = ci.symbolize_keys
             ci[:_type] = ci[:type]
@@ -302,6 +302,14 @@ def column_definitions(table_name)
           end
         end
 
+        def info_schema_table_name_column
+          @info_schema_table_name_column ||= new_column 'table_name', nil, lookup_cast_type('nvarchar(128)'), 'nvarchar(128)', true
+        end
+
+        def info_schema_table_schema_column
+          @info_schema_table_schema_column ||= new_column 'table_schema', nil, lookup_cast_type('nvarchar(128)'), 'nvarchar(128)', true
+        end
+
         def remove_check_constraints(table_name, column_name)
           constraints = select_values "SELECT CONSTRAINT_NAME FROM INFORMATION_SCHEMA.CONSTRAINT_COLUMN_USAGE where TABLE_NAME = '#{quote_string(table_name)}' and COLUMN_NAME = '#{quote_string(column_name)}'", 'SCHEMA'
           constraints.each do |constraint|

lib/active_record/connection_adapters/sqlserver/showplan.rb

@@ -13,7 +13,7 @@ module Showplan
 
         def explain(arel, binds = [])
           sql = to_sql(arel)
-          result = with_showplan_on { do_exec_query(sql, 'EXPLAIN', binds) }
+          result = with_showplan_on { sp_executesql(sql, 'EXPLAIN', binds) }
           printer = showplan_printer.new(result)
           printer.pp
         end