Merge branch 'schema'

* schema:
  add uuid primary key support
  separate primary key from column type
  push the mysql add_column up to the abstract adapter
  allow multiple add columns
  pull add_column_options! off the pg connection class
  add a pg visitor for dealing with schema modification
  push alter table add column sql in to the schema modification visitor
  there is no reason to check for an already defined column
  push column initialization down to the factory method
  @columns list is no longer necessary
  keep ivars private, do not manipulate them outside their owner object
  factory methods should not alter object state
  push SQL generation inside the schema creation object
  mostly decouple TableDefinition from the database connection
  remove to_sql from TableDefinition
  decouple column definition from the database connection
  stop depending on sql_type in pg
  remove knowledge of SQL from the column definition object

10 files changed, 230 insertions, 104 deletions

diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
index 902dbd148e..b358d84365 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
@@ -15,34 +15,14 @@ module ActiveRecord
     # are typically created by methods in TableDefinition, and added to the
     # +columns+ attribute of said TableDefinition object, in order to be used
     # for generating a number of table creation or table changing SQL statements.
-    class ColumnDefinition < Struct.new(:base, :name, :type, :limit, :precision, :scale, :default, :null) #:nodoc:
+    class ColumnDefinition < Struct.new(:name, :type, :limit, :precision, :scale, :default, :null, :first, :after, :primary_key) #:nodoc:
       def string_to_binary(value)
         value
       end
 
-      def sql_type
-        base.type_to_sql(type.to_sym, limit, precision, scale)
-      end
-
       def primary_key?
-        type.to_sym == :primary_key
-      end
-
-      def to_sql
-        column_sql = "#{base.quote_column_name(name)} #{sql_type}"
-        column_options = {}
-        column_options[:null] = null unless null.nil?
-        column_options[:default] = default unless default.nil?
-        column_options[:column] = self
-        add_column_options!(column_sql, column_options) unless primary_key?
-        column_sql
+        primary_key || type.to_sym == :primary_key
       end
-
-      private
-
-        def add_column_options!(sql, options)
-          base.add_column_options!(sql, options)
-        end
     end
 
     # Represents the schema of an SQL table in an abstract way. This class
@@ -68,19 +48,25 @@ module ActiveRecord
     class TableDefinition
       # An array of ColumnDefinition objects, representing the column changes
       # that have been defined.
-      attr_accessor :columns, :indexes
+      attr_accessor :indexes
+      attr_reader :name, :temporary, :options
 
-      def initialize(base)
-        @columns = []
+      def initialize(types, name, temporary, options)
         @columns_hash = {}
         @indexes = {}
-        @base = base
+        @native = types
+        @temporary = temporary
+        @options = options
+        @name = name
       end
 
+      def columns; @columns_hash.values; end
+
       # Appends a primary key definition to the table definition.
       # Can be called multiple times, but this is probably not a good idea.
-      def primary_key(name)
-        column(name, :primary_key)
+      def primary_key(name, type = :primary_key, options = {})
+        options[:primary_key] = true
+        column(name, type, options)
       end
 
       # Returns a ColumnDefinition for the column with name +name+.
@@ -233,20 +219,14 @@ module ActiveRecord
           raise ArgumentError, "you can't redefine the primary key column '#{name}'. To define a custom primary key, pass { id: false } to create_table."
         end
 
-        column = self[name] || new_column_definition(@base, name, type)
-
-        limit = options.fetch(:limit) do
-          native[type][:limit] if native[type].is_a?(Hash)
-        end
-
-        column.limit     = limit
-        column.precision = options[:precision]
-        column.scale     = options[:scale]
-        column.default   = options[:default]
-        column.null      = options[:null]
+        @columns_hash[name] = new_column_definition(name, type, options)
         self
       end
 
+      def remove_column(name)
+        @columns_hash.delete name.to_s
+      end
+
       [:string, :text, :integer, :float, :decimal, :datetime, :timestamp, :time, :date, :binary, :boolean].each do |column_type|
         define_method column_type do |*args|
           options = args.extract_options!
@@ -283,23 +263,26 @@ module ActiveRecord
       end
       alias :belongs_to :references
 
-      # Returns a String whose contents are the column definitions
-      # concatenated together. This string can then be prepended and appended to
-      # to generate the final SQL to create the table.
-      def to_sql
-        columns.map { |c| c.to_sql } * ', '
-      end
+      def new_column_definition(name, type, options) # :nodoc:
+        column = create_column_definition name, type
+        limit = options.fetch(:limit) do
+          native[type][:limit] if native[type].is_a?(Hash)
+        end
 
-      private
-      def create_column_definition(base, name, type)
-        ColumnDefinition.new base, name, type
+        column.limit       = limit
+        column.precision   = options[:precision]
+        column.scale       = options[:scale]
+        column.default     = options[:default]
+        column.null        = options[:null]
+        column.first       = options[:first]
+        column.after       = options[:after]
+        column.primary_key = type == :primary_key || options[:primary_key]
+        column
       end
 
-      def new_column_definition(base, name, type)
-        definition = create_column_definition base, name, type
-        @columns << definition
-        @columns_hash[name] = definition
-        definition
+      private
+      def create_column_definition(name, type)
+        ColumnDefinition.new name, type
       end
 
       def primary_key_column_name
@@ -308,7 +291,24 @@ module ActiveRecord
       end
 
       def native
-        @base.native_database_types
+        @native
+      end
+    end
+
+    class AlterTable # :nodoc:
+      attr_reader :adds
+
+      def initialize(td)
+        @td   = td
+        @adds = []
+      end
+
+      def name; @td.name; end
+
+      def add_column(name, type, options)
+        name = name.to_s
+        type = type.to_sym
+        @adds << @td.new_column_definition(name, type, options)
       end
     end
 
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
index cd4409295f..a26a0b54ea 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -171,14 +171,14 @@ module ActiveRecord
       #
       # See also TableDefinition#column for details on how to create columns.
       def create_table(table_name, options = {})
-        td = create_table_definition
+        td = create_table_definition table_name, options[:temporary], options[:options]
 
         unless options[:id] == false
           pk = options.fetch(:primary_key) {
             Base.get_primary_key table_name.to_s.singularize
           }
 
-          td.primary_key pk
+          td.primary_key pk, options.fetch(:id, :primary_key), options
         end
 
         yield td if block_given?
@@ -187,11 +187,7 @@ module ActiveRecord
           drop_table(table_name, options)
         end
 
-        create_sql = "CREATE#{' TEMPORARY' if options[:temporary]} TABLE "
-        create_sql << "#{quote_table_name(table_name)} ("
-        create_sql << td.to_sql
-        create_sql << ") #{options[:options]}"
-        execute create_sql
+        execute schema_creation.accept td
         td.indexes.each_pair { |c,o| add_index table_name, c, o }
       end
 
@@ -359,9 +355,9 @@ module ActiveRecord
       # Adds a new column to the named table.
       # See TableDefinition#column for details of the options you can use.
       def add_column(table_name, column_name, type, options = {})
-        add_column_sql = "ALTER TABLE #{quote_table_name(table_name)} ADD #{quote_column_name(column_name)} #{type_to_sql(type, options[:limit], options[:precision], options[:scale])}"
-        add_column_options!(add_column_sql, options)
-        execute(add_column_sql)
+        at = create_alter_table table_name
+        at.add_column(column_name, type, options)
+        execute schema_creation.accept at
       end
 
       # Removes the given columns from the table definition.
@@ -829,8 +825,12 @@ module ActiveRecord
         end
 
       private
-      def create_table_definition
-        TableDefinition.new(self)
+      def create_table_definition(name, temporary, options)
+        TableDefinition.new native_database_types, name, temporary, options
+      end
+
+      def create_alter_table(name)
+        AlterTable.new create_table_definition(name, false, {})
       end
 
       def update_table_definition(table_name, base)
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
index 7949bcb5ce..0ebe46be55 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
@@ -100,6 +100,75 @@ module ActiveRecord
         @visitor             = nil
       end
 
+      class SchemaCreation
+        def initialize(conn)
+          @conn  = conn
+          @cache = {}
+        end
+
+        def accept(o)
+          m = @cache[o.class] ||= "visit_#{o.class.name.split('::').last}"
+          send m, o
+        end
+
+        private
+
+        def visit_AlterTable(o)
+          sql = "ALTER TABLE #{quote_table_name(o.name)} "
+          sql << o.adds.map { |col| visit_AddColumn col }.join(' ')
+        end
+
+        def visit_AddColumn(o)
+          sql_type = type_to_sql(o.type.to_sym, o.limit, o.precision, o.scale)
+          sql = "ADD #{quote_column_name(o.name)} #{sql_type}"
+          add_column_options!(sql, column_options(o))
+        end
+
+        def visit_ColumnDefinition(o)
+          sql_type = type_to_sql(o.type.to_sym, o.limit, o.precision, o.scale)
+          column_sql = "#{quote_column_name(o.name)} #{sql_type}"
+          add_column_options!(column_sql, column_options(o)) unless o.primary_key?
+          column_sql
+        end
+
+        def visit_TableDefinition(o)
+          create_sql = "CREATE#{' TEMPORARY' if o.temporary} TABLE "
+          create_sql << "#{quote_table_name(o.name)} ("
+          create_sql << o.columns.map { |c| accept c }.join(', ')
+          create_sql << ") #{o.options}"
+          create_sql
+        end
+
+        def column_options(o)
+          column_options = {}
+          column_options[:null] = o.null unless o.null.nil?
+          column_options[:default] = o.default unless o.default.nil?
+          column_options[:column] = o
+          column_options
+        end
+
+        def quote_column_name(name)
+          @conn.quote_column_name name
+        end
+
+        def quote_table_name(name)
+          @conn.quote_table_name name
+        end
+
+        def type_to_sql(type, limit, precision, scale)
+          @conn.type_to_sql type.to_sym, limit, precision, scale
+        end
+
+        def add_column_options!(column_sql, column_options)
+          @conn.add_column_options! column_sql, column_options
+          column_sql
+        end
+      end
+
+      def schema_creation
+        SchemaCreation.new self
+      end
+
       def lease
         synchronize do
           unless in_use
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
index f88f5742a8..b1baf24cb6 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -3,6 +3,27 @@ require 'arel/visitors/bind_visitor'
 module ActiveRecord
   module ConnectionAdapters
     class AbstractMysqlAdapter < AbstractAdapter
+      class SchemaCreation < AbstractAdapter::SchemaCreation
+        private
+
+        def visit_AddColumn(o)
+          add_column_position!(super, o)
+        end
+
+        def add_column_position!(sql, column)
+          if column.first
+            sql << " FIRST"
+          elsif column.after
+            sql << " AFTER #{quote_column_name(column.after)}"
+          end
+          sql
+        end
+      end
+
+      def schema_creation
+        SchemaCreation.new self
+      end
+
       class Column < ConnectionAdapters::Column # :nodoc:
         attr_reader :collation, :strict
 
@@ -459,10 +480,6 @@ module ActiveRecord
         rename_table_indexes(table_name, new_name)
       end
 
-      def add_column(table_name, column_name, type, options = {})
-        execute("ALTER TABLE #{quote_table_name(table_name)} #{add_column_sql(table_name, column_name, type, options)}")
-      end
-
       def change_column_default(table_name, column_name, default)
         column = column_for(table_name, column_name)
         change_column table_name, column_name, column.sql_type, :default => default
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb b/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb
index 43f991b362..49b93e5626 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb
@@ -18,10 +18,12 @@ module ActiveRecord
         def quote(value, column = nil) #:nodoc:
           return super unless column
 
+          sql_type = type_to_sql(column.type, column.limit, column.precision, column.scale)
+
           case value
           when Range
-            if /range$/ =~ column.sql_type
-              "'#{PostgreSQLColumn.range_to_string(value)}'::#{column.sql_type}"
+            if /range$/ =~ sql_type
+              "'#{PostgreSQLColumn.range_to_string(value)}'::#{sql_type}"
             else
               super
             end
@@ -32,13 +34,13 @@ module ActiveRecord
               super
             end
           when Hash
-            case column.sql_type
+            case sql_type
             when 'hstore' then super(PostgreSQLColumn.hstore_to_string(value), column)
             when 'json' then super(PostgreSQLColumn.json_to_string(value), column)
             else super
             end
           when IPAddr
-            case column.sql_type
+            case sql_type
             when 'inet', 'cidr' then super(PostgreSQLColumn.cidr_to_string(value), column)
             else super
             end
@@ -51,14 +53,14 @@ module ActiveRecord
               super
             end
           when Numeric
-            if column.sql_type == 'money' || [:string, :text].include?(column.type)
+            if sql_type == 'money' || [:string, :text].include?(column.type)
               # Not truly string input, so doesn't require (or allow) escape string syntax.
               "'#{value}'"
             else
               super
             end
           when String
-            case column.sql_type
+            case sql_type
             when 'bytea' then "'#{escape_bytea(value)}'"
             when 'xml'   then "xml '#{quote_string(value)}'"
             when /^bit/
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
index 3bc61c5e0c..0622d0a909 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
@@ -1,6 +1,42 @@
 module ActiveRecord
   module ConnectionAdapters
     class PostgreSQLAdapter < AbstractAdapter
+      class SchemaCreation < AbstractAdapter::SchemaCreation
+        private
+
+        def visit_AddColumn(o)
+          sql_type = type_to_sql(o.type.to_sym, o.limit, o.precision, o.scale)
+          sql = "ADD COLUMN #{quote_column_name(o.name)} #{sql_type}"
+          add_column_options!(sql, column_options(o))
+        end
+
+        def visit_ColumnDefinition(o)
+          sql = super
+          if o.primary_key? && o.type == :uuid
+            sql << " PRIMARY KEY "
+            add_column_options!(sql, column_options(o))
+          end
+          sql
+        end
+
+        def add_column_options!(sql, options)
+          if options[:array] || options[:column].try(:array)
+            sql << '[]'
+          end
+
+          column = options.fetch(:column) { return super }
+          if column.type == :uuid && options[:default] =~ /\(\)/
+            sql << " DEFAULT #{options[:default]}"
+          else
+            super
+          end
+        end
+      end
+
+      def schema_creation
+        SchemaCreation.new self
+      end
+
       module SchemaStatements
         # Drops the database specified on the +name+ attribute
         # and creates it again using the provided +options+.
@@ -337,10 +373,7 @@ module ActiveRecord
         # See TableDefinition#column for details of the options you can use.
         def add_column(table_name, column_name, type, options = {})
           clear_cache!
-          add_column_sql = "ALTER TABLE #{quote_table_name(table_name)} ADD COLUMN #{quote_column_name(column_name)} #{type_to_sql(type, options[:limit], options[:precision], options[:scale])}"
-          add_column_options!(add_column_sql, options)
-
-          execute add_column_sql
+          super
         end
 
         # Changes the column of a table.
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
index 940de7e4f6..6f1eb25990 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -330,6 +330,13 @@ module ActiveRecord
       class TableDefinition < ActiveRecord::ConnectionAdapters::TableDefinition
         include ColumnMethods
 
+        def primary_key(name, type = :primary_key, options = {})
+          return super unless type == :uuid
+          options[:default] ||= 'uuid_generate_v4()'
+          options[:primary_key] = true
+          column name, type, options
+        end
+
         def column(name, type = nil, options = {})
           super
           column = self[name]
@@ -344,8 +351,8 @@ module ActiveRecord
 
         private
 
-        def create_column_definition(base, name, type)
-          ColumnDefinition.new base, name, type
+        def create_column_definition(name, type)
+          ColumnDefinition.new name, type
         end
       end
 
@@ -627,19 +634,6 @@ module ActiveRecord
         @table_alias_length ||= query('SHOW max_identifier_length', 'SCHEMA')[0][0].to_i
       end
 
-      def add_column_options!(sql, options)
-        if options[:array] || options[:column].try(:array)
-          sql << '[]'
-        end
-
-        column = options.fetch(:column) { return super }
-        if column.type == :uuid && options[:default] =~ /\(\)/
-          sql << " DEFAULT #{options[:default]}"
-        else
-          super
-        end
-      end
-
       # Set the authorized user for this session
       def session_auth=(user)
         clear_cache!
@@ -900,8 +894,8 @@ module ActiveRecord
           $1.strip if $1
         end
 
-        def create_table_definition
-          TableDefinition.new(self)
+        def create_table_definition(name, temporary, options)
+          TableDefinition.new native_database_types, name, temporary, options
         end
 
         def update_table_definition(table_name, base)
diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
index d3ffee3a8b..50d189d27a 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
@@ -458,7 +458,7 @@ module ActiveRecord
 
       def remove_column(table_name, column_name, type = nil, options = {}) #:nodoc:
         alter_table(table_name) do |definition|
-          definition.columns.delete(definition[column_name])
+          definition.remove_column column_name
         end
       end
 
diff --git a/activerecord/test/cases/adapters/postgresql/uuid_test.rb b/activerecord/test/cases/adapters/postgresql/uuid_test.rb
index 53002c5265..c0c0e8898c 100644
--- a/activerecord/test/cases/adapters/postgresql/uuid_test.rb
+++ b/activerecord/test/cases/adapters/postgresql/uuid_test.rb
@@ -35,6 +35,16 @@ class PostgresqlUUIDTest < ActiveRecord::TestCase
     @connection.execute 'drop table if exists pg_uuids'
   end
 
+  def test_id_is_uuid
+    assert_equal :uuid, UUID.columns_hash['id'].type
+    assert UUID.primary_key
+  end
+
+  def test_id_has_a_default
+    u = UUID.create
+    assert_not_nil u.id
+  end
+
   def test_auto_create_uuid
     u = UUID.create
     u.reload
diff --git a/activerecord/test/cases/column_definition_test.rb b/activerecord/test/cases/column_definition_test.rb
index bd2fbaa7db..dbb2f223cd 100644
--- a/activerecord/test/cases/column_definition_test.rb
+++ b/activerecord/test/cases/column_definition_test.rb
@@ -8,6 +8,7 @@ module ActiveRecord
         def @adapter.native_database_types
           {:string => "varchar"}
         end
+        @viz = @adapter.schema_creation
       end
 
       def test_can_set_coder
@@ -35,25 +36,25 @@ module ActiveRecord
       def test_should_not_include_default_clause_when_default_is_null
         column = Column.new("title", nil, "varchar(20)")
         column_def = ColumnDefinition.new(
-          @adapter, column.name, "string",
+          column.name, "string",
           column.limit, column.precision, column.scale, column.default, column.null)
-          assert_equal "title varchar(20)", column_def.to_sql
+          assert_equal "title varchar(20)", @viz.accept(column_def)
       end
 
       def test_should_include_default_clause_when_default_is_present
         column = Column.new("title", "Hello", "varchar(20)")
         column_def = ColumnDefinition.new(
-          @adapter, column.name, "string",
+          column.name, "string",
           column.limit, column.precision, column.scale, column.default, column.null)
-          assert_equal %Q{title varchar(20) DEFAULT 'Hello'}, column_def.to_sql
+          assert_equal %Q{title varchar(20) DEFAULT 'Hello'}, @viz.accept(column_def)
       end
 
       def test_should_specify_not_null_if_null_option_is_false
         column = Column.new("title", "Hello", "varchar(20)", false)
         column_def = ColumnDefinition.new(
-          @adapter, column.name, "string",
+          column.name, "string",
           column.limit, column.precision, column.scale, column.default, column.null)
-          assert_equal %Q{title varchar(20) DEFAULT 'Hello' NOT NULL}, column_def.to_sql
+          assert_equal %Q{title varchar(20) DEFAULT 'Hello' NOT NULL}, @viz.accept(column_def)
       end
 
       if current_adapter?(:MysqlAdapter)