SQLite adapters now support DDL transactions [#2080 state:resolved]

Signed-off-by: Pratik Naik <pratiknaik@gmail.com>

3 files changed, 68 insertions, 28 deletions

diff --git a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb
index 6077ddcdb6..afd6472db8 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb
@@ -72,6 +72,18 @@ module ActiveRecord
     #
     # * <tt>:database</tt> - Path to the database file.
     class SQLiteAdapter < AbstractAdapter
+      class Version
+        include Comparable
+
+        def initialize(version_string)
+          @version = version_string.split('.').map(&:to_i)
+        end
+
+        def <=>(version_string)
+          @version <=> version_string.split('.').map(&:to_i)
+        end
+      end
+
       def initialize(connection, logger, config)
         super(connection, logger)
         @config = config
@@ -81,6 +93,10 @@ module ActiveRecord
         'SQLite'
       end
 
+      def supports_ddl_transactions?
+        sqlite_version >= '2.0.0'
+      end
+
       def supports_migrations? #:nodoc:
         true
       end
@@ -88,6 +104,10 @@ module ActiveRecord
       def requires_reloading?
         true
       end
+
+      def supports_add_column?
+        sqlite_version >= '3.1.6'
+      end
  
       def disconnect!
         super
@@ -169,7 +189,6 @@ module ActiveRecord
         catch_schema_changes { @connection.rollback }
       end
 
-
       # SELECT ... FOR UPDATE is redundant since the table is locked.
       def add_lock!(sql, options) #:nodoc:
         sql
@@ -218,14 +237,20 @@ module ActiveRecord
         execute "ALTER TABLE #{name} RENAME TO #{new_name}"
       end
 
+      # See: http://www.sqlite.org/lang_altertable.html
+      # SQLite has an additional restriction on the ALTER TABLE statement
+      def valid_alter_table_options( type, options)
+        type.to_sym != :primary_key
+      end
+
       def add_column(table_name, column_name, type, options = {}) #:nodoc:
-        if @connection.respond_to?(:transaction_active?) && @connection.transaction_active?
-          raise StatementInvalid, 'Cannot add columns to a SQLite database while inside a transaction'
+        if supports_add_column? && valid_alter_table_options( type, options )
+          super(table_name, column_name, type, options)
+        else
+          alter_table(table_name) do |definition|
+            definition.column(column_name, type, options)
+          end
         end
-        
-        super(table_name, column_name, type, options)
-        # See last paragraph on http://www.sqlite.org/lang_altertable.html
-        execute "VACUUM"
       end
 
       def remove_column(table_name, *column_names) #:nodoc:
@@ -385,7 +410,7 @@ module ActiveRecord
         end
 
         def sqlite_version
-          @sqlite_version ||= select_value('select sqlite_version(*)')
+          @sqlite_version ||= SQLiteAdapter::Version.new(select_value('select sqlite_version(*)'))
         end
 
         def default_primary_key_type
@@ -398,23 +423,9 @@ module ActiveRecord
     end
 
     class SQLite2Adapter < SQLiteAdapter # :nodoc:
-      def supports_count_distinct? #:nodoc:
-        false
-      end
-
       def rename_table(name, new_name)
         move_table(name, new_name)
       end
-
-      def add_column(table_name, column_name, type, options = {}) #:nodoc:
-        if @connection.respond_to?(:transaction_active?) && @connection.transaction_active?
-          raise StatementInvalid, 'Cannot add columns to a SQLite database while inside a transaction'
-        end
-
-        alter_table(table_name) do |definition|
-          definition.column(column_name, type, options)
-        end
-      end
     end
 
     class DeprecatedSQLiteAdapter < SQLite2Adapter # :nodoc:
diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb
index 50d039ec77..16861f21b1 100644
--- a/activerecord/test/cases/migration_test.rb
+++ b/activerecord/test/cases/migration_test.rb
@@ -93,6 +93,30 @@ if ActiveRecord::Base.connection.supports_migrations?
       end
     end
 
+    def testing_table_with_only_foo_attribute
+      Person.connection.create_table :testings, :id => false do |t|
+        t.column :foo, :string
+      end
+
+      yield Person.connection
+    ensure
+      Person.connection.drop_table :testings rescue nil
+    end
+    protected :testing_table_with_only_foo_attribute
+
+    def test_create_table_without_id
+      testing_table_with_only_foo_attribute do |connection|
+        assert_equal connection.columns(:testings).size, 1
+      end
+    end
+
+    def test_add_column_with_primary_key_attribute
+      testing_table_with_only_foo_attribute do |connection|
+        assert_nothing_raised { connection.add_column :testings, :id, :primary_key }
+        assert_equal connection.columns(:testings).size, 2
+      end
+    end
+
     def test_create_table_adds_id
       Person.connection.create_table :testings do |t|
         t.column :foo, :string
@@ -928,7 +952,7 @@ if ActiveRecord::Base.connection.supports_migrations?
       assert_equal(0, ActiveRecord::Migrator.current_version)
     end
 
-    if current_adapter?(:PostgreSQLAdapter)
+    if ActiveRecord::Base.connection.supports_ddl_transactions?
       def test_migrator_one_up_with_exception_and_rollback
         assert !Person.column_methods_hash.include?(:last_name)
 
diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb
index f9ae5115fc..f6533b5396 100644
--- a/activerecord/test/cases/transactions_test.rb
+++ b/activerecord/test/cases/transactions_test.rb
@@ -349,7 +349,7 @@ class TransactionTest < ActiveRecord::TestCase
     end
   end
 
-  def test_sqlite_add_column_in_transaction_raises_statement_invalid
+  def test_sqlite_add_column_in_transaction
     return true unless current_adapter?(:SQLite3Adapter, :SQLiteAdapter)
 
     # Test first if column creation/deletion works correctly when no
@@ -368,10 +368,15 @@ class TransactionTest < ActiveRecord::TestCase
       assert !Topic.column_names.include?('stuff')
     end
 
-    # Test now inside a transaction: add_column should raise a StatementInvalid
-    Topic.transaction do
-      assert_raise(ActiveRecord::StatementInvalid) { Topic.connection.add_column('topics', 'stuff', :string) }
-      raise ActiveRecord::Rollback
+    if Topic.connection.supports_ddl_transactions?
+      assert_nothing_raised do
+        Topic.transaction { Topic.connection.add_column('topics', 'stuff', :string) }
+      end
+    else
+      Topic.transaction do
+        assert_raise(ActiveRecord::StatementInvalid) { Topic.connection.add_column('topics', 'stuff', :string) }
+        raise ActiveRecord::Rollback
+      end
     end
   end