From 3a8668bd7389c2bfa8c7c0b0ec17dc078734852a Mon Sep 17 00:00:00 2001 From: Bob Lail Date: Sat, 2 Nov 2019 18:37:48 -0500 Subject: [PATCH] Touch updated_at when upsert_all modifies a record - When a user passes `updated_at` to `upsert_all`, the given value is used. - When a user omits `updated_at`, `upsert_all` touches the timestamp if (but only if) any upserted values differ. Preserve Rails' ability to generate intelligent cache keys for ActiveRecord when using `upsert_all` frequently to sync imported data. --- .../abstract_mysql_adapter.rb | 1 + .../connection_adapters/postgresql_adapter.rb | 1 + .../connection_adapters/sqlite3_adapter.rb | 1 + activerecord/lib/active_record/insert_all.rb | 12 ++++++ activerecord/test/cases/insert_all_test.rb | 40 +++++++++++++++++++ activerecord/test/schema/schema.rb | 4 ++ 6 files changed, 59 insertions(+) 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 98d5be183a..cb40ef4934 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -510,6 +510,7 @@ def build_insert_sql(insert) # :nodoc: sql << " ON DUPLICATE KEY UPDATE #{no_op_column}=#{no_op_column}" elsif insert.update_duplicates? sql << " ON DUPLICATE KEY UPDATE " + sql << insert.touch_model_timestamps_unless { |column| "#{column}<=>VALUES(#{column})" } sql << insert.updatable_columns.map { |column| "#{column}=VALUES(#{column})" }.join(",") end diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index ff9c669b96..edfd5d3cac 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -450,6 +450,7 @@ def build_insert_sql(insert) # :nodoc: sql << " ON CONFLICT #{insert.conflict_target} DO NOTHING" elsif insert.update_duplicates? sql << " ON CONFLICT #{insert.conflict_target} DO UPDATE SET " + sql << insert.touch_model_timestamps_unless { |column| "#{insert.model.quoted_table_name}.#{column} IS NOT DISTINCT FROM excluded.#{column}" } sql << insert.updatable_columns.map { |column| "#{column}=excluded.#{column}" }.join(",") end diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb index 14184947d3..adb5c16178 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb @@ -319,6 +319,7 @@ def build_insert_sql(insert) # :nodoc: sql << " ON CONFLICT #{insert.conflict_target} DO NOTHING" elsif insert.update_duplicates? sql << " ON CONFLICT #{insert.conflict_target} DO UPDATE SET " + sql << insert.touch_model_timestamps_unless { |column| "#{column} IS excluded.#{column}" } sql << insert.updatable_columns.map { |column| "#{column}=excluded.#{column}" }.join(",") end diff --git a/activerecord/lib/active_record/insert_all.rb b/activerecord/lib/active_record/insert_all.rb index 7d49c136fe..34d0af6f4f 100644 --- a/activerecord/lib/active_record/insert_all.rb +++ b/activerecord/lib/active_record/insert_all.rb @@ -153,9 +153,21 @@ def updatable_columns quote_columns(insert_all.updatable_columns) end + def touch_model_timestamps_unless(&block) + model.send(:timestamp_attributes_for_update_in_model).map do |column_name| + if touch_timestamp_attribute?(column_name) + "#{column_name}=(CASE WHEN (#{updatable_columns.map(&block).join(" AND ")}) THEN #{model.quoted_table_name}.#{column_name} ELSE CURRENT_TIMESTAMP END)," + end + end.compact.join + end + private attr_reader :connection, :insert_all + def touch_timestamp_attribute?(column_name) + update_duplicates? && !insert_all.updatable_columns.include?(column_name) + end + def columns_list format_columns(insert_all.keys) end diff --git a/activerecord/test/cases/insert_all_test.rb b/activerecord/test/cases/insert_all_test.rb index 294d40de1e..16fe630f5e 100644 --- a/activerecord/test/cases/insert_all_test.rb +++ b/activerecord/test/cases/insert_all_test.rb @@ -273,6 +273,46 @@ def test_upsert_all_does_not_perform_an_upsert_if_a_partial_index_doesnt_apply assert_equal ["Out of the Silent Planet", "Perelandra"], Book.where(isbn: "1974522598").order(:name).pluck(:name) end + def test_upsert_all_does_not_touch_updated_at_when_values_do_not_change + skip unless supports_insert_on_duplicate_update? + + updated_at = Time.now.utc - 5.years + Book.insert_all [{ id: 101, name: "Out of the Silent Planet", published_on: Date.new(1938, 4, 1), updated_at: updated_at }] + Book.upsert_all [{ id: 101, name: "Out of the Silent Planet", published_on: Date.new(1938, 4, 1) }] + + assert_in_delta updated_at, Book.find(101).updated_at, 1 + end + + def test_upsert_all_touches_updated_at_and_updated_on_when_values_change + skip unless supports_insert_on_duplicate_update? + + Book.insert_all [{ id: 101, name: "Out of the Silent Planet", published_on: Date.new(1938, 4, 1), updated_at: 5.years.ago, updated_on: 5.years.ago }] + Book.upsert_all [{ id: 101, name: "Out of the Silent Planet", published_on: Date.new(1938, 4, 8) }] + + assert_equal Time.now.year, Book.find(101).updated_at.year + assert_equal Time.now.year, Book.find(101).updated_on.year + end + + def test_upsert_all_uses_given_updated_at_over_implicit_updated_at + skip unless supports_insert_on_duplicate_update? + + updated_at = Time.now.utc - 1.year + Book.insert_all [{ id: 101, name: "Out of the Silent Planet", published_on: Date.new(1938, 4, 1), updated_at: 5.years.ago }] + Book.upsert_all [{ id: 101, name: "Out of the Silent Planet", published_on: Date.new(1938, 4, 8), updated_at: updated_at }] + + assert_in_delta updated_at, Book.find(101).updated_at, 1 + end + + def test_upsert_all_uses_given_updated_on_over_implicit_updated_on + skip unless supports_insert_on_duplicate_update? + + updated_on = Time.now.utc.to_date - 30 + Book.insert_all [{ id: 101, name: "Out of the Silent Planet", published_on: Date.new(1938, 4, 1), updated_on: 5.years.ago }] + Book.upsert_all [{ id: 101, name: "Out of the Silent Planet", published_on: Date.new(1938, 4, 8), updated_on: updated_on }] + + assert_equal updated_on, Book.find(101).updated_on + end + def test_insert_all_raises_on_unknown_attribute assert_raise ActiveRecord::UnknownAttributeError do Book.insert_all! [{ unknown_attribute: "Test" }] diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index fa68290919..98481ece59 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -119,6 +119,10 @@ t.datetime :published_on t.index [:author_id, :name], unique: true t.index :isbn, where: "published_on IS NOT NULL", unique: true + + t.datetime :created_at + t.datetime :updated_at + t.date :updated_on end create_table :booleans, force: true do |t|