Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fixed incorrect argument handling. #333

Merged
merged 7 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/acceptance-tests-on-emulator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ jobs:
max-parallel: 4
matrix:
ruby: ["2.7", "3.0", "3.1", "3.2", "3.3"]
ar: ["~> 6.1.0", "~> 7.0.0", "~> 7.1.0"]
ar: ["~> 6.1.0", "~> 7.0.0", "~> 7.1.0", "~> 7.2.0"]
# Exclude combinations that are not supported.
exclude:
- ruby: "2.7"
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ jobs:
max-parallel: 4
matrix:
ruby: ["2.7", "3.0", "3.1", "3.2", "3.3"]
ar: ["~> 6.1.0", "~> 7.0.0", "~> 7.1.0"]
ar: ["~> 6.1.0", "~> 7.0.0", "~> 7.1.0", "~> 7.2.0"]
# Exclude combinations that are not supported.
exclude:
- ruby: "2.7"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ jobs:
max-parallel: 4
matrix:
ruby: ["2.7", "3.0", "3.1", "3.2", "3.3"]
ar: ["~> 6.1.0", "~> 7.0.0", "~> 7.1.0"]
ar: ["~> 6.1.0", "~> 7.0.0", "~> 7.1.0", "~> 7.2.0"]
# Exclude combinations that are not supported.
exclude:
- ruby: "2.7"
Expand Down
30 changes: 21 additions & 9 deletions acceptance/cases/migration/schema_dumper_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,27 @@ def is_7_1_or_higher?
ActiveRecord::gem_version >= Gem::Version.create('7.1.0')
end

def is_7_2_or_higher?
ActiveRecord::gem_version >= Gem::Version.create('7.2.0')
end

def pool_or_connection
if is_7_2_or_higher?
ActiveRecord::Base.connection_pool
else
ActiveRecord::Base.connection
end
end

def test_dump_schema_contains_start_batch_ddl
connection = ActiveRecord::Base.connection
connection = pool_or_connection
schema = StringIO.new
ActiveRecord::SchemaDumper.dump connection, schema
assert schema.string.include?("connection.start_batch_ddl")
end

def test_dump_schema_contains_run_batch
connection = ActiveRecord::Base.connection
connection = pool_or_connection
schema = StringIO.new
ActiveRecord::SchemaDumper.dump connection, schema
assert schema.string.include?(" connection.run_batch\n"\
Expand All @@ -35,7 +47,7 @@ def test_dump_schema_contains_run_batch
end

def test_dump_schema_contains_albums_table
connection = ActiveRecord::Base.connection
connection = pool_or_connection
schema = StringIO.new
ActiveRecord::SchemaDumper.dump connection, schema
sql = schema.string
Expand All @@ -47,42 +59,42 @@ def test_dump_schema_contains_albums_table
end

def test_dump_schema_contains_interleaved_index
connection = ActiveRecord::Base.connection
connection = pool_or_connection
schema = StringIO.new
ActiveRecord::SchemaDumper.dump connection, schema
assert schema.string.include?("t.index [\"singerid\", \"albumid\", \"title\"], name: \"index_tracks_on_singerid_and_albumid_and_title\", order: { singerid: :asc, albumid: :asc, title: :asc }, null_filtered: true, interleave_in: \"albums\""), schema.string
end

def test_dump_schema_should_not_contain_id_limit
connection = ActiveRecord::Base.connection
connection = pool_or_connection
schema = StringIO.new
ActiveRecord::SchemaDumper.dump connection, schema
assert !schema.string.include?("id: { limit: 8 }")
end

def test_dump_schema_contains_commit_timestamp
connection = ActiveRecord::Base.connection
connection = pool_or_connection
schema = StringIO.new
ActiveRecord::SchemaDumper.dump connection, schema
assert schema.string.include?("t.time \"last_updated\", allow_commit_timestamp: true"), schema.string
end

def test_dump_schema_contains_virtual_column
connection = ActiveRecord::Base.connection
connection = pool_or_connection
schema = StringIO.new
ActiveRecord::SchemaDumper.dump connection, schema
assert schema.string.include?("t.virtual \"full_name\", type: :string, as: \"COALESCE(first_name || ' ', '') || last_name\", stored: true"), schema.string
end

def test_dump_schema_contains_string_array
connection = ActiveRecord::Base.connection
connection = pool_or_connection
schema = StringIO.new
ActiveRecord::SchemaDumper.dump connection, schema
assert schema.string.include?("t.string \"col_array_string\", array: true"), schema.string
end

def test_dump_schema_index_storing
connection = ActiveRecord::Base.connection
connection = pool_or_connection
schema = StringIO.new
ActiveRecord::SchemaDumper.dump connection, schema
assert schema.string.include?("t.index [\"last_name\"], name: \"index_singers_on_last_name\", order: { last_name: :asc }, storing: [\"first_name\", \"tracks_count\"]"), schema.string
Expand Down
5 changes: 4 additions & 1 deletion acceptance/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@
require "composite_primary_keys" if ActiveRecord::gem_version < Gem::Version.create('7.1.0')

# rubocop:disable Style/GlobalVars

#
if ActiveRecord.gem_version >= Gem::Version.create("7.2.0")
ActiveRecord::ConnectionAdapters.register("spanner", "ActiveRecord::ConnectionAdapters::SpannerAdapter")
end
$spanner_test_database = "ar-test-#{SecureRandom.hex 4}"

def connector_config
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,8 @@ def to_types_and_params binds
elsif bind.class == Symbol
# This ensures that for example :environment is sent as the string 'environment' to Cloud Spanner.
type = :STRING
elsif bind.class == TrueClass || bind.class == FalseClass
type = :BOOL
end
[
# Generates binds for named parameters in the format `@p1, @p2, ...`
Expand Down
18 changes: 15 additions & 3 deletions lib/active_record/connection_adapters/spanner/quoting.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,23 @@

module ActiveRecord
module ConnectionAdapters
QUOTED_COLUMN_NAMES = Concurrent::Map.new # :nodoc:
QUOTED_TABLE_NAMES = Concurrent::Map.new # :nodoc:

module Quoting
module ClassMethods
# This is used for ActiveRecord v8 and higher.
def quote_column_name name
QUOTED_COLUMN_NAMES[name] ||= "`#{name.to_s.gsub '`', '``'}`".freeze
end

def quote_table_name name
QUOTED_TABLE_NAMES[name] ||= "`#{name.to_s.gsub '.', '`.`'}`".freeze
end
end
end
module Spanner
module Quoting
QUOTED_COLUMN_NAMES = Concurrent::Map.new # :nodoc:
QUOTED_TABLE_NAMES = Concurrent::Map.new # :nodoc:

def quote_column_name name
QUOTED_COLUMN_NAMES[name] ||= "`#{name.to_s.gsub '`', '``'}`".freeze
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ module Spanner
module SchemaStatements
VERSION_6_1_0 = Gem::Version.create "6.1.0"
VERSION_6_0_3 = Gem::Version.create "6.0.3"
VERSION_7_2 = Gem::Version.create "7.2.0"

def current_database
@connection.database_id
Expand Down Expand Up @@ -401,6 +402,16 @@ def check_constraints table_name
information_schema { |i| i.check_constraints table_name }
end

if ActiveRecord.gem_version >= VERSION_7_2
def migration_context
pool.migration_context
end

def schema_migration
pool.schema_migration
end
end

def assume_migrated_upto_version version
version = version.to_i
sm_table = quote_table_name schema_migration.table_name
Expand Down
30 changes: 26 additions & 4 deletions lib/activerecord_spanner_adapter/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,19 @@ def self._should_use_standard_insert_record? values
!(buffered_mutations? || (primary_key && values.is_a?(Hash))) || !spanner_adapter?
end

def self._internal_insert_record values
if ActiveRecord.gem_version < VERSION_7_2
_insert_record values
else
_insert_record nil, values
end
end

def self._insert_record *args
if ActiveRecord.gem_version < VERSION_7_2
values, returning = args
else
_connection, values, returning = []
_connection, values, returning = args
end

if _should_use_standard_insert_record? values
Expand Down Expand Up @@ -126,6 +134,13 @@ def self.insert_all _attributes, **_kwargs
raise NotImplementedError, "Cloud Spanner does not support skip_duplicates. Use insert! or upsert instead."
end

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

insert_all! [attributes], returning: returning, **kwargs
end

def self.insert_all! attributes, returning: nil, **_kwargs
return super unless spanner_adapter?
return super if active_transaction? && !buffered_mutations?
Expand All @@ -134,18 +149,25 @@ def self.insert_all! attributes, returning: nil, **_kwargs
# The mutations will be sent as one batch when the transaction is committed.
if active_transaction?
attributes.each do |record|
_insert_record record
_internal_insert_record record
end
else
transaction isolation: :buffered_mutations do
attributes.each do |record|
_insert_record record
_internal_insert_record record
end
end
end
end

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

upsert_all [attributes], returning: returning, **kwargs
end

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. " \
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ module TestInterleavedTables_7_1_AndHigher
class InterleavedTablesTest < Minitest::Test
def setup
super
if ActiveRecord.version >= Gem::Version.create("7.2.0")
ActiveRecord::ConnectionAdapters.register("spanner", "ActiveRecord::ConnectionAdapters::SpannerAdapter")
end

@server = GRPC::RpcServer.new
@port = @server.add_http2_port "localhost:0", :this_port_is_insecure
@mock = SpannerMockServer.new
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ module MockServerTests
class BaseSpannerMockServerTest < Minitest::Test
def setup
super
if ActiveRecord.version >= Gem::Version.create("7.2.0")
ActiveRecord::ConnectionAdapters.register("spanner", "ActiveRecord::ConnectionAdapters::SpannerAdapter")
end

@server = GRPC::RpcServer.new
@port = @server.add_http2_port "localhost:0", :this_port_is_insecure
@mock = SpannerMockServer.new
Expand Down
Loading