Sybase Adapter type conversion cleanup [dev@metacasa.net]

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@4270 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

3 files changed, 69 insertions, 74 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index a07b82632d..3155de959f 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Sybase Adapter type conversion cleanup. Closes #4736. [dev@metacasa.net]
+
 * Fix bug where calculations with long alias names return null. [Rick]
 
 * Raise error when trying to add to a has_many :through association.  Use the Join Model instead. [Rick]
diff --git a/activerecord/lib/active_record/connection_adapters/sybase_adapter.rb b/activerecord/lib/active_record/connection_adapters/sybase_adapter.rb
index 33dbebce7d..3272e6d598 100644
--- a/activerecord/lib/active_record/connection_adapters/sybase_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/sybase_adapter.rb
@@ -1,11 +1,13 @@
 # sybase_adaptor.rb
 # Author: John Sheets <dev@metacasa.net>
-# Date:   01 Mar 2006
+# 
+# 01 Mar 2006: Initial version.
+# 17 Mar 2006: Added support for migrations; fixed issues with :boolean columns.
+# 13 Apr 2006: Improved column type support to properly handle dates and user-defined
+#              types; fixed quoting of integer columns.
 #
 # Based on code from Will Sobel (http://dev.rubyonrails.org/ticket/2030)
 #
-# 17 Mar 2006: Added support for migrations; fixed issues with :boolean columns.
-#
 
 require 'active_record/connection_adapters/abstract_adapter'
 
@@ -35,7 +37,7 @@ module ActiveRecord
 
       ConnectionAdapters::SybaseAdapter.new(
         SybSQL.new({'S' => host, 'U' => username, 'P' => password},
-          ConnectionAdapters::SybaseAdapterContext), database, logger)
+          ConnectionAdapters::SybaseAdapterContext), database, config, logger)
     end
   end # class Base
 
@@ -48,7 +50,7 @@ module ActiveRecord
     #
     # * <tt>:host</tt> -- The name of the database server. No default, must be provided.
     # * <tt>:database</tt> -- The name of the database. No default, must be provided.
-    # * <tt>:username</tt>  -- Defaults to sa.
+    # * <tt>:username</tt>  -- Defaults to "sa".
     # * <tt>:password</tt>  -- Defaults to empty string.
     #
     # Usage Notes:
@@ -95,6 +97,16 @@ module ActiveRecord
           end
         end
 
+        def self.string_to_time(string)
+          return string unless string.is_a?(String)
+
+          # Since Sybase doesn't handle DATE or TIME, handle it here.
+          # Populate nil year/month/day with string_to_dummy_time() values.
+          time_array = ParseDate.parsedate(string)[0..5]
+          time_array[0] ||= 2000; time_array[1] ||= 1; time_array[2] ||= 1;
+          Time.send(Base.default_timezone, *time_array) rescue nil
+        end
+
         def self.string_to_binary(value)
           "0x#{value.unpack("H*")[0]}"
         end
@@ -106,10 +118,12 @@ module ActiveRecord
       end # class ColumnWithIdentity
 
       # Sybase adapter
-      def initialize(connection, database, logger = nil)
+      def initialize(connection, database, config = {}, logger = nil)
         super(connection, logger)
         context = connection.context
         context.init(logger)
+        @config = config
+        @numconvert = config.has_key?(:numconvert) ? config[:numconvert] : true
         @limit = @offset = 0
         unless connection.sql_norow("USE #{database}")
           raise "Cannot USE #{database}"
@@ -154,16 +168,10 @@ module ActiveRecord
         30
       end
 
-      # Check for a limit statement and parse out the limit and
-      # offset if specified. Remove the limit from the sql statement
-      # and call select.
       def select_all(sql, name = nil)
         select(sql, name)
       end
 
-      # Remove limit clause from statement. This will almost always
-      # contain LIMIT 1 from the caller. set the rowcount to 1 before
-      # calling select.
       def select_one(sql, name = nil)
         result = select(sql, name)
         result.nil? ? nil : result.first
@@ -186,7 +194,7 @@ module ActiveRecord
           if col != nil
             if query_contains_identity_column(sql, col)
               begin
-                execute enable_identity_insert(table_name, true)
+                enable_identity_insert(table_name, true)
                 ii_enabled = true
               rescue Exception => e
                 raise ActiveRecordError, "IDENTITY_INSERT could not be turned ON"
@@ -202,7 +210,7 @@ module ActiveRecord
         ensure
           if ii_enabled
             begin
-              execute enable_identity_insert(table_name, false)
+              enable_identity_insert(table_name, false)
             rescue Exception => e
               raise ActiveRecordError, "IDENTITY_INSERT could not be turned OFF"
             end
@@ -231,6 +239,10 @@ module ActiveRecord
       def commit_db_transaction()   execute "COMMIT TRAN" end
       def rollback_db_transaction() execute "ROLLBACK TRAN" end
 
+      def current_database
+        select_one("select DB_NAME() as name")["name"]
+      end
+
       def tables(name = nil)
         tables = []
         select("select name from sysobjects where type='U'", name).each do |row|
@@ -265,7 +277,7 @@ module ActiveRecord
           when String                
             if column && column.type == :binary && column.class.respond_to?(:string_to_binary)
               "#{quote_string(column.class.string_to_binary(value))}"
-            elsif value =~ /^[+-]?[0-9]+$/o
+            elsif @numconvert && force_numeric?(column) && value =~ /^[+-]?[0-9]+$/o
               value
             else
               "'#{quote_string(value)}'"
@@ -273,39 +285,18 @@ module ActiveRecord
           when NilClass              then (column && column.type == :boolean) ? '0' : "NULL"
           when TrueClass             then '1'
           when FalseClass            then '0'
-          when Float, Fixnum, Bignum then value.to_s
+          when Float, Fixnum, Bignum
+            force_numeric?(column) ? value.to_s : "'#{value.to_s}'"
           when Date                  then "'#{value.to_s}'" 
           when Time, DateTime        then "'#{value.strftime("%Y-%m-%d %H:%M:%S")}'"
           else                            "'#{quote_string(value.to_yaml)}'"
         end
       end
 
-      def quote_column(type, value)
-        case type
-          when :boolean
-            case value
-              when String then value =~ /^[ty]/o ? 1 : 0
-              when true   then 1
-              when false  then 0
-              else             value.to_i
-            end
-          when :integer then value.to_i
-          when :float   then value.to_f
-          when :text, :string, :enum
-            case value
-              when String, Symbol, Fixnum, Float, Bignum, TrueClass, FalseClass 
-                "'#{quote_string(value.to_s)}'"
-              else
-                "'#{quote_string(value.to_yaml)}'"
-            end
-          when :date, :datetime, :time
-            case value
-              when Time, DateTime then "'#{value.strftime("%Y-%m-%d %H:%M:%S")}'"
-              when Date           then "'#{value.to_s}'"
-              else                     "'#{quote_string(value)}'"
-            end
-          else "'#{quote_string(value.to_yaml)}'"
-        end
+      # True if column is explicitly declared non-numeric, or
+      # if column is nil (not specified).
+      def force_numeric?(column)
+        (column.nil? || [:integer, :float].include?(column.type))
       end
 
       def quote_string(s)
@@ -313,7 +304,9 @@ module ActiveRecord
       end
 
       def quote_column_name(name)
-        "[#{name}]"
+        # If column name is close to max length, skip the quotes, since they
+        # seem to count as part of the length.
+        ((name.to_s.length + 2) <= table_alias_length) ? "[#{name}]" : name.to_s
       end
 
       def add_limit_offset!(sql, options) # :nodoc:
@@ -348,12 +341,17 @@ module ActiveRecord
       end
 
       def change_column(table_name, column_name, type, options = {}) #:nodoc:
-        sql_commands = ["ALTER TABLE #{table_name} MODIFY #{column_name} #{type_to_sql(type, options[:limit])}"]
+        begin
+          execute "ALTER TABLE #{table_name} MODIFY #{column_name} #{type_to_sql(type, options[:limit])}"
+        rescue StatementInvalid => e
+          # Swallow exception if no-op.
+          raise e unless e.message =~ /no columns to drop, add or modify/
+        end
+
         if options[:default]
           remove_default_constraint(table_name, column_name)
-          sql_commands << "ALTER TABLE #{table_name} ADD CONSTRAINT DF_#{table_name}_#{column_name} DEFAULT #{options[:default]} FOR #{column_name}"
+          execute "ALTER TABLE #{table_name} REPLACE #{column_name} DEFAULT #{options[:default]}"
         end
-        sql_commands.each { |c| execute(c) }
       end
 
       def remove_column(table_name, column_name)
@@ -381,6 +379,12 @@ module ActiveRecord
         sql
       end
 
+      def enable_identity_insert(table_name, enable = true)
+        if has_identity_column(table_name)
+          execute "SET IDENTITY_INSERT #{table_name} #{enable ? 'ON' : 'OFF'}"
+        end
+      end
+
     private
       def check_null_for_column?(col, sql)
         # Sybase columns are NOT NULL by default, so explicitly set NULL
@@ -426,7 +430,7 @@ module ActiveRecord
 
       def normal_select?
         # If limit is not set at all, we can ignore offset;
-        # If limit *is* set but offset is zero, use normal select
+        # if limit *is* set but offset is zero, use normal select
         # with simple SET ROWCOUNT.  Thus, only use the temp table
         # if limit is set and offset > 0.
         has_limit = !@limit.nil?
@@ -468,7 +472,7 @@ module ActiveRecord
         else
           results = @connection.top_row_result
           if results && results.rows.length > 0
-            fields = fixup_column_names(results.columns)
+            fields = results.columns.map { |column| column.sub(/_$/, '') }
             results.rows.each do |row|
               hashed_row = {}
               row.zip(fields) { |cell, column| hashed_row[column] = cell }
@@ -481,12 +485,6 @@ module ActiveRecord
         return rows
       end
 
-      def enable_identity_insert(table_name, enable = true)
-        if has_identity_column(table_name)
-          "SET IDENTITY_INSERT #{table_name} #{enable ? 'ON' : 'OFF'}"
-        end
-      end
-
       def get_table_name(sql)
         if sql =~ /^\s*insert\s+into\s+([^\(\s]+)\s*|^\s*update\s+([^\(\s]+)\s*/i
           $1
@@ -502,24 +500,18 @@ module ActiveRecord
       end
 
       def get_identity_column(table_name)
-        @table_columns = {} unless @table_columns
-        @table_columns[table_name] = columns(table_name) if @table_columns[table_name] == nil
+        @table_columns ||= {}
+        @table_columns[table_name] ||= columns(table_name)
         @table_columns[table_name].each do |col|
           return col.name if col.identity
         end
-
-        return nil
+        nil
       end
 
       def query_contains_identity_column(sql, col)
         sql =~ /\[#{col}\]/
       end
 
-      # Remove trailing _ from names.
-      def fixup_column_names(columns)
-        columns.map { |column| column.sub(/_$/, '') }
-      end
-
       def table_structure(table_name)
         sql = <<SQLTEXT
 SELECT col.name AS name, type.name AS type, col.prec, col.scale, col.length,
@@ -558,18 +550,23 @@ SQLTEXT
         end
       end
 
+      # Resolve all user-defined types (udt) to their fundamental types.
+      def resolve_type(field_type)
+        (@udts ||= {})[field_type] ||= select_one("sp_help #{field_type}")["Storage_type"].strip
+      end
+
       def normalize_type(field_type, prec, scale, length)
         if field_type =~ /numeric/i and (scale.nil? or scale == 0)
           type = 'int'
         elsif field_type =~ /money/i
           type = 'numeric'
         else
-          type = field_type
+          type = resolve_type(field_type.strip)
         end
         size = ''
         if prec
           size = "(#{prec})"
-        elsif length
+        elsif length && !(type =~ /date|time/)
           size = "(#{length})"
         end
         return type + size
@@ -668,17 +665,13 @@ class Fixtures
 
   def insert_fixtures
     values.each do |fixture|
-      allow_identity_inserts table_name, true
+      @connection.enable_identity_insert(table_name, true)
       @connection.execute "INSERT INTO #{@table_name} (#{fixture.key_list}) VALUES (#{fixture.value_list})", 'Fixture Insert'
-      allow_identity_inserts table_name, false
+      @connection.enable_identity_insert(table_name, false)
     end
   end
-
-  def allow_identity_inserts(table_name, enable)
-      @connection.execute "SET IDENTITY_INSERT #{table_name} #{enable ? 'ON' : 'OFF'}" rescue nil
-  end
 end
 
 rescue LoadError => cannot_require_sybase
   # Couldn't load sybase adapter
-end
\ No newline at end of file
+end
diff --git a/activerecord/test/fixtures/db_definitions/sybase.sql b/activerecord/test/fixtures/db_definitions/sybase.sql
index 07f670867e..98022f8887 100644
--- a/activerecord/test/fixtures/db_definitions/sybase.sql
+++ b/activerecord/test/fixtures/db_definitions/sybase.sql
@@ -26,7 +26,7 @@ CREATE TABLE topics (
   author_name varchar(255) NULL,
   author_email_address varchar(255) NULL,
   written_on datetime NULL,
-  bonus_time time NULL,
+  bonus_time datetime NULL,
   last_read datetime NULL,
   content varchar(255) NULL,
   approved bit default 1,
@@ -118,7 +118,7 @@ CREATE TABLE mixins (
 
 CREATE TABLE people (
   id numeric(9,0) IDENTITY PRIMARY KEY,
-  first_name varchar(40) NOT NULL,
+  first_name varchar(40) NULL,
   lock_version int DEFAULT 0
 )