diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f81df9c6..a51e6d6d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -50,16 +50,33 @@ 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 + {path: .[0], 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 +84,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..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/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..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" 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..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/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..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/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/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..7561a95b 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,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) @@ -250,6 +356,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, @@ -295,23 +402,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 3a98cbff..1f0caa23 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 @@ -128,10 +145,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 @@ -299,7 +312,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 +391,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 +402,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 +433,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| @@ -450,7 +454,7 @@ def crdb_column_definitions(table_name) 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..561cd10a 100644 --- a/test/cases/adapters/postgresql/postgis_test.rb +++ b/test/cases/adapters/postgresql/postgis_test.rb @@ -185,12 +185,6 @@ 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) end end 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/helper_cockroachdb.rb b/test/cases/helper_cockroachdb.rb index 6ff746e3..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 @@ -180,7 +173,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/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/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" 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/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/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/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." 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