diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f81df9c6..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 | 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 @@ -67,6 +85,9 @@ jobs:
# TestFailure
\(.key)\(.value)
\(.count)\(.klass)#\(.NAME)
\(.failure)
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..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: @@ -72,7 +73,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,9 +98,9 @@ jobs: EOF jq --slurp --raw-output ' - sort_by(.total_time)[] - | {seed, time: .total_time | strftime("%H:%M:%S"), failure: .failed_tests[0].NAME } - | "\(.seed)\(.time)\(.failure)" + sort_by(.total_time)[0:100][] + | {seed, time: .total_time | strftime("%H:%M:%S"), klass: .failed_tests[0].klass, test: .failed_tests[0].NAME } + | "\(.seed)\(.time)\(.klass)#\(.test)" ' reports/*/report.json >> $GITHUB_STEP_SUMMARY @@ -107,6 +108,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/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/activerecord-cockroachdb-adapter.gemspec b/activerecord-cockroachdb-adapter.gemspec index 9492cb0e..7a372915 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" @@ -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/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 fdaab7c7..bf995eb9 100644 --- a/lib/active_record/connection_adapters/cockroachdb/column.rb +++ b/lib/active_record/connection_adapters/cockroachdb/column.rb @@ -20,33 +20,29 @@ 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 - @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) end - super(name, 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 @@ -59,20 +55,18 @@ def initialize(name, 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 @@ -81,15 +75,63 @@ def serial? default_function == 'unique_rowid()' end - private + # 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 - def set_geometric_type_from_name(name) - @geometric_type = RGeo::ActiveRecord.geometric_type_from_name(name) || RGeo::Feature::Geometry 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) 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/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/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 diff --git a/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb b/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb index eed4ffc7..2b37f3e6 100644 --- a/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb +++ b/lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb @@ -26,9 +26,22 @@ class Spatial < Type::Value # "geometry(Polygon,4326) NOT NULL" # "geometry(Geography,4326)" def initialize(oid, sql_type) - @sql_type = sql_type - @geo_type, @srid, @has_z, @has_m = self.class.parse_sql_type(sql_type) + super() + @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, :factory_attrs # sql_type: geometry, geometry(Point), geometry(Point,4326), ... # @@ -59,15 +72,8 @@ 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/ + @sql_type.start_with?("geography") end def spatial? @@ -92,6 +98,19 @@ def serialize(value) .generate(geo_value) end + # TODO: add tests (see #390) + def ==(other) + super && + @sql_type == other.sql_type && + @factory_attrs == other.factory_attrs + end + alias eql? == + + # TODO: add tests (see #390) + def hash + super ^ [@sql_type, @factory_attrs].hash + end + private def cast_value(value) @@ -108,7 +127,7 @@ 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) @@ -119,14 +138,10 @@ def wkt_parser(string) 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/lib/active_record/connection_adapters/cockroachdb/quoting.rb b/lib/active_record/connection_adapters/cockroachdb/quoting.rb index 44decd30..3439828f 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 @@ -58,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 diff --git a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb index fa4cb179..e8b818ef 100644 --- a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb +++ b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb @@ -34,8 +34,47 @@ def check_all_foreign_keys_valid! end def disable_referential_integrity - foreign_keys = all_foreign_keys + 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 + 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,47 +85,30 @@ 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 - - ActiveRecord::Base.table_name_prefix = "" - ActiveRecord::Base.table_name_suffix = "" - - 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 + # 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] } + + 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") end - 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 +121,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 +154,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 ba5f469a..11de1d94 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. @@ -27,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 @@ -55,34 +130,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 +153,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 +167,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 +175,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 +196,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 +213,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 +290,51 @@ 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 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) @@ -245,11 +350,9 @@ 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), default_value, type_metadata, !notnull, @@ -258,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 ) @@ -295,34 +397,11 @@ 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) 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 3a98cbff..b38f0544 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" @@ -111,6 +110,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 @@ -128,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 @@ -233,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 @@ -299,7 +315,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 @@ -378,18 +394,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), @@ -397,7 +405,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 @@ -428,7 +436,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| @@ -444,13 +451,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 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/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/cases/adapters/postgresql/postgis_test.rb b/test/cases/adapters/postgresql/postgis_test.rb index f3dd811d..efcde465 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 @@ -100,13 +104,9 @@ 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 - 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 +121,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 +137,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 +179,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 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 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 6ff746e3..7afc95c3 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 @@ -102,36 +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 = false - if res.error? - res.failures.each do |f| - retryable = true if f.message.include?("ActiveRecord::InvalidForeignKey") - end - end - (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 @@ -180,7 +149,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, 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 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..23964730 100644 --- a/test/excludes/ActiveRecord/AdapterTestWithoutTransaction.rb +++ b/test/excludes/ActiveRecord/AdapterTestWithoutTransaction.rb @@ -1,3 +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" -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" + +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/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 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 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 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/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 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" diff --git a/test/excludes/PostgreSQLReferentialIntegrityTest.rb b/test/excludes/PostgreSQLReferentialIntegrityTest.rb index b3c7bcb7..262ba4b5 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." 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..b2504214 100644 --- a/test/support/copy_cat.rb +++ b/test/support/copy_cat.rb @@ -21,7 +21,13 @@ 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) + 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)