From c24efdbd2720bd4d936a3ffab0c22af0a718d2f1 Mon Sep 17 00:00:00 2001
From: Bob Lail <bob.lailfamily@gmail.com>
Date: Thu, 4 Apr 2019 07:35:04 -0500
Subject: When skipping duplicates in bulk insert on MySQL, avoid assigning id
 when not specified

If `id` is an `AUTONUMBER` column, then my former strategy here of assigning `no_op_column` to an arbitrary column would fail in this specific scenario:

1. `model.columns.first` is an AUTONUMBER column
2. `model.columns.first` is not assigned in the insert attributes

I added three tests: the first test covers the actual error; the second test documents that this _isn't_ a problem when a value is given for the AUTONUMBER column and the third test ensures that this no-op strategy isn't secretly doing an UPSERT.
---
 .../connection_adapters/abstract_mysql_adapter.rb  |  4 +--
 activerecord/lib/active_record/insert_all.rb       |  4 +--
 activerecord/test/cases/insert_all_test.rb         | 38 ++++++++++++++++++++++
 3 files changed, 42 insertions(+), 4 deletions(-)

(limited to 'activerecord')

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 f5da19f0f6..ca8bbc14da 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -516,8 +516,8 @@ module ActiveRecord
         sql = +"INSERT #{insert.into} #{insert.values_list}"
 
         if insert.skip_duplicates?
-          any_column = quote_column_name(insert.model.columns.first.name)
-          sql << " ON DUPLICATE KEY UPDATE #{any_column}=#{any_column}"
+          no_op_column = quote_column_name(insert.keys.first)
+          sql << " ON DUPLICATE KEY UPDATE #{no_op_column}=#{no_op_column}"
         elsif insert.update_duplicates?
           sql << " ON DUPLICATE KEY UPDATE "
           sql << insert.updatable_columns.map { |column| "#{column}=VALUES(#{column})" }.join(",")
diff --git a/activerecord/lib/active_record/insert_all.rb b/activerecord/lib/active_record/insert_all.rb
index 4b02d40aa0..d7808919ee 100644
--- a/activerecord/lib/active_record/insert_all.rb
+++ b/activerecord/lib/active_record/insert_all.rb
@@ -111,7 +111,7 @@ module ActiveRecord
       class Builder
         attr_reader :model
 
-        delegate :skip_duplicates?, :update_duplicates?, to: :insert_all
+        delegate :skip_duplicates?, :update_duplicates?, :keys, to: :insert_all
 
         def initialize(insert_all)
           @insert_all, @model, @connection = insert_all, insert_all.model, insert_all.connection
@@ -122,7 +122,7 @@ module ActiveRecord
         end
 
         def values_list
-          types = extract_types_from_columns_on(model.table_name, keys: insert_all.keys)
+          types = extract_types_from_columns_on(model.table_name, keys: keys)
 
           values_list = insert_all.map_key_with_value do |key, value|
             bind = Relation::QueryAttribute.new(key, value, types[key])
diff --git a/activerecord/test/cases/insert_all_test.rb b/activerecord/test/cases/insert_all_test.rb
index fc25701c80..61fbda1f0f 100644
--- a/activerecord/test/cases/insert_all_test.rb
+++ b/activerecord/test/cases/insert_all_test.rb
@@ -104,6 +104,44 @@ class InsertAllTest < ActiveRecord::TestCase
     end
   end
 
+  def test_insert_all_with_skip_duplicates_and_autonumber_id_not_given
+    skip unless supports_insert_on_duplicate_skip?
+
+    assert_difference "Book.count", 1 do
+      # These two books are duplicates according to an index on %i[author_id name]
+      # but their IDs are not specified so they will be assigned different IDs
+      # by autonumber. We will get an exception from MySQL if we attempt to skip
+      # one of these records by assigning its ID.
+      Book.insert_all [
+        { author_id: 8, name: "Refactoring" },
+        { author_id: 8, name: "Refactoring" }
+      ]
+    end
+  end
+
+  def test_insert_all_with_skip_duplicates_and_autonumber_id_given
+    skip unless supports_insert_on_duplicate_skip?
+
+    assert_difference "Book.count", 1 do
+      Book.insert_all [
+        { id: 200, author_id: 8, name: "Refactoring" },
+        { id: 201, author_id: 8, name: "Refactoring" }
+      ]
+    end
+  end
+
+  def test_skip_duplicates_strategy_does_not_secretly_upsert
+    skip unless supports_insert_on_duplicate_skip?
+
+    book = Book.create!(author_id: 8, name: "Refactoring", format: "EXPECTED")
+
+    assert_no_difference "Book.count" do
+      Book.insert(author_id: 8, name: "Refactoring", format: "UNEXPECTED")
+    end
+
+    assert_equal "EXPECTED", book.reload.format
+  end
+
   def test_insert_all_will_raise_if_duplicates_are_skipped_only_for_a_certain_conflict_target
     skip unless supports_insert_on_duplicate_skip? && supports_insert_conflict_target?
 
-- 
cgit v1.2.3