Oracle: resolve test failures, use prefetched primary key for inserts, check for null defaults. Factor out some common methods from all adapters. Closes #6515.

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

12 files changed, 64 insertions, 117 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index 73e4b4fe9b..db50f823ae 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Oracle: resolve test failures, use prefetched primary key for inserts, check for null defaults. Factor out some common methods from all adapters.  #6515 [Michael Schoen]
+
 * Make add_column use the options hash with the Sqlite Adapter. Closes #6464 [obrie]
 
 * Document other options available to migration's add_column. #6419 [grg]
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
index d91b919116..7d7c22c74b 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
@@ -4,11 +4,14 @@ module ActiveRecord
       # Returns an array of record hashes with the column names as keys and
       # column values as values.
       def select_all(sql, name = nil)
+        select(sql, name)
       end
 
       # Returns a record hash with the column names as keys and column values
       # as values.
       def select_one(sql, name = nil)
+        result = select(sql, name)
+        result.first if result
       end
 
       # Returns a single value from a record
@@ -25,19 +28,24 @@ module ActiveRecord
       end
 
       # Executes the SQL statement in the context of this connection.
-      # This abstract method raises a NotImplementedError.
       def execute(sql, name = nil)
         raise NotImplementedError, "execute is an abstract method"
       end
 
       # Returns the last auto-generated ID from the affected table.
-      def insert(sql, name = nil, pk = nil, id_value = nil, sequence_name = nil) end
+      def insert(sql, name = nil, pk = nil, id_value = nil, sequence_name = nil)
+        raise NotImplementedError, "insert is an abstract method"
+      end
 
       # Executes the update statement and returns the number of rows affected.
-      def update(sql, name = nil) end
+      def update(sql, name = nil)
+        execute(sql, name)
+      end
 
       # Executes the delete statement and returns the number of rows affected.
-      def delete(sql, name = nil) end
+      def delete(sql, name = nil)
+        update(sql, name)
+      end
 
       # Wrap a block in a transaction.  Returns result of block.
       def transaction(start_db_transaction = true)
@@ -110,6 +118,13 @@ module ActiveRecord
       def reset_sequence!(table, column, sequence = nil)
         # Do nothing by default.  Implement for PostgreSQL, Oracle, ...
       end
+
+      protected
+        # Returns an array of record hashes with the column names as keys and
+        # column values as values.
+        def select(sql, name = nil)
+          raise NotImplementedError, "select is an abstract method"
+        end
     end
   end
 end
diff --git a/activerecord/lib/active_record/connection_adapters/db2_adapter.rb b/activerecord/lib/active_record/connection_adapters/db2_adapter.rb
index 3ff6bdfb98..7a3cd22789 100644
--- a/activerecord/lib/active_record/connection_adapters/db2_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/db2_adapter.rb
@@ -47,14 +47,6 @@ begin
             end
           end
         end
-        
-        def select_all(sql, name = nil)
-          select(sql, name)
-        end
-
-        def select_one(sql, name = nil)
-          select(sql, name).first
-        end
 
         def insert(sql, name = nil, pk = nil, id_value = nil, sequence_name = nil)
           execute(sql, name = nil)
@@ -72,9 +64,6 @@ begin
           rows_affected
         end
 
-        alias_method :update, :execute
-        alias_method :delete, :execute
-
         def begin_db_transaction
           @connection.set_auto_commit_off
         end
diff --git a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb
index 036092bbdf..d5e2348856 100755
--- a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb
@@ -217,16 +217,7 @@ module ActiveRecord
 
       # DATABASE STATEMENTS ======================================
 
-      def select_all(sql, name = nil) #:nodoc:
-        select(sql, name)
-      end
-
-      def select_one(sql, name = nil) #:nodoc:
-        result = select(sql, name)
-        result.nil? ? nil : result.first
-      end
-
-      def execute(sql, name = nil, retries = 2) #:nodoc:
+      def execute(sql, name = nil) #:nodoc:
         log(sql, name) { @connection.query(sql) }
       rescue ActiveRecord::StatementInvalid => exception
         if exception.message.split(":").first =~ /Packets out of order/
@@ -246,9 +237,6 @@ module ActiveRecord
         @connection.affected_rows
       end
 
-      alias_method :delete, :update #:nodoc:
-
-
       def begin_db_transaction #:nodoc:
         execute "BEGIN"
       rescue Exception
diff --git a/activerecord/lib/active_record/connection_adapters/oracle_adapter.rb b/activerecord/lib/active_record/connection_adapters/oracle_adapter.rb
index 2e5260d9be..bd67ea7d75 100644
--- a/activerecord/lib/active_record/connection_adapters/oracle_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/oracle_adapter.rb
@@ -41,28 +41,16 @@ begin
         self.oracle_connection(config)
       end
 
-      # Enable the id column to be bound into the sql later, by the adapter's insert method.
-      # This is preferable to inserting the hard-coded value here, because the insert method
-      # needs to know the id value explicitly.
-      alias :attributes_with_quotes_pre_oracle :attributes_with_quotes
-      def attributes_with_quotes(include_primary_key = true) #:nodoc:
-        aq = attributes_with_quotes_pre_oracle(include_primary_key)
-        if connection.class == ConnectionAdapters::OracleAdapter
-          aq[self.class.primary_key] = ":id" if include_primary_key && aq[self.class.primary_key].nil?
-        end
-        aq
-      end
-
       # After setting large objects to empty, select the OCI8::LOB
       # and write back the data.
-      after_save :write_lobs 
+      after_save :write_lobs
       def write_lobs() #:nodoc:
         if connection.is_a?(ConnectionAdapters::OracleAdapter)
           self.class.columns.select { |c| c.sql_type =~ /LOB$/i }.each { |c|
             value = self[c.name]
             next if value.nil?  || (value == '')
             lob = connection.select_one(
-              "SELECT #{c.name} FROM #{self.class.table_name} WHERE #{self.class.primary_key} = #{quote(id)}",
+              "SELECT #{c.name} FROM #{self.class.table_name} WHERE #{self.class.primary_key} = #{quote_value(id)}",
               'Writable Large Object')[c.name]
             lob.write value
           }
@@ -145,7 +133,7 @@ begin
         def supports_migrations? #:nodoc:
           true
         end
-        
+
         def native_database_types #:nodoc
           {
             :primary_key => "NUMBER(38) NOT NULL PRIMARY KEY",
@@ -192,7 +180,7 @@ begin
         def quoted_true
           "1"
         end
-      
+
         def quoted_false
           "0"
         end
@@ -204,7 +192,7 @@ begin
         # Returns true if the connection is active.
         def active?
           # Pings the connection to check if it's still good. Note that an
-          # #active? method is also available, but that simply returns the 
+          # #active? method is also available, but that simply returns the
           # last known state, which isn't good enough if the connection has
           # gone stale since the last use.
           @connection.ping
@@ -230,35 +218,24 @@ begin
         #
         # see: abstract/database_statements.rb
 
-        def select_all(sql, name = nil) #:nodoc:
-          select(sql, name)
-        end
-
-        def select_one(sql, name = nil) #:nodoc:
-          result = select_all(sql, name)
-          result.size > 0 ? result.first : nil
-        end
-
         def execute(sql, name = nil) #:nodoc:
           log(sql, name) { @connection.exec sql }
         end
 
-        def insert(sql, name = nil, pk = nil, id_value = nil, sequence_name = nil) #:nodoc:
-          if pk.nil? # Who called us? What does the sql look like? No idea!
-            execute sql, name
-          elsif id_value # Pre-assigned id
-            log(sql, name) { @connection.exec sql }
-          else # Assume the sql contains a bind-variable for the id
-            id_value = select_one("select #{sequence_name}.nextval id from dual")['id'].to_i
-            log(sql.sub(/\B:id\b/, id_value.to_s), name) { @connection.exec sql, id_value }
-          end
+        # Returns the next sequence value from a sequence generator. Not generally
+        # called directly; used by ActiveRecord to get the next primary key value
+        # when inserting a new database record (see #prefetch_primary_key?).
+        def next_sequence_value(sequence_name)
+          id = 0
+          @connection.exec("select #{sequence_name}.nextval id from dual") { |r| id = r[0].to_i }
+          id
+        end
 
+        def insert(sql, name = nil, pk = nil, id_value = nil, sequence_name = nil) #:nodoc:
+          execute(sql, name)
           id_value
         end
 
-        alias :update :execute #:nodoc:
-        alias :delete :execute #:nodoc:
-
         def begin_db_transaction #:nodoc:
           @connection.autocommit = false
         end
@@ -285,6 +262,12 @@ begin
           end
         end
 
+        # Returns true for Oracle adapter (since Oracle requires primary key
+        # values to be pre-fetched before insert). See also #next_sequence_value.
+        def prefetch_primary_key?(table_name = nil)
+          true
+        end
+
         def default_sequence_name(table, column) #:nodoc:
           "#{table}_seq"
         end
@@ -331,6 +314,7 @@ begin
 
         def columns(table_name, name = nil) #:nodoc:
           (owner, table_name) = @connection.describe(table_name)
+          raise "Couldn't describe #{table_name}. Does it exist?" unless owner and table_name
 
           table_cols = %Q{
             select column_name as name, data_type as sql_type, data_default, nullable,
@@ -355,6 +339,7 @@ begin
             if row['data_default']
               row['data_default'].sub!(/^(.*?)\s*$/, '\1')
               row['data_default'].sub!(/^'(.*)'$/, '\1')
+              row['data_default'] = nil if row['data_default'] =~ /^null$/i
             end
 
             OracleColumn.new(oracle_downcase(row['name']),
@@ -372,7 +357,7 @@ begin
         def rename_table(name, new_name) #:nodoc:
           execute "RENAME #{name} TO #{new_name}"
           execute "RENAME #{name}_seq TO #{new_name}_seq" rescue nil
-        end  
+        end
 
         def drop_table(name) #:nodoc:
           super(name)
@@ -407,20 +392,20 @@ begin
           end
 
           select_all("select table_name from user_tables").inject(s) do |structure, table|
-            ddl = "create table #{table.to_a.first.last} (\n "  
+            ddl = "create table #{table.to_a.first.last} (\n "
             cols = select_all(%Q{
               select column_name, data_type, data_length, data_precision, data_scale, data_default, nullable
               from user_tab_columns
               where table_name = '#{table.to_a.first.last}'
               order by column_id
-            }).map do |row|              
-              col = "#{row['column_name'].downcase} #{row['data_type'].downcase}"      
+            }).map do |row|
+              col = "#{row['column_name'].downcase} #{row['data_type'].downcase}"
               if row['data_type'] =='NUMBER' and !row['data_precision'].nil?
                 col << "(#{row['data_precision'].to_i}"
                 col << ",#{row['data_scale'].to_i}" if !row['data_scale'].nil?
                 col << ')'
               elsif row['data_type'].include?('CHAR')
-                col << "(#{row['data_length'].to_i})"  
+                col << "(#{row['data_length'].to_i})"
               end
               col << " default #{row['data_default']}" if !row['data_default'].nil?
               col << ' not null' if row['nullable'] == 'N'
@@ -518,7 +503,7 @@ begin
     def describe(name)
       @desc ||= @@env.alloc(OCIDescribe)
       @desc.attrSet(OCI_ATTR_DESC_PUBLIC, -1) if VERSION >= '0.1.14'
-      @desc.describeAny(@svc, name.to_s, OCI_PTYPE_UNK)
+      @desc.describeAny(@svc, name.to_s, OCI_PTYPE_UNK) rescue return nil
       info = @desc.attrGet(OCI_ATTR_PARAM)
 
       case info.attrGet(OCI_ATTR_PTYPE)
@@ -552,10 +537,10 @@ begin
 
   # The OCI8AutoRecover class enhances the OCI8 driver with auto-recover and
   # reset functionality. If a call to #exec fails, and autocommit is turned on
-  # (ie., we're not in the middle of a longer transaction), it will 
+  # (ie., we're not in the middle of a longer transaction), it will
   # automatically reconnect and try again. If autocommit is turned off,
   # this would be dangerous (as the earlier part of the implied transaction
-  # may have failed silently if the connection died) -- so instead the 
+  # may have failed silently if the connection died) -- so instead the
   # connection is marked as dead, to be reconnected on it's next use.
   class OCI8AutoRecover < DelegateClass(OCI8) #:nodoc:
     attr_accessor :active
@@ -601,7 +586,7 @@ begin
     end
 
     # ORA-00028: your session has been killed
-    # ORA-01012: not logged on 
+    # ORA-01012: not logged on
     # ORA-03113: end-of-file on communication channel
     # ORA-03114: not connected to ORACLE
     LOST_CONNECTION_ERROR_CODES = [ 28, 1012, 3113, 3114 ]
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
index e5e098a4e1..e16af71762 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -136,15 +136,6 @@ module ActiveRecord
 
       # DATABASE STATEMENTS ======================================
 
-      def select_all(sql, name = nil) #:nodoc:
-        select(sql, name)
-      end
-
-      def select_one(sql, name = nil) #:nodoc:
-        result = select(sql, name)
-        result.first if result
-      end
-
       def insert(sql, name = nil, pk = nil, id_value = nil, sequence_name = nil) #:nodoc:
         execute(sql, name)
         table = sql.split(" ", 4)[2]
@@ -175,9 +166,6 @@ module ActiveRecord
         execute(sql, name).cmdtuples
       end
 
-      alias_method :delete, :update #:nodoc:
-
-
       def begin_db_transaction #:nodoc:
         execute "BEGIN"
       end
diff --git a/activerecord/lib/active_record/connection_adapters/sqlserver_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlserver_adapter.rb
index ff77c382d2..3ecf26f530 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlserver_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlserver_adapter.rb
@@ -253,16 +253,6 @@ module ActiveRecord
         @connection.disconnect rescue nil
       end
 
-      def select_all(sql, name = nil)
-        select(sql, name)
-      end
-
-      def select_one(sql, name = nil)
-        add_limit!(sql, :limit => 1)
-        result = select(sql, name)
-        result.nil? ? nil : result.first
-      end
-
       def columns(table_name, name = nil)
         return [] if table_name.blank?
         table_name = table_name.to_s if table_name.is_a?(Symbol)
@@ -331,7 +321,7 @@ module ActiveRecord
           end
         end
       end
-      
+
       def begin_db_transaction
         @connection["AutoCommit"] = false
       rescue Exception => e
diff --git a/activerecord/lib/active_record/connection_adapters/sybase_adapter.rb b/activerecord/lib/active_record/connection_adapters/sybase_adapter.rb
index dc1cce451a..b8a32de2f8 100644
--- a/activerecord/lib/active_record/connection_adapters/sybase_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/sybase_adapter.rb
@@ -170,15 +170,6 @@ module ActiveRecord
         30
       end
 
-      def select_all(sql, name = nil)
-        select(sql, name)
-      end
-
-      def select_one(sql, name = nil)
-        result = select(sql, name)
-        result.nil? ? nil : result.first
-      end
-
       def columns(table_name, name = nil)
         table_structure(table_name).inject([]) do |columns, column|
           name, default, type, nullable, identity, primary = column
@@ -234,9 +225,6 @@ module ActiveRecord
         @connection.results[0].row_count
       end
 
-      alias_method :update, :execute
-      alias_method :delete, :execute
-
       def begin_db_transaction()    execute "BEGIN TRAN" end
       def commit_db_transaction()   execute "COMMIT TRAN" end
       def rollback_db_transaction() execute "ROLLBACK TRAN" end
diff --git a/activerecord/test/associations/eager_test.rb b/activerecord/test/associations/eager_test.rb
index 909083fd28..e8e14c2c50 100644
--- a/activerecord/test/associations/eager_test.rb
+++ b/activerecord/test/associations/eager_test.rb
@@ -121,10 +121,11 @@ class EagerAssociationTest < Test::Unit::TestCase
     author = Author.find(:first, :include => :special_nonexistant_post_comments, :order => 'authors.id')
     assert_equal [], author.special_nonexistant_post_comments
   end
-  
+
   def test_eager_with_has_many_through_join_model_with_conditions
-    assert_equal Author.find(:first, :include => :hello_post_comments).hello_post_comments,
-      Author.find(:first).hello_post_comments
+    assert_equal Author.find(:first, :include => :hello_post_comments,
+                             :order => 'authors.id').hello_post_comments.sort_by(&:id),
+                 Author.find(:first, :order => 'authors.id').hello_post_comments.sort_by(&:id)
   end
 
   def test_eager_with_has_many_and_limit
diff --git a/activerecord/test/associations_test.rb b/activerecord/test/associations_test.rb
index d033ef087a..0be63fccbf 100755
--- a/activerecord/test/associations_test.rb
+++ b/activerecord/test/associations_test.rb
@@ -1782,7 +1782,7 @@ class HasAndBelongsToManyAssociationsTest < Test::Unit::TestCase
     assert_equal projects(:active_record), developer.projects[0] 
     assert_equal projects(:action_controller), developer.projects[1] 
   end
-  
+
   def test_select_limited_ids_list
     # Set timestamps
     Developer.transaction do
@@ -1790,10 +1790,10 @@ class HasAndBelongsToManyAssociationsTest < Test::Unit::TestCase
         record.update_attributes(:created_at => 5.years.ago + (i * 5.minutes))
       end
     end
-    
+
     join_base = ActiveRecord::Associations::ClassMethods::JoinDependency::JoinBase.new(Project)
     join_dep  = ActiveRecord::Associations::ClassMethods::JoinDependency.new(join_base, :developers, nil)
     projects  = Project.send(:select_limited_ids_list, {:order => 'developers.created_at'}, join_dep)
-    assert_equal "'1', '2'", projects
+    assert_equal %w(1 2), projects.scan(/\d/).sort
   end
 end
diff --git a/activerecord/test/fixtures/db_definitions/schema.rb b/activerecord/test/fixtures/db_definitions/schema.rb
index 6f4aca971e..9c851817ec 100644
--- a/activerecord/test/fixtures/db_definitions/schema.rb
+++ b/activerecord/test/fixtures/db_definitions/schema.rb
@@ -54,7 +54,7 @@ ActiveRecord::Schema.define do
     t.column :lock_version, :integer
   end
   
-  create_table :lock_with_custom_column_without_defaults, :force => true do |t|
+  create_table :lock_without_defaults_cust, :force => true do |t|
     t.column :custom_lock_version, :integer
   end
 end
diff --git a/activerecord/test/locking_test.rb b/activerecord/test/locking_test.rb
index 80f0dc8496..b5bec031e2 100644
--- a/activerecord/test/locking_test.rb
+++ b/activerecord/test/locking_test.rb
@@ -5,6 +5,7 @@ require 'fixtures/legacy_thing'
 class LockWithoutDefault < ActiveRecord::Base; end
 
 class LockWithCustomColumnWithoutDefault < ActiveRecord::Base
+  set_table_name :lock_without_defaults_cust
   set_locking_column :custom_lock_version
 end