Merge pull request #8054 from senny/6378_create_table_raises_when_defining_pk_column

create_table raises an ArgumentError when the primary key is redefined.

5 files changed, 45 insertions, 6 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index f630be6f70..4916777ce7 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,5 +1,10 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   The `create_table` method raises an `ArgumentError` when the primary key column is redefined.
+    Fix #6378
+
+    *Yves Senn*
+
 *   `ActiveRecord::AttributeMethods#[]` raises `ActiveModel::MissingAttributeError`
     error if the given attribute is missing. Fixes #5433.
 
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 dca355aa93..0f6b177b62 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
@@ -234,6 +234,10 @@ module ActiveRecord
         name = name.to_s
         type = type.to_sym
 
+        if primary_key_column_name == name
+          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
@@ -302,6 +306,11 @@ module ActiveRecord
         definition
       end
 
+      def primary_key_column_name
+        primary_key_column = columns.detect { |c| c.type == :primary_key }
+        primary_key_column && primary_key_column.name
+      end
+
       def native
         @base.native_database_types
       end
diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
index 4d5cb72c67..c5a859475f 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
@@ -519,24 +519,22 @@ module ActiveRecord
 
         def copy_table(from, to, options = {}) #:nodoc:
           from_primary_key = primary_key(from)
-          options[:primary_key] = from_primary_key if from_primary_key != 'id'
-          unless options[:primary_key]
-            options[:id] = columns(from).detect{|c| c.name == 'id'}.present? && from_primary_key == 'id'
-          end
+          options[:id] = false
           create_table(to, options) do |definition|
             @definition = definition
+            @definition.primary_key(from_primary_key) if from_primary_key.present?
             columns(from).each do |column|
               column_name = options[:rename] ?
                 (options[:rename][column.name] ||
                  options[:rename][column.name.to_sym] ||
                  column.name) : column.name
+              next if column_name == from_primary_key
 
               @definition.column(column_name, column.type,
                 :limit => column.limit, :default => column.default,
                 :precision => column.precision, :scale => column.scale,
                 :null => column.null)
             end
-            @definition.primary_key(from_primary_key) if from_primary_key
             yield @definition if block_given?
           end
 
diff --git a/activerecord/test/cases/adapters/sqlite3/copy_table_test.rb b/activerecord/test/cases/adapters/sqlite3/copy_table_test.rb
index 74288a98d1..d03d1dd94c 100644
--- a/activerecord/test/cases/adapters/sqlite3/copy_table_test.rb
+++ b/activerecord/test/cases/adapters/sqlite3/copy_table_test.rb
@@ -32,6 +32,11 @@ class CopyTableTest < ActiveRecord::TestCase
     end
   end
 
+  def test_copy_table_allows_to_pass_options_to_create_table
+    @connection.create_table('blocker_table')
+    test_copy_table('customers', 'blocker_table', force: true)
+  end
+
   def test_copy_table_with_index
     test_copy_table('comments', 'comments_with_index') do
       @connection.add_index('comments_with_index', ['post_id', 'type'])
@@ -43,7 +48,9 @@ class CopyTableTest < ActiveRecord::TestCase
   end
 
   def test_copy_table_without_primary_key
-    test_copy_table('developers_projects', 'programmers_projects')
+    test_copy_table('developers_projects', 'programmers_projects') do
+      assert_nil @connection.primary_key('programmers_projects')
+    end
   end
 
   def test_copy_table_with_id_col_that_is_not_primary_key
diff --git a/activerecord/test/cases/migration/change_schema_test.rb b/activerecord/test/cases/migration/change_schema_test.rb
index 17c1634444..86451289e7 100644
--- a/activerecord/test/cases/migration/change_schema_test.rb
+++ b/activerecord/test/cases/migration/change_schema_test.rb
@@ -132,6 +132,26 @@ module ActiveRecord
         assert_equal %w(foo testingid), connection.columns(:testings).map(&:name).sort
       end
 
+      def test_create_table_raises_when_redefining_primary_key_column
+        error = assert_raise(ArgumentError) do
+          connection.create_table :testings do |t|
+            t.column :id, :string
+          end
+        end
+
+        assert_equal "you can't redefine the primary key column 'id'. To define a custom primary key, pass { id: false } to create_table.", error.message
+      end
+
+      def test_create_table_raises_when_redefining_custom_primary_key_column
+        error = assert_raise(ArgumentError) do
+          connection.create_table :testings, primary_key: :testing_id do |t|
+            t.column :testing_id, :string
+          end
+        end
+
+        assert_equal "you can't redefine the primary key column 'testing_id'. To define a custom primary key, pass { id: false } to create_table.", error.message
+      end
+
       def test_create_table_with_timestamps_should_create_datetime_columns
         connection.create_table table_name do |t|
           t.timestamps