From 096e25bce7768d1ebbc8fb3c419ca4a34abe723d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Wed, 4 Nov 2020 01:17:49 +0000 Subject: [PATCH] Don't over protect the MySQL users on insert_all MySQL don't support conflict target on insert_all so we can't pass the :unique_by option. But, if you don't check if the index exists MySQL will successfully inset the data. If there is a conflict in an index like the primary key the database will raise an error. We should let the database decide if the data is acceptable or not. The test added on this commit exercise a table that has composite primary keys, but, in order to keep all the Rails behavior working (given Rails doesn't officially support composite primary keys) we set the primary key to an auto increment :id column. --- activerecord/lib/active_record/insert_all.rb | 2 + activerecord/test/cases/insert_all_test.rb | 42 +++++++++++++++++++- activerecord/test/models/cart.rb | 5 +++ activerecord/test/schema/schema.rb | 10 +++++ 4 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 activerecord/test/models/cart.rb diff --git a/activerecord/lib/active_record/insert_all.rb b/activerecord/lib/active_record/insert_all.rb index 3b5598d03d..13cd173d84 100644 --- a/activerecord/lib/active_record/insert_all.rb +++ b/activerecord/lib/active_record/insert_all.rb @@ -69,6 +69,8 @@ module ActiveRecord attr_reader :scope_attributes def find_unique_index_for(unique_by) + return unique_by if !connection.supports_insert_conflict_target? + name_or_columns = unique_by || model.primary_key match = Array(name_or_columns).map(&:to_s) diff --git a/activerecord/test/cases/insert_all_test.rb b/activerecord/test/cases/insert_all_test.rb index 61621d862e..b35c4724fe 100644 --- a/activerecord/test/cases/insert_all_test.rb +++ b/activerecord/test/cases/insert_all_test.rb @@ -3,6 +3,7 @@ require "cases/helper" require "models/author" require "models/book" +require "models/cart" require "models/speedometer" require "models/subscription" require "models/subscriber" @@ -208,6 +209,33 @@ class InsertAllTest < ActiveRecord::TestCase end end + def test_insert_all_and_upsert_all_works_with_composite_primary_keys_when_unique_by_is_provided + skip unless supports_insert_conflict_target? + + assert_difference "Cart.count", 2 do + Cart.insert_all [{ id: 1, shop_id: 1, title: "My cart" }], unique_by: [:shop_id, :id] + + Cart.upsert_all [{ id: 3, shop_id: 2, title: "My other cart" }], unique_by: [:shop_id, :id] + end + + error = assert_raises ArgumentError do + Cart.insert_all! [{ id: 2, shop_id: 1, title: "My cart" }] + end + assert_match "No unique index found for id", error.message + end + + def test_insert_all_and_upsert_all_works_with_composite_primary_keys_when_unique_by_is_not_provided + skip unless supports_insert_on_duplicate_skip? && !supports_insert_conflict_target? + + assert_difference "Cart.count", 3 do + Cart.insert_all [{ id: 1, shop_id: 1, title: "My cart" }] + + Cart.insert_all! [{ id: 2, shop_id: 1, title: "My cart 2" }] + + Cart.upsert_all [{ id: 3, shop_id: 2, title: "My other cart" }] + end + end + def test_insert_logs_message_including_model_name skip unless supports_insert_conflict_target? @@ -260,8 +288,18 @@ class InsertAllTest < ActiveRecord::TestCase assert_equal "New edition", Book.find(1).name end - def test_upsert_all_updates_existing_record_by_configured_primary_key - skip unless supports_insert_on_duplicate_update? + def test_upsert_all_does_notupdates_existing_record_by_when_there_is_no_key + skip unless supports_insert_on_duplicate_update? && !supports_insert_conflict_target? + + Speedometer.create!(speedometer_id: "s3", name: "Very fast") + + Speedometer.upsert_all [{ speedometer_id: "s3", name: "New Speedometer" }] + + assert_equal "Very fast", Speedometer.find("s3").name + end + + def test_upsert_all_updates_existing_record_by_configured_primary_key_fails_when_database_supports_insert_conflict_target + skip unless supports_insert_on_duplicate_update? && supports_insert_conflict_target? error = assert_raises ArgumentError do Speedometer.upsert_all [{ speedometer_id: "s1", name: "New Speedometer" }] diff --git a/activerecord/test/models/cart.rb b/activerecord/test/models/cart.rb new file mode 100644 index 0000000000..5fa6a572f2 --- /dev/null +++ b/activerecord/test/models/cart.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +class Cart < ActiveRecord::Base + self.primary_key = :id +end diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 91b7c247ee..23377911d9 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -152,6 +152,16 @@ ActiveRecord::Schema.define do create_table :carriers, force: true + create_table :carts, force: true, primary_key: [:shop_id, :id] do |t| + if current_adapter?(:Mysql2Adapter) + t.bigint :id, index: true, auto_increment: true, null: false + else + t.bigint :id, index: true, null: false + end + t.bigint :shop_id + t.string :title + end + create_table :categories, force: true do |t| t.string :name, null: false t.string :type