Skip to content

Commit

Permalink
fix: more permissive arg passthrough for insert_all and upsert_all
Browse files Browse the repository at this point in the history
Credits to @abachman for creating #275. This PR is created separately to
work around issues with the CLA check in the original PR.

Fixes #274
  • Loading branch information
olavloite committed Dec 7, 2023
1 parent e6dcf21 commit 9cce377
Show file tree
Hide file tree
Showing 3 changed files with 82 additions and 4 deletions.
22 changes: 22 additions & 0 deletions acceptance/cases/models/insert_all_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ def test_insert_all
assert_raise(NotImplementedError) { Author.insert_all(values) }
end

def test_insert
value = { id: Author.next_sequence_value, name: "Alice" }

assert_raise(NotImplementedError) { Author.insert(value) }
end

def test_insert_all!
values = [
{ id: Author.next_sequence_value, name: "Alice" },
Expand Down Expand Up @@ -85,6 +91,22 @@ def test_insert_all_with_buffered_mutation_transaction
assert_equal "Carol", authors[2].name
end

def test_upsert
Author.create id: 1, name: "David"
authors = Author.all.order(:name)
assert_equal 1, authors.length
assert_equal "David", authors[0].name

value = { id: 1, name: "Alice" }

Author.upsert(value)

authors = Author.all.order(:name)

assert_equal 1, authors.length
assert_equal "Alice", authors[0].name
end

def test_upsert_all
Author.create id: 1, name: "David"
authors = Author.all.order(:name)
Expand Down
8 changes: 4 additions & 4 deletions lib/activerecord_spanner_adapter/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,11 @@ def self._upsert_record values
_buffer_record values, :insert_or_update
end

def self.insert_all _attributes, _returning: nil, _unique_by: nil
raise NotImplementedError, "Cloud Spanner does not support skip_duplicates."
def self.insert_all _attributes, **_kwargs
raise NotImplementedError, "Cloud Spanner does not support skip_duplicates. Use insert! or upsert instead."
end

def self.insert_all! attributes, returning: nil
def self.insert_all! attributes, **_kwargs
return super unless spanner_adapter?
return super if active_transaction? && !buffered_mutations?

Expand All @@ -90,7 +90,7 @@ def self.insert_all! attributes, returning: nil
end
end

def self.upsert_all attributes, returning: nil, unique_by: nil
def self.upsert_all attributes, **_kwargs
return super unless spanner_adapter?
if active_transaction? && !buffered_mutations?
raise NotImplementedError, "Cloud Spanner does not support upsert using DML. " \
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,64 @@
require_relative "./base_spanner_mock_server_test"

module MockServerTests
CommitRequest = Google::Cloud::Spanner::V1::CommitRequest

class SpannerActiveRecordMockServerTest < BaseSpannerMockServerTest

def test_insert
singer = { first_name: "Alice", last_name: "Ecila" }

assert_raises(NotImplementedError) { Singer.insert(singer) }
end

def test_insert!
singer = { first_name: "Alice", last_name: "Ecila" }
singer = Singer.insert! singer

commit_requests = @mock.requests.select { |req| req.is_a?(CommitRequest) }
assert_equal 1, commit_requests.length
mutations = commit_requests[0].mutations
assert_equal 1, mutations.length
mutation = mutations[0]
assert_equal :insert, mutation.operation
assert_equal "singers", mutation.insert.table

assert_equal 1, mutation.insert.values.length
assert_equal 3, mutation.insert.values[0].length
assert_equal "Alice", mutation.insert.values[0][0]
assert_equal "Ecila", mutation.insert.values[0][1]
assert_equal singer[0]["id"], mutation.insert.values[0][2].to_i

assert_equal 3, mutation.insert.columns.length
assert_equal "first_name", mutation.insert.columns[0]
assert_equal "last_name", mutation.insert.columns[1]
assert_equal "id", mutation.insert.columns[2]
end

def test_upsert
singer = { first_name: "Alice", last_name: "Ecila" }
singer = Singer.upsert singer

commit_requests = @mock.requests.select { |req| req.is_a?(CommitRequest) }
assert_equal 1, commit_requests.length
mutations = commit_requests[0].mutations
assert_equal 1, mutations.length
mutation = mutations[0]
assert_equal :insert_or_update, mutation.operation
assert_equal "singers", mutation.insert_or_update.table

assert_equal 1, mutation.insert_or_update.values.length
assert_equal 3, mutation.insert_or_update.values[0].length
assert_equal "Alice", mutation.insert_or_update.values[0][0]
assert_equal "Ecila", mutation.insert_or_update.values[0][1]
assert_equal singer[0]["id"], mutation.insert_or_update.values[0][2].to_i

assert_equal 3, mutation.insert_or_update.columns.length
assert_equal "first_name", mutation.insert_or_update.columns[0]
assert_equal "last_name", mutation.insert_or_update.columns[1]
assert_equal "id", mutation.insert_or_update.columns[2]
end

def test_selects_all_singers_without_transaction
sql = "SELECT `singers`.* FROM `singers`"
@mock.put_statement_result sql, MockServerTests::create_random_singers_result(4)
Expand Down

0 comments on commit 9cce377

Please sign in to comment.