From 3ef44219258d704e9fa366e4925b8ac509142b29 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Thu, 9 Oct 2025 13:18:37 +0200 Subject: [PATCH 01/27] feat: bump to Rails 8.1 --- activerecord-cockroachdb-adapter.gemspec | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/activerecord-cockroachdb-adapter.gemspec b/activerecord-cockroachdb-adapter.gemspec index 9492cb0e..4d61ca68 100644 --- a/activerecord-cockroachdb-adapter.gemspec +++ b/activerecord-cockroachdb-adapter.gemspec @@ -14,9 +14,9 @@ Gem::Specification.new do |spec| spec.description = "Allows the use of CockroachDB as a backend for ActiveRecord and Rails apps." spec.homepage = "https://github.com/cockroachdb/activerecord-cockroachdb-adapter" - spec.add_dependency "activerecord", "~> 8.0.0" + spec.add_dependency "activerecord", "~> 8.1.0" spec.add_dependency "pg", "~> 1.5" - spec.add_dependency "rgeo-activerecord", "~> 8.0.0" + spec.add_dependency "rgeo-activerecord", "~> 8.1.0" spec.add_development_dependency "benchmark-ips", "~> 2.9.1" From 22f79b7d2a9a8ee10be3c9d84cf3711dee5d45b3 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Thu, 16 Oct 2025 18:15:35 +0200 Subject: [PATCH 02/27] fix: add `cast_type` to `Column.new` --- lib/active_record/connection_adapters/cockroachdb/column.rb | 4 ++-- .../connection_adapters/cockroachdb/schema_statements.rb | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/active_record/connection_adapters/cockroachdb/column.rb b/lib/active_record/connection_adapters/cockroachdb/column.rb index fdaab7c7..eee40652 100644 --- a/lib/active_record/connection_adapters/cockroachdb/column.rb +++ b/lib/active_record/connection_adapters/cockroachdb/column.rb @@ -20,7 +20,7 @@ module CockroachDB class Column < PostgreSQL::Column # most functions taken from activerecord-postgis-adapter spatial_column # https://github.com/rgeo/activerecord-postgis-adapter/blob/master/lib/active_record/connection_adapters/postgis/spatial_column.rb - def initialize(name, default, sql_type_metadata = nil, null = true, + def initialize(name, cast_type, default, sql_type_metadata = nil, null = true, default_function = nil, collation: nil, comment: nil, identity: nil, serial: nil, spatial: nil, generated: nil, hidden: nil) @sql_type_metadata = sql_type_metadata @@ -45,7 +45,7 @@ def initialize(name, default, sql_type_metadata = nil, null = true, # @geometric_type = geo_type_from_sql_type(sql_type) build_from_sql_type(sql_type_metadata.sql_type) end - super(name, default, sql_type_metadata, null, default_function, + super(name, cast_type, default, sql_type_metadata, null, default_function, collation: collation, comment: comment, serial: serial, generated: generated, identity: identity) if spatial? && @srid @limit = { srid: @srid, type: to_type_name(geometric_type) } diff --git a/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb b/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb index ba5f469a..a7500781 100644 --- a/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb +++ b/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb @@ -250,6 +250,7 @@ def new_column_from_field(table_name, field, _definition) CockroachDB::Column.new( column_name, + get_oid_type(oid.to_i, fmod.to_i, column_name, type), default_value, type_metadata, !notnull, From 0ed7b7c06801ad6e7c4debc7be4201d13e9c53e5 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Fri, 17 Oct 2025 13:25:04 +0200 Subject: [PATCH 03/27] fix(test): don't alter backtrace from gems --- .../AssociationDeprecationTest/NotifyModeTest.rb | 2 ++ .../RaiseBacktraceModeTest.rb | 2 ++ .../AssociationDeprecationTest/RaiseModeTest.rb | 2 ++ .../WarnBacktraceModeTest.rb | 2 ++ .../AssociationDeprecationTest/WarnModeTest.rb | 2 ++ .../fix_backtrace_cleaner.rb | 10 ++++++++++ 6 files changed, 20 insertions(+) create mode 100644 test/excludes/AssociationDeprecationTest/NotifyModeTest.rb create mode 100644 test/excludes/AssociationDeprecationTest/RaiseBacktraceModeTest.rb create mode 100644 test/excludes/AssociationDeprecationTest/RaiseModeTest.rb create mode 100644 test/excludes/AssociationDeprecationTest/WarnBacktraceModeTest.rb create mode 100644 test/excludes/AssociationDeprecationTest/WarnModeTest.rb create mode 100644 test/excludes/AssociationDeprecationTest/fix_backtrace_cleaner.rb diff --git a/test/excludes/AssociationDeprecationTest/NotifyModeTest.rb b/test/excludes/AssociationDeprecationTest/NotifyModeTest.rb new file mode 100644 index 00000000..ed1f6040 --- /dev/null +++ b/test/excludes/AssociationDeprecationTest/NotifyModeTest.rb @@ -0,0 +1,2 @@ +require_relative "fix_backtrace_cleaner" +include(FixBacktraceCleaner) diff --git a/test/excludes/AssociationDeprecationTest/RaiseBacktraceModeTest.rb b/test/excludes/AssociationDeprecationTest/RaiseBacktraceModeTest.rb new file mode 100644 index 00000000..ed1f6040 --- /dev/null +++ b/test/excludes/AssociationDeprecationTest/RaiseBacktraceModeTest.rb @@ -0,0 +1,2 @@ +require_relative "fix_backtrace_cleaner" +include(FixBacktraceCleaner) diff --git a/test/excludes/AssociationDeprecationTest/RaiseModeTest.rb b/test/excludes/AssociationDeprecationTest/RaiseModeTest.rb new file mode 100644 index 00000000..ed1f6040 --- /dev/null +++ b/test/excludes/AssociationDeprecationTest/RaiseModeTest.rb @@ -0,0 +1,2 @@ +require_relative "fix_backtrace_cleaner" +include(FixBacktraceCleaner) diff --git a/test/excludes/AssociationDeprecationTest/WarnBacktraceModeTest.rb b/test/excludes/AssociationDeprecationTest/WarnBacktraceModeTest.rb new file mode 100644 index 00000000..ed1f6040 --- /dev/null +++ b/test/excludes/AssociationDeprecationTest/WarnBacktraceModeTest.rb @@ -0,0 +1,2 @@ +require_relative "fix_backtrace_cleaner" +include(FixBacktraceCleaner) diff --git a/test/excludes/AssociationDeprecationTest/WarnModeTest.rb b/test/excludes/AssociationDeprecationTest/WarnModeTest.rb new file mode 100644 index 00000000..ed1f6040 --- /dev/null +++ b/test/excludes/AssociationDeprecationTest/WarnModeTest.rb @@ -0,0 +1,2 @@ +require_relative "fix_backtrace_cleaner" +include(FixBacktraceCleaner) diff --git a/test/excludes/AssociationDeprecationTest/fix_backtrace_cleaner.rb b/test/excludes/AssociationDeprecationTest/fix_backtrace_cleaner.rb new file mode 100644 index 00000000..9ed08029 --- /dev/null +++ b/test/excludes/AssociationDeprecationTest/fix_backtrace_cleaner.rb @@ -0,0 +1,10 @@ +module FixBacktraceCleaner + def setup + super + bc = ActiveSupport::BacktraceCleaner.new + bc.remove_silencers! + bc.remove_filters! + bc.add_silencer { !_1.include?(::AssociationDeprecationTest::TestCase::THIS_FILE) } + ActiveRecord::LogSubscriber.backtrace_cleaner = bc + end +end From c26a1b34d1c3c007515d0fe5167d63784f710519 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Fri, 17 Oct 2025 15:09:03 +0200 Subject: [PATCH 04/27] fix(oid): freeze spatial oid Rails is moving towards ractor compatibility, and to do so it swapped to frozen database objects. Hence we cannot simply memoize the factories anymore. I've checked the initialization speed for factories, and the memoization was too fast for it to be worth implementing a cache. See https://github.com/rails/rails/commit/d2da81670c0049aeb2b7b062a80dd069ee2e725a See https://github.com/rails/rails/commit/76448a01667f9d477e6884a986276687167dfc83 --- activerecord-cockroachdb-adapter.gemspec | 10 ++++----- .../cockroachdb/oid/spatial.rb | 17 ++++++-------- .../cockroachdb_adapter.rb | 2 +- .../cases/adapters/postgresql/postgis_test.rb | 22 +++++-------------- .../postgresql/spatial_queries_test.rb | 2 -- 5 files changed, 18 insertions(+), 35 deletions(-) diff --git a/activerecord-cockroachdb-adapter.gemspec b/activerecord-cockroachdb-adapter.gemspec index 4d61ca68..7a372915 100644 --- a/activerecord-cockroachdb-adapter.gemspec +++ b/activerecord-cockroachdb-adapter.gemspec @@ -29,10 +29,8 @@ Gem::Specification.new do |spec| "public gem pushes." end - spec.files = `git ls-files -z`.split("\x0").reject do |f| - f.match(%r{^(test|spec|features)/}) - end - spec.bindir = "exe" - spec.executables = spec.files.grep(%r{^exe/}) { |f| File.basename(f) } - spec.require_paths = ["lib"] + git_files = `git ls-files -z`.split("\x0") + spec.files = (Dir["lib/**/*.rb"] + %w(LICENSE Rakefile Gemfile)) & git_files + spec.extra_rdoc_files = Dir["**/*.md"] & git_files + spec.test_files = Dir["test/**/*"] & git_files end diff --git a/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb b/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb index eed4ffc7..740b8421 100644 --- a/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb +++ b/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb @@ -28,6 +28,10 @@ class Spatial < Type::Value def initialize(oid, sql_type) @sql_type = sql_type @geo_type, @srid, @has_z, @has_m = self.class.parse_sql_type(sql_type) + @spatial_factory = + RGeo::ActiveRecord::SpatialFactoryStore.instance.factory( + factory_attrs + ) end # sql_type: geometry, geometry(Point), geometry(Point,4326), ... @@ -59,13 +63,6 @@ def self.parse_sql_type(sql_type) [geo_type, srid, has_z, has_m] end - def spatial_factory - @spatial_factory ||= - RGeo::ActiveRecord::SpatialFactoryStore.instance.factory( - factory_attrs - ) - end - def geographic? @sql_type =~ /geography/ end @@ -108,14 +105,14 @@ def parse_wkt(string) end def binary_string?(string) - string[0] == "\x00" || string[0] == "\x01" || string[0, 4] =~ /[0-9a-fA-F]{4}/ + string[0] == "\x00" || string[0] == "\x01" || string[0, 4].match?(/[0-9a-fA-F]{4}/) end def wkt_parser(string) if binary_string?(string) - RGeo::WKRep::WKBParser.new(spatial_factory, support_ewkb: true, default_srid: @srid) + RGeo::WKRep::WKBParser.new(@spatial_factory, support_ewkb: true, default_srid: @srid) else - RGeo::WKRep::WKTParser.new(spatial_factory, support_ewkt: true, default_srid: @srid) + RGeo::WKRep::WKTParser.new(@spatial_factory, support_ewkt: true, default_srid: @srid) end end diff --git a/lib/active_record/connection_adapters/cockroachdb_adapter.rb b/lib/active_record/connection_adapters/cockroachdb_adapter.rb index 3a98cbff..489ba726 100644 --- a/lib/active_record/connection_adapters/cockroachdb_adapter.rb +++ b/lib/active_record/connection_adapters/cockroachdb_adapter.rb @@ -299,7 +299,7 @@ def initialize_type_map(m = type_map) st_polygon ).each do |geo_type| m.register_type(geo_type) do |oid, _, sql_type| - CockroachDB::OID::Spatial.new(oid, sql_type) + CockroachDB::OID::Spatial.new(oid, sql_type).freeze end end diff --git a/test/cases/adapters/postgresql/postgis_test.rb b/test/cases/adapters/postgresql/postgis_test.rb index f3dd811d..64508a1d 100644 --- a/test/cases/adapters/postgresql/postgis_test.rb +++ b/test/cases/adapters/postgresql/postgis_test.rb @@ -6,8 +6,12 @@ class PostGISTest < ActiveRecord::PostgreSQLTestCase def setup @connection = ActiveRecord::Base.lease_connection + end + + def teardown spatial_factory_store.default = nil spatial_factory_store.clear + reset_memoized_spatial_factories end def test_postgis_available @@ -104,9 +108,6 @@ def test_custom_factory end def test_spatial_factory_attrs_parsing - klass.reset_column_information - reset_memoized_spatial_factories - factory = RGeo::Cartesian.preferred_factory(srid: 3857) spatial_factory_store.register(factory, { srid: 3857, sql_type: "geometry", @@ -121,13 +122,9 @@ def test_spatial_factory_attrs_parsing object.save! object.reload assert_equal(factory, object.boundary.factory) - - spatial_factory_store.clear end def test_spatial_factory_retrieval - reset_memoized_spatial_factories - geo_factory = RGeo::Geographic.spherical_factory(srid: 4326) spatial_factory_store.register(geo_factory, geo_type: "point", sql_type: "geography") @@ -141,8 +138,6 @@ def test_spatial_factory_retrieval object.save! object.reload refute_equal geo_factory, object.shape.factory - - spatial_factory_store.clear end def test_point_to_json @@ -185,12 +180,7 @@ def reset_memoized_spatial_factories # necessary to reset the @spatial_factory variable on spatial # OIDs, otherwise the results of early tests will be memoized # since the table is not dropped and recreated between test cases. - ObjectSpace.each_object(spatial_oid) do |oid| - oid.instance_variable_set(:@spatial_factory, nil) - end - end - - def spatial_oid - ActiveRecord::ConnectionAdapters::CockroachDB::OID::Spatial + klass.lease_connection.send(:reload_type_map) + klass.reset_column_information end end diff --git a/test/cases/adapters/postgresql/spatial_queries_test.rb b/test/cases/adapters/postgresql/spatial_queries_test.rb index 9199c01f..c2c2691a 100644 --- a/test/cases/adapters/postgresql/spatial_queries_test.rb +++ b/test/cases/adapters/postgresql/spatial_queries_test.rb @@ -6,8 +6,6 @@ class SpatialQueriesTest < ActiveSupport::TestCase def setup Building.delete_all - spatial_factory_store.clear - spatial_factory_store.default = nil end def test_query_point From bb34d481c299604bcf73deac45e7689051fff054 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Fri, 17 Oct 2025 15:55:24 +0200 Subject: [PATCH 05/27] fix: update run_command execution --- .../connection_adapters/cockroachdb/database_tasks.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/active_record/connection_adapters/cockroachdb/database_tasks.rb b/lib/active_record/connection_adapters/cockroachdb/database_tasks.rb index b951e4f6..c5abc93e 100644 --- a/lib/active_record/connection_adapters/cockroachdb/database_tasks.rb +++ b/lib/active_record/connection_adapters/cockroachdb/database_tasks.rb @@ -74,7 +74,7 @@ def structure_load(filename, extra_flags=nil) "https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/new" end - run_cmd("cockroach", ["sql", "--set", "errexit=false", "--file", filename], "loading") + run_cmd("cockroach", "sql", "--set", "errexit=false", "--file", filename) end private From 1bf0a3f3dca629935a3fc71cd87bd29aca4c41f4 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Thu, 23 Oct 2025 16:58:12 +0200 Subject: [PATCH 06/27] fix: use class level native_database_type --- .../cockroachdb/schema_statements.rb | 17 ----------------- .../connection_adapters/cockroachdb_adapter.rb | 17 +++++++++++++++++ 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb b/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb index a7500781..17aa9425 100644 --- a/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb +++ b/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb @@ -296,23 +296,6 @@ def type_to_sql(type, limit: nil, precision: nil, scale: nil, array: nil, **) # sql end - # override - def native_database_types - # Add spatial types - super.merge( - geography: { name: "geography" }, - geometry: { name: "geometry" }, - geometry_collection: { name: "geometry_collection" }, - line_string: { name: "line_string" }, - multi_line_string: { name: "multi_line_string" }, - multi_point: { name: "multi_point" }, - multi_polygon: { name: "multi_polygon" }, - spatial: { name: "geometry" }, - st_point: { name: "st_point" }, - st_polygon: { name: "st_polygon" } - ) - end - # override def create_table_definition(*args, **kwargs) CockroachDB::TableDefinition.new(self, *args, **kwargs) diff --git a/lib/active_record/connection_adapters/cockroachdb_adapter.rb b/lib/active_record/connection_adapters/cockroachdb_adapter.rb index 489ba726..009b0675 100644 --- a/lib/active_record/connection_adapters/cockroachdb_adapter.rb +++ b/lib/active_record/connection_adapters/cockroachdb_adapter.rb @@ -111,6 +111,23 @@ def self.spatial_column_options(key) SPATIAL_COLUMN_OPTIONS[key] end + def self.native_database_types + return @native_database_types if defined?(@native_database_types) + # Add spatial types + @native_database_types = super.merge( + geography: { name: "geography" }, + geometry: { name: "geometry" }, + geometry_collection: { name: "geometry_collection" }, + line_string: { name: "line_string" }, + multi_line_string: { name: "multi_line_string" }, + multi_point: { name: "multi_point" }, + multi_polygon: { name: "multi_polygon" }, + spatial: { name: "geometry" }, + st_point: { name: "st_point" }, + st_polygon: { name: "st_polygon" } + ) + end + def postgis_lib_version @postgis_lib_version ||= select_value("SELECT PostGIS_Lib_Version()") end From a4520921b2f0ed8a1bdaaed116d9a02cac93f964 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Fri, 24 Oct 2025 14:28:22 +0200 Subject: [PATCH 07/27] fix(test): default is now serialized See https://github.com/rails/rails/commit/205cdd50ba626325b26ce2919d5e961c6df50df0 --- test/cases/defaults_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cases/defaults_test.rb b/test/cases/defaults_test.rb index ca6cadc2..5fbdd995 100644 --- a/test/cases/defaults_test.rb +++ b/test/cases/defaults_test.rb @@ -39,7 +39,7 @@ class DefaultNumber < ActiveRecord::Base; end def test_default_decimal_zero_with_large_scale record = DefaultNumber.new assert_equal 0.0, record.decimal_number - assert_equal "0.0000000000000000", record.decimal_number_before_type_cast + assert_equal 0.0, record.decimal_number_before_type_cast end end end From 8c8e256641f828090bd1c078f4fa7d834acc3408 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Thu, 30 Oct 2025 11:12:37 +0100 Subject: [PATCH 08/27] fix(schema): hidden columns - update some outdated overrides. - update override comments to know when was the last override. - remove the `#column_names_from_column_numbers` method that was generating N+1 queries. - update related methods. See: - https://github.com/rails/rails/commit/249c367b4d3a65a3c0406c4a508cb2ba0493eed9 - https://github.com/rails/rails/commit/4e0546c5e6c2aef4bb1db771492be0bdc1202eab - https://github.com/rails/rails/commit/c93d1b09fcc013033af506b10fd60829267be85c --- .../cockroachdb/referential_integrity.rb | 48 ++-- .../cockroachdb/schema_statements.rb | 246 +++++++++++++----- .../cockroachdb_adapter.rb | 32 +-- 3 files changed, 227 insertions(+), 99 deletions(-) diff --git a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb index fa4cb179..a8b138ad 100644 --- a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb +++ b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb @@ -85,8 +85,8 @@ def disable_referential_integrity private - # Copy/paste of the `#foreign_keys(table)` method adapted to return every single - # foreign key in the database. + # NOTE: Copy/paste of the `#foreign_keys(table)` method adapted + # to return every single foreign key in the database. def all_foreign_keys fk_info = internal_exec_query(<<~SQL, "SCHEMA") SELECT CASE @@ -99,14 +99,30 @@ def all_foreign_keys THEN '' ELSE n2.nspname || '.' END || t2.relname AS to_table, - a1.attname AS column, a2.attname AS primary_key, c.conname AS name, c.confupdtype AS on_update, c.confdeltype AS on_delete, c.convalidated AS valid, c.condeferrable AS deferrable, c.condeferred AS deferred, - c.conkey, c.confkey, c.conrelid, c.confrelid + c.conname AS name, c.confupdtype AS on_update, c.confdeltype AS on_delete, c.convalidated AS valid, c.condeferrable AS deferrable, c.condeferred AS deferred, c.conrelid, c.confrelid, + ( + SELECT array_agg(a.attname ORDER BY idx) + FROM ( + SELECT idx, c.conkey[idx] AS conkey_elem + FROM generate_subscripts(c.conkey, 1) AS idx + ) indexed_conkeys + JOIN pg_attribute a ON a.attrelid = t1.oid + AND a.attnum = indexed_conkeys.conkey_elem + AND NOT a.attishidden + ) AS conkey_names, + ( + SELECT array_agg(a.attname ORDER BY idx) + FROM ( + SELECT idx, c.confkey[idx] AS confkey_elem + FROM generate_subscripts(c.confkey, 1) AS idx + ) indexed_confkeys + JOIN pg_attribute a ON a.attrelid = t2.oid + AND a.attnum = indexed_confkeys.confkey_elem + AND NOT a.attishidden + ) AS confkey_names FROM pg_constraint c JOIN pg_class t1 ON c.conrelid = t1.oid JOIN pg_class t2 ON c.confrelid = t2.oid - JOIN pg_attribute a1 ON a1.attnum = c.conkey[1] AND a1.attrelid = t1.oid - JOIN pg_attribute a2 ON a2.attnum = c.confkey[1] AND a2.attrelid = t2.oid - JOIN pg_namespace t3 ON c.connamespace = t3.oid JOIN pg_namespace n1 ON t1.relnamespace = n1.oid JOIN pg_namespace n2 ON t2.relnamespace = n2.oid WHERE c.contype = 'f' @@ -116,22 +132,16 @@ def all_foreign_keys fk_info.map do |row| from_table = PostgreSQL::Utils.unquote_identifier(row["from_table"]) to_table = PostgreSQL::Utils.unquote_identifier(row["to_table"]) - conkey = row["conkey"].scan(/\d+/).map(&:to_i) - confkey = row["confkey"].scan(/\d+/).map(&:to_i) - - if conkey.size > 1 - column = column_names_from_column_numbers(row["conrelid"], conkey) - primary_key = column_names_from_column_numbers(row["confrelid"], confkey) - else - column = PostgreSQL::Utils.unquote_identifier(row["column"]) - primary_key = row["primary_key"] - end + + column = decode_string_array(row["conkey_names"]) + primary_key = decode_string_array(row["confkey_names"]) options = { - column: column, + column: column.size == 1 ? column.first : column, name: row["name"], - primary_key: primary_key + primary_key: primary_key.size == 1 ? primary_key.first : primary_key } + options[:on_delete] = extract_foreign_key_action(row["on_delete"]) options[:on_update] = extract_foreign_key_action(row["on_update"]) options[:deferrable] = extract_constraint_deferrable(row["deferrable"], row["deferred"]) diff --git a/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb b/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb index 17aa9425..d850ec04 100644 --- a/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb +++ b/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb @@ -20,6 +20,91 @@ module CockroachDB module SchemaStatements include ActiveRecord::ConnectionAdapters::PostgreSQL::SchemaStatements + # OVERRIDE(v8.1.1): + # - prepend Utils with PostgreSQL:: + # - handle hidden attributes + # Returns an array of indexes for the given table. + def indexes(table_name) # :nodoc: + scope = quoted_scope(table_name) + + result = query(<<~SQL, "SCHEMA") + SELECT distinct i.relname, d.indisunique, d.indkey, pg_get_indexdef(d.indexrelid), + pg_catalog.obj_description(i.oid, 'pg_class') AS comment, d.indisvalid, + ARRAY( + SELECT pg_get_indexdef(d.indexrelid, k + 1, true) + FROM generate_subscripts(d.indkey, 1) AS k + ORDER BY k + ) AS columns, + ARRAY( + SELECT a.attname + FROM pg_attribute a + WHERE a.attrelid = d.indexrelid AND a.attishidden + ) AS hidden_columns + FROM pg_class t + INNER JOIN pg_index d ON t.oid = d.indrelid + INNER JOIN pg_class i ON d.indexrelid = i.oid + LEFT JOIN pg_namespace n ON n.oid = t.relnamespace + WHERE i.relkind IN ('i', 'I') + AND d.indisprimary = 'f' + AND t.relname = #{scope[:name]} + AND n.nspname = #{scope[:schema]} + ORDER BY i.relname + SQL + + unquote = -> (column) { + PostgreSQL::Utils.unquote_identifier(column.strip.gsub('""', '"')) + } + result.map do |row| + index_name = row[0] + unique = row[1] + indkey = row[2].split(" ").map(&:to_i) + inddef = row[3] + comment = row[4] + valid = row[5] + columns = decode_string_array(row[6]).map(&unquote) + hidden_columns = decode_string_array(row[7]).map(&unquote) + + using, expressions, include, nulls_not_distinct, where = inddef.scan(/ USING (\w+?) \((.+?)\)(?: INCLUDE \((.+?)\))?( NULLS NOT DISTINCT)?(?: WHERE (.+))?\z/m).flatten + + orders = {} + opclasses = {} + include_columns = include ? include.split(",").map(&unquote) : [] + + if indkey.include?(0) + columns = expressions + else + # prevent INCLUDE and hidden columns from being matched + columns.reject! { |c| include_columns.include?(c) || hidden_columns.include?(c) } + + # add info on sort order (only desc order is explicitly specified, asc is the default) + # and non-default opclasses + expressions.scan(/(?\w+)"?\s?(?\w+_ops(_\w+)?)?\s?(?DESC)?\s?(?NULLS (?:FIRST|LAST))?/).each do |column, opclass, desc, nulls| + opclasses[column] = opclass.to_sym if opclass + if nulls + orders[column] = [desc, nulls].compact.join(" ") + else + orders[column] = :desc if desc + end + end + end + + IndexDefinition.new( + table_name, + index_name, + unique, + columns, + orders: orders, + opclasses: opclasses, + where: where, + using: using.to_sym, + include: include_columns.presence, + nulls_not_distinct: nulls_not_distinct.present?, + comment: comment.presence, + valid: valid + ) + end + end + # OVERRIDE: We do not want to see the crdb_internal schema in the names. # # Returns an array of schema names. @@ -55,34 +140,18 @@ def primary_key(table_name) end end + # OVERRIDE(v8.1.1): handle hidden attributes def primary_keys(table_name) - return super unless database_version >= 24_02_02 - query_values(<<~SQL, "SCHEMA") SELECT a.attname - FROM ( - SELECT indrelid, indkey, generate_subscripts(indkey, 1) idx - FROM pg_index - WHERE indrelid = #{quote(quote_table_name(table_name))}::regclass - AND indisprimary - ) i - JOIN pg_attribute a - ON a.attrelid = i.indrelid - AND a.attnum = i.indkey[i.idx] - AND NOT a.attishidden - ORDER BY i.idx - SQL - end - - def column_names_from_column_numbers(table_oid, column_numbers) - return super unless database_version >= 24_02_02 - - Hash[query(<<~SQL, "SCHEMA")].values_at(*column_numbers).compact - SELECT a.attnum, a.attname - FROM pg_attribute a - WHERE a.attrelid = #{table_oid} - AND a.attnum IN (#{column_numbers.join(", ")}) - AND NOT a.attishidden + FROM pg_index i + JOIN pg_attribute a + ON a.attrelid = i.indrelid + AND a.attnum = ANY(i.indkey) + AND NOT a.attishidden + WHERE i.indrelid = #{quote(quote_table_name(table_name))}::regclass + AND i.indisprimary + ORDER BY array_position(i.indkey, a.attnum) SQL end @@ -94,7 +163,7 @@ def foreign_key_options(from_table, to_table, options) options end - # OVERRIDE: Added `unique_rowid` to the last line of the second query. + # OVERRIDE(v8.1.1): Added `unique_rowid` to the last line of the second query. # This is a CockroachDB-specific function used for primary keys. # Also make sure we don't consider `NOT VISIBLE` columns. # @@ -108,11 +177,7 @@ def pk_and_sequence_for(table) # :nodoc: pg_attribute attr, pg_depend dep, pg_constraint cons, - pg_namespace nsp, - -- TODO: use the pg_catalog.pg_attribute(attishidden) column when - -- it is added instead of joining on crdb_internal. - -- See https://github.com/cockroachdb/cockroach/pull/126397 - crdb_internal.table_columns tc + pg_namespace nsp WHERE seq.oid = dep.objid AND seq.relkind = 'S' AND attr.attrelid = dep.refobjid @@ -120,12 +185,10 @@ def pk_and_sequence_for(table) # :nodoc: AND attr.attrelid = cons.conrelid AND attr.attnum = cons.conkey[1] AND seq.relnamespace = nsp.oid - AND attr.attrelid = tc.descriptor_id - AND attr.attname = tc.column_name - AND tc.hidden = false AND cons.contype = 'p' AND dep.classid = 'pg_class'::regclass AND dep.refobjid = #{quote(quote_table_name(table))}::regclass + AND not attr.attishidden SQL if result.nil? || result.empty? @@ -143,12 +206,8 @@ def pk_and_sequence_for(table) # :nodoc: JOIN pg_attrdef def ON (adrelid = attrelid AND adnum = attnum) JOIN pg_constraint cons ON (conrelid = adrelid AND adnum = conkey[1]) JOIN pg_namespace nsp ON (t.relnamespace = nsp.oid) - -- TODO: use the pg_catalog.pg_attribute(attishidden) column when - -- it is added instead of joining on crdb_internal. - -- See https://github.com/cockroachdb/cockroach/pull/126397 - JOIN crdb_internal.table_columns tc ON (attr.attrelid = tc.descriptor_id AND attr.attname = tc.column_name) WHERE t.oid = #{quote(quote_table_name(table))}::regclass - AND tc.hidden = false + AND NOT attr.attishidden AND cons.contype = 'p' AND pg_get_expr(def.adbin, def.adrelid) ~* 'nextval|uuid_generate|gen_random_uuid|unique_rowid' SQL @@ -164,53 +223,66 @@ def pk_and_sequence_for(table) # :nodoc: nil end - # override - # Modified version of the postgresql foreign_keys method. - # Replaces t2.oid::regclass::text with t2.relname since this is - # more efficient in CockroachDB. - # Also, CockroachDB does not append the schema name in relname, - # so we append it manually. + # OVERRIDE(v8.1.1): + # - Replaces t2.oid::regclass::text with t2.relname + # since this is more efficient in CockroachDB. + # - prepend schema name to relname (see `AS to_table`) + # - handle hidden attributes. + # + # NOTE: If you edit this method, you'll need to edit + # the `#all_foreign_keys` method as well. def foreign_keys(table_name) scope = quoted_scope(table_name) - fk_info = internal_exec_query(<<~SQL, "SCHEMA") + fk_info = internal_exec_query(<<~SQL, "SCHEMA", allow_retry: true, materialize_transactions: false) SELECT CASE WHEN n2.nspname = current_schema() THEN '' ELSE n2.nspname || '.' END || t2.relname AS to_table, - a1.attname AS column, a2.attname AS primary_key, c.conname AS name, c.confupdtype AS on_update, c.confdeltype AS on_delete, c.convalidated AS valid, c.condeferrable AS deferrable, c.condeferred AS deferred, - c.conkey, c.confkey, c.conrelid, c.confrelid + c.conname AS name, c.confupdtype AS on_update, c.confdeltype AS on_delete, c.convalidated AS valid, c.condeferrable AS deferrable, c.condeferred AS deferred, c.conrelid, c.confrelid, + ( + SELECT array_agg(a.attname ORDER BY idx) + FROM ( + SELECT idx, c.conkey[idx] AS conkey_elem + FROM generate_subscripts(c.conkey, 1) AS idx + ) indexed_conkeys + JOIN pg_attribute a ON a.attrelid = t1.oid + AND a.attnum = indexed_conkeys.conkey_elem + AND NOT a.attishidden + ) AS conkey_names, + ( + SELECT array_agg(a.attname ORDER BY idx) + FROM ( + SELECT idx, c.confkey[idx] AS confkey_elem + FROM generate_subscripts(c.confkey, 1) AS idx + ) indexed_confkeys + JOIN pg_attribute a ON a.attrelid = t2.oid + AND a.attnum = indexed_confkeys.confkey_elem + AND NOT a.attishidden + ) AS confkey_names FROM pg_constraint c JOIN pg_class t1 ON c.conrelid = t1.oid JOIN pg_class t2 ON c.confrelid = t2.oid - JOIN pg_attribute a1 ON a1.attnum = c.conkey[1] AND a1.attrelid = t1.oid - JOIN pg_attribute a2 ON a2.attnum = c.confkey[1] AND a2.attrelid = t2.oid - JOIN pg_namespace t3 ON c.connamespace = t3.oid + JOIN pg_namespace n1 ON t1.relnamespace = n1.oid JOIN pg_namespace n2 ON t2.relnamespace = n2.oid WHERE c.contype = 'f' AND t1.relname = #{scope[:name]} - AND t3.nspname = #{scope[:schema]} + AND n1.nspname = #{scope[:schema]} ORDER BY c.conname SQL fk_info.map do |row| to_table = PostgreSQL::Utils.unquote_identifier(row["to_table"]) - conkey = row["conkey"].scan(/\d+/).map(&:to_i) - confkey = row["confkey"].scan(/\d+/).map(&:to_i) - if conkey.size > 1 - column = column_names_from_column_numbers(row["conrelid"], conkey) - primary_key = column_names_from_column_numbers(row["confrelid"], confkey) - else - column = PostgreSQL::Utils.unquote_identifier(row["column"]) - primary_key = row["primary_key"] - end + column = decode_string_array(row["conkey_names"]) + primary_key = decode_string_array(row["confkey_names"]) options = { - column: column, + column: column.size == 1 ? column.first : column, name: row["name"], - primary_key: primary_key + primary_key: primary_key.size == 1 ? primary_key.first : primary_key } + options[:on_delete] = extract_foreign_key_action(row["on_delete"]) options[:on_update] = extract_foreign_key_action(row["on_update"]) options[:deferrable] = extract_constraint_deferrable(row["deferrable"], row["deferred"]) @@ -228,8 +300,52 @@ def default_sequence_name(table_name, pk = "id") nil end - # override - # https://github.com/rails/rails/blob/6-0-stable/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb#L624 + # OVERRIDE(v8.1.1): handle hidden attributes + # + # Returns an array of unique constraints for the given table. + # The unique constraints are represented as UniqueConstraintDefinition objects. + def unique_constraints(table_name) + scope = quoted_scope(table_name) + + unique_info = internal_exec_query(<<~SQL, "SCHEMA", allow_retry: true, materialize_transactions: false) + SELECT c.conname, c.conrelid, c.condeferrable, c.condeferred, pg_get_constraintdef(c.oid) AS constraintdef, + ( + SELECT array_agg(a.attname ORDER BY idx) + FROM ( + SELECT idx, c.conkey[idx] AS conkey_elem + FROM generate_subscripts(c.conkey, 1) AS idx + ) indexed_conkeys + JOIN pg_attribute a ON a.attrelid = t.oid + AND a.attnum = indexed_conkeys.conkey_elem + AND NOT a.attishidden + ) AS conkey_names + FROM pg_constraint c + JOIN pg_class t ON c.conrelid = t.oid + JOIN pg_namespace n ON n.oid = c.connamespace + WHERE c.contype = 'u' + AND t.relname = #{scope[:name]} + AND n.nspname = #{scope[:schema]} + SQL + + unique_info.map do |row| + columns = decode_string_array(row["conkey_names"]) + + nulls_not_distinct = row["constraintdef"].start_with?("UNIQUE NULLS NOT DISTINCT") + deferrable = extract_constraint_deferrable(row["condeferrable"], row["condeferred"]) + + options = { + name: row["conname"], + nulls_not_distinct: nulls_not_distinct, + deferrable: deferrable + } + + UniqueConstraintDefinition.new(table_name, columns, options) + end + end + + # OVERRIDE(v8.1.1): + # - Add spatial information + # - Add hidden information def new_column_from_field(table_name, field, _definition) column_name, type, default, notnull, oid, fmod, collation, comment, identity, attgenerated, hidden = field type_metadata = fetch_type_metadata(column_name, type, oid.to_i, fmod.to_i) diff --git a/lib/active_record/connection_adapters/cockroachdb_adapter.rb b/lib/active_record/connection_adapters/cockroachdb_adapter.rb index 009b0675..71183a0d 100644 --- a/lib/active_record/connection_adapters/cockroachdb_adapter.rb +++ b/lib/active_record/connection_adapters/cockroachdb_adapter.rb @@ -395,18 +395,10 @@ def extract_decimal_from_default(default) nil end - # override - # This method makes a query to gather information about columns - # in a table. It returns an array of arrays (one for each col) and - # passes each to the SchemaStatements#new_column_from_field method - # as the field parameter. This data is then used to format the column - # objects for the model and sent to the OID for data casting. - # - # Sometimes there are differences between how data is formatted - # in Postgres and CockroachDB, so additional queries for certain types - # may be necessary to properly form the column definition. - # - # @see: https://github.com/rails/rails/blob/8695b028261bdd244e254993255c6641bdbc17a5/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L829 + # OVERRIDE(v8.1.1): + # - comment is retrieved differently than PG for performance + # - gather detailed information about spatial columns. See + # `#crdb_column_definitions` def column_definitions(table_name) fields = query(<<~SQL, "SCHEMA") SELECT a.attname, format_type(a.atttypid, a.atttypmod), @@ -414,7 +406,7 @@ def column_definitions(table_name) c.collname, NULL AS comment, attidentity, attgenerated, - NULL as is_hidden + a.attishidden FROM pg_attribute a LEFT JOIN pg_attrdef d ON a.attrelid = d.adrelid AND a.attnum = d.adnum LEFT JOIN pg_type t ON a.atttypid = t.oid @@ -445,7 +437,6 @@ def column_definitions(table_name) dtype = field[f_type] field[f_type] = crdb_fields[field[f_attname]][2].downcase if re.match(dtype) field[f_comment] = crdb_fields[field[f_attname]][1] - field[f_is_hidden] = true if crdb_fields[field[f_attname]][3] field end fields.delete_if do |field| @@ -461,13 +452,24 @@ def column_definitions(table_name) # column_definitions. This will include limit, # precision, and scale information in the type. # Ex. geometry -> geometry(point, 4326) + # + # The performance difference to fetch comments is very + # significant. On a M1 Mac, CockroachDB v25.2.2 and + # data from the test suite, it is 20x times faster. + # + # SELECT col_description(a.attrelid, a.attnum) + # FROM pg_attribute a; -- 1.052s + # + # SELECT c.column_comment + # FROM information_schema.columns c; -- 50ms + # def crdb_column_definitions(table_name) table_name = PostgreSQL::Utils.extract_schema_qualified_name(table_name) table = table_name.identifier with_schema = " AND c.table_schema = #{quote(table_name.schema)}" if table_name.schema fields = \ query(<<~SQL, "SCHEMA") - SELECT c.column_name, c.column_comment, c.crdb_sql_type, c.is_hidden::BOOLEAN + SELECT c.column_name, c.column_comment, c.crdb_sql_type FROM information_schema.columns c WHERE c.table_name = #{quote(table)}#{with_schema} SQL From 8072f97007da43a592279997fe80ded1f26f3452 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Thu, 30 Oct 2025 13:44:56 +0100 Subject: [PATCH 09/27] fix(ci): smaller summary without skips --- .github/workflows/ci.yml | 5 ++++- .github/workflows/flaky.yml | 8 ++++++-- .../connection_adapters/cockroachdb/quoting.rb | 7 ++++--- test/cases/helper_cockroachdb.rb | 2 +- 4 files changed, 15 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f81df9c6..49158499 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -58,7 +58,7 @@ jobs: EOF jq --slurp --raw-output ' - map(.failed_tests|map("\(.klass)#\(.NAME)")) | flatten | unique | sort | to_entries[] + map(.failed_tests|map("\(.klass)#\(.NAME)")) | flatten | unique | sort[0:100] | to_entries[] | "\(.key)\(.value)" ' reports/*/report.json >>$GITHUB_STEP_SUMMARY @@ -67,6 +67,9 @@ jobs: EOF + # Do not print json if too large. + [[ "$(du -s reports | cut -f1)" -gt 124 ]] && exit 0 + echo >>$GITHUB_STEP_SUMMARY echo '```json' >>$GITHUB_STEP_SUMMARY jq --slurp --compact-output '.' reports/*/report.json >>$GITHUB_STEP_SUMMARY diff --git a/.github/workflows/flaky.yml b/.github/workflows/flaky.yml index fdcd0e06..1c9e6f9c 100644 --- a/.github/workflows/flaky.yml +++ b/.github/workflows/flaky.yml @@ -72,7 +72,7 @@ jobs: name: report-${{ matrix.crdb }}-${{ matrix.ruby }}-${{ matrix.seed }} path: report.json collect: - if: failure() + if: failure() || cancelled() needs: test runs-on: ubuntu-latest steps: @@ -97,7 +97,7 @@ jobs: EOF jq --slurp --raw-output ' - sort_by(.total_time)[] + sort_by(.total_time)[0:100][] | {seed, time: .total_time | strftime("%H:%M:%S"), failure: .failed_tests[0].NAME } | "\(.seed)\(.time)\(.failure)" ' reports/*/report.json >> $GITHUB_STEP_SUMMARY @@ -107,6 +107,10 @@ jobs: EOF + + # Do not print json if too large. + [[ "$(du -s reports | cut -f1)" -gt 124 ]] && exit 0 + echo >> $GITHUB_STEP_SUMMARY echo '```json' >> $GITHUB_STEP_SUMMARY jq --slurp --compact-output '.' reports/*/report.json >> $GITHUB_STEP_SUMMARY diff --git a/lib/active_record/connection_adapters/cockroachdb/quoting.rb b/lib/active_record/connection_adapters/cockroachdb/quoting.rb index 44decd30..cd1f86ec 100644 --- a/lib/active_record/connection_adapters/cockroachdb/quoting.rb +++ b/lib/active_record/connection_adapters/cockroachdb/quoting.rb @@ -34,7 +34,8 @@ module Quoting # but when creating objects, using RGeo features is more convenient than # converting to WKB, so this does it automatically. def quote(value) - if value.is_a?(Numeric) + case value + when Numeric # NOTE: The fact that integers are quoted is important and helps # mitigate a potential vulnerability. # @@ -42,9 +43,9 @@ def quote(value) # - https://nvd.nist.gov/vuln/detail/CVE-2022-44566 # - https://github.com/cockroachdb/activerecord-cockroachdb-adapter/pull/280#discussion_r1288692977 "'#{quote_string(value.to_s)}'" - elsif RGeo::Feature::Geometry.check_type(value) + when RGeo::Feature::Geometry "'#{RGeo::WKRep::WKBGenerator.new(hex_format: true, type_format: :ewkb, emit_ewkb_srid: true).generate(value)}'" - elsif value.is_a?(RGeo::Cartesian::BoundingBox) + when RGeo::Cartesian::BoundingBox "'#{value.min_x},#{value.min_y},#{value.max_x},#{value.max_y}'::box" else super diff --git a/test/cases/helper_cockroachdb.rb b/test/cases/helper_cockroachdb.rb index 6ff746e3..b2415b53 100644 --- a/test/cases/helper_cockroachdb.rb +++ b/test/cases/helper_cockroachdb.rb @@ -180,7 +180,7 @@ def report seed: Minitest.seed, assertions: assertions, count: count, - failed_tests: results, + failed_tests: results.reject(&:skipped?), total_time: total_time, failures: failures, errors: errors, From 41622c06513319bfbae97126e85d12593dcb57e0 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Thu, 30 Oct 2025 16:04:44 +0100 Subject: [PATCH 10/27] fix(test): us english names --- .../ConnectionAdapters/PostgreSQLAdapterTest.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/excludes/ActiveRecord/ConnectionAdapters/PostgreSQLAdapterTest.rb b/test/excludes/ActiveRecord/ConnectionAdapters/PostgreSQLAdapterTest.rb index b45bcd60..d63b7d17 100644 --- a/test/excludes/ActiveRecord/ConnectionAdapters/PostgreSQLAdapterTest.rb +++ b/test/excludes/ActiveRecord/ConnectionAdapters/PostgreSQLAdapterTest.rb @@ -24,11 +24,11 @@ # NOTE: The expression `do $$ BEGIN RAISE WARNING 'foo'; END; $$` works with PG, not CRDB. plpgsql_needed = "PL-PGSQL differs in CockroachDB. Not tested yet. See #339" -exclude :test_ignores_warnings_when_behaviour_ignore, plpgsql_needed -exclude :test_logs_warnings_when_behaviour_log, plpgsql_needed -exclude :test_raises_warnings_when_behaviour_raise, plpgsql_needed -exclude :test_reports_when_behaviour_report, plpgsql_needed -exclude :test_warnings_behaviour_can_be_customized_with_a_proc, plpgsql_needed +exclude :test_ignores_warnings_when_behavior_ignore, plpgsql_needed +exclude :test_logs_warnings_when_behavior_log, plpgsql_needed +exclude :test_raises_warnings_when_behavior_raise, plpgsql_needed +exclude :test_reports_when_behavior_report, plpgsql_needed +exclude :test_warnings_behavior_can_be_customized_with_a_proc, plpgsql_needed exclude :test_allowlist_of_warnings_to_ignore, plpgsql_needed exclude :test_allowlist_of_warning_codes_to_ignore, plpgsql_needed From b7d065641b8fa163a0c7a724b60fef8361406e9c Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Thu, 30 Oct 2025 16:05:17 +0100 Subject: [PATCH 11/27] fix(test): coerced value for generate column --- test/excludes/PostgresqlVirtualColumnTest.rb | 15 +++++++++++++++ test/support/copy_cat.rb | 3 +++ 2 files changed, 18 insertions(+) diff --git a/test/excludes/PostgresqlVirtualColumnTest.rb b/test/excludes/PostgresqlVirtualColumnTest.rb index 059cbf5e..5c2f7bc7 100644 --- a/test/excludes/PostgresqlVirtualColumnTest.rb +++ b/test/excludes/PostgresqlVirtualColumnTest.rb @@ -1,2 +1,17 @@ +require "support/copy_cat" + exclude :test_build_fixture_sql, "Skipping because CockroachDB cannot write directly to computed columns." exclude :test_schema_dumping, "Replaced with local version" + +# CRDB doesn't support implicit casts. +# See https://github.com/cockroachdb/cockroach/issues/75101 +# +# From: "ASCII(name)" +# To: "ASCII(name)""::string" +CopyCat.copy_methods(self, self, :test_change_table_without_stored_option) do + def on_str(node) + return unless node in [:str, "ASCII(name)"] + + insert_after(node.loc.expression, '"::string"') + end +end diff --git a/test/support/copy_cat.rb b/test/support/copy_cat.rb index 30803fc5..4c4ec79a 100644 --- a/test/support/copy_cat.rb +++ b/test/support/copy_cat.rb @@ -21,6 +21,9 @@ def warn(message, category: nil, **kwargs) # Copy methods from The original rails class to our adapter. # While copying, we can rewrite the source code of the method using # ast. Use `debug: true` to lead you through that process. + # + # Once debug is set, you can check the closest node you want to edit + # and then create a method `on_` to handle it. def copy_methods(new_klass, old_klass, *methods, debug: false, &block) methods.each do |met| file, _ = old_klass.instance_method(met).source_location From 96c5cb647f9dbc533d305ef5aef736bd5df7a1db Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Mon, 3 Nov 2025 14:19:20 +0100 Subject: [PATCH 12/27] fix(referential-integrity): make sure fks are added back This would silently fail in the test `test_bulk_insert_with_a_multi_statement_query_raises_an_exception_when_any_insert_fails`. Removing some foreign keys and thus failing afterwards in the test suite. Moreover, the `#disable_referential_integrity` method is unfortunately public, so we need it to be robust. --- .../cockroachdb/referential_integrity.rb | 72 ++++++++++--------- 1 file changed, 39 insertions(+), 33 deletions(-) diff --git a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb index a8b138ad..85abc43a 100644 --- a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb +++ b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb @@ -36,6 +36,29 @@ def check_all_foreign_keys_valid! def disable_referential_integrity foreign_keys = all_foreign_keys + remove_foreign_keys(foreign_keys) + + # Prefixes and suffixes are added in add_foreign_key + # in AR7+ so we need to temporarily disable them here, + # otherwise prefixes/suffixes will be erroneously added. + old_prefix = ActiveRecord::Base.table_name_prefix + old_suffix = ActiveRecord::Base.table_name_suffix + + begin + yield + ensure + ActiveRecord::Base.table_name_prefix = "" + ActiveRecord::Base.table_name_suffix = "" + add_foreign_keys(foreign_keys) # Never raises. + + ActiveRecord::Base.table_name_prefix = old_prefix if defined?(old_prefix) + ActiveRecord::Base.table_name_suffix = old_suffix if defined?(old_suffix) + end + end + + private + + def remove_foreign_keys(foreign_keys) statements = foreign_keys.map do |foreign_key| # We do not use the `#remove_foreign_key` method here because it # checks for foreign keys existance in the schema cache. This method @@ -46,45 +69,28 @@ def disable_referential_integrity schema_creation.accept(at) end execute_batch(statements, "Disable referential integrity -> remove foreign keys") + end - yield - - # Prefixes and suffixes are added in add_foreign_key - # in AR7+ so we need to temporarily disable them here, - # otherwise prefixes/suffixes will be erroneously added. - old_prefix = ActiveRecord::Base.table_name_prefix - old_suffix = ActiveRecord::Base.table_name_suffix + # NOTE: This method should never raise, otherwise we risk polluting table name + # prefixes and suffixes. The good thing is: if this happens, tests will crash + # hard, no way we miss it. + def add_foreign_keys(foreign_keys) + # We avoid using `foreign_key_exists?` here because it checks the schema cache + # for every key. This method is performance critical for the test suite, hence + # we use the `#all_foreign_keys` method that only make one query to the database. + already_inserted_foreign_keys = all_foreign_keys + statements = foreign_keys.map do |foreign_key| + next if already_inserted_foreign_keys.any? { |fk| fk.from_table == foreign_key.from_table && fk.options[:name] == foreign_key.options[:name] } - ActiveRecord::Base.table_name_prefix = "" - ActiveRecord::Base.table_name_suffix = "" + options = foreign_key_options(foreign_key.from_table, foreign_key.to_table, foreign_key.options) + at = create_alter_table foreign_key.from_table + at.add_foreign_key foreign_key.to_table, options - begin - # Avoid having PG:DuplicateObject error if a test is ran in transaction. - # TODO: verify that there is no cache issue related to running this (e.g: fk - # still in cache but not in db) - # - # We avoid using `foreign_key_exists?` here because it checks the schema cache - # for every key. This method is performance critical for the test suite, hence - # we use the `#all_foreign_keys` method that only make one query to the database. - already_inserted_foreign_keys = all_foreign_keys - statements = foreign_keys.map do |foreign_key| - next if already_inserted_foreign_keys.any? { |fk| fk.from_table == foreign_key.from_table && fk.options[:name] == foreign_key.options[:name] } - - options = foreign_key_options(foreign_key.from_table, foreign_key.to_table, foreign_key.options) - at = create_alter_table foreign_key.from_table - at.add_foreign_key foreign_key.to_table, options - - schema_creation.accept(at) - end - execute_batch(statements.compact, "Disable referential integrity -> add foreign keys") - ensure - ActiveRecord::Base.table_name_prefix = old_prefix - ActiveRecord::Base.table_name_suffix = old_suffix + schema_creation.accept(at) end + execute_batch(statements.compact, "Disable referential integrity -> add foreign keys") end - private - # NOTE: Copy/paste of the `#foreign_keys(table)` method adapted # to return every single foreign key in the database. def all_foreign_keys From 8d86cf200797799e6b38e1d05b530f56d2d9a32c Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Mon, 3 Nov 2025 15:36:51 +0100 Subject: [PATCH 13/27] refactor: cleanup old code - remove `debugging?` utility - use variables for `autocommit_before_ddl`: > This is the default intended approach, as it will > quote value if needed. - remove spatial_column_info.rb and its calls as it is never used. See #391 to maybe include it back, properly. - fix geos-config file fetching - refactor for loop into any? - unexclude working tests - reset `test_create_fixtures` to the original, it works. And our version would corrupt foreign keys --- Gemfile | 1 + bin/start-cockroachdb | 2 +- .../connection_adapters/cockroachdb/column.rb | 27 ++-- .../cockroachdb/schema_statements.rb | 21 --- .../cockroachdb/spatial_column_info.rb | 60 --------- .../cockroachdb_adapter.rb | 5 - test/cases/adapter_test.rb | 121 ------------------ test/cases/fixtures_test.rb | 10 -- test/cases/helper_cockroachdb.rb | 11 +- test/config.yml | 5 +- .../ActiveRecord/AdapterForeignKeyTest.rb | 3 - .../AdapterTestWithoutTransaction.rb | 2 - .../ActiveRecord/PostgresqlConnectionTest.rb | 1 - test/excludes/FixturesTest.rb | 1 - 14 files changed, 18 insertions(+), 252 deletions(-) delete mode 100644 lib/active_record/connection_adapters/cockroachdb/spatial_column_info.rb delete mode 100644 test/excludes/ActiveRecord/AdapterForeignKeyTest.rb diff --git a/Gemfile b/Gemfile index 971be35a..a93d777c 100644 --- a/Gemfile +++ b/Gemfile @@ -52,6 +52,7 @@ group :development, :test do gem "msgpack", ">= 1.7.0" gem "mutex_m", "~> 0.2.0" + gem "tracer" gem "rake" gem "debug" gem "minitest-bisect", github: "BuonOmo/minitest-bisect", branch: "main" diff --git a/bin/start-cockroachdb b/bin/start-cockroachdb index a4268c37..39fac09f 100755 --- a/bin/start-cockroachdb +++ b/bin/start-cockroachdb @@ -19,7 +19,7 @@ fi cockroach start-single-node \ --insecure --store=type=mem,size=0.25 --advertise-addr=localhost \ - --spatial-libs="$(geos-config --includes)" \ + --spatial-libs="$(geos-config --prefix)/lib" \ --pid-file "$pid_file" \ &> "$log_file" & diff --git a/lib/active_record/connection_adapters/cockroachdb/column.rb b/lib/active_record/connection_adapters/cockroachdb/column.rb index eee40652..16770c0f 100644 --- a/lib/active_record/connection_adapters/cockroachdb/column.rb +++ b/lib/active_record/connection_adapters/cockroachdb/column.rb @@ -22,25 +22,22 @@ class Column < PostgreSQL::Column # https://github.com/rgeo/activerecord-postgis-adapter/blob/master/lib/active_record/connection_adapters/postgis/spatial_column.rb def initialize(name, cast_type, default, sql_type_metadata = nil, null = true, default_function = nil, collation: nil, comment: nil, identity: nil, - serial: nil, spatial: nil, generated: nil, hidden: nil) - @sql_type_metadata = sql_type_metadata - @geographic = !!(sql_type_metadata.sql_type =~ /geography\(/i) + serial: nil, generated: nil, hidden: nil) + + super(name, cast_type, default, sql_type_metadata, null, default_function, + collation: collation, comment: comment, serial: serial, generated: generated, identity: identity) + + @geographic = sql_type_metadata.sql_type.match?(/geography\(/i) @hidden = hidden - if spatial - # This case comes from an entry in the geometry_columns table - set_geometric_type_from_name(spatial[:type]) - @srid = spatial[:srid].to_i - @has_z = !!spatial[:has_z] - @has_m = !!spatial[:has_m] - elsif @geographic + if @geographic # Geographic type information is embedded in the SQL type @srid = 4326 @has_z = @has_m = false build_from_sql_type(sql_type_metadata.sql_type) - elsif sql_type =~ /geography|geometry|point|linestring|polygon/i + elsif sql_type.match?(/geography|geometry|point|linestring|polygon/i) build_from_sql_type(sql_type_metadata.sql_type) - elsif sql_type_metadata.sql_type =~ /geography|geometry|point|linestring|polygon/i + elsif sql_type_metadata.sql_type.match?(/geography|geometry|point|linestring|polygon/i) # A geometry column with no geometry_columns entry. # @geometric_type = geo_type_from_sql_type(sql_type) build_from_sql_type(sql_type_metadata.sql_type) @@ -83,13 +80,9 @@ def serial? private - def set_geometric_type_from_name(name) - @geometric_type = RGeo::ActiveRecord.geometric_type_from_name(name) || RGeo::Feature::Geometry - end - def build_from_sql_type(sql_type) geo_type, @srid, @has_z, @has_m = OID::Spatial.parse_sql_type(sql_type) - set_geometric_type_from_name(geo_type) + @geometric_type = RGeo::ActiveRecord.geometric_type_from_name(geo_type) || RGeo::Feature::Geometry end def to_type_name(geometric_type) diff --git a/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb b/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb index d850ec04..11de1d94 100644 --- a/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb +++ b/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb @@ -112,16 +112,6 @@ def schema_names super - ["crdb_internal"] end - def add_index(table_name, column_name, **options) - super - rescue ActiveRecord::StatementInvalid => error - if debugging? && error.cause.class == PG::FeatureNotSupported - warn "#{error}\n\nThis error will be ignored and the index will not be created.\n\n" - else - raise error - end - end - # ActiveRecord allows for tables to exist without primary keys. # Databases like PostgreSQL support this behavior, but CockroachDB does # not. If a table is created without a primary key, CockroachDB will add @@ -344,7 +334,6 @@ def unique_constraints(table_name) end # OVERRIDE(v8.1.1): - # - Add spatial information # - Add hidden information def new_column_from_field(table_name, field, _definition) column_name, type, default, notnull, oid, fmod, collation, comment, identity, attgenerated, hidden = field @@ -361,9 +350,6 @@ def new_column_from_field(table_name, field, _definition) serial = sequence_name_from_parts(table_name, column_name, match[:suffix]) == match[:sequence_name] end - # {:dimension=>2, :has_m=>false, :has_z=>false, :name=>"latlon", :srid=>0, :type=>"GEOMETRY"} - spatial = spatial_column_info(table_name).get(column_name, type_metadata.sql_type) - CockroachDB::Column.new( column_name, get_oid_type(oid.to_i, fmod.to_i, column_name, type), @@ -375,7 +361,6 @@ def new_column_from_field(table_name, field, _definition) comment: comment.presence, serial: serial, identity: identity.presence, - spatial: spatial, generated: attgenerated, hidden: hidden ) @@ -417,12 +402,6 @@ def create_table_definition(*args, **kwargs) CockroachDB::TableDefinition.new(self, *args, **kwargs) end - # memoize hash of column infos for tables - def spatial_column_info(table_name) - @spatial_column_info ||= {} - @spatial_column_info[table_name.to_sym] ||= SpatialColumnInfo.new(self, table_name.to_s) - end - def create_schema_dumper(options) CockroachDB::SchemaDumper.create(self, options) end diff --git a/lib/active_record/connection_adapters/cockroachdb/spatial_column_info.rb b/lib/active_record/connection_adapters/cockroachdb/spatial_column_info.rb deleted file mode 100644 index 5e53ff02..00000000 --- a/lib/active_record/connection_adapters/cockroachdb/spatial_column_info.rb +++ /dev/null @@ -1,60 +0,0 @@ -# frozen_string_literal: true - -# Copyright 2024 The Cockroach Authors. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -module ActiveRecord - module ConnectionAdapters - module CockroachDB - class SpatialColumnInfo - def initialize(adapter, table_name) - @adapter = adapter - @table_name = table_name - end - - def all - info = @adapter.query( - "SELECT f_geometry_column,coord_dimension,srid,type FROM geometry_columns WHERE f_table_name='#{@table_name}'" - ) - result = {} - info.each do |row| - name = row[0] - type = row[3] - dimension = row[1].to_i - has_m = !!(type =~ /m$/i) - type.sub!(/m$/, '') - has_z = dimension > 3 || dimension == 3 && !has_m - result[name] = { - dimension: dimension, - has_m: has_m, - has_z: has_z, - name: name, - srid: row[2].to_i, - type: type - } - end - result - end - - # do not query the database for non-spatial columns/tables - def get(column_name, type) - return unless CockroachDBAdapter.spatial_column_options(type.to_sym) - - @spatial_column_info ||= all - @spatial_column_info[column_name] - end - end - end - end -end diff --git a/lib/active_record/connection_adapters/cockroachdb_adapter.rb b/lib/active_record/connection_adapters/cockroachdb_adapter.rb index 71183a0d..8995b8ec 100644 --- a/lib/active_record/connection_adapters/cockroachdb_adapter.rb +++ b/lib/active_record/connection_adapters/cockroachdb_adapter.rb @@ -30,7 +30,6 @@ require "active_record/connection_adapters/cockroachdb/quoting" require "active_record/connection_adapters/cockroachdb/type" require "active_record/connection_adapters/cockroachdb/column" -require "active_record/connection_adapters/cockroachdb/spatial_column_info" require "active_record/connection_adapters/cockroachdb/setup" require "active_record/connection_adapters/cockroachdb/oid/spatial" require "active_record/connection_adapters/cockroachdb/oid/interval" @@ -145,10 +144,6 @@ def srs_database_columns } end - def debugging? - !!ENV["DEBUG_COCKROACHDB_ADAPTER"] - end - def max_transaction_retries @max_transaction_retries ||= @config.fetch(:max_transaction_retries, 3) end diff --git a/test/cases/adapter_test.rb b/test/cases/adapter_test.rb index 99535ed1..866830e8 100644 --- a/test/cases/adapter_test.rb +++ b/test/cases/adapter_test.rb @@ -64,88 +64,9 @@ def test_remove_index_when_name_and_wrong_column_name_specified_positional_argum end - class AdapterForeignKeyTest < ActiveRecord::TestCase - self.use_transactional_tests = false - - fixtures :fk_test_has_pk - - def before_setup - super - conn = ActiveRecord::Base.lease_connection - - conn.drop_table :fk_test_has_fk, if_exists: true - conn.drop_table :fk_test_has_pk, if_exists: true - - conn.create_table :fk_test_has_pk, primary_key: "pk_id", force: :cascade do |t| - end - - conn.create_table :fk_test_has_fk, force: true do |t| - t.references :fk, null: false - t.foreign_key :fk_test_has_pk, column: "fk_id", name: "fk_name", primary_key: "pk_id" - end - - conn.execute "INSERT INTO fk_test_has_pk (pk_id) VALUES (1)" - end - - def setup - super - @connection = ActiveRecord::Base.lease_connection - end - - def test_foreign_key_violations_are_translated_to_specific_exception_with_validate_false - klass_has_fk = Class.new(ActiveRecord::Base) do - self.table_name = "fk_test_has_fk" - end - - error = assert_raises(ActiveRecord::InvalidForeignKey) do - has_fk = klass_has_fk.new - has_fk.fk_id = 1231231231 - has_fk.save(validate: false) - end - - assert_not_nil error.cause - end - - # This is override to prevent an intermittent error - # Table fk_test_has_pk has constrain droped and not created back - def test_foreign_key_violations_on_insert_are_translated_to_specific_exception - error = assert_raises(ActiveRecord::InvalidForeignKey) do - insert_into_fk_test_has_fk - end - - assert_not_nil error.cause - end - - # This is override to prevent an intermittent error - # Table fk_test_has_pk has constrain droped and not created back - def test_foreign_key_violations_on_delete_are_translated_to_specific_exception - insert_into_fk_test_has_fk fk_id: 1 - - error = assert_raises(ActiveRecord::InvalidForeignKey) do - @connection.execute "DELETE FROM fk_test_has_pk WHERE pk_id = 1" - end - - assert_not_nil error.cause - end - - private - - def insert_into_fk_test_has_fk(fk_id: 0) - # Oracle adapter uses prefetched primary key values from sequence and passes them to connection adapter insert method - if @connection.prefetch_primary_key? - id_value = @connection.next_sequence_value(@connection.default_sequence_name("fk_test_has_fk", "id")) - @connection.execute "INSERT INTO fk_test_has_fk (id,fk_id) VALUES (#{id_value},#{fk_id})" - else - @connection.execute "INSERT INTO fk_test_has_fk (fk_id) VALUES (#{fk_id})" - end - end - end - class AdapterTestWithoutTransaction < ActiveRecord::TestCase self.use_transactional_tests = false - fixtures :posts, :authors, :author_addresses - class Widget < ActiveRecord::Base self.primary_key = "widgetid" end @@ -156,7 +77,6 @@ def setup teardown do @connection.drop_table :widgets, if_exists: true - @connection.exec_query("DROP SEQUENCE IF EXISTS widget_seq") @connection.exec_query("DROP SEQUENCE IF EXISTS widgets_seq") end @@ -174,46 +94,5 @@ def test_reset_empty_table_with_custom_pk_sequence ") assert_equal 1, Widget.create(name: "weather").id end - - def test_truncate_tables - assert_operator Post.count, :>, 0 - assert_operator Author.count, :>, 0 - assert_operator AuthorAddress.count, :>, 0 - - @connection.truncate_tables("author_addresses", "authors", "posts") - - assert_equal 0, Post.count - assert_equal 0, Author.count - assert_equal 0, AuthorAddress.count - ensure - reset_fixtures("author_addresses", "authors", "posts") - end - - def test_truncate_tables_with_query_cache - @connection.enable_query_cache! - - assert_operator Post.count, :>, 0 - assert_operator Author.count, :>, 0 - assert_operator AuthorAddress.count, :>, 0 - - @connection.truncate_tables("author_addresses", "authors", "posts") - - assert_equal 0, Post.count - assert_equal 0, Author.count - assert_equal 0, AuthorAddress.count - ensure - reset_fixtures("author_addresses", "authors", "posts") - @connection.disable_query_cache! - end - - private - - def reset_fixtures(*fixture_names) - ActiveRecord::FixtureSet.reset_cache - - fixture_names.each do |fixture_name| - ActiveRecord::FixtureSet.create_fixtures(FIXTURES_ROOT, fixture_name) - end - end end end diff --git a/test/cases/fixtures_test.rb b/test/cases/fixtures_test.rb index 5c714242..de8418cb 100644 --- a/test/cases/fixtures_test.rb +++ b/test/cases/fixtures_test.rb @@ -122,16 +122,6 @@ def test_auto_value_on_primary_key assert_equal fixtures, result.to_a end - # This replaces the same test that's been excluded from - # FixturesTest. The test is exactly the same, but the tables - # under test will have primary key sequences, and the connection is from ActiveRecord::Base. - # Normally, the primary keys would use CockroachDB's unique_rowid(). - def test_create_fixtures - fixtures = ActiveRecord::FixtureSet.create_fixtures(FIXTURES_ROOT, "parrots") - assert Parrot.find_by_name("Curious George"), "George is not in the database" - assert fixtures.detect { |f| f.name == "parrots" }, "no fixtures named 'parrots' in #{fixtures.map(&:name).inspect}" - end - # This replaces the same test that's been excluded from # FixturesTest. The test is exactly the same, but the tables # under test will have primary key sequences, and the connection is from ActiveRecord::Base. diff --git a/test/cases/helper_cockroachdb.rb b/test/cases/helper_cockroachdb.rb index b2415b53..30af4668 100644 --- a/test/cases/helper_cockroachdb.rb +++ b/test/cases/helper_cockroachdb.rb @@ -12,9 +12,6 @@ module ExcludeMessage # some rails specific messages are then ignored. Minitest::Test.make_my_diffs_pretty! if ENV['VERBOSE'] -# Turn on debugging for the test environment -ENV['DEBUG_COCKROACHDB_ADAPTER'] = "1" - # Override the load_schema_helper for the # two ENV variables COCKROACH_LOAD_FROM_TEMPLATE # and COCKROACH_SKIP_LOAD_SCHEMA that can @@ -115,13 +112,9 @@ def run_one_method(klass, method_name, reporter) res = Minitest.run_one_method(klass, method_name) final_res ||= res - retryable = false - if res.error? - res.failures.each do |f| - retryable = true if f.message.include?("ActiveRecord::InvalidForeignKey") - end - end + retryable = res.error? && res.failures.any? { _1.message.include?("ActiveRecord::InvalidForeignKey") } (final_res = res) && break unless retryable + end # report message from first failure or from success diff --git a/test/config.yml b/test/config.yml index f554f02c..6ef37aa0 100644 --- a/test/config.yml +++ b/test/config.yml @@ -12,7 +12,10 @@ connections: # # This options keyword is referenced here in libpq: # https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNECT-OPTIONS - options: "-c autocommit_before_ddl=false" + # + # NOTE: with command lines or a URI, one could use `-c autocommit_before_ddl=false` + variables: + autocommit_before_ddl: false arunit_without_prepared_statements: <<: *arunit prepared_statements: false diff --git a/test/excludes/ActiveRecord/AdapterForeignKeyTest.rb b/test/excludes/ActiveRecord/AdapterForeignKeyTest.rb deleted file mode 100644 index 9e82adef..00000000 --- a/test/excludes/ActiveRecord/AdapterForeignKeyTest.rb +++ /dev/null @@ -1,3 +0,0 @@ -exclude :test_foreign_key_violations_are_translated_to_specific_exception_with_validate_false, "Skipping until we can triage further. See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/48" -exclude :test_foreign_key_violations_on_insert_are_translated_to_specific_exception, "This is override to prevent an intermittent error. Table fk_test_has_pk has constrain droped and not created back" -exclude :test_foreign_key_violations_on_delete_are_translated_to_specific_exception, "This is override to prevent an intermittent error. Table fk_test_has_pk has constrain droped and not created back" diff --git a/test/excludes/ActiveRecord/AdapterTestWithoutTransaction.rb b/test/excludes/ActiveRecord/AdapterTestWithoutTransaction.rb index 473369c0..9abe321c 100644 --- a/test/excludes/ActiveRecord/AdapterTestWithoutTransaction.rb +++ b/test/excludes/ActiveRecord/AdapterTestWithoutTransaction.rb @@ -1,3 +1 @@ exclude :test_reset_empty_table_with_custom_pk, "The test fails because serial primary keys in CockroachDB are created with unique_rowid() where PostgreSQL will create them with a sequence. See https://www.cockroachlabs.com/docs/v19.2/serial.html#modes-of-operation" -exclude :test_truncate_tables, "This is override to prevent an intermittent error. Table fk_test_has_pk has constrain droped and not created back" -exclude :test_truncate_tables_with_query_cache, "This is override to prevent an intermittent error. Table fk_test_has_pk has constrain droped and not created back" diff --git a/test/excludes/ActiveRecord/PostgresqlConnectionTest.rb b/test/excludes/ActiveRecord/PostgresqlConnectionTest.rb index 681eaa01..371ace24 100644 --- a/test/excludes/ActiveRecord/PostgresqlConnectionTest.rb +++ b/test/excludes/ActiveRecord/PostgresqlConnectionTest.rb @@ -1,4 +1,3 @@ -exclude :test_default_client_min_messages, "Skipping until we can triage further. See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/48" exclude :test_get_and_release_advisory_lock, "Skipping until we can triage further. See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/48" exclude :test_reconnection_after_actual_disconnection_with_verify, "Skipping until we can triage further. See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/48" exclude :test_reset, "Skipping until we can triage further. See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/48" diff --git a/test/excludes/FixturesTest.rb b/test/excludes/FixturesTest.rb index 639daaee..9353d0dc 100644 --- a/test/excludes/FixturesTest.rb +++ b/test/excludes/FixturesTest.rb @@ -2,7 +2,6 @@ exclude :test_bulk_insert_with_a_multi_statement_query_in_a_nested_transaction, "Skipping until we can triage further. See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/48" exclude :test_clean_fixtures, "Skipping the PostgreSQL test, but reimplemented for CockroachDB in test/cases/fixtures_test.rb" exclude :test_auto_value_on_primary_key, "Skipping the PostgreSQL test, but reimplemented for CockroachDB in test/cases/fixtures_test.rb" -exclude :test_create_fixtures, "Skipping the PostgreSQL test, but reimplemented for CockroachDB in test/cases/fixtures_test.rb" exclude :test_multiple_clean_fixtures, "Skipping the PostgreSQL test, but reimplemented for CockroachDB in test/cases/fixtures_test.rb" exclude :test_bulk_insert_with_a_multi_statement_query_raises_an_exception_when_any_insert_fails, "Skipping the PostgreSQL test, but reimplemented for CockroachDB in test/cases/fixtures_test.rb" exclude :test_inserts_with_pre_and_suffix, "Skipping the PostgreSQL test, but reimplemented for CockroachDB in test/cases/fixtures_test.rb" From 14f0180322552469bfb4fae33ebcc54664cdda73 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Tue, 4 Nov 2025 10:13:40 +0100 Subject: [PATCH 14/27] feat(referential-integrity): handle autocommit_before_ddl If `#disable_referential_integrity` is ran within a transaction, we may be able to still run the user's code. Otherwise we warn the user why it failed. --- .../cockroachdb/database_statements.rb | 1 - .../cockroachdb/referential_integrity.rb | 54 ++++++++++++------- .../cockroachdb/referential_integrity_test.rb | 51 ++++++++++++++++++ .../PostgreSQLReferentialIntegrityTest.rb | 18 +++++-- 4 files changed, 100 insertions(+), 24 deletions(-) create mode 100644 test/cases/adapters/cockroachdb/referential_integrity_test.rb diff --git a/lib/active_record/connection_adapters/cockroachdb/database_statements.rb b/lib/active_record/connection_adapters/cockroachdb/database_statements.rb index 100b4ca0..9a2a2312 100644 --- a/lib/active_record/connection_adapters/cockroachdb/database_statements.rb +++ b/lib/active_record/connection_adapters/cockroachdb/database_statements.rb @@ -34,7 +34,6 @@ def insert_fixtures_set(fixture_set, tables_to_delete = []) # our statements by calling `#execute` instead of `#execute_batch`. # # [1]: https://github.com/rails/rails/pull/52428 - begin # much faster without disabling referential integrity, worth trying. transaction(requires_new: true) do execute(statements, "Fixtures Load") diff --git a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb index 85abc43a..e8b818ef 100644 --- a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb +++ b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb @@ -34,25 +34,41 @@ def check_all_foreign_keys_valid! end def disable_referential_integrity - foreign_keys = all_foreign_keys - - remove_foreign_keys(foreign_keys) - - # Prefixes and suffixes are added in add_foreign_key - # in AR7+ so we need to temporarily disable them here, - # otherwise prefixes/suffixes will be erroneously added. - old_prefix = ActiveRecord::Base.table_name_prefix - old_suffix = ActiveRecord::Base.table_name_suffix - - begin - yield - ensure - ActiveRecord::Base.table_name_prefix = "" - ActiveRecord::Base.table_name_suffix = "" - add_foreign_keys(foreign_keys) # Never raises. - - ActiveRecord::Base.table_name_prefix = old_prefix if defined?(old_prefix) - ActiveRecord::Base.table_name_suffix = old_suffix if defined?(old_suffix) + if transaction_open? && query_value("SHOW autocommit_before_ddl") == "off" + begin + yield + rescue ActiveRecord::InvalidForeignKey => e + warn <<-WARNING +WARNING: Rails was not able to disable referential integrity. + +This is due to CockroachDB's need of committing transactions +before a schema change occurs. To bypass this, you can set +`autocommit_before_ddl: "on"` in your database configuration. +WARNING + raise e + end + else + foreign_keys = all_foreign_keys + + remove_foreign_keys(foreign_keys) + + # Prefixes and suffixes are added in add_foreign_key + # in AR7+ so we need to temporarily disable them here, + # otherwise prefixes/suffixes will be erroneously added. + old_prefix = ActiveRecord::Base.table_name_prefix + old_suffix = ActiveRecord::Base.table_name_suffix + + begin + yield + ensure + ActiveRecord::Base.table_name_prefix = "" + ActiveRecord::Base.table_name_suffix = "" + + add_foreign_keys(foreign_keys) # Never raises. + + ActiveRecord::Base.table_name_prefix = old_prefix if defined?(old_prefix) + ActiveRecord::Base.table_name_suffix = old_suffix if defined?(old_suffix) + end end end diff --git a/test/cases/adapters/cockroachdb/referential_integrity_test.rb b/test/cases/adapters/cockroachdb/referential_integrity_test.rb new file mode 100644 index 00000000..2b01f7e9 --- /dev/null +++ b/test/cases/adapters/cockroachdb/referential_integrity_test.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require "cases/helper_cockroachdb" +require "support/connection_helper" # for #reset_connection +require "support/copy_cat" + +class CockroachDBReferentialIntegrityTest < ActiveRecord::PostgreSQLTestCase + include ConnectionHelper + + module ProgrammerMistake + def execute_batch(sql, name = nil) + raise ArgumentError, "something is not right." if name.match?(/referential integrity/) + super + end + end + + def setup + @connection = ActiveRecord::Base.lease_connection + end + + def teardown + reset_connection + end + + exclude_from_transactional_tests :test_only_catch_active_record_errors_others_bubble_up + CopyCat.copy_methods(self, ::PostgreSQLReferentialIntegrityTest, :test_only_catch_active_record_errors_others_bubble_up) + + def test_should_reraise_invalid_foreign_key_exception_and_show_warning + warning = capture(:stderr) do + e = assert_raises(ActiveRecord::InvalidForeignKey) do + @connection.disable_referential_integrity do + @connection.execute("INSERT INTO authors (name, author_address_id) VALUES ('Mona Chollet', 42)") + end + end + assert_match (/Key \(author_address_id\)=\(42\) is not present in table/), e.message + end + assert_match (/WARNING: Rails was not able to disable referential integrity/), warning + assert_match (/autocommit_before_ddl/), warning + end + + def test_no_warning_nor_error_with_autocommit_before_ddl + @connection.execute("SET SESSION autocommit_before_ddl = 'on'") + warning = capture(:stderr) do + @connection.disable_referential_integrity do + @connection.execute("INSERT INTO authors (name, author_address_id) VALUES ('Mona Chollet', 42)") + @connection.truncate(:authors) + end + end + assert_predicate warning, :blank?, "expected no warnings but got:\n#{warning}" + end +end diff --git a/test/excludes/PostgreSQLReferentialIntegrityTest.rb b/test/excludes/PostgreSQLReferentialIntegrityTest.rb index b3c7bcb7..9565eb39 100644 --- a/test/excludes/PostgreSQLReferentialIntegrityTest.rb +++ b/test/excludes/PostgreSQLReferentialIntegrityTest.rb @@ -1,4 +1,14 @@ -exclude :test_only_catch_active_record_errors_others_bubble_up, "Skipping until we can triage further. See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/48" -exclude :test_should_reraise_invalid_foreign_key_exception_and_show_warning, "Skipping until we can triage further. See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/48" -exclude :test_does_not_break_transactions, "Skipping until we can triage further. See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/48" -exclude :test_does_not_break_nested_transactions, "Skipping until we can triage further. See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/48" +exclude :test_should_reraise_invalid_foreign_key_exception_and_show_warning, + "CockroachDB has a different limitation as there is no" \ + "'DISABLE TRIGGER' statement." + +break_tx = "CockroachDB will always alter transactions when " \ + "trying to disable referential integrity. Either it cannot " \ + "work within transaction, or autocommit_before_ddl is set " \ + "and transactions will be committed." +exclude :test_does_not_break_transactions, break_tx +exclude :test_does_not_break_nested_transactions, break_tx + +exclude :test_only_catch_active_record_errors_others_bubble_up, + "Reimplemented in test/cases/adapters/cockroachdb/referential_integrity_test.rb" \ + " to use a different trigger for the error." From 2a03ac5d98cbcf55e58c0d84b6efc7a47ba93799 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Tue, 4 Nov 2025 13:32:27 +0100 Subject: [PATCH 15/27] feat(ci): more complete error output --- .github/workflows/ci.yml | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 49158499..48534dc8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -50,16 +50,34 @@ jobs: - + + EOF jq --slurp --raw-output ' - map(.failed_tests|map("\(.klass)#\(.NAME)")) | flatten | unique | sort[0:100] | to_entries[] - | "" + map(.failed_tests) | flatten | map({ + klass, + NAME, + failure: .failures[0], + source_url: ( + .source_location | if (.[0] | contains("/gems/")) then + (.[0] | capture("rails-(?.*?)/(?.*)")) * + {line: .[1], server: "https://github.com", repo: "rails/rails"} + else + (.[0] | capture("activerecord-cockroachdb-adapter/(?test/.*)") * + {line: .[1], sha: $ENV.GITHUB_SHA, repo: $ENV.GITHUB_REPOSITORY, server: $ENV.GITHUB_SERVER_URL} + end | "\(.server)/\(.repo)/blob/\(.sha)/\(.path)#L\(.line)" + ) + }) | group_by(.) | map(.[0] * { count: length }) | sort[0:100][] + | "" + + "" + + "" + + "" + + "" ' reports/*/report.json >>$GITHUB_STEP_SUMMARY cat <>$GITHUB_STEP_SUMMARY From 3fc2227bad4b3b15c4c13a24a1bd2f7facb86f8f Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Tue, 4 Nov 2025 16:30:59 +0100 Subject: [PATCH 16/27] fix(test): test_remove_foreign_key_on_8_0 Since CockroachDB does not support DDL transactions, we adapt the test. --- .../Migration/CompatibilityTest.rb | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/test/excludes/ActiveRecord/Migration/CompatibilityTest.rb b/test/excludes/ActiveRecord/Migration/CompatibilityTest.rb index aecd68fd..8852fe68 100644 --- a/test/excludes/ActiveRecord/Migration/CompatibilityTest.rb +++ b/test/excludes/ActiveRecord/Migration/CompatibilityTest.rb @@ -17,3 +17,34 @@ def on_sym(node) insert_after(node.loc.expression, "_and_actually_way_longer_because_cockroach_is_in_the_128_game") end end + +# CockroachDB does not support DDL transactions. Hence the migration is +# not rolled back and the already removed index is not restored. +# +# From: +# if current_adapter?(:PostgreSQLAdapter, :SQLite3Adapter) +# assert_equal 2, foreign_keys.size +# else +# assert_equal 1, foreign_keys.size +# end +# To: +# assert_equal 1, foreign_keys.size +CopyCat.copy_methods(self, self, :test_remove_foreign_key_on_8_0) do + def on_if(node) + return unless node in + [:if, + [:send, nil, :current_adapter?, + [:sym, :PostgreSQLAdapter], + [:sym, :SQLite3Adapter]], + [:send, nil, :assert_equal, + [:int, 2], + [:send, + [:lvar, :foreign_keys], :size]], + [:send, nil, :assert_equal, + [:int, 1], + [:send, + [:lvar, :foreign_keys], :size]] => else_block] + + replace(node.loc.expression, else_block.location.expression.source) + end +end From 81699b3741d4f632430b40a952093f9cf4418ab0 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Tue, 4 Nov 2025 17:22:37 +0100 Subject: [PATCH 17/27] fix(test): type_map not ractor shareable --- test/excludes/ActiveRecord/AdapterTest.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/test/excludes/ActiveRecord/AdapterTest.rb b/test/excludes/ActiveRecord/AdapterTest.rb index c8c31d6e..16e1f3a4 100644 --- a/test/excludes/ActiveRecord/AdapterTest.rb +++ b/test/excludes/ActiveRecord/AdapterTest.rb @@ -1,3 +1,4 @@ exclude :test_indexes, "Rails transactional tests are being used while making schema changes. See https://www.cockroachlabs.com/docs/stable/online-schema-changes.html#limited-support-for-schema-changes-within-transactions." exclude :test_remove_index_when_name_and_wrong_column_name_specified, "Rails transactional tests are being used while making schema changes. See https://www.cockroachlabs.com/docs/stable/online-schema-changes.html#limited-support-for-schema-changes-within-transactions." exclude :test_remove_index_when_name_and_wrong_column_name_specified_positional_argument, "Rails transactional tests are being used while making schema changes. See https://www.cockroachlabs.com/docs/stable/online-schema-changes.html#limited-support-for-schema-changes-within-transactions." +exclude :test_type_map_is_ractor_shareable, "Not yet, see https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/389" From 5b50d10f43c610c14cd0e0ab2696ba83441374c9 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Mon, 17 Nov 2025 23:02:21 +0100 Subject: [PATCH 18/27] fix(test): flaky test_truncate_tables_with_query_cache This fixes a bug where our `TestRetryHelper` logic combined with `reset_fixtures` trying to reset a table without foreign keys from another table. It would first crash, removing the foreign key constraint (due to how we handle `disable_referential_integrity`). And then pass, since the foreign key constraint is gone. But we need that constraint in later tests. --- .../AdapterTestWithoutTransaction.rb | 24 +++++++++++++++++++ .../PostgreSQLReferentialIntegrityTest.rb | 2 +- test/support/copy_cat.rb | 3 +++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/test/excludes/ActiveRecord/AdapterTestWithoutTransaction.rb b/test/excludes/ActiveRecord/AdapterTestWithoutTransaction.rb index 9abe321c..23964730 100644 --- a/test/excludes/ActiveRecord/AdapterTestWithoutTransaction.rb +++ b/test/excludes/ActiveRecord/AdapterTestWithoutTransaction.rb @@ -1 +1,25 @@ exclude :test_reset_empty_table_with_custom_pk, "The test fails because serial primary keys in CockroachDB are created with unique_rowid() where PostgreSQL will create them with a sequence. See https://www.cockroachlabs.com/docs/v19.2/serial.html#modes-of-operation" + +require "support/copy_cat" + +# This fixes a bug where our `TestRetryHelper` logic combined +# with `reset_fixtures` trying to reset a table without foreign +# keys from another table. +# It would first crash, removing the foreign key constraint (due +# to how we handle `disable_referential_integrity`). And then pass, +# since the foreign key constraint is gone. But we need that +# constraint in later tests. +# +# From: +# fixture_names.each do |fixture_name| +# ActiveRecord::FixtureSet.create_fixtures(FIXTURES_ROOT, fixture_name) +# end +# To: +# ActiveRecord::FixtureSet.create_fixtures(FIXTURES_ROOT, fixture_names) +CopyCat.copy_methods(self, self, :reset_fixtures) do + def on_block(node) + return unless node in [:block, [:send, [:lvar, :fixture_names], :each], *] + + replace(node.loc.expression, "ActiveRecord::FixtureSet.create_fixtures(FIXTURES_ROOT, fixture_names)") + end +end diff --git a/test/excludes/PostgreSQLReferentialIntegrityTest.rb b/test/excludes/PostgreSQLReferentialIntegrityTest.rb index 9565eb39..262ba4b5 100644 --- a/test/excludes/PostgreSQLReferentialIntegrityTest.rb +++ b/test/excludes/PostgreSQLReferentialIntegrityTest.rb @@ -1,5 +1,5 @@ exclude :test_should_reraise_invalid_foreign_key_exception_and_show_warning, - "CockroachDB has a different limitation as there is no" \ + "CockroachDB has a different limitation as there is no " \ "'DISABLE TRIGGER' statement." break_tx = "CockroachDB will always alter transactions when " \ diff --git a/test/support/copy_cat.rb b/test/support/copy_cat.rb index 4c4ec79a..b2504214 100644 --- a/test/support/copy_cat.rb +++ b/test/support/copy_cat.rb @@ -25,6 +25,9 @@ def warn(message, category: nil, **kwargs) # Once debug is set, you can check the closest node you want to edit # and then create a method `on_` to handle it. def copy_methods(new_klass, old_klass, *methods, debug: false, &block) + if debug and not block_given? + puts "You need to provide a block to debug." + end methods.each do |met| file, _ = old_klass.instance_method(met).source_location ast = find_method(Prism::Translation::Parser.parse_file(file), met) From 271e8deee8b326e82bb264b6a21a412967a61e84 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Tue, 18 Nov 2025 14:22:20 +0100 Subject: [PATCH 19/27] fix: proper override for `Column` and `Spatial` This is WIP and there is likely more to do. Hence the reference #390. Fixes the flaky test `PostGISTest#test_spatial_factory_attrs_parsing` --- .../connection_adapters/cockroachdb/column.rb | 63 ++++++++++++++++--- .../cockroachdb/oid/spatial.rb | 14 +++++ .../cases/adapters/postgresql/postgis_test.rb | 1 - 3 files changed, 70 insertions(+), 8 deletions(-) diff --git a/lib/active_record/connection_adapters/cockroachdb/column.rb b/lib/active_record/connection_adapters/cockroachdb/column.rb index 16770c0f..bf995eb9 100644 --- a/lib/active_record/connection_adapters/cockroachdb/column.rb +++ b/lib/active_record/connection_adapters/cockroachdb/column.rb @@ -42,8 +42,7 @@ def initialize(name, cast_type, default, sql_type_metadata = nil, null = true, # @geometric_type = geo_type_from_sql_type(sql_type) build_from_sql_type(sql_type_metadata.sql_type) end - super(name, cast_type, default, sql_type_metadata, null, default_function, - collation: collation, comment: comment, serial: serial, generated: generated, identity: identity) + if spatial? && @srid @limit = { srid: @srid, type: to_type_name(geometric_type) } @limit[:has_z] = true if @has_z @@ -56,20 +55,18 @@ def initialize(name, cast_type, default, sql_type_metadata = nil, null = true, :geometric_type, :has_m, :has_z, - :srid + :srid, + :hidden alias geographic? geographic alias has_z? has_z alias has_m? has_m + alias hidden? hidden def limit spatial? ? @limit : super end - def hidden? - @hidden - end - def spatial? %i[geometry geography].include?(@sql_type_metadata.type) end @@ -78,6 +75,58 @@ def serial? default_function == 'unique_rowid()' end + # TODO: add tests (see #390) + def init_with(coder) + @geographic = coder["geographic"] + @geometric_type = coder["geometric_type"] + @has_m = coder["has_m"] + @has_z = coder["has_z"] + @srid = coder["srid"] + @hidden = coder["hidden"] + @limit = coder["limit"] + super + end + + # TODO: add tests (see #390) + def encode_with(coder) + coder["geographic"] = @geographic + coder["geometric_type"] = @geometric_type + coder["has_m"] = @has_m + coder["has_z"] = @has_z + coder["srid"] = @srid + coder["hidden"] = @hidden + coder["limit"] = @limit + super + end + + # TODO: add tests (see #390) + def ==(other) + other.is_a?(Column) && + super && + other.geographic == geographic && + other.geometric_type == geometric_type && + other.has_m == has_m && + other.has_z == has_z && + other.srid == srid && + other.hidden == hidden && + other.limit == limit + + end + alias :eql? :== + + # TODO: add tests (see #390) + def hash + Column.hash ^ + super.hash ^ + geographic.hash ^ + geometric_type.hash ^ + has_m.hash ^ + has_z.hash ^ + srid.hash ^ + hidden.hash ^ + limit.hash + end + private def build_from_sql_type(sql_type) diff --git a/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb b/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb index 740b8421..26da72a0 100644 --- a/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb +++ b/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb @@ -33,6 +33,7 @@ def initialize(oid, sql_type) factory_attrs ) end + protected attr_reader :sql_type, :spatial_factory # sql_type: geometry, geometry(Point), geometry(Point,4326), ... # @@ -89,6 +90,19 @@ def serialize(value) .generate(geo_value) end + # TODO: add tests (see #390) + def ==(other) + super && + @sql_type == other.sql_type + @spatial_factory == other.spatial_factory + end + alias eql? == + + # TODO: add tests (see #390) + def hash + super ^ [@sql_type, @spatial_factory].hash + end + private def cast_value(value) diff --git a/test/cases/adapters/postgresql/postgis_test.rb b/test/cases/adapters/postgresql/postgis_test.rb index 64508a1d..efcde465 100644 --- a/test/cases/adapters/postgresql/postgis_test.rb +++ b/test/cases/adapters/postgresql/postgis_test.rb @@ -104,7 +104,6 @@ def test_custom_factory object.save! object.reload assert_equal boundary.to_s, object.boundary.to_s - spatial_factory_store.clear end def test_spatial_factory_attrs_parsing From d8ae08e5f95181dbba387eed0b80cb1608279562 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Tue, 18 Nov 2025 17:48:31 +0100 Subject: [PATCH 20/27] fix(test): test_payload_name_on_eager_load --- .../excludes/ActiveRecord/InstrumentationTest.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 test/excludes/ActiveRecord/InstrumentationTest.rb diff --git a/test/excludes/ActiveRecord/InstrumentationTest.rb b/test/excludes/ActiveRecord/InstrumentationTest.rb new file mode 100644 index 00000000..1bf67bfb --- /dev/null +++ b/test/excludes/ActiveRecord/InstrumentationTest.rb @@ -0,0 +1,16 @@ +require "support/copy_cat" + +# We override this test since in our codebase, there is a SCHEMA call +# made with `SHOW max_identifier_length`. +# TODO: We should however inspect why that is. +# +# See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/392 +# +# From: notification.first +# To: notification.last +CopyCat.copy_methods(self, self, :test_payload_name_on_eager_load) do + def on_send(node) + return super unless node in [:send, [:lvar, :notification], :first] + replace(node.loc.expression, "notification.last") + end +end From 907fab111ef4e8abd1ae765740202c271cdba7db Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Tue, 18 Nov 2025 19:18:20 +0100 Subject: [PATCH 21/27] feat(ci): timeout for flaky.yml --- .github/workflows/flaky.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/flaky.yml b/.github/workflows/flaky.yml index 1c9e6f9c..af240333 100644 --- a/.github/workflows/flaky.yml +++ b/.github/workflows/flaky.yml @@ -44,6 +44,7 @@ jobs: ruby: ${{ steps.generate-matrix.outputs.ruby }} seeds: ${{ steps.generate-matrix.outputs.seeds }} test: + timeout-minutes: 9 runs-on: ubuntu-latest needs: prepare-matrix strategy: @@ -98,8 +99,8 @@ jobs: jq --slurp --raw-output ' sort_by(.total_time)[0:100][] - | {seed, time: .total_time | strftime("%H:%M:%S"), failure: .failed_tests[0].NAME } - | "" + | {seed, time: .total_time | strftime("%H:%M:%S"), klass: .failed_tests[0].klass, test: .failed_tests[0].NAME } + | "" ' reports/*/report.json >> $GITHUB_STEP_SUMMARY From 618ed048450f460d9bd9be9a539793f5351e4ef1 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Wed, 19 Nov 2025 09:31:33 +0100 Subject: [PATCH 22/27] fix(ci): Remove retrying logic Tests that should fail would instead corrupt the database state, causing subsequent tests to fail. This is of course related to the unfortunate way we disable referential integrity. If we see tests failing again with `InvalidForeignKey` errors, it will be a good time to reopen [cockroach#71496] [cockroach#71496]: https://github.com/cockroachdb/cockroach/issues/71496 --- test/cases/helper_cockroachdb.rb | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/test/cases/helper_cockroachdb.rb b/test/cases/helper_cockroachdb.rb index 30af4668..7afc95c3 100644 --- a/test/cases/helper_cockroachdb.rb +++ b/test/cases/helper_cockroachdb.rb @@ -99,32 +99,8 @@ def time_it end end -# Retry tests that fail due to foreign keys not always being removed synchronously -# in disable_referential_integrity, which causes foreign key errors during -# fixutre creation. -# -# Can be removed once cockroachdb/cockroach#71496 is resolved. -module TestRetryHelper - def run_one_method(klass, method_name, reporter) - reporter.prerecord(klass, method_name) - final_res = nil - 2.times do - res = Minitest.run_one_method(klass, method_name) - final_res ||= res - - retryable = res.error? && res.failures.any? { _1.message.include?("ActiveRecord::InvalidForeignKey") } - (final_res = res) && break unless retryable - - end - - # report message from first failure or from success - reporter.record(final_res) - end -end - module ActiveSupport class TestCase - extend TestRetryHelper include TestTimeoutHelper def postgis_version From 9aef11e95316d93e8af2c062c18f280c3f5da22b Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Wed, 19 Nov 2025 09:57:08 +0100 Subject: [PATCH 23/27] feat: support close_prepared --- lib/active_record/connection_adapters/cockroachdb_adapter.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/active_record/connection_adapters/cockroachdb_adapter.rb b/lib/active_record/connection_adapters/cockroachdb_adapter.rb index 8995b8ec..b38f0544 100644 --- a/lib/active_record/connection_adapters/cockroachdb_adapter.rb +++ b/lib/active_record/connection_adapters/cockroachdb_adapter.rb @@ -245,6 +245,10 @@ def supports_deferrable_constraints? false end + def supports_close_prepared? + true + end + def check_version # :nodoc: # https://www.cockroachlabs.com/docs/releases/release-support-policy if database_version < 23_01_12 # < 23.1.12 From 1770c4117be762c85e268eb868fc968da36f967e Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Wed, 19 Nov 2025 10:42:00 +0100 Subject: [PATCH 24/27] fix: make sure we eagerly compute SerializeCastValue --- lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb b/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb index 26da72a0..d83faf8d 100644 --- a/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb +++ b/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb @@ -26,6 +26,7 @@ class Spatial < Type::Value # "geometry(Polygon,4326) NOT NULL" # "geometry(Geography,4326)" def initialize(oid, sql_type) + super() @sql_type = sql_type @geo_type, @srid, @has_z, @has_m = self.class.parse_sql_type(sql_type) @spatial_factory = From ae9c973283d64592ceaef4d0834bcc5f27b20deb Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Wed, 19 Nov 2025 18:53:03 +0100 Subject: [PATCH 25/27] fix: ensure type_map gets sql type details In our adapter, we rely on the sql type string for spatial details to properly map the type to the correct ActiveRecord type. It was not shared. --- .../connection_adapters/cockroachdb/quoting.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/lib/active_record/connection_adapters/cockroachdb/quoting.rb b/lib/active_record/connection_adapters/cockroachdb/quoting.rb index cd1f86ec..3439828f 100644 --- a/lib/active_record/connection_adapters/cockroachdb/quoting.rb +++ b/lib/active_record/connection_adapters/cockroachdb/quoting.rb @@ -59,6 +59,21 @@ def quoted_date(value) # This is tested by `BasicsTest#test_default_in_local_time`. super + value.strftime("%z") end + + # NOTE: This method should be private in future rails versions. + # Hence we should also make it private then. + # + # See https://github.com/rails/rails/blob/v8.1.1/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb#L190 + def lookup_cast_type(sql_type) + type_map.lookup( + # oid + query_value("SELECT #{quote(sql_type)}::regtype::oid", "SCHEMA").to_i, + # fmod, not needed. + nil, + # details needed for `..::CockroachDB::OID::Spatial` (e.g. `geometry(point,3857)`) + sql_type + ) + end end end end From df349465f1a1bee6715b5cc5761c52dc94cd6747 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Wed, 19 Nov 2025 19:23:14 +0100 Subject: [PATCH 26/27] feat: make type_map ractor shareable Fixes #389 --- .../cockroachdb/oid/spatial.rb | 45 ++++++++++--------- test/excludes/ActiveRecord/AdapterTest.rb | 1 - 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb b/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb index d83faf8d..2b37f3e6 100644 --- a/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb +++ b/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb @@ -27,14 +27,21 @@ class Spatial < Type::Value # "geometry(Geography,4326)" def initialize(oid, sql_type) super() - @sql_type = sql_type - @geo_type, @srid, @has_z, @has_m = self.class.parse_sql_type(sql_type) - @spatial_factory = - RGeo::ActiveRecord::SpatialFactoryStore.instance.factory( - factory_attrs - ) + @sql_type = sql_type.freeze + @factory_attrs = self.class + .parse_sql_type(sql_type) + .then { |geo_type, srid, has_z, has_m| + { + geo_type: geo_type.underscore.freeze, + srid: srid.freeze, + has_z: has_z.freeze, + has_m: has_m.freeze, + sql_type: type.to_s.freeze + } + } + .freeze end - protected attr_reader :sql_type, :spatial_factory + protected attr_reader :sql_type, :factory_attrs # sql_type: geometry, geometry(Point), geometry(Point,4326), ... # @@ -66,7 +73,7 @@ def self.parse_sql_type(sql_type) end def geographic? - @sql_type =~ /geography/ + @sql_type.start_with?("geography") end def spatial? @@ -94,14 +101,14 @@ def serialize(value) # TODO: add tests (see #390) def ==(other) super && - @sql_type == other.sql_type - @spatial_factory == other.spatial_factory + @sql_type == other.sql_type && + @factory_attrs == other.factory_attrs end alias eql? == # TODO: add tests (see #390) def hash - super ^ [@sql_type, @spatial_factory].hash + super ^ [@sql_type, @factory_attrs].hash end private @@ -125,20 +132,16 @@ def binary_string?(string) def wkt_parser(string) if binary_string?(string) - RGeo::WKRep::WKBParser.new(@spatial_factory, support_ewkb: true, default_srid: @srid) + RGeo::WKRep::WKBParser.new(spatial_factory, support_ewkb: true, default_srid: @srid) else - RGeo::WKRep::WKTParser.new(@spatial_factory, support_ewkt: true, default_srid: @srid) + RGeo::WKRep::WKTParser.new(spatial_factory, support_ewkt: true, default_srid: @srid) end end - def factory_attrs - { - geo_type: @geo_type.underscore, - has_m: @has_m, - has_z: @has_z, - srid: @srid, - sql_type: type.to_s - } + def spatial_factory + RGeo::ActiveRecord::SpatialFactoryStore.instance.factory( + factory_attrs + ) end end end diff --git a/test/excludes/ActiveRecord/AdapterTest.rb b/test/excludes/ActiveRecord/AdapterTest.rb index 16e1f3a4..c8c31d6e 100644 --- a/test/excludes/ActiveRecord/AdapterTest.rb +++ b/test/excludes/ActiveRecord/AdapterTest.rb @@ -1,4 +1,3 @@ exclude :test_indexes, "Rails transactional tests are being used while making schema changes. See https://www.cockroachlabs.com/docs/stable/online-schema-changes.html#limited-support-for-schema-changes-within-transactions." exclude :test_remove_index_when_name_and_wrong_column_name_specified, "Rails transactional tests are being used while making schema changes. See https://www.cockroachlabs.com/docs/stable/online-schema-changes.html#limited-support-for-schema-changes-within-transactions." exclude :test_remove_index_when_name_and_wrong_column_name_specified_positional_argument, "Rails transactional tests are being used while making schema changes. See https://www.cockroachlabs.com/docs/stable/online-schema-changes.html#limited-support-for-schema-changes-within-transactions." -exclude :test_type_map_is_ractor_shareable, "Not yet, see https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/389" From 6d3dad0ab06c85aeefe6c9afda11ebddf2794f16 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Thu, 20 Nov 2025 15:46:14 +0100 Subject: [PATCH 27/27] fix(test): flaky encryption Minimal set of test reproduction: ```sh COCKROACH_SKIP_LOAD_SCHEMA=1 SEED=13092 bundle exec rake test TESTOPTS="--name='/\A("\ "test_concurrent_insert_with_processes"\ "|test_uniqueness_validations_work_when_using_old_encryption_schemes"\ "|test_fixtures_get_encrypted_automatically"\ "|test_ciphertext_for_returns_the_ciphertext_of_a_value_when_the_record_is_new)\z/'" ``` It is important NOT TO run `ActiveRecord::Base.reset_column_information`. Some tests are eagerly loading information to prevent side effects [^1]. We could run `Avenger.reset_column_information` but we actually do not need it here. [^1]: See https://github.com/rails/rails/blob/v8.1.1/activerecord/test/cases/encryption/helper.rb#L115-L120 --- test/cases/transactions_test.rb | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/test/cases/transactions_test.rb b/test/cases/transactions_test.rb index 6f3a39d6..a3106d6f 100644 --- a/test/cases/transactions_test.rb +++ b/test/cases/transactions_test.rb @@ -18,13 +18,18 @@ def validate_unique_username end end - def test_concurrent_insert_with_processes - conn = ActiveRecord::Base.lease_connection - conn.create_table :avengers, force: true do |t| + def setup + @conn = ActiveRecord::Base.lease_connection + @conn.create_table :avengers, force: true do |t| t.string :name end - ActiveRecord::Base.reset_column_information + end + + def teardown + @conn.drop_table :avengers, if_exists: true + end + def test_concurrent_insert_with_processes # corrupting #1 avengers = %w[Hulk Thor Loki] Avenger.cyclic_barrier = Concurrent::CyclicBarrier.new(avengers.size - 1) Thread.current[:name] = "Main" # For debug logs. @@ -41,8 +46,6 @@ def test_concurrent_insert_with_processes assert_equal avengers.size, Avenger.count ensure Thread.current[:name] = nil - conn = ActiveRecord::Base.lease_connection - conn.drop_table :avengers, if_exists: true end end end
# TestFailure
\(.key)\(.value)
\(.count)\(.klass)#\(.NAME)
\(.failure)
\(.seed)\(.time)\(.failure)
\(.seed)\(.time)\(.klass)#\(.test)