From 7378e6d3e6bc1a19d50060d3337378710fc5f72f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Thu, 12 Dec 2024 18:46:24 +0100 Subject: [PATCH] feat: INSERT OR [IGNORE|UPDATE] (#332) * feat: INSERT OR [IGNORE|UPDATE] Add support for `INSERT OR [IGNORE|UPDATE]` DML statements. * chore: fix acceptance tests * test: wait a bit for sessions to be deleted --- acceptance/cases/models/insert_all_test.rb | 29 ++++++++++++---- .../cases/sessions/session_not_found_test.rb | 2 ++ .../connection_adapters/spanner_adapter.rb | 12 ++++--- lib/activerecord_spanner_adapter/base.rb | 15 ++++---- ...ner_active_record_with_mock_server_test.rb | 34 +++++++++++++++---- 5 files changed, 66 insertions(+), 26 deletions(-) diff --git a/acceptance/cases/models/insert_all_test.rb b/acceptance/cases/models/insert_all_test.rb index b149f17e..c397832b 100644 --- a/acceptance/cases/models/insert_all_test.rb +++ b/acceptance/cases/models/insert_all_test.rb @@ -30,13 +30,24 @@ def test_insert_all { id: Author.next_sequence_value, name: "Carol" }, ] - assert_raise(NotImplementedError) { Author.insert_all(values) } + Author.insert_all(values) + + authors = Author.all.order(:name) + + assert_equal "Alice", authors[0].name + assert_equal "Bob", authors[1].name + assert_equal "Carol", authors[2].name end def test_insert value = { id: Author.next_sequence_value, name: "Alice" } - assert_raise(NotImplementedError) { Author.insert(value) } + Author.insert(value) + + authors = Author.all.order(:name) + + assert_equal 1, authors.length + assert_equal "Alice", authors[0].name end def test_insert_all! @@ -136,12 +147,16 @@ def test_upsert_all_with_transaction { id: Author.next_sequence_value, name: "Carol" }, ] - err = assert_raise(NotImplementedError) do - ActiveRecord::Base.transaction do - Author.upsert_all(values) - end + ActiveRecord::Base.transaction do + Author.upsert_all(values) end - assert_match "Use upsert outside a transaction block", err.message + + authors = Author.all.order(:name) + + assert_equal 3, authors.length + assert_equal "Alice", authors[0].name + assert_equal "Bob", authors[1].name + assert_equal "Carol", authors[2].name end def test_upsert_all_with_buffered_mutation_transaction diff --git a/acceptance/cases/sessions/session_not_found_test.rb b/acceptance/cases/sessions/session_not_found_test.rb index 4b92cc99..17706617 100644 --- a/acceptance/cases/sessions/session_not_found_test.rb +++ b/acceptance/cases/sessions/session_not_found_test.rb @@ -51,6 +51,8 @@ def delete_all_sessions sessions.each do |session| client.delete_session Google::Cloud::Spanner::V1::DeleteSessionRequest.new name: session.name end + # Wait a bit to ensure that the sessions are really deleted. + sleep 5 unless ENV["SPANNER_EMULATOR_HOST"] end def test_single_read diff --git a/lib/active_record/connection_adapters/spanner_adapter.rb b/lib/active_record/connection_adapters/spanner_adapter.rb index 2fe571e4..367aefec 100644 --- a/lib/active_record/connection_adapters/spanner_adapter.rb +++ b/lib/active_record/connection_adapters/spanner_adapter.rb @@ -209,12 +209,14 @@ def build_insert_sql insert raise "ActiveRecordSpannerAdapter does not support insert_sql with buffered_mutations transaction." end - if insert.skip_duplicates? || insert.update_duplicates? - raise NotImplementedError, "CloudSpanner does not support skip_duplicates and update_duplicates." - end - values_list, = insert.values_list - "INSERT #{insert.into} #{values_list}" + prefix = "INSERT" + if insert.update_duplicates? + prefix += " OR UPDATE" + elsif insert.skip_duplicates? + prefix += " OR IGNORE" + end + "#{prefix} #{insert.into} #{values_list}" end module TypeMapBuilder diff --git a/lib/activerecord_spanner_adapter/base.rb b/lib/activerecord_spanner_adapter/base.rb index fc37baaf..91ac92fb 100644 --- a/lib/activerecord_spanner_adapter/base.rb +++ b/lib/activerecord_spanner_adapter/base.rb @@ -130,8 +130,13 @@ def self._upsert_record values, returning _buffer_record values, :insert_or_update, returning end - def self.insert_all _attributes, **_kwargs - raise NotImplementedError, "Cloud Spanner does not support skip_duplicates. Use insert! or upsert instead." + def self.insert_all attributes, returning: nil, **_kwargs + if active_transaction? && buffered_mutations? + raise NotImplementedError, + "Spanner does not support skip_duplicates for mutations. " \ + "Use a transaction that uses DML, or use insert! or upsert instead." + end + super end def self.insert! attributes, returning: nil, **kwargs @@ -169,11 +174,7 @@ def self.upsert attributes, returning: nil, **kwargs def self.upsert_all attributes, returning: nil, unique_by: nil, **kwargs return super unless spanner_adapter? - if active_transaction? && !buffered_mutations? - raise NotImplementedError, "Cloud Spanner does not support upsert using DML. " \ - "Use upsert outside a transaction block or in a transaction " \ - "block with isolation: :buffered_mutations" - end + return super if active_transaction? && !buffered_mutations? # This might seem inefficient, but is actually not, as it is only buffering a mutation locally. # The mutations will be sent as one batch when the transaction is committed. diff --git a/test/activerecord_spanner_mock_server/spanner_active_record_with_mock_server_test.rb b/test/activerecord_spanner_mock_server/spanner_active_record_with_mock_server_test.rb index ee2980f0..63d4df2e 100644 --- a/test/activerecord_spanner_mock_server/spanner_active_record_with_mock_server_test.rb +++ b/test/activerecord_spanner_mock_server/spanner_active_record_with_mock_server_test.rb @@ -18,9 +18,18 @@ class SpannerActiveRecordMockServerTest < BaseSpannerMockServerTest VERSION_7_1_0 = Gem::Version.create('7.1.0') def test_insert + sql = "INSERT OR IGNORE INTO `singers` (`first_name`,`last_name`) VALUES ('Alice', 'Ecila')" + @mock.put_statement_result sql, StatementResult.new(1) + singer = { first_name: "Alice", last_name: "Ecila" } + Singer.insert(singer) - assert_raises(NotImplementedError) { Singer.insert(singer) } + execute_requests = @mock.requests.select { |req| req.is_a?(ExecuteSqlRequest) && req.sql == sql } + assert_equal 1, execute_requests.length + commit_requests = @mock.requests.select { |req| req.is_a?(CommitRequest) } + assert_equal 1, commit_requests.length + mutations = commit_requests[0].mutations + assert_equal 0, mutations.length end def test_insert_other_adapter @@ -1149,12 +1158,16 @@ def _disable_query_logs current_query_transformers end def test_insert_all + sql = "INSERT OR IGNORE INTO `singers` (`id`,`first_name`,`last_name`) " + + "VALUES (1, 'Dave', 'Allison'), (2, 'Alice', 'Davidson'), (3, 'Rene', 'Henderson')" + @mock.put_statement_result sql, StatementResult.new(3) + values = [ {id: 1, first_name: "Dave", last_name: "Allison"}, {id: 2, first_name: "Alice", last_name: "Davidson"}, {id: 3, first_name: "Rene", last_name: "Henderson"}, ] - assert_raises(NotImplementedError) { Singer.insert_all values } + Singer.insert_all values end def test_insert_all_bang_mutations @@ -1224,17 +1237,24 @@ def test_upsert_all_mutations end def test_upsert_all_dml + sql = "INSERT OR UPDATE INTO `singers` (`id`,`first_name`,`last_name`) " + + "VALUES (1, 'Dave', 'Allison'), (2, 'Alice', 'Davidson'), (3, 'Rene', 'Henderson')" + @mock.put_statement_result sql, StatementResult.new(3) + values = [ {id: 1, first_name: "Dave", last_name: "Allison"}, {id: 2, first_name: "Alice", last_name: "Davidson"}, {id: 3, first_name: "Rene", last_name: "Henderson"}, ] - err = assert_raises(NotImplementedError) do - Singer.transaction do - Singer.upsert_all values - end + Singer.transaction do + Singer.upsert_all values end - assert_match "Use upsert outside a transaction block", err.message + + commit_requests = @mock.requests.select { |req| req.is_a?(CommitRequest) } + assert_equal 1, commit_requests.length + assert_equal 0, commit_requests[0].mutations.length + execute_requests = @mock.requests.select { |req| req.is_a?(ExecuteSqlRequest) && req.sql == sql } + assert_equal 1, execute_requests.length end private