From c7613dde539a8ffb710e432d131ccc7891520066 Mon Sep 17 00:00:00 2001 From: Vladimir Dementyev Date: Fri, 15 Mar 2019 11:18:45 -0400 Subject: [PATCH 1/3] Support string returning clause for AR#insert_all (cherry picked from commit 15b3310a4d6b2044da4ff79737e2b19fef9b6267) --- activerecord/CHANGELOG.md | 15 +++++++++++++++ activerecord/lib/active_record/insert_all.rb | 8 +++++++- activerecord/lib/active_record/persistence.rb | 9 +++++++++ activerecord/test/cases/insert_all_test.rb | 7 +++++++ 4 files changed, 38 insertions(+), 1 deletion(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index d6f081ff18..b38e2b8c35 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,18 @@ +* Allow passing raw SQL as `returning` statement to `#upsert_all`: + + ```ruby + Article.insert_all( + [ + {title: "Article 1", slug: "article-1", published: false}, + {title: "Article 2", slug: "article-2", published: false} + ], + # Some PostgreSQL magic here to detect which rows have been actually inserted + returning: "id, (xmax = '0') as inserted, name as new_name" + ) + ``` + + *Vladimir Dementyev* + * Deprecate `legacy_connection_handling`. *Eileen M. Uchitelle* diff --git a/activerecord/lib/active_record/insert_all.rb b/activerecord/lib/active_record/insert_all.rb index 97c440cd9c..61c13ab4ab 100644 --- a/activerecord/lib/active_record/insert_all.rb +++ b/activerecord/lib/active_record/insert_all.rb @@ -151,7 +151,13 @@ module ActiveRecord end def returning - format_columns(insert_all.returning) if insert_all.returning + return unless insert_all.returning + + if insert_all.returning.is_a?(String) + insert_all.returning + else + format_columns(insert_all.returning) + end end def conflict_target diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index 9406b84274..7004f447a9 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -91,6 +91,9 @@ module ActiveRecord # or returning: false to omit the underlying RETURNING SQL # clause entirely. # + # You can also pass an SQL string if you need more control on the return values + # (for example, returning: "id, name as new_name"). + # # [:unique_by] # (PostgreSQL and SQLite only) By default rows are considered to be unique # by every unique index on the table. Any duplicate rows are skipped. @@ -168,6 +171,9 @@ module ActiveRecord # or returning: false to omit the underlying RETURNING SQL # clause entirely. # + # You can also pass an SQL string if you need more control on the return values + # (for example, returning: "id, name as new_name"). + # # ==== Examples # # # Insert multiple records @@ -216,6 +222,9 @@ module ActiveRecord # or returning: false to omit the underlying RETURNING SQL # clause entirely. # + # You can also pass an SQL string if you need more control on the return values + # (for example, returning: "id, name as new_name"). + # # [:unique_by] # (PostgreSQL and SQLite only) By default rows are considered to be unique # by every unique index on the table. Any duplicate rows are skipped. diff --git a/activerecord/test/cases/insert_all_test.rb b/activerecord/test/cases/insert_all_test.rb index b35c4724fe..87851518c2 100644 --- a/activerecord/test/cases/insert_all_test.rb +++ b/activerecord/test/cases/insert_all_test.rb @@ -109,6 +109,13 @@ class InsertAllTest < ActiveRecord::TestCase assert_equal %w[ Rework ], result.pluck("name") end + def test_insert_all_returns_requested_sql_fields + skip unless supports_insert_returning? + + result = Book.insert_all! [{ name: "Rework", author_id: 1 }], returning: "UPPER(name) as name" + assert_equal %w[ REWORK ], result.pluck("name") + end + def test_insert_all_can_skip_duplicate_records skip unless supports_insert_on_duplicate_skip? From 8f3c12f8803b56943956e8a1539eca5df9f5a6a6 Mon Sep 17 00:00:00 2001 From: Vladimir Dementyev Date: Mon, 12 Apr 2021 18:01:42 +0300 Subject: [PATCH 2/3] Add update_sql option to #upsert_all --- activerecord/CHANGELOG.md | 11 +++++++++++ .../abstract_mysql_adapter.rb | 8 ++++++-- .../connection_adapters/postgresql_adapter.rb | 8 ++++++-- .../connection_adapters/sqlite3_adapter.rb | 8 ++++++-- activerecord/lib/active_record/insert_all.rb | 12 +++++++++--- activerecord/lib/active_record/persistence.rb | 13 +++++++++---- activerecord/test/cases/insert_all_test.rb | 17 +++++++++++++++++ 7 files changed, 64 insertions(+), 13 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index b38e2b8c35..3d6bd9f2a5 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,14 @@ +* Add `update_sql` option to `#upsert_all` to make it possible to use raw SQL to update columns on conflict: + + ```ruby + Book.upsert_all( + [{ id: 1, status: 1 }, { id: 2, status: 1 }], + update_sql: "status = GREATEST(books.status, EXCLUDED.status)" + ) + ``` + + *Vladimir Dementyev* + * Allow passing raw SQL as `returning` statement to `#upsert_all`: ```ruby 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 637358807c..1f9d796fc2 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -551,8 +551,12 @@ module ActiveRecord 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(",") + if insert.raw_update_sql? + sql << insert.raw_update_sql + else + sql << insert.touch_model_timestamps_unless { |column| "#{column}<=>VALUES(#{column})" } + sql << insert.updatable_columns.map { |column| "#{column}=VALUES(#{column})" }.join(",") + end end sql diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index 4bdb230a28..b19bf9baf1 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -439,8 +439,12 @@ module ActiveRecord 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(",") + if insert.raw_update_sql? + sql << insert.raw_update_sql + else + 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 end sql << " RETURNING #{insert.returning}" if insert.returning diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb index 965839d752..a51bb5d1cf 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb @@ -313,8 +313,12 @@ module ActiveRecord 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(",") + if insert.raw_update_sql? + sql << insert.raw_update_sql + else + sql << insert.touch_model_timestamps_unless { |column| "#{column} IS excluded.#{column}" } + sql << insert.updatable_columns.map { |column| "#{column}=excluded.#{column}" }.join(",") + end end sql diff --git a/activerecord/lib/active_record/insert_all.rb b/activerecord/lib/active_record/insert_all.rb index 61c13ab4ab..e7e17fa799 100644 --- a/activerecord/lib/active_record/insert_all.rb +++ b/activerecord/lib/active_record/insert_all.rb @@ -5,13 +5,13 @@ require "active_support/core_ext/enumerable" module ActiveRecord class InsertAll # :nodoc: attr_reader :model, :connection, :inserts, :keys - attr_reader :on_duplicate, :returning, :unique_by + attr_reader :on_duplicate, :returning, :unique_by, :update_sql - def initialize(model, inserts, on_duplicate:, returning: nil, unique_by: nil) + def initialize(model, inserts, on_duplicate:, returning: nil, unique_by: nil, update_sql: nil) raise ArgumentError, "Empty list of attributes passed" if inserts.blank? @model, @connection, @inserts, @keys = model, model.connection, inserts, inserts.first.keys.map(&:to_s) - @on_duplicate, @returning, @unique_by = on_duplicate, returning, unique_by + @on_duplicate, @returning, @unique_by, @update_sql = on_duplicate, returning, unique_by, update_sql if model.scope_attributes? @scope_attributes = model.scope_attributes @@ -182,6 +182,12 @@ module ActiveRecord end.compact.join end + def raw_update_sql + insert_all.update_sql + end + + alias raw_update_sql? raw_update_sql + private attr_reader :connection, :insert_all diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index 7004f447a9..686b438e31 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -198,8 +198,8 @@ module ActiveRecord # go through Active Record's type casting and serialization. # # See ActiveRecord::Persistence#upsert_all for documentation. - def upsert(attributes, returning: nil, unique_by: nil) - upsert_all([ attributes ], returning: returning, unique_by: unique_by) + def upsert(attributes, returning: nil, unique_by: nil, update_sql: nil) + upsert_all([ attributes ], returning: returning, unique_by: unique_by, update_sql: update_sql) end # Updates or inserts (upserts) multiple records into the database in a @@ -245,6 +245,11 @@ module ActiveRecord # :unique_by is recommended to be paired with # Active Record's schema_cache. # + # [:update_sql] + # Specify a custom SQL for updating rows on conflict. + # + # NOTE: in this case you must provide all the columns you want to update by yourself. + # # ==== Examples # # # Inserts multiple records, performing an upsert when records have duplicate ISBNs. @@ -256,8 +261,8 @@ module ActiveRecord # ], unique_by: :isbn) # # Book.find_by(isbn: "1").title # => "Eloquent Ruby" - def upsert_all(attributes, returning: nil, unique_by: nil) - InsertAll.new(self, attributes, on_duplicate: :update, returning: returning, unique_by: unique_by).execute + def upsert_all(attributes, returning: nil, unique_by: nil, update_sql: nil) + InsertAll.new(self, attributes, on_duplicate: :update, returning: returning, unique_by: unique_by, update_sql: update_sql).execute end # Given an attributes hash, +instantiate+ returns a new instance of diff --git a/activerecord/test/cases/insert_all_test.rb b/activerecord/test/cases/insert_all_test.rb index 87851518c2..3eaa7791c5 100644 --- a/activerecord/test/cases/insert_all_test.rb +++ b/activerecord/test/cases/insert_all_test.rb @@ -473,6 +473,19 @@ class InsertAllTest < ActiveRecord::TestCase assert_raise(ArgumentError) { book.subscribers.upsert_all([ { nick: "Jimmy" } ]) } end + def test_upsert_all_updates_using_provided_sql + skip unless supports_insert_on_duplicate_update? + + operator = sqlite? ? "MAX" : "GREATEST" + + Book.upsert_all( + [{ id: 1, status: 1 }, { id: 2, status: 1 }], + update_sql: "status = #{operator}(books.status, 1)" + ) + assert_equal "published", Book.find(1).status + assert_equal "written", Book.find(2).status + end + private def capture_log_output output = StringIO.new @@ -484,4 +497,8 @@ class InsertAllTest < ActiveRecord::TestCase ActiveRecord::Base.logger = old_logger end end + + def sqlite? + ActiveRecord::Base.connection.adapter_name.match?(/sqlite/i) + end end From 4bef82217cd6a63cc10fe7beb16298e935babb5f Mon Sep 17 00:00:00 2001 From: Vladimir Dementyev Date: Mon, 12 Apr 2021 20:23:46 +0300 Subject: [PATCH 3/3] Force Arel.sql for returning and on_duplicate --- activerecord/CHANGELOG.md | 13 ++++++------ activerecord/lib/active_record/insert_all.rb | 21 +++++++++++++++++-- activerecord/lib/active_record/persistence.rb | 10 ++++----- activerecord/test/cases/insert_all_test.rb | 4 ++-- 4 files changed, 32 insertions(+), 16 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 3d6bd9f2a5..0eb071eb6f 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,24 +1,23 @@ -* Add `update_sql` option to `#upsert_all` to make it possible to use raw SQL to update columns on conflict: +* Allow passing SQL as `on_duplicate` value to `#upsert_all` to make it possible to use raw SQL to update columns on conflict: ```ruby Book.upsert_all( [{ id: 1, status: 1 }, { id: 2, status: 1 }], - update_sql: "status = GREATEST(books.status, EXCLUDED.status)" + on_duplicate: Arel.sql("status = GREATEST(books.status, EXCLUDED.status)") ) ``` *Vladimir Dementyev* -* Allow passing raw SQL as `returning` statement to `#upsert_all`: +* Allow passing SQL as `returning` statement to `#upsert_all`: ```ruby Article.insert_all( [ - {title: "Article 1", slug: "article-1", published: false}, - {title: "Article 2", slug: "article-2", published: false} + { title: "Article 1", slug: "article-1", published: false }, + { title: "Article 2", slug: "article-2", published: false } ], - # Some PostgreSQL magic here to detect which rows have been actually inserted - returning: "id, (xmax = '0') as inserted, name as new_name" + returning: Arel.sql("id, (xmax = '0') as inserted, name as new_name") ) ``` diff --git a/activerecord/lib/active_record/insert_all.rb b/activerecord/lib/active_record/insert_all.rb index e7e17fa799..78c81f52cc 100644 --- a/activerecord/lib/active_record/insert_all.rb +++ b/activerecord/lib/active_record/insert_all.rb @@ -7,11 +7,19 @@ module ActiveRecord attr_reader :model, :connection, :inserts, :keys attr_reader :on_duplicate, :returning, :unique_by, :update_sql - def initialize(model, inserts, on_duplicate:, returning: nil, unique_by: nil, update_sql: nil) + def initialize(model, inserts, on_duplicate:, returning: nil, unique_by: nil) raise ArgumentError, "Empty list of attributes passed" if inserts.blank? @model, @connection, @inserts, @keys = model, model.connection, inserts, inserts.first.keys.map(&:to_s) - @on_duplicate, @returning, @unique_by, @update_sql = on_duplicate, returning, unique_by, update_sql + @on_duplicate, @returning, @unique_by = on_duplicate, returning, unique_by + + disallow_raw_sql!(returning) + disallow_raw_sql!(on_duplicate) + + if Arel.arel_node?(on_duplicate) + @update_sql = on_duplicate + @on_duplicate = :update + end if model.scope_attributes? @scope_attributes = model.scope_attributes @@ -127,6 +135,15 @@ module ActiveRecord end end + def disallow_raw_sql!(value) + return if !value.is_a?(String) || Arel.arel_node?(value) + + raise ArgumentError, "Dangerous query method (method whose arguments are used as raw " \ + "SQL) called: #{value}. " \ + "Known-safe values can be passed " \ + "by wrapping them in Arel.sql()." + end + class Builder # :nodoc: attr_reader :model diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index 686b438e31..4a0da374fa 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -198,8 +198,8 @@ module ActiveRecord # go through Active Record's type casting and serialization. # # See ActiveRecord::Persistence#upsert_all for documentation. - def upsert(attributes, returning: nil, unique_by: nil, update_sql: nil) - upsert_all([ attributes ], returning: returning, unique_by: unique_by, update_sql: update_sql) + def upsert(attributes, on_duplicate: :update, returning: nil, unique_by: nil) + upsert_all([ attributes ], on_duplicate: on_duplicate, returning: returning, unique_by: unique_by) end # Updates or inserts (upserts) multiple records into the database in a @@ -245,7 +245,7 @@ module ActiveRecord # :unique_by is recommended to be paired with # Active Record's schema_cache. # - # [:update_sql] + # [:on_duplicate] # Specify a custom SQL for updating rows on conflict. # # NOTE: in this case you must provide all the columns you want to update by yourself. @@ -261,8 +261,8 @@ module ActiveRecord # ], unique_by: :isbn) # # Book.find_by(isbn: "1").title # => "Eloquent Ruby" - def upsert_all(attributes, returning: nil, unique_by: nil, update_sql: nil) - InsertAll.new(self, attributes, on_duplicate: :update, returning: returning, unique_by: unique_by, update_sql: update_sql).execute + def upsert_all(attributes, on_duplicate: :update, returning: nil, unique_by: nil, update_sql: nil) + InsertAll.new(self, attributes, on_duplicate: on_duplicate, returning: returning, unique_by: unique_by).execute end # Given an attributes hash, +instantiate+ returns a new instance of diff --git a/activerecord/test/cases/insert_all_test.rb b/activerecord/test/cases/insert_all_test.rb index 3eaa7791c5..902f0157e3 100644 --- a/activerecord/test/cases/insert_all_test.rb +++ b/activerecord/test/cases/insert_all_test.rb @@ -112,7 +112,7 @@ class InsertAllTest < ActiveRecord::TestCase def test_insert_all_returns_requested_sql_fields skip unless supports_insert_returning? - result = Book.insert_all! [{ name: "Rework", author_id: 1 }], returning: "UPPER(name) as name" + result = Book.insert_all! [{ name: "Rework", author_id: 1 }], returning: Arel.sql("UPPER(name) as name") assert_equal %w[ REWORK ], result.pluck("name") end @@ -480,7 +480,7 @@ class InsertAllTest < ActiveRecord::TestCase Book.upsert_all( [{ id: 1, status: 1 }, { id: 2, status: 1 }], - update_sql: "status = #{operator}(books.status, 1)" + on_duplicate: Arel.sql("status = #{operator}(books.status, 1)") ) assert_equal "published", Book.find(1).status assert_equal "written", Book.find(2).status